1322945 - stylo: Fix some crashes on incubator

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Bobby Holley (:bholley)]

Some recent landings have caused new crashes on incubator. I have a few patches.

Comment 1

[Bobby Holley (:bholley)]

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

Comment 2

[Bobby Holley (:bholley)]

This is still missing a fix for the !has_dirty_descendants() assertion in AssertTreeIsClean on layout/reftests/bugs/404553-1.html . I probably won't have time to look into it tonight, but it should be relatively straightforward if anybody is blocked on it and wants to investigate. Otherwise I'll look tomorrow.

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 1 - Only assert against _restyling_ roots with later siblings.

The problem is with restyles, so we should leave the door open on
initial styling if that ends up making sense.

MozReview-Commit-ID: 5GOFBEUZhDe

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 2 - Remove the requirement that the parent styles must be current to style a subtree.

Sometimes Gecko eagerly styles things without processing pending restyles first. In
general we'd like to avoid this, but there can be good reasons (for example, needing
to construct a frame for some small piece of newly-added content in order to do something
specific with that frame, but not wanting to flush all of layout). Just handle it.

MozReview-Commit-ID: EjXs0M4855Q

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 3 - Change skip_root to unstyled_children_only and use StyleNewChildren in more places.

I noticed that our current behavior in ContentRangeInserted is incorrect. Unlike
ContentInserted (where this code lived originally), ContentRangeInserted takes a
start and end element. I'm not sure if we ever take that path for new content that
needs style, but it seemed sketchy. And generally, it seems nice to just always
style new content the same way (though we still need to style NAC by the subtree
root, since it hasn't been attached to the parent yet).

For situations where there is indeed only one unstyled child, the traversal
overhead should be neglible, since we special-case the single-element in
parallel.rs to avoid calling into rayon.

Being more explicit about what we want here also makes us more robust against
the other handful of callpaths that can take us into
nsCSSFrameConstructor::{ContentRangeInserted,ContentAppended}. Currently we
can call StyleNewSubtree on an already-styled element via RecreateFramesForContent,
which triggers an assertion in the servo traversal.

MozReview-Commit-ID: DqCGh90deHH

Comment 6

[Cameron McCormack (:heycam)]

Comment on attachment 8818060 [details] [diff] [review]
Part 3 - Change skip_root to unstyled_children_only and use StyleNewChildren in more places.

Review of attachment 8818060 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/ServoStyleSet.h
@@ +136,4 @@
>  
>    /**
> +   * Like the above, but skips the root node, and only styles unstyled children.
> +   * When potentially appending multiple children, it's wpreferable to call

preferable

::: servo/components/style/binding_tools/regen.py
@@ +97,5 @@
>              "mozilla::ServoElementSnapshot.*",
>              "mozilla::ConsumeStyleBehavior",
>              "mozilla::LazyComputeBehavior",
>              "mozilla::css::SheetParsingMode",
> +            "mozilla::TraversalRootBehavior",

Now that build time bindgen has landed, you'll need to move these changes into components/style/build_gecko.rs.

Comment 7

[Bobby Holley (:bholley)]

Attachment: Part 4 - Improve ergonomics and share more code for style crate DOM tree logging.

This has been on my todo list for a while, and I wrote it to make it easier to
figure out the dirty_descendants crash, which I'll look at next.

MozReview-Commit-ID: 4Fy3ujpI4n2

Comment 8

[Cameron McCormack (:heycam)]

Comment on attachment 8818131 [details] [diff] [review]
Part 4 - Improve ergonomics and share more code for style crate DOM tree logging.

Review of attachment 8818131 [details] [diff] [review]:
-----------------------------------------------------------------

::: servo/components/style/dom.rs
@@ +161,5 @@
> +        let dd = el.has_dirty_descendants();
> +        let data = el.borrow_data();
> +        let styles = data.as_ref().and_then(|d| d.get_styles());
> +        let values = styles.map(|s| &s.primary.values);
> +        write!(f, "{:?} dd={}, data={:?} values={:?}", el, dd, &data, values)

No comma after dd, to match fmt_with_data's output?

@@ +167,5 @@
> +        write!(f, "{:?}", n)
> +    }
> +}
> +
> +fn fmt_subtree<F, N: TNode>(f: &mut fmt::Formatter, stringify: &F, n: N, indent: u32)

(FWIW I think Debug impls generally do multi-line indented output only when f.alternate() is true (i.e. if formatted with {:#?}), but I guess the explicit use of these debug wrappers means we don't have to follow that convention.)

Comment 9

[Bobby Holley (:bholley)]

Corresponding servo PR: https://github.com/servo/servo/pull/14560

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/48a1fc156b8d
Change skip_root to unstyled_children_only and use StyleNewChildren in more places. r=heycam

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Please, in the future don't remove useful assertions completely, just make them servo-only.

I did that for the ones removed here in https://github.com/servo/servo/pull/14565

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/48a1fc156b8d

Comment 13

[Bobby Holley (:bholley)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #11)
> Please, in the future don't remove useful assertions completely, just make
> them servo-only.

Fair enough. In general I'm trying to keep the model as similar as possible between Gecko and Servo to reduce the surface of "works in one but crashes in the other" bugs, but I agree in this case that it doesn't seem like Servo should ever need to do this.

> I did that for the ones removed here in
> https://github.com/servo/servo/pull/14565

Thanks!