Prevent crashes in WPT due to UAF of TestInterfaces::Delegate

This works around crashes that surfaced in Cross-origin-opener-policy
WPT tests. Two possible crashes happened:
  1 - The |RenderView| that created the |BlinkTestRunner| used as a
    delegate got deleted. Later a new |RenderView| is created, followed
    by its |RenderFrame|. The creation of the |RenderFrame|'s associated
    |WebFrameTestClient| crashes as |TestInterfaces::delegate_| is now
    uninitialized (DCHECK in |WebFrameTestClient| ctor).
  2 - Two |RenderViews| are created, both create a |BlinkTestRunner|,
    the first one created is used for |TestInterfaces::delegate_|. Then
    |WebFrameTestClient|s are created, and take a pointer to this
    |delegate_|. Later the |RenderView| associated to the delegate is
    deleted, deleting the delegate. Leading to UAFs by
    |WebFrameTestClient|s under the other |RenderView|.

This failures happen in COOP tests as multiple tests opening and closing
popups are ran simultaneously from the same window.

A bit more context: The creation of the |RenderView|, instrumentalized
for tests through |CreateWebViewTestProxy| creates a new
|BlinkTestRunner| and a child class of |RenderView|: |WebViewTestProxy|.
The first execution of |CreateWebViewTestProxy| registers the
|BlinkTestRunner| using |TestInterfaces::SetDelegate|. The |RenderView|
destruction updates |TestInterfaces::delegate_| to nullptr.
Likewise, the creation of |RenderFrame| through
|CreateWebFrameTestProxy| creates a |WebFrameTestProxy| and closely
related |WebFrameTestClient|. The latter saves a pointer to
|TestInterfaces::delegate_|.

To Prevent these crashes, this CL changes:
  - When a |RenderView| is created and no delegate exists, i.e.
    |TestInterfaces::delegate_| is nullptr, use the newly created
    |BlinkTestRunner| as the delegate_. This avoids 1-.
  - When a |RenderView| is deleted and another exists, switch to that
    one's |BlinkTestRunner| as the |TestInterfaces::delegate_|.
  - |WebFrameTestClient| to not hold a pointer to
    |TestInterfaces::delegate_| and instead access  it through the
    |WebViewTestProxy| pointer it already had:
    |web_view_test_proxy_->test_interfaces()->GetDelegate()|. These two
    combined avoid 2-.

Bug: 922191
Change-Id: I83e16a4064afca1c595b04c5ccbb46f1488188f5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2074482
Commit-Queue: Arthur Hemery <ahemery@chromium.org>
Auto-Submit: Pâris Meuleman <pmeuleman@chromium.org>
Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Camille Lamy <clamy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#746316}

content/shell/test_runner/test_interfaces.cc

@@ -69,6 +69,18 @@ void TestInterfaces::ResetTestHelperControllers() {
     web_view_test_proxy->Reset();
 }
 
