"ASSERTION: How did that happen?" in FrameConstructionItem::IsWhitespace

RESOLVED FIXED in mozilla30

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jruderman, Assigned: dholbert)

Tracking

(Blocks 1 bug, {assertion, regression, testcase})

Trunk
mozilla30
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 1 obsolete attachment)

Reporter

Description

5 years ago
Posted file testcase
###!!! ASSERTION: How did that happen?: 'aState.mCreatingExtraFrames || !mContent->GetPrimaryFrame()', file layout/base/nsCSSFrameConstructor.cpp, line 11160
Reporter

Comment 1

5 years ago
Posted file stack

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
Blocks: 969460
Keywords: regression
OS: Mac OS X → All
Hardware: x86_64 → All

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 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);
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.