Closed Bug 1324627 Opened 5 years ago Closed 5 years ago

stylo: Assertion failure "!el.has_dirty_descendants()" on test test_{initial,inherit,value}_computation.html

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: xidorn, Assigned: bholley)

References

Details

Attachments

(4 files, 2 obsolete files)

Run "./mach mochitest --disable-e10s layout/style/test/test_initial_computation.html", it would crash with
> thread '<unnamed>' panicked at 'assertion failed: !el.has_dirty_descendants()'
Flags: needinfo?(bobbyholley)
Got a chance to look at this today. Our resolution of style for getComputedStyle in display:none subtrees needs to be reworked, I'm working on some patches.
Blocks: 1324983
I didn't quite get time to finish this today, but I'm going to run the patches here through try, and flag for review on the patches that should be ready to go.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=af7cd34015a27eb0b9ce96ce6e1eb76c3020d4fd
Flags: needinfo?(bobbyholley)
This one still needs some tweaking (the mochitest triggers an assert in the
new code), so I'm not flagging for review, but putting it here for reference.
Attachment #8821011 - Flags: review?(cam) → review+
Comment on attachment 8821012 [details] [diff] [review]
Part 2 - Use Gecko's existing mechanism to coordinate flushing the Stylist. v1

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

Looks good.
Attachment #8821012 - Flags: review?(cam) → review+
Comment on attachment 8821013 [details] [diff] [review]
Part 3 - Simplify pseudo-element handling. v1

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

::: layout/style/ServoStyleSet.cpp
@@ +191,5 @@
>      NS_ERROR("stylo: We don't support CSS_PSEUDO_ELEMENT_SUPPORTS_USER_ACTION_STATE yet");
>    }
> +
> +  // NB: We ignore aParentContext, on the assumption that pseudo element styles
> +  // should just inherit from the primary style, which Servo already knows.

"from aParentElement's primary style"?

(We should maybe rename aParentElement to aOriginatingElement, so that that is an official term: https://drafts.csswg.org/selectors-4/#originating-element)
Attachment #8821013 - Flags: review?(cam) → review+
*now that
While we're at it, we remove the lazy style resolution path in Servo_ResolveStyle.
I plan to remove the whole concept of 'consume' soon, but we support it for now.

There's still an issue surrounding style consumption for scrollbar resolution
that I need to look into tomorrow. But this should be good enough for review,
per IRC discussion.
Attachment #8821441 - Flags: review?(cam)
Comment on attachment 8821441 [details] [diff] [review]
Part 4 - Add a special, explicit path for lazy style resolution and use it for GetComputedStyle. v1

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

r=me with an explanation of why we switch to aPresShell->GetPresContext().

::: layout/style/nsComputedDOMStyle.cpp
@@ +471,2 @@
>  
> +  nsPresContext *presContext = aPresShell->GetPresContext();

Why this change to use aPresShell?

::: servo/components/style/traversal.rs
@@ +396,5 @@
> +    callback(element.borrow_data().unwrap().styles());
> +
> +    // Clear any styles in display:none subtrees to leave the tree in a valid state.
> +    if let Some(root) = display_none_root {
> +        clear_descendant_data(root, clear_data);

Is it that we'll only have stored styles on the path from element up to display_none_root, and nowhere else?  Seems like we should try to avoid looking through all of the children for a single node at each level.  Happy for this to be a followup, if that's right.
Attachment #8821441 - Flags: review?(cam) → review+
Depends on: 1325728
(In reply to Cameron McCormack (:heycam) from comment #11)
> Comment on attachment 8821441 [details] [diff] [review]
> ::: layout/style/nsComputedDOMStyle.cpp
> @@ +471,2 @@
> >  
> > +  nsPresContext *presContext = aPresShell->GetPresContext();
> 
> Why this change to use aPresShell?

Whoops, this was a typo / holdover from some earlier refactoring that I undid. Good catch!

> ::: servo/components/style/traversal.rs
> @@ +396,5 @@
> > +    callback(element.borrow_data().unwrap().styles());
> > +
> > +    // Clear any styles in display:none subtrees to leave the tree in a valid state.
> > +    if let Some(root) = display_none_root {
> > +        clear_descendant_data(root, clear_data);
> 
> Is it that we'll only have stored styles on the path from element up to
> display_none_root, and nowhere else?  Seems like we should try to avoid
> looking through all of the children for a single node at each level.  Happy
> for this to be a followup, if that's right.

Fair point, I've switched it to bottom-up, which should be more efficient.
Attachment #8821014 - Attachment is obsolete: true
Attachment #8821441 - Attachment is obsolete: true
Unfortunately, this patch still causes some issues which I don't have time to dig into before heading out of town. I'm landing the other stuff under bug 1325728.

In general, there's a lot of weirdness right now around consuming styles. I plan to get rid of this concept in bug 1325734, but I'd like to find a way to limp along here rather than blocking on that.

The main problem I ran into is an infinite recursion issue on dom/base/crashtests/375399-1.html and dom/base/crashtests/554230-1.xhtml. Somehow we get stuck when setting up the scrollbars and resolving XBL bindings:  we post an nsHideViewer scriptrunner during ProcessPendingRestyles, which triggers another ProcessPendingRestyles, and so forth. Switching from DontConsume to Consume in GetPropagatedScrollbarStylesForViewport avoids the hang in 375399-1.html but not in 554230-1.xhtml. I haven't had time to dig into the exact pathology yet.

Another side-problem is that, with the aforementioned workaround, 375399-1.html crashes in the parent.has_dirty_descendants() assertion in resolve_style_internal. This is because our XBL traversal optimization at [1] leaves an unstyled subtree without setting the dirty descendants bit. I think we probably need to fix the patch in this bug to avoid assuming that the parent has the bit set properly and propagate it up ourselves.

Cameron, feel free to look at the infinite recursion issue if you want, otherwise I'll take a look when I get back.

[1] https://github.com/servo/servo/blob/b13afccb8a4e922f66c77a92914e6505c62ae483/components/style/traversal.rs#L185
Flags: needinfo?(bobbyholley)
Looking at 554230-1.xhtml, the infinite recursion is due to not consuming style inside Servo_ResolveStyleLazily if we find the node already has style.  I have a patch for that now, but it's not really clear to me whether it makes sense to use that style if it comes out of an ElementData::Restyle(..), although in this case it's an Initial(Some(..)) style so that seems safe.
(In reply to Bobby Holley (PTO) (busy with Stylo) from comment #14)
> Another side-problem is that, with the aforementioned workaround,
> 375399-1.html crashes in the parent.has_dirty_descendants() assertion in
> resolve_style_internal. This is because our XBL traversal optimization at
> [1] leaves an unstyled subtree without setting the dirty descendants bit. I
> think we probably need to fix the patch in this bug to avoid assuming that
> the parent has the bit set properly and propagate it up ourselves.

Do we need to propagate the bit though?  We're not relying on traversal in here, so it if propagating the bit is just to satisfy the assertion, I'd say just remove the assertion.  Or are there other places that we might bump into the lack of dirty bits from unstyled XBL stuff that we need to fix too?
Comment on attachment 8821700 [details] [diff] [review]
Add a special, explicit path for lazy style resolution and use it for GetComputedStyle. v2

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

r=me with the comments above about the assertion and consuming styles addressed, which I'll do.  I'm still not sure about the validity of persisting an ElementData::Restyle in this case, since there might still be a restyle hint (which would need descendants to be restyled) that get dropped.  I'll add a comment about that.
Attachment #8821700 - Flags: review+
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5986d8880ad1
Add a special, explicit path for lazy style resolution and use it for GetComputedStyle. r=heycam
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f25c47181e91
Add a special, explicit path for lazy style resolution and use it for GetComputedStyle. r=heycam
https://hg.mozilla.org/mozilla-central/rev/f25c47181e91
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Assignee: nobody → bobbyholley
Flags: needinfo?(bobbyholley)
You need to log in before you can comment on or make changes to this bug.