Crash in nsXULWindow::Destroy

RESOLVED FIXED in Firefox 61

Status

()

defect
--
critical
RESOLVED FIXED
11 months ago
11 months ago

People

(Reporter: calixte, Assigned: emilio)

Tracking

(Blocks 1 bug, {crash, regression})

Trunk
mozilla62
Unspecified
Windows 10
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox-esr60 unaffected, firefox60 unaffected, firefox61 fixed, firefox62 fixed)

Details

(crash signature)

Attachments

(1 attachment)

(Reporter)

Description

11 months ago
This bug was filed from the Socorro interface and is
report bp-8d99d04e-8a53-4f11-b547-d287f0180523.
=============================================================

Top 10 frames of crashing thread:

0 xul.dll nsXULWindow::Destroy xpfe/appshell/nsXULWindow.cpp:453
1 xul.dll nsWebShellWindow::Destroy xpfe/appshell/nsWebShellWindow.cpp:776
2 xul.dll nsWebShellWindow::RequestWindowClose xpfe/appshell/nsWebShellWindow.cpp:336
3 xul.dll nsWebShellWindow::WidgetListenerDelegate::RequestWindowClose xpfe/appshell/nsWebShellWindow.cpp:811
4 xul.dll nsWindow::ProcessMessage widget/windows/nsWindow.cpp:5544
5 xul.dll nsWindow::WindowProcInternal widget/windows/nsWindow.cpp:5031
6 xul.dll CallWindowProcCrashProtected xpcom/base/nsCrashOnException.cpp:32
7 xul.dll nsWindow::WindowProc widget/windows/nsWindow.cpp:4983
8 user32.dll UserCallWinProcCheckWow 
9 user32.dll DispatchClientMessage 

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

There is 1 crash in nightly 62 with buildid 20180523100147. In analyzing the backtrace, the regression may have been introduced by patch [1] to fix bug 1458999.

[1] https://hg.mozilla.org/mozilla-central/rev?node=bdf1c7272fcc
Flags: needinfo?(emilio)
(Assignee)

Comment 1

11 months ago
Jim, is this stack expected? (SetNonClientMargins closing the window?). AFAIK this only happens on Windows.

If so, we can remove this assertion and just carry on with the null-checks added in bug 1458999. Otherwise this is probably a Widget / Win32 bug.
Flags: needinfo?(emilio) → needinfo?(jmathies)

Comment 2

11 months ago
Looks like it. The SHAppBarMessage isn't really at fault here, it's an api we call to query for information about taskbar state. It is triggering delivery of non-queued windowing events, which is common. During this, Windows delivers a sync message to a Firefox window procedure indicating something closed that window (WM_CLOSE). Not sure if this is the same window associated the SyncAttributesToWidget() call, but looks like it due to your assert.

Something is closing this window very early, Windows is trying to make that happen and your assert gets triggers in the middle because we're doing initial layout on a Windows that's about to get destroyed. All of the crashes thus far seem to happen early in startup (under 15 seconds). Hope this helps.
Flags: needinfo?(jmathies)
(Assignee)

Updated

11 months ago
Attachment #8980729 - Flags: review?(enndeakin)
Assignee: nobody → emilio

Comment 4

11 months ago
mozreview-review
Comment on attachment 8980729 [details]
Bug 1463819: Account for the possibility of SyncAttributesToWidget destroying the window.

https://reviewboard.mozilla.org/r/246888/#review253514

::: xpfe/appshell/nsXULWindow.cpp:1124
(Diff revision 1)
>  
>    if (NS_SUCCEEDED(rv)) {
>      mChromeLoaded = true;
>      ApplyChromeFlags();
>      SyncAttributesToWidget();
> +    if (mWindow) {

Destroy has been called, but have we guaranteed that 'this' is still around?
(Assignee)

Comment 5

11 months ago
mozreview-review-reply
Comment on attachment 8980729 [details]
Bug 1463819: Account for the possibility of SyncAttributesToWidget destroying the window.

https://reviewboard.mozilla.org/r/246888/#review253514

> Destroy has been called, but have we guaranteed that 'this' is still around?

Yes, this is always called with a strong reference on the stack.
(Assignee)

Comment 6

11 months ago
mozreview-review-reply
Comment on attachment 8980729 [details]
Bug 1463819: Account for the possibility of SyncAttributesToWidget destroying the window.

https://reviewboard.mozilla.org/r/246888/#review253514

> Yes, this is always called with a strong reference on the stack.

Hmm, let's sure about OnChromeLoaded though... Let me audit a bit or I'll kungfudeathgrip it.
(Assignee)

Comment 7

11 months ago
mozreview-review-reply
Comment on attachment 8980729 [details]
Bug 1463819: Account for the possibility of SyncAttributesToWidget destroying the window.

https://reviewboard.mozilla.org/r/246888/#review253514

> Hmm, let's sure about OnChromeLoaded though... Let me audit a bit or I'll kungfudeathgrip it.

Well, in fairness, it'd better be the case, since we were poking at the members afterwards already, so I think this should be good to go.

Comment 8

11 months ago
mozreview-review
Comment on attachment 8980729 [details]
Bug 1463819: Account for the possibility of SyncAttributesToWidget destroying the window.

https://reviewboard.mozilla.org/r/246888/#review253540
Attachment #8980729 - Flags: review?(enndeakin) → review+

Comment 9

11 months ago
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/21dcddb9bc73
Account for the possibility of SyncAttributesToWidget destroying the window. r=enn

Comment 10

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/21dcddb9bc73
Status: NEW → RESOLVED
Last Resolved: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
(Assignee)

Comment 11

11 months ago
Comment on attachment 8980729 [details]
Bug 1463819: Account for the possibility of SyncAttributesToWidget destroying the window.

Approval Request Comment
[Feature/Bug causing the regression]: pre-existing bug exposed by bug 1439875 and friends.
[User impact if declined]: crashes, mostly on startup on windows.
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: pretty sure it fixes the crashes seen in bug 1458999 and in this bug, though we can wait for a few days to see crash rates if you want.
[Needs manual test from QE? If yes, steps to reproduce]: 
[List of other uplifts needed for the feature/fix]: bug 1458999
[Is the change risky?]: not risky
[Why is the change risky/not risky?]: only more null-checks to handle a very rare situation.
[String changes made/needed]:
Attachment #8980729 - Flags: approval-mozilla-beta?
Did you forget to request approval on bug 1458999 too? :)
Flags: needinfo?(emilio)
(Assignee)

Comment 13

11 months ago
Maybe... :)
Flags: needinfo?(emilio)
Comment on attachment 8980729 [details]
Bug 1463819: Account for the possibility of SyncAttributesToWidget destroying the window.

Needed for bug 1458999. Not a ton of Nightly crash data to go off so far, but initial results are encouraging. Approved for 61.0b10.
Attachment #8980729 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.