1516423 - Crash in mozilla::widget::TaskbarPreview::MainWindowHook. Tray related?

Status: VERIFIED FIXED

(Thunderbird :: General, defect)

Description

[Wayne Mery (:wsmwk)]

I crashed on resume from sleep.  Looking at crash-stats, this only seems to happen in the Windows world.

bp-03c4a4f2-c5ad-4922-a546-408c30181226.
0 xul.dll mozilla::widget::TaskbarPreview::MainWindowHook widget/windows/TaskbarPreview.cpp:417
1 xul.dll mozilla::widget::WindowHook::CallbackData::Invoke widget/windows/WindowHook.cpp:124
2 xul.dll mozilla::widget::WindowHook::Notify widget/windows/WindowHook.cpp:111
3 xul.dll nsWindow::ExternalHandlerProcessMessage widget/windows/nsWindow.cpp:5106
4 xul.dll nsWindow::ProcessMessage widget/windows/nsWindow.cpp:5159
5 xul.dll nsWindow::WindowProcInternal widget/windows/nsWindow.cpp:5010
6 xul.dll CallWindowProcCrashProtected xpcom/base/nsCrashOnException.cpp:32
7 xul.dll nsWindow::WindowProc widget/windows/nsWindow.cpp:4962
8 user32.dll user32.dll@0x162f9 
9 user32.dll user32.dll@0x16d39 

bp-6f02f042-e24e-45bb-915a-c16940181024 is a Firefox crash

related to bug 1277167?

Comment 1

[Wayne Mery (:wsmwk)]

something has changed - crash rate increased 10x with 78.3.2

https://crash-stats.mozilla.org/signature/?signature=mozilla%3A%3Awidget%3A%3ATaskbarPreview%3A%3AMainWindowHook&date=%3E%3D2020-10-03T05%3A46%3A00.000Z&date=%3C2020-10-17T05%3A46%3A00.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_columns=install_time&_columns=startup_crash&_sort=-date&page=1#graphs

some examples bp-9cf72fa5-6c86-4bd0-92b1-764f30201017 bp-585078fe-a1e4-4812-a7c2-5cbad0201017 bp-2f13b8ab-d765-42ce-b63b-22ea40201017

Comment 2

[Makoto Kato [:m_kato]]

Kikuchi-san has fixed related issue (bug 1661485). Wayne, does this still occurs after landing bug 1661485?

Comment 3

[Toshihito Kikuchi [:toshi]]

Interesting. Most of the recent instances of bug 1661485 were from German locale, but this case is not. This is from Thunderbird, and user comments say this happened when double-clicking a taskbar icon of a minimized Thunderbird. We should check that scenario.

Comment 4

[Wayne Mery (:wsmwk)]

