Skip to content

Commit

Permalink
Revert of Reparent SWProcessManager onto SWContextWrapper. (https://c…
Browse files Browse the repository at this point in the history 
…odereview.chromium.org/292973003/)

Reason for revert:
I suspect this broke ServiceWorkerDispatcherHostTest.EarlyContextDeletion on linux

Original issue's description:
> Reparent SWProcessManager onto SWContextWrapper.
> 
> This will allow Wrapper::Shutdown to drop all process references synchronously
> in a future change, which will make it possible to shutdown Chrome with service
> workers running and without hitting the DCHECK in
> ProfileDestroyer::DestroyProfileWhenAppropriate().
> 
> BUG=368570
> 
> Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272839

TBR=falken@chromium.org,kinuko@chromium.org,jyasskin@chromium.org
NOTREECHECKS=true
NOTRY=true
BUG=368570

Review URL: https://codereview.chromium.org/307443003

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@272858 0039d316-1c4b-4281-b951-d872f2087c98
  • Loading branch information
jochen@chromium.org committed May 26, 2014
1 parent 407ac82 commit 643aff6
Show file tree
Hide file tree
Showing 10 changed files with 75 additions and 89 deletions.
5 changes: 4 additions & 1 deletion content/browser/service_worker/embedded_worker_test_helper.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,10 @@ EmbeddedWorkerTestHelper::EmbeddedWorkerTestHelper(int mock_render_process_id)
base::MessageLoopProxy::current(),
base::MessageLoopProxy::current(),
NULL);
wrapper_->process_manager()->SetProcessIdForTest(mock_render_process_id);
scoped_ptr<ServiceWorkerProcessManager> process_manager(
new ServiceWorkerProcessManager(wrapper_));
process_manager->SetProcessIdForTest(mock_render_process_id);
wrapper_->context()->SetProcessManagerForTest(process_manager.Pass());
registry()->AddChildProcessSender(mock_render_process_id, this);
}

Expand Down
16 changes: 5 additions & 11 deletions content/browser/service_worker/service_worker_context_core.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -82,16 +82,14 @@ ServiceWorkerContextCore::ServiceWorkerContextCore(
base::MessageLoopProxy* disk_cache_thread,
quota::QuotaManagerProxy* quota_manager_proxy,
ObserverListThreadSafe<ServiceWorkerContextObserver>* observer_list,
ServiceWorkerContextWrapper* wrapper)
scoped_ptr<ServiceWorkerProcessManager> process_manager)
: weak_factory_(this),
wrapper_(wrapper),
storage_(new ServiceWorkerStorage(path,
AsWeakPtr(),
database_task_runner,
disk_cache_thread,
quota_manager_proxy)),
storage_(new ServiceWorkerStorage(
path, AsWeakPtr(), database_task_runner, disk_cache_thread,
quota_manager_proxy)),
embedded_worker_registry_(new EmbeddedWorkerRegistry(AsWeakPtr())),
job_coordinator_(new ServiceWorkerJobCoordinator(AsWeakPtr())),
process_manager_(process_manager.Pass()),
next_handle_id_(0),
observer_list_(observer_list) {
}
Expand Down Expand Up @@ -309,8 +307,4 @@ void ServiceWorkerContextCore::OnReportConsoleMessage(
source_identifier, message_level, message, line_number, source_url));
}

ServiceWorkerProcessManager* ServiceWorkerContextCore::process_manager() {
return wrapper_->process_manager();
}

} // namespace content
18 changes: 11 additions & 7 deletions content/browser/service_worker/service_worker_context_core.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@ namespace content {

class EmbeddedWorkerRegistry;
class ServiceWorkerContextObserver;
class ServiceWorkerContextWrapper;
class ServiceWorkerHandle;
class ServiceWorkerJobCoordinator;
class ServiceWorkerProviderHost;
Expand Down Expand Up @@ -92,7 +91,7 @@ class CONTENT_EXPORT ServiceWorkerContextCore
base::MessageLoopProxy* disk_cache_thread,
quota::QuotaManagerProxy* quota_manager_proxy,
ObserverListThreadSafe<ServiceWorkerContextObserver>* observer_list,
ServiceWorkerContextWrapper* wrapper);
scoped_ptr<ServiceWorkerProcessManager> process_manager);
virtual ~ServiceWorkerContextCore();

