Closed Bug 1463819 Opened 2 years ago Closed 2 years ago

Crash in nsXULWindow::Destroy

Categories

(Core :: XUL, defect)

Unspecified
Windows 10
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- unaffected
firefox60 --- unaffected
firefox61 --- fixed
firefox62 --- fixed

People

(Reporter: calixte, Assigned: emilio)

References

(Blocks 1 open bug)

Details

(Keywords: crash, regression)

Crash Data

Attachments

(1 file)

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)
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)
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)
Attachment #8980729 - Flags: review?(enndeakin)
Assignee: nobody → emilio
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?
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.
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.
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 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+
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
https://hg.mozilla.org/mozilla-central/rev/21dcddb9bc73
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
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)
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.