(In reply to Makoto Kato [:m_kato] from comment #2)

Kikuchi-san has fixed related issue (bug 1661485). Wayne, does this still occurs after landing bug 1661485?

Short answer is yes, still occurs. bp-e547a495-bbca-4040-9716-52a5c0201024 is 83 beta.

Magnus, is this related to another issue we have with tray?

Comment 5

[Magnus Melin [:mkmelin]]

I suppose it's possible (maybe even likely?) it could be related to reports we have of new mail tray icons sometimes not disappearing as they should.

Comment 6

[Wayne Mery (:wsmwk)]

Any further ideas about cause and regression?

To recap:

• TB 78.3.2 is buildid 20201006011220, which predates core bug 1661485 (fixed in firefox esr 78.5.0)
• Not seeing Thunderbird checkins that would have regressed this
  ** 78.3.2 https://hg.mozilla.org/releases/comm-esr78/pushloghtml?fromchange=THUNDERBIRD_78_3_1_RELEASE&tochange=6ee0e0ff64f4ccda9c478426d0b5d15cd583f8e6&full=1
  ** 78.3.1 (just the one crasher) https://hg.mozilla.org/releases/comm-esr78/pushloghtml?fromchange=THUNDERBIRD_78_3_0_RELEASE&tochange=00912779d73faef4277c57fe99aa5259b11d244d&full=1
  ** 78.3.0 https://hg.mozilla.org/releases/comm-esr78/pushloghtml?fromchange=THUNDERBIRD_78_2_2_RELEASE&tochange=35e647ebd4fa09407a0fe151d3823b1b58172a2a&full=1
• TB 78.3.0 never shipped, but 78.3.1 was live for a week before 78.3.2 shipped
• tray support shipped in 78.0 (so not new in version 78.3.x)
• We fixed a crasher in 78.3.0, but the fix predates this bug Bug 1645724 - Crash in [@ IconWindowProc] related to Windows minimize to tray
• Lots of locales https://crash-stats.mozilla.org/search/?signature=%3Dmozilla%3A%3Awidget%3A%3ATaskbarPreview%3A%3AMainWindowHook&product=Thunderbird&date=%3E%3D2021-03-08T12%3A13%3A00.000Z&date=%3C2021-03-22T12%3A13%3A00.000Z&_facets=signature&_facets=useragent_locale&_sort=-date&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#facet-useragent_locale

#29 crash for 78.8.1

Beta crashes were steady for 84 and 85. Then, strange, not a single 86 or 87 beta crash. And, it's now a top crash for 86 and 88 nightly.
https://crash-stats.mozilla.org/signature/?product=Thunderbird&signature=mozilla%3A%3Awidget%3A%3ATaskbarPreview%3A%3AMainWindowHook&date=%3E%3D2020-12-22T12%3A05%3A00.000Z&date=%3C2021-03-22T12%3A05%3A00.000Z#summary

Comment 7

[Ben Campbell]

My notes from looking at the crash stack and poking about (and knowing nothing about the taskbar or windows integration!):

The crash occurs in the static fn TaskbarPreview::MainWindowHook(), and I think it's being passed a borked aContext pointer - dereferencing it causes a EXCEPTION_ACCESS_VIOLATION_READ. It's not null.

The context is actually the TaskbarPreview object - it's set up previously in TaskBarPreview::Enable(), like so:

    hook->AddMonitor(nsAppShell::GetTaskbarButtonCreatedMessage(),
                     MainWindowHook, this);

(there's another AddMonitor() call in TaskbarPreview::Init() for the WM_DESTROY message, but that's not the one that's causing this crash).

So. The window receives a "TaskbarButtonCreated" message, invokes TaskbarPreview::MainWindowHook(), but by then the TaskbarPreview is borked (or at least what the hook thinks is the TaskbarPreview).

Maybe the TaskbarPreview is being deleted before the message is processed? (there is a message queue, after all)
Or maybe something is corrupting the hook's context pointer?

Looking at nsITaskbarPreview, it seems that it's for both the taskbar preview (hovering over the app in the task bar at the bottom of the screen?) and window preview (eg cycling through windows with the alt-tab or whatever?)...

Comment 8

[Toshihito Kikuchi [:toshi]]

I think I got the nearly stable repro. Setting mail.minimizeToTray = true, keep minimizing a window and reverting it by clicking a tray icon caused this crash. I'll spend more time on figuring out the root cause.

https://crash-stats.mozilla.org/report/index/7836651a-519e-4aa0-b4b0-bdbbb0210323
https://crash-stats.mozilla.org/report/index/60fc7124-aa51-4720-85a5-da77e0210323

Comment 9

[Toshihito Kikuchi [:toshi]]

Attachment: bug1516423-change-WinUnreadBadge.patch

Comment 10

[Toshihito Kikuchi [:toshi]]

I think the problem was TaskbarWindowPreview was freed a few seconds after TB started. It happens because nsWindow holds mTaskbarPreview as a weak reference. I changed it to a strong reference like this. The crash was gone, but not sure what's the impact on Firefox side.

Another approach I can think of is to keep a reference of TaskbarWindowPreview inside WinUnreadBadge (see attached). I confirmed this also fixed the crash and the risk is lower because there is no impact on Firefox. Magnus, let me add needinfo on you because you reviewed the latest WinUnreadBadge (bug 715799). What do you think?

Comment 11

[Toshihito Kikuchi [:toshi]]

(In reply to Magnus Melin [:mkmelin] from comment #5)

I suppose it's possible (maybe even likely?) it could be related to reports we have of new mail tray icons sometimes not disappearing as they should.

While investigating, I think I hit an opposite issue. When mail.minimizeToTray = true is set, if I minimize a TB window without any unread email, the tray icon should be always shown to bring back the window. However, the tray icon disappeared first time I minimized a window, and after that there was no way to bring back the window (so I needed to kill the process). I think it's a different bug.

Comment 12

[Toshihito Kikuchi [:toshi]]

Regardless of whether we change comm-central or convert a weak to a strong ref, I think there is a bug in TaskbarWindowPreview. The reason why we have a dangling pointer in the queue is TaskbarWindowPreview's dtor does not clean up a hook. On the other hand, a sibling class TaskbarTabPreview does it by calling Disable(). I checked all other pairs of AddHook/RemoveHook and AddMonitor/RemoveMonitor, but that's the only one left.

Comment 13

[Toshihito Kikuchi [:toshi]]

Attachment: Bug 1516423 - Clean up MessageData holding TaskbarWindowPreview when it's destroyed. r=cmartin

When TaskbarWindowPreview is constructed, it registers its adress in WindowHook
as a listener. However, when an instance of TaskbarWindowPreview is destroyed,
it does not remove all listeners. This emerges as Thunderbird's crash because TB
creates TaskbarWindowPreview to show a tray icon but the instance is accidentally
freed when the icon is still active due to another bug. As a result, TB crashes
when TaskbarPreview::MainWindowHook tries to access a freed TaskbarWindowPreview
to handle a click on the icon.

This patch adds a call to SetVisible(false) in TaskbarWindowPreview's dtor to
clean up the things. This will stop the crash in TB, but TB's taskbar icon still
does not behave correctly because it accidentally disappears without crash.
We need an additional patch to fix it.

Comment 15

[Magnus Melin [:mkmelin]]

(In reply to Toshihito Kikuchi [:toshi] from comment #10)

Another approach I can think of is to keep a reference of TaskbarWindowPreview inside WinUnreadBadge (see attached). I confirmed this also fixed the crash and the risk is lower because there is no impact on Firefox. Magnus, let me add needinfo on you because you reviewed the latest WinUnreadBadge (bug 715799). What do you think?

I'd trust your judgement on which approach to prefer (or do both, I'd imagine the Thunderbird part could be good to take in addition to be sure?)

Comment 16

[Toshihito Kikuchi [:toshi]]

Attachment: Bug 1516423 - Have WinUnreadBadge own TaskbarWindowPreview. r=mkmelin

In Thunderbird, an instance of TaskbarWindowPreview is created when
WinUnreadBadge.updateUnreadCount calls WinTaskbar::GetOverlayIconController.
However, that instance is freed immdiately after updateUnreadCount exits
because nobody owns it. As a result, the taskbar icon disappears even when
mail.minimizeToTray = false.

This patch makes WinUnreadBadge own a reference to TaskbarWindowPreview
so that its instance is kept alive after the method exits.
This approach is the same as Firefox stores TaskbarWindowPreview as
DownloadsTaskbar._taskbarProgress.

Comment 17

[Toshihito Kikuchi [:toshi]]

(In reply to Magnus Melin [:mkmelin] from comment #15)

(In reply to Toshihito Kikuchi [:toshi] from comment #10)
I'd trust your judgement on which approach to prefer (or do both, I'd imagine the Thunderbird part could be good to take in addition to be sure?)

Thanks! Yes, I'll land both patches for mozilla-central and comm-central. I've just uploaded a new patch for comm-central with comments.

I'll also file a new bug for the taskbar icon issue mentioned in comment 11.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/d783ea4ee651
Have WinUnreadBadge own TaskbarWindowPreview. r=mkmelin

Comment 20

[Ali Savas]

What are the chances that the fix will be integrated in 88 Beta 2? I really can't work with this version. Would not like to switch to the stable version.

Comment 21

[Wayne Mery (:wsmwk)]

(In reply to Ali Savas from comment #20)

What are the chances that the fix will be integrated in 88 Beta 2?

zero.

If you are able to start thunderbird, disable minimize to tray in tools > options.
If you are not able to start thunderbird, you'll need to edit prefs.js and create a preference line if it doesn't already exist for mail.minimizeToTray with a value of false.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e818608c2b9d
Clean up MessageData holding TaskbarWindowPreview when it's destroyed. r=cmartin

Comment 23

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/e818608c2b9d

Comment 24

[Wayne Mery (:wsmwk)]

I can say that crashes on thunderbird nightly channel have stopped. The last build to crash is 20210329105238, and the graph reflects this ... https://crash-stats.mozilla.org/signature/?product=Thunderbird&release_channel=nightly&signature=mozilla%3A%3Awidget%3A%3ATaskbarPreview%3A%3AMainWindowHook&date=%3E%3D2021-03-08T13%3A38%3A00.000Z&date=%3C2021-04-08T13%3A38%3A00.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_columns=install_time&_columns=startup_crash&_sort=-build_id&_sort=-date&page=1#graphs

Comment 25

[Magnus Melin [:mkmelin]]

Enough code changes to the effected files, that not worth uplifting to 78.