Closed
Bug 1059138
Opened 10 years ago
Closed 7 years ago
"ASSERTION: Child frames aren't sorted correctly" when a web components custom element mixes shadow-DOM & non-shadow-DOM elements as sibling flex items
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla55
People
(Reporter: ting, Assigned: dholbert)
References
Details
(Keywords: assertion, testcase)
Attachments
(2 files)
1.06 KB,
text/html
|
Details | |
1.66 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
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
Updated•10 years ago
|
Flags: needinfo?(dholbert)
Assignee | ||
Comment 1•10 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•10 years ago
|
||
(fortunately, this is not dangerous; it just means we may end up with the wrong layout.)
Keywords: assertion
Assignee | ||
Updated•10 years ago
|
Keywords: testcase-wanted
Assignee | ||
Comment 3•10 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•10 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 | ||
Comment 6•10 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•10 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•10 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•10 years ago
|
||
I can reproduce this in my desktop build with "dom.webcomponents.enabled" turned on.
Assignee | ||
Comment 10•10 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•10 years ago
|
Keywords: testcase-wanted → testcase
Assignee | ||
Comment 11•10 years ago
|
||
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)
Comment 12•10 years ago
|
||
> 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)
Updated•10 years ago
|
Attachment #8480245 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Updated•10 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
Assignee | ||
Comment 13•10 years ago
|
||
Landed band-aid: https://hg.mozilla.org/integration/mozilla-inbound/rev/6dd4ea56ab11
Flags: in-testsuite?
Keywords: leave-open
Comment 14•10 years ago
|
||
Comment 15•10 years ago
|
||
There currently isn't a PositionIsBefore equivalent that accounts for shadow DOM.
Flags: needinfo?(wchen)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Assignee | ||
Comment 16•9 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•9 years ago
|
Flags: needinfo?(wchen)
Assignee | ||
Comment 17•7 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
Closed: 7 years ago
Depends on: 812687
Flags: needinfo?(wchen)
Keywords: leave-open
Resolution: --- → FIXED
Updated•7 years ago
|
status-firefox54:
--- → wontfix
status-firefox55:
--- → fixed
status-firefox-esr52:
--- → wontfix
Target Milestone: --- → mozilla55
Assignee | ||
Comment 18•7 years ago
|
||
ni=me to be sure we land the testcase as a crashtest
Flags: needinfo?(dholbert)
Comment hidden (obsolete) |
Assignee | ||
Comment 20•7 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.
Assignee | ||
Comment 21•7 years ago
|
||
hopefully-better try run (on recent mozilla-inbound):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=44b882368b01ac4b5477645297648d752d5fa477
Comment 22•7 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)
Comment 23•7 years ago
|
||
bugherder |
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(dholbert)
You need to log in
before you can comment on or make changes to this bug.
Description
•