ServiceWorker: Create a proxy when adding the worker to DevToolsManager

ServiceWorker: Create a proxy when adding the worker to DevToolsManager

Currently ServiceWorkerDevToolsAgentHost is created on the UI thread in
ServiceWorkerDevToolsManager::WorkerCreated which is invoked on SetupOnUI, and
DevToolsProxy managing the agent is created in the task posted by SetupOnUI
which runs on the io thread. However, if the mojo interface which is connected
in the SetupOnUI fails to establish the connection to the renderer process,
OnDetach gets called before setting up the DevToolsProxy. This means the
ServiceWorkerDevToolsAgentHost is dangling even though EWInstance no longer has
any process.  This patch is to fix that by creating DevToolsProxy at the same
time with WorkerCreated. This enables us to manage the lifetime of the agent
even if EmbeddedWorkerInstance fails making connection somehow.

BUG=712515
Review-Url: https://codereview.chromium.org/2823853005
Cr-Commit-Position: refs/heads/master@{#465504}
Committed: https://chromium.googlesource.com/chromium/src/+/6c2885ec2d91839b39a248c1461b5ecd7d857bf9

[shimazu, 2017-04-18 08:02:16]

The CQ bit was checked by shimazu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-18 08:02:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[shimazu, 2017-04-18 08:02:45]

shimazu@chromium.org changed reviewers:
+ falken@chromium.org

[shimazu, 2017-04-18 08:02:46]

ptal, thanks!

[falken, 2017-04-18 08:38:11]

This seems complicated.... if I understand correctly, the failure mode we are
trying to avoid is having devtools_proxy_ set after OnDetached() was called.

With this patch how do we know OnRegisteredToDevToolsManager() is called before
OnDetached()? Can you walk me through how OnDetached() is called if the Mojo
interface fails to establish the connection to the renderer process in
SetupOnUI? Is that the BindInterface(rph, std::move(request)) line?

https://codereview.chromium.org/2823853005/diff/1/content/browser/service_wor...
File content/browser/service_worker/embedded_worker_instance.cc (right):

https://codereview.chromium.org/2823853005/diff/1/content/browser/service_wor...
content/browser/service_worker/embedded_worker_instance.cc:87: nullptr;
nit: don't need to assign nullptr explicitly, the unique_ptr constructor sets
nullptr by default

[shimazu, 2017-04-18 09:11:00]

On 2017/04/18 08:38:11, falken wrote:
> This seems complicated.... if I understand correctly, the failure mode we are
> trying to avoid is having devtools_proxy_ set after OnDetached() was called.
> 
> With this patch how do we know OnRegisteredToDevToolsManager() is called
before
> OnDetached()? Can you walk me through how OnDetached() is called if the Mojo
> interface fails to establish the connection to the renderer process in
> SetupOnUI? Is that the BindInterface(rph, std::move(request)) line?

Yes. The error handler which is set in EWInstance::Start is queued when
BindInterface fails, and it calls ReleaseProcess. This removes the
|inflight_start_task_|, so the callback (StartTask::OnSetupOnUICompleted) is
canceled. My patch is to detect the cancellation.

> 
>
https://codereview.chromium.org/2823853005/diff/1/content/browser/service_wor...
> File content/browser/service_worker/embedded_worker_instance.cc (right):
> 
>
https://codereview.chromium.org/2823853005/diff/1/content/browser/service_wor...
> content/browser/service_worker/embedded_worker_instance.cc:87: nullptr;
> nit: don't need to assign nullptr explicitly, the unique_ptr constructor sets
> nullptr by default

[shimazu, 2017-04-18 09:12:11]

Oops. How do I publish the draft with replying? Copy and paste...?

https://codereview.chromium.org/2823853005/diff/1/content/browser/service_wor...
File content/browser/service_worker/embedded_worker_instance.cc (right):

https://codereview.chromium.org/2823853005/diff/1/content/browser/service_wor...
content/browser/service_worker/embedded_worker_instance.cc:87: nullptr;
On 2017/04/18 08:38:11, falken wrote:
> nit: don't need to assign nullptr explicitly, the unique_ptr constructor sets
> nullptr by default

Done.

[falken, 2017-04-19 03:35:57]

OK thanks for explaining. lgtm

[shimazu, 2017-04-19 04:23:59]

The CQ bit was checked by shimazu@chromium.org

[commit-bot: I haz the power, 2017-04-19 04:24:13]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-19 05:48:56]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1492575839085390,
"parent_rev": "64dafab9bb2755e8866dc4efa778302598dfd6ea", "commit_rev":
"6c2885ec2d91839b39a248c1461b5ecd7d857bf9"}

[commit-bot: I haz the power, 2017-04-19 05:49:04]

Description was changed from

==========
ServiceWorker: Create a proxy when adding the worker to DevToolsManager

Currently ServiceWorkerDevToolsAgentHost is created on the UI thread in
ServiceWorkerDevToolsManager::WorkerCreated which is invoked on SetupOnUI, and
DevToolsProxy managing the agent is created in the task posted by SetupOnUI
which runs on the io thread. However, if the mojo interface which is connected
in the SetupOnUI fails to establish the connection to the renderer process,
OnDetach gets called before setting up the DevToolsProxy. This means the
ServiceWorkerDevToolsAgentHost is dangling even though EWInstance no longer has
any process.  This patch is to fix that by creating DevToolsProxy at the same
time with WorkerCreated. This enables us to manage the lifetime of the agent
even if EmbeddedWorkerInstance fails making connection somehow.

BUG=712515
==========

to

==========
ServiceWorker: Create a proxy when adding the worker to DevToolsManager

Currently ServiceWorkerDevToolsAgentHost is created on the UI thread in
ServiceWorkerDevToolsManager::WorkerCreated which is invoked on SetupOnUI, and
DevToolsProxy managing the agent is created in the task posted by SetupOnUI
which runs on the io thread. However, if the mojo interface which is connected
in the SetupOnUI fails to establish the connection to the renderer process,
OnDetach gets called before setting up the DevToolsProxy. This means the
ServiceWorkerDevToolsAgentHost is dangling even though EWInstance no longer has
any process.  This patch is to fix that by creating DevToolsProxy at the same
time with WorkerCreated. This enables us to manage the lifetime of the agent
even if EmbeddedWorkerInstance fails making connection somehow.

BUG=712515

Review-Url: https://codereview.chromium.org/2823853005
Cr-Commit-Position: refs/heads/master@{#465504}
Committed:
https://chromium.googlesource.com/chromium/src/+/6c2885ec2d91839b39a248c1461b...
==========

[commit-bot: I haz the power, 2017-04-19 05:49:05]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/6c2885ec2d91839b39a248c1461b...