appview: Fix crash when embedding app which doesn't listen for embed requests

AppViewGuest::LaunchAppAndFireEvent rejects guest contents creation if
the app to be embedded does not listen for embed requests. However, the
callback has already been moved into a pending response, so this is a
use-after-move. We now create the pending response after checking for
the existence of the embed request listener and before sending the
request.

Bug: 1124060
Change-Id: I27e6e2b0752be733d3415f5dd88eae314f90572b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2440266Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
Reviewed-by: James MacLean <wjmaclean@chromium.org>
Commit-Queue: Kevin McNee <mcnee@chromium.org>
Cr-Commit-Position: refs/heads/master@{#813195}

extensions/browser/guest_view/app_view/app_view_apitest.cc

@@ -35,8 +35,8 @@ namespace {
 
 class MockShellAppDelegate : public extensions::ShellAppDelegate {
  public:
-  MockShellAppDelegate() : requested_(false) {
-    DCHECK(instance_ == nullptr);
+  MockShellAppDelegate() {
+    EXPECT_EQ(instance_, nullptr);
     instance_ = this;
   }
   ~MockShellAppDelegate() override { instance_ = nullptr; }
@@ -46,23 +46,25 @@ class MockShellAppDelegate : public extensions::ShellAppDelegate {
       const content::MediaStreamRequest& request,
       content::MediaResponseCallback callback,
       const extensions::Extension* extension) override {
-    requested_ = true;
-    if (request_message_loop_runner_.get())
-      request_message_loop_runner_->Quit();
+    media_access_requested_ = true;
+    if (media_access_request_quit_closure_)
+      std::move(media_access_request_quit_closure_).Run();
   }
 
   void WaitForRequestMediaPermission() {
-    if (requested_)
+    if (media_access_requested_)
       return;
-    request_message_loop_runner_ = new content::MessageLoopRunner;
-    request_message_loop_runner_->Run();
+    base::RunLoop run_loop;
+    media_access_request_quit_closure_ = run_loop.QuitClosure();
+    run_loop.Run();
   }
 
   static MockShellAppDelegate* Get() { return instance_; }
 
  private:
-  bool requested_;
-  scoped_refptr<content::MessageLoopRunner> request_message_loop_runner_;
+  bool media_access_requested_ = false;
+  base::OnceClosure media_access_request_quit_closure_;
+
   static MockShellAppDelegate* instance_;
 };
 
@@ -71,7 +73,7 @@ MockShellAppDelegate* MockShellAppDelegate::instance_ = nullptr;
 class MockShellAppViewGuestDelegate
     : public extensions::ShellAppViewGuestDelegate {
  public:
-  MockShellAppViewGuestDelegate() {}
+  MockShellAppViewGuestDelegate() = default;
 
   extensions::AppDelegate* CreateAppDelegate() override {
     return new MockShellAppDelegate();
@@ -80,7 +82,7 @@ class MockShellAppViewGuestDelegate
 
 class MockExtensionsAPIClient : public extensions::ShellExtensionsAPIClient {
  public:
-  MockExtensionsAPIClient() {}
+  MockExtensionsAPIClient() = default;
 
   extensions::AppViewGuestDelegate* CreateAppViewGuestDelegate()
       const override {
@@ -105,7 +107,7 @@ class AppViewTest : public AppShellTest {
   content::WebContents* GetFirstAppWindowWebContents() {
     const AppWindowRegistry::AppWindowList& app_window_list =
         AppWindowRegistry::Get(browser_context_)->app_windows();
-    DCHECK(app_window_list.size() == 1);
+    EXPECT_EQ(1U, app_window_list.size());
     return (*app_window_list.begin())->web_contents();
   }
 
@@ -143,6 +145,7 @@ class AppViewTest : public AppShellTest {
     ASSERT_TRUE(done_listener.WaitUntilSatisfied());
   }
 
+ private:
   content::WebContents* embedder_web_contents_;
   TestGuestViewManagerFactory factory_;
 };
@@ -198,4 +201,9 @@ IN_PROC_BROWSER_TEST_F(AppViewTest,
           "app_view/apitest/skeleton");
 }
 
+IN_PROC_BROWSER_TEST_F(AppViewTest, TestAppViewNoEmbedRequestListener) {
+  RunTest("testAppViewNoEmbedRequestListener", "app_view/apitest",
+          "app_view/apitest/no_embed_request_listener");
+}
+
 }  // namespace extensions

extensions/browser/guest_view/app_view/app_view_guest.cc

@@ -55,8 +55,8 @@ struct ResponseInfo {
 };
 
 using PendingResponseMap = std::map<int, std::unique_ptr<ResponseInfo>>;
-static base::LazyInstance<PendingResponseMap>::DestructorAtExit
-    pending_response_map = LAZY_INSTANCE_INITIALIZER;
+base::LazyInstance<PendingResponseMap>::DestructorAtExit
+    g_pending_response_map = LAZY_INSTANCE_INITIALIZER;
 
 }  // namespace
 
@@ -70,7 +70,7 @@ bool AppViewGuest::CompletePendingRequest(
     int guest_instance_id,
     const std::string& guest_extension_id,
     content::RenderProcessHost* guest_render_process_host) {
-  PendingResponseMap* response_map = pending_response_map.Pointer();
+  PendingResponseMap* response_map = g_pending_response_map.Pointer();
   auto it = response_map->find(guest_instance_id);
   // Kill the requesting process if it is not the real guest.
   if (it == response_map->end()) {
@@ -190,11 +190,6 @@ void AppViewGuest::CreateWebContents(const base::DictionaryValue& create_params,
     return;
   }
 
-  pending_response_map.Get().insert(std::make_pair(
-      guest_instance_id(), std::make_unique<ResponseInfo>(
-                               guest_extension, weak_ptr_factory_.GetWeakPtr(),
-                               std::move(callback))));
-
   const LazyContextId context_id(browser_context(), guest_extension->id());
   LazyContextTaskQueue* queue = context_id.GetTaskQueue();
   if (queue->ShouldEnqueueTask(browser_context(), guest_extension)) {
@@ -267,15 +262,21 @@ void AppViewGuest::LaunchAppAndFireEvent(
     return;
   }
 
+  const Extension* const extension =
+      extensions::ExtensionRegistry::Get(context_info->browser_context)
+          ->enabled_extensions()
+          .GetByID(context_info->extension_id);
+
+  g_pending_response_map.Get().insert(std::make_pair(
+      guest_instance_id(),
+      std::make_unique<ResponseInfo>(extension, weak_ptr_factory_.GetWeakPtr(),
+                                     std::move(callback))));
+
   std::unique_ptr<base::DictionaryValue> embed_request(
       new base::DictionaryValue());
   embed_request->SetInteger(appview::kGuestInstanceID, guest_instance_id());
   embed_request->SetString(appview::kEmbedderID, owner_host());
   embed_request->Set(appview::kData, std::move(data));
-  const Extension* const extension =
-      extensions::ExtensionRegistry::Get(context_info->browser_context)
-          ->enabled_extensions()
-          .GetByID(context_info->extension_id);
   AppRuntimeEventRouter::DispatchOnEmbedRequestedEvent(
       browser_context(), std::move(embed_request), extension);
 }
@@ -286,7 +287,7 @@ void AppViewGuest::SetAppDelegateForTest(AppDelegate* delegate) {
 
 std::vector<int> AppViewGuest::GetAllRegisteredInstanceIdsForTesting() {
   std::vector<int> instances;
-  for (const auto& key_value : pending_response_map.Get()) {
+  for (const auto& key_value : g_pending_response_map.Get()) {
     instances.push_back(key_value.first);
   }
   return instances;

extensions/test/data/app_view/OWNERS

+file://components/guest_view/OWNERS
+
+# COMPONENT: Platform>Apps>BrowserTag

extensions/test/data/app_view/apitest/main.js

@@ -132,12 +132,29 @@ function testAppViewWithUndefinedDataShouldSucceed(appToEmbed) {
   });
 };
 
+function testAppViewNoEmbedRequestListener(appToEmbed) {
+  var appview = new AppView();
+  LOG('appToEmbed  ' + appToEmbed);
+  document.body.appendChild(appview);
+  LOG('Attempting to connect to app that does not listen for embed requests.');
+  appview.connect(appToEmbed, null, (success) => {
+    if (success) {
+      LOG('Should not have connected.');
+      embedder.test.fail();
+    } else {
+      LOG('Connection was correctly rejected.');
+      embedder.test.succeed();
+    }
+  });
+};
+
 embedder.test.testList = {
   'testAppViewGoodDataShouldSucceed': testAppViewGoodDataShouldSucceed,
   'testAppViewMediaRequest': testAppViewMediaRequest,
   'testAppViewRefusedDataShouldFail': testAppViewRefusedDataShouldFail,
   'testAppViewWithUndefinedDataShouldSucceed':
-      testAppViewWithUndefinedDataShouldSucceed
+      testAppViewWithUndefinedDataShouldSucceed,
+  'testAppViewNoEmbedRequestListener': testAppViewNoEmbedRequestListener
 };
 
 onload = function() {

extensions/test/data/app_view/apitest/no_embed_request_listener/background.js

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Intentionally not listening for onEmbedRequested.
+
+chrome.app.runtime.onLaunched.addListener(function() {
+  chrome.app.window.create('main.html', {}, function () {});
+});

extensions/test/data/app_view/apitest/no_embed_request_listener/main.html

+<!DOCTYPE html>
+<!--
+ * Copyright 2020 The Chromium Authors. All rights reserved.
+ * Use of this source code is governed by a BSD-style license that can be
+ * found in the LICENSE file.
+-->
+<html>
+  <body>
+    <p>Hello</p>
+  </body>
+</html>

extensions/test/data/app_view/apitest/no_embed_request_listener/manifest.json

+{
+  "name": "No embed request listener",
+  "description": "An app that does not listen for onEmbedRequested",
+  "manifest_version": 2,
+  "version": "1.0",
+  "app": {
+    "background": {
+      "scripts": ["background.js"]
+    }
+  }
+}