document.open(): Check frame_ after StopAllLoaders

TimothyGu · speednoisemovement · commit 1f0040675e06 · 2018-09-05T18:45:49.000Z
FrameLoader::StopAllLoaders() has this explicit note: Warning: stopAllLoaders can and will detach the LocalFrame out from under you. All callers need to either protect the LocalFrame or guarantee they won't in any way access the LocalFrame after stopAllLoaders returns. Check frame_'s existence after the call to prevent a NULL dereference. Bug: 879366 Change-Id: I1e537374f59fbad7b069f9de63cfa3b6b2b2b00c Reviewed-on: https://chromium-review.googlesource.com/1198022 Reviewed-by: Nate Chapin <japhet@chromium.org> Reviewed-by: Kent Tamura <tkent@chromium.org> Reviewed-by: Hayato Ito <hayato@chromium.org> Commit-Queue: Timothy Gu <timothygu@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#587933}(cherry picked from commit 09b4427) Reviewed-on: https://chromium-review.googlesource.com/1207612 Reviewed-by: Leonard Grey <lgrey@chromium.org> Cr-Commit-Position: refs/branch-heads/3538@{#61} Cr-Branched-From: 79f7c91-refs/heads/master@{#587811}
diff --git a/third_party/WebKit/LayoutTests/fast/dom/Document/open-with-pending-load2-expected.txt b/third_party/WebKit/LayoutTests/fast/dom/Document/open-with-pending-load2-expected.txt
@@ -0,0 +1 @@
+This tests that calling document.open on a document that has a pending load correctly cancels the load and does not crash even if the frame is removed.
diff --git a/third_party/WebKit/LayoutTests/fast/dom/Document/open-with-pending-load2.html b/third_party/WebKit/LayoutTests/fast/dom/Document/open-with-pending-load2.html
@@ -0,0 +1,19 @@
+<script>
+if (window.testRunner)
+    testRunner.dumpAsText();
+</script>
+<body>
+This tests that calling document.open on a document that has a pending load correctly cancels the load and does not crash even if the frame is removed.
+<script>
+const div = document.body.appendChild(document.createElement("div"));
+div.innerHTML = "<iframe src='data:text/html,'></iframe>";
+const frame = div.childNodes[0];
+const client = new frame.contentWindow.XMLHttpRequest();
+client.open("GET", "data:text/html,");
+client.onabort = e => {
+  div.remove();
+};
+client.send();
+frame.contentWindow.document.open();
+</script>
+</body>
diff --git a/third_party/blink/renderer/core/dom/document.cc b/third_party/blink/renderer/core/dom/document.cc
@@ -3108,7 +3108,7 @@ void Document::open() {
   if (frame_ && frame_->Loader().HasProvisionalNavigation()) {
     frame_->Loader().StopAllLoaders();
     // Navigations handled by the client should also be cancelled.
-    if (frame_->Client())
+    if (frame_ && frame_->Client())
       frame_->Client()->AbortClientNavigation();
   }
 

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1 @@`
	`1`	`+This tests that calling document.open on a document that has a pending load correctly cancels the load and does not crash even if the frame is removed.`
Original file line number	Diff line number	Diff line change
`@@ -3108,7 +3108,7 @@ void Document::open() {`
`3108`	`3108`	`if (frame_ && frame_->Loader().HasProvisionalNavigation()) {`
`3109`	`3109`	`frame_->Loader().StopAllLoaders();`
`3110`	`3110`	`// Navigations handled by the client should also be cancelled.`
`3111`		`- if (frame_->Client())`
	`3111`	`+ if (frame_ && frame_->Client())`
`3112`	`3112`	`frame_->Client()->AbortClientNavigation();`
`3113`	`3113`	`}`
`3114`	`3114`