1422634 - Crash in mozilla::ServoStyleSet::SetStylistStyleSheetsDirty

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Nicholas Nethercote [inactive]]

This bug was filed from the Socorro interface and is
report bp-0d4a3038-322b-43d7-8f46-e9de20171203.
=============================================================

Top 10 frames of crashing thread:

0 xul.dll mozilla::ServoStyleSet::SetStylistStyleSheetsDirty layout/style/ServoStyleSet.cpp:1050
1 xul.dll mozilla::StyleSheet::RuleRemoved layout/style/StyleSheet.cpp:641
2 xul.dll mozilla::ServoStyleSheet::DeleteRuleInternal layout/style/ServoStyleSheet.cpp:440
3 xul.dll mozilla::StyleSheet::DeleteRule layout/style/StyleSheet.cpp:588
4 xul.dll mozilla::dom::CSSStyleSheetBinding::deleteRule dom/bindings/CSSStyleSheetBinding.cpp:223
5 xul.dll mozilla::dom::GenericBindingMethod dom/bindings/BindingUtils.cpp:3042
6 xul.dll js::InternalCallOrConstruct js/src/vm/Interpreter.cpp:473
7 xul.dll Interpret js/src/vm/Interpreter.cpp:3096
8 xul.dll js::RunScript js/src/vm/Interpreter.cpp:423
9 xul.dll js::InternalCallOrConstruct js/src/vm/Interpreter.cpp:495

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

The crash address is always 0x20, and it occurs on this line:

mPresContext->RestyleManager()->AsServo()->IncrementUndisplayedRestyleGeneration();

So it's probably a null pointer somewhere in that dereference chain.

heycam, any ideas?

Comment 1

[Cameron McCormack (:heycam)]

Assuming no funny business with deleted objects, we must be in a situation where:

* the StyleSheet still has this ServoStyleSet in its mStyleSets array
* the nsPresContext has had its mRestyleManager cleared, which means we've called nsPresContext::DetachShell
* we must have called PresShell::Destroy, which is what calls nsPresContext::DetachShell
* but if the ServoStyleSet still exists, we can't have destroyed the PresShell object yet (since it is deleted in the PresShell destructor)

It's conceivable the PresShell is lasting a bit longer than we expect, since it's a COM object and other things might have a strong reference to it.  (Though we do try to break some of those references, apparently, in nsDocumentViewer::DestroyPresShell.)

If this is all correct (and reasonable), then we could start recording mInShutdown in ServoStyleSet, and just return early from these style sheet dirty methods when it's true.

Does that sound reasonable Emilio?

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Yup. I have another idea, given we need to support null pres contexts anyway for XBL / Shadow DOM... But we should really fix bug 154199...

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1422634: Avoid sheet notifications to dereference a null restyle manager.

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

Comment 4

[Cameron McCormack (:heycam)]

Comment on attachment 8934118 [details]
Bug 1422634: Avoid sheet notifications to dereference a null restyle manager.

https://reviewboard.mozilla.org/r/205058/#review210526

This works too. :-)  Please mention when mPresContext becomes null in the comment on its declaration.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8934118 [details]
Bug 1422634: Avoid sheet notifications to dereference a null restyle manager.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/205058/diff/1-2/

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8934118 [details]
Bug 1422634: Avoid sheet notifications to dereference a null restyle manager.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/205058/diff/2-3/

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5091ed61e147
Avoid sheet notifications to dereference a null restyle manager. r=heycam

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e1b04d5511d5
followup: fix typo in the comment. r=comment-only

Comment 9

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/5091ed61e147
https://hg.mozilla.org/mozilla-central/rev/e1b04d5511d5

Comment 10

[Calixte Denizet (:calixte)]

No more crashes in nightly 59 since the patch landed.