Fix SharedMemory redirection queue and add telemetry events

dev/AppLifecycle/AppInstance.cpp

@@ -1,4 +1,4 @@
-// Copyright (c) Microsoft Corporation and Contributors.
+// Copyright (c) Microsoft Corporation and Contributors.
 // Licensed under the MIT License.
 
 #include <pch.h>
@@ -171,23 +171,27 @@ namespace winrt::Microsoft::Windows::AppLifecycle::implementation
     GUID AppInstance::DequeueRedirectionRequestId()
     {
         auto releaseOnExit = m_dataMutex.acquire();
+        AppLifecycleTelemetry::DequeueRedirectionRequestId();
         auto id = m_redirectionArgs.Dequeue();
         return id;
     }
 
     void AppInstance::EnqueueRedirectionRequestId(GUID id)
     {
         auto releaseOnExit = m_dataMutex.acquire();
+        AppLifecycleTelemetry::EnqueueRedirectionRequestId(id);
         m_redirectionArgs.Enqueue(id);
     }
 
     void AppInstance::ProcessRedirectionRequests()
     {
+        auto activity{ AppLifecycleTelemetry::ProcessRedirectionRequests::Start(m_processId, m_isCurrent) };
         m_innerActivated.ResetEvent();
 
         GUID id;
         while ((id = DequeueRedirectionRequestId()) != GUID_NULL)
         {
+            activity.DequeueRedirectionRequest(id);
             wil::unique_cotaskmem_string idString;
             THROW_IF_FAILED(StringFromCLSID(id, &idString));
 
@@ -199,6 +203,7 @@ namespace winrt::Microsoft::Windows::AppLifecycle::implementation
 
             // Notify the app that the redirection request is here.
             m_activatedEvent(*this, args);
+            activity.RedrectionActivatedEvent(id);
 
             std::wstring eventName = name + c_activatedEventNameSuffix;
             wil::unique_event cleanupEvent;
@@ -207,7 +212,10 @@ namespace winrt::Microsoft::Windows::AppLifecycle::implementation
                 // If the event is missing, it means the waiter gave up.  Ignore the error.
                 cleanupEvent.SetEvent();
             }
+            activity.RequestCleanupEvent(id);
         }
+
+        activity.Stop();
     }
 
     IAsyncAction AppInstance::QueueRequest(AppLifecycle::AppActivationArguments args)
@@ -229,6 +237,7 @@ namespace winrt::Microsoft::Windows::AppLifecycle::implementation
 
         GUID id;
         THROW_IF_FAILED(CoCreateGuid(&id));
+        auto activity{ AppLifecycleTelemetry::QueueRequest::Start(m_processId, m_isCurrent, id) };
 
         wil::unique_cotaskmem_string idString;
         THROW_IF_FAILED(StringFromCLSID(id, &idString));
@@ -248,9 +257,11 @@ namespace winrt::Microsoft::Windows::AppLifecycle::implementation
 
         // Signal the activation.
         m_innerActivated.SetEvent();
+        activity.InnerActivatedEvent(id);
 
         // Wait for the other instance to open the memory mapped file before exiting and cleaning our interest in it.
         cleanupEvent.wait();
+        activity.Stop();
         co_return;
     }
 
@@ -559,11 +570,13 @@ namespace winrt::Microsoft::Windows::AppLifecycle::implementation
 
     event_token AppInstance::Activated(EventHandler<Microsoft::Windows::AppLifecycle::AppActivationArguments> const& handler)
     {
+        AppLifecycleTelemetry::ActivatedEventAdd(m_processId);
         return m_activatedEvent.add(handler);
     }
 
     void AppInstance::Activated(event_token const& token) noexcept
     {
+        AppLifecycleTelemetry::ActivatedEventRemove(m_processId);
         m_activatedEvent.remove(token);
     }
 

dev/AppLifecycle/AppInstance.h

@@ -1,4 +1,4 @@
-// Copyright (c) Microsoft Corporation and Contributors.
+// Copyright (c) Microsoft Corporation and Contributors.
 // Licensed under the MIT License.
 #pragma once
 

dev/AppLifecycle/AppLifecycle.vcxitems

@@ -1,4 +1,4 @@
-<?xml version="1.0" encoding="utf-8"?>
+<?xml version="1.0" encoding="utf-8"?>
 <Project xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
   <PropertyGroup Label="Globals">
     <MSBuildAllProjects>$(MSBuildAllProjects);$(MSBuildThisFileFullPath)</MSBuildAllProjects>
@@ -40,4 +40,4 @@
     <ClInclude Include="$(MSBuildThisFileDirectory)ValueMarshaling.h" />
     <Midl Include="$(MSBuildThisFileDirectory)AppLifecycle.idl" />
   </ItemGroup>
-</Project>
+</Project>

dev/AppLifecycle/AppLifecycleTelemetry.h

@@ -1,4 +1,4 @@
-// Copyright (c) Microsoft Corporation and Contributors.
+// Copyright (c) Microsoft Corporation and Contributors.
 // Licensed under the MIT License.
 #pragma once
 
@@ -17,4 +17,42 @@ class AppLifecycleTelemetry : public wil::TraceLoggingProvider
     DEFINE_COMPLIANT_MEASURES_EVENT(RedirectActivationToAsync, PDT_ProductAndServiceUsage);
     DEFINE_COMPLIANT_MEASURES_EVENT(ActivationRegistrationManager, PDT_ProductAndServiceUsage);
     DEFINE_COMPLIANT_MEASURES_EVENT(Restart, PDT_ProductAndServiceUsage);
+
+    DEFINE_COMPLIANT_TELEMETRY_EVENT_PARAM1(ActivatedEventAdd, PDT_ProductAndServiceUsage, uint32_t, processId);
+    DEFINE_COMPLIANT_TELEMETRY_EVENT_PARAM1(ActivatedEventRemove, PDT_ProductAndServiceUsage, uint32_t, processId);
+
+    DEFINE_COMPLIANT_MEASURES_EVENT_PARAM1(EnqueueRedirectionRequestId, PDT_ProductAndServiceUsage, GUID, requestId);
+    DEFINE_COMPLIANT_MEASURES_EVENT(DequeueRedirectionRequestId, PDT_ProductAndServiceUsage);
+
+    BEGIN_COMPLIANT_MEASURES_ACTIVITY_CLASS(ProcessRedirectionRequests, PDT_ProductAndServicePerformance);
+        DEFINE_ACTIVITY_START(uint32_t processId, bool isCurrent) noexcept try
+        {
+            TraceLoggingClassWriteStart(
+                ProcessRedirectionRequests,
+                _GENERIC_PARTB_FIELDS_ENABLED,
+                TraceLoggingUInt32(processId, "processId"),
+                TraceLoggingBool(isCurrent, "isCurrent"));
+        }
+        CATCH_LOG()
+
+        DEFINE_TAGGED_COMPLIANT_MEASURES_EVENT_PARAM1(DequeueRedirectionRequest, PDT_ProductAndServicePerformance, GUID, requestId);
+        DEFINE_TAGGED_COMPLIANT_MEASURES_EVENT_PARAM1(RedrectionActivatedEvent, PDT_ProductAndServicePerformance, GUID, requestId);
+        DEFINE_TAGGED_COMPLIANT_MEASURES_EVENT_PARAM1(RequestCleanupEvent, PDT_ProductAndServicePerformance, GUID, requestId);
+    END_ACTIVITY_CLASS();
+
+    BEGIN_COMPLIANT_MEASURES_ACTIVITY_CLASS(QueueRequest, PDT_ProductAndServicePerformance)
+        DEFINE_ACTIVITY_START(uint32_t processId, bool isCurrent, GUID requestId) noexcept try
+        {
+            TraceLoggingClassWriteStart(
+                QueueRequest,
+                _GENERIC_PARTB_FIELDS_ENABLED,
+                TraceLoggingUInt32(processId, "processId"),
+                TraceLoggingBool(isCurrent, "isCurrent"),
+                TraceLoggingGuid(requestId, "requestId"));
+        }
+        CATCH_LOG()
+
+        DEFINE_TAGGED_COMPLIANT_MEASURES_EVENT_PARAM1(InnerActivatedEvent, PDT_ProductAndServicePerformance, GUID, requestId);
+    END_ACTIVITY_CLASS()
+
 };

dev/AppLifecycle/RedirectionRequestQueue.h

@@ -1,4 +1,4 @@
-// Copyright (c) Microsoft Corporation and Contributors.
+// Copyright (c) Microsoft Corporation and Contributors.
 // Licensed under the MIT License.
 #pragma once
 #include "SharedMemory.h"
@@ -7,6 +7,8 @@
 
 namespace winrt::Microsoft::Windows::AppLifecycle::implementation
 {
+    constexpr size_t c_queueSize = 4096;
+
     class RedirectionRequestQueue
     {
         struct QueueItem
@@ -16,15 +18,38 @@ namespace winrt::Microsoft::Windows::AppLifecycle::implementation
             GUID id{ 0 };
         };
 
+        // NOTE: SharedMemory layout
+        // In shared memory, we store headpointer (sizeof(size_t)) followed by c_queueSize QueueItems.
+        // But shared memory also stores the above requested size (head + queue) at the beginning of the memory
+        //      See SharedMemory.h:
+        //          Result of MapViewOfFile() is stored in m_view, which is of type DynamicSharedMemory<T>*.
+        //          DynamicSharedMemory has a "size" member at the beginning, followed by "data" member.
+        // m_data.Get() returns pointer to "data" member of above DynamicSharedMemory. This is our "usable" region i.e. head + queue.
+        // So the overall layout in memory can be represented as below:
+        // | DynamicSharedMemory.size |-------------------------- DynamicSharedMemory.data ---------------------------|
+        //
+        //                              m_data.Get() (start of head pointer storage)
+        //                              v
+        // | DynamicSharedMemory.size | head pointer | QueueItem[0] | QueueItem[1] | ... | QueueItem[c_queueSize - 1] |
+        //                                             ^
+        //                                             m_dataStart (first QueueItem in shared memory)
+
     public:
         void Init(const std::wstring& name)
         {
             m_name = name;
 
             // We store the head pointer at the beginning of the memory, and then items in the queue after.
-            m_data.Open(name, (sizeof(QueueItem) * 4096) + sizeof(QueueItem*));
-#pragma warning(suppress: 6305) // C6305: PREFast does not know m_data.Get() is compatible with "sizeof(size_t)".
-            m_dataStart = reinterpret_cast<QueueItem*>(m_data.Get() + sizeof(size_t));
+            auto headPointerSizeInBytes = sizeof(size_t);
+            auto queueSizeInBytes = sizeof(QueueItem) * c_queueSize;
+
+            auto totalSharedMemoryDataSizeInBytes = queueSizeInBytes + headPointerSizeInBytes;
+
+            m_data.Open(name, totalSharedMemoryDataSizeInBytes);
+
+            auto sharedMemoryBase = reinterpret_cast<std::byte*>(m_data.Get());
+            auto queueStartInSharedMemory = sharedMemoryBase + headPointerSizeInBytes;
+            m_dataStart = reinterpret_cast<QueueItem*>(queueStartInSharedMemory);
         }
 
         void Enqueue(const GUID& itemId)
@@ -120,23 +145,28 @@ namespace winrt::Microsoft::Windows::AppLifecycle::implementation
 
         QueueItem* AllocateItem()
         {
-            QueueItem* upperBounds = reinterpret_cast<QueueItem*>(m_data.Get()) + m_data.Size();
+            // m_dataStart points to the first QueueItem in the shared memory.
+            // The total size of the queue is c_queueSize items.
+            // m_dataStart + c_queueSize will point to one past the end of the queue.
+            // For example, if c_queueSize is 4, and m_dataStart is at address 0x22506a80010
+            // then m_dataStart + c_queueSize is at address 0x0000022506a80090.
+            // Valid items are at:
+            //    0x0000022506a80010, 0x0000022506a80030, 0x0000022506a80050, 0x0000022506a80070
+            // Since cur is of type QueueItem*, incrementing it moves by sizeof(QueueItem) in below loop.
+            QueueItem* upperBounds = m_dataStart + c_queueSize;
             auto cur = m_dataStart;
 
-#pragma warning(suppress: 6305) // C6305: PREFast does not know upperBounds was also computed in byte count so it is compatible with "sizeof(QueueItem)".
-            while (cur < (upperBounds - sizeof(QueueItem)) && cur->inUse)
+            while (cur < upperBounds && cur->inUse)
             {
-#pragma warning(suppress: 6305) // C6305: PREFast does not know cur was also computed in byte count so it is compatible with "sizeof(QueueItem)".
-                cur += sizeof(QueueItem);
+                cur++; // cur is of type QueueItem*, so ++ moves by sizeof(QueueItem).
             }
 
-#pragma warning(suppress: 6305) // C6305: PREFast does not know upperBounds was also computed in byte count so it is compatible with "sizeof(QueueItem)".
-            THROW_HR_IF(E_OUTOFMEMORY, cur >= (upperBounds - sizeof(QueueItem)));
+            THROW_HR_IF(E_OUTOFMEMORY, cur >= upperBounds);
             return cur;
         }
 
         std::wstring m_name;
-        QueueItem* m_dataStart{ nullptr };
+        QueueItem* m_dataStart{ nullptr }; // First "QueueItem" in the shared memory after the head pointer.
         SharedMemory<size_t> m_data;
     };
 }

dev/AppLifecycle/SharedMemory.h

@@ -1,4 +1,4 @@
-// Copyright (c) Microsoft Corporation and Contributors.
+// Copyright (c) Microsoft Corporation and Contributors.
 // Licensed under the MIT License.
 #pragma once
 
@@ -23,6 +23,8 @@ class SharedMemory
         if (createdFile)
         {
             Clear();
+
+            // We only store size of the request.
             m_view.get()->size = size;
         }
         else
@@ -31,6 +33,7 @@ class SharedMemory
             auto newSize = m_view.get()->size;
             m_view.reset();
             m_file.reset();
+
             OpenInternal(newSize);
         }
 
@@ -45,6 +48,7 @@ class SharedMemory
     void Clear()
     {
         // Clear only the data portion, not the size.
+        // and Size() accounts for only requested size. See comment in Open().
         memset(Get(), 0, Size());
     }
 
@@ -54,6 +58,7 @@ class SharedMemory
         Reset();
 
         m_name = name;
+
         OpenInternal(size);
         m_view.get()->size = size;
     }
@@ -88,11 +93,13 @@ class SharedMemory
 protected:
     bool OpenInternal(size_t size)
     {
-        m_file = wil::unique_handle(CreateFileMapping(INVALID_HANDLE_VALUE, nullptr, PAGE_READWRITE, 0, static_cast<DWORD>(size), m_name.c_str()));
+        // OpenInternal needs to account for "size" member of DynamicSharedMemory struct.
+        size_t totalSize = size + sizeof(size_t);
+        m_file = wil::unique_handle(CreateFileMapping(INVALID_HANDLE_VALUE, nullptr, PAGE_READWRITE, 0, static_cast<DWORD>(totalSize), m_name.c_str()));
         THROW_LAST_ERROR_IF_NULL(m_file);
 
         bool createdFile = (GetLastError() != ERROR_ALREADY_EXISTS);
-        m_view.reset(reinterpret_cast<DynamicSharedMemory<T>*>(MapViewOfFile(m_file.get(), FILE_MAP_READ | FILE_MAP_WRITE, 0, 0, size)));
+        m_view.reset(reinterpret_cast<DynamicSharedMemory<T>*>(MapViewOfFile(m_file.get(), FILE_MAP_READ | FILE_MAP_WRITE, 0, 0, totalSize)));
         THROW_LAST_ERROR_IF_NULL(m_view);
 
         return createdFile;