Bug 1330294 - Fixing disabled script tags that cause crashes in disabled SVG nodes r?hsivonen r?smaug draft
authorJonathan Kingston <jkt@mozilla.com>
Wed, 11 Jan 2017 16:29:13 +0000
changeset 459656 5036ab65f3afaa0aaac1b575dc33845ed90d8b1c
parent 458771 2963cf6be7f830c0d2155e2968cfc53585868a76
child 541957 427bab293e01d1eb3947e334ac826275f2d90973
push id41288
push userjkingston@mozilla.com
push dateThu, 12 Jan 2017 12:02:47 +0000
reviewershsivonen, smaug
bugs1330294
milestone53.0a1
Bug 1330294 - Fixing disabled script tags that cause crashes in disabled SVG nodes r?hsivonen r?smaug MozReview-Commit-ID: Lr4s98aZM4W
dom/xml/nsXMLContentSink.cpp
dom/xml/nsXMLFragmentContentSink.cpp
dom/xslt/xslt/txMozillaXMLOutput.cpp
layout/svg/tests/chrome.ini
layout/svg/tests/svg_example_script.svg
layout/svg/tests/test_disabled.html
layout/svg/tests/test_disabled_chrome.html
parser/html/nsHtml5TreeOpExecutor.cpp
parser/html/nsHtml5TreeOperation.cpp
--- a/dom/xml/nsXMLContentSink.cpp
+++ b/dom/xml/nsXMLContentSink.cpp
@@ -471,18 +471,22 @@ nsXMLContentSink::CreateElement(const ch
   RefPtr<Element> content;
   rv = NS_NewElement(getter_AddRefs(content), ni.forget(), aFromParser);
   NS_ENSURE_SUCCESS(rv, rv);
 
   if (aNodeInfo->Equals(nsGkAtoms::script, kNameSpaceID_XHTML)
       || aNodeInfo->Equals(nsGkAtoms::script, kNameSpaceID_SVG)
     ) {
     nsCOMPtr<nsIScriptElement> sele = do_QueryInterface(content);
-    sele->SetScriptLineNumber(aLineNumber);
-    sele->SetCreatorParser(GetParser());
+    if (sele) {
+      sele->SetScriptLineNumber(aLineNumber);
+      sele->SetCreatorParser(GetParser());
+    } else {
+      MOZ_ASSERT(nsNameSpaceManager::GetInstance()->mSVGDisabled, "Node didn't QI to script, but SVG wasn't disabled.");
+    }
   }
 
   // XHTML needs some special attention
   if (aNodeInfo->NamespaceEquals(kNameSpaceID_XHTML)) {
     mPrettyPrintHasFactoredElements = true;
   }
   else {
     // If we care, find out if we just used a special factory.
@@ -550,16 +554,20 @@ nsXMLContentSink::CloseElement(nsIConten
       !nodeInfo->NamespaceEquals(kNameSpaceID_SVG)) {
     return NS_OK;
   }
 
   if (nodeInfo->Equals(nsGkAtoms::script, kNameSpaceID_XHTML)
       || nodeInfo->Equals(nsGkAtoms::script, kNameSpaceID_SVG)
     ) {
     nsCOMPtr<nsIScriptElement> sele = do_QueryInterface(aContent);
+    if (!sele) {
+      MOZ_ASSERT(nsNameSpaceManager::GetInstance()->mSVGDisabled, "Node didn't QI to script, but SVG wasn't disabled.");
+      return NS_OK;
+    }
 
     if (mPreventScriptExecution) {
       sele->PreventExecution();
       return NS_OK;
     }
 
     // Always check the clock in nsContentSink right after a script
     StopDeflecting();
--- a/dom/xml/nsXMLFragmentContentSink.cpp
+++ b/dom/xml/nsXMLFragmentContentSink.cpp
@@ -224,18 +224,21 @@ nsXMLFragmentContentSink::CreateElement(
 nsresult
 nsXMLFragmentContentSink::CloseElement(nsIContent* aContent)
 {
   // don't do fancy stuff in nsXMLContentSink
   if (mPreventScriptExecution &&
       (aContent->IsHTMLElement(nsGkAtoms::script),
        aContent->IsSVGElement(nsGkAtoms::script))) {
     nsCOMPtr<nsIScriptElement> sele = do_QueryInterface(aContent);
-    NS_ASSERTION(sele, "script did QI correctly!");
-    sele->PreventExecution();
+    if (sele) {
+      sele->PreventExecution();
+    } else {
+      NS_ASSERTION(nsNameSpaceManager::GetInstance()->mSVGDisabled, "Script did QI correctly, but wasn't a disabled SVG!");
+    }
   }
   return NS_OK;
 }
 
 void
 nsXMLFragmentContentSink::MaybeStartLayout(bool aIgnorePendingSheets)
 {
   return;
--- a/dom/xslt/xslt/txMozillaXMLOutput.cpp
+++ b/dom/xslt/xslt/txMozillaXMLOutput.cpp
@@ -291,23 +291,26 @@ txMozillaXMLOutput::endElement()
                                          nsGkAtoms::applet,
                                          nsGkAtoms::select,
                                          nsGkAtoms::textarea) ||
             element->IsSVGElement(nsGkAtoms::title)) {
             element->DoneAddingChildren(true);
         } else if (element->IsSVGElement(nsGkAtoms::script) ||
                    element->IsHTMLElement(nsGkAtoms::script)) {
             nsCOMPtr<nsIScriptElement> sele = do_QueryInterface(element);
-            MOZ_ASSERT(sele, "script elements need to implement nsIScriptElement");
-            bool block = sele->AttemptToExecute();
-            // If the act of insertion evaluated the script, we're fine.
-            // Else, add this script element to the array of loading scripts.
-            if (block) {
-                rv = mNotifier->AddScriptElement(sele);
-                NS_ENSURE_SUCCESS(rv, rv);
+            if (sele) {
+              bool block = sele->AttemptToExecute();
+              // If the act of insertion evaluated the script, we're fine.
+              // Else, add this script element to the array of loading scripts.
+              if (block) {
+                  rv = mNotifier->AddScriptElement(sele);
+                  NS_ENSURE_SUCCESS(rv, rv);
+              }
+            } else {
+              MOZ_ASSERT(nsNameSpaceManager::GetInstance()->mSVGDisabled, "Script elements need to implement nsIScriptElement and SVG wasn't disabled.");
             }
         } else if (element->IsAnyOfHTMLElements(nsGkAtoms::input,
                                                 nsGkAtoms::button,
                                                 nsGkAtoms::menuitem,
                                                 nsGkAtoms::audio,
                                                 nsGkAtoms::video)) {
           element->DoneCreatingElement();
         }   
--- a/layout/svg/tests/chrome.ini
+++ b/layout/svg/tests/chrome.ini
@@ -1,6 +1,7 @@
 [DEFAULT]
 
 support-files =
   svg_example_test.html
+  svg_example_script.svg
 
 [test_disabled_chrome.html]
new file mode 100644
--- /dev/null
+++ b/layout/svg/tests/svg_example_script.svg
@@ -0,0 +1,7 @@
+<svg version="1.1">
+  <script>
+    document.documentElement.style.backgroundColor = 'rebeccapurple';
+    throw "badment, should never fire.";
+  </script>
+  <g><circle cx="25.8" cy="9.3" r="1.5"/></g>
+</svg>
--- a/layout/svg/tests/test_disabled.html
+++ b/layout/svg/tests/test_disabled.html
@@ -29,16 +29,25 @@ https://bugzilla.mozilla.org/show_bug.cg
               fill: currentColor;
           }
       </style>
       <g><circle cx="25.8" cy="9.3" r="1.5"/></g>
     </svg>
     `;
     is(t.firstChild.tagName.toLowerCase(), 'svg');
 
+    // This test crashes if the script tags are not handled correctly
+    t.innerHTML = `<svg version="1.1">
+      <scri` + `pt>
+        throw "badment, should never fire.";
+      </scri` + `pt>
+      <g><circle cx="25.8" cy="9.3" r="1.5"/></g>
+    </svg>`;
+    is(t.firstChild.tagName.toLowerCase(), 'svg');
+
     t.innerHTML = null;
     t.appendChild(document.createElementNS("http://www.w3.org/2000/svg", "svg"));
     is(t.firstChild.namespaceURI, "http://www.w3.org/2000/svg");
     t.firstChild.appendChild(document.createElementNS("http://www.w3.org/2000/svg", "script"));
     is(t.firstChild.firstChild.namespaceURI, "http://www.w3.org/2000/svg");
     t.firstChild.firstChild.textContent = "1&2<3>4\xA0";
     is(t.innerHTML, '<svg><script>1&amp;2&lt;3&gt;4&nbsp;\u003C/script></svg>');
 
--- a/layout/svg/tests/test_disabled_chrome.html
+++ b/layout/svg/tests/test_disabled_chrome.html
@@ -32,22 +32,28 @@ https://bugzilla.mozilla.org/show_bug.cg
     url = "http://mochi.test:8888/chrome/layout/svg/tests/svg_example_test.html";
     const iframeEl = document.createElement('iframe');
     iframeEl.src = url;
     let loadPromise = ContentTaskUtils.waitForEvent(iframeEl, 'load', false);
     t.appendChild(iframeEl);
     yield loadPromise;
 
     const contentBR = iframeEl.contentDocument.body.getBoundingClientRect();
-
-yield new Promise(_ => setTimeout(_, 1000));
     ok(chromeBR.height > contentBR.height, "Chrome content height should be bigger than content due to layout");
 
     ok(!("hasExtension" in iframeEl.contentDocument.getElementById('svgel')), 'SVG is disabled so no hasExtension support is available in content iframe');
     ok(chromeIframeEl.contentDocument.getElementById('svgel').hasExtension("http://www.w3.org/1998/Math/MathML"), 'SVG namespace support is enabled in chrome iframe');
 
+    url = "http://mochi.test:8888/chrome/layout/svg/tests/svg_example_script.svg";
+    const iframeElScript = document.createElement("iframe");
+    let loadPromiseScript = ContentTaskUtils.waitForEvent(iframeElScript, "load", false);
+    iframeElScript.src = url;
+    t.appendChild(iframeElScript);
+    yield loadPromiseScript;
+    ok(!iframeElScript.contentDocument.documentElement.style, "Content should not be styled");
+
     SpecialPowers.setBoolPref("svg.disabled", initialPrefValue);
   });
 </script>
 </pre>
 </body>
 </html>
 
--- a/parser/html/nsHtml5TreeOpExecutor.cpp
+++ b/parser/html/nsHtml5TreeOpExecutor.cpp
@@ -636,16 +636,20 @@ nsHtml5TreeOpExecutor::RunScript(nsICont
     // We are in createContextualFragment() or in the upcoming document.parse().
     // Do nothing. Let's not even mark scripts malformed here, because that
     // could cause serialization weirdness later.
     return;
   }
 
   NS_ASSERTION(aScriptElement, "No script to run");
   nsCOMPtr<nsIScriptElement> sele = do_QueryInterface(aScriptElement);
+  if (!sele) {
+    MOZ_ASSERT(nsNameSpaceManager::GetInstance()->mSVGDisabled, "Node didn't QI to script, but SVG wasn't disabled.");
+    return;
+  }
   
   if (!mParser) {
     NS_ASSERTION(sele->IsMalformed(), "Script wasn't marked as malformed.");
     // We got here not because of an end tag but because the tree builder
     // popped an incomplete script element on EOF. Returning here to avoid
     // calling back into mParser anymore.
     return;
   }
--- a/parser/html/nsHtml5TreeOperation.cpp
+++ b/parser/html/nsHtml5TreeOperation.cpp
@@ -591,18 +591,21 @@ nsHtml5TreeOperation::GetFosterParent(ns
   nsIContent* tableParent = aTable->GetParent();
   return IsElementOrTemplateContent(tableParent) ? tableParent : aStackParent;
 }
 
 void
 nsHtml5TreeOperation::PreventScriptExecution(nsIContent* aNode)
 {
   nsCOMPtr<nsIScriptElement> sele = do_QueryInterface(aNode);
-  MOZ_ASSERT(sele);
-  sele->PreventExecution();
+  if (sele) {
+    sele->PreventExecution();
+  } else {
+    MOZ_ASSERT(nsNameSpaceManager::GetInstance()->mSVGDisabled, "Node didn't QI to script, but SVG wasn't disabled.");
+  }
 }
 
 void
 nsHtml5TreeOperation::DoneAddingChildren(nsIContent* aNode)
 {
   aNode->DoneAddingChildren(aNode->HasParserNotified());
 }
 
@@ -829,19 +832,22 @@ nsHtml5TreeOperation::Perform(nsHtml5Tre
       } else {
         MOZ_ASSERT(nsNameSpaceManager::GetInstance()->mSVGDisabled, "Node didn't QI to style, but SVG wasn't disabled.");
       }
       return NS_OK;
     }
     case eTreeOpSetScriptLineNumberAndFreeze: {
       nsIContent* node = *(mOne.node);
       nsCOMPtr<nsIScriptElement> sele = do_QueryInterface(node);
-      NS_ASSERTION(sele, "Node didn't QI to script.");
-      sele->SetScriptLineNumber(mFour.integer);
-      sele->FreezeUriAsyncDefer();
+      if (sele) {
+        sele->SetScriptLineNumber(mFour.integer);
+        sele->FreezeUriAsyncDefer();
+      } else {
+        MOZ_ASSERT(nsNameSpaceManager::GetInstance()->mSVGDisabled, "Node didn't QI to script, but SVG wasn't disabled.");
+      }
       return NS_OK;
     }
     case eTreeOpSvgLoad: {
       nsIContent* node = *(mOne.node);
       SvgLoad(node);
       return NS_OK;
     }
     case eTreeOpMaybeComplainAboutCharset: {