Crash in style::traversal::compute_style

VERIFIED FIXED in Firefox -esr60

Status

()

defect
--
critical
VERIFIED FIXED
Last year
11 months ago

People

(Reporter: philipp, Assigned: emilio)

Tracking

({crash, regression})

60 Branch
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox-esr60 fixed, firefox61 wontfix, firefox62 fixed, firefox63 verified)

Details

(crash signature)

Attachments

(2 attachments)

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.
Flags: needinfo?(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
Posted file 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.
Assignee: nobody → emilio
Status: NEW → ASSIGNED
Flags: needinfo?(emilio)
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.
Attachment #8999431 - Attachment description: Don't clear the insertion point from ClearInsertedChildren if we're not the insertion point the node is assigned to. r=bz → Don't clear the insertion point from ClearInsertedChildren if we're not the insertion point the node is assigned to.
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.
Attachment #8999431 - Flags: review+
Pushed by emilio@crisal.io:
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
https://hg.mozilla.org/mozilla-central/rev/dc6e00ee89a8
Status: ASSIGNED → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
i can confirm that the steps in comment #0 no longer lead to the crash in today's nightly build containing the patch.
Status: RESOLVED → VERIFIED
Please request Beta & ESR60 approval on this when you get a chance.
Flags: needinfo?(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
Flags: needinfo?(emilio)
Attachment #8999431 - Flags: approval-mozilla-esr60?
Attachment #8999431 - Flags: approval-mozilla-beta?
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.
Attachment #8999431 - Flags: approval-mozilla-esr60?
Attachment #8999431 - Flags: approval-mozilla-esr60+
Attachment #8999431 - Flags: approval-mozilla-beta?
Attachment #8999431 - Flags: approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.