diff --git a/content/browser/service_worker/service_worker_provider_host.cc b/content/browser/service_worker/service_worker_provider_host.cc index 20687528248c1..aa52f1b82b329 100644 --- a/content/browser/service_worker/service_worker_provider_host.cc +++ b/content/browser/service_worker/service_worker_provider_host.cc @@ -559,13 +559,6 @@ ServiceWorkerProviderHost::PrepareForCrossSiteTransfer() { DCHECK_EQ(kDocumentMainThreadId, render_thread_id_); DCHECK_NE(SERVICE_WORKER_PROVIDER_UNKNOWN, info_.type); - // Clear the controller from the renderer-side provider, since no one knows - // what's going to happen until after cross-site transfer finishes. - if (controller_) { - SendSetControllerServiceWorker(nullptr, - false /* notify_controllerchange */); - } - std::unique_ptr provisional_host = base::WrapUnique(new ServiceWorkerProviderHost( process_id(), @@ -578,6 +571,13 @@ ServiceWorkerProviderHost::PrepareForCrossSiteTransfer() { RemoveAllMatchingRegistrations(); + // Clear the controller from the renderer-side provider, since no one knows + // what's going to happen until after cross-site transfer finishes. + if (controller_) { + SendSetControllerServiceWorker(nullptr, + false /* notify_controllerchange */); + } + render_process_id_ = ChildProcessHost::kInvalidUniqueID; render_thread_id_ = kInvalidEmbeddedWorkerThreadId; dispatcher_host_ = nullptr; @@ -881,20 +881,14 @@ void ServiceWorkerProviderHost::SendSetControllerServiceWorker( DCHECK_EQ(controller_.get(), version); } - std::vector used_features; - if (version) { - for (const auto& feature : version->used_features()) { - // TODO: version->used_features() should never have a feature outside the - // known feature range. But there is special case, see the details in - // crbug.com/758419. - if (feature >= 0 && - feature < static_cast( - blink::mojom::WebFeature::kNumberOfFeatures)) - used_features.push_back(static_cast(feature)); - } - } - container_->SetController(GetOrCreateServiceWorkerHandle(version), - used_features, notify_controllerchange); + ServiceWorkerMsg_SetControllerServiceWorker_Params params; + params.thread_id = render_thread_id_; + params.provider_id = provider_id(); + params.object_info = GetOrCreateServiceWorkerHandle(version); + params.should_notify_controllerchange = notify_controllerchange; + if (version) + params.used_features = version->used_features(); + Send(new ServiceWorkerMsg_SetControllerServiceWorker(params)); } void ServiceWorkerProviderHost::Register( diff --git a/content/browser/service_worker/service_worker_provider_host_unittest.cc b/content/browser/service_worker/service_worker_provider_host_unittest.cc index 5c691cebd8aa6..5c1fe628c68a9 100644 --- a/content/browser/service_worker/service_worker_provider_host_unittest.cc +++ b/content/browser/service_worker/service_worker_provider_host_unittest.cc @@ -35,6 +35,17 @@ namespace content { namespace { +// Sets the document URL for |host| and associates it with |registration|. +// A dumb version of +// ServiceWorkerControlleeRequestHandler::PrepareForMainResource(). +void SimulateServiceWorkerControlleeRequestHandler( + ServiceWorkerProviderHost* host, + ServiceWorkerRegistration* registration) { + host->SetDocumentUrl(GURL("https://www.example.com/page")); + host->AssociateRegistration(registration, + false /* notify_controllerchange */); +} + const char kServiceWorkerScheme[] = "i-can-use-service-worker"; class ServiceWorkerTestContentClient : public TestContentClient { @@ -361,27 +372,6 @@ TEST_F(ServiceWorkerProviderHostTest, RemoveProvider) { EXPECT_FALSE(context_->GetProviderHost(process_id, provider_id)); } -class MockServiceWorkerContainer : public mojom::ServiceWorkerContainer { - public: - explicit MockServiceWorkerContainer( - mojom::ServiceWorkerContainerAssociatedRequest request) - : binding_(this, std::move(request)) {} - - ~MockServiceWorkerContainer() override = default; - - void SetController(const ServiceWorkerObjectInfo& controller, - const std::vector& used_features, - bool should_notify_controllerchange) override { - was_set_controller_called_ = true; - } - - bool was_set_controller_called() const { return was_set_controller_called_; } - - private: - bool was_set_controller_called_ = false; - mojo::AssociatedBinding binding_; -}; - TEST_F(ServiceWorkerProviderHostTest, Controller) { base::CommandLine::ForCurrentProcess()->AppendSwitch( switches::kEnableBrowserSideNavigation); @@ -395,31 +385,27 @@ TEST_F(ServiceWorkerProviderHostTest, Controller) { true /* is_parent_frame_secure */); remote_endpoints_.emplace_back(); remote_endpoints_.back().BindWithProviderHostInfo(&info); - auto container = base::MakeUnique( - std::move(*remote_endpoints_.back().client_request())); // Create an active version and then start the navigation. scoped_refptr version = new ServiceWorkerVersion( registration1_.get(), GURL("https://www.example.com/sw.js"), 1 /* version_id */, helper_->context()->AsWeakPtr()); registration1_->SetActiveVersion(version); + SimulateServiceWorkerControlleeRequestHandler(host.get(), + registration1_.get()); // Finish the navigation. - host->SetDocumentUrl(GURL("https://www.example.com/page")); host->CompleteNavigationInitialized( helper_->mock_render_process_id(), std::move(info), helper_->GetDispatcherHostForProcess(helper_->mock_render_process_id()) ->AsWeakPtr()); - host->AssociateRegistration(registration1_.get(), - false /* notify_controllerchange */); - base::RunLoop().RunUntilIdle(); - // The page should be controlled since there was an active version at the // time navigation started. The SetController IPC should have been sent. EXPECT_TRUE(host->active_version()); EXPECT_EQ(host->active_version(), host->controller()); - EXPECT_TRUE(container->was_set_controller_called()); + EXPECT_TRUE(helper_->ipc_sink()->GetUniqueMessageMatching( + ServiceWorkerMsg_SetControllerServiceWorker::ID)); } TEST_F(ServiceWorkerProviderHostTest, ActiveIsNotController) { @@ -435,34 +421,31 @@ TEST_F(ServiceWorkerProviderHostTest, ActiveIsNotController) { true /* is_parent_frame_secure */); remote_endpoints_.emplace_back(); remote_endpoints_.back().BindWithProviderHostInfo(&info); - auto container = base::MakeUnique( - std::move(*remote_endpoints_.back().client_request())); - // Create an installing version and then start the navigation. + // Associate it with an installing registration then start the navigation. scoped_refptr version = new ServiceWorkerVersion( registration1_.get(), GURL("https://www.example.com/sw.js"), 1 /* version_id */, helper_->context()->AsWeakPtr()); registration1_->SetInstallingVersion(version); + SimulateServiceWorkerControlleeRequestHandler(host.get(), + registration1_.get()); + + // Promote the worker to active while navigation is still happening. + registration1_->SetActiveVersion(version); // Finish the navigation. - host->SetDocumentUrl(GURL("https://www.example.com/page")); host->CompleteNavigationInitialized( helper_->mock_render_process_id(), std::move(info), helper_->GetDispatcherHostForProcess(helper_->mock_render_process_id()) ->AsWeakPtr()); - host->AssociateRegistration(registration1_.get(), - false /* notify_controllerchange */); - // Promote the worker to active while navigation is still happening. - registration1_->SetActiveVersion(version); - base::RunLoop().RunUntilIdle(); - // The page should not be controlled since there was no active version at the // time navigation started. Furthermore, no SetController IPC should have been // sent. EXPECT_TRUE(host->active_version()); EXPECT_FALSE(host->controller()); - EXPECT_FALSE(container->was_set_controller_called()); + EXPECT_EQ(nullptr, helper_->ipc_sink()->GetFirstMessageMatching( + ServiceWorkerMsg_SetControllerServiceWorker::ID)); } } // namespace content diff --git a/content/child/service_worker/service_worker_dispatcher.cc b/content/child/service_worker/service_worker_dispatcher.cc index 0e03d96ed8603..093df6827bbd6 100644 --- a/content/child/service_worker/service_worker_dispatcher.cc +++ b/content/child/service_worker/service_worker_dispatcher.cc @@ -93,6 +93,8 @@ void ServiceWorkerDispatcher::OnMessageReceived(const IPC::Message& msg) { OnSetVersionAttributes) IPC_MESSAGE_HANDLER(ServiceWorkerMsg_UpdateFound, OnUpdateFound) + IPC_MESSAGE_HANDLER(ServiceWorkerMsg_SetControllerServiceWorker, + OnSetControllerServiceWorker) IPC_MESSAGE_HANDLER(ServiceWorkerMsg_MessageToDocument, OnPostMessage) IPC_MESSAGE_HANDLER(ServiceWorkerMsg_CountFeature, OnCountFeature) @@ -184,18 +186,9 @@ void ServiceWorkerDispatcher::AddProviderClient( } void ServiceWorkerDispatcher::RemoveProviderClient(int provider_id) { - auto iter = provider_clients_.find(provider_id); // This could be possibly called multiple times to ensure termination. - if (iter != provider_clients_.end()) - provider_clients_.erase(iter); -} - -blink::WebServiceWorkerProviderClient* -ServiceWorkerDispatcher::GetProviderClient(int provider_id) { - auto iter = provider_clients_.find(provider_id); - if (iter != provider_clients_.end()) - return iter->second; - return nullptr; + if (base::ContainsKey(provider_clients_, provider_id)) + provider_clients_.erase(provider_id); } ServiceWorkerDispatcher* @@ -224,11 +217,6 @@ ServiceWorkerDispatcher* ServiceWorkerDispatcher::GetThreadSpecificInstance() { g_dispatcher_tls.Pointer()->Get()); } -std::unique_ptr ServiceWorkerDispatcher::Adopt( - const ServiceWorkerObjectInfo& info) { - return ServiceWorkerHandleReference::Adopt(info, thread_safe_sender_.get()); -} - void ServiceWorkerDispatcher::WillStopCurrentWorkerThread() { delete this; } @@ -522,6 +510,43 @@ void ServiceWorkerDispatcher::OnUpdateFound( found->second->OnUpdateFound(); } +void ServiceWorkerDispatcher::OnSetControllerServiceWorker( + const ServiceWorkerMsg_SetControllerServiceWorker_Params& params) { + TRACE_EVENT2( + "ServiceWorker", "ServiceWorkerDispatcher::OnSetControllerServiceWorker", + "Thread ID", params.thread_id, "Provider ID", params.provider_id); + + // Adopt the reference sent from the browser process and pass it to the + // provider context if it exists. + std::unique_ptr handle_ref = + Adopt(params.object_info); + ProviderContextMap::iterator provider = + provider_contexts_.find(params.provider_id); + if (provider != provider_contexts_.end()) { + provider->second->SetController(std::move(handle_ref), + params.used_features); + } + + ProviderClientMap::iterator found = + provider_clients_.find(params.provider_id); + if (found != provider_clients_.end()) { + // Sync the controllee's use counter with the service worker's one. + for (uint32_t feature : params.used_features) + found->second->CountFeature(feature); + + // Get the existing worker object or create a new one with a new reference + // to populate the .controller field. + scoped_refptr worker = + GetOrCreateServiceWorker(ServiceWorkerHandleReference::Create( + params.object_info, thread_safe_sender_.get())); + found->second->SetController(WebServiceWorkerImpl::CreateHandle(worker), + params.should_notify_controllerchange); + // You must not access |found| after setController() because it may fire the + // controllerchange event that may remove the provider client, for example, + // by detaching an iframe. + } +} + void ServiceWorkerDispatcher::OnPostMessage( const ServiceWorkerMsg_MessageToDocument_Params& params) { // Make sure we're on the main document thread. (That must be the only @@ -586,4 +611,9 @@ void ServiceWorkerDispatcher::RemoveServiceWorkerRegistration( registrations_.erase(registration_handle_id); } +std::unique_ptr ServiceWorkerDispatcher::Adopt( + const ServiceWorkerObjectInfo& info) { + return ServiceWorkerHandleReference::Adopt(info, thread_safe_sender_.get()); +} + } // namespace content diff --git a/content/child/service_worker/service_worker_dispatcher.h b/content/child/service_worker/service_worker_dispatcher.h index 3b7dfd5e7d1bb..a87c24ce3dfa0 100644 --- a/content/child/service_worker/service_worker_dispatcher.h +++ b/content/child/service_worker/service_worker_dispatcher.h @@ -112,8 +112,6 @@ class CONTENT_EXPORT ServiceWorkerDispatcher : public WorkerThread::Observer { blink::WebServiceWorkerProviderClient* client); void RemoveProviderClient(int provider_id); - blink::WebServiceWorkerProviderClient* GetProviderClient(int provider_id); - // Returns the existing service worker or a newly created one with the given // handle reference. Returns nullptr if the given reference is invalid. scoped_refptr GetOrCreateServiceWorker( @@ -141,11 +139,6 @@ class CONTENT_EXPORT ServiceWorkerDispatcher : public WorkerThread::Observer { // instance if thread-local instance doesn't exist. static ServiceWorkerDispatcher* GetThreadSpecificInstance(); - // Assumes that the given object information retains an interprocess handle - // reference passed from the browser process, and adopts it. - std::unique_ptr Adopt( - const ServiceWorkerObjectInfo& info); - base::SingleThreadTaskRunner* main_thread_task_runner() { return main_thread_task_runner_.get(); } @@ -234,6 +227,11 @@ class CONTENT_EXPORT ServiceWorkerDispatcher : public WorkerThread::Observer { void RemoveServiceWorkerRegistration( int registration_handle_id); + // Assumes that the given object information retains an interprocess handle + // reference passed from the browser process, and adopts it. + std::unique_ptr Adopt( + const ServiceWorkerObjectInfo& info); + UpdateCallbackMap pending_update_callbacks_; UnregistrationCallbackMap pending_unregistration_callbacks_; EnableNavigationPreloadCallbackMap enable_navigation_preload_callbacks_; diff --git a/content/child/service_worker/service_worker_dispatcher_unittest.cc b/content/child/service_worker/service_worker_dispatcher_unittest.cc index ef1d2abbc0c59..6e329c3848b15 100644 --- a/content/child/service_worker/service_worker_dispatcher_unittest.cc +++ b/content/child/service_worker/service_worker_dispatcher_unittest.cc @@ -98,6 +98,20 @@ class ServiceWorkerDispatcherTest : public testing::Test { return ContainsKey(dispatcher_->registrations_, registration_handle_id); } + void OnSetControllerServiceWorker(int thread_id, + int provider_id, + const ServiceWorkerObjectInfo& info, + bool should_notify_controllerchange, + const std::set& used_features) { + ServiceWorkerMsg_SetControllerServiceWorker_Params params; + params.thread_id = thread_id; + params.provider_id = provider_id; + params.object_info = info; + params.should_notify_controllerchange = should_notify_controllerchange; + params.used_features = used_features; + dispatcher_->OnSetControllerServiceWorker(params); + } + void OnPostMessage(const ServiceWorkerMsg_MessageToDocument_Params& params) { dispatcher_->OnPostMessage(params); } @@ -139,7 +153,11 @@ class MockWebServiceWorkerProviderClientImpl } void SetController(std::unique_ptr handle, - bool should_notify_controller_change) override {} + bool shouldNotifyControllerChange) override { + // WebPassOwnPtr cannot be owned in Chromium, so drop the handle here. + // The destruction releases ServiceWorkerHandleReference. + is_set_controlled_called_ = true; + } void DispatchMessageEvent( std::unique_ptr handle, @@ -154,17 +172,136 @@ class MockWebServiceWorkerProviderClientImpl used_features_.insert(feature); } + bool is_set_controlled_called() const { return is_set_controlled_called_; } + bool is_dispatch_message_event_called() const { return is_dispatch_message_event_called_; } private: const int provider_id_; + bool is_set_controlled_called_ = false; bool is_dispatch_message_event_called_ = false; ServiceWorkerDispatcher* dispatcher_; std::set used_features_; }; +TEST_F(ServiceWorkerDispatcherTest, OnSetControllerServiceWorker) { + const int kProviderId = 10; + bool should_notify_controllerchange = true; + + // Assume that these objects are passed from the browser process and own + // references to browser-side registration/worker representations. + blink::mojom::ServiceWorkerRegistrationObjectInfoPtr info; + ServiceWorkerVersionAttributes attrs; + CreateObjectInfoAndVersionAttributes(&info, &attrs); + + // (1) In the case there are no SWProviderContext and WebSWProviderClient for + // the provider, the passed reference to the active worker should be adopted + // but immediately released because there is no provider context to own it. + OnSetControllerServiceWorker(kDocumentMainThreadId, kProviderId, attrs.active, + should_notify_controllerchange, + std::set()); + ASSERT_EQ(1UL, ipc_sink()->message_count()); + EXPECT_EQ(ServiceWorkerHostMsg_DecrementServiceWorkerRefCount::ID, + ipc_sink()->GetMessageAt(0)->type()); + ipc_sink()->ClearMessages(); + + // (2) In the case there is no WebSWProviderClient but SWProviderContext for + // the provider, the passed referecence should be adopted and owned by the + // provider context. + auto provider_context = base::MakeRefCounted( + kProviderId, SERVICE_WORKER_PROVIDER_FOR_WINDOW, + nullptr /* provider_request */, nullptr /* host_ptr_info */, dispatcher(), + nullptr /* loader_factory_getter */); + ipc_sink()->ClearMessages(); + OnSetControllerServiceWorker(kDocumentMainThreadId, kProviderId, attrs.active, + should_notify_controllerchange, + std::set()); + EXPECT_EQ(0UL, ipc_sink()->message_count()); + + // Destruction of the provider context should release references to the + // associated registration and the controller. + provider_context = nullptr; + ASSERT_EQ(1UL, ipc_sink()->message_count()); + EXPECT_EQ(ServiceWorkerHostMsg_DecrementServiceWorkerRefCount::ID, + ipc_sink()->GetMessageAt(0)->type()); + ipc_sink()->ClearMessages(); + + // (3) In the case there is no SWProviderContext but WebSWProviderClient for + // the provider, the new reference should be created and owned by the provider + // client (but the reference is immediately released due to limitation of the + // mock provider client. See the comment on setController() of the mock). + // In addition, the passed reference should be adopted but immediately + // released because there is no provider context to own it. + std::unique_ptr provider_client( + new MockWebServiceWorkerProviderClientImpl(kProviderId, dispatcher())); + ASSERT_FALSE(provider_client->is_set_controlled_called()); + OnSetControllerServiceWorker(kDocumentMainThreadId, kProviderId, attrs.active, + should_notify_controllerchange, + std::set()); + EXPECT_TRUE(provider_client->is_set_controlled_called()); + ASSERT_EQ(3UL, ipc_sink()->message_count()); + EXPECT_EQ(ServiceWorkerHostMsg_IncrementServiceWorkerRefCount::ID, + ipc_sink()->GetMessageAt(0)->type()); + EXPECT_EQ(ServiceWorkerHostMsg_DecrementServiceWorkerRefCount::ID, + ipc_sink()->GetMessageAt(1)->type()); + EXPECT_EQ(ServiceWorkerHostMsg_DecrementServiceWorkerRefCount::ID, + ipc_sink()->GetMessageAt(2)->type()); + provider_client.reset(); + ipc_sink()->ClearMessages(); + + // (4) In the case there are both SWProviderContext and SWProviderClient for + // the provider, the passed referecence should be adopted and owned by the + // provider context. In addition, the new reference should be created for the + // provider client and immediately released due to limitation of the mock + // implementation. + provider_context = base::MakeRefCounted( + kProviderId, SERVICE_WORKER_PROVIDER_FOR_WINDOW, + nullptr /* provider_request */, nullptr /* host_ptr_info */, dispatcher(), + nullptr /* loader_factory_getter */); + provider_client.reset( + new MockWebServiceWorkerProviderClientImpl(kProviderId, dispatcher())); + ASSERT_FALSE(provider_client->is_set_controlled_called()); + ipc_sink()->ClearMessages(); + OnSetControllerServiceWorker(kDocumentMainThreadId, kProviderId, attrs.active, + should_notify_controllerchange, + std::set()); + EXPECT_TRUE(provider_client->is_set_controlled_called()); + ASSERT_EQ(2UL, ipc_sink()->message_count()); + EXPECT_EQ(ServiceWorkerHostMsg_IncrementServiceWorkerRefCount::ID, + ipc_sink()->GetMessageAt(0)->type()); + EXPECT_EQ(ServiceWorkerHostMsg_DecrementServiceWorkerRefCount::ID, + ipc_sink()->GetMessageAt(1)->type()); +} + +// Test that clearing the controller by sending a kInvalidServiceWorkerHandle +// results in the provider context having a null controller. +TEST_F(ServiceWorkerDispatcherTest, OnSetControllerServiceWorker_Null) { + const int kProviderId = 10; + bool should_notify_controllerchange = true; + + blink::mojom::ServiceWorkerRegistrationObjectInfoPtr info; + ServiceWorkerVersionAttributes attrs; + CreateObjectInfoAndVersionAttributes(&info, &attrs); + + std::unique_ptr provider_client( + new MockWebServiceWorkerProviderClientImpl(kProviderId, dispatcher())); + auto provider_context = base::MakeRefCounted( + kProviderId, SERVICE_WORKER_PROVIDER_FOR_WINDOW, + nullptr /* provider_request */, nullptr /* host_ptr_info */, dispatcher(), + nullptr /* loader_factory_getter */); + + // Set the controller to kInvalidServiceWorkerHandle. + OnSetControllerServiceWorker( + kDocumentMainThreadId, kProviderId, ServiceWorkerObjectInfo(), + should_notify_controllerchange, std::set()); + + // Check that it became null. + EXPECT_EQ(nullptr, provider_context->controller()); + EXPECT_TRUE(provider_client->is_set_controlled_called()); +} + TEST_F(ServiceWorkerDispatcherTest, OnPostMessage) { const int kProviderId = 10; diff --git a/content/child/service_worker/service_worker_message_filter.cc b/content/child/service_worker/service_worker_message_filter.cc index 546362769d72b..a6861e4324df9 100644 --- a/content/child/service_worker/service_worker_message_filter.cc +++ b/content/child/service_worker/service_worker_message_filter.cc @@ -60,6 +60,8 @@ void ServiceWorkerMessageFilter::OnStaleMessageReceived( IPC_BEGIN_MESSAGE_MAP(ServiceWorkerMessageFilter, msg) IPC_MESSAGE_HANDLER(ServiceWorkerMsg_SetVersionAttributes, OnStaleSetVersionAttributes) + IPC_MESSAGE_HANDLER(ServiceWorkerMsg_SetControllerServiceWorker, + OnStaleSetControllerServiceWorker) IPC_MESSAGE_HANDLER(ServiceWorkerMsg_MessageToDocument, OnStaleMessageToDocument) IPC_END_MESSAGE_MAP() @@ -80,6 +82,12 @@ void ServiceWorkerMessageFilter::OnStaleSetVersionAttributes( // SetVersionAttributes message doesn't increment it. } +void ServiceWorkerMessageFilter::OnStaleSetControllerServiceWorker( + const ServiceWorkerMsg_SetControllerServiceWorker_Params& params) { + SendServiceWorkerObjectDestroyed(thread_safe_sender(), + params.object_info.handle_id); +} + void ServiceWorkerMessageFilter::OnStaleMessageToDocument( const ServiceWorkerMsg_MessageToDocument_Params& params) { SendServiceWorkerObjectDestroyed(thread_safe_sender(), diff --git a/content/child/service_worker/service_worker_message_filter.h b/content/child/service_worker/service_worker_message_filter.h index 2c700e7797d95..91947da627fa8 100644 --- a/content/child/service_worker/service_worker_message_filter.h +++ b/content/child/service_worker/service_worker_message_filter.h @@ -13,6 +13,7 @@ #include "content/common/content_export.h" struct ServiceWorkerMsg_MessageToDocument_Params; +struct ServiceWorkerMsg_SetControllerServiceWorker_Params; namespace content { @@ -42,6 +43,8 @@ class CONTENT_EXPORT ServiceWorkerMessageFilter int registration_handle_id, int changed_mask, const ServiceWorkerVersionAttributes& attrs); + void OnStaleSetControllerServiceWorker( + const ServiceWorkerMsg_SetControllerServiceWorker_Params& params); void OnStaleMessageToDocument( const ServiceWorkerMsg_MessageToDocument_Params& params); diff --git a/content/child/service_worker/service_worker_provider_context.cc b/content/child/service_worker/service_worker_provider_context.cc index a7ae0472e8fca..61d785d9ef1fe 100644 --- a/content/child/service_worker/service_worker_provider_context.cc +++ b/content/child/service_worker/service_worker_provider_context.cc @@ -17,7 +17,6 @@ #include "content/child/service_worker/service_worker_dispatcher.h" #include "content/child/service_worker/service_worker_handle_reference.h" #include "content/child/service_worker/service_worker_subresource_loader.h" -#include "content/child/service_worker/web_service_worker_impl.h" #include "content/child/thread_safe_sender.h" #include "content/child/worker_thread_registry.h" #include "content/common/service_worker/service_worker_utils.h" @@ -55,10 +54,6 @@ struct ServiceWorkerProviderContext::ControlleeState { // Tracks feature usage for UseCounter. std::set used_features; - // Corresponds to a ServiceWorkerContainer. We notify it when - // ServiceWorkerContainer#controller should be changed. - base::WeakPtr web_service_worker_provider; - // Keeps ServiceWorkerWorkerClient pointers of dedicated or shared workers // which are associated with the ServiceWorkerProviderContext. // - If this ServiceWorkerProviderContext is for a Document, then @@ -163,6 +158,50 @@ void ServiceWorkerProviderContext::TakeRegistrationForServiceWorkerGlobalScope( attrs->active = state->active->info(); } +void ServiceWorkerProviderContext::SetController( + std::unique_ptr controller, + const std::set& used_features) { + DCHECK(main_thread_task_runner_->RunsTasksInCurrentSequence()); + ControlleeState* state = controllee_state_.get(); + DCHECK(state); + DCHECK(!state->controller || + state->controller->handle_id() != kInvalidServiceWorkerHandleId); + + state->controller = std::move(controller); + state->used_features = used_features; + + // Propagate the controller to workers related to this provider. + if (state->controller) { + for (const auto& worker : state->worker_clients) { + // This is a Mojo interface call to the (dedicated or shared) worker + // thread. + worker->SetControllerServiceWorker(state->controller->version_id()); + } + } + + // S13nServiceWorker + // Set up the URL loader factory for sending URL requests to the controller. + if (!ServiceWorkerUtils::IsServicificationEnabled() || !state->controller) { + state->controller_connector = nullptr; + state->subresource_loader_factory = nullptr; + return; + } + storage::mojom::BlobRegistryPtr blob_registry_ptr; + ChildThreadImpl::current()->GetConnector()->BindInterface( + mojom::kBrowserServiceName, mojo::MakeRequest(&blob_registry_ptr)); + auto blob_registry = base::MakeRefCounted< + base::RefCountedData>( + std::move(blob_registry_ptr)); + state->controller_connector = + base::MakeRefCounted( + container_host_.get()); + mojo::MakeStrongBinding( + base::MakeUnique( + state->controller_connector, state->default_loader_factory_getter, + state->controller->url().GetOrigin(), std::move(blob_registry)), + mojo::MakeRequest(&state->subresource_loader_factory)); +} + ServiceWorkerHandleReference* ServiceWorkerProviderContext::controller() { DCHECK(main_thread_task_runner_->RunsTasksInCurrentSequence()); DCHECK(controllee_state_); @@ -194,12 +233,6 @@ const std::set& ServiceWorkerProviderContext::used_features() const { return controllee_state_->used_features; } -void ServiceWorkerProviderContext::SetWebServiceWorkerProvider( - base::WeakPtr provider) { - DCHECK(controllee_state_); - controllee_state_->web_service_worker_provider = provider; -} - mojom::ServiceWorkerWorkerClientRequest ServiceWorkerProviderContext::CreateWorkerClientRequest() { DCHECK(main_thread_task_runner_->RunsTasksInCurrentSequence()); @@ -224,63 +257,6 @@ void ServiceWorkerProviderContext::UnregisterWorkerFetchContext( }); } -void ServiceWorkerProviderContext::SetController( - const ServiceWorkerObjectInfo& controller, - const std::vector& used_features, - bool should_notify_controllerchange) { - DCHECK(main_thread_task_runner_->RunsTasksInCurrentSequence()); - ControlleeState* state = controllee_state_.get(); - DCHECK(state); - DCHECK(!state->controller || - state->controller->handle_id() != kInvalidServiceWorkerHandleId); - ServiceWorkerDispatcher* dispatcher = - ServiceWorkerDispatcher::GetThreadSpecificInstance(); - - state->controller = dispatcher->Adopt(controller); - - // Propagate the controller to workers related to this provider. - if (state->controller) { - for (const auto& worker : state->worker_clients) { - // This is a Mojo interface call to the (dedicated or shared) worker - // thread. - worker->SetControllerServiceWorker(state->controller->version_id()); - } - } - for (blink::mojom::WebFeature feature : used_features) - state->used_features.insert(static_cast(feature)); - - // S13nServiceWorker - // Set up the URL loader factory for sending URL requests to the controller. - if (!ServiceWorkerUtils::IsServicificationEnabled() || !state->controller) { - state->controller_connector = nullptr; - state->subresource_loader_factory = nullptr; - } else { - storage::mojom::BlobRegistryPtr blob_registry_ptr; - ChildThreadImpl::current()->GetConnector()->BindInterface( - mojom::kBrowserServiceName, mojo::MakeRequest(&blob_registry_ptr)); - auto blob_registry = base::MakeRefCounted< - base::RefCountedData>(); - blob_registry->data = std::move(blob_registry_ptr); - state->controller_connector = - base::MakeRefCounted( - container_host_.get()); - mojo::MakeStrongBinding( - base::MakeUnique( - state->controller_connector, state->default_loader_factory_getter, - state->controller->url().GetOrigin(), std::move(blob_registry)), - mojo::MakeRequest(&state->subresource_loader_factory)); - } - - // The WebServiceWorkerProviderImpl might not exist yet because the document - // has not yet been created (as WebServiceWorkerImpl is created for a - // ServiceWorkerContainer). In that case, once it's created it will still get - // the controller from |this| via WebServiceWorkerProviderImpl::SetClient(). - if (state->web_service_worker_provider) { - state->web_service_worker_provider->SetController( - controller, state->used_features, should_notify_controllerchange); - } -} - void ServiceWorkerProviderContext::OnNetworkProviderDestroyed() { container_host_.reset(); if (controllee_state_ && controllee_state_->controller_connector) diff --git a/content/child/service_worker/service_worker_provider_context.h b/content/child/service_worker/service_worker_provider_context.h index 1e059ccd42b07..b09c55a8cd50c 100644 --- a/content/child/service_worker/service_worker_provider_context.h +++ b/content/child/service_worker/service_worker_provider_context.h @@ -11,14 +11,12 @@ #include "base/memory/ref_counted.h" #include "base/sequenced_task_runner_helpers.h" #include "content/child/service_worker/service_worker_dispatcher.h" -#include "content/child/service_worker/web_service_worker_provider_impl.h" #include "content/common/content_export.h" #include "content/common/service_worker/service_worker_container.mojom.h" #include "content/common/service_worker/service_worker_provider.mojom.h" #include "content/common/service_worker/service_worker_types.h" #include "content/public/child/child_url_loader_factory_getter.h" #include "mojo/public/cpp/bindings/associated_binding.h" -#include "third_party/WebKit/public/platform/modules/serviceworker/WebServiceWorkerProviderClient.h" #include "third_party/WebKit/public/platform/modules/serviceworker/service_worker_registration.mojom.h" namespace base { @@ -104,6 +102,8 @@ class CONTENT_EXPORT ServiceWorkerProviderContext // For service worker clients. The controller for // ServiceWorkerContainer#controller. + void SetController(std::unique_ptr controller, + const std::set& used_features); ServiceWorkerHandleReference* controller(); // S13nServiceWorker: @@ -115,14 +115,6 @@ class CONTENT_EXPORT ServiceWorkerProviderContext void CountFeature(uint32_t feature); const std::set& used_features() const; - // For service worker clients. Sets a weak pointer back to the - // WebServiceWorkerProviderImpl (which corresponds to ServiceWorkerContainer - // in JavaScript) which has a strong reference to |this|. This allows us to - // notify the WebServiceWorkerProviderImpl when - // ServiceWorkerContainer#controller should be changed. - void SetWebServiceWorkerProvider( - base::WeakPtr provider); - // For service worker clients. Creates a ServiceWorkerWorkerClientRequest // which can be used to bind with a WorkerFetchContextImpl in a (dedicated or // shared) worker thread and receive SetControllerServiceWorker() method call @@ -155,7 +147,6 @@ class CONTENT_EXPORT ServiceWorkerProviderContext friend class base::RefCountedThreadSafe; friend struct ServiceWorkerProviderContextDeleter; - struct ControlleeState; struct ControllerState; @@ -166,11 +157,6 @@ class CONTENT_EXPORT ServiceWorkerProviderContext // shared) worker, when the connection to the worker is disconnected. void UnregisterWorkerFetchContext(mojom::ServiceWorkerWorkerClient*); - // Implementation of mojom::ServiceWorkerContainer. - void SetController(const ServiceWorkerObjectInfo& controller, - const std::vector& used_features, - bool should_notify_controllerchange) override; - const ServiceWorkerProviderType provider_type_; const int provider_id_; scoped_refptr main_thread_task_runner_; diff --git a/content/child/service_worker/service_worker_provider_context_unittest.cc b/content/child/service_worker/service_worker_provider_context_unittest.cc index 59ffd94991288..0adc051ce3c9e 100644 --- a/content/child/service_worker/service_worker_provider_context_unittest.cc +++ b/content/child/service_worker/service_worker_provider_context_unittest.cc @@ -21,7 +21,6 @@ #include "ipc/ipc_test_sink.h" #include "mojo/public/cpp/bindings/associated_binding_set.h" #include "testing/gtest/include/gtest/gtest.h" -#include "third_party/WebKit/public/platform/modules/serviceworker/WebServiceWorkerProviderClient.h" #include "third_party/WebKit/public/platform/modules/serviceworker/service_worker_registration.mojom.h" namespace content { @@ -47,41 +46,6 @@ class MockServiceWorkerRegistrationObjectHost bindings_; }; -class MockWebServiceWorkerProviderClientImpl - : public blink::WebServiceWorkerProviderClient { - public: - MockWebServiceWorkerProviderClientImpl() {} - - ~MockWebServiceWorkerProviderClientImpl() override {} - - void SetController(std::unique_ptr handle, - bool should_notify_controller_change) override { - was_set_controller_called_ = true; - } - - void DispatchMessageEvent( - std::unique_ptr handle, - const blink::WebString& message, - blink::WebVector ports) override { - was_dispatch_message_event_called_ = true; - } - - void CountFeature(uint32_t feature) override { - used_features_.insert(feature); - } - - bool was_set_controller_called() const { return was_set_controller_called_; } - - bool was_dispatch_message_event_called() const { - return was_dispatch_message_event_called_; - } - - private: - bool was_set_controller_called_ = false; - bool was_dispatch_message_event_called_ = false; - std::set used_features_; -}; - class ServiceWorkerTestSender : public ThreadSafeSender { public: explicit ServiceWorkerTestSender(IPC::TestSink* ipc_sink) @@ -188,106 +152,4 @@ TEST_F(ServiceWorkerProviderContextTest, CreateForController) { EXPECT_EQ(0, remote_registration_object_host().GetBindingCount()); } -TEST_F(ServiceWorkerProviderContextTest, SetController) { - const int kProviderId = 10; - - // Assume that these objects are passed from the browser process and own - // references to browser-side registration/worker representations. - blink::mojom::ServiceWorkerRegistrationObjectInfoPtr info; - ServiceWorkerVersionAttributes attrs; - CreateObjectInfoAndVersionAttributes(&info, &attrs); - - { - // (1) In the case there is no WebSWProviderClient but SWProviderContext for - // the provider, the passed reference should be adopted and owned by the - // provider context. - mojom::ServiceWorkerContainerAssociatedPtr container_ptr; - mojom::ServiceWorkerContainerAssociatedRequest container_request = - mojo::MakeIsolatedRequest(&container_ptr); - auto provider_context = base::MakeRefCounted( - kProviderId, SERVICE_WORKER_PROVIDER_FOR_WINDOW, - std::move(container_request), nullptr /* host_ptr_info */, dispatcher(), - nullptr /* loader_factory_getter */); - - ipc_sink()->ClearMessages(); - container_ptr->SetController(attrs.active, - std::vector(), true); - base::RunLoop().RunUntilIdle(); - EXPECT_EQ(0UL, ipc_sink()->message_count()); - - // Destruction of the provider context should release references to the - // the controller. - provider_context = nullptr; - ASSERT_EQ(1UL, ipc_sink()->message_count()); - EXPECT_EQ(ServiceWorkerHostMsg_DecrementServiceWorkerRefCount::ID, - ipc_sink()->GetMessageAt(0)->type()); - ipc_sink()->ClearMessages(); - } - - { - // (2) In the case there are both SWProviderContext and SWProviderClient for - // the provider, the passed reference should be adopted and owned by the - // provider context. In addition, the new reference should be created for - // the provider client and immediately released due to limitation of the - // mock implementation. - mojom::ServiceWorkerContainerHostAssociatedPtrInfo host_ptr_info; - mojom::ServiceWorkerContainerHostAssociatedRequest host_request = - mojo::MakeRequest(&host_ptr_info); - - mojom::ServiceWorkerContainerAssociatedPtr container_ptr; - mojom::ServiceWorkerContainerAssociatedRequest container_request = - mojo::MakeIsolatedRequest(&container_ptr); - auto provider_context = base::MakeRefCounted( - kProviderId, SERVICE_WORKER_PROVIDER_FOR_WINDOW, - std::move(container_request), std::move(host_ptr_info), dispatcher(), - nullptr /* loader_factory_getter */); - auto provider_impl = base::MakeUnique( - thread_safe_sender(), provider_context.get()); - auto client = base::MakeUnique(); - provider_impl->SetClient(client.get()); - ASSERT_FALSE(client->was_set_controller_called()); - - ipc_sink()->ClearMessages(); - container_ptr->SetController(attrs.active, - std::vector(), true); - base::RunLoop().RunUntilIdle(); - - EXPECT_TRUE(client->was_set_controller_called()); - ASSERT_EQ(2UL, ipc_sink()->message_count()); - EXPECT_EQ(ServiceWorkerHostMsg_IncrementServiceWorkerRefCount::ID, - ipc_sink()->GetMessageAt(0)->type()); - EXPECT_EQ(ServiceWorkerHostMsg_DecrementServiceWorkerRefCount::ID, - ipc_sink()->GetMessageAt(1)->type()); - } -} - -// Test that clearing the controller by sending a kInvalidServiceWorkerHandle -// results in the provider context having a null controller. -TEST_F(ServiceWorkerProviderContextTest, SetController_Null) { - const int kProviderId = 10; - - mojom::ServiceWorkerContainerHostAssociatedPtrInfo host_ptr_info; - mojom::ServiceWorkerContainerHostAssociatedRequest host_request = - mojo::MakeRequest(&host_ptr_info); - - mojom::ServiceWorkerContainerAssociatedPtr container_ptr; - mojom::ServiceWorkerContainerAssociatedRequest container_request = - mojo::MakeIsolatedRequest(&container_ptr); - auto provider_context = base::MakeRefCounted( - kProviderId, SERVICE_WORKER_PROVIDER_FOR_WINDOW, - std::move(container_request), std::move(host_ptr_info), dispatcher(), - nullptr /* loader_factory_getter */); - auto provider_impl = base::MakeUnique( - thread_safe_sender(), provider_context.get()); - auto client = base::MakeUnique(); - provider_impl->SetClient(client.get()); - - container_ptr->SetController(ServiceWorkerObjectInfo(), - std::vector(), true); - base::RunLoop().RunUntilIdle(); - - EXPECT_EQ(nullptr, provider_context->controller()); - EXPECT_TRUE(client->was_set_controller_called()); -} - } // namespace content diff --git a/content/child/service_worker/web_service_worker_provider_impl.cc b/content/child/service_worker/web_service_worker_provider_impl.cc index df0513dbccbe8..ead184d88d01a 100644 --- a/content/child/service_worker/web_service_worker_provider_impl.cc +++ b/content/child/service_worker/web_service_worker_provider_impl.cc @@ -39,20 +39,8 @@ WebServiceWorkerProviderImpl::WebServiceWorkerProviderImpl( context_(context), weak_factory_(this) { DCHECK(context_); - switch (context_->provider_type()) { - case SERVICE_WORKER_PROVIDER_FOR_WINDOW: - DCHECK(context_->container_host()); - context_->SetWebServiceWorkerProvider(weak_factory_.GetWeakPtr()); - break; - case SERVICE_WORKER_PROVIDER_FOR_CONTROLLER: - // Do nothing. - break; - case SERVICE_WORKER_PROVIDER_FOR_WORKER: - case SERVICE_WORKER_PROVIDER_FOR_SHARED_WORKER: - case SERVICE_WORKER_PROVIDER_UNKNOWN: - NOTREACHED() << "Unimplemented type: " << context_->provider_type(); - break; - } + DCHECK(context_->provider_type() != SERVICE_WORKER_PROVIDER_FOR_WINDOW || + context_->container_host()); } WebServiceWorkerProviderImpl::~WebServiceWorkerProviderImpl() { @@ -76,8 +64,17 @@ void WebServiceWorkerProviderImpl::SetClient( if (!context_->controller()) return; - SetController(context_->controller()->info(), context_->used_features(), - false /* notify_controllerchange */); + scoped_refptr controller = + GetDispatcher()->GetOrCreateServiceWorker( + ServiceWorkerHandleReference::Create(context_->controller()->info(), + thread_safe_sender_.get())); + + // Sync the controllee's use counter with |context_|'s, which keeps + // track of the controller's use counter. + for (uint32_t feature : context_->used_features()) + client->CountFeature(feature); + client->SetController(WebServiceWorkerImpl::CreateHandle(controller), + false /* shouldNotifyControllerChange */); } void WebServiceWorkerProviderImpl::RegisterServiceWorker( @@ -195,26 +192,6 @@ bool WebServiceWorkerProviderImpl::ValidateScopeAndScriptURL( return !has_error; } -void WebServiceWorkerProviderImpl::SetController( - const ServiceWorkerObjectInfo& info, - const std::set& features, - bool should_notify_controller_change) { - blink::WebServiceWorkerProviderClient* provider_client = - GetDispatcher()->GetProviderClient(context_->provider_id()); - // The caller should only call this after SetClient() has been called. - DCHECK(provider_client); - scoped_refptr controller = - GetDispatcher()->GetOrCreateServiceWorker( - ServiceWorkerHandleReference::Create(info, - thread_safe_sender_.get())); - - for (uint32_t feature : features) - provider_client->CountFeature(feature); - - provider_client->SetController(WebServiceWorkerImpl::CreateHandle(controller), - should_notify_controller_change); -} - int WebServiceWorkerProviderImpl::provider_id() const { return context_->provider_id(); } diff --git a/content/child/service_worker/web_service_worker_provider_impl.h b/content/child/service_worker/web_service_worker_provider_impl.h index f461c29c779d6..f55eedd7bf74a 100644 --- a/content/child/service_worker/web_service_worker_provider_impl.h +++ b/content/child/service_worker/web_service_worker_provider_impl.h @@ -11,7 +11,6 @@ #include "base/macros.h" #include "base/memory/ref_counted.h" #include "base/memory/weak_ptr.h" -#include "content/common/service_worker/service_worker_types.h" #include "third_party/WebKit/public/platform/modules/serviceworker/WebServiceWorkerProvider.h" #include "third_party/WebKit/public/platform/modules/serviceworker/service_worker_error_type.mojom.h" #include "third_party/WebKit/public/platform/modules/serviceworker/service_worker_registration.mojom.h" @@ -31,8 +30,7 @@ struct ServiceWorkerVersionAttributes; // This class corresponds to one ServiceWorkerContainer interface in // JS context (i.e. navigator.serviceWorker). -class CONTENT_EXPORT WebServiceWorkerProviderImpl - : public blink::WebServiceWorkerProvider { +class WebServiceWorkerProviderImpl : public blink::WebServiceWorkerProvider { public: WebServiceWorkerProviderImpl(ThreadSafeSender* thread_safe_sender, ServiceWorkerProviderContext* context); @@ -57,12 +55,6 @@ class CONTENT_EXPORT WebServiceWorkerProviderImpl const blink::WebURL& script_url, blink::WebString* error_message) override; - // Sets the ServiceWorkerContainer#controller for this provider. It's not - // used when this WebServiceWorkerProvider is for a service worker context. - void SetController(const ServiceWorkerObjectInfo& info, - const std::set& features, - bool should_notify_controller_change); - int provider_id() const; private: diff --git a/content/common/service_worker/service_worker_container.mojom b/content/common/service_worker/service_worker_container.mojom index d4796d2ec4848..a998621ffec71 100644 --- a/content/common/service_worker/service_worker_container.mojom +++ b/content/common/service_worker/service_worker_container.mojom @@ -8,7 +8,6 @@ import "content/common/service_worker/controller_service_worker.mojom"; import "content/common/service_worker/service_worker_types.mojom"; import "third_party/WebKit/public/platform/modules/serviceworker/service_worker_error_type.mojom"; import "third_party/WebKit/public/platform/modules/serviceworker/service_worker_registration.mojom"; -import "third_party/WebKit/public/platform/web_feature.mojom"; import "url/mojo/url.mojom"; // mojom::ServiceWorkerContainerHost is a browser-side interface. The renderer @@ -80,17 +79,8 @@ interface ServiceWorkerContainerHost { // that can touch these objects should be a ServiceWorkerContainer, so it’s OK // to use this name. interface ServiceWorkerContainer { - // Corresponds to setting ServiceWorkerContainer#controller. - // If |controller| is invalid (its |handle_id| is invalid), then - // ServiceWorkerContainer#controller is cleared. - // If |controller| is valid, |used_features| is the set of features the - // controller has used, for UseCounter purposes. - // If |should_notify_controllerchange| is true, dispatch a 'controllerchange' - // event. - SetController(ServiceWorkerObjectInfo controller, - array used_features, - bool should_notify_controllerchange); // TODO(xiaofeng.zhang): implement them. + // SetController(); // ServiceWorkerStateChanged(); // ServiceWorkerRegistrationUpdateFound(); // PostMessage(); diff --git a/content/common/service_worker/service_worker_messages.h b/content/common/service_worker/service_worker_messages.h index f4a2a7aff63bc..1cff06661794f 100644 --- a/content/common/service_worker/service_worker_messages.h +++ b/content/common/service_worker/service_worker_messages.h @@ -140,6 +140,17 @@ IPC_STRUCT_TRAITS_BEGIN(content::PushEventPayload) IPC_STRUCT_TRAITS_MEMBER(is_null) IPC_STRUCT_TRAITS_END() +IPC_STRUCT_BEGIN(ServiceWorkerMsg_SetControllerServiceWorker_Params) + IPC_STRUCT_MEMBER(int, thread_id) + IPC_STRUCT_MEMBER(int, provider_id) + IPC_STRUCT_MEMBER(content::ServiceWorkerObjectInfo, object_info) + IPC_STRUCT_MEMBER(bool, should_notify_controllerchange) + + // |used_features| is the set of features that the worker has used. + // The values must be from blink::UseCounter::Feature enum. + IPC_STRUCT_MEMBER(std::set, used_features) +IPC_STRUCT_END() + //--------------------------------------------------------------------------- // Messages sent from the child process to the browser. @@ -311,6 +322,11 @@ IPC_MESSAGE_CONTROL2(ServiceWorkerMsg_UpdateFound, int /* thread_id */, int /* registration_handle_id */) +// Tells the child process to set the controller ServiceWorker for the given +// provider. +IPC_MESSAGE_CONTROL1(ServiceWorkerMsg_SetControllerServiceWorker, + ServiceWorkerMsg_SetControllerServiceWorker_Params) + IPC_MESSAGE_CONTROL2(ServiceWorkerMsg_DidEnableNavigationPreload, int /* thread_id */, int /* request_id */) diff --git a/content/common/service_worker/service_worker_types.mojom b/content/common/service_worker/service_worker_types.mojom index ff37088b15651..ae8c0bd40f0df 100644 --- a/content/common/service_worker/service_worker_types.mojom +++ b/content/common/service_worker/service_worker_types.mojom @@ -24,7 +24,3 @@ enum ServiceWorkerProviderType { // Defined in service_worker_types.h. [Native] struct ServiceWorkerVersionAttributes; - -// Defined in service_worker_types.h. -[Native] -struct ServiceWorkerObjectInfo; diff --git a/content/common/service_worker/service_worker_types.typemap b/content/common/service_worker/service_worker_types.typemap index ba00b3467e43c..ed6313591dead 100644 --- a/content/common/service_worker/service_worker_types.typemap +++ b/content/common/service_worker/service_worker_types.typemap @@ -17,5 +17,4 @@ sources = [ type_mappings = [ "content.mojom.ServiceWorkerProviderType=::content::ServiceWorkerProviderType", "content.mojom.ServiceWorkerVersionAttributes=::content::ServiceWorkerVersionAttributes", - "content.mojom.ServiceWorkerObjectInfo=::content::ServiceWorkerObjectInfo", ]