Allow LoadNavigationErrorPage to run scripts

Move the LoadNavigationErrorPage call from DidFinishDocumentLoad to
RunScriptsAtDocumentReady, because the former is in a
ScriptForbiddenScope and caused an assertion to be triggered in
blink::beforeCallEnteredCallback when the beforeunload listeners were
ran as a part of the navigation.

BUG=590634

Review URL: https://codereview.chromium.org/1837483002

Cr-Commit-Position: refs/heads/master@{#383355}

15 files changed, 49 insertions, 42 deletions

diff --git a/components/test_runner/web_frame_test_proxy.h b/components/test_runner/web_frame_test_proxy.h
index b4d01b7..7f90583 100644
--- a/components/test_runner/web_frame_test_proxy.h
+++ b/components/test_runner/web_frame_test_proxy.h
@@ -113,9 +113,9 @@ class WebFrameTestProxy : public Base {
     Base::didChangeIcon(frame, icon_type);
   }
 
-  void didFinishDocumentLoad(blink::WebLocalFrame* frame, bool empty) override {
+  void didFinishDocumentLoad(blink::WebLocalFrame* frame) override {
     base_proxy_->DidFinishDocumentLoad(frame);
-    Base::didFinishDocumentLoad(frame, empty);
+    Base::didFinishDocumentLoad(frame);
   }
 
   void didHandleOnloadEvents(blink::WebLocalFrame* frame) override {
diff --git a/content/renderer/render_frame_impl.cc b/content/renderer/render_frame_impl.cc
index 67703b2..8d8d967 100644
--- a/content/renderer/render_frame_impl.cc
+++ b/content/renderer/render_frame_impl.cc
@@ -3355,8 +3355,7 @@ void RenderFrameImpl::didChangeIcon(blink::WebLocalFrame* frame,
   render_view_->didChangeIcon(frame, icon_type);
 }
 
-void RenderFrameImpl::didFinishDocumentLoad(blink::WebLocalFrame* frame,
-                                            bool document_is_empty) {
+void RenderFrameImpl::didFinishDocumentLoad(blink::WebLocalFrame* frame) {
   TRACE_EVENT1("navigation,benchmark", "RenderFrameImpl::didFinishDocumentLoad",
                "id", routing_id_);
   DCHECK_EQ(frame_, frame);
@@ -3372,6 +3371,26 @@ void RenderFrameImpl::didFinishDocumentLoad(blink::WebLocalFrame* frame,
 
   // Check whether we have new encoding name.
   UpdateEncoding(frame, frame->view()->pageEncoding().utf8());
+}
+
+void RenderFrameImpl::runScriptsAtDocumentReady(blink::WebLocalFrame* frame,
+                                                bool document_is_empty) {
+  DCHECK_EQ(frame_, frame);
+  base::WeakPtr<RenderFrameImpl> weak_self = weak_factory_.GetWeakPtr();
+
+  MojoBindingsController* mojo_bindings_controller =
+      MojoBindingsController::Get(this);
+  if (mojo_bindings_controller)
+    mojo_bindings_controller->RunScriptsAtDocumentReady();
+
+  if (!weak_self.get())
+    return;
+
+  GetContentClient()->renderer()->RunScriptsAtDocumentEnd(this);
+
+  // ContentClient might have deleted |frame| and |this| by now!
+  if (!weak_self.get())
+    return;
 
   // If this is an empty document with an http status code indicating an error,
   // we may want to display our own error page, so the user doesn't end up
@@ -3400,23 +3419,10 @@ void RenderFrameImpl::didFinishDocumentLoad(blink::WebLocalFrame* frame,
     error.unreachableURL = frame->document().url();
     error.domain = WebString::fromUTF8(error_domain);
     error.reason = http_status_code;
+    // This call may run scripts, e.g. via the beforeunload event.
     LoadNavigationErrorPage(frame->dataSource()->request(), error, true);
   }
-}
-
-void RenderFrameImpl::runScriptsAtDocumentReady(blink::WebLocalFrame* frame) {
-  base::WeakPtr<RenderFrameImpl> weak_self = weak_factory_.GetWeakPtr();
-
-  MojoBindingsController* mojo_bindings_controller =
-      MojoBindingsController::Get(this);
-  if (mojo_bindings_controller)
-    mojo_bindings_controller->RunScriptsAtDocumentReady();
-
-  if (!weak_self.get())
-    return;
-
-  GetContentClient()->renderer()->RunScriptsAtDocumentEnd(this);
-  // Do not use |this| or |frame|! ContentClient might have deleted them by now!
+  // Do not use |this| or |frame| here without checking |weak_self|.
 }
 
 void RenderFrameImpl::didHandleOnloadEvents(blink::WebLocalFrame* frame) {
diff --git a/content/renderer/render_frame_impl.h b/content/renderer/render_frame_impl.h
index 42f0f02..729c949 100644
--- a/content/renderer/render_frame_impl.h
+++ b/content/renderer/render_frame_impl.h
@@ -495,9 +495,9 @@ class CONTENT_EXPORT RenderFrameImpl
                        blink::WebTextDirection direction) override;
   void didChangeIcon(blink::WebLocalFrame* frame,
                      blink::WebIconURL::Type icon_type) override;
-  void didFinishDocumentLoad(blink::WebLocalFrame* frame,
-                             bool document_is_empty) override;
-  void runScriptsAtDocumentReady(blink::WebLocalFrame* frame) override;
+  void didFinishDocumentLoad(blink::WebLocalFrame* frame) override;
+  void runScriptsAtDocumentReady(blink::WebLocalFrame* frame,
+                                 bool document_is_empty) override;
   void didHandleOnloadEvents(blink::WebLocalFrame* frame) override;
   void didFailLoad(blink::WebLocalFrame* frame,
                    const blink::WebURLError& error,
diff --git a/content/renderer/render_view_browsertest.cc b/content/renderer/render_view_browsertest.cc
index 0695669..765a06e 100644
--- a/content/renderer/render_view_browsertest.cc
+++ b/content/renderer/render_view_browsertest.cc
@@ -1861,7 +1861,8 @@ TEST_F(RendererErrorPageTest, MAYBE_HttpStatusCodeErrorWithEmptyBody) {
 
   // Emulate a 4xx/5xx main resource response with an empty body.
   main_frame->didReceiveResponse(1, response);
-  main_frame->didFinishDocumentLoad(web_frame, true);
+  main_frame->didFinishDocumentLoad(web_frame);
+  main_frame->runScriptsAtDocumentReady(web_frame, true);
 
   // The error page itself is loaded asynchronously.
   FrameLoadWaiter(main_frame).Wait();
diff --git a/third_party/WebKit/Source/core/loader/EmptyClients.h b/third_party/WebKit/Source/core/loader/EmptyClients.h
index eed3a0f..da0e924 100644
--- a/third_party/WebKit/Source/core/loader/EmptyClients.h
+++ b/third_party/WebKit/Source/core/loader/EmptyClients.h
@@ -211,7 +211,7 @@ public:
     void dispatchDidCommitLoad(HistoryItem*, HistoryCommitType) override {}
     void dispatchDidFailProvisionalLoad(const ResourceError&, HistoryCommitType) override {}
     void dispatchDidFailLoad(const ResourceError&, HistoryCommitType) override {}
-    void dispatchDidFinishDocumentLoad(bool) override {}
+    void dispatchDidFinishDocumentLoad() override {}
     void dispatchDidFinishLoad() override {}
     void dispatchDidChangeThemeColor() override {}
 
@@ -255,7 +255,7 @@ public:
     void dispatchDidClearWindowObjectInMainWorld() override {}
     void documentElementAvailable() override {}
     void runScriptsAtDocumentElementAvailable() override {}
-    void runScriptsAtDocumentReady() override {}
+    void runScriptsAtDocumentReady(bool) override {}
 
     void didCreateScriptContext(v8::Local<v8::Context>, int extensionGroup, int worldId) override {}
     void willReleaseScriptContext(v8::Local<v8::Context>, int worldId) override {}
diff --git a/third_party/WebKit/Source/core/loader/FrameLoader.cpp b/third_party/WebKit/Source/core/loader/FrameLoader.cpp
index 9026d06..a149585 100644
--- a/third_party/WebKit/Source/core/loader/FrameLoader.cpp
+++ b/third_party/WebKit/Source/core/loader/FrameLoader.cpp
@@ -505,11 +505,11 @@ void FrameLoader::finishedParsing()
 
     if (client()) {
         ScriptForbiddenScope forbidScripts;
-        client()->dispatchDidFinishDocumentLoad(m_documentLoader ? m_documentLoader->isCommittedButEmpty() : true);
+        client()->dispatchDidFinishDocumentLoad();
     }
 
     if (client())
-        client()->runScriptsAtDocumentReady();
+        client()->runScriptsAtDocumentReady(m_documentLoader ? m_documentLoader->isCommittedButEmpty() : true);
 
     checkCompleted();
 
diff --git a/third_party/WebKit/Source/core/loader/FrameLoaderClient.h b/third_party/WebKit/Source/core/loader/FrameLoaderClient.h
index 9e85601..fb99716 100644
--- a/third_party/WebKit/Source/core/loader/FrameLoaderClient.h
+++ b/third_party/WebKit/Source/core/loader/FrameLoaderClient.h
@@ -96,7 +96,7 @@ public:
     virtual void dispatchDidCommitLoad(HistoryItem*, HistoryCommitType) = 0;
     virtual void dispatchDidFailProvisionalLoad(const ResourceError&, HistoryCommitType) = 0;
     virtual void dispatchDidFailLoad(const ResourceError&, HistoryCommitType) = 0;
-    virtual void dispatchDidFinishDocumentLoad(bool documentIsEmpty) = 0;
+    virtual void dispatchDidFinishDocumentLoad() = 0;
     virtual void dispatchDidFinishLoad() = 0;
     virtual void dispatchDidChangeThemeColor() = 0;
 
@@ -171,7 +171,7 @@ public:
     virtual void dispatchDidClearWindowObjectInMainWorld() = 0;
     virtual void documentElementAvailable() = 0;
     virtual void runScriptsAtDocumentElementAvailable() = 0;
-    virtual void runScriptsAtDocumentReady() = 0;
+    virtual void runScriptsAtDocumentReady(bool documentIsEmpty) = 0;
 
     virtual v8::Local<v8::Value> createTestInterface(const AtomicString& name) = 0;
 
diff --git a/third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp b/third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp
index bc04835..06eb954 100644
--- a/third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp
+++ b/third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp
@@ -194,10 +194,10 @@ void FrameLoaderClientImpl::runScriptsAtDocumentElementAvailable()
     // The callback might have deleted the frame, do not use |this|!
 }
 
-void FrameLoaderClientImpl::runScriptsAtDocumentReady()
+void FrameLoaderClientImpl::runScriptsAtDocumentReady(bool documentIsEmpty)
 {
     if (m_webFrame->client())
-        m_webFrame->client()->runScriptsAtDocumentReady(m_webFrame);
+        m_webFrame->client()->runScriptsAtDocumentReady(m_webFrame, documentIsEmpty);
     // The callback might have deleted the frame, do not use |this|!
 }
 
@@ -437,7 +437,7 @@ void FrameLoaderClientImpl::dispatchDidFinishLoading(DocumentLoader* loader,
         m_webFrame->client()->didFinishResourceLoad(m_webFrame, identifier);
 }
 
-void FrameLoaderClientImpl::dispatchDidFinishDocumentLoad(bool documentIsEmpty)
+void FrameLoaderClientImpl::dispatchDidFinishDocumentLoad()
 {
     if (!m_webFrame->parent()) {
         if (WebViewImpl* webview = m_webFrame->viewImpl())
@@ -449,7 +449,7 @@ void FrameLoaderClientImpl::dispatchDidFinishDocumentLoad(bool documentIsEmpty)
     // the fake WebLocalFrame that they create, which means that you should not
     // put any code touching `this` after the two lines below.
     if (m_webFrame->client())
-        m_webFrame->client()->didFinishDocumentLoad(m_webFrame, documentIsEmpty);
+        m_webFrame->client()->didFinishDocumentLoad(m_webFrame);
 }
 
 void FrameLoaderClientImpl::dispatchDidLoadResourceFromMemoryCache(const ResourceRequest& request, const ResourceResponse& response)
diff --git a/third_party/WebKit/Source/web/FrameLoaderClientImpl.h b/third_party/WebKit/Source/web/FrameLoaderClientImpl.h
index 0d800824..9ff9d19 100644
--- a/third_party/WebKit/Source/web/FrameLoaderClientImpl.h
+++ b/third_party/WebKit/Source/web/FrameLoaderClientImpl.h
@@ -61,7 +61,7 @@ public:
     void dispatchDidClearWindowObjectInMainWorld() override;
     void documentElementAvailable() override;
     void runScriptsAtDocumentElementAvailable() override;
-    void runScriptsAtDocumentReady() override;
+    void runScriptsAtDocumentReady(bool documentIsEmpty) override;
 
     void didCreateScriptContext(v8::Local<v8::Context>, int extensionGroup, int worldId) override;
     void willReleaseScriptContext(v8::Local<v8::Context>, int worldId) override;
@@ -97,7 +97,7 @@ public:
     void dispatchDidCommitLoad(HistoryItem*, HistoryCommitType) override;
     void dispatchDidFailProvisionalLoad(const ResourceError&, HistoryCommitType) override;
     void dispatchDidFailLoad(const ResourceError&, HistoryCommitType) override;
-    void dispatchDidFinishDocumentLoad(bool documentIsEmpty) override;
+    void dispatchDidFinishDocumentLoad() override;
     void dispatchDidFinishLoad() override;
 
     void dispatchDidChangeThemeColor() override;
diff --git a/third_party/WebKit/Source/web/WebEmbeddedWorkerImpl.cpp b/third_party/WebKit/Source/web/WebEmbeddedWorkerImpl.cpp
index 8d86fa7..6615a27 100644
--- a/third_party/WebKit/Source/web/WebEmbeddedWorkerImpl.cpp
+++ b/third_party/WebKit/Source/web/WebEmbeddedWorkerImpl.cpp
@@ -274,7 +274,7 @@ void WebEmbeddedWorkerImpl::willSendRequest(
         m_networkProvider->willSendRequest(frame->dataSource(), request);
 }
 
-void WebEmbeddedWorkerImpl::didFinishDocumentLoad(WebLocalFrame* frame, bool)
+void WebEmbeddedWorkerImpl::didFinishDocumentLoad(WebLocalFrame* frame)
 {
     ASSERT(!m_mainScriptLoader);
     ASSERT(!m_networkProvider);
diff --git a/third_party/WebKit/Source/web/WebEmbeddedWorkerImpl.h b/third_party/WebKit/Source/web/WebEmbeddedWorkerImpl.h
index 48ac58f..264e7e9 100644
--- a/third_party/WebKit/Source/web/WebEmbeddedWorkerImpl.h
+++ b/third_party/WebKit/Source/web/WebEmbeddedWorkerImpl.h
@@ -78,7 +78,7 @@ private:
     void willSendRequest(
         WebLocalFrame*, unsigned identifier, WebURLRequest&,
         const WebURLResponse& redirectResponse) override;
-    void didFinishDocumentLoad(WebLocalFrame*, bool documentIsEmpty) override;
+    void didFinishDocumentLoad(WebLocalFrame*) override;
 
     // WebDevToolsAgentClient overrides.
     void sendProtocolMessage(int sessionId, int callId, const WebString&, const WebString&) override;
diff --git a/third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp b/third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp
index 50a864b..5abee88 100644
--- a/third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp
+++ b/third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp
@@ -166,7 +166,7 @@ void WebSharedWorkerImpl::willSendRequest(
         m_networkProvider->willSendRequest(frame->dataSource(), request);
 }
 
-void WebSharedWorkerImpl::didFinishDocumentLoad(WebLocalFrame* frame, bool)
+void WebSharedWorkerImpl::didFinishDocumentLoad(WebLocalFrame* frame)
 {
     ASSERT(!m_loadingDocument);
     ASSERT(!m_mainScriptLoader);
diff --git a/third_party/WebKit/Source/web/WebSharedWorkerImpl.h b/third_party/WebKit/Source/web/WebSharedWorkerImpl.h
index bf9ea66..20c16bb 100644
--- a/third_party/WebKit/Source/web/WebSharedWorkerImpl.h
+++ b/third_party/WebKit/Source/web/WebSharedWorkerImpl.h
@@ -89,7 +89,7 @@ public:
     // WebFrameClient methods to support resource loading thru the 'shadow page'.
     WebApplicationCacheHost* createApplicationCacheHost(WebApplicationCacheHostClient*) override;
     void willSendRequest(WebLocalFrame*, unsigned identifier, WebURLRequest&, const WebURLResponse& redirectResponse) override;
-    void didFinishDocumentLoad(WebLocalFrame*, bool documentIsEmpty) override;
+    void didFinishDocumentLoad(WebLocalFrame*) override;
     bool isControlledByServiceWorker(WebDataSource&) override;
     int64_t serviceWorkerID(WebDataSource&) override;
 
diff --git a/third_party/WebKit/Source/web/tests/WebFrameTest.cpp b/third_party/WebKit/Source/web/tests/WebFrameTest.cpp
index b7367ce..5466924 100644
--- a/third_party/WebKit/Source/web/tests/WebFrameTest.cpp
+++ b/third_party/WebKit/Source/web/tests/WebFrameTest.cpp
@@ -8415,7 +8415,7 @@ public:
     }
     void didStartProvisionalLoad(WebLocalFrame*, double) override { EXPECT_EQ(1, m_callbackCount++); }
     void didCommitProvisionalLoad(WebLocalFrame*, const WebHistoryItem&, WebHistoryCommitType) override { EXPECT_EQ(2, m_callbackCount++); }
-    void didFinishDocumentLoad(WebLocalFrame*, bool documentIsEmpty) override { EXPECT_EQ(3, m_callbackCount++); }
+    void didFinishDocumentLoad(WebLocalFrame*) override { EXPECT_EQ(3, m_callbackCount++); }
     void didHandleOnloadEvents(WebLocalFrame*) override { EXPECT_EQ(4, m_callbackCount++); }
     void didFinishLoad(WebLocalFrame*) override { EXPECT_EQ(5, m_callbackCount++); }
     void didStopLoading() override
diff --git a/third_party/WebKit/public/web/WebFrameClient.h b/third_party/WebKit/public/web/WebFrameClient.h
index 2ec7b28..8ac6ce0f 100644
--- a/third_party/WebKit/public/web/WebFrameClient.h
+++ b/third_party/WebKit/public/web/WebFrameClient.h
@@ -321,11 +321,11 @@ public:
 
     // The frame's document finished loading.
     // This method may not execute JavaScript code.
-    virtual void didFinishDocumentLoad(WebLocalFrame*, bool documentIsEmpty) { }
+    virtual void didFinishDocumentLoad(WebLocalFrame*) { }
 
     // Like |didFinishDocumentLoad|, except this method may run JavaScript
     // code (and possibly invalidate the frame).
-    virtual void runScriptsAtDocumentReady(WebLocalFrame*) { }
+    virtual void runScriptsAtDocumentReady(WebLocalFrame*, bool documentIsEmpty) { }
 
     // The 'load' event was dispatched.
     virtual void didHandleOnloadEvents(WebLocalFrame*) { }