From 3b854aa20fa94bc57526924d6b0b8cc9c9223a79 Mon Sep 17 00:00:00 2001 From: Henri Sivonen Date: Fri, 20 Jan 2012 13:16:26 +0200 Subject: [PATCH] Bug 715112 - Remove duplicate document.close() state tracking. r=smaug. --- .../html/document/src/nsHTMLContentSink.cpp | 74 +---- content/html/document/src/nsHTMLDocument.cpp | 265 ++++-------------- content/html/document/src/nsHTMLDocument.h | 20 -- content/html/document/src/nsIHTMLDocument.h | 17 +- parser/html/nsHtml5Parser.cpp | 4 + parser/html/nsHtml5TreeOpExecutor.cpp | 17 -- parser/html/nsHtml5TreeOpExecutor.h | 1 - 7 files changed, 63 insertions(+), 335 deletions(-) diff --git a/content/html/document/src/nsHTMLContentSink.cpp b/content/html/document/src/nsHTMLContentSink.cpp index 93c62613118..21622245c2c 100644 --- a/content/html/document/src/nsHTMLContentSink.cpp +++ b/content/html/document/src/nsHTMLContentSink.cpp @@ -295,9 +295,6 @@ protected: void CloseHeadContext(); // nsContentSink overrides - virtual void PreEvaluateScript(); - virtual void PostEvaluateScript(nsIScriptElement *aElement); - void UpdateChildCounts(); void NotifyInsert(nsIContent* aContent, @@ -2716,79 +2713,12 @@ HTMLContentSink::UpdateChildCounts() mCurrentContext->UpdateChildCounts(); } -void -HTMLContentSink::PreEvaluateScript() -{ - // Eagerly append all pending elements (including the current body child) - // to the body (so that they can be seen by scripts) and force reflow. - SINK_TRACE(gSinkLogModuleInfo, SINK_TRACE_CALLS, - ("HTMLContentSink::PreEvaluateScript: flushing tags before " - "evaluating script")); - - // XXX Should this call FlushTags()? - mCurrentContext->FlushText(); -} - -void -HTMLContentSink::PostEvaluateScript(nsIScriptElement *aElement) -{ - mHTMLDocument->ScriptExecuted(aElement); -} - nsresult HTMLContentSink::ProcessSCRIPTEndTag(nsGenericHTMLElement *content, bool aMalformed) { - // Flush all tags up front so that we are in as stable state as possible - // when calling DoneAddingChildren. This may not be strictly needed since - // any ScriptAvailable calls will cause us to flush anyway. But it gives a - // warm fuzzy feeling to be in a stable state before even attempting to - // run scripts. - // It would however be needed if we properly called BeginUpdate and - // EndUpdate while we were inserting stuff into the DOM. - - // XXX Should this call FlushTags()? - mCurrentContext->FlushText(); - - nsCOMPtr sele = do_QueryInterface(content); - NS_ASSERTION(sele, "Not really closing a script tag?"); - - if (aMalformed) { - // Make sure to serialize this script correctly, for nice round tripping. - sele->SetIsMalformed(); - } - if (mFrameset) { - sele->PreventExecution(); - } - - // Notify our document that we're loading this script. - mHTMLDocument->ScriptLoading(sele); - - // Now tell the script that it's ready to go. This may execute the script - // or return true, or neither if the script doesn't need executing. - bool block = sele->AttemptToExecute(); - - // If the act of insertion evaluated the script, we're fine. - // Else, block the parser till the script has loaded. - if (block) { - // If this append fails we'll never unblock the parser, but the UI will - // still remain responsive. There are other ways to deal with this, but - // the end result is always that the page gets botched, so there is no - // real point in making it more complicated. - mScriptElements.AppendObject(sele); - } else { - // This may have already happened if the script executed, but in case - // it didn't then remove the element so that it doesn't get stuck forever. - mHTMLDocument->ScriptExecuted(sele); - } - - // If the parser got blocked, make sure to return the appropriate rv. - // I'm not sure if this is actually needed or not. - if (mParser && !mParser->IsParserEnabled()) { - block = true; - } - - return block ? NS_ERROR_HTMLPARSER_BLOCK : NS_OK; + MOZ_NOT_REACHED("Must not use HTMLContentSink to run scripts."); + return NS_ERROR_NOT_IMPLEMENTED; } // 3 ways to load a style sheet: inline, style src=, link tag diff --git a/content/html/document/src/nsHTMLDocument.cpp b/content/html/document/src/nsHTMLDocument.cpp index 96365c14ed7..f21f36d6a21 100644 --- a/content/html/document/src/nsHTMLDocument.cpp +++ b/content/html/document/src/nsHTMLDocument.cpp @@ -906,78 +906,17 @@ nsHTMLDocument::StartDocumentLoad(const char* aCommand, void nsHTMLDocument::StopDocumentLoad() { - if (nsHtml5Module::sEnabled) { - BlockOnload(); - if (mWriteState == eDocumentOpened) { - NS_ASSERTION(IsHTML(), "document.open()ed doc is not HTML?"); + BlockOnload(); - // Marking the document as closed, since pending scripts will be - // stopped by nsDocument::StopDocumentLoad() below - mWriteState = eDocumentClosed; + // Remove the wyciwyg channel request from the document load group + // that we added in Open() if Open() was called on this doc. + RemoveWyciwygChannel(); + NS_ASSERTION(!mWyciwygChannel, "nsHTMLDocument::StopDocumentLoad(): " + "nsIWyciwygChannel could not be removed!"); - // Remove the wyciwyg channel request from the document load group - // that we added in Open(). - NS_ASSERTION(mWyciwygChannel, "nsHTMLDocument::StopDocumentLoad(): " - "Trying to remove nonexistent wyciwyg channel!"); - RemoveWyciwygChannel(); - NS_ASSERTION(!mWyciwygChannel, "nsHTMLDocument::StopDocumentLoad(): " - "nsIWyciwygChannel could not be removed!"); - } - nsDocument::StopDocumentLoad(); - UnblockOnload(false); - return; - } - // Code for the old parser: - - // If we're writing (i.e., there's been a document.open call), then - // nsDocument::StopDocumentLoad will do the wrong thing and simply terminate - // our parser. - if (mWriteState != eNotWriting) { - Close(); - } else { - nsDocument::StopDocumentLoad(); - } -} - -// static -void -nsHTMLDocument::DocumentWriteTerminationFunc(nsISupports *aRef) -{ - nsCOMPtr arr = do_QueryInterface(aRef); - NS_ASSERTION(arr, "Must have array!"); - - nsCOMPtr doc = do_QueryElementAt(arr, 0); - NS_ASSERTION(doc, "Must have document!"); - - nsCOMPtr parser = do_QueryElementAt(arr, 1); - NS_ASSERTION(parser, "Must have parser!"); - - nsHTMLDocument *htmldoc = static_cast(doc.get()); - - // Check whether htmldoc still has the same parser. If not, it's - // not for us to mess with it. - if (htmldoc->mParser != parser) { - return; - } - - // If the document is in the middle of a document.write() call, this - // most likely means that script on a page document.write()'d out a - // script tag that did location="..." and we're right now finishing - // up executing the script that was written with - // document.write(). Since there's still script on the stack (the - // script that called document.write()) we don't want to release the - // parser now, that would cause the next document.write() call to - // cancel the load that was initiated by the location="..." in the - // script that was written out by document.write(). - - if (!htmldoc->mWriteLevel && htmldoc->mWriteState != eDocumentOpened) { - // Release the document's parser so that the call to EndLoad() - // doesn't just return early and set the termination function again. - - htmldoc->mParser = nsnull; - } - - htmldoc->EndLoad(); + nsDocument::StopDocumentLoad(); + UnblockOnload(false); + return; } void @@ -998,65 +937,6 @@ nsHTMLDocument::BeginLoad() void nsHTMLDocument::EndLoad() { - if (mParser && mWriteState != eDocumentClosed) { - nsCOMPtr stack = - do_GetService("@mozilla.org/js/xpc/ContextStack;1"); - - if (stack) { - JSContext *cx = nsnull; - stack->Peek(&cx); - - if (cx) { - nsIScriptContext *scx = nsJSUtils::GetDynamicScriptContext(cx); - - if (scx) { - // The load of the document was terminated while we're - // called from within JS and we have a parser (i.e. we're in - // the middle of doing document.write()). In stead of - // releasing the parser and ending the document load - // directly, we'll make that happen once the script is done - // executing. This way subsequent document.write() calls - // won't end up creating a new parser and interrupting other - // loads that were started while the script was - // running. I.e. this makes the following case work as - // expected: - // - // document.write("foo"); - // location.href = "http://www.mozilla.org"; - // document.write("bar"); - - nsresult rv; - - nsCOMPtr arr = - do_CreateInstance(NS_ARRAY_CONTRACTID, &rv); - if (NS_SUCCEEDED(rv)) { - rv = arr->AppendElement(static_cast(this), - false); - if (NS_SUCCEEDED(rv)) { - rv = arr->AppendElement(mParser, false); - if (NS_SUCCEEDED(rv)) { - rv = scx->SetTerminationFunction(DocumentWriteTerminationFunc, - arr); - // If we fail to set the termination function, just go ahead - // and EndLoad now. The slight bugginess involved is better - // than leaking. - if (NS_SUCCEEDED(rv)) { - return; - } - } - } - } - } - } - } - } - - // Reset this now, since we're really done "loading" this document.written - // document. - NS_ASSERTION(mWriteState == eNotWriting || mWriteState == ePendingClose || - mWriteState == eDocumentClosed, "EndLoad called early"); - mWriteState = eNotWriting; - bool turnOnEditing = mParser && (HasFlag(NODE_IS_EDITABLE) || mContentEditableCount > 0); // Note: nsDocument::EndLoad nulls out mParser. @@ -1673,8 +1553,6 @@ nsHTMLDocument::Open(const nsAString& aContentTypeOrUrl, // This will be propagated to the parser when someone actually calls write() SetContentTypeInternal(contentType); - mWriteState = eDocumentOpened; - if (NS_SUCCEEDED(rv)) { if (loadAsHtml5) { nsHtml5Module::Initialize(mParser, this, uri, shell, channel); @@ -1686,7 +1564,6 @@ nsHTMLDocument::Open(const nsAString& aContentTypeOrUrl, if (NS_FAILED(rv)) { // Don't use a parser without a content sink. mParser = nsnull; - mWriteState = eNotWriting; return rv; } @@ -1746,55 +1623,51 @@ nsHTMLDocument::Close() return NS_ERROR_DOM_INVALID_STATE_ERR; } - nsresult rv = NS_OK; - - if (mParser && mWriteState == eDocumentOpened) { - mPendingScripts.RemoveElement(GenerateParserKey()); - - mWriteState = mPendingScripts.IsEmpty() ? eDocumentClosed : ePendingClose; - - ++mWriteLevel; - rv = mParser->Parse(EmptyString(), mParser->GetRootContextKey(), - GetContentTypeInternal(), true); - --mWriteLevel; - - // XXX Make sure that all the document.written content is - // reflowed. We should remove this call once we change - // nsHTMLDocument::OpenCommon() so that it completely destroys the - // earlier document's content and frame hierarchy. Right now, it - // re-uses the earlier document's root content object and - // corresponding frame objects. These re-used frame objects think - // that they have already been reflowed, so they drop initial - // reflows. For certain cases of document.written content, like a - // frameset document, the dropping of the initial reflow means - // that we end up in document.close() without appended any reflow - // commands to the reflow queue and, consequently, without adding - // the dummy layout request to the load group. Since the dummy - // layout request is not added to the load group, the onload - // handler of the frameset fires before the frames get reflowed - // and loaded. That is the long explanation for why we need this - // one line of code here! - // XXXbz as far as I can tell this may not be needed anymore; all - // the testcases in bug 57636 pass without this line... Leaving - // it be for now, though. In any case, there's no reason to do - // this if we have no presshell, since in that case none of the - // above about reusing frames applies. - if (GetShell()) { - FlushPendingNotifications(Flush_Layout); - } - - // Remove the wyciwyg channel request from the document load group - // that we added in OpenCommon(). If all other requests between - // document.open() and document.close() have completed, then this - // method should cause the firing of an onload event. - NS_ASSERTION(mWyciwygChannel, "nsHTMLDocument::Close(): Trying to remove " - "nonexistent wyciwyg channel!"); - RemoveWyciwygChannel(); - NS_ASSERTION(!mWyciwygChannel, "nsHTMLDocument::Close(): " - "nsIWyciwygChannel could not be removed!"); + if (!mParser || !mParser->IsScriptCreated()) { + return NS_OK; } - return NS_OK; + ++mWriteLevel; + nsresult rv = mParser->Parse(EmptyString(), mParser->GetRootContextKey(), + GetContentTypeInternal(), true); + --mWriteLevel; + + // XXX Make sure that all the document.written content is + // reflowed. We should remove this call once we change + // nsHTMLDocument::OpenCommon() so that it completely destroys the + // earlier document's content and frame hierarchy. Right now, it + // re-uses the earlier document's root content object and + // corresponding frame objects. These re-used frame objects think + // that they have already been reflowed, so they drop initial + // reflows. For certain cases of document.written content, like a + // frameset document, the dropping of the initial reflow means + // that we end up in document.close() without appended any reflow + // commands to the reflow queue and, consequently, without adding + // the dummy layout request to the load group. Since the dummy + // layout request is not added to the load group, the onload + // handler of the frameset fires before the frames get reflowed + // and loaded. That is the long explanation for why we need this + // one line of code here! + // XXXbz as far as I can tell this may not be needed anymore; all + // the testcases in bug 57636 pass without this line... Leaving + // it be for now, though. In any case, there's no reason to do + // this if we have no presshell, since in that case none of the + // above about reusing frames applies. + // + // XXXhsivonen keeping this around for bug 577508 / 253951 still :-( + if (GetShell()) { + FlushPendingNotifications(Flush_Layout); + } + + // Removing the wyciwygChannel here is wrong when document.close() is + // called from within the document itself. However, legacy requires the + // channel to be removed here. Otherwise, the load event never fires. + NS_ASSERTION(mWyciwygChannel, "nsHTMLDocument::Close(): Trying to remove " + "nonexistent wyciwyg channel!"); + RemoveWyciwygChannel(); + NS_ASSERTION(!mWyciwygChannel, "nsHTMLDocument::Close(): " + "nsIWyciwygChannel could not be removed!"); + return rv; } nsresult @@ -1815,10 +1688,7 @@ nsHTMLDocument::WriteCommon(JSContext *cx, nsresult rv = NS_OK; void *key = GenerateParserKey(); - if (mWriteState == eDocumentClosed || - (mWriteState == ePendingClose && - !mPendingScripts.Contains(key)) || - (mParser && !mParser->IsInsertionPointDefined())) { + if (mParser && !mParser->IsInsertionPointDefined()) { if (mExternalScriptsBeingEvaluated) { // Instead of implying a call to document.open(), ignore the call. nsContentUtils::ReportToConsole(nsIScriptError::warningFlag, @@ -1829,7 +1699,6 @@ nsHTMLDocument::WriteCommon(JSContext *cx, mDocumentURI); return NS_OK; } - mWriteState = eDocumentClosed; mParser->Terminate(); NS_ASSERTION(!mParser, "mParser should have been null'd out"); } @@ -1881,11 +1750,11 @@ nsHTMLDocument::WriteCommon(JSContext *cx, if (aNewlineTerminate) { rv = mParser->Parse(aText + new_line, key, GetContentTypeInternal(), - (mWriteState == eNotWriting || (mWriteLevel > 1))); + false); } else { rv = mParser->Parse(aText, key, GetContentTypeInternal(), - (mWriteState == eNotWriting || (mWriteLevel > 1))); + false); } --mWriteLevel; @@ -1939,30 +1808,6 @@ nsHTMLDocument::GetElementsByName(const nsAString& aElementName, return NS_OK; } -void -nsHTMLDocument::ScriptLoading(nsIScriptElement *aScript) -{ - if (mWriteState == eNotWriting) { - return; - } - - mPendingScripts.AppendElement(aScript); -} - -void -nsHTMLDocument::ScriptExecuted(nsIScriptElement *aScript) -{ - if (mWriteState == eNotWriting) { - return; - } - - mPendingScripts.RemoveElement(aScript); - if (mPendingScripts.IsEmpty() && mWriteState == ePendingClose) { - // The last pending script just finished, terminate our parser now. - mWriteState = eDocumentClosed; - } -} - void nsHTMLDocument::AddedForm() { diff --git a/content/html/document/src/nsHTMLDocument.h b/content/html/document/src/nsHTMLDocument.h index da2eeadeb0a..92534956f11 100644 --- a/content/html/document/src/nsHTMLDocument.h +++ b/content/html/document/src/nsHTMLDocument.h @@ -149,9 +149,6 @@ public: nsISupports **aResult, nsWrapperCache **aCache); - virtual void ScriptLoading(nsIScriptElement *aScript); - virtual void ScriptExecuted(nsIScriptElement *aScript); - virtual void AddedForm(); virtual void RemovedForm(); virtual PRInt32 GetNumFormsSynchronous(); @@ -275,29 +272,12 @@ protected: // Override so we can munge the charset on our wyciwyg channel as needed. virtual void SetDocumentCharacterSet(const nsACString& aCharSetID); - // 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 if there have been no - // external script loads in the meantime. If there have been, then mWriteState - // becomes ePendingClose, indicating that we might still be writing, but that - // we shouldn't process any further close() calls. - enum { - eNotWriting, - eDocumentOpened, - ePendingClose, - eDocumentClosed - } mWriteState; - // Tracks if we are currently processing any document.write calls (either // implicit or explicit). Note that if a write call writes out something which // would block the parser, then mWriteLevel will be incorrect until the parser // finishes processing that script. PRUint32 mWriteLevel; - nsAutoTArray mPendingScripts; - // Load flags of the document's channel PRUint32 mLoadFlags; diff --git a/content/html/document/src/nsIHTMLDocument.h b/content/html/document/src/nsIHTMLDocument.h index 0534f382210..4e926e9ea29 100644 --- a/content/html/document/src/nsIHTMLDocument.h +++ b/content/html/document/src/nsIHTMLDocument.h @@ -49,9 +49,8 @@ class nsContentList; class nsWrapperCache; #define NS_IHTMLDOCUMENT_IID \ -{ 0x51a360fa, 0xd659, 0x4d85, \ - { 0xa5, 0xc5, 0x4a, 0xbb, 0x0d, 0x97, 0x0f, 0x7a } } - +{ 0xa921276f, 0x5e70, 0x42e0, \ + { 0xb8, 0x36, 0x7e, 0x6a, 0xb8, 0x30, 0xb3, 0xc0 } } /** * HTML document extensions to nsIDocument. @@ -71,18 +70,6 @@ public: nsISupports **aResult, nsWrapperCache **aCache) = 0; - /** - * Called from the script loader to notify this document that a new - * script is being loaded. - */ - virtual void ScriptLoading(nsIScriptElement *aScript) = 0; - - /** - * Called from the script loader to notify this document that a script - * just finished executing. - */ - virtual void ScriptExecuted(nsIScriptElement *aScript) = 0; - /** * Called when form->BindToTree() is called so that document knows * immediately when a form is added diff --git a/parser/html/nsHtml5Parser.cpp b/parser/html/nsHtml5Parser.cpp index 45ffd8fc64c..058c85968b5 100644 --- a/parser/html/nsHtml5Parser.cpp +++ b/parser/html/nsHtml5Parser.cpp @@ -288,6 +288,10 @@ nsHtml5Parser::Parse(const nsAString& aSourceBuffer, // document.close() NS_ASSERTION(!mStreamParser, "Had stream parser but got document.close()."); + if (mDocumentClosed) { + // already closed + return NS_OK; + } mDocumentClosed = true; if (!mBlocked && !mInDocumentWrite) { ParseUntilBlocked(); diff --git a/parser/html/nsHtml5TreeOpExecutor.cpp b/parser/html/nsHtml5TreeOpExecutor.cpp index 09b58170b7b..d205e0ff01f 100644 --- a/parser/html/nsHtml5TreeOpExecutor.cpp +++ b/parser/html/nsHtml5TreeOpExecutor.cpp @@ -297,14 +297,6 @@ nsHtml5TreeOpExecutor::FlushTags() return NS_OK; } -void -nsHtml5TreeOpExecutor::PostEvaluateScript(nsIScriptElement *aElement) -{ - nsCOMPtr htmlDocument = do_QueryInterface(mDocument); - NS_ASSERTION(htmlDocument, "Document didn't QI into HTML document."); - htmlDocument->ScriptExecuted(aElement); -} - void nsHtml5TreeOpExecutor::ContinueInterruptedParsingAsync() { @@ -756,11 +748,6 @@ nsHtml5TreeOpExecutor::RunScript(nsIContent* aScriptElement) sele->SetCreatorParser(mParser); - // Notify our document that we're loading this script. - nsCOMPtr htmlDocument = do_QueryInterface(mDocument); - NS_ASSERTION(htmlDocument, "Document didn't QI into HTML document."); - htmlDocument->ScriptLoading(sele); - // Copied from nsXMLContentSink // Now tell the script that it's ready to go. This may execute the script // or return true, or neither if the script doesn't need executing. @@ -774,10 +761,6 @@ nsHtml5TreeOpExecutor::RunScript(nsIContent* aScriptElement) mParser->BlockParser(); } } else { - // This may have already happened if the script executed, but in case - // it didn't then remove the element so that it doesn't get stuck forever. - htmlDocument->ScriptExecuted(sele); - // mParser may have been nulled out by now, but the flusher deals // If this event isn't needed, it doesn't do anything. It is sometimes diff --git a/parser/html/nsHtml5TreeOpExecutor.h b/parser/html/nsHtml5TreeOpExecutor.h index c679643edfe..b91c3bb0076 100644 --- a/parser/html/nsHtml5TreeOpExecutor.h +++ b/parser/html/nsHtml5TreeOpExecutor.h @@ -202,7 +202,6 @@ class nsHtml5TreeOpExecutor : public nsContentSink, // nsContentSink methods virtual void UpdateChildCounts(); virtual nsresult FlushTags(); - virtual void PostEvaluateScript(nsIScriptElement *aElement); virtual void ContinueInterruptedParsingAsync(); /**