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

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Xidorn Quan [:xidorn] UTC+11]

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()'

Comment 1

[Bobby Holley (:bholley)]

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.

Comment 2

[Bobby Holley (:bholley)]

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

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 1 - Add an assertion to catch the issue earlier. v1

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 2 - Use Gecko's existing mechanism to coordinate flushing the Stylist. v1

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 3 - Simplify pseudo-element handling. v1

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 4 - Add a special, explicit path for manual style resolution and use it for GetComputedStyle. WIP

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.

Comment 7

[Cameron McCormack (:heycam)]

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.

Comment 8

[Cameron McCormack (:heycam)]

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)

Comment 9

[Cameron McCormack (:heycam)]

*now that

Comment 10

[Bobby Holley (:bholley)]

Attachment: Part 4 - Add a special, explicit path for lazy style resolution and use it for GetComputedStyle. v1

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.

Comment 11

[Cameron McCormack (:heycam)]

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.

Comment 12

[Bobby Holley (:bholley)]

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

Comment 13

[Bobby Holley (:bholley)]

Attachment: Add a special, explicit path for lazy style resolution and use it for GetComputedStyle. v2

Comment 14

[Bobby Holley (:bholley)]

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

Comment 15

[Cameron McCormack (:heycam)]

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.

Comment 16

[Cameron McCormack (:heycam)]

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

[Cameron McCormack (:heycam)]

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.

Comment 18

[Cameron McCormack (:heycam)]

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

Comment 19

[Cameron McCormack (:heycam)]

https://github.com/servo/servo/pull/14751

Comment 20

[Pulsebot]

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

Comment 21

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/faba110ce12b for a nice little variety of things, probably best covered with https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=5986d8880ad1ad68950daa821c39d0a2775de821&filter-failure_classification_id=2

Comment 22

[Pulsebot]

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

Comment 23

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f8a2cbf8bc47e1fcdb25af86e628f78b16df4c6d&group_state=expanded

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f25c47181e91