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)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox-esr52 --- wontfix
firefox54 --- wontfix
firefox55 --- fixed

People

(Reporter: ting, Assigned: dholbert)

References

Details

(Keywords: assertion, testcase)

Attachments

(2 files)

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)
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)
(fortunately, this is not dangerous; it just means we may end up with the wrong layout.)
Keywords: assertion
Keywords: testcase-wanted
(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)
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.
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
(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.
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
I can reproduce this in my desktop build with "dom.webcomponents.enabled" turned on.
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)
Attached 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+
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: nobody → dholbert
Status: NEW → ASSIGNED
See Also: → 1221112
See Also: → 1219153
Blocks: 1219153
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?
Flags: needinfo?(wchen)
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
Target Milestone: --- → mozilla55
ni=me to be sure we land the testcase as a crashtest
Flags: needinfo?(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.
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)
Flags: needinfo?(dholbert)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: