321781 - ###!!! ASSERTION: More UnblockOnload() calls than BlockOnload() calls; dropping call: 'Not Reached', file d:/mozbuild1/mozilla/content/base/src/nsDocument.cpp, line 5188

Status: RESOLVED FIXED

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

Description

[Blake Kaplan (:mrbkap) (inactive)]

STEPS TO REPRODUCE:
*) Go to the URL in the URL field in a build with e4x enabled.
*) Click on link 0 and the "test" button.

ACTUAL RESULTS: We assert.
EXPECTED RESULTS: No assertion.

Comment 1

[Biju]

backlink: bug 321558  attachment 206870 [details]

Comment 2

[Scott MacGregor]

Between Christmas and New Year's i started seeing this same assertion when I try to load messages in the mail message pane.

Every time I click on a message, I get 

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

and the message fails to load. Given the date Blake filed this bug, it could be caused by the same thing.

Comment 3

[Scott MacGregor]

stack trace leading up to the assertion:

nsDocument::UnblockOnload(int 1) line 5189
nsDocument::EndLoad() line 2246
nsHTMLDocument::EndLoad() line 1021
nsHTMLDocument::DocumentWriteTerminationFunc(nsISupports * 0x04e7b258) line 974
nsJSContext::ScriptEvaluated(int 1) line 2054 + 17 bytes
nsJSContext::CallEventHandler(JSObject * 0x03de5aa8, JSObject * 0x03fd2860, unsigned int 1, long * 0x0012d24c, long * 0x0012d248) line 1456
nsJSEventListener::HandleEvent(nsJSEventListener * const 0x0370f5b0, nsIDOMEvent * 0x04e8f530) line 186 + 54 bytes
nsEventListenerManager::HandleEventSubType(nsListenerStruct * 0x0370f680, nsIDOMEventListener * 0x0370f5b0, nsIDOMEvent * 0x04e8f530, nsIDOMEventTarget * 0x04e73f10, unsigned int 8, unsigned int 7) line 1684 + 16 bytes
nsEventListenerManager::HandleEvent(nsEventListenerManager * const 0x0370f268, nsPresContext * 0x034df8b0, nsEvent * 0x0012d6b0, nsIDOMEvent * * 0x0012d654, nsIDOMEventTarget * 0x04e73f10, unsigned int 7, nsEventStatus * 0x0012d6d0) line 1791
nsXULElement::HandleDOMEvent(nsPresContext * 0x034df8b0, nsEvent * 0x0012d6b0, nsIDOMEvent * * 0x0012d654, unsigned int 7, nsEventStatus * 0x0012d6d0) line 1865
nsTreeSelection::FireOnSelectHandler() line 789
nsTreeSelection::Select(nsTreeSelection * const 0x04320b18, int 46) line 378

Comment 4

[Boris Zbarsky [:bzbarsky]]

Scott, you're probably (thought not certainly) seeing a different issue.  It's worth tracking in a separate bug, possibly depending on this one...

Comment 5

[Boris Zbarsky [:bzbarsky]]

This seems to be fallout from bug 319123 -- we're calling EndLoad twice from document.close -- once from Parse() and once from Terminate.  So we set the termination func twice, and then call EndLoad() for real twice.  The second call is what asserts.

Comment 6

[Boris Zbarsky [:bzbarsky]]

I think it's worth thinking about how to fix both this and bug 321782 at once.... They seem pretty related.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Possible fix

This basically backs out the change from bug 319123 in favor of a different fix for the leak. This fix is basically to inform the parser that the root context (the one that wrote out the external script) is really the final context and ensures that we call DidBuildModel after we're done parsing that context.

The only scary part of this patch is that if we ever use the wrong parser key (somehow executing the same script element, nested in itself), then we'll do the wrong thing and misplace the document.written text. Perhaps we should move key generation onto the parser or have |NS_IMETHOD_(void *) EnsureUniqueKey(void *aBaseKey)| to avoid this problem?

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 207750 [details] [diff] [review]
Possible fix

>-        ResumeParse(PR_FALSE, PR_FALSE, PR_FALSE);
>+
>+        if (pc == mParserContext) {
>+          ResumeParse(PR_FALSE, PR_FALSE, PR_FALSE);
>+        }

Note to self: Add a comment that if pc != mParserContext, then pc is not the active context and ResumeParse will do the wrong thing.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Revised potential fix

The first fix was missing two mParserContext->pc fixes in Parse. This fixes that and also remembers to include nsParser.h in the diff.

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: More fixed fix

This needs to be tested pretty hard. I took this opportunity to rename mIsWriting and added a large comment explaining it. I'll attach the testcase that I'm using as a stress test in a sec.

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: 3rd file for testcase

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: 2nd file for testcase

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: 1st file for testcase

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: stress test

Comment 15

[Johnny Stenback (:jst)]

Comment on attachment 208142 [details] [diff] [review]
More fixed fix

This breaks calling document.write() to create a "new" document after document.close() has been called to terminate an earlier document.write() that created a "new" document. sr-

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Closer...

This patch fixes jst's concern, but sicking came up with another problem that I don't think we got right even before my patch. Hoping he'll attach a testcase.

Note: I need to study my patch a bit more to make sure I fully understand why and how it works.

Comment 17

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

Attachment: 3rd file for testcase2

Comment 18

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

Attachment: 2nd file for testcase2

Comment 19

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

Attachment: 1st file for testcase2

Comment 20

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

Attachment: testcase2

So with above patch, what do we do for this testcase where the document.close() is called from an asynchronously loaded script?

Comment 21

[Blake Kaplan (:mrbkap) (inactive)]

With my patch, I see "one two" and the throbber stops spinning.

Comment 22

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

Comment on attachment 208302 [details] [diff] [review]
Closer...

>Index: content/html/document/src/nsHTMLDocument.h

>+  // mWriteState tracks the status of this document if the document is being
>+  // entirely created by script. In the normal load case, mWriteState will be
>+  // eNotWriting. Once document.open has been called (either implicitly or
>+  // explicitly), mWriteState will be eDocumentOpened. When document.close has
>+  // been called, mWriteState will become eDocumentClosed, indicating that we

Shouldn't that be "mWriteState will become ePendingClose". And please write when we transition into eDocumentClosed.

>+  // might still be writing, but that we shouldn't process any further close()
>+  // calls.
>+  enum WritingState { eNotWriting, eDocumentOpened, ePendingClose, eDocumentClosed };
>+  WritingState mWriteState;

You could make this
  enum { ... } mWriteState;

>Index: content/html/document/src/nsHTMLContentSink.cpp
>+HTMLContentSink::PreEvaluateScript(nsIScriptElement *aElement)
> {

This argument seems unused, nuke it.

r=me if you don't make any more changes to document.write.... ever!

Comment 23

[Johnny Stenback (:jst)]

Comment on attachment 208302 [details] [diff] [review]
Closer...

sr=jst

Comment 24

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Final patch

This is the final patch I'm about to check in. The only substantive change is that I call mPendingScripts.RemoveElement(GenerateParserKey()) in nsHTMLDocument::Close now, so subsequent writes from that script will reset the document.

Comment 25

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into trunk.