Keep alive ServiceWorkers when devtools is attached

Keep alive ServiceWorkers when devtools is attached

- Plumbed from EmbeddedWorkerDevToolsAgentHost to
 ServiceWorkerVersion to flip 'DevToolsAttached' flag
- Added EmbeddedWorkerInstance::StopIfIdle() which doesn't stop
 the worker if devtools is attached
- Changed stop-worker-timer code to call StopIfIdle() instead of Stop()
- Added a hook in EmbeddedDevToolsManager that is called when we
 didn't stop the worker (to show an educational console message)

BUG=429582
TEST=EmbeddedWorkerInstanceTest.StopWhenDevToolsAttached

Committed: https://crrev.com/756d8aa20291c1e6c286e0c61f630c6b211bf13e
Cr-Commit-Position: refs/heads/master@{#308332}

[kinuko, 2014-12-04 07:32:48]

kinuko@chromium.org changed reviewers:
+ falken@chromium.org, horo@chromium.org

[kinuko, 2014-12-04 07:35:21]

horo@: can you review (esp. sw-internals changes)?
falken@: can you review (esp. sw-version changes)?

https://codereview.chromium.org/761923004/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_context_core.h (right):

https://codereview.chromium.org/761923004/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_context_core.h:239: // Debug
options.
Will remove this comment..

[horo, 2014-12-05 00:40:42]

lgtm
Thank you!

[kinuko, 2014-12-05 03:40:51]

kinuko@chromium.org changed reviewers:
+ vsevik@chromium.org

[kinuko, 2014-12-05 03:41:36]

vsevik@: could you review content/browser/devtools changes?

[pfeldman, 2014-12-05 09:41:19]

I just recommended a slightly different solution in the bug. tl;dr, do not
schedule stop-worker-timer while devtools is attached.

[falken, 2014-12-08 02:30:16]

Just a note, didn't look closely due to ongoing bug discussion.

https://codereview.chromium.org/761923004/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/761923004/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:855: HasControllee() ||
My first reaction was we should still stop the worker even if there's a
controllee, but code inspection shows we currently try to not do that. That's
surprising because empirically a SW seems to get killed every 30 seconds even
with an controllee. So I'm not sure what the current behavior is now...

[kinuko, 2014-12-09 15:04:50]

Updated the patch to keep workers alive when devtools is attached.
horo@, falken@: could you take another look?
vsevik@, pfeldman@: could you take a look at devtools changes?

[kinuko, 2014-12-10 01:14:14]

Patchset #4 (id:60001) has been deleted

[falken, 2014-12-10 05:00:36]

lgtm!

(for the record, regarding my previous comment, it appears i was mistaken. we
currently don't kill a SW that has a controllee)

[horo, 2014-12-10 05:29:02]

lgtm

[kinuko, 2014-12-12 04:54:14]

ping: vsevik@ or someone in devtools, is it possible for some of you to take a
look at the changes in content/browser/devtools?

[kinuko, 2014-12-15 05:32:31]

kinuko@chromium.org changed reviewers:
+ pfeldman@chromium.org

[kinuko, 2014-12-15 05:35:06]

Pavel, could you perhaps review this CL's changes in devtools? Thanks,

[pfeldman, 2014-12-15 05:57:05]

https://codereview.chromium.org/761923004/diff/80001/content/browser/service_...
File content/browser/service_worker/embedded_worker_instance.cc (right):

https://codereview.chromium.org/761923004/diff/80001/content/browser/service_...
content/browser/service_worker/embedded_worker_instance.cc:61: if
(!BrowserThread::CurrentlyOn(BrowserThread::UI)) {
This implies it can be called multiple threads, but the call site touches local
variable.

https://codereview.chromium.org/761923004/diff/80001/content/browser/service_...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/761923004/diff/80001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:537:
embedded_worker()->set_devtools_attached(attached);
Do this first for clarity.

[pfeldman, 2014-12-15 06:11:09]

https://codereview.chromium.org/761923004/diff/80001/content/browser/devtools...
File content/browser/devtools/embedded_worker_devtools_agent_host.cc (right):

https://codereview.chromium.org/761923004/diff/80001/content/browser/devtools...
content/browser/devtools/embedded_worker_devtools_agent_host.cc:138: if
(service_worker_) {
Is this always true?

https://codereview.chromium.org/761923004/diff/80001/content/browser/devtools...
content/browser/devtools/embedded_worker_devtools_agent_host.cc:155: if
(service_worker_) {
ditto

https://codereview.chromium.org/761923004/diff/80001/content/browser/service_...
File content/browser/service_worker/embedded_worker_instance.cc (right):

https://codereview.chromium.org/761923004/diff/80001/content/browser/service_...
content/browser/service_worker/embedded_worker_instance.cc:61: if
(!BrowserThread::CurrentlyOn(BrowserThread::UI)) {
I can see you are following a pattern here, but the pattern is unhealthy. I can
also see this code under shared_worker.

[kinuko, 2014-12-15 06:39:14]

Thanks!

https://codereview.chromium.org/761923004/diff/80001/content/browser/devtools...
File content/browser/devtools/embedded_worker_devtools_agent_host.cc (right):

https://codereview.chromium.org/761923004/diff/80001/content/browser/devtools...
content/browser/devtools/embedded_worker_devtools_agent_host.cc:138: if
(service_worker_) {
On 2014/12/15 06:11:08, pfeldman wrote:
> Is this always true?

Looks like it's not, the same code is used for SharedWorker and ServiceWorker
and this is only non-null it's initialized for ServiceWorker.

https://codereview.chromium.org/761923004/diff/80001/content/browser/service_...
File content/browser/service_worker/embedded_worker_instance.cc (right):

https://codereview.chromium.org/761923004/diff/80001/content/browser/service_...
content/browser/service_worker/embedded_worker_instance.cc:61: if
(!BrowserThread::CurrentlyOn(BrowserThread::UI)) {
On 2014/12/15 06:11:08, pfeldman wrote:
> I can see you are following a pattern here, but the pattern is unhealthy. I
can
> also see this code under shared_worker.

This pattern is used pretty widely in content/browser. Why do you think it's
unhealthy? (We can start discussion on chromium-dev if it really should be
avoided)

(Separately I added an assertion line to make the thread restriction a  bit
clearer)

https://codereview.chromium.org/761923004/diff/80001/content/browser/service_...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/761923004/diff/80001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:537:
embedded_worker()->set_devtools_attached(attached);
On 2014/12/15 05:57:04, pfeldman wrote:
> Do this first for clarity.

Done.

[pfeldman, 2014-12-15 07:05:04]

lgtm, but please consider inheriting the hosts from a single abstract in a
follow up chant. With the additional service_worker checks that you add, it is
getting increasingly clear that those should be in different classes.

https://codereview.chromium.org/761923004/diff/80001/content/browser/devtools...
File content/browser/devtools/embedded_worker_devtools_agent_host.cc (right):

https://codereview.chromium.org/761923004/diff/80001/content/browser/devtools...
content/browser/devtools/embedded_worker_devtools_agent_host.cc:138: if
(service_worker_) {
Oh. I was not familiar with the code and did not realize that it was used either
for service or for shared workers. Looking at the code, it seems like
introducing ServiceWorkerDevToolsAgentHost and SharedWorkerDevToolsAgentHost
that inherit from EmbeddedWorkerDevToolsAgentHost and add corresponding
specifics to it would make my confusion disappear.

https://codereview.chromium.org/761923004/diff/80001/content/browser/service_...
File content/browser/service_worker/embedded_worker_instance.cc (right):

https://codereview.chromium.org/761923004/diff/80001/content/browser/service_...
content/browser/service_worker/embedded_worker_instance.cc:61: if
(!BrowserThread::CurrentlyOn(BrowserThread::UI)) {
Hm, I am surprised how widely it has spread. I think it is Ok where
notifications originate from different threads and the design is complex enough
to let go and fall back to this solution. But in cases where code still allows
tracking the threads situation, one should do so.

[pfeldman, 2014-12-15 07:18:43]

https://codereview.chromium.org/761923004/diff/80001/content/browser/service_...
File content/browser/service_worker/embedded_worker_instance.cc (right):

https://codereview.chromium.org/761923004/diff/80001/content/browser/service_...
content/browser/service_worker/embedded_worker_instance.cc:61: if
(!BrowserThread::CurrentlyOn(BrowserThread::UI)) {
I looked at the usages of the pattern and found out that they do indeed belong
either to non-thread-safe APIs or to internal methods that absolutely need to be
non-thread-safe. We definitely should not be doing it just in case. I can
initiate a chromium-dev discussion.

[kinuko, 2014-12-15 07:41:18]

Thanks, I'll follow up about splitting the DevAgentHost class into two.

https://codereview.chromium.org/761923004/diff/80001/content/browser/service_...
File content/browser/service_worker/embedded_worker_instance.cc (right):

https://codereview.chromium.org/761923004/diff/80001/content/browser/service_...
content/browser/service_worker/embedded_worker_instance.cc:61: if
(!BrowserThread::CurrentlyOn(BrowserThread::UI)) {
On 2014/12/15 07:18:42, pfeldman wrote:
> I looked at the usages of the pattern and found out that they do indeed belong
> either to non-thread-safe APIs or to internal methods that absolutely need to
be
> non-thread-safe. We definitely should not be doing it just in case. I can
> initiate a chromium-dev discussion.

Is the reason you think so because this pattern gives the wrong impression that
the method is thread-safe while it's not really the case?  I think we can
probably introduce a very explicit helper template if it helps getting rid of
this kind of confusion. Anyways I found your email on chromium-dev (thanks) so
let's see how we conclude.

[kinuko, 2014-12-15 08:08:57]

The CQ bit was checked by kinuko@chromium.org

[commit-bot: I haz the power, 2014-12-15 08:09:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/761923004/100001

[commit-bot: I haz the power, 2014-12-15 08:20:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-15 08:20:13]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[kinuko, 2014-12-15 09:34:06]

The CQ bit was checked by kinuko@chromium.org

[kinuko, 2014-12-15 09:34:12]

The CQ bit was checked by kinuko@chromium.org

[commit-bot: I haz the power, 2014-12-15 09:35:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/761923004/120001

[commit-bot: I haz the power, 2014-12-15 10:27:35]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2014-12-15 10:28:35]

Patchset 6 (id:??) landed as
https://crrev.com/756d8aa20291c1e6c286e0c61f630c6b211bf13e
Cr-Commit-Position: refs/heads/master@{#308332}