1477079 - Crash in nsXBLResourceLoader::StyleSheetLoaded

Status: RESOLVED FIXED

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

Description

[Marcia Knous [:marcia]]

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

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

Comment 1

[Cameron McCormack (:heycam)]

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.

Comment 2

[Cameron McCormack (:heycam)]

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.

Comment 3

[Cameron McCormack (:heycam)]

Attachment: Bug 1477079 - Skip XBL style building if document's pres shell went away.

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

Comment 4

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0281e3361d0569152f6d9ebb961b0e18332c9595

Comment 5

[Emilio Cobos Álvarez (:emilio)]

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...

Comment 6

[Cameron McCormack (:heycam)]

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.)

Comment 7

[Emilio Cobos Álvarez (: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?

Comment 8

[Jim Mathies [:jimm]]

Latest nightly appears fixed, will revisit next week.

Comment 9

[Marcia Knous [:marcia]]

Crash stats still shows 2-3 crashes per day on nightly, with the exception of August 3rd and August 5th builds.

Comment 10

[Marcia Knous [:marcia]]

Latest nightly shows 44 crashes/36 installations.

Comment 11

[David Durst [:ddurst]]

Let's try landing this to see what happens.

Comment 12

[Marcia Knous [:marcia]]

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.

Comment 13

[[:philipp]]

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).

Comment 14

[Pascal Chevrel:pascalc (PTO until April 26)]

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

Comment 15

[Sean Voisen (:svoisen)]

Upping to P1 given prevalence of crashes in nightly and beta.

Comment 16

[Cameron McCormack (:heycam)]

Sorry for missing that needinfo before.  I'll get this landed.

Comment 17

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4a594a407b7da520922f732cd1f13f5bcb0de734

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/194fe4b5ec09
Skip XBL style building if document's pres shell went away. r=emilio

Comment 19

[Cameron McCormack (:heycam)]

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

Comment 20

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/194fe4b5ec09

Comment 21

[Pascal Chevrel:pascalc (PTO until April 26)]

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

Comment 22

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/releases/mozilla-beta/rev/1ff507e18201