1133615 - "ASSERTION: expected descendants to be handled by now"

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Jesse Ruderman]

Attachment: 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.

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Cameron McCormack (:heycam)]

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.

Comment 3

[Cameron McCormack (:heycam)]

Attachment: patch

Comment 4

[Cameron McCormack (:heycam)]

Brendan, does this avoid the assertion firing for your bug 1104916 patches?

Comment 5

[Brendan Dahl [:bdahl]]

(In reply to Cameron McCormack (:heycam) from comment #4)
> Brendan, does this avoid the assertion firing for your bug 1104916 patches?

Yes!

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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?

Comment 7

[Cameron McCormack (:heycam)]

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?

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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?

Comment 9

[Cameron McCormack (:heycam)]

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

Comment 10

[Cameron McCormack (:heycam)]

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

Comment 11

[Cameron McCormack (:heycam)]

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

Comment 12

[Cameron McCormack (:heycam)]

Attachment: patch v2

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

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 14

[Cameron McCormack (:heycam)]

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

Comment 15

[Cameron McCormack (:heycam)]

Attachment: patch v2.1

Something that actually compiles.

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

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 17

[Cameron McCormack (:heycam)]

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

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/712bfced88a6

Comment 20

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/712bfced88a6