Closed Bug 1351535 Opened 3 years ago Closed 3 years ago

stylo: frame constructor can call RecreateFramesForContent in content insertion/removal notifications, expecting up to date styles

Categories

(Core :: CSS Parsing and Computation, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: heycam, Assigned: heycam)

References

(Blocks 1 open bug)

Details

Attachments

(7 files, 3 obsolete files)

59 bytes, text/x-review-board-request
bholley
: review+
Details
59 bytes, text/x-review-board-request
bholley
: review+
Details
59 bytes, text/x-review-board-request
bholley
: review+
Details
59 bytes, text/x-review-board-request
bholley
: review+
Details
59 bytes, text/x-review-board-request
hiro
: review+
Details
59 bytes, text/x-review-board-request
bholley
: review+
Details
59 bytes, text/x-review-board-request
bholley
: review+
Details
While looking into more cases of the bug 1345695 assertions, I realized that there a bunch of cases where we call nsCSSFrameConstructor::RecreateFramesForContent inside the content notification methods (ContentRemoved, ContentRangeInserted, ContentAppended) on things other than the nodes subject to the notification.  For example:

* We recreate frames for a parent <fieldset> element, when we append a <legend> (and similarly for <details> and appending a <summary>, and appending anything to an element with a MathML frame):

http://searchfox.org/mozilla-central/rev/7419b368156a6efa24777b21b0e5706be89a9c2f/layout/base/nsCSSFrameConstructor.cpp#8042

* We recreate frames for the displayed ancestor of a display:contents element that has generated content when it is removed:

http://searchfox.org/mozilla-central/rev/7419b368156a6efa24777b21b0e5706be89a9c2f/layout/base/nsCSSFrameConstructor.cpp#8461

* We recreate frames for a <frameset> element when a child <frameset> or <frame> is inserted or removed:

http://searchfox.org/mozilla-central/rev/7419b368156a6efa24777b21b0e5706be89a9c2f/layout/base/nsCSSFrameConstructor.cpp#8461

And some more similar cases.  The problem is that we might have out of dates styles on that ancestor that we decide to reframe, or on any of its descendants that were unaffected by the content insertion/removal notification.

It seems like we do want to have up to date styles here, since we're going to have to restyle soon anyway, and we'll have wasted work creating these frames with their old styles.

Bobby, do we need a new method on ServoStyleSet, something like "StyleExistingSubtree", which can handle doing a restyle of content that may or may not have existing styles?  I wonder what we should do about out of date styles on nodes above the ancestor we choose to reframe.  I wonder what Gecko does...
Flags: needinfo?(bobbyholley)
Per IRC discussion, we decided that we should (re)style the subtree, ignoring the possibility that the parent styles might be stale. We'll want a special bit on the SharedStyleContext that tells us to:
* Avoid generating change hints for any restyles.
* Clear any existing change hints.
* Clear the dirty descendants bit.

i.e. it should leave the tree in an initial-style state. We could call this mode restyle_for_reconstruct or somesuch, to indicate that the old frame state isn't relevant anymore.

As a bonus, we can fix up part 3 of bug 1350441 to use this smarter traversal instead of what it's doing now, since it's dealing with the same problem (stale change hints).
Flags: needinfo?(bobbyholley)
I confirmed btw that stock Gecko does use the old styles of the parent of the element we're reframing to inherit from, rather than resolve new styles right up the tree.
Assignee: nobody → cam
Priority: -- → P1
Comment on attachment 8854429 [details]
Bug 1351535 - Part 5: Add EffectCompositor method to pre-traverse within a specific subtree.

https://reviewboard.mozilla.org/r/126378/#review129068

Thanks!  This way looks reasonable to me.
Attachment #8854429 - Flags: review?(hikezoe) → review+
Comment on attachment 8854423 [details]
Bug 1351535 - Part 1: Remove some out of date comments in stylo reftest manifest.

https://reviewboard.mozilla.org/r/126366/#review129144
Attachment #8854423 - Flags: review?(bobbyholley) → review+
Attachment #8854440 - Flags: review?(bobbyholley)
That part 10 patch, which I added at the last minute, seems to cause a crashtest failure, so I'll look at doing that part in a followup bug.
Comment on attachment 8854424 [details]
Bug 1351535 - Part 2: Explicitly indicate when a ContentRangeInserted call is for frame reconstruction.

