Prepare Cast Shell for InterfaceProvider removal
Upstream Chromium no longer uses an InterfaceProvider to expose browser
interfaces to render frames. Cast Shell still does this for a small
handful of interfaces registered by downstream internal-only logic.
In order to support the transition of the internal repository away from
this mechanism, this CL introduces a bit of redundancy: interfaces
registered with CastWebContents through either of its two registration
mechanisms will now be exposed to *both* InterfaceProvider consumers as
well as BrowserInterfaceBroker.
Once this lands, internal renderer code can simply be rewritten to use
RenderFrame::GetBrowserInterfaceBroker instead of
RenderFrame::GetRemoteInterfaces(), and then upstream Chromium can
completely remove all its gnarly support for a browser-exposed
InterfaceProvider.
Bug: 977637
Change-Id: I21ae1fc31b35b0ca77ab2a00645feafe0ac67a73
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2547054
Reviewed-by: Sean Topping <[email protected]>
Reviewed-by: Daniel Cheng <[email protected]>
Commit-Queue: Ken Rockot <[email protected]>
Cr-Commit-Position: refs/heads/master@{#829858}
diff --git a/chromecast/browser/BUILD.gn b/chromecast/browser/BUILD.gn
index 91bc55d6..0d7cd67 100644
--- a/chromecast/browser/BUILD.gn
+++ b/chromecast/browser/BUILD.gn
@@ -6,6 +6,7 @@
import("//build/config/ui.gni")
import("//chromecast/chromecast.gni")
import("//media/media_options.gni")
+import("//mojo/public/tools/bindings/mojom.gni")
import("//testing/test.gni")
import("//tools/grit/grit_rule.gni")
@@ -540,6 +541,11 @@
]
}
+mojom("test_interfaces") {
+ testonly = true
+ sources = [ "test_interfaces.test-mojom" ]
+}
+
cast_source_set("browsertests") {
testonly = true
sources = [
@@ -555,6 +561,7 @@
defines = [ "HAS_OUT_OF_PROC_TEST_RUNNER" ]
deps = [
+ ":test_interfaces",
":test_support",
"//base",
"//chromecast:chromecast_buildflags",
@@ -567,6 +574,8 @@
"//content/test:test_support",
"//media:test_support",
"//net:test_support",
+ "//services/service_manager/public/cpp",
+ "//services/service_manager/public/mojom",
]
data = [
diff --git a/chromecast/browser/cast_browser_interface_binders.cc b/chromecast/browser/cast_browser_interface_binders.cc
index 5f66833..b33fd59 100644
--- a/chromecast/browser/cast_browser_interface_binders.cc
+++ b/chromecast/browser/cast_browser_interface_binders.cc
@@ -54,6 +54,26 @@
::media::mojom::Remotee::Name_, &interface_pipe);
}
+// Some Cast internals still dynamically set up interface binders after
+// frame host initialization. This is used to generically forward incoming
+// interface receivers to those objects until they can be reworked as static
+// registrations below.
+bool HandleGenericReceiver(content::RenderFrameHost* frame_host,
+ mojo::GenericPendingReceiver& receiver) {
+ auto* web_contents = content::WebContents::FromRenderFrameHost(frame_host);
+ if (!web_contents)
+ return false;
+
+ // Only WebContents created for Cast Webviews will have a CastWebContents
+ // object associated with them. We ignore these requests for any other
+ // WebContents.
+ auto* cast_web_contents = CastWebContents::FromWebContents(web_contents);
+ if (!cast_web_contents || !cast_web_contents->can_bind_interfaces())
+ return false;
+
+ return cast_web_contents->TryBindReceiver(receiver);
+}
+
} // namespace
void PopulateCastFrameBinders(
@@ -65,6 +85,9 @@
base::BindRepeating(&BindApplicationMediaCapabilities));
binder_map->Add<::media::mojom::Remotee>(
base::BindRepeating(&BindMediaRemotingRemotee));
+
+ binder_map->SetDefaultBinderDeprecated(
+ base::BindRepeating(&HandleGenericReceiver));
}
} // namespace shell
diff --git a/chromecast/browser/cast_web_contents.h b/chromecast/browser/cast_web_contents.h
index 2cf04fa0..b7fa8fb 100644
--- a/chromecast/browser/cast_web_contents.h
+++ b/chromecast/browser/cast_web_contents.h
@@ -16,6 +16,7 @@
#include "base/strings/string16.h"
#include "chromecast/common/mojom/feature_manager.mojom.h"
#include "content/public/common/media_playback_renderer_type.mojom.h"
+#include "mojo/public/cpp/bindings/generic_pending_receiver.h"
#include "services/service_manager/public/cpp/binder_registry.h"
#include "services/service_manager/public/cpp/interface_provider.h"
#include "third_party/blink/public/common/messaging/web_message_port.h"
@@ -380,6 +381,10 @@
// when it is ready.
virtual service_manager::BinderRegistry* binder_registry() = 0;
+ // Asks the CastWebContents to bind an interface receiver using either its
+ // registry or any registered InterfaceProvider.
+ virtual bool TryBindReceiver(mojo::GenericPendingReceiver& receiver) = 0;
+
// Used for owner to pass its |InterfaceProvider| pointers to CastWebContents.
// It is owner's responsibility to make sure each |InterfaceProvider| pointer
// has distinct mojo interface set.
diff --git a/chromecast/browser/cast_web_contents_browsertest.cc b/chromecast/browser/cast_web_contents_browsertest.cc
index 454478a..0e4e7bc 100644
--- a/chromecast/browser/cast_web_contents_browsertest.cc
+++ b/chromecast/browser/cast_web_contents_browsertest.cc
@@ -19,11 +19,13 @@
#include "base/strings/string_piece.h"
#include "base/strings/utf_string_conversions.h"
#include "base/test/bind.h"
+#include "base/threading/sequenced_task_runner_handle.h"
#include "chromecast/base/chromecast_switches.h"
#include "chromecast/base/metrics/cast_metrics_helper.h"
#include "chromecast/browser/cast_browser_context.h"
#include "chromecast/browser/cast_browser_process.h"
#include "chromecast/browser/cast_web_contents_impl.h"
+#include "chromecast/browser/test_interfaces.test-mojom.h"
#include "content/public/browser/browser_thread.h"
#include "content/public/browser/web_contents.h"
#include "content/public/browser/web_contents_delegate.h"
@@ -33,11 +35,16 @@
#include "content/public/test/browser_test_base.h"
#include "content/public/test/browser_test_utils.h"
#include "content/public/test/url_loader_interceptor.h"
-#include "mojo/public/cpp/bindings/connector.h"
+#include "mojo/public/cpp/bindings/pending_receiver.h"
+#include "mojo/public/cpp/bindings/pending_remote.h"
+#include "mojo/public/cpp/bindings/receiver_set.h"
+#include "mojo/public/cpp/bindings/remote.h"
#include "net/http/http_status_code.h"
#include "net/test/embedded_test_server/embedded_test_server.h"
#include "net/test/embedded_test_server/http_request.h"
#include "net/test/embedded_test_server/http_response.h"
+#include "services/service_manager/public/cpp/interface_provider.h"
+#include "services/service_manager/public/mojom/interface_provider.mojom.h"
#include "testing/gmock/include/gmock/gmock.h"
#include "testing/gtest/include/gtest/gtest.h"
#include "url/gurl.h"
@@ -239,8 +246,9 @@
SetUpCommandLine(base::CommandLine::ForCurrentProcess());
BrowserTestBase::SetUp();
}
- void SetUpCommandLine(base::CommandLine* command_line) final {
+ void SetUpCommandLine(base::CommandLine* command_line) override {
command_line->AppendSwitchASCII(switches::kTestType, "browser");
+ command_line->AppendSwitchASCII(switches::kEnableBlinkFeatures, "MojoJS");
}
void PreRunTestOnMainThread() override {
// Pump startup related events.
@@ -955,6 +963,147 @@
run_loop2.Run();
}
+// Helper for the test below. This exposes two interfaces, TestAdder and
+// TestDoubler. TestAdder is exposed only through a binder (see MakeAdderBinder)
+// which the test will register in the CastWebContents' binder_registry().
+// TestDoubler is exposed only through an InterfaceProvider, registered with the
+// CastWebContents using RegisterInterfaceProvider.
+class TestInterfaceProvider : public service_manager::mojom::InterfaceProvider,
+ public mojom::TestAdder,
+ public mojom::TestDoubler {
+ public:
+ TestInterfaceProvider()
+ : provider_(receiver_.BindNewPipeAndPassRemote(),
+ base::SequencedTaskRunnerHandle::Get()) {}
+ ~TestInterfaceProvider() override = default;
+
+ size_t num_adders() const { return adders_.size(); }
+ size_t num_doublers() const { return doublers_.size(); }
+
+ service_manager::InterfaceProvider* interface_provider() {
+ return &provider_;
+ }
+
+ base::RepeatingCallback<void(mojo::PendingReceiver<mojom::TestAdder>)>
+ MakeAdderBinder() {
+ return base::BindLambdaForTesting(
+ [this](mojo::PendingReceiver<mojom::TestAdder> receiver) {
+ adders_.Add(this, std::move(receiver));
+ OnRequestHandled();
+ });
+ }
+
+ // Waits for some number of new interface binding requests to be dispatched
+ // and then invokes `callback`.
+ void WaitForRequests(size_t n, base::OnceClosure callback) {
+ wait_callback_ = std::move(callback);
+ num_requests_to_wait_for_ = n;
+ }
+
+ // service_manager::mojom::InterfaceProvider:
+ void GetInterface(const std::string& interface_name,
+ mojo::ScopedMessagePipeHandle interface_pipe) override {
+ if (interface_name == mojom::TestDoubler::Name_) {
+ doublers_.Add(this, mojo::PendingReceiver<mojom::TestDoubler>(
+ std::move(interface_pipe)));
+ OnRequestHandled();
+ }
+ }
+
+ // mojom::TestAdder:
+ void Add(int32_t a, int32_t b, AddCallback callback) override {
+ std::move(callback).Run(a + b);
+ }
+
+ // mojom::TestDouble:
+ void Double(int32_t x, DoubleCallback callback) override {
+ std::move(callback).Run(x * 2);
+ }
+
+ private:
+ void OnRequestHandled() {
+ if (num_requests_to_wait_for_ == 0)
+ return;
+ DCHECK(wait_callback_);
+ if (--num_requests_to_wait_for_ == 0)
+ std::move(wait_callback_).Run();
+ }
+
+ mojo::Receiver<service_manager::mojom::InterfaceProvider> receiver_{this};
+ service_manager::InterfaceProvider provider_;
+ mojo::ReceiverSet<mojom::TestAdder> adders_;
+ mojo::ReceiverSet<mojom::TestDoubler> doublers_;
+ size_t num_requests_to_wait_for_ = 0;
+ base::OnceClosure wait_callback_;
+};
+
+IN_PROC_BROWSER_TEST_F(CastWebContentsBrowserTest, InterfaceBinding) {
+ // This test verifies that interfaces registered with the CastWebContents --
+ // either via its binder_registry() or its RegisterInterfaceProvider() API --
+ // are reachable from render frames using either the deprecated
+ // InterfaceProvider API (which results in an OnInterfaceRequestFromFrame call
+ // on the WebContents) or the newer BrowserInterfaceBroker API which is used
+ // in most other places (including from Mojo JS).
+ TestInterfaceProvider provider;
+ cast_web_contents_->binder_registry()->AddInterface(
+ provider.MakeAdderBinder());
+ cast_web_contents_->RegisterInterfaceProvider(
+ CastWebContents::InterfaceSet{mojom::TestDoubler::Name_},
+ provider.interface_provider());
+
+ // First verify that both interfaces are reachable using the deprecated
+ // WebContents path, which is triggered only by renderer-side use of
+ // RenderFrame::GetRemoteInterfaces(). Since poking renderer state in browser
+ // tests is challenging, we simply simulate the resulting WebContentsObbserver
+ // calls here instead and verify end-to-end connection for each interface.
+ content::RenderFrameHost* main_frame =
+ cast_web_contents_->web_contents()->GetMainFrame();
+ mojo::Remote<mojom::TestAdder> adder;
+ mojo::ScopedMessagePipeHandle adder_receiver_pipe =
+ adder.BindNewPipeAndPassReceiver().PassPipe();
+ cast_web_contents_->OnInterfaceRequestFromFrame(
+ main_frame, mojom::TestAdder::Name_, &adder_receiver_pipe);
+ mojo::Remote<mojom::TestDoubler> doubler;
+ mojo::ScopedMessagePipeHandle doubler_receiver_pipe =
+ doubler.BindNewPipeAndPassReceiver().PassPipe();
+ cast_web_contents_->OnInterfaceRequestFromFrame(
+ main_frame, mojom::TestDoubler::Name_, &doubler_receiver_pipe);
+
+ base::RunLoop add_loop;
+ adder->Add(37, 5, base::BindLambdaForTesting([&](int32_t result) {
+ EXPECT_EQ(42, result);
+ add_loop.Quit();
+ }));
+ add_loop.Run();
+
+ base::RunLoop double_loop;
+ doubler->Double(21, base::BindLambdaForTesting([&](int32_t result) {
+ EXPECT_EQ(42, result);
+ double_loop.Quit();
+ }));
+ double_loop.Run();
+
+ EXPECT_EQ(1u, provider.num_adders());
+ EXPECT_EQ(1u, provider.num_doublers());
+
+ // Now verify that the same interfaces are also reachable at the same binders
+ // when going through the newer BrowserInterfaceBroker path. For simplicity
+ // the test JS here does not have access to bindings and so does not make
+ // calls on the interfaces. It is however totally sufficient for us to verify
+ // that the page's requests result in new receivers being bound inside
+ // TestInterfaceProvider.
+ base::RunLoop loop;
+ provider.WaitForRequests(2, loop.QuitClosure());
+ embedded_test_server()->ServeFilesFromSourceDirectory(GetTestDataPath());
+ StartTestServer();
+ const GURL kUrl{embedded_test_server()->GetURL("/interface_binding.html")};
+ cast_web_contents_->LoadUrl(kUrl);
+ loop.Run();
+
+ EXPECT_EQ(2u, provider.num_adders());
+ EXPECT_EQ(2u, provider.num_doublers());
+}
+
} // namespace chromecast
#endif // CHROMECAST_BROWSER_CAST_WEB_CONTENTS_BROWSERTEST_H_
diff --git a/chromecast/browser/cast_web_contents_impl.cc b/chromecast/browser/cast_web_contents_impl.cc
index c962ebf..f327e2ec 100644
--- a/chromecast/browser/cast_web_contents_impl.cc
+++ b/chromecast/browser/cast_web_contents_impl.cc
@@ -397,6 +397,31 @@
return &binder_registry_;
}
+bool CastWebContentsImpl::TryBindReceiver(
+ mojo::GenericPendingReceiver& receiver) {
+ const std::string interface_name = *receiver.interface_name();
+ mojo::ScopedMessagePipeHandle interface_pipe = receiver.PassPipe();
+ if (binder_registry_.TryBindInterface(interface_name, &interface_pipe)) {
+ return true;
+ }
+
+ for (auto& entry : interface_providers_map_) {
+ auto const& interface_set = entry.first;
+ // Interface is provided by this InterfaceProvider.
+ if (interface_set.find(interface_name) != interface_set.end()) {
+ auto* interface_provider = entry.second;
+ interface_provider->GetInterfaceByName(interface_name,
+ std::move(interface_pipe));
+ return true;
+ }
+ }
+
+ // Unsuccessful, so give the caller its receiver back.
+ receiver =
+ mojo::GenericPendingReceiver(interface_name, std::move(interface_pipe));
+ return false;
+}
+
void CastWebContentsImpl::RegisterInterfaceProvider(
const InterfaceSet& interface_set,
service_manager::InterfaceProvider* interface_provider) {
@@ -504,18 +529,12 @@
if (!can_bind_interfaces()) {
return;
}
- if (binder_registry_.TryBindInterface(interface_name, interface_pipe)) {
- return;
- }
- for (auto& entry : interface_providers_map_) {
- auto const& interface_set = entry.first;
- // Interface is provided by this InterfaceProvider.
- if (interface_set.find(interface_name) != interface_set.end()) {
- auto* interface_provider = entry.second;
- interface_provider->GetInterfaceByName(interface_name,
- std::move(*interface_pipe));
- break;
- }
+
+ mojo::GenericPendingReceiver receiver(interface_name,
+ std::move(*interface_pipe));
+ if (!TryBindReceiver(receiver)) {
+ // If binding was unsuccessful, give the caller its pipe back.
+ *interface_pipe = receiver.PassPipe();
}
}
diff --git a/chromecast/browser/cast_web_contents_impl.h b/chromecast/browser/cast_web_contents_impl.h
index 535c99df..9a319763 100644
--- a/chromecast/browser/cast_web_contents_impl.h
+++ b/chromecast/browser/cast_web_contents_impl.h
@@ -63,6 +63,7 @@
const InterfaceSet& interface_set,
service_manager::InterfaceProvider* interface_provider) override;
service_manager::BinderRegistry* binder_registry() override;
+ bool TryBindReceiver(mojo::GenericPendingReceiver& receiver) override;
void BlockMediaLoading(bool blocked) override;
void BlockMediaStarting(bool blocked) override;
void EnableBackgroundVideoPlayback(bool enabled) override;
diff --git a/chromecast/browser/test/data/interface_binding.html b/chromecast/browser/test/data/interface_binding.html
new file mode 100644
index 0000000..70d2c48
--- /dev/null
+++ b/chromecast/browser/test/data/interface_binding.html
@@ -0,0 +1,17 @@
+<html>
+ <head><title>Binding some Mojo interfaces</title></head>
+ <body>
+ <script>
+ function bindInterface(name) {
+ const {handle0, handle1} = Mojo.createMessagePipe();
+ Mojo.bindInterface(name, handle0);
+ }
+
+ // The browser test which uses this page will succeed once these
+ // bindInterface requests reach browser-side binders registered by
+ // the test.
+ bindInterface('chromecast.mojom.TestAdder');
+ bindInterface('chromecast.mojom.TestDoubler');
+ </script>
+ </body>
+</html>
diff --git a/chromecast/browser/test/mock_cast_web_view.cc b/chromecast/browser/test/mock_cast_web_view.cc
index 6af9c9d8..fa9bf9b 100644
--- a/chromecast/browser/test/mock_cast_web_view.cc
+++ b/chromecast/browser/test/mock_cast_web_view.cc
@@ -13,6 +13,10 @@
return ®istry_;
}
+bool MockCastWebContents::TryBindReceiver(mojo::GenericPendingReceiver&) {
+ return false;
+}
+
MockCastWebView::MockCastWebView() {
mock_cast_web_contents_ = std::make_unique<MockCastWebContents>();
}
diff --git a/chromecast/browser/test/mock_cast_web_view.h b/chromecast/browser/test/mock_cast_web_view.h
index 22c366c1..5393bb5 100644
--- a/chromecast/browser/test/mock_cast_web_view.h
+++ b/chromecast/browser/test/mock_cast_web_view.h
@@ -64,6 +64,7 @@
MOCK_METHOD(bool, can_bind_interfaces, (), (override));
service_manager::BinderRegistry* binder_registry() override;
+ bool TryBindReceiver(mojo::GenericPendingReceiver&) override;
private:
service_manager::BinderRegistry registry_;
diff --git a/chromecast/browser/test_interfaces.test-mojom b/chromecast/browser/test_interfaces.test-mojom
new file mode 100644
index 0000000..f5ca2a76
--- /dev/null
+++ b/chromecast/browser/test_interfaces.test-mojom
@@ -0,0 +1,13 @@
+// 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.
+
+module chromecast.mojom;
+
+interface TestAdder {
+ Add(int32 a, int32 b) => (int32 result);
+};
+
+interface TestDoubler {
+ Double(int32 x) => (int32 result);
+};
diff --git a/mojo/public/cpp/bindings/binder_map.h b/mojo/public/cpp/bindings/binder_map.h
index 35bd8fc..09f67b6 100644
--- a/mojo/public/cpp/bindings/binder_map.h
+++ b/mojo/public/cpp/bindings/binder_map.h
@@ -100,12 +100,24 @@
"ContextType is void.");
auto it = binders_.find(*receiver->interface_name());
if (it == binders_.end())
- return false;
+ return default_binder_ && default_binder_.Run(context, *receiver);
it->second->BindInterface(std::move(context), receiver->PassPipe());
return true;
}
+ // DO NOT USE. This sets a generic default handler for any receiver that
+ // doesn't match a registered binder. It's a transitional API to help migrate
+ // some older code to BinderMap. Reliance on this mechanism makes security
+ // auditing more difficult. Note that this intentionally only supports use
+ // with a non-void ContextType, since that's the only existing use case.
+ using DefaultBinder =
+ base::RepeatingCallback<bool(ContextValueType context,
+ mojo::GenericPendingReceiver&)>;
+ void SetDefaultBinderDeprecated(DefaultBinder binder) {
+ default_binder_ = std::move(binder);
+ }
+
private:
using IsVoidContext = std::is_same<ContextType, void>;
@@ -113,6 +125,7 @@
std::string,
std::unique_ptr<internal::GenericCallbackBinderWithContext<ContextType>>>
binders_;
+ DefaultBinder default_binder_;
};
// Common alias for BinderMapWithContext that has no context. Binders added to
