Closed Bug 1322945 Opened 8 years ago Closed 8 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+
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
Status: NEW → RESOLVED
Closed: 8 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.

Attachment

General

Created:
Updated:
Size: