Closed Bug 1322945 Opened 5 years ago Closed 5 years ago

stylo: Fix some crashes on incubator

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: bholley, Unassigned)

References

Details

Attachments

(4 files)

Some recent landings have caused new crashes on incubator. I have a few patches.
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.
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
Attachment #8818058 - Flags: review?(ecoal95)
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
Attachment #8818059 - Flags: review?(ecoal95)
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
Attachment #8818060 - Flags: review?(ecoal95)
Attachment #8818058 - Flags: review?(ecoal95) → review+
Attachment #8818059 - Flags: review?(ecoal95) → review+
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.
Attachment #8818060 - Flags: review?(ecoal95) → review+
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
Attachment #8818131 - Flags: review?(cam)
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.)
Attachment #8818131 - Flags: review?(cam) → review+
Corresponding servo PR: https://github.com/servo/servo/pull/14560
Pushed by bholley@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/48a1fc156b8d
Change skip_root to unstyled_children_only and use StyleNewChildren in more places. r=heycam
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
https://hg.mozilla.org/mozilla-central/rev/48a1fc156b8d
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
(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!
You need to log in before you can comment on or make changes to this bug.