Deal with frame removal by content scripts

Blink and the RenderFrame implementations are currently not prepared to deal
with frame detachments in their callbacks. Consequently, extension code
(content scripts, chrome.app.window.create) that run arbitrary code in the
"document element created" and "document loaded" notifications may result in
unexpected invalidation of memory, resulting in a UAF.

This patch fixes the bug by moving all code that runs untrusted code from
observers to dedicated callbacks, which are only run at a safe point.
All document parsers in Blink have been modified to make sure that they still
work even when the document creation is interrupted by frame removal.

An extensive set of tests for all different kinds of documents, frame removal
methods (e.g. synchronously / in mutation events / ...) and injection points
(document start/end) have been added to avoid regressions.

BUG=582008

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

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

8 files changed, 124 insertions, 8 deletions

diff --git a/extensions/renderer/dispatcher.cc b/extensions/renderer/dispatcher.cc
index 5ff2616e..4bca09c 100644
--- a/extensions/renderer/dispatcher.cc
+++ b/extensions/renderer/dispatcher.cc
@@ -500,6 +500,24 @@ void Dispatcher::DidCreateDocumentElement(blink::WebLocalFrame* frame) {
   }
 }
 
+void Dispatcher::RunScriptsAtDocumentStart(content::RenderFrame* render_frame) {
+  ExtensionFrameHelper* frame_helper = ExtensionFrameHelper::Get(render_frame);
+  if (!frame_helper)
+    return;  // The frame is invisible to extensions.
+
+  frame_helper->RunScriptsAtDocumentStart();
+  // |frame_helper| and |render_frame| might be dead by now.
+}
+
+void Dispatcher::RunScriptsAtDocumentEnd(content::RenderFrame* render_frame) {
+  ExtensionFrameHelper* frame_helper = ExtensionFrameHelper::Get(render_frame);
+  if (!frame_helper)
+    return;  // The frame is invisible to extensions.
+
+  frame_helper->RunScriptsAtDocumentEnd();
+  // |frame_helper| and |render_frame| might be dead by now.
+}
+
 void Dispatcher::OnExtensionResponse(int request_id,
                                      bool success,
                                      const base::ListValue& response,
diff --git a/extensions/renderer/dispatcher.h b/extensions/renderer/dispatcher.h
index 40194a0..eb4372c 100644
--- a/extensions/renderer/dispatcher.h
+++ b/extensions/renderer/dispatcher.h
@@ -108,8 +108,14 @@ class Dispatcher : public content::RenderProcessObserver,
       v8::Local<v8::Context> v8_context,
       const GURL& url);
 
+  // This method is not allowed to run JavaScript code in the frame.
   void DidCreateDocumentElement(blink::WebLocalFrame* frame);
 
