Revert "Convert ExtensionHostMsg_Request to mojom.LocalFrameHost mess…

…age"

This reverts commit 8eba1ef.

Reason for revert: 91.0.4469.0 canary is DOA with https://crbug.com/1196377.

Original change's description:
> Convert ExtensionHostMsg_Request to mojom.LocalFrameHost message
>
> This CL converts ExtensionHostMsg_Request to
> extensions::mojom::LocalFrameHost::Request() and the reply of it
> is sent back to the renderer through the RequestCallback instead
> of ExtensionMsg_Response.
>
> Bug: 1183518
> Change-Id: Idc9faf41c8d5159fcd2b6617f4e45de621ed14f3
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2784353
> Commit-Queue: Julie Kim <jkim@igalia.com>
> Reviewed-by: Reilly Grant <reillyg@chromium.org>
> Reviewed-by: Sam McNally <sammc@chromium.org>
> Reviewed-by: Dave Tapuska <dtapuska@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#869478}

(cherry picked from commit ba5865e)

Bug: 1183518
Bug: 1196377
Change-Id: Iee0ba7675a4d7b6a9cc300b81836a5d4783bdbfe
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2809395
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Reviewed-by: Greg Thompson <grt@chromium.org>
Reviewed-by: Sam McNally <sammc@chromium.org>
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Owners-Override: Daniel Cheng <dcheng@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#869905}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2809175
Reviewed-by: Takuto Ikuta <tikuta@chromium.org>
Commit-Queue: Takuto Ikuta <tikuta@chromium.org>
Owners-Override: Takuto Ikuta <tikuta@chromium.org>
Cr-Commit-Position: refs/branch-heads/4469@{#4}
Cr-Branched-From: 8563c4c-refs/heads/master@{#869727}

chrome/browser/extensions/chrome_extension_frame_host.cc

@@ -10,7 +10,7 @@ namespace extensions {
 
 ChromeExtensionFrameHost::ChromeExtensionFrameHost(
     content::WebContents* web_contents)
-    : ExtensionFrameHost(web_contents) {}
+    : ExtensionFrameHost(web_contents), web_contents_(web_contents) {}
 
 ChromeExtensionFrameHost::~ChromeExtensionFrameHost() = default;
 
@@ -20,7 +20,7 @@ void ChromeExtensionFrameHost::RequestScriptInjectionPermission(
     mojom::RunLocation run_location,
     RequestScriptInjectionPermissionCallback callback) {
   ExtensionActionRunner* runner =
-      ExtensionActionRunner::GetForWebContents(web_contents());
+      ExtensionActionRunner::GetForWebContents(web_contents_);
   if (!runner) {
     std::move(callback).Run(false);
     return;

chrome/browser/extensions/chrome_extension_frame_host.h

@@ -29,6 +29,13 @@ class ChromeExtensionFrameHost : public ExtensionFrameHost {
       mojom::InjectionType script_type,
       mojom::RunLocation run_location,
       RequestScriptInjectionPermissionCallback callback) override;
+
+ private:
+  // This raw pointer is safe to use because ExtensionWebContentsObserver whose
+  // lifetime is tied to the WebContents owns this instance.
+  // The parent class ExtensionFrameHost uses WebContentsFrameReceiverSet with
+  // |web_contents_| for mojom::LocalFrameHost.
+  content::WebContents* web_contents_;
 };
 
 }  // namespace extensions

chrome/browser/extensions/chrome_extension_function_unittest.cc

@@ -23,15 +23,15 @@ namespace {
 
 void SuccessCallback(bool* did_respond,
                      ExtensionFunction::ResponseType type,
-                     const base::Value& results,
+                     const base::ListValue& results,
                      const std::string& error) {
   EXPECT_EQ(ExtensionFunction::ResponseType::SUCCEEDED, type);
   *did_respond = true;
 }
 
 void FailCallback(bool* did_respond,
                   ExtensionFunction::ResponseType type,
-                  const base::Value& results,
+                  const base::ListValue& results,
                   const std::string& error) {
   EXPECT_EQ(ExtensionFunction::ResponseType::FAILED, type);
   *did_respond = true;

extensions/browser/api_test_utils.cc

@@ -50,7 +50,7 @@ bool SendResponseHelper::GetResponse() {
 }
 
 void SendResponseHelper::OnResponse(ExtensionFunction::ResponseType response,
-                                    const base::Value& results,
+                                    const base::ListValue& results,
                                     const std::string& error) {
   ASSERT_NE(ExtensionFunction::BAD_MESSAGE, response);
   response_ = std::make_unique<bool>(response == ExtensionFunction::SUCCEEDED);

extensions/browser/api_test_utils.h

@@ -49,7 +49,7 @@ class SendResponseHelper {
  private:
   // Response handler.
   void OnResponse(ExtensionFunction::ResponseType response,
-                  const base::Value& results,
+                  const base::ListValue& results,
                   const std::string& error);
 
   base::RunLoop run_loop_;

extensions/browser/extension_frame_host.cc

@@ -4,14 +4,10 @@
 
 #include "extensions/browser/extension_frame_host.h"
 
-#include "content/public/browser/render_process_host.h"
-#include "extensions/browser/extension_function_dispatcher.h"
-#include "extensions/browser/extension_web_contents_observer.h"
-
 namespace extensions {
 
 ExtensionFrameHost::ExtensionFrameHost(content::WebContents* web_contents)
-    : web_contents_(web_contents), receivers_(web_contents, this) {}
+    : receivers_(web_contents, this) {}
 
 ExtensionFrameHost::~ExtensionFrameHost() = default;
 
@@ -23,14 +19,4 @@ void ExtensionFrameHost::RequestScriptInjectionPermission(
   std::move(callback).Run(false);
 }
 
-void ExtensionFrameHost::Request(mojom::RequestParamsPtr params,
-                                 RequestCallback callback) {
-  content::RenderFrameHost* render_frame_host =
-      receivers_.GetCurrentTargetFrame();
-  ExtensionWebContentsObserver::GetForWebContents(web_contents_)
-      ->dispatcher()
-      ->Dispatch(std::move(params), render_frame_host,
-                 render_frame_host->GetProcess()->GetID(), std::move(callback));
-}
-
 }  // namespace extensions

extensions/browser/extension_frame_host.h

@@ -17,7 +17,7 @@ class WebContents;
 namespace extensions {
 
 // Implements the mojo interface of extensions::mojom::LocalFrameHost.
-// ExtensionWebContentsObserver creates and owns this class and it's destroyed
+// ExtensionWebContentsObserver creates this class and it's destroyed with it
 // when WebContents is destroyed.
 class ExtensionFrameHost : public mojom::LocalFrameHost {
  public:
@@ -32,16 +32,8 @@ class ExtensionFrameHost : public mojom::LocalFrameHost {
       mojom::InjectionType script_type,
       mojom::RunLocation run_location,
       RequestScriptInjectionPermissionCallback callback) override;
-  void Request(mojom::RequestParamsPtr params,
-               RequestCallback callback) override;
-
- protected:
-  content::WebContents* web_contents() { return web_contents_; }
 
  private:
-  // This raw pointer is safe to use because ExtensionWebContentsObserver whose
-  // lifetime is tied to the WebContents owns this instance.
-  content::WebContents* web_contents_;
   content::WebContentsFrameReceiverSet<mojom::LocalFrameHost> receivers_;
 };
 

extensions/browser/extension_function.cc

@@ -418,17 +418,6 @@ ExtensionFunction::~ExtensionFunction() {
   };
 
   DCHECK(did_respond() || can_be_destroyed_before_responding()) << name();
-
-  // If ignore_did_respond_for_testing() has been called it could cause another
-  // DCHECK about not calling Mojo callback.
-  // Since the ExtensionFunction request on the frame is a Mojo message
-  // which has a reply callback, it should be called before it's destroyed.
-  if (!response_callback_.is_null()) {
-    constexpr char kShouldCallMojoCallback[] = "Ignored did_respond()";
-    std::move(response_callback_)
-        .Run(ResponseType::FAILED, base::Value(base::Value::Type::LIST),
-             kShouldCallMojoCallback);
-  }
 #endif  // DCHECK_IS_ON()
 }
 

extensions/browser/extension_function.h

@@ -105,10 +105,10 @@ class ExtensionFunction : public base::RefCountedThreadSafe<
     BAD_MESSAGE
   };
 
-  // TODO(crbug.com/1196205): Convert the type of |results| to a base::Value.
-  using ResponseCallback = base::OnceCallback<void(ResponseType type,
-                                                   const base::Value& results,
-                                                   const std::string& error)>;
+  using ResponseCallback =
+      base::OnceCallback<void(ResponseType type,
+                              const base::ListValue& results,
+                              const std::string& error)>;
 
   ExtensionFunction();
 

extensions/browser/extension_function_dispatcher.cc

@@ -42,9 +42,9 @@
 #include "extensions/common/extension_api.h"
 #include "extensions/common/extension_messages.h"
 #include "extensions/common/extension_set.h"
+#include "extensions/common/mojom/frame.mojom.h"
 #include "ipc/ipc_message.h"
 #include "ipc/ipc_message_macros.h"
-#include "mojo/public/cpp/bindings/message.h"
 
 using content::BrowserThread;
 
@@ -92,27 +92,25 @@ class ExtensionFunctionDispatcher::ResponseCallbackWrapper
     }
   }
 
-  ExtensionFunction::ResponseCallback CreateCallback(
-      mojom::LocalFrameHost::RequestCallback callback) {
+  ExtensionFunction::ResponseCallback CreateCallback(int request_id) {
     return base::BindOnce(
         &ResponseCallbackWrapper::OnExtensionFunctionCompleted,
-        weak_ptr_factory_.GetWeakPtr(), std::move(callback));
+        weak_ptr_factory_.GetWeakPtr(), request_id);
   }
 
  private:
-  void OnExtensionFunctionCompleted(
-      mojom::LocalFrameHost::RequestCallback callback,
-      ExtensionFunction::ResponseType type,
-      const base::Value& results,
-      const std::string& error) {
+  void OnExtensionFunctionCompleted(int request_id,
+                                    ExtensionFunction::ResponseType type,
+                                    const base::ListValue& results,
+                                    const std::string& error) {
     if (type == ExtensionFunction::BAD_MESSAGE) {
       // The renderer will be shut down from ExtensionFunction::SetBadMessage().
-      std::move(callback).Run(false, results.Clone(), error);
       return;
     }
 
-    std::move(callback).Run(type == ExtensionFunction::SUCCEEDED,
-                            results.Clone(), error);
+    render_frame_host_->Send(new ExtensionMsg_Response(
+        render_frame_host_->GetRoutingID(), request_id,
+        type == ExtensionFunction::SUCCEEDED, results, error));
   }
 
   base::WeakPtr<ExtensionFunctionDispatcher> dispatcher_;
@@ -167,15 +165,15 @@ class ExtensionFunctionDispatcher::WorkerResponseCallbackWrapper
   void OnExtensionFunctionCompleted(int request_id,
                                     int worker_thread_id,
                                     ExtensionFunction::ResponseType type,
-                                    const base::Value& results,
+                                    const base::ListValue& results,
                                     const std::string& error) {
     if (type == ExtensionFunction::BAD_MESSAGE) {
       // The renderer will be shut down from ExtensionFunction::SetBadMessage().
       return;
     }
     render_process_host_->Send(new ExtensionMsg_ResponseWorker(
         worker_thread_id, request_id, type == ExtensionFunction::SUCCEEDED,
-        base::Value::AsListValue(results), error));
+        results, error));
   }
 
   base::WeakPtr<ExtensionFunctionDispatcher> dispatcher_;
@@ -226,66 +224,68 @@ ExtensionFunctionDispatcher::~ExtensionFunctionDispatcher() {
 }
 
 void ExtensionFunctionDispatcher::Dispatch(
-    mojom::RequestParamsPtr params,
-    content::RenderFrameHost* render_frame_host,
-    int render_process_id,
-    mojom::LocalFrameHost::RequestCallback callback) {
-  if (!render_frame_host || IsRequestFromServiceWorker(*params)) {
-    constexpr char kBadMessage[] = "LocalFrameHost::Request got a bad message.";
-    std::move(callback).Run(ExtensionFunction::FAILED,
-                            base::Value(base::Value::Type::LIST), kBadMessage);
-    // Kill the renderer if it's an invalid request.
-    mojo::ReportBadMessage(kBadMessage);
-    return;
-  }
-  // Extension API from a non Service Worker context, e.g. extension page,
-  // background page, content script.
-  std::unique_ptr<ResponseCallbackWrapper>& callback_wrapper =
-      response_callback_wrappers_[render_frame_host];
-  if (!callback_wrapper) {
-    callback_wrapper = std::make_unique<ResponseCallbackWrapper>(
-        AsWeakPtr(), render_frame_host);
-  }
-
-  DispatchWithCallbackInternal(
-      *params, render_frame_host, render_process_id,
-      callback_wrapper->CreateCallback(std::move(callback)));
-}
-
-void ExtensionFunctionDispatcher::DispatchForServiceWorker(
     const mojom::RequestParams& params,
+    content::RenderFrameHost* render_frame_host,
     int render_process_id) {
-  if (!IsRequestFromServiceWorker(params)) {
-    // Kill the renderer if it's an invalid request.
+  // Kill the renderer if it's an invalid request.
+  const bool is_valid_request =
+      (!render_frame_host && IsRequestFromServiceWorker(params)) ||
+      (render_frame_host && !IsRequestFromServiceWorker(params));
+  if (!is_valid_request) {
     bad_message::ReceivedBadMessage(render_process_id,
                                     bad_message::EFD_BAD_MESSAGE);
     return;
   }
 
-  content::RenderProcessHost* rph =
-      content::RenderProcessHost::FromID(render_process_id);
-  // WorkerResponseCallbackWrapper requires render process host to be around.
-  if (!rph)
-    return;
+  if (render_frame_host) {
+    // Extension API from a non Service Worker context, e.g. extension page,
+    // background page, content script.
+    ResponseCallbackWrapperMap::const_iterator iter =
+        response_callback_wrappers_.find(render_frame_host);
+    ResponseCallbackWrapper* callback_wrapper = nullptr;
+    if (iter == response_callback_wrappers_.end()) {
+      callback_wrapper =
+          new ResponseCallbackWrapper(AsWeakPtr(), render_frame_host);
+      response_callback_wrappers_[render_frame_host] =
+          base::WrapUnique(callback_wrapper);
+    } else {
+      callback_wrapper = iter->second.get();
+    }
+    DispatchWithCallbackInternal(
+        params, render_frame_host, render_process_id,
+        callback_wrapper->CreateCallback(params.request_id));
+  } else {
+    content::RenderProcessHost* rph =
+        content::RenderProcessHost::FromID(render_process_id);
+    // WorkerResponseCallbackWrapper requires render process host to be around.
+    if (!rph)
+      return;
 
-  WorkerId worker_id{params.extension_id, render_process_id,
-                     params.service_worker_version_id, params.worker_thread_id};
-  // Ignore if the worker has already stopped.
-  if (!ProcessManager::Get(browser_context_)->HasServiceWorker(worker_id))
-    return;
+    WorkerId worker_id{params.extension_id, render_process_id,
+                       params.service_worker_version_id,
+                       params.worker_thread_id};
+    // Ignore if the worker has already stopped.
+    if (!ProcessManager::Get(browser_context_)->HasServiceWorker(worker_id))
+      return;
 
-  WorkerResponseCallbackMapKey key(render_process_id,
-                                   params.service_worker_version_id);
-  std::unique_ptr<WorkerResponseCallbackWrapper>& callback_wrapper =
-      response_callback_wrappers_for_worker_[key];
-  if (!callback_wrapper) {
-    callback_wrapper = std::make_unique<WorkerResponseCallbackWrapper>(
-        AsWeakPtr(), rph, params.worker_thread_id);
+    WorkerResponseCallbackMapKey key(render_process_id,
+                                     params.service_worker_version_id);
+    WorkerResponseCallbackWrapperMap::const_iterator iter =
+        response_callback_wrappers_for_worker_.find(key);
+    WorkerResponseCallbackWrapper* callback_wrapper = nullptr;
+    if (iter == response_callback_wrappers_for_worker_.end()) {
+      callback_wrapper = new WorkerResponseCallbackWrapper(
+          AsWeakPtr(), rph, params.worker_thread_id);
+      response_callback_wrappers_for_worker_[key] =
+          base::WrapUnique(callback_wrapper);
+    } else {
+      callback_wrapper = iter->second.get();
+    }
+    DispatchWithCallbackInternal(
+        params, nullptr, render_process_id,
+        callback_wrapper->CreateCallback(params.request_id,
+                                         params.worker_thread_id));
   }
-
-  DispatchWithCallbackInternal(params, nullptr, render_process_id,
-                               callback_wrapper->CreateCallback(
-                                   params.request_id, params.worker_thread_id));
 }
 
 void ExtensionFunctionDispatcher::DispatchWithCallbackInternal(
@@ -294,13 +294,8 @@ void ExtensionFunctionDispatcher::DispatchWithCallbackInternal(
     int render_process_id,
     ExtensionFunction::ResponseCallback callback) {
   ProcessMap* process_map = ProcessMap::Get(browser_context_);
-  if (!process_map) {
-    constexpr char kProcessNotFound[] =
-        "The process for the extension is not found.";
-    std::move(callback).Run(ExtensionFunction::FAILED, base::Value(),
-                            kProcessNotFound);
+  if (!process_map)
     return;
-  }
 
   ExtensionRegistry* registry = ExtensionRegistry::Get(browser_context_);
   const Extension* extension =
@@ -496,7 +491,7 @@ ExtensionFunctionDispatcher::CreateExtensionFunction(
       ExtensionFunctionRegistry::GetInstance().NewFunction(params.name);
   if (!function) {
     LOG(ERROR) << "Unknown Extension API - " << params.name;
-    std::move(callback).Run(ExtensionFunction::FAILED, base::Value(),
+    std::move(callback).Run(ExtensionFunction::FAILED, base::ListValue(),
                             kCreationFailed);
     return nullptr;
   }

extensions/browser/extension_function_dispatcher.h

@@ -12,7 +12,7 @@
 
 #include "base/memory/weak_ptr.h"
 #include "extensions/browser/extension_function.h"
-#include "extensions/common/mojom/frame.mojom.h"
+#include "extensions/common/mojom/frame.mojom-forward.h"
 #include "ipc/ipc_sender.h"
 
 namespace content {
@@ -72,18 +72,12 @@ class ExtensionFunctionDispatcher
       content::BrowserContext* browser_context);
   ~ExtensionFunctionDispatcher();
 
-  // Dispatches a request and the response is sent in |callback| that is a reply
-  // of mojom::LocalFrameHost::Request.
-  void Dispatch(mojom::RequestParamsPtr params,
-                content::RenderFrameHost* render_frame_host,
-                int render_process_id,
-                mojom::LocalFrameHost::RequestCallback callback);
-
   // Message handlers.
-  // Dispatches a request for service woker and the response is sent to the
-  // corresponding render process in an ExtensionMsg_ResponseWorker message.
-  void DispatchForServiceWorker(const mojom::RequestParams& params,
-                                int render_process_id);
+  // The response is sent to the corresponding render view in an
+  // ExtensionMsg_Response message.
+  void Dispatch(const mojom::RequestParams& params,
+                content::RenderFrameHost* render_frame_host,
+                int render_process_id);
 
   // Called when an ExtensionFunction is done executing, after it has sent
   // a response (if any) to the extension.