Bug 1550845. Don't terminate the old parser, if we have one, in document.open() until after we remove event listeners. r=hsivonen
authorBoris Zbarsky <bzbarsky@mit.edu>
Sun, 12 May 2019 02:47:58 +0000
changeset 474570 bb69a96aea8403f807a74652168c71c97da6fbeb
parent 474569 c27a9d7837349ab8b0a49e00892346c5a1351d21
child 474571 09ef16b1c51fa6917986aea632e61a3f153b7ce8
push id36042
push userdvarga@mozilla.com
push dateTue, 21 May 2019 04:19:40 +0000
treeherdermozilla-central@ca560ff55451 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewershsivonen
bugs1550845
milestone69.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1550845. Don't terminate the old parser, if we have one, in document.open() until after we remove event listeners. r=hsivonen This way we won't accidentally run script during termination, hopefully. Differential Revision: https://phabricator.services.mozilla.com/D30763
dom/html/nsHTMLDocument.cpp
--- a/dom/html/nsHTMLDocument.cpp
+++ b/dom/html/nsHTMLDocument.cpp
@@ -1015,30 +1015,19 @@ Document* nsHTMLDocument::Open(const Opt
   if (!callerDoc->NodePrincipal()->Equals(NodePrincipal())) {
     aError.Throw(NS_ERROR_DOM_SECURITY_ERR);
     return nullptr;
   }
 
   // Step 5 -- if we have an active parser with a nonzero script nesting level,
   // just no-op.
   //
-  // If we have a parser that has a zero script nesting level, we need to
-  // properly terminate it before setting up a new parser.  See the similar code
-  // in WriteCommon that handles the !IsInsertionPointDefined() case and should
-  // stay in sync with this code.
-  if (mParser && !mParser->HasNonzeroScriptNestingLevel()) {
-    // Make sure we don't re-enter.
-    IgnoreOpensDuringUnload ignoreOpenGuard(this);
-    mParser->Terminate();
-    MOZ_RELEASE_ASSERT(!mParser, "mParser should have been null'd out");
-  }
-
   // The mParserAborted check here is probably wrong.  Removing it is
   // tracked in https://bugzilla.mozilla.org/show_bug.cgi?id=1475000
-  if (mParser || mParserAborted) {
+  if ((mParser && mParser->HasNonzeroScriptNestingLevel()) || mParserAborted) {
     return this;
   }
 
   // Step 6 -- check for open() during unload.  Per spec, this is just a check
   // of the ignore-opens-during-unload counter, but our unload event code
   // doesn't affect that counter yet (unlike pagehide and beforeunload, which
   // do), so we check for unload directly.
   if (ShouldIgnoreOpens()) {
@@ -1092,16 +1081,32 @@ Document* nsHTMLDocument::Open(const Opt
     if (win->GetExtantDoc() == this) {
       if (EventListenerManager* elm =
               nsGlobalWindowInner::Cast(win)->GetExistingListenerManager()) {
         elm->RemoveAllListeners();
       }
     }
   }
 
+  // If we have a parser that has a zero script nesting level, we need to
+  // properly terminate it.  We do that after we've removed all the event
+  // listeners (so termination won't trigger event listeners if it does
+  // something to the DOM), but before we remove all elements from the document
+  // (so if termination does modify the DOM in some way we will just blow it
+  // away immediately.  See the similar code in WriteCommon that handles the
+  // !IsInsertionPointDefined() case and should stay in sync with this code.
+  if (mParser) {
+    MOZ_ASSERT(!mParser->HasNonzeroScriptNestingLevel(),
+               "Why didn't we take the early return?");
+    // Make sure we don't re-enter.
+    IgnoreOpensDuringUnload ignoreOpenGuard(this);
+    mParser->Terminate();
+    MOZ_RELEASE_ASSERT(!mParser, "mParser should have been null'd out");
+  }
+
   // Step 10 -- remove all our DOM kids without firing any mutation events.
   {
     // We want to ignore any recursive calls to Open() that happen while
     // disconnecting the node tree.  The spec doesn't say to do this, but the
     // spec also doesn't envision unload events on subframes firing while we do
     // this, while all browsers fire them in practice.  See
     // <https://github.com/whatwg/html/issues/4611>.
     IgnoreOpensDuringUnload ignoreOpenGuard(this);