1315894 - stylo: clear dirty bits on entire subtree when stopping in RecreateStyleContexts due to no frame or ReconstructFrame hint

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Cameron McCormack (:heycam)]

When we stop traversing the tree in ServoRestyleManager::RecreateStyleContexts due to encountering an element with no frame, or due to having an nsChangeHint_ReconstructFrame change hint, we currently just clear the IsDirtyForServo bit on the element.  However, as we have been traversing down the tree in RecreateStyleContexts, we've been clearing the HasDirtyDescendantsForServo bit.  For this to be correct, we need to ensure that no elements in the entire subtree whose root we stop at have the dirty / dirty descendants bits set.

Comment 1

[Cameron McCormack (:heycam)]

Attachment: patch

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8808520 [details] [diff] [review]
patch

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

Looks good, though probably we should open a bug to do this in the frame constructor when possible instead.

::: layout/base/ServoRestyleManager.cpp
@@ +77,5 @@
> +MarkSelfAndDescendantsAsNotDirtyForServo(nsIContent* aContent)
> +{
> +  aContent->UnsetIsDirtyForServo();
> +
> +  if (aContent->HasDirtyDescendantsForServo()) {

nit: I use to prefer early-returning here to avoid rightward drift, but this is fine too.

@@ +159,5 @@
>      //
>      // Note that we must leave the old style on an existing frame that is
>      // about to be reframed, since some frame constructor code wants to
>      // inspect the old style to work out what to do.
>      if (!primaryFrame || (changeHint & nsChangeHint_ReconstructFrame)) {

Is this also needed if there's no change hint? That'd be kind of unfortunate, but I think it's true.

::: layout/style/crashtests/1315894-1.html
@@ +6,5 @@
> +document.querySelector("div").style.display = "inline";
> +dump("#2\n");
> +document.body.offsetWidth;
> +dump("#3\n");
> +document.querySelector("span").style.color = "blue";

So the crash was something like this?

When we arrive to this line, we see that the element is already dirty, so we somehow bailout and restyle without the root being marked as having dirty descendants?

If it's not that, could you elaborate a bit either as a comment in the bug or in the commit message?

Thanks!

Comment 3

[Cameron McCormack (:heycam)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #2)
> ::: layout/base/ServoRestyleManager.cpp
> @@ +77,5 @@
> > +MarkSelfAndDescendantsAsNotDirtyForServo(nsIContent* aContent)
> > +{
> > +  aContent->UnsetIsDirtyForServo();
> > +
> > +  if (aContent->HasDirtyDescendantsForServo()) {
> 
> nit: I use to prefer early-returning here to avoid rightward drift, but this
> is fine too.
> 
> @@ +159,5 @@
> >      //
> >      // Note that we must leave the old style on an existing frame that is
> >      // about to be reframed, since some frame constructor code wants to
> >      // inspect the old style to work out what to do.
> >      if (!primaryFrame || (changeHint & nsChangeHint_ReconstructFrame)) {
> 
> Is this also needed if there's no change hint? That'd be kind of
> unfortunate, but I think it's true.

Do you mean, if primaryFrame is null and changeHint is empty?  Then... maybe?  I'm not sure.  If a try run limiting the MarkSelfAndDescendantsAsNotDirtyForServo call to the ReconstructFrame case is sufficient, I'll do that, and add a comment.

> ::: layout/style/crashtests/1315894-1.html
> @@ +6,5 @@
> > +document.querySelector("div").style.display = "inline";
> > +dump("#2\n");
> > +document.body.offsetWidth;
> > +dump("#3\n");
> > +document.querySelector("span").style.color = "blue";

(I'll remove those dump() calls in the patch I land.)

> So the crash was something like this?
> 
> When we arrive to this line, we see that the element is already dirty, so we
> somehow bailout and restyle without the root being marked as having dirty
> descendants?
> 
> If it's not that, could you elaborate a bit either as a comment in the bug
> or in the commit message?

The problem was:

* We start off with this:

    <body>       IsDirty = false, HasDirtyDescendants = false
      <div>      IsDirty = false, HasDirtyDescendants = false
        <span>   IsDirty = false, HasDirtyDescendants = false

  and with neither the div nor the span having a frame.

* We do div.style.display = "inline" and then flush.  After the NoteRestyleHint call during flushing, we have:

    <body>       IsDirty = false, HasDirtyDescendants = true
      <div>      IsDirty = true,  HasDirtyDescendants = false
        <span>   IsDirty = false, HasDirtyDescendants = false

as expected.

* Then we call ServoStyleSet::StyleDocument, to compute styles for the dirty elements.  Here, we end up computing new styles for the <span> and generating a ReconstructFrame change hint for it, since it previously had no frame/styles and now it has some.  (This happens in compute_restyle_damage.)  So after StyleDocument, we have:

    <body>       IsDirty = false, HasDirtyDescendants = true
      <div>      IsDirty = true,  HasDirtyDescendants = true
        <span>   IsDirty = true, HasDirtyDescendants = false

* In RecreateStyleContexts, we traverse down the tree, and we reach the div.  For its new style, we generate a ReconstructFrame hint, append it to the list of change hints to process, and return early.  (We would have returned early due to the div not having a frame yet, even before the bug 1315889 change that would also make us return early due to having generated ReconstructFrame.)

* At the return early point -- which is the part this patch changes -- we just unset the IsDirty bit for the div, and as we unwind the recursive RecreateStyleContexts calls, we clear the HasDirtyDescendants bits as we go.  Once we're back up in ProcessPendingRestyles, just after the RecreateStyleContexts call, we have:

    <body>       IsDirty = false, HasDirtyDescendants = false
      <div>      IsDirty = false, HasDirtyDescendants = true
        <span>   IsDirty = true,  HasDirtyDescendants = false

So here is where we have the inconsistent bits set, and at some later point we end up doing some assertions and they fail.


So probably I could summarise that with the following, at the early return point I'm changing:

  // Since we might still have some dirty bits set on descendants, inconsistent with the clearing
  // of HasDirtyDescendants we will do as we return from these recursive RecreateStyleContexts
  // calls, we explicitly clear them here.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc55188f14b6
Clear dirty bits on entire subtree when stopping in RecreateStyleContexts due to no frame or ReconstructFrame hint. r=emilio

Comment 5

[Carsten Book [:Tomcat]]

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