1485253 - Crash in mozilla::widget::WindowHook::LookupOrCreate

Status: RESOLVED FIXED

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

Description

[Makoto Kato [:m_kato]]

When closing tab, my nightly is crashed by the following stack.


This bug was filed from the Socorro interface and is
report bp-9ee63d03-0e8d-464c-b310-daefb0180822.
=============================================================

Top 10 frames of crashing thread:

0 xul.dll struct mozilla::widget::WindowHook::MessageData* mozilla::widget::WindowHook::LookupOrCreate widget/windows/WindowHook.cpp:77
1 xul.dll mozilla::widget::WindowHook::AddMonitor widget/windows/WindowHook.cpp:45
2 xul.dll mozilla::widget::TaskbarPreview::Enable widget/windows/TaskbarPreview.cpp:173
3 xul.dll mozilla::widget::TaskbarTabPreview::Enable widget/windows/TaskbarTabPreview.cpp:237
4 xul.dll XPTC__InvokebyIndex xpcom/reflect/xptcall/md/win32/xptcinvoke_asm_x86_64.asm:97
5 xul.dll exp2 
6 xul.dll exp2 
7 xul.dll exp2 
8 xul.dll XPCWrappedNative::CallMethod js/xpconnect/src/XPCWrappedNative.cpp:1201
9 xul.dll static bool XPC_WN_GetterSetter js/xpconnect/src/XPCWrappedNativeJSOps.cpp:923

=============================================================


0:000> k 5
Child-SP          RetAddr           Call Site
000000d6`1a1f61f0 00007ff8`a8944050 xul!mozilla::widget::WindowHook::LookupOrCreate+0x8 [z:\build\build\src\widget\windows\WindowHook.cpp @ 77]
000000d6`1a1f6230 00007ff8`a894410b xul!mozilla::widget::WindowHook::AddMonitor+0x20 [z:\build\build\src\widget\windows\WindowHook.cpp @ 46]
000000d6`1a1f6280 00007ff8`ab793c7c xul!mozilla::widget::TaskbarPreview::Enable+0x4b [z:\build\build\src\widget\windows\TaskbarPreview.cpp @ 175]
000000d6`1a1f62c0 00007ff8`ac754782 xul!mozilla::widget::TaskbarTabPreview::Enable+0xfc [z:\build\build\src\widget\windows\TaskbarTabPreview.cpp @ 237]
000000d6`1a1f63a0 00007ff8`a83b6c9d xul!XPTC__InvokebyIndex+0x72 [z:\build\build\src\xpcom\reflect\xptcall\md\win32\xptcinvoke_asm_x86_64.asm @ 99]

0:000> x
@rcx              this = 0x00000000`000002c8
@edi              nMsg = 0xc0d5
<unavailable>     data = <value unavailable>

mozilla::widget::TaskbarPreview::Enable doesn't check whether window is still alive.  So we should add check it.

Comment 1

[Marcia Knous [:marcia]]

Stack looks similar to Bug 1484999. Gijs is looking for some STR in that bug.

Comment 2

[:Gijs (he/him)]

As Marcia said, if you have STR, and/or some explanation of how my patch in bug 1418793 would have caused this, that'd be really helpful.

Comment 3

[Makoto Kato [:m_kato]]

(In reply to :Gijs (he/him) from comment #2)
> As Marcia said, if you have STR, and/or some explanation of how my patch in
> bug 1418793 would have caused this, that'd be really helpful.

My cause is that closing tab causes this crash.   I think that "preview.visible = true" is called in this crash, but I don't know whether preview.visible is setting true when closing tab.  (Closing tab will set visible property to false.)

Is TabWindow.enabled = true called at this timing? But I don't know...

If crash isn't fixed, https://searchfox.org/mozilla-central/rev/3fa761ade83ed0b8ab463acb057c2cf0b104689e/widget/windows/TaskbarPreview.cpp#104 should use IsWindowAvailable  instead of nullptr check as workaround.

Comment 4

[:Gijs (he/him)]

(In reply to Makoto Kato [:m_kato] from comment #3)
> (In reply to :Gijs (he/him) from comment #2)
> > As Marcia said, if you have STR, and/or some explanation of how my patch in
> > bug 1418793 would have caused this, that'd be really helpful.
> 
> My cause is that closing tab causes this crash.

Sorry, so, closing any tab causes this crash? I can't reproduce that... Any more detailed steps that allow me to repro would be really helpful...

>   I think that
> "preview.visible = true" is called in this crash, but I don't know whether
> preview.visible is setting true when closing tab.  (Closing tab will set
> visible property to false.)

I'm guessing we set preview.visible on some other tab? Maybe it only happens once you have more tabs than the maximum number of per-tab previews we do? But unsure how we end up with tab previews in our list where the window is dead, just from opening/closing tabs in a window that stays open...

> Is TabWindow.enabled = true called at this timing? But I don't know...
> 
> If crash isn't fixed,
> https://searchfox.org/mozilla-central/rev/
> 3fa761ade83ed0b8ab463acb057c2cf0b104689e/widget/windows/TaskbarPreview.
> cpp#104 should use IsWindowAvailable  instead of nullptr check as workaround.

I mean, I'd be fine with that, but it does feel a bit wallpaper-y.

Comment 5

[Makoto Kato [:m_kato]]

(In reply to :Gijs (he/him) from comment #4)
> (In reply to Makoto Kato [:m_kato] from comment #3)
> > (In reply to :Gijs (he/him) from comment #2)
> > > As Marcia said, if you have STR, and/or some explanation of how my patch in
> > > bug 1418793 would have caused this, that'd be really helpful.
> > 
> > My cause is that closing tab causes this crash.
> 
> Sorry, so, closing any tab causes this crash? I can't reproduce that... Any
> more detailed steps that allow me to repro would be really helpful...

I don't know exactly reproduce step.  But when this crash occurs (including today), I clicked close button [x] of tab.  But I cannot reproduce when clicking close button only.

> >   I think that
> > "preview.visible = true" is called in this crash, but I don't know whether
> > preview.visible is setting true when closing tab.  (Closing tab will set
> > visible property to false.)
> 
> I'm guessing we set preview.visible on some other tab? Maybe it only happens
> once you have more tabs than the maximum number of per-tab previews we do?
> But unsure how we end up with tab previews in our list where the window is
> dead, just from opening/closing tabs in a window that stays open...

This crash stack will call Enable() on my minidump.  Since this isn't XPCOM method, I think that visible setter is called from script.
 
> > Is TabWindow.enabled = true called at this timing? But I don't know...
> > 
> > If crash isn't fixed,
> > https://searchfox.org/mozilla-central/rev/
> > 3fa761ade83ed0b8ab463acb057c2cf0b104689e/widget/windows/TaskbarPreview.
> > cpp#104 should use IsWindowAvailable  instead of nullptr check as workaround.
> 
> I mean, I'd be fine with that, but it does feel a bit wallpaper-y.

Comment 7

[:Gijs (he/him)]

(In reply to Makoto Kato [:m_kato] from comment #5)
> (In reply to :Gijs (he/him) from comment #4)
> > (In reply to Makoto Kato [:m_kato] from comment #3)
> > > (In reply to :Gijs (he/him) from comment #2)
> > > > As Marcia said, if you have STR, and/or some explanation of how my patch in
> > > > bug 1418793 would have caused this, that'd be really helpful.
> > > 
> > > My cause is that closing tab causes this crash.
> > 
> > Sorry, so, closing any tab causes this crash? I can't reproduce that... Any
> > more detailed steps that allow me to repro would be really helpful...
> 
> I don't know exactly reproduce step.  But when this crash occurs (including
> today), I clicked close button [x] of tab.  But I cannot reproduce when
> clicking close button only.

Close button on the tab in the window, or in the taskbar, or something else?

Any chance you can catch this in the debugger and get a JS stack?



As it is, I would really like to fix this, as apparently I may have regressed this, but without STR or a clear idea of how my patch is causing this to happen (more frequently, per bug 1277167?), I'm really not sure how. (As noted, just adding wallpaper-y nullchecks would maybe work, but would feel like there'd still be an interaction we don't fully understand)

Comment 8

[Makoto Kato [:m_kato]]

(In reply to :Gijs (he/him) from comment #7)
> (In reply to Makoto Kato [:m_kato] from comment #5)
> > (In reply to :Gijs (he/him) from comment #4)
> > > (In reply to Makoto Kato [:m_kato] from comment #3)
> > > > (In reply to :Gijs (he/him) from comment #2)
> > > > > As Marcia said, if you have STR, and/or some explanation of how my patch in
> > > > > bug 1418793 would have caused this, that'd be really helpful.
> > > > 
> > > > My cause is that closing tab causes this crash.
> > > 
> > > Sorry, so, closing any tab causes this crash? I can't reproduce that... Any
> > > more detailed steps that allow me to repro would be really helpful...
> > 
> > I don't know exactly reproduce step.  But when this crash occurs (including
> > today), I clicked close button [x] of tab.  But I cannot reproduce when
> > clicking close button only.
> 
> Close button on the tab in the window, or in the taskbar, or something else?

The tab close button of Firefox window..

Comment 9

[Makoto Kato [:m_kato]]

(In reply to :Gijs (he/him) from comment #7)
> Any chance you can catch this in the debugger and get a JS stack?

Maybe, call stack is TabWindow.removeTab -> AeroPeek.removePreview -> AeroPeek.checkPreviewCount -> AeroPeek.enabled -> TabWindow.enabled -> preview.visible.  I don't know why _AeroPeek.enabled is false at this time.

Comment 10

[Makoto Kato [:m_kato]]

I guess that tab count is important.  open tab is 20+ and open any sites each tab, then, when close some tab, preview.visible is called even if closing tab.  (when remained tab is 20, preview.visible sets true on closing tab).

Comment 11

[:Gijs (he/him)]

(In reply to Makoto Kato [:m_kato] (PTO 9/2 - 9/9) from comment #10)
> I guess that tab count is important.  open tab is 20+ and open any sites
> each tab, then, when close some tab, preview.visible is called even if
> closing tab.  (when remained tab is 20, preview.visible sets true on closing
> tab).

Ahhh. Now I can reproduce.

JS stack:

0 set enabled(enable = true) ["resource:///modules/WindowsPreviewPerTab.jsm":464]
    this = [object Object]
1 set enabled/<(win = [object Object], 0, [object Object],[object Object]) ["resource:///modules/WindowsPreviewPerTab.jsm":693]
2 forEach(callbackfn = function(win) {
      win.enabled = enable;
    }) ["self-hosted":268]
    this = [object Object],[object Object]
3 set enabled(enable = true) ["resource:///modules/WindowsPreviewPerTab.jsm":692]
    this = [object Object]
4 checkPreviewCount() ["resource:///modules/WindowsPreviewPerTab.jsm":765]
    this = [object Object]
5 removePreview(preview = [xpconnect wrapped nsITaskbarTabPreview]) ["resource:///modules/WindowsPreviewPerTab.jsm":758]
    this = [object Object]
6 removeTab(tab = [object XULElement]) ["resource:///modules/WindowsPreviewPerTab.jsm":450]
    this = [object Object]
7 handleEvent(evt = [object CustomEvent]) ["resource:///modules/WindowsPreviewPerTab.jsm":504]
    this = [object Object]
8 _beginRemoveTab(aTab = [object XULElement], aAdoptedByTab = null, aCloseWindowWithLastTab = null, aCloseWindowFastpath = true, aSkipPermitUnload = undefined) ["chrome://browser/content/tabbrowser.js":2893]
    this = [object Object]
9 removeTab(aTab = [object XULElement], (destructured parameter) = [object Object]) ["chrome://browser/content/tabbrowser.js":2729]
    this = [object Object]
10 removeCurrentTab(aParams = [object Object]) ["chrome://browser/content/tabbrowser.js":2694]
    this = [object Object]
11 BrowserCloseTabOrWindow(event = [object XULCommandEvent]) ["chrome://browser/content/browser.js":2365]
    this = [object ChromeWindow]
12 oncommand(event = [object XULCommandEvent]) ["chrome://browser/content/browser.xul":1]
    this = [object XULElement]

Comment 12

[:Gijs (he/him)]

STR:

0. open Firefox
1. in the options, enable "Show tab previews in the Windows taskbar"
2. open 21 tabs in total in that window by hitting ctrl-t repeatedly
3. open a new window
4. close the new window
5. close tabs in the original window

ER:
no crash

AR:
crash as soon as you get to <20 tabs.


This is the default value of browser.taskbar.previews.max, you could also set it to a lower number and then you could reproduce with that number of tabs.

Comment 13

[:Gijs (he/him)]

Attachment: Bug 1485253 - fix crashes due to dead windows' taskbar preview objects sticking around, r?florian,m_kato

The fix in bug 1418793 accidentally removed the onCloseWindow call, which
meant we were setting `.enabled` on taskbar window objects whose windows
were already dead, which was causing crashes.

I've reverted the removal, and also added some nullchecking, because the
C++ component shouldn't make it this easy for consumers to cause crashes.

Comment 14

[Florian Quèze [:florian]]

Comment on attachment 9004919 [details]
Bug 1485253 - fix crashes due to dead windows' taskbar preview objects sticking around, r?florian,m_kato

Florian Quèze [:florian] has approved the revision.

Comment 15

[Makoto Kato [:m_kato]]

Comment on attachment 9004919 [details]
Bug 1485253 - fix crashes due to dead windows' taskbar preview objects sticking around, r?florian,m_kato

Makoto Kato [:m_kato] (PTO 9/2 - 9/9) has approved the revision.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/12a3fa1ec069
fix crashes due to dead windows' taskbar preview objects sticking around, r=m_kato,florian

Comment 17

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/12a3fa1ec069

Comment 18

[Cristian Fogel [:cfogel]]

(In reply to :Gijs (he/him) from comment #12)
> STR:
> 
> 0. open Firefox
> 1. in the options, enable "Show tab previews in the Windows taskbar"
> 2. open 21 tabs in total in that window by hitting ctrl-t repeatedly
> 3. open a new window
> 4. close the new window
> 5. close tabs in the original window
> 
> ER:
> no crash
> 
> AR:
> crash as soon as you get to <20 tabs.
> 
> 
> This is the default value of browser.taskbar.previews.max, you could also
> set it to a lower number and then you could reproduce with that number of
> tabs.

Marking as GFV, thank you for the STR!