Closed Bug 1477079 Opened Last year Closed Last year

Crash in nsXBLResourceLoader::StyleSheetLoaded

Categories

(Core :: CSS Parsing and Computation, defect, P1, critical)

defect

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- unaffected
firefox61 --- unaffected
firefox62 --- unaffected
firefox63 + fixed
firefox64 --- fixed

People

(Reporter: marcia, Assigned: heycam)

Details

(Keywords: crash, regression)

Crash Data

Attachments

(1 file)

This bug was filed from the Socorro interface and is
report bp-7e470007-5e67-4c4e-99c9-2a42f0180713.
=============================================================

Small volume, cross platform regression which appears to have started using  	20180710100040: https://bit.ly/2JzzE2J

Apologies if this is the wrong component.

Possible regression range based on Build ID: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=3d20b0701781731e0f9b08e1cd40ac842f385e03&tochange=70f901964f9725e6dfd09750f48996e9d6670492

Top 10 frames of crashing thread:

0 xul.dll nsXBLResourceLoader::StyleSheetLoaded dom/xbl/nsXBLResourceLoader.cpp:183
1 xul.dll mozilla::css::Loader::SheetComplete layout/style/Loader.cpp:1705
2 xul.dll static void <lambda_fb59d116f6a4f0ce69ce8dcdc439c5d4>::operator layout/style/Loader.cpp:1660
3 xul.dll void mozilla::MozPromise<bool, bool, 1>::ThenValue<<lambda_fb59d116f6a4f0ce69ce8dcdc439c5d4>, <lambda_a44192e6e5651bca308ccf0279fd0dc2> >::DoResolveOrRejectInternal xpcom/threads/MozPromise.h:774
4 xul.dll mozilla::MozPromise<bool, bool, 1>::ThenValueBase::DoResolveOrReject xpcom/threads/MozPromise.h:506
5 xul.dll mozilla::MozPromise<bool, bool, 1>::ThenValueBase::ResolveOrRejectRunnable::Run xpcom/threads/MozPromise.h:411
6 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1051
7 xul.dll NS_ProcessNextEvent xpcom/threads/nsThreadUtils.cpp:519
8 xul.dll nsThread::Shutdown xpcom/threads/nsThread.cpp:799
9 xul.dll LoadLoadableRootsTask::Run security/manager/ssl/nsNSSComponent.cpp:1020

=============================================================
We're parsing a style sheet asynchronously here:

https://hg.mozilla.org/mozilla-central/annotate/6fb4a61f6c1f617045fb2ea9618b226cfb3ce8d1/layout/style/Loader.cpp#l1652

and by the time we notify the nsXBLResourceLoader that the sheet has loaded here:

https://hg.mozilla.org/mozilla-central/annotate/6fb4a61f6c1f617045fb2ea9618b226cfb3ce8d1/dom/xbl/nsXBLResourceLoader.cpp#l183

I'm guessing that the document no longer has a pres shell.
Priority: -- → P3
I figure we should continue to do the NotifyBoundElements stuff since that will at least unblock the load event for each element with a binding.  I'm not sure what the implications would be if we left that unbalanced, and the document was displayed again later.
Assignee: nobody → cam
Status: NEW → ASSIGNED
Comment on attachment 8994078 [details]
Bug 1477079 - Skip XBL style building if document's pres shell went away.

https://reviewboard.mozilla.org/r/258668/#review265618

::: dom/xbl/nsXBLResourceLoader.cpp:187
(Diff revision 1)
> -      *mBoundDocument->GetShell()->StyleSet());
>  
> +    // Our document might have been undisplayed after this sheet load
> +    // was started, so check before building the XBL cascade data.
> +    if (nsIPresShell* shell = mBoundDocument->GetShell()) {
> +      mResources->ComputeServoStyles(*shell->StyleSet());

So, there's nothing that would build them if the page is shown again, right?

I guess it's better than crashing, but...
Attachment #8994078 - Flags: review?(emilio) → review+
Should we set some flag on nsXBLPrototypeResources to say that the next time we call GetServoStyles() we do a ComputeServoStyles() if the document has a pres shell?  (We'd need to pass the document in to GetServoStyles() I suppose.)
Flags: needinfo?(emilio)
I think I'd prefer not doing it unless we have proof that it causes correctness issues.

In particular, I suspect this can only happen when tearing stuff down. When re-creating the shell we need to reload the bindings anyway, right?
Flags: needinfo?(emilio)
Latest nightly appears fixed, will revisit next week.
Crash stats still shows 2-3 crashes per day on nightly, with the exception of August 3rd and August 5th builds.
Latest nightly shows 44 crashes/36 installations.
Let's try landing this to see what happens.
Flags: needinfo?(cam)
Hello David and Cameron - it looks as if we still didn't land anything yet. Now we are entering the 63 beta cycle, and I do see 17 crashes in this signature in the first 63 beta which is dev edition only so far. Since this is a cross platform crash it might be good to try to fix it in the 63 cycle.
Flags: needinfo?(ddurst)
this is the top browser crash in very early stability data from the rollout of 63.0b to the beta population (9% of browser crashes).
Flags: needinfo?(ddurst)
Cameron, it seems that your patch for this crash never landed and that crash is spiking in 63 Beta, could you have a look please? Thanks
Flags: needinfo?(cam)
Flags: needinfo?(cam)
Upping to P1 given prevalence of crashes in nightly and beta.
Priority: P3 → P1
Sorry for missing that needinfo before.  I'll get this landed.
Flags: needinfo?(cam)
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/194fe4b5ec09
Skip XBL style building if document's pres shell went away. r=emilio
Comment on attachment 8994078 [details]
Bug 1477079 - Skip XBL style building if document's pres shell went away.

Approval Request Comment
[Feature/Bug causing the regression]: unsure
[User impact if declined]: this is a top crash on beta
[Is this code covered by automated tests?]: not reproduced locally, so no regression test is added here
[Has the fix been verified in Nightly?]: no; patch just landed on inbound
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: -
[Is the change risky?]: no
[Why is the change risky/not risky?]: this is effectively just a null check bailout for a case where a style sheet finishes loading after the pres shell goes away, so even if this does not fix the issue (though I think it probably will) we should not be in any worse position
[String changes made/needed]: -
Attachment #8994078 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/mozilla-central/rev/194fe4b5ec09
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Comment on attachment 8994078 [details]
Bug 1477079 - Skip XBL style building if document's pres shell went away.

Fix for a spiking  crash on beta, uplift approved for 63 beta 6
Attachment #8994078 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.