+void TestInterfaces::DelegateDestroyed(WebTestDelegate* delegate) {
+  if (delegate == delegate_) {
+    if (!window_list_.empty()) {
+      SetDelegate(window_list_[0]->delegate());
+      SetMainView(window_list_[0]->GetWebView());
+    } else {
+      SetDelegate(nullptr);
+      SetMainView(nullptr);
+    }
+  }
+}
+
 void TestInterfaces::ResetAll() {
   ResetTestHelperControllers();
   test_runner_->Reset();
@@ -137,6 +149,8 @@ void TestInterfaces::WindowClosed(WebViewTestProxy* proxy) {
     return;
   }
   window_list_.erase(pos);
+
+  DelegateDestroyed(proxy->delegate());
 }
 
 TestRunner* TestInterfaces::GetTestRunner() {

content/shell/test_runner/test_interfaces.h

@@ -47,6 +47,13 @@ class TestInterfaces {
   const std::vector<WebViewTestProxy*>& GetWindowList();
 
  private:
+  // Called when a WebTestDelegate is destroyed, if it is the currently used
+  // delegate, switch to another delegate in window_list_ as there might be
+  // WebFrameTestClients that require it.
+  // If window_list_ is empty set delegate_ to nullptr, a new one will be
+  // assigned the next time a WebViewTestProxy is built.
+  void DelegateDestroyed(WebTestDelegate* delegate);
+
   std::unique_ptr<GamepadController> gamepad_controller_;
   std::unique_ptr<TestRunner> test_runner_;
   WebTestDelegate* delegate_;

content/shell/test_runner/test_plugin.cc

@@ -18,6 +18,7 @@
 #include "cc/layers/texture_layer.h"
 #include "cc/resources/cross_thread_shared_bitmap.h"
 #include "components/viz/common/resources/bitmap_allocation.h"
+#include "content/shell/test_runner/test_interfaces.h"
 #include "content/shell/test_runner/web_test_delegate.h"
 #include "gpu/GLES2/gl2extchromium.h"
 #include "gpu/command_buffer/client/gles2_interface.h"
@@ -116,9 +117,9 @@ blink::WebPluginContainer::TouchEventRequestType ParseTouchEventRequestType(
 }  // namespace
 
 TestPlugin::TestPlugin(const blink::WebPluginParams& params,
-                       WebTestDelegate* delegate,
+                       TestInterfaces* test_interfaces,
                        blink::WebLocalFrame* frame)
-    : delegate_(delegate),
+    : test_interfaces_(test_interfaces),
       container_(nullptr),
       web_local_frame_(frame),
       gl_(nullptr),
@@ -577,21 +578,22 @@ blink::WebInputEventResult TestPlugin::HandleInputEvent(
   const char* event_name = blink::WebInputEvent::GetName(event.GetType());
   if (!strcmp(event_name, "") || !strcmp(event_name, "Undefined"))
     event_name = "unknown";
-  delegate_->PrintMessage(std::string("Plugin received event: ") + event_name +
-                          "\n");
+  test_interfaces_->GetDelegate()->PrintMessage(
+      std::string("Plugin received event: ") + event_name + "\n");
   if (print_event_details_)
-    PrintEventDetails(delegate_, event);
+    PrintEventDetails(test_interfaces_->GetDelegate(), event);
 
   if (print_user_gesture_status_) {
     bool has_transient_user_activation =
         web_local_frame_->HasTransientUserActivation();
-    delegate_->PrintMessage(std::string("* ") +
-                            (has_transient_user_activation ? "" : "not ") +
-                            "handling user gesture\n");
+    test_interfaces_->GetDelegate()->PrintMessage(
+        std::string("* ") + (has_transient_user_activation ? "" : "not ") +
+        "handling user gesture\n");
   }
 
   if (is_persistent_)
-    delegate_->PrintMessage(std::string("TestPlugin: isPersistent\n"));
+    test_interfaces_->GetDelegate()->PrintMessage(
+        std::string("TestPlugin: isPersistent\n"));
   return blink::WebInputEventResult::kNotHandled;
 }
 
@@ -617,15 +619,15 @@ bool TestPlugin::HandleDragStatusUpdate(blink::WebDragStatus drag_status,
     case blink::kWebDragStatusUnknown:
       NOTREACHED();
   }
-  delegate_->PrintMessage(std::string("Plugin received event: ") +
-                          drag_status_name + "\n");
+  test_interfaces_->GetDelegate()->PrintMessage(
+      std::string("Plugin received event: ") + drag_status_name + "\n");
   return false;
 }
 
 TestPlugin* TestPlugin::Create(const blink::WebPluginParams& params,
-                               WebTestDelegate* delegate,
+                               TestInterfaces* test_interfaces,
                                blink::WebLocalFrame* frame) {
-  return new TestPlugin(params, delegate, frame);
+  return new TestPlugin(params, test_interfaces, frame);
 }
 
 const blink::WebString& TestPlugin::MimeType() {

content/shell/test_runner/test_plugin.h

@@ -44,9 +44,9 @@ struct TransferableResource;
 
 namespace test_runner {
 
-class WebTestDelegate;
+class TestInterfaces;
 
-// A fake implemention of blink::WebPlugin for testing purposes.
+// A fake implementation of blink::WebPlugin for testing purposes.
 //
 // It uses GL to paint a scene consisiting of a primitive over a background. The
 // primitive and background can be customized using the following plugin
@@ -61,7 +61,7 @@ class WebTestDelegate;
 class TestPlugin : public blink::WebPlugin, public cc::TextureLayerClient {
  public:
   static TestPlugin* Create(const blink::WebPluginParams& params,
-                            WebTestDelegate* delegate,
+                            TestInterfaces* test_interfaces,
                             blink::WebLocalFrame* frame);
   ~TestPlugin() override;
 
@@ -106,7 +106,7 @@ class TestPlugin : public blink::WebPlugin, public cc::TextureLayerClient {
 
  private:
   TestPlugin(const blink::WebPluginParams& params,
-             WebTestDelegate* delegate,
+             TestInterfaces* test_interfaces,
              blink::WebLocalFrame* frame);
 
   enum Primitive { PrimitiveNone, PrimitiveTriangle };
@@ -168,7 +168,7 @@ class TestPlugin : public blink::WebPlugin, public cc::TextureLayerClient {
       const gpu::SyncToken& sync_token,
       bool lost);
 
-  WebTestDelegate* delegate_;
+  TestInterfaces* test_interfaces_;
   blink::WebPluginContainer* container_;
   blink::WebLocalFrame* web_local_frame_;
 

content/shell/test_runner/web_frame_test_client.cc

@@ -129,13 +129,10 @@ const char* WebNavigationTypeToString(blink::WebNavigationType type) {
 
 }  // namespace
 
-WebFrameTestClient::WebFrameTestClient(WebTestDelegate* delegate,
-                                       WebViewTestProxy* web_view_test_proxy,
+WebFrameTestClient::WebFrameTestClient(WebViewTestProxy* web_view_test_proxy,
                                        WebFrameTestProxy* web_frame_test_proxy)
-    : delegate_(delegate),
-      web_view_test_proxy_(web_view_test_proxy),
+    : web_view_test_proxy_(web_view_test_proxy),
       web_frame_test_proxy_(web_frame_test_proxy) {
-  DCHECK(delegate_);
   DCHECK(web_frame_test_proxy_);
   DCHECK(web_view_test_proxy_);
 }
@@ -286,20 +283,20 @@ void WebFrameTestClient::HandleWebAccessibilityEvent(
       }
     }
 
-    delegate_->PrintMessage(message + "\n");
+    delegate()->PrintMessage(message + "\n");
   }
 }
 
 void WebFrameTestClient::DidChangeSelection(bool is_empty_callback) {
   if (test_runner()->ShouldDumpEditingCallbacks())
-    delegate_->PrintMessage(
+    delegate()->PrintMessage(
         "EDITING DELEGATE: "
         "webViewDidChangeSelection:WebViewDidChangeSelectionNotification\n");
 }
 
 void WebFrameTestClient::DidChangeContents() {
   if (test_runner()->ShouldDumpEditingCallbacks())
-    delegate_->PrintMessage(
+    delegate()->PrintMessage(
         "EDITING DELEGATE: webViewDidChange:WebViewDidChangeNotification\n");
 }
 
@@ -307,13 +304,15 @@ blink::WebPlugin* WebFrameTestClient::CreatePlugin(
     const blink::WebPluginParams& params) {
   blink::WebLocalFrame* frame = web_frame_test_proxy_->GetWebFrame();
   if (TestPlugin::IsSupportedMimeType(params.mime_type))
-    return TestPlugin::Create(params, delegate_, frame);
-  return delegate_->CreatePluginPlaceholder(params);
+    return TestPlugin::Create(params, web_view_test_proxy_->test_interfaces(),
+                              frame);
+  return delegate()->CreatePluginPlaceholder(params);
 }
 
 void WebFrameTestClient::ShowContextMenu(
     const blink::WebContextMenuData& context_menu_data) {
-  delegate_->GetWebWidgetTestProxy(web_frame_test_proxy_->GetWebFrame())
+  delegate()
+      ->GetWebWidgetTestProxy(web_frame_test_proxy_->GetWebFrame())
       ->event_sender()
       ->SetContextMenuData(context_menu_data);
 }
@@ -328,8 +327,8 @@ void WebFrameTestClient::DidStopLoading() {
 
 void WebFrameTestClient::DidDispatchPingLoader(const blink::WebURL& url) {
   if (test_runner()->ShouldDumpPingLoaderCallbacks())
-    delegate_->PrintMessage(std::string("PingLoader dispatched to '") +
-                            URLDescription(url).c_str() + "'.\n");
+    delegate()->PrintMessage(std::string("PingLoader dispatched to '") +
+                             URLDescription(url).c_str() + "'.\n");
 }
 
 void WebFrameTestClient::WillSendRequest(blink::WebURLRequest& request) {
@@ -361,16 +360,16 @@ void WebFrameTestClient::WillSendRequest(blink::WebURLRequest& request) {
         ((site_for_cookies.scheme() != url::kHttpScheme &&
           site_for_cookies.scheme() != url::kHttpsScheme) ||
          IsLocalHost(site_for_cookies.registrable_domain())) &&
-        !delegate_->AllowExternalPages()) {
-      delegate_->PrintMessage(std::string("Blocked access to external URL ") +
-                              url.possibly_invalid_spec() + "\n");
+        !delegate()->AllowExternalPages()) {
+      delegate()->PrintMessage(std::string("Blocked access to external URL ") +
+                               url.possibly_invalid_spec() + "\n");
       BlockRequest(request);
       return;
     }
   }
 
   // Set the new substituted URL.
-  request.SetUrl(delegate_->RewriteWebTestsURL(
+  request.SetUrl(delegate()->RewriteWebTestsURL(
       request.Url().GetString().Utf8(),
       test_runner()->is_web_platform_tests_mode()));
 }
@@ -424,16 +423,16 @@ void WebFrameTestClient::DidAddMessageToConsole(
   console_message += "\n";
 
   if (dump_to_stderr) {
-    delegate_->PrintMessageToStderr(console_message);
+    delegate()->PrintMessageToStderr(console_message);
   } else {
-    delegate_->PrintMessage(console_message);
+    delegate()->PrintMessage(console_message);
   }
 }
 
 bool WebFrameTestClient::ShouldContinueNavigation(
     blink::WebNavigationInfo* info) {
   if (test_runner()->ShouldDumpNavigationPolicy()) {
-    delegate_->PrintMessage(
+    delegate()->PrintMessage(
         "Default policy for navigation to '" +
         URLDescription(info->url_request.Url()) + "' is '" +
         WebNavigationPolicyToString(info->navigation_policy) + "'\n");
@@ -442,18 +441,18 @@ bool WebFrameTestClient::ShouldContinueNavigation(
   if (test_runner()->ShouldDumpFrameLoadCallbacks()) {
     GURL url = info->url_request.Url();
     WebFrameTestClient::PrintFrameDescription(
-        delegate_, web_frame_test_proxy_->GetWebFrame());
-    delegate_->PrintMessage(" - BeginNavigation request to '");
-    delegate_->PrintMessage(
+        delegate(), web_frame_test_proxy_->GetWebFrame());
+    delegate()->PrintMessage(" - BeginNavigation request to '");
+    delegate()->PrintMessage(
         DescriptionSuitableForTestResult(url.possibly_invalid_spec()));
-    delegate_->PrintMessage("', http method ");
-    delegate_->PrintMessage(info->url_request.HttpMethod().Utf8().data());
-    delegate_->PrintMessage("\n");
+    delegate()->PrintMessage("', http method ");
+    delegate()->PrintMessage(info->url_request.HttpMethod().Utf8().data());
+    delegate()->PrintMessage("\n");
   }
 
   bool should_continue = true;
   if (test_runner()->PolicyDelegateEnabled()) {
-    delegate_->PrintMessage(
+    delegate()->PrintMessage(
         std::string("Policy delegate: attempt to load ") +
         URLDescription(info->url_request.Url()) + " with navigation type '" +
         WebNavigationTypeToString(info->navigation_type) + "'\n");
@@ -478,7 +477,7 @@ bool WebFrameTestClient::ShouldContinueNavigation(
         network::mojom::ReferrerPolicy::kDefault);
   }
 
-  info->url_request.SetUrl(delegate_->RewriteWebTestsURL(
+  info->url_request.SetUrl(delegate()->RewriteWebTestsURL(
       info->url_request.Url().GetString().Utf8(),
       test_runner()->is_web_platform_tests_mode()));
   return should_continue;
@@ -501,7 +500,7 @@ void WebFrameTestClient::DidClearWindowObject() {
   blink::WebLocalFrame* frame = web_frame_test_proxy_->GetWebFrame();
   web_view_test_proxy_->test_interfaces()->BindTo(frame);
   web_view_test_proxy_->BindTo(frame);
-  delegate_->GetWebWidgetTestProxy(frame)->BindTo(frame);
+  delegate()->GetWebWidgetTestProxy(frame)->BindTo(frame);
 }
 
 blink::WebEffectiveConnectionType
@@ -509,6 +508,10 @@ WebFrameTestClient::GetEffectiveConnectionType() {
   return test_runner()->effective_connection_type();
 }
 
+WebTestDelegate* WebFrameTestClient::delegate() {
+  return web_view_test_proxy_->test_interfaces()->GetDelegate();
+}
+
 TestRunner* WebFrameTestClient::test_runner() {
   return web_view_test_proxy_->test_interfaces()->GetTestRunner();
 }

content/shell/test_runner/web_frame_test_client.h

@@ -25,10 +25,9 @@ class WebViewTestProxy;
 // RenderFrameImpl).
 class WebFrameTestClient : public blink::WebLocalFrameClient {
  public:
-  // Caller has to ensure that all arguments (|delegate|,
-  // |web_view_test_proxy| and so forth) live longer than |this|.
-  WebFrameTestClient(WebTestDelegate* delegate,
-                     WebViewTestProxy* web_view_test_proxy,
+  // Caller has to ensure that all arguments (|web_view_test_proxy| and so
+  // forth) live longer than |this|.
+  WebFrameTestClient(WebViewTestProxy* web_view_test_proxy,
                      WebFrameTestProxy* web_frame_test_proxy);
 
   ~WebFrameTestClient() override;
@@ -64,11 +63,12 @@ class WebFrameTestClient : public blink::WebLocalFrameClient {
 
  private:
   TestRunner* test_runner();
+  WebTestDelegate* delegate();
+
   void HandleWebAccessibilityEvent(const blink::WebAXObject& obj,
                                    const char* event_name);
 
   // Borrowed pointers to other parts of web tests state.
-  WebTestDelegate* delegate_;
   WebViewTestProxy* web_view_test_proxy_;
   WebFrameTestProxy* web_frame_test_proxy_;
 

content/shell/test_runner/web_test_interfaces.cc

@@ -26,6 +26,10 @@ void WebTestInterfaces::SetDelegate(WebTestDelegate* delegate) {
   interfaces_->SetDelegate(delegate);
 }
 
+bool WebTestInterfaces::HasDelegate() {
+  return interfaces_->GetDelegate();
+}
+
 void WebTestInterfaces::ResetAll() {
   interfaces_->ResetAll();
 }
@@ -54,10 +58,8 @@ TestInterfaces* WebTestInterfaces::GetTestInterfaces() {
 std::unique_ptr<WebFrameTestClient> WebTestInterfaces::CreateWebFrameTestClient(
     WebViewTestProxy* web_view_test_proxy,
     WebFrameTestProxy* web_frame_test_proxy) {
-  // TODO(lukasza): Do not pass the WebTestDelegate below - it's lifetime can
-  // differ from the lifetime of WebFrameTestClient - https://crbug.com/606594.
-  return std::make_unique<WebFrameTestClient>(
-      interfaces_->GetDelegate(), web_view_test_proxy, web_frame_test_proxy);
+  return std::make_unique<WebFrameTestClient>(web_view_test_proxy,
+                                              web_frame_test_proxy);
 }
 
 std::vector<blink::WebView*> WebTestInterfaces::GetWindowList() {

content/shell/test_runner/web_test_interfaces.h

@@ -35,6 +35,7 @@ class TEST_RUNNER_EXPORT WebTestInterfaces {
 
   void SetMainView(blink::WebView* web_view);
   void SetDelegate(WebTestDelegate* delegate);
+  bool HasDelegate();
   void ResetAll();
   bool TestIsRunning();
   void SetTestIsRunning(bool running);

content/shell/test_runner/web_view_test_proxy.cc

@@ -102,10 +102,6 @@ void WebViewTestProxy::BindTo(blink::WebLocalFrame* frame) {
 
 WebViewTestProxy::~WebViewTestProxy() {
   test_interfaces_->WindowClosed(this);
-  if (test_interfaces_->GetDelegate() == delegate_.get()) {
-    test_interfaces_->SetDelegate(nullptr);
-    test_interfaces_->SetMainView(nullptr);
-  }
 }
 
 TestRunner* WebViewTestProxy::GetTestRunner() {

content/test/web_test_support.cc

@@ -73,13 +73,11 @@ RenderViewImpl* CreateWebViewTestProxy(CompositorDependencies* compositor_deps,
       new test_runner::WebViewTestProxy(compositor_deps, params);
 
   auto test_runner = std::make_unique<BlinkTestRunner>(render_view_proxy);
-  // TODO(lukasza): Using the 1st BlinkTestRunner as the main delegate is wrong,
-  // but it is difficult to change because this behavior has been baked for a
-  // long time into test assumptions (i.e. which PrintMessage gets delivered to
-  // the browser depends on this).
-  static bool first_test_runner = true;
-  if (first_test_runner) {
-    first_test_runner = false;
+  // TODO(lukasza): Using the first BlinkTestRunner as the main delegate is
+  // wrong, but it is difficult to change because this behavior has been baked
+  // for a long time into test assumptions (i.e. which PrintMessage gets
+  // delivered to the browser depends on this).
+  if (!interfaces->HasDelegate()) {
     interfaces->SetDelegate(test_runner.get());
   }