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

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: jruderman, Assigned: bzbarsky)

Tracking

(Blocks 1 bug, {assertion, testcase, verified1.9.2})

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

Firefox Tracking Flags

(blocking1.9.2 -, status1.9.2 .7-fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

Reporter

Description

10 years ago
Posted 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
Posted 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: 10 years ago
Resolution: --- → FIXED
Posted 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 13

9 years ago
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.