366207 - [FIX]-moz-binding on root element triggers "ASSERTION: null ptr" and more

Status: RESOLVED FIXED

(Core :: XBL, defect)

Description

[Jesse Ruderman]

Attachment: testcase

Loading this testcase triggers two assertions:

###!!! ASSERTION: null ptr: 'nsnull != aFrameList', file /Users/admin/trunk/mozilla/layout/generic/nsFrameList.cpp, line 68

###!!! ASSERTION: initial containing block already created: 'nsnull == mInitialContainingBlock', file /Users/admin/trunk/mozilla/layout/base/nsCSSFrameConstructor.cpp, line 8786

The second assertion also appears in bug 366200.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase that allows correctness testing

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Same as diff -w

The fix for the first assertion is the null-check of newChild after the call to ConstructDocElementFrame() -- if the root is XBL-bound and the XBL hasn't loaded yet, this will return NS_OK and null.

The rest is the fix for the second assert.  Instead of calling ContentInserted directly, just tell the presshell to reconstruct frames.  And change ReconstructDocElementHierarchyInternal to work even if there isn't a docElementFrame yet.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Really as -w

Comment 5

[Boris Zbarsky [:bzbarsky]]

Fixed.

Comment 6

[Jesse Ruderman]

bz, can you make a reftest based on your attachment, "Testcase that allows correctness testing"?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Sort of, but that testcase behaves identically before and after this checkin....  still worth it?

Comment 8

[Jesse Ruderman]

bz, I think so.  You created it because you wanted to avoid breaking it, right? :)

Comment 9

[Boris Zbarsky [:bzbarsky]]

Sort of... I can't quite recall at this point.  I've checked it in anyway.

Comment 10

[(no longer active)]

Boris, are the two 1 second timeouts really necessary?  I guess they could just be setTimeout(foo, 0), right?

Comment 11

[Boris Zbarsky [:bzbarsky]]

I'm not sure.  The one from the constructor probably could.  The other... I'm not sure.

Comment 7 complicates figuring out whether they're necessary.  :(

Comment 12

[(no longer active)]

Then are you fine with me making them both 0 at least?  Especially since this test is not testing anything useful anyways... ;-)

Comment 13

[Boris Zbarsky [:bzbarsky]]

<shrug>.  There's a good chance that making them 0 is equivalent to hg removing the testcase.  I just can't tell, unfortunately.

Comment 14

[Martijn Wargers (dead)]

What's the point of making it 0? If you want to speedup the unit tests there are better ways of doing that.

Comment 15

[(no longer active)]

(In reply to comment #14)
> What's the point of making it 0? If you want to speedup the unit tests there
> are better ways of doing that.

The point is that the current test, if testing anything useful, is using magic numbers, and we're planning to move away from using such numbers in the future.  There is nothing in the test which requires exact 1s waits, and the test can't get exact waits anyways.  It's a matter of code cleanliness, and the test not being flaky.

Comment 16

[(no longer active)]

(In reply to comment #13)
> <shrug>.  There's a good chance that making them 0 is equivalent to hg removing
> the testcase.  I just can't tell, unfortunately.

Hmm, I _guess_ that the 1 second timeouts only meant to make sure that the frame construction and instantiation is finished.  If that's the case, maybe we could just replace them with explicit flushes?

Comment 17

[Martijn Wargers (dead)]

If you're changing the testcase, how will you know that it's still testing for the original assertion?

Comment 18

[Boris Zbarsky [:bzbarsky]]

We could test the modified testcase against a pre-patch build.

Ehsan, what about posting 0-length timeout to do a flush and post another 0-length timeout?  I would be pretty comfortable with that.

Comment 19

[(no longer active)]

Filed bug 649223.