From ff26856348279c39eb5d5ab4f96d76596e80756b Mon Sep 17 00:00:00 2001 From: Nasko Oskov Date: Fri, 1 Sep 2017 20:01:40 +0000 Subject: [PATCH] Allow tests to swap Mojo interface implementations. This CL adds support to Binding and BindingSet to allow tests to swap the existing interface implementation with a different one. It allows tests to interpose an implementation that can change the interface behavior or inputs to it. Bug: Change-Id: If22a4d0124ab0d961519de9cefb2dab4bebf939f Reviewed-on: https://chromium-review.googlesource.com/644187 Commit-Queue: Yuzhu Shen Reviewed-by: Yuzhu Shen Cr-Commit-Position: refs/heads/master@{#499267} --- .../browser/isolated_origin_browsertest.cc | 56 +++++++++++-------- content/browser/storage_partition_impl.cc | 6 +- content/browser/storage_partition_impl.h | 10 +++- mojo/public/cpp/bindings/binding.h | 5 ++ mojo/public/cpp/bindings/binding_set.h | 16 ++++++ mojo/public/cpp/bindings/lib/binding_state.h | 5 ++ .../cpp_templates/interface_declaration.tmpl | 2 +- 7 files changed, 71 insertions(+), 29 deletions(-) diff --git a/content/browser/isolated_origin_browsertest.cc b/content/browser/isolated_origin_browsertest.cc index c516436ddc7eb3..c1c9d5083a5e7a 100644 --- a/content/browser/isolated_origin_browsertest.cc +++ b/content/browser/isolated_origin_browsertest.cc @@ -508,28 +508,42 @@ IN_PROC_BROWSER_TEST_F(IsolatedOriginTest, IsolatedOriginWithSubdomain) { // This class allows intercepting the OpenLocalStorage method and changing // the parameters to the real implementation of it. class StoragePartitonInterceptor - : public mojom::StoragePartitionServiceInterceptorForTesting { + : public mojom::StoragePartitionServiceInterceptorForTesting, + public RenderProcessHostObserver { public: - StoragePartitonInterceptor(RenderProcessHostImpl* rph) { - // Install a custom callback for handling errors during interface calls. - // This is needed because the passthrough calls that this object makes - // to the real implementaion of the service don't have the correct - // context to invoke the real error callback. - mojo::edk::SetDefaultProcessErrorCallback(base::Bind( - [](int process_id, const std::string& s) { - bad_message::ReceivedBadMessage(process_id, - bad_message::DSMF_LOAD_STORAGE); - }, - rph->GetID())); - - static_cast(rph->GetStoragePartition()) - ->Bind(rph->GetID(), mojo::MakeRequest(&storage_partition_service_)); + StoragePartitonInterceptor(RenderProcessHostImpl* rph, + mojom::StoragePartitionServiceRequest request) { + StoragePartitionImpl* storage_partition = + static_cast(rph->GetStoragePartition()); + + // Bind the real StoragePartitionService implementation. + mojo::BindingId binding_id = + storage_partition->Bind(rph->GetID(), std::move(request)); + + // Now replace it with this object and keep a pointer to the real + // implementation. + storage_partition_service_ = + storage_partition->bindings_for_testing().SwapImplForTesting(binding_id, + this); + + // Register the |this| as a RenderProcessHostObserver, so it can be + // correctly cleaned up when the process exits. + rph->AddObserver(this); + } + + // Ensure this object is cleaned up when the process goes away, since it + // is not owned by anyone else. + void RenderProcessExited(RenderProcessHost* host, + base::TerminationStatus status, + int exit_code) override { + host->RemoveObserver(this); + delete this; } // Allow all methods that aren't explicitly overriden to pass through // unmodified. mojom::StoragePartitionService* GetForwardingInterface() override { - return storage_partition_service_.get(); + return storage_partition_service_; } // Override this method to allow changing the origin. It simulates a @@ -545,17 +559,15 @@ class StoragePartitonInterceptor private: // Keep a pointer to the original implementation of the service, so all // calls can be forwarded to it. - // Note: When making calls through this object, they are in-process calls, - // so state on the receiving side of the call will be missing the real - // information of which process has made the real method call. - mojom::StoragePartitionServicePtr storage_partition_service_; + mojom::StoragePartitionService* storage_partition_service_; }; void CreateTestStoragePartitionService( RenderProcessHostImpl* rph, mojom::StoragePartitionServiceRequest request) { - mojo::MakeStrongBinding(base::MakeUnique(rph), - std::move(request)); + // This object will register as RenderProcessHostObserver, so it will + // clean itself automatically on process exit. + new StoragePartitonInterceptor(rph, std::move(request)); } // Verify that an isolated renderer process cannot read localStorage of an diff --git a/content/browser/storage_partition_impl.cc b/content/browser/storage_partition_impl.cc index dcd12d2fd06b26..a65c893b536337 100644 --- a/content/browser/storage_partition_impl.cc +++ b/content/browser/storage_partition_impl.cc @@ -689,7 +689,7 @@ void StoragePartitionImpl::OpenLocalStorage( int process_id = bindings_.dispatch_context(); if (!ChildProcessSecurityPolicy::GetInstance()->CanAccessDataForOrigin( process_id, origin.GetURL())) { - mojo::ReportBadMessage("Access denied for localStorage request"); + bindings_.ReportBadMessage("Access denied for localStorage request"); return; } dom_storage_context_->OpenLocalStorage(origin, std::move(request)); @@ -1003,10 +1003,10 @@ BrowserContext* StoragePartitionImpl::browser_context() const { return browser_context_; } -void StoragePartitionImpl::Bind( +mojo::BindingId StoragePartitionImpl::Bind( int process_id, mojo::InterfaceRequest request) { - bindings_.AddBinding(this, std::move(request), process_id); + return bindings_.AddBinding(this, std::move(request), process_id); } void StoragePartitionImpl::OverrideQuotaManagerForTesting( diff --git a/content/browser/storage_partition_impl.h b/content/browser/storage_partition_impl.h index b5902ac7f70fbc..47137cf1d58e68 100644 --- a/content/browser/storage_partition_impl.h +++ b/content/browser/storage_partition_impl.h @@ -135,9 +135,13 @@ class CONTENT_EXPORT StoragePartitionImpl // Can return nullptr while |this| is being destroyed. BrowserContext* browser_context() const; - // Called by each renderer process once. - void Bind(int process_id, - mojo::InterfaceRequest request); + // Called by each renderer process once. Returns the id of the created + // binding. + mojo::BindingId Bind( + int process_id, + mojo::InterfaceRequest request); + + auto& bindings_for_testing() { return bindings_; } struct DataDeletionHelper; struct QuotaManagedDataDeletionHelper; diff --git a/mojo/public/cpp/bindings/binding.h b/mojo/public/cpp/bindings/binding.h index b0e22244c3ed31..409f9cd9323a78 100644 --- a/mojo/public/cpp/bindings/binding.h +++ b/mojo/public/cpp/bindings/binding.h @@ -229,6 +229,11 @@ class Binding { return internal_state_.RouterForTesting(); } + // Allows test code to swap the interface implementation. + ImplPointerType SwapImplForTesting(ImplPointerType new_impl) { + return internal_state_.SwapImplForTesting(new_impl); + } + private: internal::BindingState internal_state_; diff --git a/mojo/public/cpp/bindings/binding_set.h b/mojo/public/cpp/bindings/binding_set.h index 7e5ece27bb8a89..737affb0e298ec 100644 --- a/mojo/public/cpp/bindings/binding_set.h +++ b/mojo/public/cpp/bindings/binding_set.h @@ -123,6 +123,18 @@ class BindingSetBase { return true; } + // Swaps the interface implementation with a different one, to allow tests + // to modify behavior. + // + // Returns the existing interface implementation to the caller. + ImplPointerType SwapImplForTesting(BindingId id, ImplPointerType new_impl) { + auto it = bindings_.find(id); + if (it == bindings_.end()) + return nullptr; + + return it->second->SwapImplForTesting(new_impl); + } + void CloseAllBindings() { bindings_.clear(); } bool empty() const { return bindings_.empty(); } @@ -216,6 +228,10 @@ class BindingSetBase { void FlushForTesting() { binding_.FlushForTesting(); } + ImplPointerType SwapImplForTesting(ImplPointerType new_impl) { + return binding_.SwapImplForTesting(new_impl); + } + private: class DispatchFilter : public MessageReceiver { public: diff --git a/mojo/public/cpp/bindings/lib/binding_state.h b/mojo/public/cpp/bindings/lib/binding_state.h index 465951abe694d7..4d9be92fc502c2 100644 --- a/mojo/public/cpp/bindings/lib/binding_state.h +++ b/mojo/public/cpp/bindings/lib/binding_state.h @@ -121,6 +121,11 @@ class BindingState : public BindingStateBase { } Interface* impl() { return ImplRefTraits::GetRawPointer(&stub_.sink()); } + ImplPointerType SwapImplForTesting(ImplPointerType new_impl) { + Interface* old_impl = impl(); + stub_.set_sink(std::move(new_impl)); + return old_impl; + } private: typename Interface::template Stub_ stub_; diff --git a/mojo/public/tools/bindings/generators/cpp_templates/interface_declaration.tmpl b/mojo/public/tools/bindings/generators/cpp_templates/interface_declaration.tmpl index f5acd37de6902a..4221832264ac6d 100644 --- a/mojo/public/tools/bindings/generators/cpp_templates/interface_declaration.tmpl +++ b/mojo/public/tools/bindings/generators/cpp_templates/interface_declaration.tmpl @@ -69,6 +69,6 @@ class {{export_attribute}} {{interface.name}}InterceptorForTesting : public {{in virtual {{interface.name}}* GetForwardingInterface() = 0; {%- for method in interface.methods %} - void {{method.name}}({{interface_macros.declare_request_params("", method, use_once_callback)}}); + void {{method.name}}({{interface_macros.declare_request_params("", method, use_once_callback)}}) override; {%- endfor %} };