375051 - [FIX]"ASSERTION: Shouldn't have a count of zero here, since we stabilized in PostUnblockOnloadEvent" and more, changing frame src rapidly

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

[Jesse Ruderman]

Attachment: testcase

Loading the testcase triggers three assertions:

###!!! ASSERTION: More UnblockOnload() calls than BlockOnload() calls; dropping call: 'Not Reached', file /Users/jruderman/trunk/mozilla/content/base/src/nsDocument.cpp, line 5535

###!!! ASSERTION: Shouldn't have a count of zero here, since we stabilized in PostUnblockOnloadEvent: 'mOnloadBlockCount != 0', file /Users/jruderman/trunk/mozilla/content/base/src/nsDocument.cpp, line 5583

###!!! ASSERTION: Adding child where we already have a child?  This will likely misbehave: 'Error', file /Users/jruderman/trunk/mozilla/docshell/shistory/src/nsSHEntry.cpp, line 536

The first assertion also shows up in bug 326621.  The third assertion also shows up in bug 344216.

Also, reloading the testcase (not shift-reloading) puts the first frame's url in the second frame, but that might be a separate bug.

Comment 1

[Jesse Ruderman]

I minimized for the second assertion (the one in the summary); I think it's possible to make smaller testcases for the first and third.

Comment 2

[Boris Zbarsky [:bzbarsky]]

So as far as I can tell, we're stopping the load, calling Terminate() on the parser, unblocking onload.  Then stopping the load _again_.  And we still have a parser, because of the termination func stuff -- that makes us not null out the mParser in the document when we stop the load.  So we Terminate() the _same_ parser again.  This calls DidBuildModel on the _same_ sink.  And then we unblock unload an extra time.

We could easily check for a non-null mParser in DropParserAndPerfHint, but even calling DidBuildModel twice on the same sink seems weird, no?  Blake?  Thoughts?

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

This seems scary, but unless we can think of a way to exploit I don't think it's a blocker. That said, it'd be great if you could take a look at this blake?

Comment 4

[Boris Zbarsky [:bzbarsky]]

This might not be exploitable, but I think it _could_ lead to onload firing too early in some cases.  I think we want to make sure to fix it, and checking for an mParser in DropParserAndPerfHint is really easy, even if we don't fix nsParser's Terminate impl.

So I feel we should go ahead and block on this to make sure that at least the simple fix is in.

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

ok :)

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Fine!

Comment 7

[Boris Zbarsky [:bzbarsky]]

Note that I can't reproduce this anyway... but this is still the right thing to do.

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 261921 [details] [diff] [review]
Fine!

Should nsContentSink::DropParserAndPerfHint assert if there is no mParser?

sr=me either way

Comment 9

[Boris Zbarsky [:bzbarsky]]

> Should nsContentSink::DropParserAndPerfHint assert if there is no mParser?

No, since there's clearly an easy way to get here...  I'll file a followup bug on fixing that.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Fixed.  Filed bug 378982.

Comment 11

[Carsten Book [:Tomcat]]

verified using using the testcase from jesse and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b3pre) Gecko/2008011009 Firefox/3.0b3pre ID:2008011009