1059138 - "ASSERTION: Child frames aren't sorted correctly" when a web components custom element mixes shadow-DOM & non-shadow-DOM elements as sibling flex items

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Ting-Yu Chou [:ting] (away)]

Keep seeing this from logcat, not sure how does it impact though:

I/Gecko   (  281): [Parent 281] ###!!! ASSERTION: Child frames aren't sorted correctly: 'nsIFrame::IsFrameListSorted<IsOrderLEQWithDOMFallback>(mFrames)', file ../../../gecko/layout/generic/nsFlexContainerFrame.cpp, line 1846

Gaia: 52ba2a04bf0130f594237ec78eac3a002d4c6203
Gecko: 3bf1a97537d3651304bb158e5cd9a500d9dcb846
Version: 34.0a1

Comment 1

[Daniel Holbert [:dholbert]]

Thanks for the report.  Could you see if you can figure out which app triggers it?

(FWIW, we softened this from MOZ_ASSERT to NS_ASSERTION in bug 876074, because a stylechange-triggered frame reconstruction messes up the frame tree ordering, as discussed in bug 876749.  So, it's possible this is a version of bug 876749.)

Comment 2

[Daniel Holbert [:dholbert]]

(fortunately, this is not dangerous; it just means we may end up with the wrong layout.)

Comment 3

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #1)
> Thanks for the report.  Could you see if you can figure out which app
> triggers it?

I just made a debug B2G trunk build for my unagi, and I can confirm I see this spammed several times during bootup, and then several times each time I turn the screen on after the phone has booted.  (just viewing the lock screen)

If I unlock the phone and view the homescreen, the assertion spams continuously to logcat, about twice a second.

So, this is likely triggered by something in the status bar, the home screen, or the lock screen.

Comment 4

[Daniel Holbert [:dholbert]]

Poking at this right now -- the app in question is app://system.gaiamobile.org/index.html, and I see failures even before the "based on mozilla technology" red-square splash screen appears during bootup.

Comment 6

[Daniel Holbert [:dholbert]]

In a lot of the times this is hit, the flex container here is inside of a div with id "crash-reports-header".

The flex container itself has no ID, but it has 2 children: a <button> and a <h1> element.

Given that, I'm guessing this was introduced by this change:
> <!-- "Crash Reports" information page -->
>- <section role="region" class="skin-dark">
>- <header>
>- <button id="crash-reports-close"><span class="icon icon-close">close</span></button>
>+ <section role="region">
>+ <gaia-header id="crash-reports-header" action="close">
><h1 data-l10n-id="crashReports">Crash Reports</h1>
https://github.com/mozilla-b2g/gaia/commit/2081541a2177f42d99d205c4350f399e648d0c20#diff-61c16a0a0190e95d51031835f62b30d7L649

Note that we removed the explicit <button> from this element, and we're now using some magic to insert it, apparently in a way where the DOM ends up reversed with respect to the frame tree, or something.

(That change was for bug 1016814, which landed yesterday; that makes a lot of sense, timeline-wise, given that we've now had two instances of this bug reported today.)

Comment 7

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #6)
> Note that we removed the explicit <button> from this element, and we're now
> using some magic to insert it, apparently in a way where the DOM ends up
> reversed with respect to the frame tree, or something.

I'm guessing that magic is web components, based on bug 1016814 comment 11. I'd expect we should be able to trigger this using a reduced testcase of some sort, w/ web components, in desktop builds as well.

Comment 8

[Daniel Holbert [:dholbert]]

Yeah, so we have 2 sibling frames, and one of them has its mContent.mParent.mParent being an instance of ShadowRoot, and the ShadowRoot's "mParent" is nullptr. (So, it's in a web component shadow DOM.)

This means nsContentUtils::PositionIsBefore() fails to compare these frames' DOM nodes' positions, since it uses nsINode::CompareDocumentPosition(), which tries to find the nearest common ancestor of the two frames (and there is no common ancestor).

We probably need to do something smarter in IsOrderLEQWithDOMFallback() [1] to handle cases where one of the flex items is in a shadow DOM and the other is not. (I'm not sure if they could also be in *two different* shadow DOMs...? I suspect that's not possible, for sibling frames, but I don't know web components well enough to be sure.)

[1]http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFlexContainerFrame.cpp?rev=aece7f9f944c#852

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: testcase 1 (need to enable dom.webcomponents.enabled)

I can reproduce this in my desktop build with "dom.webcomponents.enabled" turned on.

Comment 10

[Daniel Holbert [:dholbert]]

bz, do you know if there's any equivalent of nsContentUtils::PositionIsBefore() that works on elements in a shadow DOM? (where one of the elements may be from <content></content> and hence won't have the ShadowRoot as its ancestor)

Comment 11

[Daniel Holbert [:dholbert]]

Attachment: band-aid v1

In the meantime, here's a band-aid to silence this assertion when the first child is part of a shadow DOM (which is the case with the 'gaia-header' custom element that's triggering this on B2G).

I think it'd be worth landing this ASAP, to keep this from dominating logcat output for dogfooders / testers.  We can remove this hack when we've got a real fix to land (hopefully in the next few days).

Comment 12

[Boris Zbarsky [:bzbarsky]]

> I suspect that's not possible, for sibling frames

It's possible.  Consider <div><span/><div> with a web component applied to the <div> that has

  <p>Text1<content/>Text2</p>

as its shadow DOM and the <p> has a web component applied that has

  <pre><i/><content/><b/></pre>

as its shadow DOM.

Then the composed tree looks like this (with some whitespace inserted for readability):

  <div>
    <p>
      <pre>
        <i/>Text1<span/>Text2<b/>
      </pre>
    </p>
  </div>

> do you know if there's any equivalent of nsContentUtils::PositionIsBefore()

No.  Maybe wchen does?  We should implement something for sure.

Comment 13

[Daniel Holbert [:dholbert]]

Landed band-aid: https://hg.mozilla.org/integration/mozilla-inbound/rev/6dd4ea56ab11

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6dd4ea56ab11

Comment 15

[William Chen [:wchen]]

There currently isn't a PositionIsBefore equivalent that accounts for shadow DOM.

Comment 16

[Daniel Holbert [:dholbert]]

Heads-up: we ran into this same issue (shadow dom breaking our ability to sort & check sortedness in a flex container) in bug 1219153.  I'm likely going to soften an assertions over there as a result, but really we need to make this work...

wchen, do you have a rough idea of what we'd need to do here, to make this work?  The setup is that we've got two frames, and we want to compare their "effective" DOM positions to find out which one is earlier.

Either or both could be part of a shadow DOM, which means PositionIsBefore doesn't directly work, since it walks off the ShadowRoot and thinks the frames are disconnected.  Is it feasible to extend PositionIsBefore to work here? (i.e. can we get from the ShadowRoot to the "correct" parent node, or is there like one shared ShadowRoot and we have no idea which of its parents is the right one?)  And if PositionIsBefore can't work, what would we need to do, to find the actual correct parent, given the frame and/or content node in question?

Comment 17

[Daniel Holbert [:dholbert]]

This is now a non-issue, as of bug 812687. We now leave the flex item child list unmodified (so, in correct DOM order by default), and we use CSSOrderAwareFrameIterator to traverse them, which means we don't have to do any DOM position comparisons anymore.

Note that this bug's "band-aid" code was replaced in a commit for bug 812687, too -- here:
  https://hg.mozilla.org/mozilla-central/rev/173a4f49dfe3db98a490a037db937cb06da5170d#l1.305

Resolving as FIXED by bug 812687.

Comment 18

[Daniel Holbert [:dholbert]]

ni=me to be sure we land the testcase as a crashtest

Comment 19

[Daniel Holbert [:dholbert]]

Try run of crashtest: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9becc164916ec980615415f248c07ca96a44065d

Comment 20

[Daniel Holbert [:dholbert]]

Disregard the Try run in comment 19 -- that was based on top of a leaky mozilla-inbound cset. I'll rebase on a better cset and try again.

Also: I verified that the crashtest does effectively trigger the assertion, from the crashtest harness, in affected builds.

I updated to the parent of bug 812687's fixes, and then reverted the band-aid patch (comment 13), and then applied the crashtest patch that I just ran through Try (directly using attachment 8480223 [details] as a crashtest, with a "pref()" line to enable dom.webcomponents.enabled from crashtests.list).  I used my own custom crashtest manifest file to just run this test on its own, for local testing.

That did indeed trigger the assertion and "TEST-UNEXPECTED-FAIL".

> REFTEST SUITE-START | Running 1 tests
> REFTEST TEST-START | file:///scratch/work/builds/mozilla-inbound/mozilla/layout/generic/crashtests/1059138-1.html
> REFTEST INFO | SET PREFERENCE pref(dom.webcomponents.enabled,true)
> REFTEST TEST-LOAD | file:///scratch/work/builds/mozilla-inbound/mozilla/layout/generic/crashtests/1059138-1.html | 0 / 1 (0%)
> [Child 24045] ###!!! ASSERTION: Child frames aren't sorted correctly: 'nsIFrame::IsFrameListSorted<IsOrderLEQWithDOMFallback>(mFrames)', file [...]nsFlexContainerFrame.cpp, line 2451
> REFTEST TEST-PASS | file:///scratch/work/builds/mozilla-inbound/mozilla/layout/generic/crashtests/1059138-1.html | (LOAD ONLY)
> REFTEST TEST-END | file:///scratch/work/builds/mozilla-inbound/mozilla/layout/generic/crashtests/1059138-1.html
> REFTEST TEST-UNEXPECTED-FAIL | file:///scratch/work/builds/mozilla-inbound/mozilla/layout/generic/crashtests/1059138-1.html | assertion count 1 is more than expected 0 assertions
> REFTEST INFO | RESTORE PREFERENCE pref(dom.webcomponents.enabled,false)

For some reason, though (in this old/patched build), the test gets reported *both* as a TEST-PASS *and* as TEST-UNEXPECTED-FAIL, as you can see above. And at the end of the summary, it looks like the "pass" is what counted:

> REFTEST INFO | Successful: 1 (0 pass, 1 load only)
> REFTEST INFO | Unexpected: 0 (0 unexpected fail, 0 unexpected pass, 0 unexpected asserts, 0 failed load, 0 exception)

I have no idea why this happens. I tried including some other crashtests before/after, and I also tried adding "reftest-wait" in case there was a race condition and the assertion was being spammed after we'd decided the test had completed.  But neither of those changed the outcome.

I'm just chalking this up to something that was broken in the test harness a while back that has hopefully been fixed now.  And in any case, the main thing I wanted to verify was that the test still asserted, when run from the crashtest harness, in a buggy build -- and it does indeed.

Comment 21

[Daniel Holbert [:dholbert]]

hopefully-better try run (on recent mozilla-inbound):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=44b882368b01ac4b5477645297648d752d5fa477

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/90b3cc6e0191
add this bug's testcase as a crashtest. (no review, test-only)

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/90b3cc6e0191