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

RESOLVED FIXED in Firefox 52

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: heycam, Assigned: heycam)

Tracking

unspecified
mozilla52
Points:
---

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
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.
(Assignee)

Comment 1

2 years ago
Created attachment 8808520 [details] [diff] [review]
patch
Attachment #8808520 - Flags: review?(ecoal95)
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!
Attachment #8808520 - Flags: review?(ecoal95) → review+
(Assignee)

Comment 3

2 years ago
(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

2 years ago
Pushed by cmccormack@mozilla.com:
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fc55188f14b6
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Blocks: 1330412
You need to log in before you can comment on or make changes to this bug.