715739 - document.write from inside a wyciwyg document adds to the cache stream

Status: RESOLVED FIXED

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

Description

[Christopher Blizzard (:blizzard)]

Attachment: test case

Just a guess in the summary.

Test case attached.  It should only show "Failure" once.  It does when you load it.  If you use reload it shows "Failure" twice.

Comment 1

[Christopher Blizzard (:blizzard)]

Attachment: fixed test case

Comment 2

[Boris Zbarsky [:bzbarsky]]

Um... Why are we ending up with a wyciwyg URL here?  Henri?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Oh, I see what's going one.

So the document you write out contains a script that does _another_ write into the same document.  So the wyciwyg bit is expected (because it's doc.write-generated), but the content that we cache for that wyciwyg URI contains both the script _and_ the content that script wrote out.  Then on reload, the script runs a second time, and writes out the h1 again, plus we read the h1 from the cache.

Ideally, document.write coming from scripts _inside_ the doc.written document wouldn't add to the cache stream, right?

Comment 4

[Henri Sivonen (:hsivonen)]

I take it that this is a problem for a real site? And this works in WebKit, because it doesn't support reloading document.open()ed pages and instead goes and reloads the original page whose URL the document.open()ed page had inherited?

Comment 5

[Christopher Blizzard (:blizzard)]

Yeah, it's a real problem for real sites, mostly on mobile.  People are doing document re-writing for mobile and this was one of the problems that was pointed out.

Comment 6

[Henri Sivonen (:hsivonen)]

Taking, though I'd prefer doing what WebKit does and getting rid of wyciwyg channels altogether.

Comment 7

[Henri Sivonen (:hsivonen)]

Attachment: Patch that addresses the current bug summary but not the test case

This patch addresses the bug summary as formulated by bz. The patch makes blizzard's test case say Failure, though, because our document.open() doesn't force the reuse of the inner window. However, even if I force the reuse of the inner window (not sure if it's safe to do that always), reloading makes the test case say Failure.

To make reloading say Success, we'd need to stop caching document.open()ed docs (remove wyciwyg channels) and should make reloading a document.open()ed doc load the URL that the document.open()ed doc has inherited. That's what WebKit does.

It's worth noting that the test case says Failure in Opera and IE9 & 10, so WebKit is alone reusing the inner window here. The spec says not to reuse the inner window.

If a real site is depending on the inner window reuse, it looks like the site is WebKit-only and is depending on a WebKit bug.

Comment 8

[Henri Sivonen (:hsivonen)]

Actually, even if we ended up no longer allowing normal navigation to wyciwyg channels, we should probably keep them around for View Source.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Per comment 0:

  It should only show "Failure" once.

The other parts of the testcase seem to be for a different but, which may also be worth filing, of course.  But at least Opera shows "Failure" as well, and I bet IE does too.

Comment 10

[Henri Sivonen (:hsivonen)]

Comment on attachment 588345 [details] [diff] [review]
Patch that addresses the current bug summary but not the test case

Note that even before this patch, the root context key has been nsnull. This patch makes it clear that we always use nsnull to denote the root context.

Comment 11

[Henri Sivonen (:hsivonen)]

Attachment: Mochitest

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 588345 [details] [diff] [review]
Patch that addresses the current bug summary but not the test case

># HG changeset patch
># User Henri Sivonen <[e-mail]>
># Date 1326446303 -7200
># Node ID 58aa9a9c2937bd18316b7b7a727387fe5b176128
># Parent  1b9a31b3641ae90c10166b8606f4b6273998aeb4
>[mq]: wyciwyg
>
>diff --git a/content/html/document/src/nsHTMLDocument.cpp b/content/html/document/src/nsHTMLDocument.cpp
>--- a/content/html/document/src/nsHTMLDocument.cpp
>+++ b/content/html/document/src/nsHTMLDocument.cpp
>@@ -1623,17 +1623,17 @@ nsHTMLDocument::Close()
>     return NS_ERROR_DOM_INVALID_STATE_ERR;
>   }
> 
>   if (!mParser || !mParser->IsScriptCreated()) {
>     return NS_OK;
>   }
> 
>   ++mWriteLevel;
>-  nsresult rv = mParser->Parse(EmptyString(), mParser->GetRootContextKey(),
>+  nsresult rv = mParser->Parse(EmptyString(), nsnull,
>                                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
>@@ -1726,17 +1726,17 @@ nsHTMLDocument::WriteCommon(JSContext *c
>     }
>     NS_ABORT_IF_FALSE(!JS_IsExceptionPending(cx),
>                       "Open() succeeded but JS exception is pending");
>   }
> 
>   static NS_NAMED_LITERAL_STRING(new_line, "\n");
> 
>   // Save the data in cache
>-  if (mWyciwygChannel) {
>+  if (mWyciwygChannel && !key) {
>     if (!aText.IsEmpty()) {
>       mWyciwygChannel->WriteToCacheEntry(aText);
>     }
> 
>     if (aNewlineTerminate) {
>       mWyciwygChannel->WriteToCacheEntry(new_line);
>     }
>   }
>@@ -2251,17 +2251,17 @@ nsHTMLDocument::GenerateParserKey(void)
>   if (nsHtml5Module::sEnabled) {
>     nsIScriptElement* script = mScriptLoader->GetCurrentParserInsertedScript();
>     if (script && mParser && mParser->IsScriptCreated()) {
>       nsCOMPtr<nsIParser> creatorParser = script->GetCreatorParser();
>       if (creatorParser != mParser) {
>         // Make scripts that aren't inserted by the active parser of this document
>         // participate in the context of the script that document.open()ed 
>         // this document.
>-        return mParser->GetRootContextKey();
>+        return nsnull;
>       }
>     }
>     return script;
>   } else {
>     return mScriptLoader->GetCurrentScript();
>   }
> }
> 
>diff --git a/parser/html/nsHtml5Parser.cpp b/parser/html/nsHtml5Parser.cpp
>--- a/parser/html/nsHtml5Parser.cpp
>+++ b/parser/html/nsHtml5Parser.cpp
>@@ -209,32 +209,31 @@ NS_IMETHODIMP_(bool)
> nsHtml5Parser::IsComplete()
> {
>   return mExecutor->IsComplete();
> }
> 
> NS_IMETHODIMP
> nsHtml5Parser::Parse(nsIURI* aURL,
>                      nsIRequestObserver* aObserver,
>-                     void* aKey,
>+                     void* aKey, // legacy; ignored
>                      nsDTDMode aMode) // legacy; ignored
> {
>   /*
>    * Do NOT cause WillBuildModel to be called synchronously from here!
>    * The document won't be ready for it until OnStartRequest!
>    */
>   NS_PRECONDITION(!mExecutor->HasStarted(), 
>                   "Tried to start parse without initializing the parser.");
>   NS_PRECONDITION(mStreamParser, 
>                   "Can't call this Parse() variant on script-created parser");
>   mStreamParser->SetObserver(aObserver);
>   mStreamParser->SetViewSourceTitle(aURL); // In case we're viewing source
>   mExecutor->SetStreamParser(mStreamParser);
>   mExecutor->SetParser(this);
>-  mRootContextKey = aKey;
>   return NS_OK;
> }
> 
> NS_IMETHODIMP
> nsHtml5Parser::Parse(const nsAString& aSourceBuffer,
>                      void* aKey,
>                      const nsACString& aContentType,
>                      bool aLastCall,
>@@ -279,17 +278,17 @@ nsHtml5Parser::Parse(const nsAString& aS
>     mExecutor->WillBuildModel(eDTDMode_unknown);
>   }
> 
>   // Return early if the parser has processed EOF
>   if (mExecutor->IsComplete()) {
>     return NS_OK;
>   }
> 
>-  if (aLastCall && aSourceBuffer.IsEmpty() && aKey == GetRootContextKey()) {
>+  if (aLastCall && aSourceBuffer.IsEmpty() && !aKey) {
>     // document.close()
>     NS_ASSERTION(!mStreamParser,
>                  "Had stream parser but got document.close().");
>     if (mDocumentClosed) {
>       // already closed
>       return NS_OK;
>     }
>     mDocumentClosed = true;
>@@ -401,17 +400,17 @@ nsHtml5Parser::Parse(const nsAString& aS
> 
>   nsHtml5DependentUTF16Buffer stackBuffer(aSourceBuffer);
> 
>   while (!mBlocked && stackBuffer.hasMore()) {
>     stackBuffer.adjust(mLastWasCR);
>     mLastWasCR = false;
>     if (stackBuffer.hasMore()) {
>       PRInt32 lineNumberSave;
>-      bool inRootContext = (!mStreamParser && (aKey == mRootContextKey));
>+      bool inRootContext = (!mStreamParser && !aKey);
>       if (inRootContext) {
>         mTokenizer->setLineNumber(mRootContextLineNumber);
>       } else {
>         // we aren't the root context, so save the line number on the
>         // *stack* so that we can restore it.
>         lineNumberSave = mTokenizer->getLineNumber();
>       }
> 
>@@ -528,25 +527,16 @@ nsHtml5Parser::Parse(const nsAString& aS
>     mDocWriteSpeculativeTreeBuilder->Flush();
>     mDocWriteSpeculativeTreeBuilder->DropHandles();
>     mExecutor->FlushSpeculativeLoads();
>   }
> 
>   return NS_OK;
> }
> 
>-/**
>- * This magic value is passed to the previous method on document.close()
>- */
>-NS_IMETHODIMP_(void *)
>-nsHtml5Parser::GetRootContextKey()
>-{
>-  return mRootContextKey;
>-}
>-
> NS_IMETHODIMP
> nsHtml5Parser::Terminate()
> {
>   // We should only call DidBuildModel once, so don't do anything if this is
>   // the second time that Terminate has been called.
>   if (mExecutor->IsComplete()) {
>     return NS_OK;
>   }
>@@ -662,17 +652,16 @@ nsHtml5Parser::Reset()
>                   "Reset called on a non-fragment parser.");
>   mExecutor->Reset();
>   mLastWasCR = false;
>   UnblockParser();
>   mDocumentClosed = false;
>   mStreamParser = nsnull;
>   mRootContextLineNumber = 1;
>   mParserInsertedScriptsBeingEvaluated = 0;
>-  mRootContextKey = nsnull;
>   mAtomTable.Clear(); // should be already cleared in the fragment case anyway
>   // Portable parser objects
>   mFirstBuffer->next = nsnull;
>   mFirstBuffer->setStart(0);
>   mFirstBuffer->setEnd(0);
>   mLastBuffer = mFirstBuffer;
> }
> 
>@@ -799,18 +788,17 @@ nsHtml5Parser::ParseUntilBlocked()
>     if (mBlocked || mExecutor->IsComplete()) {
>       return;
>     }
> 
>     // now we have a non-empty buffer
>     mFirstBuffer->adjust(mLastWasCR);
>     mLastWasCR = false;
>     if (mFirstBuffer->hasMore()) {
>-      bool inRootContext = (!mStreamParser &&
>-                              (mFirstBuffer->key == mRootContextKey));
>+      bool inRootContext = (!mStreamParser && !mFirstBuffer->key);
>       if (inRootContext) {
>         mTokenizer->setLineNumber(mRootContextLineNumber);
>       }
>       mLastWasCR = mTokenizer->tokenizeBuffer(mFirstBuffer);
>       if (inRootContext) {
>         mRootContextLineNumber = mTokenizer->getLineNumber();
>       }
>       if (mTreeBuilder->HasScript()) {
>diff --git a/parser/html/nsHtml5Parser.h b/parser/html/nsHtml5Parser.h
>--- a/parser/html/nsHtml5Parser.h
>+++ b/parser/html/nsHtml5Parser.h
>@@ -182,21 +182,16 @@ class nsHtml5Parser : public nsIParser,
>      */
>     NS_IMETHOD Parse(const nsAString& aSourceBuffer,
>                      void* aKey,
>                      const nsACString& aContentType,
>                      bool aLastCall,
>                      nsDTDMode aMode = eDTDMode_autodetect);
> 
>     /**
>-     * Gets the key passed to initial Parse()
>-     */
>-    NS_IMETHOD_(void *) GetRootContextKey();
>-
>-    /**
>      * Stops the parser prematurely
>      */
>     NS_IMETHOD Terminate();
> 
>     /**
>      * Don't call. For interface backwards compat only.
>      */
>     NS_IMETHOD ParseFragment(const nsAString& aSourceBuffer,
>diff --git a/parser/htmlparser/public/nsIParser.h b/parser/htmlparser/public/nsIParser.h
>--- a/parser/htmlparser/public/nsIParser.h
>+++ b/parser/htmlparser/public/nsIParser.h
>@@ -50,18 +50,18 @@
> #include "nsISupports.h"
> #include "nsIStreamListener.h"
> #include "nsIDTD.h"
> #include "nsStringGlue.h"
> #include "nsTArray.h"
> #include "nsIAtom.h"
> 
> #define NS_IPARSER_IID \
>-{ 0x0c8c3998, 0x9959, 0x496e, \
>-  { 0xbd, 0xd9, 0x0b, 0x6f, 0xc4, 0x1c, 0x3b, 0x87 } }
>+{ 0x30ffba62, 0x0928, 0x4503, \
>+  { 0xa8, 0x95, 0xd1, 0x32, 0x76, 0x40, 0x2a, 0x2a } }
> 
> // {41421C60-310A-11d4-816F-000064657374}
> #define NS_IDEBUG_DUMP_CONTENT_IID \
> { 0x41421c60, 0x310a, 0x11d4, \
> { 0x81, 0x6f, 0x0, 0x0, 0x64, 0x65, 0x73, 0x74 } }
> 
> class nsIContentSink;
> class nsIRequestObserver;
>@@ -227,20 +227,16 @@ class nsIParser : public nsISupports {
>                      void* aKey = 0,
>                      nsDTDMode aMode = eDTDMode_autodetect) = 0;
>     NS_IMETHOD Parse(const nsAString& aSourceBuffer,
>                      void* aKey,
>                      const nsACString& aMimeType,
>                      bool aLastCall,
>                      nsDTDMode aMode = eDTDMode_autodetect) = 0;
> 
>-    // Return a key, suitable for passing into one of the Parse methods above,
>-    // that will cause this parser to use the root context.
>-    NS_IMETHOD_(void *) GetRootContextKey() = 0;
>-    
>     NS_IMETHOD Terminate(void) = 0;
> 
>     /**
>      * This method gets called when you want to parse a fragment of HTML or XML
>      * surrounded by the context |aTagStack|. It requires that the parser have
>      * been given a fragment content sink.
>      *
>      * @param aSourceBuffer The XML or HTML that hasn't been parsed yet.
>diff --git a/parser/htmlparser/src/nsParser.cpp b/parser/htmlparser/src/nsParser.cpp
>--- a/parser/htmlparser/src/nsParser.cpp
>+++ b/parser/htmlparser/src/nsParser.cpp
>@@ -1070,31 +1070,16 @@ nsParser::PopContext()
>  *  @return  current state
>  */
> void
> nsParser::SetUnusedInput(nsString& aBuffer)
> {
>   mUnusedInput = aBuffer;
> }
> 
>-NS_IMETHODIMP_(void *)
>-nsParser::GetRootContextKey()
>-{
>-  CParserContext* pc = mParserContext;
>-  if (!pc) {
>-    return nsnull;
>-  }
>-
>-  while (pc->mPrevContext) {
>-    pc = pc->mPrevContext;
>-  }
>-
>-  return pc->mKey;
>-}
>-
> /**
>  *  Call this when you want to *force* the parser to terminate the
>  *  parsing process altogether. This is binary -- so once you terminate
>  *  you can't resume without restarting altogether.
>  */
> NS_IMETHODIMP
> nsParser::Terminate(void)
> {
>diff --git a/parser/htmlparser/src/nsParser.h b/parser/htmlparser/src/nsParser.h
>--- a/parser/htmlparser/src/nsParser.h
>+++ b/parser/htmlparser/src/nsParser.h
>@@ -194,18 +194,16 @@ class nsParser : public nsIParser,
>      * @return  TRUE if all went well -- FALSE otherwise
>      */
>     NS_IMETHOD Parse(const nsAString& aSourceBuffer,
>                      void* aKey,
>                      const nsACString& aContentType,
>                      bool aLastCall,
>                      nsDTDMode aMode = eDTDMode_autodetect);
> 
>-    NS_IMETHOD_(void *) GetRootContextKey();
>-
>     /**
>      * This method needs documentation
>      */
>     NS_IMETHOD ParseFragment(const nsAString& aSourceBuffer,
>                              nsTArray<nsString>& aTagStack);
>                              
>     /**
>      * This method gets called when the tokens have been consumed, and it's time

Comment 13

[Henri Sivonen (:hsivonen)]

Thanks for the reviews.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3a9d1729cd5c
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f662941aca6

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3a9d1729cd5c
https://hg.mozilla.org/mozilla-central/rev/7f662941aca6