Closed Bug 366207 Opened 14 years ago Closed 14 years ago

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

Categories

(Core :: XBL, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9alpha1

People

(Reporter: jruderman, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase)

Attachments

(4 files, 1 obsolete file)

Attached file 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.
Attached patch FixSplinter Review
Assignee: general → bzbarsky
Status: NEW → ASSIGNED
Attached patch Same as diff -w (obsolete) — Splinter Review
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.
Attachment #250810 - Flags: superreview?(roc)
Attachment #250810 - Flags: review?(bugmail)
OS: Mac OS X 10.4 → All
Hardware: Macintosh → All
Summary: -moz-binding on root element triggers "ASSERTION: null ptr" and more → [FIX]-moz-binding on root element triggers "ASSERTION: null ptr" and more
Target Milestone: --- → mozilla1.9alpha
Attached patch Really as -wSplinter Review
Attachment #250810 - Attachment is obsolete: true
Attachment #250813 - Flags: superreview?(roc)
Attachment #250813 - Flags: review?(bugmail)
Attachment #250810 - Flags: superreview?(roc)
Attachment #250810 - Flags: review?(bugmail)
Attachment #250813 - Flags: superreview?(roc) → superreview+
Fixed.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Depends on: 368183
bz, can you make a reftest based on your attachment, "Testcase that allows correctness testing"?
Flags: in-testsuite?
Sort of, but that testcase behaves identically before and after this checkin....  still worth it?
bz, I think so.  You created it because you wanted to avoid breaking it, right? :)
Sort of... I can't quite recall at this point.  I've checked it in anyway.
Flags: in-testsuite? → in-testsuite+
Boris, are the two 1 second timeouts really necessary?  I guess they could just be setTimeout(foo, 0), right?
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.  :(
Then are you fine with me making them both 0 at least?  Especially since this test is not testing anything useful anyways... ;-)
<shrug>.  There's a good chance that making them 0 is equivalent to hg removing the testcase.  I just can't tell, unfortunately.
What's the point of making it 0? If you want to speedup the unit tests there are better ways of doing that.
(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.
(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?
If you're changing the testcase, how will you know that it's still testing for the original assertion?
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.
Depends on: 649223
Filed bug 649223.
You need to log in before you can comment on or make changes to this bug.