// ServiceWorkerVersion::Listener overrides.
Expand All @@ -112,7 +111,9 @@ class CONTENT_EXPORT ServiceWorkerContextCore
const GURL& source_url) OVERRIDE;

ServiceWorkerStorage* storage() { return storage_.get(); }
ServiceWorkerProcessManager* process_manager();
ServiceWorkerProcessManager* process_manager() {
return process_manager_.get();
}
EmbeddedWorkerRegistry* embedded_worker_registry() {
return embedded_worker_registry_.get();
}
Expand Down Expand Up @@ -158,6 +159,12 @@ class CONTENT_EXPORT ServiceWorkerContextCore
return weak_factory_.GetWeakPtr();
}

// Allows tests to change how processes are created.
void SetProcessManagerForTest(
scoped_ptr<ServiceWorkerProcessManager> new_process_manager) {
process_manager_ = new_process_manager.Pass();
}

private:
typedef std::map<int64, ServiceWorkerRegistration*> RegistrationsMap;
typedef std::map<int64, ServiceWorkerVersion*> VersionMap;
Expand All @@ -177,14 +184,11 @@ class CONTENT_EXPORT ServiceWorkerContextCore
ServiceWorkerStatusCode status);

base::WeakPtrFactory<ServiceWorkerContextCore> weak_factory_;
// It's safe to store a raw pointer instead of a scoped_refptr to |wrapper_|
// because the Wrapper::Shutdown call that hops threads to destroy |this| uses
// Bind() to hold a reference to |wrapper_| until |this| is fully destroyed.
ServiceWorkerContextWrapper* wrapper_;
ProcessToProviderMap providers_;
scoped_ptr<ServiceWorkerStorage> storage_;
scoped_refptr<EmbeddedWorkerRegistry> embedded_worker_registry_;
scoped_ptr<ServiceWorkerJobCoordinator> job_coordinator_;
scoped_ptr<ServiceWorkerProcessManager> process_manager_;
std::map<int64, ServiceWorkerRegistration*> live_registrations_;
std::map<int64, ServiceWorkerVersion*> live_versions_;
int next_handle_id_;
Expand Down
36 changes: 18 additions & 18 deletions content/browser/service_worker/service_worker_context_wrapper.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ ServiceWorkerContextWrapper::ServiceWorkerContextWrapper(
BrowserContext* browser_context)
: observer_list_(
new ObserverListThreadSafe<ServiceWorkerContextObserver>()),
process_manager_(new ServiceWorkerProcessManager(browser_context)) {
browser_context_(browser_context) {
}

ServiceWorkerContextWrapper::~ServiceWorkerContextWrapper() {
Expand All @@ -39,12 +39,16 @@ void ServiceWorkerContextWrapper::Init(
}

void ServiceWorkerContextWrapper::Shutdown() {
DCHECK_CURRENTLY_ON(BrowserThread::UI);
process_manager_->Shutdown();
BrowserThread::PostTask(
BrowserThread::IO,
FROM_HERE,
base::Bind(&ServiceWorkerContextWrapper::ShutdownOnIO, this));
if (!BrowserThread::CurrentlyOn(BrowserThread::IO)) {
DCHECK_CURRENTLY_ON(BrowserThread::UI);
browser_context_ = NULL;
BrowserThread::PostTask(
BrowserThread::IO, FROM_HERE,
base::Bind(&ServiceWorkerContextWrapper::Shutdown, this));
return;
}
// Breaks the reference cycle through ServiceWorkerProcessManager.
context_core_.reset();
}

ServiceWorkerContextCore* ServiceWorkerContextWrapper::context() {
Expand Down Expand Up @@ -145,17 +149,13 @@ void ServiceWorkerContextWrapper::InitInternal(
return;
}
DCHECK(!context_core_);
context_core_.reset(new ServiceWorkerContextCore(user_data_directory,
database_task_runner,
disk_cache_thread,
quota_manager_proxy,
observer_list_,
this));
}

void ServiceWorkerContextWrapper::ShutdownOnIO() {
DCHECK_CURRENTLY_ON(BrowserThread::IO);
context_core_.reset();
context_core_.reset(new ServiceWorkerContextCore(
user_data_directory,
database_task_runner,
disk_cache_thread,
quota_manager_proxy,
observer_list_,
make_scoped_ptr(new ServiceWorkerProcessManager(this))));
}

} // namespace content
8 changes: 1 addition & 7 deletions content/browser/service_worker/service_worker_context_wrapper.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,11 +49,6 @@ class CONTENT_EXPORT ServiceWorkerContextWrapper
// The core context is only for use on the IO thread.
ServiceWorkerContextCore* context();

// The process manager can be used on either UI or IO.
ServiceWorkerProcessManager* process_manager() {
return process_manager_.get();
}

// ServiceWorkerContext implementation:
virtual void RegisterServiceWorker(
const GURL& pattern,
Expand All @@ -76,12 +71,11 @@ class CONTENT_EXPORT ServiceWorkerContextWrapper
base::SequencedTaskRunner* database_task_runner,
base::MessageLoopProxy* disk_cache_thread,
quota::QuotaManagerProxy* quota_manager_proxy);
void ShutdownOnIO();

const scoped_refptr<ObserverListThreadSafe<ServiceWorkerContextObserver> >
observer_list_;
const scoped_ptr<ServiceWorkerProcessManager> process_manager_;
// Cleared in Shutdown():
BrowserContext* browser_context_;
scoped_ptr<ServiceWorkerContextCore> context_core_;
};

