Closed Bug 1133615 Opened 5 years ago Closed 4 years ago

"ASSERTION: expected descendants to be handled by now"

Categories

(Core :: CSS Parsing and Computation, defect)

x86_64
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: jruderman, Assigned: heycam)

References

(Blocks 2 open bugs)

Details

(Keywords: assertion, regression, testcase)

Attachments

(3 files, 2 obsolete files)

Attached file testcase
###!!! ASSERTION: expected descendants to be handled by now: 'aData->mDescendants.IsEmpty()', file layout/base/RestyleTracker.cpp, line 372

Maybe a regression from the patch that added the assertion, https://hg.mozilla.org/mozilla-central/rev/8fae88effe0d in bug 931668.
Attached file stack
The restyles caused by the DOM manipulations are:

    // For the removal of c, we add (eRestyle_Subtree | eRestyle_LaterSiblings),
    // then add it as a restyle root.  Then because it's a <style>, we add
    // eRestyle_Subtree on the root element and add it as a restyle root.
    // Nothing needs to be added for the append of c.  But because it's
    // a <style>, we again add eRestyle_Subtree on the root element, but we
    // already have that.
    x.appendChild(c);

    // For the removal of y, we add (eRestyle_Subtree | eRestyle_LaterSiblings)
    // for c.  We also find c's parent, x, as the closest existing restyle root,
    // so we add c to x's mDescendants.
    x.removeChild(y);

So we have something in an mDescendants array.  But when we come to process the restyles, we rebuild all styles (due to the style sheet changes).  Rebuilding all styles doesn't do the same expansion of eRestyle_LaterSiblings restyle hints (which are unnecessary if we know we're restyling the entire tree), which means that when GetRestyleData() is called upon traversing down to x, it finds an eRestyle_LaterSibling it believes was added after the RestyleData would have already been processed and cleared out, but in reality is the first time it's encountering it.

