Avoid storing a pointer to the "MainFrame" in the WorkQueue.

The main frame can change over the life of the TestRunner, if we swap
the frame on navigation. Instead, bind the current frame, via the
TestRunnerBindings. This means the FindInProcessMainWindowMainFrame()
is only used for
a) TestRunner::SetMockScreenOrientation() which is a bug to be fixed
b) TestRunner::FinishTest() (and callers to it).

That means we no longer rely on the main frame being stable or
consistent in order to be correct. FinishTest() will do the right
thing and find the main frame's process.

The last issue is that the "main window" is only set on the RenderView
(well, the RenderView's BlinkTestRunner) at the start of the test, so
if swapping the main frame also changes to a different RenderView then
it will not know it's the main window.

Either the browser needs to keep the "main window" bit up to date or
we can move the BlinkTestRunner::TestFinished() code up to the browser
process, where it can invoke the right things in the right renderer.
The tricky bit is.. the synchronous layout at the end of the test done
in the renderer. But this only matters if the frame is already in the
main frame's process.

R=avi@chromium.org

Bug: 1069111
Change-Id: Iae77bbabeeb074ba1595e7652f07f31cb556a1e1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2298053
Auto-Submit: danakj <danakj@chromium.org>
Commit-Queue: Nasko Oskov <nasko@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#788379}

content/shell/renderer/web_test/test_runner.cc

@@ -194,6 +194,8 @@ class TestRunnerBindings : public gin::Wrappable<TestRunnerBindings> {
   void PostV8Callback(v8::Local<v8::Function> v8_callback,
                       std::vector<v8::Local<v8::Value>> args = {});
 
+  blink::WebLocalFrame* GetWebFrame() { return frame_->GetWebFrame(); }
+
  private:
   // Watches for the RenderFrame that the TestRunnerBindings is attached to
   // being destroyed.
@@ -380,8 +382,6 @@ class TestRunnerBindings : public gin::Wrappable<TestRunnerBindings> {
                         std::vector<v8::UniquePersistent<v8::Value>> bound_args,
                         const std::vector<v8::Local<v8::Value>>& runtime_args);
 
-  blink::WebLocalFrame* GetWebFrame() { return frame_->GetWebFrame(); }
-
   // Hears about the RenderFrame in |frame_| being destroyed. The
   // TestRunningBindings should not do anything thereafter.
   void OnFrameDestroyed() {
@@ -886,13 +886,13 @@ void TestRunnerBindings::QueueReload() {
 void TestRunnerBindings::QueueLoadingScript(const std::string& script) {
   if (invalid_)
     return;
-  runner_->QueueLoadingScript(script);
+  runner_->QueueLoadingScript(script, weak_ptr_factory_.GetWeakPtr());
 }
 
 void TestRunnerBindings::QueueNonLoadingScript(const std::string& script) {
   if (invalid_)
     return;
-  runner_->QueueNonLoadingScript(script);
+  runner_->QueueNonLoadingScript(script, weak_ptr_factory_.GetWeakPtr());
 }
 
 void TestRunnerBindings::QueueLoad(gin::Arguments* args) {
@@ -2160,16 +2160,9 @@ void TestRunner::WorkQueue::AddWork(WorkItem* work) {
 }
 
 void TestRunner::WorkQueue::ProcessWork() {
-  // TODO(danakj): If we bound work to run in the frame that queued it
-  // then we would not rely on being in process with the main frame.
-  WebFrameTestProxy* in_process_main_frame =
-      controller_->FindInProcessMainWindowMainFrame();
-  if (!in_process_main_frame)
-    return;
-
   while (!queue_.empty()) {
     finished_loading_ = false;  // Watch for loading finishing inside Run().
-    bool started_load = queue_.front()->Run(controller_, in_process_main_frame);
+    bool started_load = queue_.front()->Run(controller_);
     delete queue_.front();
     queue_.pop_front();
 
@@ -2595,7 +2588,7 @@ class WorkItemBackForward : public TestRunner::WorkItem {
  public:
   explicit WorkItemBackForward(int distance) : distance_(distance) {}
 
-  bool Run(TestRunner* test_runner, WebFrameTestProxy*) override {
+  bool Run(TestRunner* test_runner) override {
     test_runner->GoToOffset(distance_);
     return true;  // FIXME: Did it really start a navigation?
   }
@@ -2640,7 +2633,7 @@ void TestRunner::QueueForwardNavigation(int how_far_forward) {
 
 class WorkItemReload : public TestRunner::WorkItem {
  public:
-  bool Run(TestRunner* test_runner, WebFrameTestProxy*) override {
+  bool Run(TestRunner* test_runner) override {
     test_runner->Reload();
     return true;
   }
@@ -2652,39 +2645,53 @@ void TestRunner::QueueReload() {
 
 class WorkItemLoadingScript : public TestRunner::WorkItem {
  public:
-  explicit WorkItemLoadingScript(const std::string& script) : script_(script) {}
-
-  bool Run(TestRunner*, WebFrameTestProxy* in_process_main_frame) override {
-    in_process_main_frame->GetWebFrame()->ExecuteScript(
+  explicit WorkItemLoadingScript(const std::string& script,
+                                 base::WeakPtr<TestRunnerBindings> bindings)
+      : script_(script), bindings_(std::move(bindings)) {}
+
+  bool Run(TestRunner*) override {
+    if (!bindings_)
+      return false;
+    bindings_->GetWebFrame()->ExecuteScript(
         blink::WebScriptSource(blink::WebString::FromUTF8(script_)));
     return true;  // FIXME: Did it really start a navigation?
   }
 
  private:
   std::string script_;
+  base::WeakPtr<TestRunnerBindings> bindings_;
 };
 
-void TestRunner::QueueLoadingScript(const std::string& script) {
-  work_queue_.AddWork(new WorkItemLoadingScript(script));
+void TestRunner::QueueLoadingScript(
+    const std::string& script,
+    base::WeakPtr<TestRunnerBindings> bindings) {
+  work_queue_.AddWork(new WorkItemLoadingScript(script, std::move(bindings)));
 }
 
 class WorkItemNonLoadingScript : public TestRunner::WorkItem {
  public:
-  explicit WorkItemNonLoadingScript(const std::string& script)
-      : script_(script) {}
-
-  bool Run(TestRunner*, WebFrameTestProxy* in_process_main_frame) override {
-    in_process_main_frame->GetWebFrame()->ExecuteScript(
+  explicit WorkItemNonLoadingScript(const std::string& script,
+                                    base::WeakPtr<TestRunnerBindings> bindings)
+      : script_(script), bindings_(std::move(bindings)) {}
+
+  bool Run(TestRunner*) override {
+    if (!bindings_)
+      return false;
+    bindings_->GetWebFrame()->ExecuteScript(
         blink::WebScriptSource(blink::WebString::FromUTF8(script_)));
     return false;
   }
 
  private:
   std::string script_;
+  base::WeakPtr<TestRunnerBindings> bindings_;
 };
 
-void TestRunner::QueueNonLoadingScript(const std::string& script) {
-  work_queue_.AddWork(new WorkItemNonLoadingScript(script));
+void TestRunner::QueueNonLoadingScript(
+    const std::string& script,
+    base::WeakPtr<TestRunnerBindings> bindings) {
+  work_queue_.AddWork(
+      new WorkItemNonLoadingScript(script, std::move(bindings)));
 }
 
 class WorkItemLoad : public TestRunner::WorkItem {
@@ -2692,7 +2699,7 @@ class WorkItemLoad : public TestRunner::WorkItem {
   WorkItemLoad(const GURL& url, const std::string& target)
       : url_(url), target_(target) {}
 
-  bool Run(TestRunner* test_runner, WebFrameTestProxy*) override {
+  bool Run(TestRunner* test_runner) override {
     test_runner->LoadURLForFrame(url_, target_);
     return true;  // FIXME: Did it really start a navigation?
   }

content/shell/renderer/web_test/test_runner.h

@@ -45,11 +45,6 @@ class WebString;
 class WebView;
 }  // namespace blink
 
-namespace content {
-class RenderView;
-struct TestPreferences;
-}
-
 namespace gin {
 class ArrayBufferView;
 class Arguments;
@@ -59,10 +54,13 @@ namespace content {
 class BlinkTestRunner;
 class MockScreenOrientationClient;
 class RenderFrame;
+class RenderView;
 class SpellCheckClient;
+class TestRunnerBindings;
 class WebFrameTestProxy;
 class WebWidgetTestProxy;
 class WebViewTestProxy;
+struct TestPreferences;
 
 // TestRunner class currently has dual purpose:
 // 1. It implements TestRunner javascript bindings for "global" / "ambient".
@@ -251,7 +249,7 @@ class TestRunner {
     virtual ~WorkItem() {}
 
     // Returns true if this started a load.
-    virtual bool Run(TestRunner*, WebFrameTestProxy* in_process_main_frame) = 0;
+    virtual bool Run(TestRunner*) = 0;
   };
 
  private:
@@ -310,8 +308,10 @@ class TestRunner {
   void QueueBackNavigation(int how_far_back);
   void QueueForwardNavigation(int how_far_forward);
   void QueueReload();
-  void QueueLoadingScript(const std::string& script);
-  void QueueNonLoadingScript(const std::string& script);
+  void QueueLoadingScript(const std::string& script,
+                          base::WeakPtr<TestRunnerBindings> bindings);
+  void QueueNonLoadingScript(const std::string& script,
+                             base::WeakPtr<TestRunnerBindings> bindings);
   void QueueLoad(const GURL& current_url,
                  const std::string& relative_url,
                  const std::string& target);