1482694 - Crash in style::traversal::compute_style

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[[:philipp]]

This bug was filed from the Socorro interface and is
report bp-773544fd-ff79-49a8-84c7-861b50180710.
=============================================================

Top 10 frames of crashing thread:

0 libmozglue.dylib mozalloc_abort memory/mozalloc/mozalloc_abort.cpp:33
1 libmozglue.dylib abort memory/mozalloc/mozalloc_abort.cpp:80
2 XUL std::panicking::rust_panic src/libpanic_abort/lib.rs:59
3 XUL std::panicking::rust_panic_with_hook::h6e16f84ca8c4a724 src/libstd/panicking.rs:593
4 XUL std::panicking::begin_panic::h950ea52f07a191e2 src/libstd/panicking.rs:538
5 XUL std::panicking::begin_panic_fmt::he83be180ce68059a src/libstd/panicking.rs:522
6 XUL core::panicking::panic_fmt::h5f860eec3f515a80 src/libstd/panicking.rs:498
7 XUL core::option::expect_failed::h7f635057bfba806a src/libcore/option.rs:891
8 XUL style::traversal::compute_style::h63ed2411e2498e09 src/libcore/option.rs:302
9 XUL _$LT$rayon_core..job..HeapJob$LT$BODY$GT$$u20$as$u20$rayon_core..job..Job$GT$::execute::h2dfad87fbf12c53f servo/components/style/traversal.rs:420

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

these crash signatures are showing up cross-platform since firefox 60 with moz crash reason "We were lied to".

many user comments refer to changing the page style settings in the view menu - in particular this crash is reproducible in about:preferences when switching from "basic page style" to "no style" and back though i don't get a crash reporter window popping up reporting that crash myself.

with these steps mozregression is pointing towards the changes in bug 1439395 as having introduced  this crash.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Well, the underlying issue is caught earlier in debug builds:

  Assertion failure: content->GetFlattenedTreeParentNodeForStyle() == &aContent, at /home/emilio/src/moz/gecko-2/layout/base/RestyleManager.cpp

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Debugging log

What's going on is that nested XBL insertion points are completely unsound, and we leave all sorts of kids in mInsertedChildren when bindings dynamically change.

So if bindings change in a particular way so that the innermost insertion point is cleared, but the outermost bindings aren't, like this, we end up with an inconsistent flattened tree.

This prevents the inconsistent flattened tree in this case, though what ought to happen in the SetXBLInsertionPoint(nullptr) case is setting the insertion point to the outer insertion point.

I don't think it's worth to fix that given XBL is going away unless it gives us more problems. See also bug 1425362 & co.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Don't clear the insertion point from ClearInsertedChildren if we're not the insertion point the node is assigned to.

What's going on is that nested XBL insertion points are completely unsound, and
we leave all sorts of kids in mInsertedChildren when bindings dynamically
change.

So if bindings change in a particular way so that the innermost insertion point
is cleared, but the outermost bindings aren't, like this, we end up with an
inconsistent flattened tree.

This prevents the inconsistent flattened tree in this case, though what ought
to happen in the SetXBLInsertionPoint(nullptr) case is setting the insertion
point to the outer insertion point.

But we don't keep track of all our insertion points, and I don't think it's
worth to fix that given XBL is going away unless it gives us more problems. See
also bug 1425362 & co.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8999431 [details]
Don't clear the insertion point from ClearInsertedChildren if we're not the insertion point the node is assigned to.

Olli Pettay [:smaug] has approved the revision.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dc6e00ee89a8
Don't clear the insertion point from ClearInsertedChildren if we're not the insertion point the node is assigned to. r=smaug

Comment 6

[Bogdan Tara[:bogdan_tara | bogdant]]

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

Comment 7

[[:philipp]]

i can confirm that the steps in comment #0 no longer lead to the crash in today's nightly build containing the patch.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Please request Beta & ESR60 approval on this when you get a chance.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8999431 [details]
Don't clear the insertion point from ClearInsertedChildren if we're not the insertion point the node is assigned to.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: Trivial crash fix.
User impact if declined: Crash
Fix Landed on Version: 63
Risk to taking this patch (and alternatives if risky): Risk is that it introduces some other tricky stuff, though honestly I don't think it would. Alternative is not landing it.
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1439395, though the root cause is pre-existing and probably as far as XBL goes.
[User impact if declined]: can crash with the STR in comment 0
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: comment 0
[List of other uplifts needed for the feature/fix]: no
[Is the change risky?]: not very risky
[Why is the change risky/not risky?]: The fix is relatively simple, and the chances it breaks non-already-broken stuff are very small.
[String changes made/needed]: none

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8999431 [details]
Don't clear the insertion point from ClearInsertedChildren if we're not the insertion point the node is assigned to.

Simple crash fix. Approved for 62.0b18 and ESR 60.2.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8b634e6d6b4c

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/b225794bcc82