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

RESOLVED FIXED in Firefox 55

Status

()

defect
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: ting, Assigned: dholbert)

Tracking

({assertion, testcase})

Trunk
mozilla55
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox-esr52 wontfix, firefox54 wontfix, firefox55 fixed)

Details

Attachments

(2 attachments)

Reporter

Description

5 years ago
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
Flags: needinfo?(dholbert)
Assignee

Comment 1

5 years ago
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.)
Depends on: 876074
Flags: needinfo?(dholbert) → needinfo?(tchou)
Assignee

Comment 2

5 years ago
(fortunately, this is not dangerous; it just means we may end up with the wrong layout.)
Keywords: assertion
Assignee

Updated

5 years ago
Keywords: testcase-wanted
Assignee

Comment 3

5 years ago
(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.
Flags: needinfo?(tchou)
Assignee

Comment 4

5 years ago
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.
Assignee

Updated

5 years ago
Duplicate of this bug: 1059496
Assignee

Comment 6

5 years ago
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.)
Depends on: 1016814
Assignee

Comment 7

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

Comment 8

5 years ago
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
Assignee

Comment 9

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

Comment 10

5 years ago
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)
Flags: needinfo?(bzbarsky)
Assignee

Updated

5 years ago
Assignee

Comment 11

5 years ago
Posted patch band-aid v1Splinter Review
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).
Attachment #8480245 - Flags: review?(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.
Flags: needinfo?(bzbarsky) → needinfo?(wchen)
Attachment #8480245 - Flags: review?(bzbarsky) → review+
Assignee

Updated

5 years ago
OS: Gonk (Firefox OS) → All
Hardware: ARM → All
Summary: ASSERTION: Child frames aren't sorted correctly → "ASSERTION: Child frames aren't sorted correctly" when a web components custom element mixes shadow-DOM & non-shadow-DOM elements as sibling flex items
There currently isn't a PositionIsBefore equivalent that accounts for shadow DOM.
Flags: needinfo?(wchen)
Assignee

Updated

5 years ago
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Assignee

Updated

4 years ago
See Also: → 1221112
Assignee

Updated

4 years ago
See Also: → 1219153
Assignee

Updated

4 years ago
Blocks: 1219153
Assignee

Comment 16

4 years ago
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?
Assignee

Updated

4 years ago
Flags: needinfo?(wchen)
Assignee

Comment 17

2 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Depends on: 812687
Flags: needinfo?(wchen)
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Assignee

Comment 18

2 years ago
ni=me to be sure we land the testcase as a crashtest
Flags: needinfo?(dholbert)
Assignee

Comment 20

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

2 years ago
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/90b3cc6e0191
add this bug's testcase as a crashtest. (no review, test-only)
Assignee

Updated

2 years ago
Flags: needinfo?(dholbert)
You need to log in before you can comment on or make changes to this bug.