I think we can just adjust the assertion so it doesn't fail if we're in a full style rebuild.
Attached patch patchSplinter Review
Assignee: nobody → cam
Status: NEW → ASSIGNED
Attachment #8718717 - Flags: review?(dbaron)
Brendan, does this avoid the assertion firing for your bug 1104916 patches?
Flags: needinfo?(bdahl)
(In reply to Cameron McCormack (:heycam) from comment #4)
> Brendan, does this avoid the assertion firing for your bug 1104916 patches?

Yes!
Flags: needinfo?(bdahl)
Why is this specific to a rebuild-all, and not something that can also happen if you have an ancestor restyle in mPendingRestyles after (in the array, so it gets processed first) a descendant restyle with an mDescendants array?
Flags: needinfo?(cam)
Hmm, to be honest, I'm not even sure how you can get into the situation (during a normal, non-rebuild-all restyle) where we've started restyling (and thus done an initial expansion of eRestyle_LaterSiblings), but then before we're finished we find an eRestyle_LaterSiblings hint has been put back.  Do you have an example of when that happens?
Flags: needinfo?(cam) → needinfo?(dbaron)
Ah, ok, I see now that this is specific to BeginProcessingRestyles doing restyling work, since the code at the beginning of the loop in RestyleTracker::DoProcessRestyles expands *all* of the later sibling restyles.  (Maybe we should try to find another solution to the root frame problem that StartRebuildAllStyleData is solving...)


So then, the other question:  does it cause bugs when we hit this assertion (in its original form)?  Can we end up dropping restyles in mDescendants on the floor?
Flags: needinfo?(dbaron) → needinfo?(cam)
I think that comes down to whether we can ever get in here when not in a rebuild-all-styles.  Doing a try run with an assertion to check that now:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=23dd9fca6c9d
(That is, when we hit the assertion as it is currently, during a rebuild-all-styles, it's safe to drop the mDescendants since we know for sure we'll restyle the entire document.  It probably would be a bug to drop mDescendants, if it has entries in it, if we trigger the current assertion during a normal restyle.)
(In reply to Cameron McCormack (:heycam) (away Feb 27 – Mar 13) from comment #9)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=23dd9fca6c9d

The assertion didn't fire, so maybe we don't need that block dealing with eRestyle_LaterSiblings at all.
Flags: needinfo?(cam)
Attached patch patch v2 (obsolete) — Splinter Review
https://treeherder.mozilla.org/#/jobs?repo=try&revision=166649779e5a
Attachment #8718717 - Attachment is obsolete: true
Attachment #8718717 - Flags: review?(dbaron)
Attachment #8721525 - Flags: review?(dbaron)
(In reply to Cameron McCormack (:heycam) (away Feb 27 – Mar 13) from comment #10)
> (That is, when we hit the assertion as it is currently, during a
> rebuild-all-styles, it's safe to drop the mDescendants since we know for
> sure we'll restyle the entire document.  It probably would be a bug to drop
> mDescendants, if it has entries in it, if we trigger the current assertion
> during a normal restyle.)

I'm not sure I buy this.  Some RebuildAllStyleData calls are done with nsRestyleHint(0), since we need to rebuild the rule tree without rerunning selector matching.  What happens if we have mDescendants queued at that time?
Flags: needinfo?(cam)
(In reply to David Baron [:dbaron] ⌚️UTC-8 from comment #13)
> I'm not sure I buy this.  Some RebuildAllStyleData calls are done with
> nsRestyleHint(0), since we need to rebuild the rule tree without rerunning
> selector matching.  What happens if we have mDescendants queued at that time?

StartRebuildAllStyleData adds eRestyle_ForceDescendants, so that should guarantee that we do indeed restyle the entire document.  When we append an entry to mDescendants to an existing restyle root ancestor, we'll have added the target element's restyle data to the pending restyle table, so we should know that we'll come to it during the rebuild-all-style.
Flags: needinfo?(cam)
Attached patch patch v2.1 (obsolete) — Splinter Review
Something that actually compiles.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=31b1374dbfae
Attachment #8721525 - Attachment is obsolete: true
Attachment #8721525 - Flags: review?(dbaron)
Attachment #8721550 - Flags: review?(dbaron)
OK, now that I remember that mDescendants was added because restyling one element doesn't imply traversing all its descendants anymore, that seems ok.

But I'm still worried about the chunk of code you're removing.  How do we know that it's safe to remove that?  What about the case of having an eRestyle_LaterSiblings posted, and then processing it during a RebuildAllStyleData that was called with nsRestyleHint(0)?  (Both of those things are rare enough that I could certainly believe we don't hit both at once in our tests.)
(In reply to David Baron [:dbaron] ⌚️UTC-8 from comment #16)
> But I'm still worried about the chunk of code you're removing.  How do we
> know that it's safe to remove that?  What about the case of having an
> eRestyle_LaterSiblings posted, and then processing it during a
> RebuildAllStyleData that was called with nsRestyleHint(0)?  (Both of those
> things are rare enough that I could certainly believe we don't hit both at
> once in our tests.)

Ah, and the eRestyle_LaterSiblings needs to be expanded into eRestyle_Subtrees for those later siblings, doesn't it.  I think then you're right we should leave it.  (The alternative would be to do the eRestyle_LaterSiblings expansion explicitly before we start the rebuild-all-style, and then remove this block, but I guess it doesn't really buy much to do that.)
Flags: needinfo?(cam)
Attachment #8721550 - Flags: review?(dbaron)
Comment on attachment 8718717 [details] [diff] [review]
patch

I think I'm happy with the first version of the patch now, though, although it might be worth extending the comment to point out that the reason it's OK in a RebuildAll is that the RebuildAll adds a ForceDescendants.
Attachment #8718717 - Flags: review+
Attachment #8718717 - Attachment is obsolete: false
Attachment #8721550 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/712bfced88a6
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.