1458999 - Crash in nsXULWindow::SyncAttributesToWidget

Status: RESOLVED FIXED

(Core :: XUL, defect, P1)

Description

[[:philipp]]

This bug was filed from the Socorro interface and is
report bp-4da7cc5a-4ce3-4125-b6df-fed540180503.
=============================================================

Top 10 frames of crashing thread:

0 xul.dll nsXULWindow::SyncAttributesToWidget xpfe/appshell/nsXULWindow.cpp:1572
1 xul.dll nsXULWindow::OnChromeLoaded xpfe/appshell/nsXULWindow.cpp:1126
2 xul.dll nsWebShellWindow::OnStateChange xpfe/appshell/nsWebShellWindow.cpp:656
3 xul.dll nsDocLoader::DoFireOnStateChange uriloader/base/nsDocLoader.cpp:1315
4 xul.dll nsDocLoader::doStopDocumentLoad uriloader/base/nsDocLoader.cpp:869
5 xul.dll nsDocLoader::DocLoaderIsEmpty uriloader/base/nsDocLoader.cpp:747
6 xul.dll nsDocLoader::OnStopRequest uriloader/base/nsDocLoader.cpp:632
7 xul.dll mozilla::net::nsLoadGroup::RemoveRequest netwerk/base/nsLoadGroup.cpp:629
8 xul.dll nsIDocument::DoUnblockOnload dom/base/nsDocument.cpp:8422
9 xul.dll nsUnblockOnloadEvent::Run dom/base/nsDocument.cpp:8373

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

this signature is regressing in volume during the firefox 61 cycle. so far it's win10 only. the first crash was reported from 61.0a1 build 20180322220118.

perhaps related to bug 1439875, 1447875 & 1446264?

Comment 1

[Marion Daly [:mdaly]]

Find someone to own this?

Comment 2

[Mike Taylor [:miketaylr]]

Neil, do we think we should fix this for 61?

Comment 3

[Mike Taylor [:miketaylr]]

(oops)

Comment 4

[Neil Deakin]

This is very likely a crash revealed by 1439875 and friends.

I'm assuming that the call to SetNonClientMargins either in or inside the call to HideWindowChrome is deleting this or mWindow or somesuch, since it looks like it sends events and invalidates.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Thanks, Neil.

Hey emilio, assuming this is fallout from bug 1439875, do you have some cycles to investigate this?

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1458999: Nullcheck for release, assert elsewhere.

This should allow us to figure out what's killing the window mid-flight, in case
there's something to fix in the chrome JS code.

Review commit: https://reviewboard.mozilla.org/r/245872/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/245872/

Comment 7

[Neil Deakin]

Comment on attachment 8979719 [details]
Bug 1458999: Nullcheck for release, assert elsewhere.

https://reviewboard.mozilla.org/r/245872/#review251964

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bdf1c7272fcc
Nullcheck for release, assert elsewhere. r=enn

Comment 9

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Triage: regression

Comment 10

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/bdf1c7272fcc

Comment 11

[[:philipp]]

hi, could you request uplift to 61 beta in case you deem fit to do so?

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Yes, this should be uplifted, but also with the patch in bug 1463819. I'll request once both patches land.

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8979719 [details]
Bug 1458999: Nullcheck for release, assert elsewhere.

Note that this needs also bug 1463819 to remove the diagnostic assertions.

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 1463819
[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]:

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8979719 [details]
Bug 1458999: Nullcheck for release, assert elsewhere.

Fixes a crash. Approved for 61.0b10.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/96eeb9f7e318