+  // These methods may run (untrusted) JavaScript code in the frame, and
+  // cause |render_frame| to become invalid.
+  void RunScriptsAtDocumentStart(content::RenderFrame* render_frame);
+  void RunScriptsAtDocumentEnd(content::RenderFrame* render_frame);
+
   void OnExtensionResponse(int request_id,
                            bool success,
                            const base::ListValue& response,
diff --git a/extensions/renderer/extension_frame_helper.cc b/extensions/renderer/extension_frame_helper.cc
index 300e16e..ba47a7c 100644
--- a/extensions/renderer/extension_frame_helper.cc
+++ b/extensions/renderer/extension_frame_helper.cc
@@ -58,6 +58,22 @@ bool RenderFrameMatches(const ExtensionFrameHelper* frame_helper,
   return true;
 }
 
+// Runs every callback in |callbacks_to_be_run_and_cleared| while |frame_helper|
+// is valid, and clears |callbacks_to_be_run_and_cleared|.
+void RunCallbacksWhileFrameIsValid(
+    base::WeakPtr<ExtensionFrameHelper> frame_helper,
+    std::vector<base::Closure>* callbacks_to_be_run_and_cleared) {
+  // The JavaScript code can cause re-entrancy. To avoid a deadlock, don't run
+  // callbacks that are added during the iteration.
+  std::vector<base::Closure> callbacks;
+  callbacks_to_be_run_and_cleared->swap(callbacks);
+  for (auto& callback : callbacks) {
+    callback.Run();
+    if (!frame_helper.get())
+      return;  // Frame and ExtensionFrameHelper invalidated by callback.
+  }
+}
+
 }  // namespace
 
 ExtensionFrameHelper::ExtensionFrameHelper(content::RenderFrame* render_frame,
@@ -68,7 +84,8 @@ ExtensionFrameHelper::ExtensionFrameHelper(content::RenderFrame* render_frame,
       tab_id_(-1),
       browser_window_id_(-1),
       extension_dispatcher_(extension_dispatcher),
-      did_create_current_document_element_(false) {
+      did_create_current_document_element_(false),
+      weak_ptr_factory_(this) {
   g_frame_helpers.Get().insert(this);
 }
 
@@ -123,6 +140,29 @@ void ExtensionFrameHelper::DidCreateNewDocument() {
   did_create_current_document_element_ = false;
 }
 
+void ExtensionFrameHelper::RunScriptsAtDocumentStart() {
+  DCHECK(did_create_current_document_element_);
+  RunCallbacksWhileFrameIsValid(weak_ptr_factory_.GetWeakPtr(),
+                                &document_element_created_callbacks_);
+  // |this| might be dead by now.
+}
+
+void ExtensionFrameHelper::RunScriptsAtDocumentEnd() {
+  RunCallbacksWhileFrameIsValid(weak_ptr_factory_.GetWeakPtr(),
+                                &document_load_finished_callbacks_);
+  // |this| might be dead by now.
+}
+
+void ExtensionFrameHelper::ScheduleAtDocumentStart(
+    const base::Closure& callback) {
+  document_element_created_callbacks_.push_back(callback);
+}
+
+void ExtensionFrameHelper::ScheduleAtDocumentEnd(
+    const base::Closure& callback) {
+  document_load_finished_callbacks_.push_back(callback);
+}
+
 void ExtensionFrameHelper::DidMatchCSS(
     const blink::WebVector<blink::WebString>& newly_matching_selectors,
     const blink::WebVector<blink::WebString>& stopped_matching_selectors) {
diff --git a/extensions/renderer/extension_frame_helper.h b/extensions/renderer/extension_frame_helper.h
index a710d4f..57ac3b6 100644
--- a/extensions/renderer/extension_frame_helper.h
+++ b/extensions/renderer/extension_frame_helper.h
@@ -8,6 +8,7 @@
 #include <vector>
 
 #include "base/macros.h"
+#include "base/memory/weak_ptr.h"
 #include "content/public/common/console_message_level.h"
 #include "content/public/renderer/render_frame_observer.h"
 #include "content/public/renderer/render_frame_observer_tracker.h"
@@ -61,6 +62,24 @@ class ExtensionFrameHelper
     return did_create_current_document_element_;
   }
 
+  // Called when the document element has been inserted in this frame. This
+  // method may invoke untrusted JavaScript code that invalidate the frame and
+  // this ExtensionFrameHelper.
+  void RunScriptsAtDocumentStart();
+
+  // Called after the DOMContentLoaded event has fired.
+  void RunScriptsAtDocumentEnd();
+
+  // Schedule a callback, to be run at the next RunScriptsAtDocumentStart
+  // notification. Only call this when you are certain that there will be such a
+  // notification, e.g. from RenderFrameObserver::DidCreateDocumentElement.
+  // Otherwise the callback is never invoked, or invoked for a document that you
+  // were not expecting.
+  void ScheduleAtDocumentStart(const base::Closure& callback);
+
+  // Schedule a callback, to be run at the next RunScriptsAtDocumentEnd call.
+  void ScheduleAtDocumentEnd(const base::Closure& callback);
+
  private:
   // RenderFrameObserver implementation.
   void DidCreateDocumentElement() override;
@@ -113,6 +132,14 @@ class ExtensionFrameHelper
   // Whether or not the current document element has been created.
   bool did_create_current_document_element_;
 
+  // Callbacks to be run at the next RunScriptsAtDocumentStart notification.
+  std::vector<base::Closure> document_element_created_callbacks_;
+
+  // Callbacks to be run at the next RunScriptsAtDocumentEnd notification.
+  std::vector<base::Closure> document_load_finished_callbacks_;
+
+  base::WeakPtrFactory<ExtensionFrameHelper> weak_ptr_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(ExtensionFrameHelper);
 };
 
diff --git a/extensions/renderer/render_frame_observer_natives.cc b/extensions/renderer/render_frame_observer_natives.cc
index 1c1f06e8..7c507b3 100644
--- a/extensions/renderer/render_frame_observer_natives.cc
+++ b/extensions/renderer/render_frame_observer_natives.cc
@@ -24,12 +24,11 @@ class LoadWatcher : public content::RenderFrameObserver {
       : content::RenderFrameObserver(frame), callback_(callback) {}
 
   void DidCreateDocumentElement() override {
-    // The callback must be run as soon as the root element is available.
-    // Running the callback may trigger DidCreateDocumentElement or
-    // DidFailProvisionalLoad, so delete this before running the callback.
-    base::Callback<void(bool)> callback = callback_;
+    // Defer the callback instead of running it now to avoid re-entrancy caused
+    // by the JavaScript callback.
+    ExtensionFrameHelper::Get(render_frame())
+        ->ScheduleAtDocumentStart(base::Bind(callback_, true));
     delete this;
-    callback.Run(true);
   }
 
   void DidFailProvisionalLoad(const blink::WebURLError& error) override {
diff --git a/extensions/renderer/script_injection_manager.cc b/extensions/renderer/script_injection_manager.cc
index aa0f5d3..0e234d0 100644
--- a/extensions/renderer/script_injection_manager.cc
+++ b/extensions/renderer/script_injection_manager.cc
@@ -88,6 +88,8 @@ class ScriptInjectionManager::RFOHelper : public content::RenderFrameObserver {
   // document_idle.
   void RunIdle();
 
+  void StartInjectScripts(UserScript::RunLocation run_location);
+
   // Indicate that the frame is no longer valid because it is starting
   // a new load or closing.
   void InvalidateAndResetFrame();
@@ -136,7 +138,10 @@ void ScriptInjectionManager::RFOHelper::DidCreateNewDocument() {
 }
 
 void ScriptInjectionManager::RFOHelper::DidCreateDocumentElement() {
-  manager_->StartInjectScripts(render_frame(), UserScript::DOCUMENT_START);
+  ExtensionFrameHelper::Get(render_frame())
+      ->ScheduleAtDocumentStart(
+          base::Bind(&ScriptInjectionManager::RFOHelper::StartInjectScripts,
+                     weak_factory_.GetWeakPtr(), UserScript::DOCUMENT_START));
 }
 
 void ScriptInjectionManager::RFOHelper::DidFailProvisionalLoad(
@@ -162,7 +167,11 @@ void ScriptInjectionManager::RFOHelper::DidFailProvisionalLoad(
 
 void ScriptInjectionManager::RFOHelper::DidFinishDocumentLoad() {
   DCHECK(content::RenderThread::Get());
-  manager_->StartInjectScripts(render_frame(), UserScript::DOCUMENT_END);
+  ExtensionFrameHelper::Get(render_frame())
+      ->ScheduleAtDocumentEnd(
+          base::Bind(&ScriptInjectionManager::RFOHelper::StartInjectScripts,
+                     weak_factory_.GetWeakPtr(), UserScript::DOCUMENT_END));
+
   // We try to run idle in two places: here and DidFinishLoad.
   // DidFinishDocumentLoad() corresponds to completing the document's load,
   // whereas DidFinishLoad corresponds to completing the document and all
@@ -231,6 +240,11 @@ void ScriptInjectionManager::RFOHelper::RunIdle() {
   }
 }
 
+void ScriptInjectionManager::RFOHelper::StartInjectScripts(
+    UserScript::RunLocation run_location) {
+  manager_->StartInjectScripts(render_frame(), run_location);
+}
+
 void ScriptInjectionManager::RFOHelper::InvalidateAndResetFrame() {
   // Invalidate any pending idle injections, and reset the frame inject on idle.
   weak_factory_.InvalidateWeakPtrs();
diff --git a/extensions/shell/renderer/shell_content_renderer_client.cc b/extensions/shell/renderer/shell_content_renderer_client.cc
index 7dc2aaa..92b7149 100644
--- a/extensions/shell/renderer/shell_content_renderer_client.cc
+++ b/extensions/shell/renderer/shell_content_renderer_client.cc
@@ -133,6 +133,16 @@ ShellContentRendererClient::CreateBrowserPluginDelegate(
   }
 }
 
+void ShellContentRendererClient::RunScriptsAtDocumentStart(
+    content::RenderFrame* render_frame) {
+  extension_dispatcher_->RunScriptsAtDocumentStart(render_frame);
+}
+
+void ShellContentRendererClient::RunScriptsAtDocumentEnd(
+    content::RenderFrame* render_frame) {
+  extension_dispatcher_->RunScriptsAtDocumentEnd(render_frame);
+}
+
 ExtensionsClient* ShellContentRendererClient::CreateExtensionsClient() {
   return new ShellExtensionsClient;
 }
diff --git a/extensions/shell/renderer/shell_content_renderer_client.h b/extensions/shell/renderer/shell_content_renderer_client.h
index a928e10..639da37 100644
--- a/extensions/shell/renderer/shell_content_renderer_client.h
+++ b/extensions/shell/renderer/shell_content_renderer_client.h
@@ -47,6 +47,8 @@ class ShellContentRendererClient : public content::ContentRendererClient {
       content::RenderFrame* render_frame,
       const std::string& mime_type,
       const GURL& original_url) override;
+  void RunScriptsAtDocumentStart(content::RenderFrame* render_frame) override;
+  void RunScriptsAtDocumentEnd(content::RenderFrame* render_frame) override;
 
  protected:
   // app_shell embedders may need custom extensions client interfaces.