This is the source of one of the crashes in bug 1323649. I have a patch.

Comment on attachment 8825254 [details] [diff] [review] Call NoteDirtyDescendants when frame construction bails out due to lack of a container frame. v1 Hm, try push is orange. Need to investigate.

Still running this through try, but might as well get the patches so far reviewed.

Comment on attachment 8826035 [details] [diff] [review] Part 1 - Don't lazily instantiate element data for unstyled elements when taking snapshots. v1 Either emilio or heycam would be fine for this one, but I guess I'll do heycam since I flagged him for the others.

Comment on attachment 8826035 [details] [diff] [review] Part 1 - Don't lazily instantiate element data for unstyled elements when taking snapshots. v1 Review of attachment 8826035 [details] [diff] [review]: ----------------------------------------------------------------- ::: servo/ports/geckolib/glue.rs @@ +874,5 @@ > pub extern "C" fn Servo_Element_GetSnapshot(element: RawGeckoElementBorrowed) -> *mut structs::ServoElementSnapshot > { > let element = GeckoElement(element); > + let snapshot = match element.mutate_data() { > + None => return ptr::null_mut(), Omit the "return"? Then we can get the debug!() logging below.

Comment on attachment 8826036 [details] [diff] [review] Part 2 - Handle suppressed frames in stylo incremental restyle. v1 Review of attachment 8826036 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/ServoRestyleManager.cpp @@ +229,5 @@ > if (traverseElementChildren && n->IsElement()) { > + if (!primaryFrame) { > + // The frame constructor presumably decided to suppress frame > + // construction on this subtree. Just clear the dirty descendants > + // bit from the subtree, since there's point in harvesting the "no point"

Comment on attachment 8826037 [details] [diff] [review] Part 3 - Avoid propagating the dirty descendants bit when appending items to display:none subtrees. v1 Review of attachment 8826037 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/ElementInlines.h @@ +59,5 @@ > Element::NoteDirtyDescendantsForServo() > { > Element* curr = this; > + // NB: The dirty descendants bit only applies to styled elements. > + while (curr && !curr->HasDirtyDescendantsForServo() && curr->HasServoData()) { We shouldn't be able to have a node with data whose parent doesn't have data, so I think we can just check curr->HasServoData() once outside of the loop. ::: servo/components/style/traversal.rs @@ +419,5 @@ > preprocess_children(traversal, element, propagated_hint, inherited_style_changed); > } > + > + // Make sure the dirty descendants bit is not set for the root of a > + // display:none subtree, even if the style didn't change. This keeps the even if the style did change?

Comment on attachment 8826038 [details] [diff] [review] Part 4 - Call NoteDirtyDescendants when frame construction bails out due to lack of a container frame. v1 Review of attachment 8826038 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsCSSFrameConstructor.cpp @@ +7370,5 @@ > if (!GetContentInsertionFrameFor(aContainer) && > !aContainer->IsActiveChildrenElement()) { > + // We're punting on frame construction because there's no container frame. > + // The Servo-backed style system handles this case like the lazy frame > + // construction case. Nit: this comment should be one indent level lower.

Comment on attachment 8826056 [details] [diff] [review] Part 1 - Don't lazily instantiate element data for unstyled elements when taking snapshots and noting explicit hints. v2 Review of attachment 8826056 [details] [diff] [review]: ----------------------------------------------------------------- ::: servo/ports/geckolib/glue.rs @@ +899,5 @@ > element, restyle_hint, change_hint); > > + let mut maybe_data = element.mutate_data(); > + let maybe_restyle_data = > + maybe_data.as_mut().and_then(|d| unsafe { maybe_restyle(d, element) }); I was very close to suggesting using and_then in the v1 patch. :-)

Need another patch to make this stack green. Hopefully this is the last one.

This is finally green, modulo one assertion expectation adjustment that I need to update: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ddd417b4de10624634b4a1186b16352b484a50e2

Comment on attachment 8826321 [details] [diff] [review] Part 5 - Check IsInStyleRefresh for all the servo-related handling in nsCSSFrameConstructor::Content{Appended,RangeInserted}. v1 Review of attachment 8826321 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsCSSFrameConstructor.cpp @@ +7371,5 @@ > + // and lazy frame construction). If we're using the Servo style system, we > + // want to ensure that styles get resolved in the first case, whereas for the > + // second case they should have already been resolved if needed. > + bool isNewlyAddedContentForServo = aContainer->IsStyledByServo() && > + !RestyleManager()->AsBase()->IsInStyleRefresh(); To verify the comment, is it worth asserting that aFirstNewContent (and its following siblings) have styles when IsStyledByServo() and IsInStyleRefresh() are both true? (And in ContentRangeInserted?)

(In reply to Cameron McCormack (:heycam) from comment #20) > Comment on attachment 8826321 [details] [diff] [review] > Part 5 - Check IsInStyleRefresh for all the servo-related handling in > nsCSSFrameConstructor::Content{Appended,RangeInserted}. v1 > > Review of attachment 8826321 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: layout/base/nsCSSFrameConstructor.cpp > @@ +7371,5 @@ > > + // and lazy frame construction). If we're using the Servo style system, we > > + // want to ensure that styles get resolved in the first case, whereas for the > > + // second case they should have already been resolved if needed. > > + bool isNewlyAddedContentForServo = aContainer->IsStyledByServo() && > > + !RestyleManager()->AsBase()->IsInStyleRefresh(); > > To verify the comment, is it worth asserting that aFirstNewContent (and its > following siblings) have styles when IsStyledByServo() and > IsInStyleRefresh() are both true? (And in ContentRangeInserted?) I thought about this, but I think this would probably end up doing the same thing as the assert we'll eventually have in Servo_ResolveStyle (which is currently a soft-assert + return default CV, because we don't handle all the edge cases yet). So I'm just going to wait until we fix that, and then see if adding another assertion in the slightly-earlier location (above) would add any value.

Pushed by bholley@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/accbd8ed10d5 Handle suppressed frames in stylo incremental restyle. r=heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/15d1f9b75794 Avoid propagating the dirty descendants bit when appending items to display:none subtrees. r=heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/0734dcc4e4bf Call NoteDirtyDescendants when frame construction bails out due to lack of a container frame. r=heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/74d4469b7841 Check IsInStyleRefresh for all the servo-related handling in nsCSSFrameConstructor::Content{Appended,RangeInserted}. r=heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/2ad9202bae1e Update crashtest expectations. r=me

Servo PR: https://github.com/servo/servo/pull/14997

https://hg.mozilla.org/mozilla-central/rev/accbd8ed10d5 https://hg.mozilla.org/mozilla-central/rev/15d1f9b75794 https://hg.mozilla.org/mozilla-central/rev/0734dcc4e4bf https://hg.mozilla.org/mozilla-central/rev/74d4469b7841 https://hg.mozilla.org/mozilla-central/rev/2ad9202bae1e