1661485 - Crash in [@ mozilla::widget::WindowHook::AddMonitor]

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect, P2)

Description

[Julien Cristau [:jcristau]]

Crash report: https://crash-stats.mozilla.org/report/index/c728ee56-3d3e-4569-9e01-ee5260200827

Top 10 frames of crashing thread:

0 xul.dll mozilla::widget::WindowHook::AddMonitor widget/windows/WindowHook.cpp:39
1 xul.dll mozilla::widget::TaskbarPreview::TaskbarPreview widget/windows/TaskbarPreview.cpp:52
2 xul.dll mozilla::widget::TaskbarTabPreview::TaskbarTabPreview widget/windows/TaskbarTabPreview.cpp:29
3 xul.dll mozilla::widget::WinTaskbar::CreateTaskbarTabPreview widget/windows/WinTaskbar.cpp:315
4 xul.dll XPTC__InvokebyIndex 
5  @0x2ce2832bd8f 
6 xul.dll trunc 
7 xul.dll trunc 
8 xul.dll static XPCWrappedNative::CallMethod js/xpconnect/src/XPCWrappedNative.cpp:1140
9 xul.dll XPC_WN_CallMethod js/xpconnect/src/XPCWrappedNativeJSOps.cpp:946

Seeing this crash appear in early 80.0 reports, with a strong correlation to "de" (German) locale.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:jimm, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 2

[Jim Mathies [:jimm]]

Looking at the graphs, this regressed in Firefox 77 Beta for sure. It starts showing up in the first build. In nightly, it's hard to tell what range we actually regressed in, I'm guessing between April 24th, and first date the crash showed up, April 26th. Unfortunately I don't see anything obvious in the push log.

Nightly:
april 23 -> april 26

20200423145559
https://hg.mozilla.org/mozilla-central/rev/03626342f6e659ac6699a21e30423e2267c1971f
20200426094312
https://hg.mozilla.org/mozilla-central/rev/21659f178a1285e10dec0579230e52eac00d05ae

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=03626342f6e659ac6699a21e30423e2267c1971f&tochange=21659f178a1285e10dec0579230e52eac00d05ae

Comment 3

[Jim Mathies [:jimm]]

URLs are a random mix, nothing interesting there. Looks like a startup crash too.

Comment 4

[Gian-Carlo Pascutto [:gcp]]

Toshihito, perhaps we can take this bug?

Comment 5

[Makoto Kato [:m_kato]]

This may be that root case is same as bug 1277167. I guess that window is destroyed, but we cannot catch it.

Comment 6

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

This is already showing up in the Fx81 topcrash list too. Would be great if we could find a fix for this even if it's not something easily upliftable for a dot release.

Comment 7

[Toshihito Kikuchi [:toshi]]

The root cause is unclear, but the immediate reason of this crash is that the expected data was not associated with a handle of Firefox window when creating a taskbar tab preview (while opening a new tab).

I cannot find anything that can explain why the number of the crash reports was drastically increased in August. An external factor (new campaign ad in Germany for instance..?) might be causing this.

Anyway, I think we should change TaskbarPreview::GetWindowHook to stop bleeding at least in release channel. Seeing how we use WinUtils::GetNSWindowPtr in our code, we should expect the return value from GetNSWindowPtr could be null for whatever reason. If we don't get a valid property from the handle, we can gracefully fail to create a taskbar tab preview here.

Comment 8

[Toshihito Kikuchi [:toshi]]

Attachment: Bug 1661485 - Make TaskbarPreview::GetWindowHook() return a pointer. r=cmartin

Our telemetry data showed WinUtils::GetNSWindowPtr returned nullptr
when constructing TaskbarTabPreview. We can't find a way to make such
a situation, but these crashes tell us we should not assume GetNSWindowPtr
always returns a valid pointer.

A proposed fix is to move all function calls of GetWindowHook() behind
IsWindowAvailable() and to return an error value to the caller without
crash.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3e47d3cde018
Make TaskbarPreview::GetWindowHook() return a pointer.  r=cmartin

Comment 10

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/3e47d3cde018

Comment 11

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

Please nominate this for Beta and Release approval when you get a chance.

Comment 12

[Toshihito Kikuchi [:toshi]]

Comment on attachment 9177949 [details]
Bug 1661485 - Make TaskbarPreview::GetWindowHook() return a pointer. r=cmartin

Beta/Release Uplift Approval Request

• User impact if declined: The repro scenario is unknown, but the browser process crashes while opening a tab if browser.taskbar.previews.enable is on. Correlations data shows 93.58% of the crashes come from German locale.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The immediate cause is clear that a pointer to nsWindow associated with the window handle was null or the window handle was invalid when creating the taskbar preview structures. The fix is to add null check to prevent us from dereferencing null. Its regression risk is low.
• String changes made/needed:

Comment 13

[Julien Cristau [:jcristau]]

Comment on attachment 9177949 [details]
Bug 1661485 - Make TaskbarPreview::GetWindowHook() return a pointer. r=cmartin

crash fix, approved for 82.0b5

Comment 14

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-beta/rev/1c9c9b0f3a13

Comment 15

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

Comment on attachment 9177949 [details]
Bug 1661485 - Make TaskbarPreview::GetWindowHook() return a pointer. r=cmartin

Topcrash fix, approved for 81.0.1.

Comment 16

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/releases/mozilla-release/rev/63bc867a155e

Comment 17

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

This is looking good across all channels. Please nominate it for ESR78 approval when you get a chance so we can eliminate this crash from all affected branches once and for all :)

Comment 18

[Toshihito Kikuchi [:toshi]]

Comment on attachment 9177949 [details]
Bug 1661485 - Make TaskbarPreview::GetWindowHook() return a pointer. r=cmartin

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: We're getting 20~30 crash reports from ESR per week. The number is high enough to consider ESR uplift.
• User impact if declined: The repro scenario is unknown, but the browser process crashes while opening a tab if browser.taskbar.previews.enable is on.
• Fix Landed on Version: 81.0.1/82.0b5/83
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The immediate cause is clear that a pointer to nsWindow associated with the window handle was null or the window handle was invalid when creating the taskbar preview structures. The fix is to add null check to prevent us from dereferencing null. There is no regression reported since this fix was released a month ago.
• String or UUID changes made by this patch: None

Comment 19

[Julien Cristau [:jcristau]]

Comment on attachment 9177949 [details]
Bug 1661485 - Make TaskbarPreview::GetWindowHook() return a pointer. r=cmartin

crash fix for 78.5esr

Comment 20

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/42bc7761a391