"ASSERTION: How did that happen?" in FrameConstructionItem::IsWhitespace
RESOLVED
FIXED
in mozilla30
Status
()
People
(Reporter: jruderman, Assigned: dholbert)
Tracking
(Blocks 1 bug, {assertion, regression, testcase})
Firefox Tracking Flags
(Not tracked)
Details
Attachments
(4 attachments, 1 obsolete attachment)
###!!! ASSERTION: How did that happen?: 'aState.mCreatingExtraFrames || !mContent->GetPrimaryFrame()', file layout/base/nsCSSFrameConstructor.cpp, line 11160
|
Reporter | |
Comment 1•5 years ago
|
||
|
||
Comment 2•5 years ago
|
||
Looks like a regression from bug 969460. ###!!! ASSERTION: How did that happen?: 'aState.mCreatingExtraFrames || !mContent->GetPrimaryFrame()' 11157 FrameConstructionItem::IsWhitespace(nsFrameConstructorState& aState) const 11158 { 11159 NS_PRECONDITION(aState.mCreatingExtraFrames || 11160 !mContent->GetPrimaryFrame(), "How did that happen?"); #3 nsCSSFrameConstructor::FrameConstructionItem::IsWhitespace #4 nsCSSFrameConstructor::FrameConstructionItemList::Iterator::SkipWhitespace #5 nsCSSFrameConstructor::CreateNeededAnonFlexItems #6 nsCSSFrameConstructor::ConstructFramesFromItemList #7 nsCSSFrameConstructor::ProcessChildren #8 nsCSSFrameConstructor::ConstructDocElementFrame #9 nsCSSFrameConstructor::ContentRangeInserted ... The mContent->GetPrimaryFrame() is the nsFlexContainerFrame root frame that ConstructDocElementFrame setup before calling ProcessChildren: http://hg.mozilla.org/mozilla-central/annotate/2bddbd180d2d/layout/base/nsCSSFrameConstructor.cpp#l2467
|
|
||
Comment 3•5 years ago
|
||
We should probably just relax this assertion condition a bit to allow this case.
|
Assignee | |
Comment 4•5 years ago
|
||
Here's a slightly-reduced testcase. A few notes: - If I drop the <head></head>, we don't assert. - If I drop the space between </head> and <body>, we don't assert. - If I change "table-cell" to "table", we don't assert.
|
|
Assignee | |
Comment 5•5 years ago
|
||
(In reply to Mats Palmgren (:mats) from comment #2) > The mContent->GetPrimaryFrame() is the nsFlexContainerFrame root frame > that ConstructDocElementFrame setup before calling ProcessChildren: > http://hg.mozilla.org/mozilla-central/annotate/2bddbd180d2d/layout/base/ > nsCSSFrameConstructor.cpp#l2467 ...and mContent->Tag() is "html". So yeah, this mContent is the root node. This suggests to me that we've got the <html> node in our list of children to process, which seems broken. I might be misunderstanding, though.
|
|
Assignee | |
Comment 6•5 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #5) > ...and mContent->Tag() is "html". So yeah, this mContent is the root node. > > This suggests to me that we've got the <html> node in our list of children > to process, which seems broken. I might be misunderstanding, though. Oh, just kidding -- mStyleContext.GetPseudo() is ":-moz-table". So this isn't the FCItem for the <html> node -- it's for the anonymous table wrapper. (which just happens to report its parent, the <html> node, as its mContent) That makes more sense.
|
|
Assignee | |
Comment 7•5 years ago
|
||
We only hit this for the root node because for that node, we call SetPrimaryFrame early, before processing the children: http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp?rev=90c1ec4ae807&mark=2466-2467#2466 ...whereas for the normal codepath (with e.g. "display:flex" on a normal div), we call SetPrimaryFrame later, *after* we've created the child frames: http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp?rev=90c1ec4ae807&mark=3762-3762#3754 Maybe we should fix this by delaying the root node's SetPrimaryFrame call a bit?
|
|
Assignee | |
Comment 8•5 years ago
|
||
Here's a patch to delay the SetPrimaryFrame call, as noted above. This fixes the assertion in the attached testcases, but I'm not sure if it has other fallout / implications. Try run: https://tbpl.mozilla.org/?tree=Try&rev=dc123139b161
|
|
Assignee | |
Comment 9•5 years ago
|
||
Comment on attachment 8377017 [details] [diff] [review] possible fix, v1: delay SetPrimaryFrame Try run is incomplete but looking good, so I'll kick this to bz for review. (I'll add a commit message & the original testcase as a crashtest before landing, of course.)
Attachment #8377017 -
Flags: review?(bzbarsky)
|
|
Assignee | |
Updated•5 years ago
|
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
|
|
Assignee | |
Comment 10•5 years ago
|
||
(Reposting patch, now with a crashtest & commit message). The first Try run was green. I'm doing one more, including the crashtest now, to make sure that the crashtest doesn't have issues anywhere: https://tbpl.mozilla.org/?tree=Try&rev=3031be4a780f
Attachment #8377017 -
Attachment is obsolete: true
Attachment #8377017 -
Flags: review?(bzbarsky)
Attachment #8377283 -
Flags: review?(bzbarsky)
|
||
Comment 11•5 years ago
|
||
Comment on attachment 8377283 [details] [diff] [review] fix v1a (now with crashtest / commit message) This seems ok, yeah.
Attachment #8377283 -
Flags: review?(bzbarsky) → review+
|
|
||
Comment 12•5 years ago
|
||
The code comment there is a bit redundant, IMO. :-)
|
|
Assignee | |
Comment 13•5 years ago
|
||
Thanks for the review. (In reply to Mats Palmgren (:mats) from comment #12) > The code comment there is a bit redundant, IMO. :-) Probably true. I didn't bother removing it for now, though, since it's preexisting, and it has at least some small value in [implicitly] calling out "This is an important main step". We can always drop it later in a DONTBUILD cset. Pushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/b7057eb401fa
Flags: in-testsuite+
|
|
||
Comment 14•5 years ago
|
||
No problem, I just think that comments like that are more distracting than helpful. There's another one right above it as well: // Set the initial child lists contentFrame->SetInitialChildList(kPrincipalList, childItems);
|
||
Comment 15•5 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b7057eb401fa
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in
before you can comment on or make changes to this bug.
Description
•