Introducing unpackaged foreground activation for PushNotifications by pmpurifoy · Pull Request #1336 · microsoft/WindowsAppSDK

pmpurifoy · 2021-08-28T21:50:39Z

This PR allows unpackaged applications to declare foreground handlers for their requested PushNotificationChannel.

jonwis

Few changes, nothing major.

… into WNP_LRP_FS

Co-authored-by: Daniel Ayala <14967941+danielayala94@users.noreply.github.com>

hulumane · 2021-09-01T17:50:51Z

+        BOOL foregroundHandled = true;
+        if (FAILED(it->second->InvokeAll(payloadSize, payload.data(), &foregroundHandled)))
+        {
+            Remove(processName);


Deadlock here. You are already in an exclusivelock and the same thread tries to acquire another exclusive lock

hulumane · 2021-09-01T17:52:37Z

+
+    HRESULT UnregisterForegroundActivator([in] LPCWSTR processName);
+
+    HRESULT SendBackgroundNotification([in] LPCWSTR processName, [in] ULONG length, [in, size_is(length)] byte* data);


Please remove from idl. No longer being used anywhere.

hulumane · 2021-09-01T17:59:57Z

+        {
+            auto lock = m_lock.lock_exclusive();
+            m_foregroundHandlers.remove(token);
+            if (!--m_foregroundHandlerCount)


Nit: Don't rely on int to bool conversion as a practise. Always better to say != 0 to be precise.

hulumane · 2021-09-01T18:02:38Z

+    m_foregroundMap.erase(processName);
+}
+
+bool ForegroundSinkManager::InvokeForegroundHandlers(std::wstring const& processName, winrt::com_array<uint8_t> const& payload, ULONG const& payloadSize)


Who calls this method? Is there a pipe from the LongRunningSink that will be calling into this as a seperate PR?

hulumane · 2021-09-01T18:03:19Z

+    return S_OK;
+}
+
+STDMETHODIMP_(HRESULT __stdcall) NotificationsLongRunningPlatformImpl::SendBackgroundNotification(_In_ PCWSTR processName, _In_ ULONG payloadSize, _In_ byte* payload)


Please Remove unused APIs

hulumane · 2021-09-01T18:08:06Z

