Using UIforETW's heap tracing I looked for memory leaks related to notifications on Windows 10 17134 on an off-corp Surface Pro. I started by running UIforETW (https://tinyurl.com/uiforetw), going to Settings to put chrome.exe in the "Heap-profiled processes" field, then setting the tracing type in the main window to "Heap tracing to file". I then launched Chrome canary 71.0.3552.2, 64-bit, but any recent version should work.

I left Chrome sitting for a few minutes to give all the startup tasks time to finish.

I then browsed to https://tests.peter.sh/notification-generator/, started tracing in UIforETW (Ctrl+Win+R) and then generated twenty notifications. I waited a few seconds and then dismissed a few notifications, some individually and then all in one batch.

Loading the trace and viewing it with the attached profile I drilled down into the browser process, AIFO (Allocation Inside Freed Outside) allocations, selected the stack column, and then searched for chrome.dll!NotificationPlatformBridgeWinImpl::Display. This represents leaked memory. The allocation times were highlighted on the graph and clearly corresponded to the mouse clicks that generated the notifications.

I saw 351 allocations leaked totaling 91,519 bytes (tests on Thursday showed 1259 allocations leaked totaling 115,686 bytes - I don't know why the discrepancy).

Some of the "leaks" appear to actually be one-time allocations on the first notification so I zoomed in on the last ten. This has to be done with care because some of the allocations are actually freed later on in the trace, which can lead to artificially inflated leak counts. I don't know where the delayed frees come from or how long I have to trace to make sure I account for them - any advice?

At this point I was seeing 163 allocations leaked totaling 49,795 bytes. These are from:
157 of those are from chrome.dll!NotificationPlatformBridgeWinImpl::GetToastNotification
 155 of those are from chrome.dll!NotificationPlatformBridgeWinImpl::GetDisplayedFromActionCenter
  153 of those are from calls to wpnapps.dll!Windows::UI::Notifications::ToastNotificationHistory::GetHistoryWithId
  2 of those are from chrome.dll!NotificationPlatformBridgeWinImpl::GetIToastNotificationHistory calling chrome.dll!`anonymous namespace'::CreateActivationFactory<50,ABI::Windows::UI::Notifications::IToastNotificationManagerStatics>
 2 of those are from combase.dll!RoActivateInstance
3 of those are from wpnapps.dll!Windows::UI::Notifications::ToastNotifier::get_Setting
2 of those are from wpnapps.dll!Windows::Internal::BrokerEventSource<Windows::Foundation::ITypedEventHandler<Windows::UI::Notifications::ToastNotification * __ptr64,Windows::UI::Notifications::ToastFailedEventArgs * __ptr64> >::Add
1 of those is from chrome.dll!NotificationTemplateBuilder::Build which calls down to AddDelayedTask

I've attached three files:
1) HeapAnalysis.wpaProfile - use this with WPA to view ETW heap traces
2) 17134_leakstacks.txt - this is the call stacks discussed here, exported from WPA
3) leak_screenshot.PNG - this is a WPA screen shot showing the leaks corresponding to mouse-up events

It is important to remember that some of these allocations are freed after a non-trivial delay so some of these leak reports may be spurious. But, it does look like 17134 probably has a notification memory leak.


Note that the amount of memory leaked per notification on 17134 is much smaller (4.5 - 5.7 KB). On a system that displays 100 notifications per day for six weeks this would be about 21 MB. This is still important to fix but will barely be noticeable.

Deleted: HeapAnalysis.wpaProfile 16.6 KB

HeapAnalysis.wpaProfile 16.6 KB Download

Deleted: 17134_leakstacks.txt 4.9 KB

17134_leakstacks.txt 4.9 KB View Download

Deleted: leak_screnshot.PNG 75.9 KB

	leak_screnshot.PNG 75.9 KB View Download

Here is the call stacks for leaks recorded on my home machine yesterday. This shows a higher leak count, perhaps because the trace wasn't long enough to capture as much of the memory being freed. It will be important to understand how long the freeing can be delayed so that we don't waste time chasing memory that isn't actually leaked.

Reassigning for next steps.

Deleted: other_17134_leakstacks.txt 5.4 KB

other_17134_leakstacks.txt 5.4 KB View Download

Re #1:

>> This has to be done with care because some of the allocations are actually freed later on in the trace, which can lead to artificially inflated leak counts. I don't know where the delayed frees come from or how long I have to trace to make sure I account for them - any advice?

It's likely because we post a delayed task to delete temp image files. The delay is 12 seconds.

https://cs.chromium.org/chromium/src/chrome/browser/notifications/win/notification_image_retainer.cc?l=95-99

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3d28a29b775e4d0828b4b50267ce090a8b0793a9

commit 3d28a29b775e4d0828b4b50267ce090a8b0793a9
Author: Xi Cheng <chengx@chromium.org>
Date: Mon Sep 17 18:02:29 2018

Fix a few memory leaks in NotificationPlatformBridgeWin

We should 1) wrap the bare pointers in ComPtr; and 2) attach
QueryInterfaces to ComPtr directly.

Bug:  884224 
Change-Id: I91cb5c7312d8e24f59962ffb33faadaba36bd7ef
Reviewed-on: https://chromium-review.googlesource.com/1227493
Commit-Queue: Xi Cheng <chengx@chromium.org>
Reviewed-by: Finnur Thorarinsson <finnur@chromium.org>
Reviewed-by: Robert Liao <robliao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#591737}
[modify] https://crrev.com/3d28a29b775e4d0828b4b50267ce090a8b0793a9/chrome/browser/notifications/notification_platform_bridge_win.cc
[modify] https://crrev.com/3d28a29b775e4d0828b4b50267ce090a8b0793a9/chrome/browser/notifications/notification_platform_bridge_win.h
[modify] https://crrev.com/3d28a29b775e4d0828b4b50267ce090a8b0793a9/chrome/browser/notifications/notification_platform_bridge_win_interactive_uitest.cc
[modify] https://crrev.com/3d28a29b775e4d0828b4b50267ce090a8b0793a9/chrome/browser/notifications/notification_platform_bridge_win_unittest.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/064908b1333c5e9d0c9d84b3c928b78940d93aeb

commit 064908b1333c5e9d0c9d84b3c928b78940d93aeb
Author: Jian Li <jianli@chromium.org>
Date: Mon Sep 17 20:50:02 2018

Revert "Fix a few memory leaks in NotificationPlatformBridgeWin"

This reverts commit 3d28a29b775e4d0828b4b50267ce090a8b0793a9.

Reason for revert: 
https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/win-asan/1441

Original change's description:
> Fix a few memory leaks in NotificationPlatformBridgeWin
> 
> We should 1) wrap the bare pointers in ComPtr; and 2) attach
> QueryInterfaces to ComPtr directly.
> 
> Bug:  884224 
> Change-Id: I91cb5c7312d8e24f59962ffb33faadaba36bd7ef
> Reviewed-on: https://chromium-review.googlesource.com/1227493
> Commit-Queue: Xi Cheng <chengx@chromium.org>
> Reviewed-by: Finnur Thorarinsson <finnur@chromium.org>
> Reviewed-by: Robert Liao <robliao@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#591737}

TBR=finnur@chromium.org,robliao@chromium.org,chengx@chromium.org

Change-Id: Ib3f762c13783bc0b7bfde106d6841ddc0972daef
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  884224 
Reviewed-on: https://chromium-review.googlesource.com/1228956
Reviewed-by: Jian Li <jianli@chromium.org>
Commit-Queue: Jian Li <jianli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#591807}
[modify] https://crrev.com/064908b1333c5e9d0c9d84b3c928b78940d93aeb/chrome/browser/notifications/notification_platform_bridge_win.cc
[modify] https://crrev.com/064908b1333c5e9d0c9d84b3c928b78940d93aeb/chrome/browser/notifications/notification_platform_bridge_win.h
[modify] https://crrev.com/064908b1333c5e9d0c9d84b3c928b78940d93aeb/chrome/browser/notifications/notification_platform_bridge_win_interactive_uitest.cc
[modify] https://crrev.com/064908b1333c5e9d0c9d84b3c928b78940d93aeb/chrome/browser/notifications/notification_platform_bridge_win_unittest.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c2fbd13843b8c61c29824e260f610fa5f4254ee9

commit c2fbd13843b8c61c29824e260f610fa5f4254ee9
Author: Xi Cheng <chengx@chromium.org>
Date: Wed Sep 19 18:32:44 2018

Reland "Fix a few memory leaks in NotificationPlatformBridgeWin"

This is a reland of 3d28a29b775e4d0828b4b50267ce090a8b0793a9.

In the original change, NotificationPlatformBridgeWinTest.Suppress and
NotificationPlatformBridgeWinUITest.GetDisplayed failed in win-asan bot.

This CL fixed it.

The fix also required MockIToastNotification to derive from RuntimeClass,
which significantly simplified its implementation.

Original change's description:
> Fix a few memory leaks in NotificationPlatformBridgeWin
>
> We should 1) wrap the bare pointers in ComPtr; and 2) attach
> QueryInterfaces to ComPtr directly.
>
> Bug:  884224 
> Change-Id: I91cb5c7312d8e24f59962ffb33faadaba36bd7ef
> Reviewed-on: https://chromium-review.googlesource.com/1227493
> Commit-Queue: Xi Cheng <chengx@chromium.org>
> Reviewed-by: Finnur Thorarinsson <finnur@chromium.org>
> Reviewed-by: Robert Liao <robliao@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#591737}

Bug:  884224 ,  886957 
Change-Id: If2674299437f04f637f6b2b8278b40a4dbf7b9d0
Reviewed-on: https://chromium-review.googlesource.com/1228991
Commit-Queue: Xi Cheng <chengx@chromium.org>
Reviewed-by: Finnur Thorarinsson <finnur@chromium.org>
Cr-Commit-Position: refs/heads/master@{#592487}
[modify] https://crrev.com/c2fbd13843b8c61c29824e260f610fa5f4254ee9/chrome/browser/notifications/notification_platform_bridge_win.cc
[modify] https://crrev.com/c2fbd13843b8c61c29824e260f610fa5f4254ee9/chrome/browser/notifications/notification_platform_bridge_win.h
[modify] https://crrev.com/c2fbd13843b8c61c29824e260f610fa5f4254ee9/chrome/browser/notifications/notification_platform_bridge_win_interactive_uitest.cc
[modify] https://crrev.com/c2fbd13843b8c61c29824e260f610fa5f4254ee9/chrome/browser/notifications/notification_platform_bridge_win_unittest.cc
[modify] https://crrev.com/c2fbd13843b8c61c29824e260f610fa5f4254ee9/chrome/browser/notifications/win/mock_itoastnotification.cc
[modify] https://crrev.com/c2fbd13843b8c61c29824e260f610fa5f4254ee9/chrome/browser/notifications/win/mock_itoastnotification.h

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3f3cb513f33eb1ea9ee84462e8a4fb47ab4f438e

commit 3f3cb513f33eb1ea9ee84462e8a4fb47ab4f438e
Author: Xi Cheng <chengx@chromium.org>
Date: Thu Sep 20 11:41:41 2018

Use WeakPtr for NotificationImageRetainer

This makes NotificationTemplateBuilder own a WeakPtr rather than
a raw pointer to NotificationImageRetainer, which helps with tracking
potential memory issues.

Bug:  884224 
Change-Id: I229ffd187ccd9b424987ab1473b240f898c06023
Reviewed-on: https://chromium-review.googlesource.com/1227717
Reviewed-by: Finnur Thorarinsson <finnur@chromium.org>
Commit-Queue: Xi Cheng <chengx@chromium.org>
Cr-Commit-Position: refs/heads/master@{#592759}
[modify] https://crrev.com/3f3cb513f33eb1ea9ee84462e8a4fb47ab4f438e/chrome/browser/notifications/notification_platform_bridge_win.cc
[modify] https://crrev.com/3f3cb513f33eb1ea9ee84462e8a4fb47ab4f438e/chrome/browser/notifications/notification_platform_bridge_win_unittest.cc
[modify] https://crrev.com/3f3cb513f33eb1ea9ee84462e8a4fb47ab4f438e/chrome/browser/notifications/win/notification_image_retainer.h
[modify] https://crrev.com/3f3cb513f33eb1ea9ee84462e8a4fb47ab4f438e/chrome/browser/notifications/win/notification_template_builder.cc
[modify] https://crrev.com/3f3cb513f33eb1ea9ee84462e8a4fb47ab4f438e/chrome/browser/notifications/win/notification_template_builder.h
[modify] https://crrev.com/3f3cb513f33eb1ea9ee84462e8a4fb47ab4f438e/chrome/browser/notifications/win/notification_template_builder_unittest.cc

The leak from the calls to wpnapps.dll!Windows::UI::Notifications::ToastNotificationHistory::GetHistoryWithId as mentioned in comment #1 has been fixed by the CL in comment #8.

I followed the steps mentioned in comment #1 to capture another ETW heap trace for Canary which contained the fix. The trace is attached below.

Before the fix: 68,288 Bytes were leaked. (This can be found in leak_screnshot.PNG attached in comment #1)
After the fix: 976 Bytes were leaked. (Note that this is a one-time allocation on the first notification. Therefore it seems to be required by the OS and will be freed later.)

Deleted: GetHistoryWithId_leak_fixed.JPG 277 KB

	GetHistoryWithId_leak_fixed.JPG 277 KB View Download

Following the last comment:

From GetHistoryWithId_leak_fixed.JPG above, we can see chrome.dll!NotificationImageRetainer::RegisterTemporaryImage caused 8,192 Bytes memory leak.

In the trace, I displayed and dismissed 20 notifications, so there should be 20 RegisterTemporaryImage() function calls. Each call will post a base::DeleteFile task to delete an image file in a 12-sec delayed manner. The leaks were the file paths of these images to delete. 

Following comment 11:

Expand the heap trace a little more to highlight the potential leak in chrome.dll!NotificationImageRetainer::RegisterTemporaryImage.

The leak consists of: 1) the file path data packaged by base::Bind; and 2) the memory allocated in PostDelayTask method.

The only possible explanation I can think of is that these 20 tasks didn't get executed for some reason before tracing was stopped. Tracing was stopped ~27 (57-20) seconds after the last task was posted. Since these tasks should be executed after a 12 sec delay, it would be very surprising that they didn't get executed. 

Deleted: RegisterTemporaryImage_potential_memory_leak.JPG 311 KB

	RegisterTemporaryImage_potential_memory_leak.JPG 311 KB View Download

What happens if you lower the timeout (just while testing)?

I just captured another ETW heap trace from the latest Canary (71.0.3569.0) following exactly the same steps. This time, I checked the image folder and made sure all the temp files got deleted before stopping tracing. The indicates the tasks were all executed. 

From the trace attached, I don't see the memory leak as mentioned in #16 anymore. The AIFO allocations under RegisterTemporaryImage were all one-time allocations and expected.

I don't understand why the trace I captured last time showed that leak but I am glad I don't see it now.

Deleted: RegisterTemporaryImage_potential_memory_leak_update.JPG 381 KB

	RegisterTemporaryImage_potential_memory_leak_update.JPG 381 KB View Download

I think we can close this bug now.