Closed Bug 536623 Opened 12 years ago Closed 12 years ago

"ASSERTION: SetMayHaveFrame failed?" with XBL, xul:label, table, wrapping

Categories

(Core :: XBL, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
blocking1.9.2 --- -
status1.9.2 --- .7-fixed

People

(Reporter: jruderman, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase, verified1.9.2)

Attachments

(3 files, 1 obsolete file)

Attached file testcase
###!!! ASSERTION: SetMayHaveFrame failed?: 'mContent->MayHaveFrame()', file /Users/jruderman/mozilla-central/layout/generic/nsFrame.cpp, line 343
Gah.  This is the usual crap where a textnode has no parent and is getting a frame created for it...
And in particular, we have an nsXULLabelFrame whose first child is an nsTextFrame, whose textnode content has a null parent.  This textframe has the text "1 2 3" and we're creating a continuation for it while reflowing the parent block.
And _that_ happens because when we go to remove the frames for the "1 2 3" text on the removeAttribute call there are actually two frames for it in the frame tree.
OK.  So the setAttribute tries to construct a textframe child of the table, which just reframes the table (already risky, since that table is anonymous, but maybe not too bad since it's at least xbl-bound anonymous).

When we go to create the frame for that table, we somehow end up creating two textframes for that text content.  Looking into why.
Aha!  When reconstructing the <table> via ContentInserted, we do AddTextItemIfNeeded, and since aIndexInContainer there is -1, we pass 0 as the index to it.  That constructs a frame for the textnode, which is in fact a child of the <label>, and the only child (hence at position 0).

We could fix this particular case by not calling AddTextItemIfNeeded if the index is -1.  But I think calling AddTextItemIfNeeded in cases when the parent has an XBL-bound child list is just generally wrong (or alternately, that AddTextItemIfNeeded needs to do that check).  The textframe could be suppressed for other XBL reasons (e.g. its parent node could be an anonymous <html:colgroup>), and then we'd construct it in the wrong place in the frame tree even in cases where non-anonymous content is being inserted.  Since cases when the parent has an XBL-bound child list don't suppress textframes to start with, skipping AddTextFrameIfNeeded in those cases would be safe, I think.

roc, does that make sense?
Blocks: 495385
Blocks: 537141
Attached patch FixSplinter Review
roc, we need to get this in on 1.9.2 as well, right?  (Possibly minus the additional asserts I added.)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #419465 - Flags: review?(roc)
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Attached patch 1.9.2 merge (obsolete) — Splinter Review
Attachment #419692 - Flags: approval1.9.2.1?
status1.9.2: --- → ?
Depends on: 538063
Attachment #419692 - Attachment is obsolete: true
Attachment #423355 - Flags: approval1.9.2.1?
Attachment #419692 - Flags: approval1.9.2.1?
blocking1.9.2: --- → ?
blocking1.9.2: ? → -
Attachment #423355 - Flags: approval1.9.2.2? → approval1.9.2.3?
Comment on attachment 423355 [details] [diff] [review]
1.9.2 merge that compiles in a debug build too (removed two asserts)

Didn't make 1.9.2.2, would like to take it in .3 - moving flag forward.
Comment on attachment 423355 [details] [diff] [review]
1.9.2 merge that compiles in a debug build too (removed two asserts)

a=LegNeato for 1.9.2.5. Please *only* land it on mozilla-1.9.2 default, as we are still working on the 3.6.4 relbranch and do not want this landed there
Attachment #423355 - Flags: approval1.9.2.5+
Attachment #423355 - Flags: approval1.9.2.4?
Attachment #423355 - Flags: approval1.9.2.4-
Attachment #423355 - Flags: approval1.9.2.5+ → approval1.9.2.6+
I just ran this in my nightly 1.9.2 build from the other day on Windows XP (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.6pre) Gecko/20100624 Namoroka/3.6.6pre ( .NET CLR 3.5.30729)).

When I run the attached testcase, I got a crash the first time which I didn't catch.

Subsequent runs, I get the original assert:

###!!! ASSERTION: SetMayHaveFrame failed?: 'mContent->MayHaveFrame()', file c:/projects/moz1.9.2/layout/generic/nsFrame.cpp, line 347
###!!! ASSERTION: SetMayHaveFrame failed?: 'mContent->MayHaveFrame()', file c:/projects/moz1.9.2/layout/generic/nsFrame.cpp, line 347

Is this meant to only be fixed on OS X and not on Windows?
This is expected to be fixed for all OSes.  What's the revision id for the build mentioned in comment 15?
I just retested on Linux, and don't see the assertions there either...
It's me. I completely screwed up which build I was using due to innate stupidity. I'm rebuilding now after blowing away my debug directory. Sorry about that.
It looks to be fixed. The only thing I see in my OS X debug build from the other day (Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.2.6pre) Gecko/20100622 Namoroka/3.6.6pre) is:

WARNING: NS_ENSURE_TRUE(globalObject) failed: file /Users/albill/mozilla-192/content/base/src/nsDocument.cpp, line 6472
You need to log in before you can comment on or make changes to this bug.