+        else
+        {
+            auto lock = m_lock.lock_exclusive();
+            if (!m_foregroundHandlerCount++)


Nit: From a clarity perspective, prefer that we don't rely on int to bool conversion. Better to say != 0 or > 0 and convert it into the bool equivalent.

hulumane · 2021-09-01T18:12:59Z


+STDMETHODIMP_(HRESULT __stdcall) NotificationsLongRunningPlatformImpl::RegisterForegroundActivator(_In_ IWpnForegroundSink* sink, _In_ PCWSTR processName)
+{
+    RETURN_HR_IF(WPN_E_PLATFORM_UNAVAILABLE, m_shutdown);


Unfortunately m_shutdown is not being protected by the lock. So you can run into race conditions. @danielayala94 What is the recommended pattern here?

hulumane · 2021-09-01T18:13:51Z

+
+STDMETHODIMP_(HRESULT __stdcall) NotificationsLongRunningPlatformImpl::UnregisterForegroundActivator(_In_ PCWSTR processName)
+{
+    RETURN_HR_IF(WPN_E_PLATFORM_UNAVAILABLE, m_shutdown);


Same race issue with shutdown. Not being protected by the lock. @danielayala94

* Original skeleton * Corrected mismatches against PowerNotifications change * Correcting few nits * Added constants usage, std::call_once and improved abstraction * Added StartupTask exe * Fixed some bugs * Mostly renaming and cleaning * Removed timer externals and let winmain to own Platform lifetime * Correction -> factory should own the platform * Addressed lifetime crashes and other issues * Improved timer/event management * Addressed feedback * Fixed compilation issues in other flavor+archs and added minor comments * Added a unit test (more coming!) * Test enhancements * Addressed comments * Moved timer/event logic to a platform component * Install Frameworkudk for LRP (#1271) * Introducing unpackaged foreground activation for PushNotifications (#1336) * Cleaned original code * Added IWpnNotificationSink COM interface to LRP MSIX * Just stuff for testing. Needs to be undone * Still working on it * Com proxy objects not being stored correctly * Receiving Data from LRP and changed sink container * Foreground activation working * Fix first round of nits * Still working on adding nits * Fix following FI of main * Addressing nits * Use com_array instead of byte* * Update dev/PushNotifications/PushNotificationDummyDeferral.h Co-authored-by: Daniel Ayala <14967941+danielayala94@users.noreply.github.com> Co-authored-by: Daniel Ayala <joayalag@microsoft.com> Co-authored-by: Eric Langlois <erlangl@microsoft.com> Co-authored-by: Daniel Ayala <14967941+danielayala94@users.noreply.github.com> * Not everything should be renamed Runtime * Ensuring the LRP msix gets built after the framework (#1356) Co-authored-by: Eric Langlois <erlangl@microsoft.com> * Bringing build management settings for release/arm64 in line with debug/arm64 * Fixing arm64 build * Addressed comments * Using winrt constructs on NotificationsLongRunningPlatformImpl :) * Channel Request for unpackaged applications (#1290) * Channel requests for Packaged and Unpackaged applications * Fix comments * Update windowsappsdk_dll to locate PushNotificationsLongRunningTask.ProxyStub headers * Address minor comments * Fix channel.Close API to handle channels when requested through frameworkudk * Address comments and introduce GetAppIdentifier in LRP * Addressing nits * Fixing a few more nits * Update externs.h * Store <processname,appidentifier> pair in windows storage localsettings * Fix minor nits * Remove helpers.h references from PushNotificationsDemoApp * Remove unused APIs * Using WIL type for CoInitializeEx instead of working too hard Co-authored-by: Eric Langlois <erlangl@microsoft.com> Co-authored-by: Paul Purifoy <purifoypaul@microsoft.com> Co-authored-by: Daniel Ayala <14967941+danielayala94@users.noreply.github.com> * Notification delivery + Protocol Activation (#1358) * Protocol Activation skeleton * Further impl, mostly on the LRP * Integration with Foreground logic and some optimizations * Added calls to the LRP from SDK, and LRP improvements * Fixed some issues * Improved payload deserializing * Fixed merging goofs * Addressed feedback + few optimizations * Further improvements and added/removed comments * Addressed build errors after merging * Nit: Removed comment Co-authored-by: Daniel Ayala Co-authored-by: Eric Langlois * Unpackaged notification unit tests (#1340) * Adding initial unpackaged test series * This function now fails silently * This is not expected to work for unpackaged apps Co-authored-by: Eric Langlois <erlangl@microsoft.com> * Ensure errors get logged with telemetry * Workaround UDK include issue (#1364) * Workaround UDK include issue * just duplicating files seem a better approach * Restoring vcproje files * PushNotificationsRT.h was interfering with package creation Co-authored-by: Eric Langlois <erlangl@microsoft.com> * Some nits * Addressed comments * Fixed mistake - Removed comments * Add LRP binaries to CopyFilestoStagingDir * Addressed comments * Merge main to LRP * Telemetry Update for Unpackaged Apps (#1385) * Adding support for unpackaged apps * Using inline statics to cache data * Code cleanup * Code cleanup * Simplifying rethrow code * Addressing feedback * Distinguishing between legacy and modern impl * caching legacy status to it is also available when the call fails * Adding const to better express the intent * Code cleanup * Code cleanup * PR feedback Co-authored-by: Eric Langlois <erlangl@microsoft.com> * Update to latest UDK and undo UDK hack (#1420) Co-authored-by: Eric Langlois <erlangl@microsoft.com> * Fixing WNP_LRP tests (#1401) * Working on tests * Call ReigsterFullTrust in RegisterLongRunningActivator * Removed g_isPackaged with IsActivatorSupported * Addressing nits * Protocol Activation bug fixes (#1406) This PR fixes a couple of issues: - Truncated payloads delivered to the app: the Push payload is now processed correctly to comply with AppLifecycle command line expectations (i.e. truncation happened at the first space character - other activation types are already escaping such characters, now we do it too. Then, the payload is unescaped at GetRawNotificationEventArgs.h). - Payloads with special characters crashed the app: Special characters weren't playing okay with wstring->string conversion at PushNotificationReceivedEventArgs, because WideCharToMultiByte was erroring out with ERROR_INSUFFICIENT_BUFFER. * Use remoteId as const value (#1435) * Upgrade LRP to latest UDK (#1459) * Upgrade LRP to latest UDK * Missed a few checks in the vcproj Co-authored-by: Eric Langlois <erlangl@microsoft.com> * fix bad merge * Update CopyFilesToStagingDir.ps1 * Remove PushNotifications from outer layer of JSON * Adding source link to projects not in main yet (#1515) Co-authored-by: Eric Langlois <erlangl@microsoft.com> * Support foregroundtask registration for packaged applications (#1521) * Support foregroundtask registration for packaged applications * Address comments * Address comments2 * Introduce PushNotificationsUtility.h/cpp * Address comments * Mark PushNotificationUtility.h file to be shared across the PushNotifications directory * Add try/catch around onRawNotificationRecveived * Set foregroundhandled to true by default * Address comments for getappusermodelId API * Address nits * Adress one more nit Co-authored-by: Venkata Sharath Chandra Manchala <vemancha@microsoft.com> * Fix for regression in protocol activation in AppLifecycle (#1558) * Hack to address AppLifecycle regression introduced in main * Code cleanup * Code cleanup * Addressing PR feedback * Adding bare bone unit test to avoid future regression Co-authored-by: Eric Langlois <erlangl@microsoft.com> * LRP E2E Scenario fixes (#1543) * LRP fixes * In progress * Revert "In progress" This reverts commit d5d021a. * Add fixes to E2E testing * Address nits * Added nits from other PR and fixed PushNotificationUtiilty * Use string converter with payload length * Update APITests.cpp * Include AppLifecycle fix for PushArgs * Revert "Include AppLifecycle fix for PushArgs" This reverts commit 33994da. * Addressing nits * Add Verify.h to demo app for helper function * Added TAEF dependencies to Demo/TestApp Co-authored-by: Daniel Ayala <14967941+danielayala94@users.noreply.github.com> * Updating LRP to use latest FrameworkUDK (#1587) Co-authored-by: Eric Langlois <erlangl@microsoft.com> Co-authored-by: Daniel Ayala <joayalag@microsoft.com> Co-authored-by: Sharath Manchala <10109130+sharath2727@users.noreply.github.com> Co-authored-by: Eric Langlois <erlangl@microsoft.com> Co-authored-by: Paul Purifoy <33183370+pmpurifoy@users.noreply.github.com> Co-authored-by: eric langlois <email@ericlanglois.com> Co-authored-by: Paul Purifoy <purifoypaul@microsoft.com> Co-authored-by: Venkata Sharath Chandra Manchala <vemancha@microsoft.com>

Daniel Ayala and others added 7 commits August 24, 2021 16:09

Cleaned original code

085c2a4

Added IWpnNotificationSink COM interface to LRP MSIX

4e3b33b

Just stuff for testing. Needs to be undone

9e92b46

Still working on it

aa8f821

Com proxy objects not being stored correctly

0869fa5

Receiving Data from LRP and changed sink container

9628910

Foreground activation working

383801c

ghost added the needs-triage label Aug 28, 2021

pmpurifoy requested review from DrusTheAxe, EHO-makai, danielayala94, hulumane, jonwis, loneursid and sharath2727 August 28, 2021 21:50

pmpurifoy changed the base branch from main to WNP_LRP August 28, 2021 22:13

danielayala94 requested changes Aug 30, 2021

View reviewed changes