Commit 9c02c1b0 authored by Matt Falkenhagen's avatar Matt Falkenhagen Committed by Commit Bot

Try to deflake ServiceWorkerProviderContextTest.SetControllerServiceWorker.

Replace vague RunUntilIdle() calls with FlushForTesting() and explicit
events.

Bug: 862294
Change-Id: I76e6cf9501b5d480960f431316e385bd7bf9941d
Reviewed-on: https://chromium-review.googlesource.com/1164737
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: default avatarKenichi Ishibashi <bashi@chromium.org>
Reviewed-by: default avatarKinuko Yasuda <kinuko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581387}
parent f6fe8d51
......@@ -572,6 +572,8 @@ target(link_target_type, "renderer") {
"service_worker/service_worker_network_provider.h",
"service_worker/service_worker_provider_context.cc",
"service_worker/service_worker_provider_context.h",
"service_worker/service_worker_provider_state_for_client.cc",
"service_worker/service_worker_provider_state_for_client.h",
"service_worker/service_worker_subresource_loader.cc",
"service_worker/service_worker_subresource_loader.h",
"service_worker/service_worker_timeout_timer.cc",
......
......@@ -56,80 +56,6 @@ void CreateSubresourceLoaderFactoryForProviderContext(
} // namespace
// Holds state for service worker clients.
struct ServiceWorkerProviderContext::ProviderStateForClient {
explicit ProviderStateForClient(
scoped_refptr<network::SharedURLLoaderFactory> fallback_loader_factory)
: fallback_loader_factory(std::move(fallback_loader_factory)) {}
~ProviderStateForClient() = default;
// |controller| will be set by SetController() and taken by TakeController().
blink::mojom::ServiceWorkerObjectInfoPtr controller;
// Keeps version id of the current controller service worker object.
int64_t controller_version_id = blink::mojom::kInvalidServiceWorkerVersionId;
// S13nServiceWorker:
// Used to intercept requests from the controllee and dispatch them
// as events to the controller ServiceWorker.
network::mojom::URLLoaderFactoryPtr subresource_loader_factory;
// S13nServiceWorker:
// Used when we create |subresource_loader_factory|.
scoped_refptr<network::SharedURLLoaderFactory> fallback_loader_factory;
// S13nServiceWorker:
// The Client#id value of the client.
std::string client_id;
blink::mojom::ControllerServiceWorkerMode controller_mode =
blink::mojom::ControllerServiceWorkerMode::kNoController;
// Tracks feature usage for UseCounter.
std::set<blink::mojom::WebFeature> used_features;
// Corresponds to a ServiceWorkerContainer. We notify it when
// ServiceWorkerContainer#controller should be changed.
base::WeakPtr<WebServiceWorkerProviderImpl> 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
// |worker_clients| contains all its dedicated workers.
// - If this ServiceWorkerProviderContext is for a SharedWorker (technically
// speaking, for its shadow page), then |worker_clients| has one element:
// the shared worker.
std::vector<mojom::ServiceWorkerWorkerClientPtr> worker_clients;
// For adding new ServiceWorkerWorkerClients.
mojo::BindingSet<mojom::ServiceWorkerWorkerClientRegistry>
worker_client_registry_bindings;
// S13nServiceWorker
// Used in |subresource_loader_factory| to get the connection to the
// controller service worker.
//
// |controller_endpoint| is a Mojo pipe to the controller service worker,
// and is to be passed to (i.e. taken by) a subresource loader factory when
// GetSubresourceLoaderFactory() is called for the first time when a valid
// controller exists.
//
// |controller_connector| is a Mojo pipe to the
// ControllerServiceWorkerConnector that is attached to the newly created
// subresource loader factory and lives on a background thread. This is
// populated when GetSubresourceLoader() creates the subresource loader
// factory and takes |controller_endpoint|.
mojom::ControllerServiceWorkerPtrInfo controller_endpoint;
mojom::ControllerServiceWorkerConnectorPtr controller_connector;
// For service worker clients. Map from registration id to JavaScript
// ServiceWorkerRegistration object.
std::map<int64_t, WebServiceWorkerRegistrationImpl*> registrations_;
// For service worker clients. Map from version id to JavaScript ServiceWorker
// object.
std::map<int64_t, WebServiceWorkerImpl*> workers_;
};
// For service worker clients.
ServiceWorkerProviderContext::ServiceWorkerProviderContext(
int provider_id,
......@@ -144,7 +70,7 @@ ServiceWorkerProviderContext::ServiceWorkerProviderContext(
binding_(this, std::move(request)),
weak_factory_(this) {
container_host_.Bind(std::move(host_ptr_info));
state_for_client_ = std::make_unique<ProviderStateForClient>(
state_for_client_ = std::make_unique<ServiceWorkerProviderStateForClient>(
std::move(fallback_loader_factory));
// Set up the URL loader factory for sending subresource requests to
......@@ -330,7 +256,7 @@ void ServiceWorkerProviderContext::PingContainerHost(
void ServiceWorkerProviderContext::DispatchNetworkQuiet() {
DCHECK(main_thread_task_runner_->RunsTasksInCurrentSequence());
ProviderStateForClient* state = state_for_client_.get();
ServiceWorkerProviderStateForClient* state = state_for_client_.get();
DCHECK(state);
// In non-S13nSW, this hint isn't needed because the browser process
......@@ -362,7 +288,7 @@ void ServiceWorkerProviderContext::SetController(
const std::vector<blink::mojom::WebFeature>& used_features,
bool should_notify_controllerchange) {
DCHECK(main_thread_task_runner_->RunsTasksInCurrentSequence());
ProviderStateForClient* state = state_for_client_.get();
ServiceWorkerProviderStateForClient* state = state_for_client_.get();
DCHECK(state);
state->controller = std::move(controller_info->object_info);
......@@ -440,7 +366,7 @@ void ServiceWorkerProviderContext::PostMessageToClient(
blink::TransferableMessage message) {
DCHECK(main_thread_task_runner_->RunsTasksInCurrentSequence());
ProviderStateForClient* state = state_for_client_.get();
ServiceWorkerProviderStateForClient* state = state_for_client_.get();
DCHECK(state);
if (state->web_service_worker_provider) {
state->web_service_worker_provider->PostMessageToClient(std::move(source),
......@@ -495,7 +421,7 @@ void ServiceWorkerProviderContext::CountFeature(
blink::mojom::WebFeature feature) {
DCHECK(main_thread_task_runner_->RunsTasksInCurrentSequence());
DCHECK(state_for_client_);
ProviderStateForClient* state = state_for_client_.get();
ServiceWorkerProviderStateForClient* state = state_for_client_.get();
// ServiceWorkerProviderContext keeps track of features in order to propagate
// it to WebServiceWorkerProviderClient, which actually records the
......
......@@ -6,6 +6,9 @@
#define CONTENT_RENDERER_SERVICE_WORKER_SERVICE_WORKER_PROVIDER_CONTEXT_H_
#include <memory>
#include <set>
#include <string>
#include <vector>
#include "base/macros.h"
#include "base/memory/ref_counted.h"
......@@ -15,6 +18,7 @@
#include "content/common/service_worker/controller_service_worker.mojom.h"
#include "content/common/service_worker/service_worker_container.mojom.h"
#include "content/common/service_worker/service_worker_provider.mojom.h"
#include "content/renderer/service_worker/service_worker_provider_state_for_client.h"
#include "content/renderer/service_worker/web_service_worker_provider_impl.h"
#include "mojo/public/cpp/bindings/associated_binding.h"
#include "services/network/public/cpp/shared_url_loader_factory.h"
......@@ -205,7 +209,6 @@ class CONTENT_EXPORT ServiceWorkerProviderContext
friend class WebServiceWorkerImpl;
friend class WebServiceWorkerRegistrationImpl;
friend struct ServiceWorkerProviderContextDeleter;
struct ProviderStateForClient;
~ServiceWorkerProviderContext() override;
void DestructOnMainThread() const;
......@@ -264,7 +267,7 @@ class CONTENT_EXPORT ServiceWorkerProviderContext
mojom::ServiceWorkerContainerHostAssociatedPtr container_host_;
// State for service worker clients.
std::unique_ptr<ProviderStateForClient> state_for_client_;
std::unique_ptr<ServiceWorkerProviderStateForClient> state_for_client_;
// NOTE: Add new members to |state_for_client_| if they are relevant only for
// service worker clients. Not here!
......
......@@ -5,6 +5,10 @@
#include "content/renderer/service_worker/service_worker_provider_context.h"
#include <memory>
#include <set>
#include <string>
#include <utility>
#include <vector>
#include "base/macros.h"
#include "base/message_loop/message_loop.h"
......@@ -18,7 +22,6 @@
#include "content/public/common/content_features.h"
#include "content/public/common/resource_type.h"
#include "content/renderer/service_worker/controller_service_worker_connector.h"
#include "content/renderer/service_worker/service_worker_provider_context.h"
#include "content/renderer/service_worker/web_service_worker_impl.h"
#include "content/renderer/service_worker/web_service_worker_registration_impl.h"
#include "mojo/public/cpp/bindings/associated_binding_set.h"
......@@ -44,7 +47,11 @@ class MockServiceWorkerObjectHost
: public blink::mojom::ServiceWorkerObjectHost {
public:
explicit MockServiceWorkerObjectHost(int64_t version_id)
: version_id_(version_id) {}
: version_id_(version_id) {
bindings_.set_connection_error_handler(
base::BindRepeating(&MockServiceWorkerObjectHost::OnConnectionError,
base::Unretained(this)));
}
~MockServiceWorkerObjectHost() override = default;
blink::mojom::ServiceWorkerObjectInfoPtr CreateObjectInfo() {
......@@ -55,6 +62,16 @@ class MockServiceWorkerObjectHost
return info;
}
void OnConnectionError() {
if (error_callback_)
std::move(error_callback_).Run();
}
void RunOnConnectionError(base::OnceClosure error_callback) {
DCHECK(!error_callback_);
error_callback_ = std::move(error_callback);
}
int GetBindingCount() const { return bindings_.size(); }
private:
......@@ -70,6 +87,7 @@ class MockServiceWorkerObjectHost
const int64_t version_id_;
mojo::AssociatedBindingSet<blink::mojom::ServiceWorkerObjectHost> bindings_;
blink::mojom::ServiceWorkerObjectAssociatedPtr remote_object_;
base::OnceClosure error_callback_;
};
class MockServiceWorkerRegistrationObjectHost
......@@ -353,6 +371,11 @@ class ServiceWorkerProviderContextTest : public testing::Test {
return provider_context->ContainsServiceWorkerObjectForTesting(version_id);
}
void FlushControllerConnector(
ServiceWorkerProviderContext* provider_context) {
provider_context->state_for_client_->controller_connector.FlushForTesting();
}
protected:
base::test::ScopedTaskEnvironment task_environment;
......@@ -482,14 +505,27 @@ TEST_F(ServiceWorkerProviderContextTest, SetControllerServiceWorker) {
EnableS13nServiceWorker();
const int kProviderId = 10;
// Make the ServiceWorkerContainerHost implementation and
// ServiceWorkerContainer request.
mojom::ServiceWorkerContainerHostAssociatedPtr host_ptr;
FakeServiceWorkerContainerHost host(
mojo::MakeRequestAssociatedWithDedicatedPipe(&host_ptr));
mojom::ServiceWorkerContainerAssociatedPtr container_ptr;
mojom::ServiceWorkerContainerAssociatedRequest container_request =
mojo::MakeRequestAssociatedWithDedicatedPipe(&container_ptr);
// (1) Test if setting the controller via the CTOR works.
LOG(ERROR) << "1 test ctor";
// Make the object host for .controller.
auto object_host1 =
std::make_unique<MockServiceWorkerObjectHost>(200 /* version_id */);
ASSERT_EQ(0, object_host1->GetBindingCount());
EXPECT_EQ(0, object_host1->GetBindingCount());
blink::mojom::ServiceWorkerObjectInfoPtr object_info1 =
object_host1->CreateObjectInfo();
EXPECT_EQ(1, object_host1->GetBindingCount());
// Make the ControllerServiceWorkerInfo.
FakeControllerServiceWorker fake_controller1;
auto controller_info1 = mojom::ControllerServiceWorkerInfo::New();
mojom::ControllerServiceWorkerPtr controller_ptr1;
......@@ -499,23 +535,15 @@ TEST_F(ServiceWorkerProviderContextTest, SetControllerServiceWorker) {
controller_info1->object_info = std::move(object_info1);
controller_info1->endpoint = controller_ptr1.PassInterface();
mojom::ServiceWorkerContainerHostAssociatedPtr host_ptr;
FakeServiceWorkerContainerHost host(
mojo::MakeRequestAssociatedWithDedicatedPipe(&host_ptr));
mojom::ServiceWorkerContainerAssociatedPtr container_ptr;
mojom::ServiceWorkerContainerAssociatedRequest container_request =
mojo::MakeRequestAssociatedWithDedicatedPipe(&container_ptr);
// Make the ServiceWorkerProviderContext, pasing it the controller, container,
// and container host.
auto provider_context = base::MakeRefCounted<ServiceWorkerProviderContext>(
kProviderId, blink::mojom::ServiceWorkerProviderType::kForWindow,
std::move(container_request), host_ptr.PassInterface(),
std::move(controller_info1), loader_factory_);
LOG(ERROR) << "1 RunUntilIdle after ctor";
base::RunLoop().RunUntilIdle();
LOG(ERROR) << "1 RunUntilIdle after ctor finished";
// Subresource loader factory must be available.
auto* subresource_loader_factory1 =
// The subresource loader factory must be available.
network::mojom::URLLoaderFactory* subresource_loader_factory1 =
provider_context->GetSubresourceLoaderFactory();
ASSERT_NE(nullptr, subresource_loader_factory1);
......@@ -533,6 +561,8 @@ TEST_F(ServiceWorkerProviderContextTest, SetControllerServiceWorker) {
// (2) Test if resetting the controller to a new one via SetController
// works.
LOG(ERROR) << "2 reset controller to new one";
// Setup the new controller.
auto object_host2 =
std::make_unique<MockServiceWorkerObjectHost>(201 /* version_id */);
ASSERT_EQ(0, object_host2->GetBindingCount());
......@@ -547,23 +577,35 @@ TEST_F(ServiceWorkerProviderContextTest, SetControllerServiceWorker) {
blink::mojom::ControllerServiceWorkerMode::kControlled;
controller_info2->object_info = std::move(object_info2);
controller_info2->endpoint = controller_ptr2.PassInterface();
// Resetting the controller will trigger many things happening, including the
// object binding being broken.
base::RunLoop drop_binding_loop;
object_host1->RunOnConnectionError(drop_binding_loop.QuitClosure());
container_ptr->SetController(std::move(controller_info2),
std::vector<blink::mojom::WebFeature>(), true);
// The controller is reset. References to the old controller must be
// released.
LOG(ERROR) << "2 RunUntilIdle after SetController";
base::RunLoop().RunUntilIdle();
LOG(ERROR) << "2 RunUntilIdle after SetController finished";
LOG(ERROR) << "2 FlushForTesting()";
container_ptr.FlushForTesting();
LOG(ERROR) << "2 FlushForTesting() finished";
LOG(ERROR) << "2 drop_binding_loop.Run()";
drop_binding_loop.Run();
LOG(ERROR) << "2 drop_binding_loop.Run() finished";
EXPECT_EQ(0, object_host1->GetBindingCount());
// Subresource loader factory must be available, and should be the same
// one as we got before.
auto* subresource_loader_factory2 =
network::mojom::URLLoaderFactory* subresource_loader_factory2 =
provider_context->GetSubresourceLoaderFactory();
ASSERT_NE(nullptr, subresource_loader_factory2);
EXPECT_EQ(subresource_loader_factory1, subresource_loader_factory2);
// The SetController() call results in another Mojo call to
// ControllerServiceWorkerConnector.UpdateController(). Flush that interface
// pointer to ensure the message was received.
LOG(ERROR) << "2 FlushControllerConnector()";
FlushControllerConnector(provider_context.get());
LOG(ERROR) << "2 FlushControllerConnector() finished";
// Performing a request should reach the new controller.
const GURL kURL2("https://www.example.com/foo2.png");
base::RunLoop loop2;
......@@ -579,14 +621,19 @@ TEST_F(ServiceWorkerProviderContextTest, SetControllerServiceWorker) {
// (3) Test if resetting the controller to nullptr works.
LOG(ERROR) << "3 reset controller to null";
base::RunLoop drop_binding_loop2;
object_host2->RunOnConnectionError(drop_binding_loop2.QuitClosure());
container_ptr->SetController(mojom::ControllerServiceWorkerInfo::New(),
std::vector<blink::mojom::WebFeature>(), true);
// The controller is reset. References to the old controller must be
// released.
LOG(ERROR) << "3 RunUntilIdle()";
base::RunLoop().RunUntilIdle();
LOG(ERROR) << "3 RunUntilIdle() finished";
LOG(ERROR) << "3 FlushForTesting()";
container_ptr.FlushForTesting();
LOG(ERROR) << "3 FlushForTesting() finished";
LOG(ERROR) << "3 drop_binding_loop2.Run()";
drop_binding_loop2.Run();
LOG(ERROR) << "3 drop_binding_loop2.Run() finished";
EXPECT_EQ(0, object_host2->GetBindingCount());
// Subresource loader factory must not be available.
......@@ -628,15 +675,22 @@ TEST_F(ServiceWorkerProviderContextTest, SetControllerServiceWorker) {
controller_info4->endpoint = controller_ptr4.PassInterface();
container_ptr->SetController(std::move(controller_info4),
std::vector<blink::mojom::WebFeature>(), true);
LOG(ERROR) << "4 RunUntilIdle()";
base::RunLoop().RunUntilIdle();
LOG(ERROR) << "4 RunUntilIdle() finished";
LOG(ERROR) << "4 FlushForTesting()";
container_ptr.FlushForTesting();
LOG(ERROR) << "4 FlushForTesting() finished";
// Subresource loader factory must be available.
auto* subresource_loader_factory4 =
provider_context->GetSubresourceLoaderFactory();
ASSERT_NE(nullptr, subresource_loader_factory4);
// The SetController() call results in another Mojo call to
// ControllerServiceWorkerConnector.UpdateController(). Flush that interface
// pointer to ensure the message was received.
LOG(ERROR) << "4 FlushControllerConnector()";
FlushControllerConnector(provider_context.get());
LOG(ERROR) << "4 FlushControllerConnector() finished";
// Performing a request should reach the new controller.
const GURL kURL4("https://www.example.com/foo4.png");
base::RunLoop loop4;
......
// Copyright 2018 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.
#include "content/renderer/service_worker/service_worker_provider_state_for_client.h"
#include <utility>
namespace content {
ServiceWorkerProviderStateForClient::ServiceWorkerProviderStateForClient(
scoped_refptr<network::SharedURLLoaderFactory> fallback_loader_factory)
: fallback_loader_factory(std::move(fallback_loader_factory)) {}
ServiceWorkerProviderStateForClient::~ServiceWorkerProviderStateForClient() =
default;
} // namespace content
// Copyright 2018 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.
#ifndef CONTENT_RENDERER_SERVICE_WORKER_SERVICE_WORKER_PROVIDER_STATE_FOR_CLIENT_H_
#define CONTENT_RENDERER_SERVICE_WORKER_SERVICE_WORKER_PROVIDER_STATE_FOR_CLIENT_H_
#include <stdint.h>
#include <map>
#include <set>
#include <string>
#include <vector>
#include "base/memory/ref_counted.h"
#include "base/memory/weak_ptr.h"
#include "content/common/content_export.h"
#include "content/common/service_worker/controller_service_worker.mojom.h"
#include "content/common/service_worker/service_worker_container.mojom.h"
#include "content/common/service_worker/service_worker_provider.mojom.h"
#include "content/renderer/service_worker/web_service_worker_impl.h"
#include "content/renderer/service_worker/web_service_worker_provider_impl.h"
#include "content/renderer/service_worker/web_service_worker_registration_impl.h"
#include "mojo/public/cpp/bindings/binding_set.h"
#include "services/network/public/cpp/shared_url_loader_factory.h"
#include "services/network/public/mojom/url_loader_factory.mojom.h"
#include "third_party/blink/public/platform/web_feature.mojom.h"
namespace content {
// Holds state for ServiceWorkerProviderContext instances for service worker
// clients.
struct ServiceWorkerProviderStateForClient {
explicit ServiceWorkerProviderStateForClient(
scoped_refptr<network::SharedURLLoaderFactory> fallback_loader_factory);
~ServiceWorkerProviderStateForClient();
// |controller| will be set by SetController() and taken by TakeController().
blink::mojom::ServiceWorkerObjectInfoPtr controller;
// Keeps version id of the current controller service worker object.
int64_t controller_version_id = blink::mojom::kInvalidServiceWorkerVersionId;
// S13nServiceWorker:
// Used to intercept requests from the controllee and dispatch them
// as events to the controller ServiceWorker.
network::mojom::URLLoaderFactoryPtr subresource_loader_factory;
// S13nServiceWorker:
// Used when we create |subresource_loader_factory|.
scoped_refptr<network::SharedURLLoaderFactory> fallback_loader_factory;
// S13nServiceWorker:
// The Client#id value of the client.
std::string client_id;
blink::mojom::ControllerServiceWorkerMode controller_mode =
blink::mojom::ControllerServiceWorkerMode::kNoController;
// Tracks feature usage for UseCounter.
std::set<blink::mojom::WebFeature> used_features;
// Corresponds to a ServiceWorkerContainer. We notify it when
// ServiceWorkerContainer#controller should be changed.
base::WeakPtr<WebServiceWorkerProviderImpl> 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
// |worker_clients| contains all its dedicated workers.
// - If this ServiceWorkerProviderContext is for a SharedWorker (technically
// speaking, for its shadow page), then |worker_clients| has one element:
// the shared worker.
std::vector<mojom::ServiceWorkerWorkerClientPtr> worker_clients;
// For adding new ServiceWorkerWorkerClients.
mojo::BindingSet<mojom::ServiceWorkerWorkerClientRegistry>
worker_client_registry_bindings;
// S13nServiceWorker
// Used in |subresource_loader_factory| to get the connection to the
// controller service worker.
//
// |controller_endpoint| is a Mojo pipe to the controller service worker,
// and is to be passed to (i.e. taken by) a subresource loader factory when
// GetSubresourceLoaderFactory() is called for the first time when a valid
// controller exists.
//
// |controller_connector| is a Mojo pipe to the
// ControllerServiceWorkerConnector that is attached to the newly created
// subresource loader factory and lives on a background thread. This is
// populated when GetSubresourceLoader() creates the subresource loader
// factory and takes |controller_endpoint|.
mojom::ControllerServiceWorkerPtrInfo controller_endpoint;
mojom::ControllerServiceWorkerConnectorPtr controller_connector;
// For service worker clients. Map from registration id to JavaScript
// ServiceWorkerRegistration object.
std::map<int64_t, WebServiceWorkerRegistrationImpl*> registrations_;
// For service worker clients. Map from version id to JavaScript ServiceWorker
// object.
std::map<int64_t, WebServiceWorkerImpl*> workers_;
};
} // namespace content
#endif // CONTENT_RENDERER_SERVICE_WORKER_SERVICE_WORKER_PROVIDER_STATE_FOR_CLIENT_H_
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment