1329854 - stylo: Track newly-appended subtrees when frame construction bails out due to lack of container frame

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Bobby Holley (:bholley)]

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

Comment 1

[Bobby Holley (:bholley)]

Attachment: Call NoteDirtyDescendants when frame construction bails out due to lack of a container frame. v1

Comment 2

[Bobby Holley (:bholley)]

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

Comment 3

[Bobby Holley (:bholley)]

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.

Comment 4

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0c02fc8b7f08b9386ebfd48b3bc75875772c9f61

Comment 5

[Bobby Holley (:bholley)]

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

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 1 - Don't lazily instantiate element data for unstyled elements when taking snapshots. v1

Comment 7

[Bobby Holley (:bholley)]

Attachment: Part 2 - Handle suppressed frames in stylo incremental restyle. v1

Comment 8

[Bobby Holley (:bholley)]

Attachment: Part 3 - Avoid propagating the dirty descendants bit when appending items to display:none subtrees. v1

Comment 9

[Bobby Holley (:bholley)]

Attachment: Part 4 - Call NoteDirtyDescendants when frame construction bails out due to lack of a container frame. v1

Comment 10

[Bobby Holley (:bholley)]

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 11

[Cameron McCormack (:heycam)]

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 12

[Cameron McCormack (:heycam)]

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 13

[Cameron McCormack (:heycam)]

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 14

[Cameron McCormack (:heycam)]

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 15

[Bobby Holley (:bholley)]

Attachment: Part 1 - Don't lazily instantiate element data for unstyled elements when taking snapshots and noting explicit hints. v2

Comment 16

[Cameron McCormack (:heycam)]

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

Comment 17

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9efe1c91710cabd2bb337aceaa23451f623cdbc4

Comment 18

[Bobby Holley (:bholley)]

Attachment: Part 5 - Check IsInStyleRefresh for all the servo-related handling in nsCSSFrameConstructor::Content{Appended,RangeInserted}. v1

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

Comment 19

[Bobby Holley (:bholley)]

This is finally green, modulo one assertion expectation adjustment that I need to update:

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

Comment 20

[Cameron McCormack (:heycam)]

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

Comment 21

[Bobby Holley (:bholley)]

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

Comment 22

[Pulsebot]

Pushed by [e-mail]:
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

Comment 23

[Bobby Holley (:bholley)]

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

Comment 24

[Carsten Book [:Tomcat]]

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