https://reviewboard.mozilla.org/r/126368/#review129146

::: layout/base/nsCSSFrameConstructor.cpp:7238
(Diff revision 2)
> -                             false, &aTreeMatchContext);
> +                             false, // aAllowLazyConstruction
> +                             true,  // aForReconstruction
> +                             &aTreeMatchContext);
>        }
>      }
>    }
>  
>    if (inRun) {
> -    ContentAppended(aContent, firstChildInRun, false, &aTreeMatchContext);
> +    ContentAppended(aContent, firstChildInRun,
> +                    false, // aAllowLazyConstruction
> +                    true,  // aForReconstruction
> +                    &aTreeMatchContext);

Hm, lazy frame construction isn't exactly 'reconstruction', but it sorta makes sense. Please document what this means at the declaration of the functions though?

Also note that, in the servo case, we do lazy frame construction via posting reframes during the posttraversal. So that should go through RecreateFramesForContent, and eventually CreateNeededFrames will go away.
Attachment #8854424 - Flags: review?(bobbyholley) → review+
Comment on attachment 8854425 [details]
Bug 1351535 - Part 3: Only explicitly style newly inserted content if the parent is not in a display:none subtree.

https://reviewboard.mozilla.org/r/126370/#review129148
Attachment #8854425 - Flags: review?(bobbyholley) → review+
Comment on attachment 8854426 [details]
Bug 1351535 - Part 4: Don't traverse children if the root of the restyle is display:none.

https://reviewboard.mozilla.org/r/126372/#review129158

Per IRC discussion, please sort out whether we need both this and Part 3.
Attachment #8854426 - Flags: review?(bobbyholley) → review+
Comment on attachment 8854427 [details]
Bug 1351535 - Part 4: Add a TraversalRestyleBehavior argument to traversal functions.

https://reviewboard.mozilla.org/r/126374/#review129160

::: layout/style/ServoTypes.h:56
(Diff revision 2)
> +// Indicates whether the Servo style system should perform normal processing or
> +// whether it should ignore existing styles and restyles and not generate change
> +// hints for any restyled elements, as required when restyling due to frame

I'm not sure what the "ignore existing styles and restyles" bit is about - we still want to use existing styles and to process any pending restyles, we just don't want change hits.

I'd also clarify a bit more that the reason that the change hints are unnecessary is that the old frames have been destroyed.
Attachment #8854427 - Flags: review?(bobbyholley) → review+
Comment on attachment 8854427 [details]
Bug 1351535 - Part 4: Add a TraversalRestyleBehavior argument to traversal functions.

https://reviewboard.mozilla.org/r/126374/#review129164

::: layout/style/ServoTypes.h:56
(Diff revision 2)
> +// Indicates whether the Servo style system should perform normal processing or
> +// whether it should ignore existing styles and restyles and not generate change
> +// hints for any restyled elements, as required when restyling due to frame

That stuff about ignoring restyles was true in previous versions of the patches during development but not true of the final patches.  Ignoring the existing styles is true in that we don't look at them to determine what change hints to generate.  Although the presence or absence of styles still does influence the traversal.
Comment on attachment 8854428 [details]
Bug 1351535 - Part 6: Handle TraversalRestyleBehavior::ForReconstruct in the Servo restyle.

https://reviewboard.mozilla.org/r/126376/#review129162

::: servo/components/style/context.rs:95
(Diff revision 2)
> +    /// True if we are restyling a subtree in preparation for reconstructing its
> +    /// frames. The result is new styles will be computed for the subtree, and
> +    /// any existing styles are discarded without generating change hints.

Again, I think referring to "existing styles are discarded" is confusing, because we will still keep the old styles in lots of cases. The only difference with the normal path is that we don't generate change hints.

::: servo/components/style/matching.rs:595
(Diff revision 2)
> +        if !context.shared.restyle_for_reconstruct {
> -        if let Some(old) = old_values {
> +            if let Some(old) = old_values {
> -            self.accumulate_damage(restyle.unwrap(), &old, &new_values, pseudo);
> +                self.accumulate_damage(restyle.unwrap(), &old, &new_values, pseudo);
> -        }
> +            }
> +        }

You're missing the other callsite to accumulate_damage (which happens with the style sharing cache). I think we should pass context to accumulate_damage and have this early-return there to avoid these kinds of bugs.

::: servo/components/style/traversal.rs:33
(Diff revision 2)
>  bitflags! {
>      /// Represents that target elements of the traversal.

This comment isn't really accurate anymore.

It would also be really nice to move TraversalFlags into SharedStyleContext and centralize all this state. Maybe in a followup?

::: servo/components/style/traversal.rs:40
(Diff revision 2)
> +        /// Traverse the entire subtree, ignoring any existing styles, and
> +        /// therefore not generating any change hints.

Similar clarification would be good here.

::: servo/components/style/traversal.rs:174
(Diff revision 2)
> -                debug_assert!(root.next_sibling_element().is_none());
> -                let _later_siblings = r.compute_final_hint(root, stylist);
> +                let later_siblings = r.compute_final_hint(root, stylist);
> +                debug_assert!(!later_siblings || traversal_flags.for_reconstruct());

I don't understand why we need this - given that the caller already does compute_final_hint_and_propagate_later_siblings, it seems like we should have already stripped the LATER_SIBLINGS bit by this point.

That said, per below, I think we should also just hoist that call into here.

::: servo/components/style/traversal.rs:180
(Diff revision 2)
> -            traverse: Self::node_needs_traversal(root.as_node(), traversal_flags.for_animation_only()),
> +            traverse: traversal_flags.for_reconstruct() ||
> +                      Self::node_needs_traversal(root.as_node(), traversal_flags.for_animation_only()),

Hm, is this because the root might not have dirty descendants, might _also_ not have a restyle or cascade hint, but still might have explicit damage that needs to be cleared?

That's pretty subtle. Instead of this, I think we should make node_needs_traversal smarter. Ideally we'd pass it the SharedStyleContext and it could use the traversal mode to decide to visit if we have a change hint (along with the servo-only case at the bottom).

However, the pre_traverse callsite doesn't have a context yet. So instead, I think we should pass in a TraversalFlags, which the other callsite can grab off the SharedStyleContext. I'm not sure if I mentioned it in this review or on IRC, but I think we should put TraversalFlags on the SharedStyleContext rather than separate bits for each of the things like we have now.

::: servo/ports/geckolib/glue.rs:165
(Diff revision 2)
> +        restyle_for_reconstruct: traversal_flags.for_reconstruct(),
> +    }
> +}
> +
> +fn compute_final_hint_and_propagate_later_siblings(element: GeckoElement, stylist: &Stylist) {
> +    if let Some(data) = element.get_data() {

Why not just do element.mutate_data() directly?

::: servo/ports/geckolib/glue.rs:170
(Diff revision 2)
> +    if let Some(data) = element.get_data() {
> +        let later_siblings = data.borrow_mut().get_restyle_mut()
> +                                 .map_or(false, |r| r.compute_final_hint(element, stylist));
> +        if later_siblings {
> +            if let Some(next) = element.next_sibling_element() {
> +                let mut next_data = unsafe { next.ensure_data() }.borrow_mut();

We don't need to ensure_data() here. If the next element has no styles, then there's certainly no point in restyling it.

::: servo/ports/geckolib/glue.rs:190
(Diff revision 2)
> +    // If we are in a restyle for reconstruction, then we must ensure that any
> +    // later sibling restyle hint is propagated onto element's next sibling
> +    // element, since we are not restyling the entire document. We must expand
> +    // an element snapshot into restyle hints here too, in case that expansion
> +    // results in a later siblings hint.
> +    if traversal_flags.for_reconstruct() {
> +        compute_final_hint_and_propagate_later_siblings(element, &per_doc_data.stylist);
> +    }

Hm, why don't we just do this in pre_traverse where we currently call compute_final_hint?
Attachment #8854428 - Flags: review?(bobbyholley) → review-
Comment on attachment 8854430 [details]
Bug 1351535 - Part 6: Add ServoStyleSet::StyleSubtreeForReconstruct.

https://reviewboard.mozilla.org/r/126380/#review129694

r=me with those fixes.

::: layout/style/ServoStyleSet.cpp:203
(Diff revision 2)
>  void
>  ServoStyleSet::PreTraverse()
>  {
>    PreTraverseSync();
>  
>    // Process animation stuff that we should avoid doing during the parallel
>    // traversal.
>    mPresContext->EffectCompositor()->PreTraverse();
>  }
>  
> +void
> +ServoStyleSet::PreTraverseSubtree(Element* aRoot)
> +{
> +  PreTraverseSync();
> +
> +  // Process animation stuff that we should avoid doing during the parallel
> +  // traversal, but only for the subtree we will be restyling.
> +  mPresContext->EffectCompositor()->PreTraverseInSubtree(aRoot);
> +}

I think these should just be the same function, with a nullable aRoot whose nullness affectse the call we make on EffectCompositor.

::: layout/style/ServoStyleSet.cpp:255
(Diff revision 2)
>          // We're doing initial styling, and the additional animation
>          // traversal changed the styles that were set by the first traversal.
>          // This would normally require a post-traversal to update the style
>          // contexts, and the DOM now has dirty descendant bits and RestyleData
>          // in expectation of that post-traversal. But since this is actually
>          // the initial styling, there are no style contexts to update and no
>          // frames to apply the change hints to, so we don't need to do that
>          // post-traversal. Instead, just drop this state and tell the caller
>          // that no post-traversal is required.
> +
> +        // We're doing initial styling or a restyle for frame reconstruction,
> +        // and the additional animation traversal changed the styles that were
> +        // set by the first traversal. This would normally require a post-
> +        // traversal to update the style contexts, and the DOM now has dirty
> +        // descendant bits and RestyleData in expectation of that post-
> +        // traversal. But when doing an initial styling or when about to
> +        // reframe, there are no style contexts to update and no frames to
> +        // apply the change hints to (and when restyling for reconstruction,
> +        // we don't generate any change hints anyway). This means we don't need
> +        // to do a post-traversal. So instead, just drop this state and tell
> +        // the caller that no post-traversal is required.

Why is this comment duplicated / modified? Given that we're asserting against forReconstruct above, it seems like this should just be left as-is.
Attachment #8854430 - Flags: review?(bobbyholley) → review+
Comment on attachment 8854431 [details]
Bug 1351535 - Part 7: Call StyleSubtreeForReconstruct when doing frame reconstruction.

https://reviewboard.mozilla.org/r/126382/#review129706

::: layout/base/nsCSSFrameConstructor.cpp:7447
(Diff revision 2)
> +nsCSSFrameConstructor::StyleChildRangeForReconstruct(nsIContent* aStartChild,
> +                                                     nsIContent* aEndChild)
> +{
> +  ServoStyleSet* styleSet = mPresShell->StyleSet()->AsServo();
> +
> +  for (nsIContent* child = aStartChild; child != aEndChild;
> +       child = child->GetNextSibling()) {
> +    if (child->IsElement()) {
> +      styleSet->StyleSubtreeForReconstruct(child->AsElement());
> +    }
> +  }
> +}

Add a comment indicating that we need to take the parallelism hit here, because we only want to reconstruct a specific range, and we don't have a great API to pass a range to servo to bound the traversal from the parent.
Attachment #8854431 - Flags: review?(bobbyholley) → review+
Comment on attachment 8854440 [details]
Bug 1351535 - Part 5.1: Move TraversalFlags into SharedStyleContext.

https://reviewboard.mozilla.org/r/126384/#review130586
Attachment #8854440 - Flags: review?(bobbyholley) → review+
Comment on attachment 8854428 [details]
Bug 1351535 - Part 6: Handle TraversalRestyleBehavior::ForReconstruct in the Servo restyle.

https://reviewboard.mozilla.org/r/126376/#review130588
Attachment #8854428 - Flags: review?(bobbyholley) → review+
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7aec59cc05f0
Part 1: Remove some out of date comments in stylo reftest manifest. r=bholley
https://hg.mozilla.org/integration/autoland/rev/cbbedd0b3075
Part 2: Explicitly indicate when a ContentRangeInserted call is for frame reconstruction. r=bholley
https://hg.mozilla.org/integration/autoland/rev/649120253cc1
Part 3: Only explicitly style newly inserted content if the parent is not in a display:none subtree. r=bholley
https://hg.mozilla.org/integration/autoland/rev/186921ff2a55
Part 4: Don't traverse children if the root of the restyle is display:none. r=bholley
https://hg.mozilla.org/integration/autoland/rev/2687cb9cb4de
Part 5: Add a TraversalRestyleBehavior argument to traversal functions. r=bholley
https://hg.mozilla.org/integration/autoland/rev/d44f633899ce
Part 5.1: Move TraversalFlags into SharedStyleContext. r=bholley
https://hg.mozilla.org/integration/autoland/rev/b1a425c54b79
Part 6: Handle TraversalRestyleBehavior::ForReconstruct in the Servo restyle. r=bholley
https://hg.mozilla.org/integration/autoland/rev/c1be168c4c63
Part 7: Add EffectCompositor method to pre-traverse within a specific subtree. r=hiro
https://hg.mozilla.org/integration/autoland/rev/bff89114412c
Part 8: Add ServoStyleSet::StyleSubtreeForReconstruct. r=bholley
https://hg.mozilla.org/integration/autoland/rev/0410ff898157
Part 9: Call StyleSubtreeForReconstruct when doing frame reconstruction. r=bholley
Attachment #8854426 - Attachment is obsolete: true
Attachment #8854440 - Attachment is obsolete: true
Attachment #8854428 - Attachment is obsolete: true
Backout by archaeopteryx@coole-files.de:
https://hg.mozilla.org/mozilla-central/rev/928f3c613164
Backed out changeset 0410ff898157 
https://hg.mozilla.org/mozilla-central/rev/61d718708470
Backed out changeset bff89114412c 
https://hg.mozilla.org/mozilla-central/rev/9350287c5078
Backed out changeset c1be168c4c63 
https://hg.mozilla.org/mozilla-central/rev/8f21c09df004
Backed out changeset b1a425c54b79 
https://hg.mozilla.org/mozilla-central/rev/e6c191fe9a7d
Backed out changeset d44f633899ce 
https://hg.mozilla.org/mozilla-central/rev/421b19e1a862
Backed out changeset 2687cb9cb4de 
https://hg.mozilla.org/mozilla-central/rev/be5401732b78
Backed out changeset 186921ff2a55 
https://hg.mozilla.org/mozilla-central/rev/594c924d62bb
Backed out changeset 649120253cc1 
https://hg.mozilla.org/mozilla-central/rev/8da0f11ebd00
Backed out changeset cbbedd0b3075 
https://hg.mozilla.org/mozilla-central/rev/d52433812f54
Backed out changeset 7aec59cc05f0 on request from developer. r=backout
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0fd626902471
Part 1: Remove some out of date comments in stylo reftest manifest. r=bholley
https://hg.mozilla.org/integration/autoland/rev/206a7c5ddcb1
Part 2: Explicitly indicate when a ContentRangeInserted call is for frame reconstruction. r=bholley
https://hg.mozilla.org/integration/autoland/rev/98731ed693b0
Part 3: Only explicitly style newly inserted content if the parent is not in a display:none subtree. r=bholley
https://hg.mozilla.org/integration/autoland/rev/d35ce8c70ea5
Part 4: Add a TraversalRestyleBehavior argument to traversal functions. r=bholley
https://hg.mozilla.org/integration/autoland/rev/cf6904042b25
Part 5: Add EffectCompositor method to pre-traverse within a specific subtree. r=hiro
https://hg.mozilla.org/integration/autoland/rev/3ce1babe4c79
Part 6: Add ServoStyleSet::StyleSubtreeForReconstruct. r=bholley
https://hg.mozilla.org/integration/autoland/rev/fbbbbb072f1a
Part 7: Call StyleSubtreeForReconstruct when doing frame reconstruction. r=bholley
You need to log in before you can comment on or make changes to this bug.