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

Status: RESOLVED FIXED

(Core :: XBL, defect)

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ASSERTION: SetMayHaveFrame failed?: 'mContent->MayHaveFrame()', file /Users/jruderman/mozilla-central/layout/generic/nsFrame.cpp, line 343

Comment 1

[Boris Zbarsky [:bzbarsky]]

Gah.  This is the usual crap where a textnode has no parent and is getting a frame created for it...

Comment 2

[Boris Zbarsky [:bzbarsky]]

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.

Comment 3

[Boris Zbarsky [:bzbarsky]]

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.

Comment 4

[Boris Zbarsky [:bzbarsky]]

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.

Comment 5

[Boris Zbarsky [:bzbarsky]]

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?

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix

roc, we need to get this in on 1.9.2 as well, right?  (Possibly minus the additional asserts I added.)

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 419465 [details] [diff] [review]
Fix

Yes.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/c410241f9d1c

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: 1.9.2 merge

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: 1.9.2 merge that compiles in a debug build too (removed two asserts)

Comment 12

[Johnathan Nightingale [:johnath]]

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

[LegNeato]

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

Comment 14

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/releases/mozilla-1.9.2/rev/5a4db3e2a4e4

Comment 15

[Al Billings [:abillings - ex-MoCo]]

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?

Comment 16

[Boris Zbarsky [:bzbarsky]]

This is expected to be fixed for all OSes.  What's the revision id for the build mentioned in comment 15?

Comment 17

[Boris Zbarsky [:bzbarsky]]

I just retested on Linux, and don't see the assertions there either...

Comment 18

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 19

[Al Billings [:abillings - ex-MoCo]]

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