Closed Bug 1409088 Opened 4 years ago Closed 4 years ago

stylo: Bloom filter depth assertion in layout/reftests/webcomponents/fallback-content-1.html

Categories

(Core :: CSS Parsing and Computation, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

Details

Attachments

(2 files)

Attached file Stack
'assertion failed: `(left == right)`, left == 4, right == 5

This is probably a DOM bug actually.
Same in dom/tests/mochitest/webcomponents/test_style_fallback_content.html
Priority: -- → P3
Comment on attachment 8920123 [details]
Bug 1409088: Fix destination insertion point removal algorithm.

https://reviewboard.mozilla.org/r/191116/#review196474

The assert is probably OK.  I doubt it's worse than the style context verification stuff Gecko's style system does...

r=me

::: commit-message-d8059:7
(Diff revision 2)
> +
> +When an insertion point (a) is added to the document before another insertion
> +point (b), and that insertion point matches nodes that used to match (b), the
> +following happens in RedistributeAllNodes:
> +
> + * Loop through (a), and clear the existing insertion points (none, since it was

Clear the existing insertion points on what?  Nodes distributed into (a)?

::: commit-message-d8059:14
(Diff revision 2)
> +
> + * Go through the node pool and add the matched nodes. That makes the node
> +   (which already had (b) in the insertion point array) have [(b), (a)] as the
> +   insertion points.
> +
> + * Go through (b), and clear the existing insertion points. That used to do

Again, clear on what?  We're presumably only doing this on nodes that are no longer distributed into (b)?  And even then, we're not "clearing" in general, but removing (b) and any "downstream" insertion points, right?

::: dom/base/ShadowRoot.cpp:241
(Diff revision 2)
>  void
>  ShadowRoot::RemoveDestInsertionPoint(nsIContent* aInsertionPoint,
>                                       nsTArray<nsIContent*>& aDestInsertionPoints)
>  {
>    // Remove the insertion point from the destination insertion points.
>    // Also remove all succeeding insertion points because it is no longer

This comment is no longer true.  I assume we _do_ clear out those deeer insertion points somewhere, right?  Assuming we do, adjust this comment to explain where?
Attachment #8920123 - Flags: review?(bzbarsky) → review+
Comment on attachment 8920123 [details]
Bug 1409088: Fix destination insertion point removal algorithm.

https://reviewboard.mozilla.org/r/191116/#review196474

> Clear the existing insertion points on what?  Nodes distributed into (a)?

Yup, adjusted the commit message.

> Again, clear on what?  We're presumably only doing this on nodes that are no longer distributed into (b)?  And even then, we're not "clearing" in general, but removing (b) and any "downstream" insertion points, right?

Right, this is exactly the bug that I'm fixing, we were incorrectly "clearing".

> This comment is no longer true.  I assume we _do_ clear out those deeer insertion points somewhere, right?  Assuming we do, adjust this comment to explain where?

Adjusted indeed.
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/2011b90ddb79
Fix destination insertion point removal algorithm. r=bz
That commit still has the confusing comments about "clear the existing insertion points on the nodes distributed to it" for (b) and the "remove all succeeding insertion points" comment, no?
Flags: needinfo?(emilio)
So if you look at https://reviewboard.mozilla.org/r/191116/diff/2-3/, I reworded it a bit to try to clarify that the destination insertion point is cleared from the nodes distributed to it. I guess it could indeed be more clear, saying that it doesn't clear all of them, but removes the relevant insertion point from the list...

I guess I can add a big comment about why not to call SetLength, but that seemed more related to the change rather than the code itself...

Let me know if you want me to back out + reland with a reworded commit message, or expand the comment, can do any or both.
Flags: needinfo?(emilio) → needinfo?(bzbarsky)
Oh, gah, part of this is the commit message, right...

So the problem I have with the commit message is hat we're clearing on the nodes that _used_ to be distributed to (b) but aren't anymore, which is not what the commit message says...

I can live with the commit message thing, but please do fix the code comment, ok?
Flags: needinfo?(bzbarsky)
Gah, of course you were talking about the comment on top of the `index` declaration, sorry for missing that (thought you were talking about the one on top of `if (index >= 0)`...

Sure, that's pretty inaccurate now, will fix it up.
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/36a034813888
followup: Reword no longer accurate part of the comment in RemoveDestInsertionPoint. r=me
https://hg.mozilla.org/mozilla-central/rev/2011b90ddb79
https://hg.mozilla.org/mozilla-central/rev/36a034813888
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Assignee: nobody → emilio
You need to log in before you can comment on or make changes to this bug.