Expand Down
17 changes: 5 additions & 12 deletions content/browser/service_worker/service_worker_process_manager.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,22 +35,15 @@ ServiceWorkerProcessManager::ProcessInfo::~ProcessInfo() {
}

ServiceWorkerProcessManager::ServiceWorkerProcessManager(
BrowserContext* browser_context)
: browser_context_(browser_context),
ServiceWorkerContextWrapper* context_wrapper)
: context_wrapper_(context_wrapper),
process_id_for_test_(-1),
weak_this_factory_(this),
weak_this_(weak_this_factory_.GetWeakPtr()) {
}

ServiceWorkerProcessManager::~ServiceWorkerProcessManager() {
DCHECK_CURRENTLY_ON(BrowserThread::UI);
DCHECK(browser_context_ == NULL)
<< "Call Shutdown() before destroying |this|, so that racing method "
<< "invocations don't use a destroyed BrowserContext.";
}

void ServiceWorkerProcessManager::Shutdown() {
browser_context_ = NULL;
}

void ServiceWorkerProcessManager::AllocateWorkerProcess(
Expand Down Expand Up @@ -98,7 +91,7 @@ void ServiceWorkerProcessManager::AllocateWorkerProcess(
}
}

if (!browser_context_) {
if (!context_wrapper_->browser_context_) {
// Shutdown has started.
BrowserThread::PostTask(
BrowserThread::IO,
Expand All @@ -107,8 +100,8 @@ void ServiceWorkerProcessManager::AllocateWorkerProcess(
return;
}
// No existing processes available; start a new one.
scoped_refptr<SiteInstance> site_instance =
SiteInstance::CreateForURL(browser_context_, script_url);
scoped_refptr<SiteInstance> site_instance = SiteInstance::CreateForURL(
context_wrapper_->browser_context_, script_url);
RenderProcessHost* rph = site_instance->GetProcess();
// This Init() call posts a task to the IO thread that adds the RPH's
// ServiceWorkerDispatcherHost to the
Expand Down
22 changes: 10 additions & 12 deletions content/browser/service_worker/service_worker_process_manager.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,25 +18,21 @@ class GURL;
namespace content {

class BrowserContext;
class ServiceWorkerContextWrapper;
class SiteInstance;

// Interacts with the UI thread to keep RenderProcessHosts alive while the
// ServiceWorker system is using them. Each instance of
// ServiceWorker system is using them. Each instance of
// ServiceWorkerProcessManager is destroyed on the UI thread shortly after its
// ServiceWorkerContextWrapper is destroyed.
// ServiceWorkerContextCore is destroyed on the IO thread.
class CONTENT_EXPORT ServiceWorkerProcessManager {
public:
// |*this| must be owned by a ServiceWorkerContextWrapper in a
// StoragePartition within |browser_context|.
explicit ServiceWorkerProcessManager(BrowserContext* browser_context);
// |*this| must be owned by |context_wrapper|->context().
explicit ServiceWorkerProcessManager(
ServiceWorkerContextWrapper* context_wrapper);

// Shutdown must be called before the ProcessManager is destroyed.
~ServiceWorkerProcessManager();

// Synchronously prevents new processes from being allocated.
// TODO(jyasskin): Drop references to RenderProcessHosts too.
void Shutdown();

// Returns a reference to a running process suitable for starting the Service
// Worker at |script_url|. Processes in |process_ids| will be checked in order
// for existence, and if none exist, then a new process will be created. Posts
Expand Down Expand Up @@ -84,8 +80,10 @@ class CONTENT_EXPORT ServiceWorkerProcessManager {
int process_id;
};

// These fields are only accessed on the UI thread.
BrowserContext* browser_context_;
// These fields are only accessed on the UI thread after construction.
// The reference cycle through context_wrapper_ is broken in
// ServiceWorkerContextWrapper::Shutdown().
scoped_refptr<ServiceWorkerContextWrapper> context_wrapper_;

// Maps the ID of a running EmbeddedWorkerInstance to information about the
// process it's running inside. Since the Instances themselves live on the IO
Expand Down
14 changes: 7 additions & 7 deletions content/browser/service_worker/service_worker_provider_host_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,13 @@ class ServiceWorkerProviderHostTest : public testing::Test {
virtual ~ServiceWorkerProviderHostTest() {}

virtual void SetUp() OVERRIDE {
context_.reset(
new ServiceWorkerContextCore(base::FilePath(),
base::MessageLoopProxy::current(),
base::MessageLoopProxy::current(),
NULL,
NULL,
NULL));
context_.reset(new ServiceWorkerContextCore(
base::FilePath(),
base::MessageLoopProxy::current(),
base::MessageLoopProxy::current(),
NULL,
NULL,
scoped_ptr<ServiceWorkerProcessManager>()));

scope_ = GURL("http://www.example.com/*");
script_url_ = GURL("http://www.example.com/service_worker.js");
Expand Down
14 changes: 7 additions & 7 deletions content/browser/service_worker/service_worker_registration_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,13 @@ class ServiceWorkerRegistrationTest : public testing::Test {
: io_thread_(BrowserThread::IO, &message_loop_) {}

virtual void SetUp() OVERRIDE {
context_.reset(
new ServiceWorkerContextCore(base::FilePath(),
base::MessageLoopProxy::current(),
base::MessageLoopProxy::current(),
NULL,
NULL,
NULL));
context_.reset(new ServiceWorkerContextCore(
base::FilePath(),
base::MessageLoopProxy::current(),
base::MessageLoopProxy::current(),
NULL,
NULL,
scoped_ptr<ServiceWorkerProcessManager>()));
context_ptr_ = context_->AsWeakPtr();
}

Expand Down
14 changes: 7 additions & 7 deletions content/browser/service_worker/service_worker_storage_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -150,13 +150,13 @@ class ServiceWorkerStorageTest : public testing::Test {
}

virtual void SetUp() OVERRIDE {
context_.reset(
new ServiceWorkerContextCore(base::FilePath(),
base::MessageLoopProxy::current(),
base::MessageLoopProxy::current(),
NULL,
NULL,
NULL));
context_.reset(new ServiceWorkerContextCore(
base::FilePath(),
base::MessageLoopProxy::current(),
base::MessageLoopProxy::current(),
NULL,
NULL,
scoped_ptr<ServiceWorkerProcessManager>()));
context_ptr_ = context_->AsWeakPtr();
}

Expand Down

0 comments on commit 643aff6

Please sign in to comment.