Crash in mozilla::widget::WindowHook::LookupOrCreate

RESOLVED FIXED in Firefox 63

Status

()

P1
critical
RESOLVED FIXED
6 months ago
5 months ago

People

(Reporter: m_kato, Assigned: Gijs)

Tracking

({crash, regression})

Trunk
mozilla63
Unspecified
Windows 10
crash, regression
Points:
---

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox-esr60 unaffected, firefox61 unaffected, firefox62 unaffected, firefox63+ fixed)

Details

(crash signature)

Attachments

(1 attachment)

(Reporter)

Description

6 months ago
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.
Stack looks similar to Bug 1484999. Gijs is looking for some STR in that bug.
(Assignee)

Comment 2

6 months ago
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.
Flags: needinfo?(m_kato)
(Reporter)

Comment 3

6 months ago
(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.
Flags: needinfo?(m_kato)
(Assignee)

Comment 4

6 months ago
(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.
Flags: needinfo?(m_kato)
(Reporter)

Comment 5

6 months ago
(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.
Flags: needinfo?(m_kato)

Updated

6 months ago
Crash Signature: [@ mozilla::widget::WindowHook::LookupOrCreate] → [@ mozilla::widget::WindowHook::LookupOrCreate] [@ struct mozilla::widget::WindowHook::MessageData* mozilla::widget::WindowHook::LookupOrCreate]

Updated

6 months ago
Duplicate of this bug: 1484999

Updated

6 months ago
See Also: → bug 1277167

Updated

6 months ago
Priority: -- → P2
status-firefox61: --- → unaffected
status-firefox62: --- → unaffected
status-firefox-esr52: --- → unaffected
status-firefox-esr60: --- → unaffected
Keywords: regression
tracking-firefox63: --- → +
(Assignee)

Comment 7

6 months ago
(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)
Flags: needinfo?(m_kato)
Keywords: steps-wanted
(Reporter)

Comment 8

6 months ago
(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..
Flags: needinfo?(m_kato)
(Reporter)

Comment 9

6 months ago
(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.
(Reporter)

Comment 10

6 months ago
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).
(Assignee)

Comment 11

6 months ago
(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]
(Assignee)

Comment 12

6 months ago
str
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.
(Assignee)

Updated

6 months ago
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
(Assignee)

Updated

6 months ago
Keywords: steps-wanted
Priority: P2 → P1
(Assignee)

Updated

6 months ago
Blocks: 1418793
(Assignee)

Comment 13

6 months ago
Created attachment 9004919 [details]
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 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.
Attachment #9004919 - Flags: review+
(Reporter)

Comment 15

6 months ago
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.
Attachment #9004919 - Flags: review+

Comment 16

6 months ago
Pushed by gijskruitbosch@gmail.com:
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

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/12a3fa1ec069
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
status-firefox63: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
(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!
QA Whiteboard: [good first verify]
You need to log in before you can comment on or make changes to this bug.