Add ref count to service workers for extension API.

Add ref count to service workers for extension API.

We need a way to keep a service worker alive
1) during extension function's request->response roundtrip completes.
2) when an event is about to be dispatched to a (stopped) service worker.

This CL shows a way to do #1. #2 can follow later.

The CL adds plumbing to expose functions to increment/decrement ref
counting to an ServiceWorkerVersion. This is done by adding a way to
add "external requests" to a ServiceWorkerVersion: when a worker has
external requests pending, it will be considered to be in working state,
i.e ServiceWorkerVersion::HasWork() will return true.
The public interface is exposed through ServiceWorkerContext. And the
interface methods expect a GUID/nonce for each such requests from service
worker renderer:
ServiceWorkerContext::StartingExternalRequest() and
ServiceWorkerContext::FinishedExternalRequest()

Extension APIs that are expected to be long running aren't handled in
this CL. For example: an extension API showing a dialog to user that
waits for user action.

BUG=602442
Test=There's no easy way to test it without tweaking the code, I've
used the following steps to make sure that we keep SW alive when an extension
API is in-flight:
Change the stop worker idle timeout and worker timeout to sth small, e.g. 3s.
Call an extension function that runs for 7s (> 3s + 3s). Without the CL, the
extension function's callback won't be called because the worker would shut
down after 6s.
The added test ServiceWorkerTest.WorkerRefCount tests this at a bit lower level:
by checking ref count (= count of external requests for a ServiceWorkerVersion).

Committed: https://crrev.com/4c82177a8b45303c941ddbc469444f0bfc760de4
Cr-Commit-Position: refs/heads/master@{#425824}

[falken, 2016-07-22 05:06:01]

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

[falken, 2016-07-22 05:06:02]

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

https://codereview.chromium.org/2166523003/diff/1/content/browser/service_wor...
content/browser/service_worker/embedded_worker_instance.h:200: void
DecrementRefCount();
ref count is kind of generic, maybe "IncrementPendingActivity" would be more
clear that this affects StopIfIdle.

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

https://codereview.chromium.org/2166523003/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_version.cc:195: const int
ServiceWorkerVersion::kTimeoutTimerDelaySeconds = 3;
Are these timer changes just for proof of concept?

I'm not sure we want to do this in production. The timers are intentionally slow
so SW doesn't cause too much work in the browser process and we also keep SW
alive for a while in case it'll need to be used again (for example, initial page
load, user looks at page for a while, then clicks a link).

[waxmiguel, 2016-07-22 05:11:41]

On 2016/07/22 05:06:02, falken wrote:
>
https://codereview.chromium.org/2166523003/diff/1/content/browser/service_wor...
> File content/browser/service_worker/embedded_worker_instance.h (right):
> 
>
https://codereview.chromium.org/2166523003/diff/1/content/browser/service_wor...
> content/browser/service_worker/embedded_worker_instance.h:200: void
> DecrementRefCount();
> ref count is kind of generic, maybe "IncrementPendingActivity" would be more
> clear that this affects StopIfIdle.
> 
>
https://codereview.chromium.org/2166523003/diff/1/content/browser/service_wor...
> File content/browser/service_worker/service_worker_version.cc (right):
> 
>
https://codereview.chromium.org/2166523003/diff/1/content/browser/service_wor...
> content/browser/service_worker/service_worker_version.cc:195: const int
> ServiceWorkerVersion::kTimeoutTimerDelaySeconds = 3;
> Are these timer changes just for proof of concept?
> 
> I'm not sure we want to do this in production. The timers are intentionally
slow
> so SW doesn't cause too much work in the browser process and we also keep SW
> alive for a while in case it'll need to be used again (for example, initial
page
> load, user looks at page for a while, then clicks a link).

[lazyboy, 2016-07-26 17:45:47]

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

[lazyboy, 2016-07-26 17:45:48]

kinuko@/horo@, can any of you take a look at the content/ part of the CL? Please
ignore the temporary LOGs and timeout value changes. Those are only meant for
testing the proof of concept. Thanks.

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

https://codereview.chromium.org/2166523003/diff/1/content/browser/service_wor...
content/browser/service_worker/embedded_worker_instance.h:200: void
DecrementRefCount();
On 2016/07/22 05:06:01, falken (away) wrote:
> ref count is kind of generic, maybe "IncrementPendingActivity" would be more
> clear that this affects StopIfIdle.

Done.

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

https://codereview.chromium.org/2166523003/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_version.cc:195: const int
ServiceWorkerVersion::kTimeoutTimerDelaySeconds = 3;
On 2016/07/22 05:06:02, falken (away) wrote:
> Are these timer changes just for proof of concept?

Yes, these are entirely for proof of concept, especially for running the test
locally for now.

I need to find a better way to test the timeout behavior in tests. Once the CL
progresses, I'll do so.

> 
> I'm not sure we want to do this in production. The timers are intentionally
slow
> so SW doesn't cause too much work in the browser process and we also keep SW
> alive for a while in case it'll need to be used again (for example, initial
page
> load, user looks at page for a while, then clicks a link).

[michaeln, 2016-07-26 18:55:48]

I didn't look closely, so i may be off base...

The ServiceWorkerVersion has StartRequest + FinishRequest methods to help keep
workers alive. This is currently used to annotate spans of time where the sw
script is asyncly processing a request (so shouldn't be killed by the browser
process). Could this mechanism also be used to "keep it alive" when the
extension system is asyncly satisfying a request on behalf of the service
worker?

The 'request' starts when the extension api is invoked by the sw script and
finishes once the result has been received by the script sort of thing?

[lazyboy, 2016-07-26 20:05:01]

On 2016/07/26 18:55:48, michaeln wrote:
> I didn't look closely, so i may be off base...
> 
> The ServiceWorkerVersion has StartRequest + FinishRequest methods to help keep
> workers alive. This is currently used to annotate spans of time where the sw
> script is asyncly processing a request (so shouldn't be killed by the browser
> process). Could this mechanism also be used to "keep it alive" when the
> extension system is asyncly satisfying a request on behalf of the service
> worker?
I've came across that too when trying to implement this.
Couple of things:

1) I didn't look hard enough how to retrieve ServiceWorkerVersion associated
with the embedded worker id. I'll take a look at it then. One caveat is
that nothing related to ServiceWorkerVersion is exposed outside of content/

2) I wasn't sure whether adding a ServiceWorkerMetrics::EventType for extension
API is a good idea. Can I use EventType::UNKNOWN?
> 
> The 'request' starts when the extension api is invoked by the sw script and
> finishes once the result has been received by the script sort of thing?
Yes, that's should be good enough for my need. I do have a start and a
finish phase for the 'request'.

Let me see if I can get #1 to work with UNKNOWN EventType?
I'll ping this thread once I'm done.

[lazyboy, 2016-07-27 01:22:30]

lazyboy@chromium.org changed reviewers:
+ michaeln@chromium.org

[lazyboy, 2016-07-27 01:22:31]

@michaeln please take a look at the approach in patch set #4:
1. I'm using SWVersion::Start/FinishRequest
2. The type of the request cannot be ServiceWorkerMetrics::EventType::UNKNOWN
because it's failing service_worker_metrics.cc,
ServiceWorkerMetrics::RecordEventDuration, maybe we can ignore that? Anyway, for
now I've added a value in EventType.

[horo, 2016-07-27 05:59:55]

> We need a way to keep a service worker alive
> 1) during extension function's request->response roundtrip completes.
> 2) when an event is about to be dispatched to a (stopped) service worker.

I don't think we should keep a service worker alive.

For example:
In this case, the SW may be stopped while calling chrome.tabs.create.
--sw.js----------
self.onmessage = function(e) {
    chrome.tabs.create({'url': 'http://www.google.com'},function(tabs) {});
};
----------------

If you want to keep the SW alive, you must use waitUntil().
--sw.js----------
self.onmessage = function(e) {
  e.waitUntil(new Promise(function(resolve) {
    chrome.tabs.create({'url': 'http://www.google.com'},function(tabs) {
      resolve();
    });
  }))
};
----------------

Sorry, I can't understand 2).
In what situation we have to start a stopped SW?

[lazyboy, 2016-07-27 20:21:07]

On 2016/07/27 05:59:55, horo wrote:
> > We need a way to keep a service worker alive
> > 1) during extension function's request->response roundtrip completes.
> > 2) when an event is about to be dispatched to a (stopped) service worker.
> 
> I don't think we should keep a service worker alive.
> 
> For example:
> In this case, the SW may be stopped while calling chrome.tabs.create.
> --sw.js----------
> self.onmessage = function(e) {
>     chrome.tabs.create({'url': 'http://www.google.com'},function(tabs) {});
> };
> ----------------
> 
> If you want to keep the SW alive, you must use waitUntil().
> --sw.js----------
> self.onmessage = function(e) {
>   e.waitUntil(new Promise(function(resolve) {
>     chrome.tabs.create({'url': 'http://www.google.com'},function(tabs) {
>       resolve();
>     });
>   }))
> };
> ----------------
This requires ExtendableEvent |e| to be available on the SW [1]
Our goal is to eventually implement chrome extension event/background pages
with extension Service Workers (behind the scenes), without exposing the
internal details to the developers. To developer, it would appear as regular
event/background page.
Background pages have a concept of keeping them alive via increment/decrement
keepalive count. Since service worker already has this internal structure
via pending request, this CL is exposing that to be used from extensions
layer.

[1]
e.g. code like these where you want to create a tab when extension is loaded
won't be able to grab an instance of ExtendableEvent:
sw.js
chrome.tabs.create(.., function() { /* no guarantee that this will fire */ });

I've also tried creating a new ExtendableEvent, but that isn't enough to
keep the SW alive:
var e1 = new ExtendableEvent('foo');
e1.waitUntil(new Promise(resolve) {
  chrome.tabs.create(.., function() { resolve(); });
});
If that worked, we could wrap (in bindings code) all extension API calls with 
ExtendableEvent.waitUntil().

> 
> Sorry, I can't understand 2).
> In what situation we have to start a stopped SW?

Sure, these are chrome events I was talking about:
https://developer.chrome.com/extensions/events
A chrome extension background page can be stopped after inactivity.
The equivalent scenario in extension SW is that the SW is stopped after
being idle.
Now if for example user clicks on browserAction on the toolbar,
chrome.browserAction.onClicked event needs to be fired and before doing
that, we wake up the background page:
https://developer.chrome.com/extensions/browserAction#event-onClicked
The equivalent scenario in extension SW is the SW needs to be started
and then chrome.browser.onClicked event needs to be dispatched to it.

A better example would be chrome.alarms:
https://developer.chrome.com/apps/alarms#event-onAlarm

[michaeln, 2016-07-27 20:47:13]

On 2016/07/27 20:21:07, lazyboy wrote:
> On 2016/07/27 05:59:55, horo wrote:
> > > We need a way to keep a service worker alive
> > > 1) during extension function's request->response roundtrip completes.
> > > 2) when an event is about to be dispatched to a (stopped) service worker.
> > 
> > I don't think we should keep a service worker alive.
> > 
> > For example:
> > In this case, the SW may be stopped while calling chrome.tabs.create.
> > --sw.js----------
> > self.onmessage = function(e) {
> >     chrome.tabs.create({'url': 'http://www.google.com'},function(tabs) {});
> > };
> > ----------------
> > 
> > If you want to keep the SW alive, you must use waitUntil().
> > --sw.js----------
> > self.onmessage = function(e) {
> >   e.waitUntil(new Promise(function(resolve) {
> >     chrome.tabs.create({'url': 'http://www.google.com'},function(tabs) {
> >       resolve();
> >     });
> >   }))
> > };
> > ----------------
> This requires ExtendableEvent |e| to be available on the SW [1]
> Our goal is to eventually implement chrome extension event/background pages
> with extension Service Workers (behind the scenes), without exposing the
> internal details to the developers. To developer, it would appear as regular
> event/background page.
> Background pages have a concept of keeping them alive via increment/decrement
> keepalive count. Since service worker already has this internal structure
> via pending request, this CL is exposing that to be used from extensions
> layer.
> 
> [1]
> e.g. code like these where you want to create a tab when extension is loaded
> won't be able to grab an instance of ExtendableEvent:
> sw.js
> chrome.tabs.create(.., function() { /* no guarantee that this will fire */ });
> 
> I've also tried creating a new ExtendableEvent, but that isn't enough to
> keep the SW alive:
> var e1 = new ExtendableEvent('foo');
> e1.waitUntil(new Promise(resolve) {
>   chrome.tabs.create(.., function() { resolve(); });
> });
> If that worked, we could wrap (in bindings code) all extension API calls with 
> ExtendableEvent.waitUntil().
> 
> > 
> > Sorry, I can't understand 2).
> > In what situation we have to start a stopped SW?
> 
> Sure, these are chrome events I was talking about:
> https://developer.chrome.com/extensions/events
> A chrome extension background page can be stopped after inactivity.
> The equivalent scenario in extension SW is that the SW is stopped after
> being idle.
> Now if for example user clicks on browserAction on the toolbar,
> chrome.browserAction.onClicked event needs to be fired and before doing
> that, we wake up the background page:
> https://developer.chrome.com/extensions/browserAction#event-onClicked
> The equivalent scenario in extension SW is the SW needs to be started
> and then chrome.browser.onClicked event needs to be dispatched to it.

See RunAfterStartWorker() usages for examples of starting a worker in order to
dispatch an event is handled. That's used in conjunction with StartRequest().

> A better example would be chrome.alarms:
> https://developer.chrome.com/apps/alarms#event-onAlarm

[horo, 2016-07-28 06:25:34]

OK. I understand why you need to increment/decrement the keepalive count to
support extension APIs on ServiceWorker.

But I still have a concern about the race condition:

ExtensionFunctionDispatcher::DispatchWithCallbackInternal() is called on UI
thread.
So, when ExtensionFunctionDispatcher receives an extension API IPC message on UI
thread,
ServiceWorkerContext may have already sent StopWorker message form IO thread.
In this case, the SW is stooped while calling the extension API.

To avoid this race condition, I think we have to increment the keepalive count
when sending an event IPC message to the SW if the SW supports extension APIs.
And ServiceWorkerVersion::FinishRequest() must decrement it via IO->UI->IO
thread hops.

[lazyboy, 2016-08-18 18:02:10]

Patchset #8 (id:140001) has been deleted

[lazyboy, 2016-08-18 18:02:21]

Patchset #8 (id:160001) has been deleted

[lazyboy, 2016-08-18 18:22:26]

lazyboy@chromium.org changed reviewers:
- falken@chromium.org, kinuko@chromium.org

[lazyboy, 2016-08-18 18:22:27]

Sorry, this kept slipping, I'm back to working on this!
(removing other reviewers to avoid email spam)

horo:
But I still have a concern about the race condition:

ExtensionFunctionDispatcher::DispatchWithCallbackInternal() is called on UI
thread.
So, when ExtensionFunctionDispatcher receives an extension API IPC message on UI
thread,
ServiceWorkerContext may have already sent StopWorker message form IO thread.
In this case, the SW is stooped while calling the extension API.

To avoid this race condition, I think we have to increment the keepalive count
when sending an event IPC message to the SW if the SW supports extension APIs.

lazyboy:
-> I've fixed this in patch set #8

horo:
And ServiceWorkerVersion::FinishRequest() must decrement it via IO->UI->IO
thread hops.

lazyboy:
-> Can you explain this one a bit as to why we need this particular hop?
Right now what happens is:
(UI) extension function starts, check worker on IO
(IO) worker checked and PendingActivityCount incremented, post task to continue
extension function on UI
(UI) extension function continues
(UI) extension function completes, i.e. we send IPC
(ExtensionMsg_ResponseWorker) to renderer
(UI) decrement PendingActivityCount for worker by post tasking to IO
(IO) worker decrements PendingActivityCount <- #

# what race could we encounter here?

[michaeln, 2016-08-19 01:01:59]

Maybe the way to do this is more from the renderer side? Consider methods like
this somewhere in the content/public/renderer api.

namespace content {
// callable from any thread
static void ServiceWorker_StartingExternalRequest(version_id);
static void ServiceWOrker_FinishedExternalRequest(version_id);
};

Under the covers, these send a 'start' and 'stop' ipcs plumb to StartRequest +
FinishRequest methods in the browser process. Call the start method before
sending the extension message and call the finished method after having handled
the response.

We don't have any public api surface are for serviceworkers yet on the renderer
side of things yet, but i'm sure we could find someplace to put this if it makes
sense.

[lazyboy, 2016-08-19 01:37:23]

On 2016/08/19 01:01:59, michaeln wrote:
> Maybe the way to do this is more from the renderer side? Consider methods like
> this somewhere in the content/public/renderer api.
> 
> namespace content {
> // callable from any thread
> static void ServiceWorker_StartingExternalRequest(version_id);
> static void ServiceWOrker_FinishedExternalRequest(version_id);
> };

@horo, WDYT?
> 
> Under the covers, these send a 'start' and 'stop' ipcs 
These would be new IPCs, right?

plumb to StartRequest +
> FinishRequest methods in the browser process. Call the start method before
> sending the extension message and call the finished method after having
handled
> the response.
This sounds reasonable to me. Before I attempt that let's see what @horo thinks.
Thanks.
> 
> We don't have any public api surface are for serviceworkers yet on the
renderer
> side of things yet, but i'm sure we could find someplace to put this if it
makes
> sense.

[horo, 2016-08-19 09:04:13]

> # what race could we encounter here?

I wrote the diagram about the race condition.
http://goo.gl/k8mk7X

> > Maybe the way to do this is more from the renderer side? Consider methods
like
> > this somewhere in the content/public/renderer api.
> > 
> > namespace content {
> > // callable from any thread
> > static void ServiceWorker_StartingExternalRequest(version_id);
> > static void ServiceWOrker_FinishedExternalRequest(version_id);
> > };
>
> @horo, WDYT?

Ah, yes. That sounds reasonable.
Sending "Starting/FinishedExternalRequest" IPC message from the renderer process
may be a better solution.

[michaeln, 2016-08-19 19:14:37]

On 2016/08/19 09:04:13, horo wrote:
> > # what race could we encounter here?
> 
> I wrote the diagram about the race condition.
> http://goo.gl/k8mk7X
> 
> > > Maybe the way to do this is more from the renderer side? Consider methods
> like
> > > this somewhere in the content/public/renderer api.
> > > 
> > > namespace content {
> > > // callable from any thread
> > > static void ServiceWorker_StartingExternalRequest(version_id);
> > > static void ServiceWOrker_FinishedExternalRequest(version_id);
> > > };
> >
> > @horo, WDYT?
> 
> Ah, yes. That sounds reasonable.
> Sending "Starting/FinishedExternalRequest" IPC message from the renderer
process
> may be a better solution.

I can imagine push back from a security review perspective because a compromised
renderer could keep its process alive indefinitely. Here's a refinement to
defeat that.

// APIs available from withing a renderer
// This will keep it alive for kDefaultExternalRequestTimeAllowance amount of
time.
void StartingExternalRequest(version_id, external_request_uuid);
void FinishedExternalRequest(version_id, external_request_uuid);

// API only available in the browser process.
// To let it ride for longer, an agency in the privileged browser process
// has to call this method. Each call resets the timer on the external request.
// So every kDefaultExternalRequestTimeAllowance/2 seconds or so, a long running
// extension function should call this method.
void ContinuingExternalRequest(version_id,  external_request_uuid);

[horo, 2016-08-24 01:55:51]

On 2016/08/19 19:14:37, michaeln wrote:
> On 2016/08/19 09:04:13, horo wrote:
> > > # what race could we encounter here?
> > 
> > I wrote the diagram about the race condition.
> > http://goo.gl/k8mk7X
> > 
> > > > Maybe the way to do this is more from the renderer side? Consider
methods
> > like
> > > > this somewhere in the content/public/renderer api.
> > > > 
> > > > namespace content {
> > > > // callable from any thread
> > > > static void ServiceWorker_StartingExternalRequest(version_id);
> > > > static void ServiceWOrker_FinishedExternalRequest(version_id);
> > > > };
> > >
> > > @horo, WDYT?
> > 
> > Ah, yes. That sounds reasonable.
> > Sending "Starting/FinishedExternalRequest" IPC message from the renderer
> process
> > may be a better solution.
> 
> I can imagine push back from a security review perspective because a
compromised
> renderer could keep its process alive indefinitely. Here's a refinement to
> defeat that.
> 
> // APIs available from withing a renderer
> // This will keep it alive for kDefaultExternalRequestTimeAllowance amount of
> time.
> void StartingExternalRequest(version_id, external_request_uuid);
> void FinishedExternalRequest(version_id, external_request_uuid);
> 
> // API only available in the browser process.
> // To let it ride for longer, an agency in the privileged browser process
> // has to call this method. Each call resets the timer on the external
request.
> // So every kDefaultExternalRequestTimeAllowance/2 seconds or so, a long
running
> // extension function should call this method.
> void ContinuingExternalRequest(version_id,  external_request_uuid);

Humm..
I don't know well about chrome extension event/background pages.
Is it possible to use this kind of timeout? > lazyboy@
Who is controlling the lifetime of the background pages now?

[michaeln, 2016-08-24 19:10:35]

On 2016/08/24 01:55:51, horo wrote:
> On 2016/08/19 19:14:37, michaeln wrote:
> > On 2016/08/19 09:04:13, horo wrote:
> > > > # what race could we encounter here?
> > > 
> > > I wrote the diagram about the race condition.
> > > http://goo.gl/k8mk7X
> > > 
> > > > > Maybe the way to do this is more from the renderer side? Consider
> methods
> > > like
> > > > > this somewhere in the content/public/renderer api.
> > > > > 
> > > > > namespace content {
> > > > > // callable from any thread
> > > > > static void ServiceWorker_StartingExternalRequest(version_id);
> > > > > static void ServiceWOrker_FinishedExternalRequest(version_id);
> > > > > };
> > > >
> > > > @horo, WDYT?
> > > 
> > > Ah, yes. That sounds reasonable.
> > > Sending "Starting/FinishedExternalRequest" IPC message from the renderer
> > process
> > > may be a better solution.
> > 
> > I can imagine push back from a security review perspective because a
> compromised
> > renderer could keep its process alive indefinitely. Here's a refinement to
> > defeat that.
> > 
> > // APIs available from withing a renderer
> > // This will keep it alive for kDefaultExternalRequestTimeAllowance amount
of
> > time.
> > void StartingExternalRequest(version_id, external_request_uuid);
> > void FinishedExternalRequest(version_id, external_request_uuid);
> > 
> > // API only available in the browser process.
> > // To let it ride for longer, an agency in the privileged browser process
> > // has to call this method. Each call resets the timer on the external
> request.
> > // So every kDefaultExternalRequestTimeAllowance/2 seconds or so, a long
> running
> > // extension function should call this method.
> > void ContinuingExternalRequest(version_id,  external_request_uuid);
> 
> Humm..
> I don't know well about chrome extension event/background pages.
> Is it possible to use this kind of timeout? > lazyboy@
> Who is controlling the lifetime of the background pages now?

Keep in mind that extension/apps are generally more trusted than drive-by-web
content. We could/should probably do this without the 'Continuing' api to start
with, have ~30 (?) second timeout allowance, if/when that is found to be
insufficient... then we could introduce the 'Continuing' api.

[lazyboy, 2016-09-15 01:33:38]

Moved increment/decrement ref count logic to renderer/->IO thread IPC, please
take a look, thanks!

[horo, 2016-09-16 16:54:52]

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

[horo, 2016-09-16 16:54:54]

On 2016/09/15 01:33:38, lazyboy wrote:
> Moved increment/decrement ref count logic to renderer/->IO thread IPC, please
> take a look, thanks!

I think this design looks good.

michaeln@ +falken@
What do you think?

[falken, 2016-09-20 06:29:53]

On 2016/09/16 16:54:54, horo wrote:
> On 2016/09/15 01:33:38, lazyboy wrote:
> > Moved increment/decrement ref count logic to renderer/->IO thread IPC,
please
> > take a look, thanks!
> 
> I think this design looks good.
> 
> michaeln@ +falken@
> What do you think?

I haven't closely followed all the discussion (thanks horo@ and michaeln@ for
the thorough review!) but using Increment/Decrement IPC messages in conjunction
with Start/FinishRequest looks good. I'll take a closer look tomorrow.

[falken, 2016-09-23 02:27:35]

This looks pretty good.

Regarding the timeout allowance, SWVersion currently imposes a 5 minute timeout
on all requests. But a malicious renderer could still stay alive that by
starting a new request right before ending the previous request, so that there's
always an inflight request. However that's true with service workers today (see
bug 647943) so I think we'll need a more general solution.

https://codereview.chromium.org/2166523003/diff/200001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/service_worker/tabs_create/sw.js
(right):

https://codereview.chromium.org/2166523003/diff/200001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/service_worker/tabs_create/sw.js:10:
console.log('got id: ' + id);
debug output?

https://codereview.chromium.org/2166523003/diff/200001/content/browser/servic...
File content/browser/service_worker/service_worker_context_wrapper.cc (right):

https://codereview.chromium.org/2166523003/diff/200001/content/browser/servic...
content/browser/service_worker/service_worker_context_wrapper.cc:60: void
DoNothing(ServiceWorkerStatusCode status) {}
you can use ServiceWorkerUtils::NoOpStatusCallback

https://codereview.chromium.org/2166523003/diff/200001/content/browser/servic...
content/browser/service_worker/service_worker_context_wrapper.cc:110: return
IncrementPendingActivityOnIO(service_worker_version_id);
Can the *OnIO functions just be removed?

https://codereview.chromium.org/2166523003/diff/200001/content/browser/servic...
content/browser/service_worker/service_worker_context_wrapper.cc:846: if
(!version) {
You should also confirm the version is RUNNING. It's possible the renderer is
confused/lying or the version started stopping right around the time of the IPC.

https://codereview.chromium.org/2166523003/diff/200001/content/browser/servic...
content/browser/service_worker/service_worker_context_wrapper.cc:848: <<
service_worker_version_id;
do you need these logs in production code? DVLOG(1)?

https://codereview.chromium.org/2166523003/diff/200001/content/browser/servic...
content/browser/service_worker/service_worker_context_wrapper.cc:863: if
(!version) {
I'm not too sure when we get in this state: the renderer might have made a
mistake or the version might have been destructed due to some timing issue. We
should probably clean up the pending_external_requests  in this case though. Or
maybe the set of external requests should just be in ServiceWorkerVersion?

Also, the case where the version is not RUNNING should probably be handled.

https://codereview.chromium.org/2166523003/diff/200001/content/browser/servic...
content/browser/service_worker/service_worker_context_wrapper.cc:874: return
true;
This ignores FinishRequest's return value. Maybe it should be used?

https://codereview.chromium.org/2166523003/diff/200001/content/browser/servic...
File content/browser/service_worker/service_worker_metrics.cc (right):

https://codereview.chromium.org/2166523003/diff/200001/content/browser/servic...
content/browser/service_worker/service_worker_metrics.cc:572: case
EventType::EXTERNAL_REQUEST:
I think we should have UMA for this, but it could be in a different patch.

https://codereview.chromium.org/2166523003/diff/200001/content/browser/servic...
File content/browser/service_worker/service_worker_metrics.h (right):

https://codereview.chromium.org/2166523003/diff/200001/content/browser/servic...
content/browser/service_worker/service_worker_metrics.h:113: // Used when
external consumers wants to add a request to
nit: "an external consumer" or "want to"

[michaeln, 2016-09-24 01:46:51]

looks nice, are we sure about the raw sw_context ptr?

https://codereview.chromium.org/2166523003/diff/200001/content/public/browser...
File content/public/browser/service_worker_context.h (right):

https://codereview.chromium.org/2166523003/diff/200001/content/public/browser...
content/public/browser/service_worker_context.h:21: class StoragePartition;
no longer needed?

https://codereview.chromium.org/2166523003/diff/200001/extensions/browser/ext...
File extensions/browser/extension_function.cc (right):

https://codereview.chromium.org/2166523003/diff/200001/extensions/browser/ext...
extensions/browser/extension_function.cc:528: bool
UIThreadExtensionFunction::IsFromServiceWorker() const {
style nit: maybe inline and rename is_from_service_worker()

https://codereview.chromium.org/2166523003/diff/200001/extensions/browser/ext...
File extensions/browser/extension_function_dispatcher.cc (right):

https://codereview.chromium.org/2166523003/diff/200001/extensions/browser/ext...
extensions/browser/extension_function_dispatcher.cc:586: // on IO thread
directly via IPC.
a comment like this at the Inc callsite might be nice

[lazyboy, 2016-09-27 21:26:45]

re: raw ServiceWorkerContext ptr in ExtensionServiceWorkerMessageFilter: I've
seen in other places where this is done, e.g. ChromeRenderMessageFilter. This
ptr comes from the profile's storage-partition.
ExtensionServiceWorkerMessageFilter tied to the render_process +
browser_context. So the pointer should outlive the filter class, no? The refptr
style is only do-able inside content/ (via ServiceWorkerContextWrapper), if
that's what you mean.

https://codereview.chromium.org/2166523003/diff/200001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/service_worker/tabs_create/sw.js
(right):

https://codereview.chromium.org/2166523003/diff/200001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/service_worker/tabs_create/sw.js:10:
console.log('got id: ' + id);
On 2016/09/23 02:27:35, falken wrote:
> debug output?

Removed.

https://codereview.chromium.org/2166523003/diff/200001/content/browser/servic...
File content/browser/service_worker/service_worker_context_wrapper.cc (right):

https://codereview.chromium.org/2166523003/diff/200001/content/browser/servic...
content/browser/service_worker/service_worker_context_wrapper.cc:60: void
DoNothing(ServiceWorkerStatusCode status) {}
On 2016/09/23 02:27:35, falken wrote:
> you can use ServiceWorkerUtils::NoOpStatusCallback

Done.

https://codereview.chromium.org/2166523003/diff/200001/content/browser/servic...
content/browser/service_worker/service_worker_context_wrapper.cc:110: return
IncrementPendingActivityOnIO(service_worker_version_id);
On 2016/09/23 02:27:35, falken wrote:
> Can the *OnIO functions just be removed?

Oh yes, these used to do thread hops, done.

https://codereview.chromium.org/2166523003/diff/200001/content/browser/servic...
content/browser/service_worker/service_worker_context_wrapper.cc:846: if
(!version) {
On 2016/09/23 02:27:35, falken wrote:
> You should also confirm the version is RUNNING. It's possible the renderer is
> confused/lying or the version started stopping right around the time of the
IPC.

Done.

https://codereview.chromium.org/2166523003/diff/200001/content/browser/servic...
content/browser/service_worker/service_worker_context_wrapper.cc:848: <<
service_worker_version_id;
On 2016/09/23 02:27:35, falken wrote:
> do you need these logs in production code? DVLOG(1)?

No, changed to DVLOG, thanks.

https://codereview.chromium.org/2166523003/diff/200001/content/browser/servic...
content/browser/service_worker/service_worker_context_wrapper.cc:863: if
(!version) {
On 2016/09/23 02:27:35, falken wrote:
> I'm not too sure when we get in this state: the renderer might have made a
> mistake or the version might have been destructed due to some timing issue. 
Changed to DCHECK. This could uncover any timing issue we might have...

We
> should probably clean up the pending_external_requests  in this case though.
Done.

Or
> maybe the set of external requests should just be in ServiceWorkerVersion?
> 
> Also, the case where the version is not RUNNING should probably be handled.
Done.

https://codereview.chromium.org/2166523003/diff/200001/content/browser/servic...
content/browser/service_worker/service_worker_context_wrapper.cc:874: return
true;
On 2016/09/23 02:27:35, falken wrote:
> This ignores FinishRequest's return value. Maybe it should be used?

Done.

https://codereview.chromium.org/2166523003/diff/200001/content/browser/servic...
File content/browser/service_worker/service_worker_metrics.cc (right):

https://codereview.chromium.org/2166523003/diff/200001/content/browser/servic...
content/browser/service_worker/service_worker_metrics.cc:572: case
EventType::EXTERNAL_REQUEST:
On 2016/09/23 02:27:35, falken wrote:
> I think we should have UMA for this, but it could be in a different patch.

Added TODO.

https://codereview.chromium.org/2166523003/diff/200001/content/browser/servic...
File content/browser/service_worker/service_worker_metrics.h (right):

https://codereview.chromium.org/2166523003/diff/200001/content/browser/servic...
content/browser/service_worker/service_worker_metrics.h:113: // Used when
external consumers wants to add a request to
On 2016/09/23 02:27:35, falken wrote:
> nit: "an external consumer" or "want to"

Done.

https://codereview.chromium.org/2166523003/diff/200001/content/public/browser...
File content/public/browser/service_worker_context.h (right):

https://codereview.chromium.org/2166523003/diff/200001/content/public/browser...
content/public/browser/service_worker_context.h:21: class StoragePartition;
On 2016/09/24 01:46:51, michaeln wrote:
> no longer needed?

Done.

https://codereview.chromium.org/2166523003/diff/200001/extensions/browser/ext...
File extensions/browser/extension_function.cc (right):

https://codereview.chromium.org/2166523003/diff/200001/extensions/browser/ext...
extensions/browser/extension_function.cc:528: bool
UIThreadExtensionFunction::IsFromServiceWorker() const {
On 2016/09/24 01:46:51, michaeln wrote:
> style nit: maybe inline and rename is_from_service_worker()

Done.

https://codereview.chromium.org/2166523003/diff/200001/extensions/browser/ext...
File extensions/browser/extension_function_dispatcher.cc (right):

https://codereview.chromium.org/2166523003/diff/200001/extensions/browser/ext...
extensions/browser/extension_function_dispatcher.cc:586: // on IO thread
directly via IPC.
On 2016/09/24 01:46:51, michaeln wrote:
> a comment like this at the Inc callsite might be nice

Done.

[falken, 2016-09-28 13:40:21]

I think you forgot to upload the new patch.

Is there any bug you can put in the BUG= line?

[lazyboy, 2016-09-28 17:18:06]

Description was changed from

==========
WIP: Attempt to add ref count to service workers for extension API.

We need a way to keep a service worker alive
1) during extension function's request->response roundtrip completes.
2) when an event is about to be dispatched to a (stopped) service worker.

This CL shows a way to do #1. #2 can follow later.
The CL adds plumbing to expose functions to increment/decrement ref
counting to an EmbeddedWorkerInstance.

BUG=
Test=ServiceWorkerTest.TabsCreate in this CL shows a proof of concept.
Both the stop worker idle timeout and worker timeout timout is reduced to 3s.
The test calls chrome.tabs.update() function and the function's implemention is
modified to return after 7s delay (> 3s + 3s). When chrome.tabs.update() is
inflight, it increments the worker's ref count. The worker won't be stopped
because of the ref count (it will bail out from StopWorkerIfIdle()).
==========

to

==========
WIP: Attempt to add ref count to service workers for extension API.

We need a way to keep a service worker alive
1) during extension function's request->response roundtrip completes.
2) when an event is about to be dispatched to a (stopped) service worker.

This CL shows a way to do #1. #2 can follow later.
The CL adds plumbing to expose functions to increment/decrement ref
counting to an EmbeddedWorkerInstance.

BUG=602442
Test=ServiceWorkerTest.TabsCreate in this CL shows a proof of concept.
Both the stop worker idle timeout and worker timeout timout is reduced to 3s.
The test calls chrome.tabs.update() function and the function's implemention is
modified to return after 7s delay (> 3s + 3s). When chrome.tabs.update() is
inflight, it increments the worker's ref count. The worker won't be stopped
because of the ref count (it will bail out from StopWorkerIfIdle()).
==========

[lazyboy, 2016-09-28 17:18:34]

On 2016/09/28 13:40:21, falken (ooo Sep 29) wrote:
> I think you forgot to upload the new patch.
> 
> Is there any bug you can put in the BUG= line?

You're right :(, uploaded patch set #10.
Updated BUG=

[falken, 2016-09-29 14:52:25]

https://codereview.chromium.org/2166523003/diff/220001/content/browser/servic...
File content/browser/service_worker/service_worker_context_wrapper.cc (right):

https://codereview.chromium.org/2166523003/diff/220001/content/browser/servic...
content/browser/service_worker/service_worker_context_wrapper.cc:845:
base::Bind(&ServiceWorkerUtils::NoOpStatusCallback));
Are you sure you don't need to use the error callback? It gets called if the
worker was stopped for whatever reason while we were trying to keep it alive.
Does the renderer always call DecrementPendingActivity to balance out the
IncrementPendingActivity, even if the worker died? If not, one potentially bad
situation is:
(1) IncrementPendingActivity
(2) worker stops, requests are cancelled
(3) maybe sometime later we try again
(4) IncrementPendingActivity
(5) DecrementPendingActivity

The DecrementPendingActivity call would use the old request_id in
pending_external_requests_, but that would have no effect. Eventually the real
request would timeout and the worker gets terminated, and again
pending_external_requests_ holds an obsolete id.

Since the renderer could always crash leaving an Increment call unbalanced, I
guess it's possible this situation can occur.

https://codereview.chromium.org/2166523003/diff/220001/content/browser/servic...
content/browser/service_worker/service_worker_context_wrapper.cc:859: int
request_id = *pending_requests.begin();
It seems like pending_requests should just be a queue instead of a set. It looks
like we're subtly depending on requests ids to be monotonically increasing here,
in order for the oldest pending request to be the one removed. This can matter
due to the timeout mechanism: long-running requests will be timed out and the
service worker terminated.

https://codereview.chromium.org/2166523003/diff/220001/content/browser/servic...
File content/browser/service_worker/service_worker_metrics.h (right):

https://codereview.chromium.org/2166523003/diff/220001/content/browser/servic...
content/browser/service_worker/service_worker_metrics.h:83: // Used for UMA.
Append-only.
Even though you dont add the UMA here, I think it's safer to add the new type to
histograms.xml to keep it in sync.

https://codereview.chromium.org/2166523003/diff/220001/content/public/browser...
File content/public/browser/service_worker_context.h (right):

https://codereview.chromium.org/2166523003/diff/220001/content/public/browser...
content/public/browser/service_worker_context.h:64: virtual bool
DecrementPendingActivity(int64_t service_worker_version_id) = 0;
Could you add brief comments including that these must be called on IO thread?

[lazyboy, 2016-09-29 18:07:20]

https://codereview.chromium.org/2166523003/diff/220001/content/browser/servic...
File content/browser/service_worker/service_worker_context_wrapper.cc (right):

https://codereview.chromium.org/2166523003/diff/220001/content/browser/servic...
content/browser/service_worker/service_worker_context_wrapper.cc:845:
base::Bind(&ServiceWorkerUtils::NoOpStatusCallback));
On 2016/09/29 14:52:24, falken (ooo Sep 29) wrote:
> Are you sure you don't need to use the error callback? It gets called if the
> worker was stopped for whatever reason while we were trying to keep it alive.
> Does the renderer always call DecrementPendingActivity to balance out the
> IncrementPendingActivity, even if the worker died? If not, one potentially bad
> situation is:
> (1) IncrementPendingActivity
> (2) worker stops, requests are cancelled
> (3) maybe sometime later we try again
> (4) IncrementPendingActivity
> (5) DecrementPendingActivity
> 
> The DecrementPendingActivity call would use the old request_id in
> pending_external_requests_, but that would have no effect. Eventually the real
> request would timeout and the worker gets terminated, and again
> pending_external_requests_ holds an obsolete id.
> 
> Since the renderer could always crash leaving an Increment call unbalanced, I
> guess it's possible this situation can occur.

(I was thinking that since the worker is running, StartRequest won't fail.)

Added cleaning up |pending_external_requests_| on error_callback.

https://codereview.chromium.org/2166523003/diff/220001/content/browser/servic...
content/browser/service_worker/service_worker_context_wrapper.cc:859: int
request_id = *pending_requests.begin();
On 2016/09/29 14:52:24, falken (ooo Sep 29) wrote:
> It seems like pending_requests should just be a queue instead of a set.
Done.

 It looks
> like we're subtly depending on requests ids to be monotonically increasing
here,
> in order for the oldest pending request to be the one removed. This can matter
> due to the timeout mechanism: long-running requests will be timed out and the
> service worker terminated.

https://codereview.chromium.org/2166523003/diff/220001/content/browser/servic...
File content/browser/service_worker/service_worker_metrics.h (right):

https://codereview.chromium.org/2166523003/diff/220001/content/browser/servic...
content/browser/service_worker/service_worker_metrics.h:83: // Used for UMA.
Append-only.
On 2016/09/29 14:52:25, falken (ooo Sep 29) wrote:
> Even though you dont add the UMA here, I think it's safer to add the new type
to
> histograms.xml to keep it in sync.

Done.

https://codereview.chromium.org/2166523003/diff/220001/content/public/browser...
File content/public/browser/service_worker_context.h (right):

https://codereview.chromium.org/2166523003/diff/220001/content/public/browser...
content/public/browser/service_worker_context.h:64: virtual bool
DecrementPendingActivity(int64_t service_worker_version_id) = 0;
On 2016/09/29 14:52:25, falken (ooo Sep 29) wrote:
> Could you add brief comments including that these must be called on IO thread?

Done.

[lazyboy, 2016-09-29 21:43:44]

@michaeln, I totally missed giving the other example of raw SWContext pointer
:(, it's in BrowsingDataServiceWorkerHelper:
https://cs.chromium.org/chromium/src/chrome/browser/browsing_data/browsing_da...
this one is used in IO thread too.

@falken, can you check the ServiceWorkerContext* member in
ExtensionServiceWorkerMessageFilter and see if that might lead to potential bad
pointer access?

[falken, 2016-09-30 01:48:56]

Sorry I have mostly scattered thoughts, I think there's still some timing issues
here.

https://codereview.chromium.org/2166523003/diff/240001/content/browser/servic...
File content/browser/service_worker/service_worker_context_wrapper.cc (right):

https://codereview.chromium.org/2166523003/diff/240001/content/browser/servic...
content/browser/service_worker/service_worker_context_wrapper.cc:25: #include
"content/browser/service_worker/embedded_worker_registry.h"
Is this used? Can it just be embedded_worker_status.h?

https://codereview.chromium.org/2166523003/diff/240001/content/browser/servic...
content/browser/service_worker/service_worker_context_wrapper.cc:385:
pending_external_requests_[service_worker_version_id].pop();
Hmm... it might be slightly safer to bind the request_id to this function and
then delete it specifically. But I guess it doesn't matter since this callback
is called for each pending external request. WDYT?

Also, now there's a new issue: the renderer doesn't know that the request got
cancelled and removed, so it might still call DecrementPendingActivity. This is
getting tricky.

I haven't thought things through but maybe a nonce could be generated on the
renderer-side for each pending activity, that get mapped to a request id on
browser-side. Or the current patch is basically okay and just silently dropping
DecrementPendingActivity when there are no pending requests will eventually lead
to a stable state again.

https://codereview.chromium.org/2166523003/diff/240001/content/browser/servic...
content/browser/service_worker/service_worker_context_wrapper.cc:843: return
false;
Sorry I didn't suggest this sooner. Consider having these unexpected cases in
Increment/DecrementPendingActivity lead to a BadMessageReceived (at
ExtensionServiceWorkerMessageFilter). That way we get a crash report and you
don't need to do any special error handling at the callsite. And you might not
need the DVLOGs or DCHECKs or NOTREACHEDs here.

https://codereview.chromium.org/2166523003/diff/240001/content/browser/servic...
content/browser/service_worker/service_worker_context_wrapper.cc:866:
DCHECK(pending_requests.size() > 0);
Now that requests can be removed by the error callback, it might be best to
silently ignore invalid DecrementPendingActivity calls. We've ran into issues
before where we used to BadMessageReceived when receiving a response for a timed
out request: https://codereview.chromium.org/2150863002

https://codereview.chromium.org/2166523003/diff/240001/extensions/browser/ext...
File extensions/browser/extension_service_worker_message_filter.cc (right):

https://codereview.chromium.org/2166523003/diff/240001/extensions/browser/ext...
extensions/browser/extension_service_worker_message_filter.cc:57:
service_worker_context_->IncrementPendingActivity(service_worker_version_id);
Check return val and BadMessageReceieved?

[lazyboy, 2016-09-30 08:02:16]

I'm liking the nonce/GUID -> request id idea much better now, thanks for the
suggestions/reviews so far. See patch set #12.

https://codereview.chromium.org/2166523003/diff/240001/content/browser/servic...
File content/browser/service_worker/service_worker_context_wrapper.cc (right):

https://codereview.chromium.org/2166523003/diff/240001/content/browser/servic...
content/browser/service_worker/service_worker_context_wrapper.cc:25: #include
"content/browser/service_worker/embedded_worker_registry.h"
On 2016/09/30 01:48:55, falken (ooo Sep 29) wrote:
> Is this used? Can it just be embedded_worker_status.h?

Thanks.
Done.

https://codereview.chromium.org/2166523003/diff/240001/content/browser/servic...
content/browser/service_worker/service_worker_context_wrapper.cc:385:
pending_external_requests_[service_worker_version_id].pop();
On 2016/09/30 01:48:55, falken (ooo Sep 29) wrote:
> Hmm... it might be slightly safer to bind the request_id to this function and
> then delete it specifically. 
It won't be possible to do this without changing StartRequest code, because
StartRequest
returns the request id (and this is a param to StartRequest).

But I guess it doesn't matter since this callback
> is called for each pending external request. WDYT?
> 
> Also, now there's a new issue: the renderer doesn't know that the request got
> cancelled and removed, so it might still call DecrementPendingActivity. This
is
> getting tricky.
> 
> I haven't thought things through but maybe a nonce could be generated on the
> renderer-side for each pending activity,
This seems like safer alternative, I've used GUID for this, let's see how we
feel about this.
We should be able to reason about a DecrementPendingActivity better now given
the
request_uuid.
 that get mapped to a request id on
> browser-side. Or the current patch is basically okay and just silently
dropping
> DecrementPendingActivity when there are no pending requests will eventually
lead
> to a stable state again.

https://codereview.chromium.org/2166523003/diff/240001/content/browser/servic...
content/browser/service_worker/service_worker_context_wrapper.cc:843: return
false;
On 2016/09/30 01:48:55, falken (ooo Sep 29) wrote:
> Sorry I didn't suggest this sooner. Consider having these unexpected cases in
> Increment/DecrementPendingActivity lead to a BadMessageReceived (at
> ExtensionServiceWorkerMessageFilter). That way we get a crash report and you
> don't need to do any special error handling at the callsite. And you might not
> need the DVLOGs or DCHECKs or NOTREACHEDs here.

Done.

https://codereview.chromium.org/2166523003/diff/240001/content/browser/servic...
content/browser/service_worker/service_worker_context_wrapper.cc:866:
DCHECK(pending_requests.size() > 0);
On 2016/09/30 01:48:55, falken (ooo Sep 29) wrote:
> Now that requests can be removed by the error callback, it might be best to
> silently ignore invalid DecrementPendingActivity calls. We've ran into issues
> before where we used to BadMessageReceived when receiving a response for a
timed
> out request: https://codereview.chromium.org/2150863002 

Slightly modified code now, but done.

https://codereview.chromium.org/2166523003/diff/240001/extensions/browser/ext...
File extensions/browser/extension_service_worker_message_filter.cc (right):

https://codereview.chromium.org/2166523003/diff/240001/extensions/browser/ext...
extensions/browser/extension_service_worker_message_filter.cc:57:
service_worker_context_->IncrementPendingActivity(service_worker_version_id);
On 2016/09/30 01:48:56, falken (ooo Sep 29) wrote:
> Check return val and BadMessageReceieved?

Done.

[falken, 2016-10-03 04:20:31]

service worker LGTM!

> @falken, can you check the ServiceWorkerContext* member in
ExtensionServiceWorkerMessageFilter and see if that might lead to potential bad
pointer access?

Not 100% certain, but since SWContext is tied to the StoragePartition which is
tied to the Profile, and ExtensionSWMessageFilter is tied to the
RenderProcessHost, I think it's OK since RPH is expected to outlive the Profile.

https://codereview.chromium.org/2166523003/diff/260001/content/browser/servic...
File content/browser/service_worker/service_worker_context_wrapper.h (right):

https://codereview.chromium.org/2166523003/diff/260001/content/browser/servic...
content/browser/service_worker/service_worker_context_wrapper.h:269: // Returns
true if the request was pending. Also returns |request_id| in that
"pending" is a bit confusing since by definition in the first sentence it's a
pending request. Maybe "if the request was found"?

[falken, 2016-10-03 04:21:05]

> Not 100% certain, but since SWContext is tied to the StoragePartition which is
> tied to the Profile, and ExtensionSWMessageFilter is tied to the
> RenderProcessHost, I think it's OK since RPH is expected to outlive the
Profile.

Err.. Profile is expected to outlive the RPH.

[lazyboy, 2016-10-03 21:39:53]

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

[commit-bot: I haz the power, 2016-10-03 21:40:35]

Dry run: 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, 2016-10-04 00:14:10]

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

[commit-bot: I haz the power, 2016-10-04 00:14:12]

Dry run: This issue passed the CQ dry run.

[lazyboy, 2016-10-04 02:41:56]

Description was changed from

==========
WIP: Attempt to add ref count to service workers for extension API.

We need a way to keep a service worker alive
1) during extension function's request->response roundtrip completes.
2) when an event is about to be dispatched to a (stopped) service worker.

This CL shows a way to do #1. #2 can follow later.
The CL adds plumbing to expose functions to increment/decrement ref
counting to an EmbeddedWorkerInstance.

BUG=602442
Test=ServiceWorkerTest.TabsCreate in this CL shows a proof of concept.
Both the stop worker idle timeout and worker timeout timout is reduced to 3s.
The test calls chrome.tabs.update() function and the function's implemention is
modified to return after 7s delay (> 3s + 3s). When chrome.tabs.update() is
inflight, it increments the worker's ref count. The worker won't be stopped
because of the ref count (it will bail out from StopWorkerIfIdle()).
==========

to

==========
Add ref count to service workers for extension API.

We need a way to keep a service worker alive
1) during extension function's request->response roundtrip completes.
2) when an event is about to be dispatched to a (stopped) service worker.

This CL shows a way to do #1. #2 can follow later.
The CL adds plumbing to expose functions to increment/decrement ref
counting to an EmbeddedWorkerInstance.

BUG=602442
Test=ServiceWorkerTest.TabsCreate in this CL shows a proof of concept.
Both the stop worker idle timeout and worker timeout timout is reduced to 3s.
The test calls chrome.tabs.update() function and the function's implemention is
modified to return after 7s delay (> 3s + 3s). When chrome.tabs.update() is
inflight, it increments the worker's ref count. The worker won't be stopped
because of the ref count (it will bail out from StopWorkerIfIdle()).
==========

[lazyboy, 2016-10-04 02:42:51]

lazyboy@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[lazyboy, 2016-10-04 02:42:52]

+Devlin for *extensions* review.

https://codereview.chromium.org/2166523003/diff/260001/content/browser/servic...
File content/browser/service_worker/service_worker_context_wrapper.h (right):

https://codereview.chromium.org/2166523003/diff/260001/content/browser/servic...
content/browser/service_worker/service_worker_context_wrapper.h:269: // Returns
true if the request was pending. Also returns |request_id| in that
On 2016/10/03 04:20:31, falken wrote:
> "pending" is a bit confusing since by definition in the first sentence it's a
> pending request. Maybe "if the request was found"?

Done.

[Devlin, 2016-10-05 17:02:26]

https://codereview.chromium.org/2166523003/diff/280001/chrome/browser/extensi...
File chrome/browser/extensions/service_worker_apitest.cc (right):

https://codereview.chromium.org/2166523003/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/service_worker_apitest.cc:625:
ASSERT_EQ("chrome.tabs.update callback", result);
I'm a little confused on the test for this.  Where do we adjust the timeout, or
ensure the worker would otherwise be terminated?

https://codereview.chromium.org/2166523003/diff/280001/content/browser/servic...
File content/browser/service_worker/service_worker_context_wrapper.cc (right):

https://codereview.chromium.org/2166523003/diff/280001/content/browser/servic...
content/browser/service_worker/service_worker_context_wrapper.cc:395: int*
request_id) {
drive-by nit: DCHECK(request_id)?

https://codereview.chromium.org/2166523003/diff/280001/content/browser/servic...
content/browser/service_worker/service_worker_context_wrapper.cc:862: if
(version->running_status() != EmbeddedWorkerStatus::RUNNING)
Can this legitimately happen?  If so, how?  (Maybe add a comment?)  If not,
(D)CHECK?

https://codereview.chromium.org/2166523003/diff/280001/content/browser/servic...
content/browser/service_worker/service_worker_context_wrapper.cc:891: if
(!version || version->running_status() != EmbeddedWorkerStatus::RUNNING)
ditto

https://codereview.chromium.org/2166523003/diff/280001/content/browser/servic...
File content/browser/service_worker/service_worker_context_wrapper.h (right):

https://codereview.chromium.org/2166523003/diff/280001/content/browser/servic...
content/browser/service_worker/service_worker_context_wrapper.h:264: void
ExternalRequestErrorCallback(int64_t service_worker_version_id,
drive-by: function comment?

https://codereview.chromium.org/2166523003/diff/280001/extensions/browser/ext...
File extensions/browser/extension_function_dispatcher.cc (right):

https://codereview.chromium.org/2166523003/diff/280001/extensions/browser/ext...
extensions/browser/extension_function_dispatcher.cc:68: return
content::kInvalidServiceWorkerVersionId !=
nit: prefer inverting these

https://codereview.chromium.org/2166523003/diff/280001/extensions/browser/ext...
extensions/browser/extension_function_dispatcher.cc:432:
content::kInvalidServiceWorkerVersionId);
nit: invert

https://codereview.chromium.org/2166523003/diff/280001/extensions/browser/ext...
extensions/browser/extension_function_dispatcher.cc:551: if
(IsRequestFromServiceWorker(params)) {
Similar comment to line 583:
if (!IsRequestFromSW()) {
  // Increment ref count for non-service worker extension API. Ref counts for
service...
}

https://codereview.chromium.org/2166523003/diff/280001/extensions/browser/ext...
extensions/browser/extension_function_dispatcher.cc:581: int64_t
service_worker_version_id) {
Perhaps just pass in bool is_from_service_worker, since we don't need the id?

https://codereview.chromium.org/2166523003/diff/280001/extensions/browser/ext...
extensions/browser/extension_function_dispatcher.cc:583: if
(service_worker_version_id == content::kInvalidServiceWorkerVersionId) {
I think this is cleaner as:
if (extension && !is_from_service_worker) {
  // Decrement ref count for non-service worker extension API. Service worker
ref counts
  // are handled separately on the IO thread directly via IPC.
}

https://codereview.chromium.org/2166523003/diff/280001/extensions/browser/ext...
File extensions/browser/extension_util.cc (right):

https://codereview.chromium.org/2166523003/diff/280001/extensions/browser/ext...
extensions/browser/extension_util.cc:19: GURL GetSiteForExtensionId(const
std::string& extension_id,
nit: if this is only used here, inline it.

https://codereview.chromium.org/2166523003/diff/280001/extensions/browser/ext...
extensions/browser/extension_util.cc:65: DCHECK(storage_partition);
seems like this should be relaxed if the extension e.g. isn't enabled.

[michaeln, 2016-10-05 19:22:53]

https://codereview.chromium.org/2166523003/diff/280001/content/browser/servic...
File content/browser/service_worker/service_worker_context_wrapper.cc (right):

https://codereview.chromium.org/2166523003/diff/280001/content/browser/servic...
content/browser/service_worker/service_worker_context_wrapper.cc:401:
*request_id = iter->second;
where is .erase(iter)? i don't see where anything is removed from the inner or
outer maps.

https://codereview.chromium.org/2166523003/diff/280001/content/browser/servic...
content/browser/service_worker/service_worker_context_wrapper.cc:865: int
request_id = version->StartRequest(
version->StartExternalRequest(uuid);  // no error callback required

the version deals with the book keeping, it might not even need a separate
mapping, it could iterate it's pending_requests_ map and find the EXTERNAL
reques with the matching uuid when FinishExternalRequest is called.

https://codereview.chromium.org/2166523003/diff/280001/content/browser/servic...
content/browser/service_worker/service_worker_context_wrapper.cc:894: return
version->FinishRequest(request_id, true, base::Time::Now());
version->FinishExternalRequest(uuid);

https://codereview.chromium.org/2166523003/diff/280001/content/browser/servic...
File content/browser/service_worker/service_worker_context_wrapper.h (right):

https://codereview.chromium.org/2166523003/diff/280001/content/browser/servic...
content/browser/service_worker/service_worker_context_wrapper.h:203: friend
class ServiceWorkerContext;
is this needed?

https://codereview.chromium.org/2166523003/diff/280001/content/browser/servic...
content/browser/service_worker/service_worker_context_wrapper.h:303:
std::map<int64_t, RequestUUIDToRequestID> pending_external_requests_;
Please consider moving this mapping of external_request_uuid to
internal_integer_id into the ServiceWorkerVersion class proper, and making that
class be responsible for cleanup in all cases, whether it be request completes
or the renderer crashes for any reason the Version goes away.

aside: The original intent of the wrapper was to contain as little logic as
possible and just to provide 'shimware' from UI thread to IO thread for the
content lib internals, and for shimware bewteen public content api and the
actual impl (this cl's use case).

[lazyboy, 2016-10-06 01:02:07]

See patchset #14

https://codereview.chromium.org/2166523003/diff/280001/chrome/browser/extensi...
File chrome/browser/extensions/service_worker_apitest.cc (right):

https://codereview.chromium.org/2166523003/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/service_worker_apitest.cc:625:
ASSERT_EQ("chrome.tabs.update callback", result);
On 2016/10/05 17:02:25, Devlin wrote:
> I'm a little confused on the test for this.  Where do we adjust the timeout,
or
> ensure the worker would otherwise be terminated?

We don't.
I only tested them with hacking the timeout values in service worker and adding
artificial delay to update response. As is, it's hard to to test this. I hope to
write some unit tests later on.

https://codereview.chromium.org/2166523003/diff/280001/content/browser/servic...
File content/browser/service_worker/service_worker_context_wrapper.cc (right):

https://codereview.chromium.org/2166523003/diff/280001/content/browser/servic...
content/browser/service_worker/service_worker_context_wrapper.cc:395: int*
request_id) {
On 2016/10/05 17:02:25, Devlin wrote:
> drive-by nit: DCHECK(request_id)?

Obsolete /w the recent change suggested by michaeln@

https://codereview.chromium.org/2166523003/diff/280001/content/browser/servic...
content/browser/service_worker/service_worker_context_wrapper.cc:401:
*request_id = iter->second;
On 2016/10/05 19:22:53, michaeln wrote:
> where is .erase(iter)? i don't see where anything is removed from the inner or
> outer maps.

Good catch, I might have dropped removing when uploading new patchsets.
Should be fixed with new approach.

https://codereview.chromium.org/2166523003/diff/280001/content/browser/servic...
content/browser/service_worker/service_worker_context_wrapper.cc:862: if
(version->running_status() != EmbeddedWorkerStatus::RUNNING)
On 2016/10/05 17:02:25, Devlin wrote:
> Can this legitimately happen?  If so, how?  (Maybe add a comment?)  If not,
> (D)CHECK?

Added comment.

https://codereview.chromium.org/2166523003/diff/280001/content/browser/servic...
content/browser/service_worker/service_worker_context_wrapper.cc:865: int
request_id = version->StartRequest(
On 2016/10/05 19:22:53, michaeln wrote:
> version->StartExternalRequest(uuid);  // no error callback required
> 
> the version deals with the book keeping, it might not even need a separate
> mapping, it could iterate it's pending_requests_ map and find the EXTERNAL
> reques with the matching uuid when FinishExternalRequest is called.

OK.
Moved the map inside ServiceWorkerVersion and exposed
bool StartExternalRequest and
bool FinishExternalRequest.

https://codereview.chromium.org/2166523003/diff/280001/content/browser/servic...
content/browser/service_worker/service_worker_context_wrapper.cc:891: if
(!version || version->running_status() != EmbeddedWorkerStatus::RUNNING)
On 2016/10/05 17:02:25, Devlin wrote:
> ditto

Done.

https://codereview.chromium.org/2166523003/diff/280001/content/browser/servic...
content/browser/service_worker/service_worker_context_wrapper.cc:894: return
version->FinishRequest(request_id, true, base::Time::Now());
On 2016/10/05 19:22:53, michaeln wrote:
> version->FinishExternalRequest(uuid);

Done.

https://codereview.chromium.org/2166523003/diff/280001/content/browser/servic...
File content/browser/service_worker/service_worker_context_wrapper.h (right):

https://codereview.chromium.org/2166523003/diff/280001/content/browser/servic...
content/browser/service_worker/service_worker_context_wrapper.h:203: friend
class ServiceWorkerContext;
On 2016/10/05 19:22:53, michaeln wrote:
> is this needed?

Not anymore, removed.

https://codereview.chromium.org/2166523003/diff/280001/content/browser/servic...
content/browser/service_worker/service_worker_context_wrapper.h:264: void
ExternalRequestErrorCallback(int64_t service_worker_version_id,
On 2016/10/05 17:02:25, Devlin wrote:
> drive-by: function comment?

Obsolete now with new patch.

https://codereview.chromium.org/2166523003/diff/280001/content/browser/servic...
content/browser/service_worker/service_worker_context_wrapper.h:303:
std::map<int64_t, RequestUUIDToRequestID> pending_external_requests_;
On 2016/10/05 19:22:53, michaeln wrote:
> Please consider moving this mapping of external_request_uuid to
> internal_integer_id into the ServiceWorkerVersion class proper, and making
that
> class be responsible for cleanup in all cases, whether it be request completes
> or the renderer crashes for any reason the Version goes away.
> 
> aside: The original intent of the wrapper was to contain as little logic as
> possible and just to provide 'shimware' from UI thread to IO thread for the
> content lib internals, and for shimware bewteen public content api and the
> actual impl (this cl's use case).

Done.

https://codereview.chromium.org/2166523003/diff/280001/extensions/browser/ext...
File extensions/browser/extension_function_dispatcher.cc (right):

https://codereview.chromium.org/2166523003/diff/280001/extensions/browser/ext...
extensions/browser/extension_function_dispatcher.cc:68: return
content::kInvalidServiceWorkerVersionId !=
On 2016/10/05 17:02:25, Devlin wrote:
> nit: prefer inverting these

Done.

https://codereview.chromium.org/2166523003/diff/280001/extensions/browser/ext...
extensions/browser/extension_function_dispatcher.cc:432:
content::kInvalidServiceWorkerVersionId);
On 2016/10/05 17:02:25, Devlin wrote:
> nit: invert

Done.

https://codereview.chromium.org/2166523003/diff/280001/extensions/browser/ext...
extensions/browser/extension_function_dispatcher.cc:551: if
(IsRequestFromServiceWorker(params)) {
On 2016/10/05 17:02:25, Devlin wrote:
> Similar comment to line 583:
> if (!IsRequestFromSW()) {
>   // Increment ref count for non-service worker extension API. Ref counts for
> service...
> }

Done.

https://codereview.chromium.org/2166523003/diff/280001/extensions/browser/ext...
extensions/browser/extension_function_dispatcher.cc:581: int64_t
service_worker_version_id) {
On 2016/10/05 17:02:25, Devlin wrote:
> Perhaps just pass in bool is_from_service_worker, since we don't need the id?

Done.

https://codereview.chromium.org/2166523003/diff/280001/extensions/browser/ext...
extensions/browser/extension_function_dispatcher.cc:583: if
(service_worker_version_id == content::kInvalidServiceWorkerVersionId) {
On 2016/10/05 17:02:25, Devlin wrote:
> I think this is cleaner as:
> if (extension && !is_from_service_worker) {
>   // Decrement ref count for non-service worker extension API. Service worker
> ref counts
>   // are handled separately on the IO thread directly via IPC.
> }

Done.

https://codereview.chromium.org/2166523003/diff/280001/extensions/browser/ext...
File extensions/browser/extension_util.cc (right):

https://codereview.chromium.org/2166523003/diff/280001/extensions/browser/ext...
extensions/browser/extension_util.cc:19: GURL GetSiteForExtensionId(const
std::string& extension_id,
On 2016/10/05 17:02:25, Devlin wrote:
> nit: if this is only used here, inline it.

Done.

https://codereview.chromium.org/2166523003/diff/280001/extensions/browser/ext...
extensions/browser/extension_util.cc:65: DCHECK(storage_partition);
On 2016/10/05 17:02:25, Devlin wrote:
> seems like this should be relaxed if the extension e.g. isn't enabled.

Done.

[Devlin, 2016-10-06 16:35:12]

Lifetime question: What's to prevent the flow of

Renderer sends SW request IPC
Browser shuts down renderer
IPC arrives at browser, browser tries to increment

Presumably this is just a fact of life in service workers (and event pages, for
that matter), but in the current implementation it seems like the browser would
then kill that renderer process for a bad message?

https://codereview.chromium.org/2166523003/diff/280001/chrome/browser/extensi...
File chrome/browser/extensions/service_worker_apitest.cc (right):

https://codereview.chromium.org/2166523003/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/service_worker_apitest.cc:625:
ASSERT_EQ("chrome.tabs.update callback", result);
On 2016/10/06 01:02:06, lazyboy wrote:
> On 2016/10/05 17:02:25, Devlin wrote:
> > I'm a little confused on the test for this.  Where do we adjust the timeout,
> or
> > ensure the worker would otherwise be terminated?
> 
> We don't.
> I only tested them with hacking the timeout values in service worker and
adding
> artificial delay to update response. As is, it's hard to to test this. I hope
to
> write some unit tests later on.

Can we add an explicit TODO?  And out of curiosity, what are the main
complications?

https://codereview.chromium.org/2166523003/diff/300001/content/browser/servic...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/2166523003/diff/300001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:569: // It's possible
that the renderer/ is confused/lying or the version started
nit: renderer/ is a file path, renderer is a process.  I think here we want the
latter.
nit #2: "confused" seems like something that shouldn't happen (lying can in the
case of compromised renderers)

https://codereview.chromium.org/2166523003/diff/300001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:608: // It's possible
that the renderer/ is confused/lying or the version started
ditto

https://codereview.chromium.org/2166523003/diff/300001/extensions/DEPS
File extensions/DEPS (right):

https://codereview.chromium.org/2166523003/diff/300001/extensions/DEPS#newcode10
extensions/DEPS:10: "+content/common",
This won't be allowed.

https://codereview.chromium.org/2166523003/diff/300001/extensions/browser/ext...
File extensions/browser/extension_function.h (right):

https://codereview.chromium.org/2166523003/diff/300001/extensions/browser/ext...
extensions/browser/extension_function.h:563: service_worker_version_id_;
nit: invert

[Devlin, 2016-10-06 16:36:13]

https://codereview.chromium.org/2166523003/diff/300001/extensions/DEPS
File extensions/DEPS (right):

https://codereview.chromium.org/2166523003/diff/300001/extensions/DEPS#newcode10
extensions/DEPS:10: "+content/common",
On 2016/10/06 16:35:12, Devlin wrote:
> This won't be allowed.

Whoops, meant to circle back to this.  Will we be able to pull
content::kInvalidServiceWorkerVersionId into content/public/common to avoid
this?

[lazyboy, 2016-10-06 17:57:37]

> Lifetime question: What's to prevent the flow of
> 
> Renderer sends SW request IPC
> Browser shuts down renderer
> IPC arrives at browser, browser tries to increment
> 
> Presumably this is just a fact of life in service workers (and event pages,
for
> that matter), but in the current implementation it seems like the browser
would
> then kill that renderer process for a bad message?

That seems like a problem, I've removed the renderer kill during ref count
increment.

https://codereview.chromium.org/2166523003/diff/280001/chrome/browser/extensi...
File chrome/browser/extensions/service_worker_apitest.cc (right):

https://codereview.chromium.org/2166523003/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/service_worker_apitest.cc:625:
ASSERT_EQ("chrome.tabs.update callback", result);
On 2016/10/06 16:35:12, Devlin wrote:
> On 2016/10/06 01:02:06, lazyboy wrote:
> > On 2016/10/05 17:02:25, Devlin wrote:
> > > I'm a little confused on the test for this.  Where do we adjust the
timeout,
> > or
> > > ensure the worker would otherwise be terminated?
> > 
> > We don't.
> > I only tested them with hacking the timeout values in service worker and
> adding
> > artificial delay to update response. As is, it's hard to to test this. I
hope
> to
> > write some unit tests later on.
> 
> Can we add an explicit TODO? 
Done.

 And out of curiosity, what are the main
> complications?


There's a worker timer that periodically fires (say X seconds) to check whether
the service worker has pending work to do. If it doesn't find on, on next timer
fire it will shutdown the worker. We need to mock the timer or override X to
small number for test. And then make an extension API run longer than (X + X)s.
Then check that we actually kept the worker running.

https://codereview.chromium.org/2166523003/diff/300001/content/browser/servic...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/2166523003/diff/300001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:569: // It's possible
that the renderer/ is confused/lying or the version started
On 2016/10/06 16:35:12, Devlin wrote:
> nit: renderer/ is a file path, renderer is a process.  I think here we want
the
> latter.
Done.
> nit #2: "confused" seems like something that shouldn't happen (lying can in
the
> case of compromised renderers)
Removed the term "confused". This means we have bug in bookkeeping the uuids.

https://codereview.chromium.org/2166523003/diff/300001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:608: // It's possible
that the renderer/ is confused/lying or the version started
On 2016/10/06 16:35:12, Devlin wrote:
> ditto

Done.

https://codereview.chromium.org/2166523003/diff/300001/extensions/DEPS
File extensions/DEPS (right):

https://codereview.chromium.org/2166523003/diff/300001/extensions/DEPS#newcode10
extensions/DEPS:10: "+content/common",
On 2016/10/06 16:36:13, Devlin wrote:
> On 2016/10/06 16:35:12, Devlin wrote:
> > This won't be allowed.
> 
> Whoops, meant to circle back to this.  Will we be able to pull
> content::kInvalidServiceWorkerVersionId into content/public/common to avoid
> this?

Ah, I keep confusing content/common for content/public/common.
I prefer do the change separately, for now I've copied the constant to
extensions/common/constants.h

https://codereview.chromium.org/2166523003/diff/300001/extensions/browser/ext...
File extensions/browser/extension_function.h (right):

https://codereview.chromium.org/2166523003/diff/300001/extensions/browser/ext...
extensions/browser/extension_function.h:563: service_worker_version_id_;
On 2016/10/06 16:35:12, Devlin wrote:
> nit: invert

Done.

[Devlin, 2016-10-06 18:58:07]

https://codereview.chromium.org/2166523003/diff/280001/chrome/browser/extensi...
File chrome/browser/extensions/service_worker_apitest.cc (right):

https://codereview.chromium.org/2166523003/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/service_worker_apitest.cc:625:
ASSERT_EQ("chrome.tabs.update callback", result);
On 2016/10/06 17:57:37, lazyboy wrote:
> On 2016/10/06 16:35:12, Devlin wrote:
> > On 2016/10/06 01:02:06, lazyboy wrote:
> > > On 2016/10/05 17:02:25, Devlin wrote:
> > > > I'm a little confused on the test for this.  Where do we adjust the
> timeout,
> > > or
> > > > ensure the worker would otherwise be terminated?
> > > 
> > > We don't.
> > > I only tested them with hacking the timeout values in service worker and
> > adding
> > > artificial delay to update response. As is, it's hard to to test this. I
> hope
> > to
> > > write some unit tests later on.
> > 
> > Can we add an explicit TODO? 
> Done.
> 
>  And out of curiosity, what are the main
> > complications?
> 
> 
> There's a worker timer that periodically fires (say X seconds) to check
whether
> the service worker has pending work to do. If it doesn't find on, on next
timer
> fire it will shutdown the worker. We need to mock the timer or override X to
> small number for test. And then make an extension API run longer than (X +
X)s.
> Then check that we actually kept the worker running.

Can we access the refcount from chrome/ (or could we add a test method or
something to do so)?  If so, we could use the test.sendMessage() API (which we
can control when it returns) and check that once the API call starts the
refcount increased by one.  That would be a pretty reasonable test, since I
think testing that a SW with any refcount isn't killed might be something better
tested in content/ (and for SWs in general).

[michaeln, 2016-10-06 19:22:32]

thnx for shuffling that around

https://codereview.chromium.org/2166523003/diff/320001/content/browser/servic...
File content/browser/service_worker/service_worker_context_wrapper.cc (right):

https://codereview.chromium.org/2166523003/diff/320001/content/browser/servic...
content/browser/service_worker/service_worker_context_wrapper.cc:844: 
whitespace nit: blank line not needed + might as well make the two methods
identical with regard to whitespace and wrapping

https://codereview.chromium.org/2166523003/diff/320001/content/browser/servic...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/2166523003/diff/320001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:573: 
where does external_request_uuid_to_request_id_[uuid] = integer_id happen?

https://codereview.chromium.org/2166523003/diff/320001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:1802:
pending_requests_.Clear();
probably be good to also clear the external_request mapping here

[lazyboy, 2016-10-07 06:00:57]

https://codereview.chromium.org/2166523003/diff/280001/chrome/browser/extensi...
File chrome/browser/extensions/service_worker_apitest.cc (right):

https://codereview.chromium.org/2166523003/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/service_worker_apitest.cc:625:
ASSERT_EQ("chrome.tabs.update callback", result);
On 2016/10/06 18:58:06, Devlin wrote:
> On 2016/10/06 17:57:37, lazyboy wrote:
> > On 2016/10/06 16:35:12, Devlin wrote:
> > > On 2016/10/06 01:02:06, lazyboy wrote:
> > > > On 2016/10/05 17:02:25, Devlin wrote:
> > > > > I'm a little confused on the test for this.  Where do we adjust the
> > timeout,
> > > > or
> > > > > ensure the worker would otherwise be terminated?
> > > > 
> > > > We don't.
> > > > I only tested them with hacking the timeout values in service worker and
> > > adding
> > > > artificial delay to update response. As is, it's hard to to test this. I
> > hope
> > > to
> > > > write some unit tests later on.
> > > 
> > > Can we add an explicit TODO? 
> > Done.
> > 
> >  And out of curiosity, what are the main
> > > complications?
> > 
> > 
> > There's a worker timer that periodically fires (say X seconds) to check
> whether
> > the service worker has pending work to do. If it doesn't find on, on next
> timer
> > fire it will shutdown the worker. We need to mock the timer or override X to
> > small number for test. And then make an extension API run longer than (X +
> X)s.
> > Then check that we actually kept the worker running.
> 
> Can we access the refcount from chrome/ (or could we add a test method or
> something to do so)?  If so, we could use the test.sendMessage() API (which we
> can control when it returns) and check that once the API call starts the
> refcount increased by one.  That would be a pretty reasonable test, since I
> think testing that a SW with any refcount isn't killed might be something
better
> tested in content/ (and for SWs in general).

Added a test only method in content/public, added test.

https://codereview.chromium.org/2166523003/diff/320001/content/browser/servic...
File content/browser/service_worker/service_worker_context_wrapper.cc (right):

https://codereview.chromium.org/2166523003/diff/320001/content/browser/servic...
content/browser/service_worker/service_worker_context_wrapper.cc:844: 
On 2016/10/06 19:22:31, michaeln wrote:
> whitespace nit: blank line not needed + might as well make the two methods
> identical with regard to whitespace and wrapping

Done.

https://codereview.chromium.org/2166523003/diff/320001/content/browser/servic...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/2166523003/diff/320001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:573: 
On 2016/10/06 19:22:31, michaeln wrote:
> where does external_request_uuid_to_request_id_[uuid] = integer_id happen?

It's supposed to be here, but I don't see it in the uploaded patch.
Done.

https://codereview.chromium.org/2166523003/diff/320001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:1802:
pending_requests_.Clear();
On 2016/10/06 19:22:31, michaeln wrote:
> probably be good to also clear the external_request mapping here

Done.

[Devlin, 2016-10-07 15:13:22]

Nice!  A couple requests for additions to the test and nits, but otherwise this
looks great.

https://codereview.chromium.org/2166523003/diff/340001/chrome/browser/extensi...
File chrome/browser/extensions/service_worker_apitest.cc (right):

https://codereview.chromium.org/2166523003/diff/340001/chrome/browser/extensi...
chrome/browser/extensions/service_worker_apitest.cc:658: 
Check ref count is 0?

https://codereview.chromium.org/2166523003/diff/340001/chrome/browser/extensi...
chrome/browser/extensions/service_worker_apitest.cc:688:
ASSERT_TRUE(extension_listener.WaitUntilSatisfied());
Check that ref count drops to 0?

I also wonder if it's worth checking that adding two methods increases it to 2 -
just to cover all the bases of 0, 1, >1.

https://codereview.chromium.org/2166523003/diff/340001/content/browser/servic...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/2166523003/diff/340001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:1804:
external_request_uuid_to_request_id_.clear();
For my own edification, how can the SW be stopped when there are external jobs?

https://codereview.chromium.org/2166523003/diff/340001/content/public/browser...
File content/public/browser/service_worker_context.h (right):

https://codereview.chromium.org/2166523003/diff/340001/content/public/browser...
content/public/browser/service_worker_context.h:70: virtual bool
IncrementPendingActivity(int64_t service_worker_version_id,
Drive-by: Since these are all associated with a specific uuid, it seems like
maybe this should be "AddPendingActivitiy" and "RemovePendingActivity" (and
maybe make activity_uuid, or replace activity with request in method names?). 
Increment/Decrement makes it sound like there's a separate counter.

But this is really a content/ OWNERS choice, so I'm happy either way - just my
$0.02.

https://codereview.chromium.org/2166523003/diff/340001/extensions/browser/ext...
File extensions/browser/extension_service_worker_message_filter.cc (right):

https://codereview.chromium.org/2166523003/diff/340001/extensions/browser/ext...
extensions/browser/extension_service_worker_message_filter.cc:59:
service_worker_context_->IncrementPendingActivity(service_worker_version_id,
maybe worth a comment why we don't check for bad messages?

[falken, 2016-10-07 15:22:03]

https://codereview.chromium.org/2166523003/diff/340001/content/browser/servic...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/2166523003/diff/340001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:1804:
external_request_uuid_to_request_id_.clear();
On 2016/10/07 15:13:22, Devlin wrote:
> For my own edification, how can the SW be stopped when there are external
jobs?

It could be intentional: the browser process kills the SW if the request is
taking too long (5 min timeout) or if the SW is apparently in a busy loop and
unresponsive to pings (30 sec timeout). It's also conceivable something went
awry and for whatever reason the renderer process or SW thread died
unexpectedly.

[Devlin, 2016-10-07 15:46:37]

https://codereview.chromium.org/2166523003/diff/340001/content/browser/servic...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/2166523003/diff/340001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:1804:
external_request_uuid_to_request_id_.clear();
On 2016/10/07 15:22:03, falken wrote:
> On 2016/10/07 15:13:22, Devlin wrote:
> > For my own edification, how can the SW be stopped when there are external
> jobs?
> 
> It could be intentional: the browser process kills the SW if the request is
> taking too long (5 min timeout) or if the SW is apparently in a busy loop and
> unresponsive to pings (30 sec timeout). It's also conceivable something went
> awry and for whatever reason the renderer process or SW thread died
> unexpectedly.

Couple of thoughts here (not necessarily ones that need to be addressed in this
CL).  What happens if an extension function round trip takes > 5 minutes?  This
could legitimately happen if e.g. the extension uses an API that shows a dialog
to the user and the user walks away.  For one thing, it sounds like the browser
would kill the SW then - which isn't necessarily the right thing to do.  For
another, assuming the entire process isn't killed, the browser would respond
once the user comes back, and the renderer might try to a) possibly call into
the dead context and b) respond to decrement the (already dead) SW - which now
would kill the renderer.

[lazyboy, 2016-10-07 18:50:10]

https://codereview.chromium.org/2166523003/diff/340001/chrome/browser/extensi...
File chrome/browser/extensions/service_worker_apitest.cc (right):

https://codereview.chromium.org/2166523003/diff/340001/chrome/browser/extensi...
chrome/browser/extensions/service_worker_apitest.cc:658: 
On 2016/10/07 15:13:22, Devlin wrote:
> Check ref count is 0?

Done.

https://codereview.chromium.org/2166523003/diff/340001/chrome/browser/extensi...
chrome/browser/extensions/service_worker_apitest.cc:688:
ASSERT_TRUE(extension_listener.WaitUntilSatisfied());
On 2016/10/07 15:13:21, Devlin wrote:
> Check that ref count drops to 0?
Done.
> 
> I also wonder if it's worth checking that adding two methods increases it to 2
-
> just to cover all the bases of 0, 1, >1.
Done.

https://codereview.chromium.org/2166523003/diff/340001/content/browser/servic...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/2166523003/diff/340001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:1804:
external_request_uuid_to_request_id_.clear();
On 2016/10/07 15:46:37, Devlin wrote:
> On 2016/10/07 15:22:03, falken wrote:
> > On 2016/10/07 15:13:22, Devlin wrote:
> > > For my own edification, how can the SW be stopped when there are external
> > jobs?
> > 
> > It could be intentional: the browser process kills the SW if the request is
> > taking too long (5 min timeout) or if the SW is apparently in a busy loop
and
> > unresponsive to pings (30 sec timeout). It's also conceivable something went
> > awry and for whatever reason the renderer process or SW thread died
> > unexpectedly.
> 
> Couple of thoughts here (not necessarily ones that need to be addressed in
this
> CL).  What happens if an extension function round trip takes > 5 minutes? 
This
> could legitimately happen if e.g. the extension uses an API that shows a
dialog
> to the user and the user walks away.  For one thing, it sounds like the
browser
> would kill the SW then - which isn't necessarily the right thing to do.  For
> another, assuming the entire process isn't killed, the browser would respond
> once the user comes back, and the renderer might try to a) possibly call into
> the dead context and b) respond to decrement the (already dead) SW - which now
> would kill the renderer.

Interesting, we definitely need to tackle this 5 min issue going forward. File a
bug to track?

https://codereview.chromium.org/2166523003/diff/340001/content/public/browser...
File content/public/browser/service_worker_context.h (right):

https://codereview.chromium.org/2166523003/diff/340001/content/public/browser...
content/public/browser/service_worker_context.h:70: virtual bool
IncrementPendingActivity(int64_t service_worker_version_id,
On 2016/10/07 15:13:22, Devlin wrote:
> Drive-by: Since these are all associated with a specific uuid, it seems like
> maybe this should be "AddPendingActivitiy" and "RemovePendingActivity" (and
> maybe make activity_uuid, or replace activity with request in method names?). 
> Increment/Decrement makes it sound like there's a separate counter.
> 
> But this is really a content/ OWNERS choice, so I'm happy either way - just my
> $0.02.

OK, will do before sending out to content/public review.

https://codereview.chromium.org/2166523003/diff/340001/extensions/browser/ext...
File extensions/browser/extension_service_worker_message_filter.cc (right):

https://codereview.chromium.org/2166523003/diff/340001/extensions/browser/ext...
extensions/browser/extension_service_worker_message_filter.cc:59:
service_worker_context_->IncrementPendingActivity(service_worker_version_id,
On 2016/10/07 15:13:22, Devlin wrote:
> maybe worth a comment why we don't check for bad messages?

Done.

[Devlin, 2016-10-07 19:06:45]

extensions lgtm

https://codereview.chromium.org/2166523003/diff/340001/content/browser/servic...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/2166523003/diff/340001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:1804:
external_request_uuid_to_request_id_.clear();
On 2016/10/07 18:50:10, lazyboy wrote:
> On 2016/10/07 15:46:37, Devlin wrote:
> > On 2016/10/07 15:22:03, falken wrote:
> > > On 2016/10/07 15:13:22, Devlin wrote:
> > > > For my own edification, how can the SW be stopped when there are
external
> > > jobs?
> > > 
> > > It could be intentional: the browser process kills the SW if the request
is
> > > taking too long (5 min timeout) or if the SW is apparently in a busy loop
> and
> > > unresponsive to pings (30 sec timeout). It's also conceivable something
went
> > > awry and for whatever reason the renderer process or SW thread died
> > > unexpectedly.
> > 
> > Couple of thoughts here (not necessarily ones that need to be addressed in
> this
> > CL).  What happens if an extension function round trip takes > 5 minutes? 
> This
> > could legitimately happen if e.g. the extension uses an API that shows a
> dialog
> > to the user and the user walks away.  For one thing, it sounds like the
> browser
> > would kill the SW then - which isn't necessarily the right thing to do.  For
> > another, assuming the entire process isn't killed, the browser would respond
> > once the user comes back, and the renderer might try to a) possibly call
into
> > the dead context and b) respond to decrement the (already dead) SW - which
now
> > would kill the renderer.
> 
> Interesting, we definitely need to tackle this 5 min issue going forward. File
a
> bug to track?

Yeah, I think it's fine to land this part, since this is all still behind trunk.

https://codereview.chromium.org/2166523003/diff/360001/chrome/browser/extensi...
File chrome/browser/extensions/service_worker_apitest.cc (right):

https://codereview.chromium.org/2166523003/diff/360001/chrome/browser/extensi...
chrome/browser/extensions/service_worker_apitest.cc:178: base::Unretained(this),
run_loop.QuitClosure()));
Optional nit: you might be able to do this in a lambda instead with something
like

size_t ref_count = 0;
base::RunLoop run_loop;
auto set_ref_count = [&ref_count, &run_loop](size_t count) {
  *ref_count = count;
  run_loop->Quit();
}

That might be a bit cleaner?  But I don't feel strongly either way.

https://codereview.chromium.org/2166523003/diff/360001/chrome/browser/extensi...
chrome/browser/extensions/service_worker_apitest.cc:700: listener.Reply("Hello
world");
TL;DR: Put the EXPECT_EQ(2u, GetWorkerRefCount(extension->url())) above the
Reply() call.

this strikes me as conceptually racy, even though I don't think it technically
is.  But since replying completes the sendMessage(), the ref count could
potentially drop any time after this.  We don't have to worry since it's handled
on the renderer side and the process hop takes time, but if we moved the
lifetime management to the browser (something I could see happening), this would
fail.

[michaeln, 2016-10-07 20:10:30]

the parts i reviewed lgtm 2

https://codereview.chromium.org/2166523003/diff/340001/content/public/browser...
File content/public/browser/service_worker_context.h (right):

https://codereview.chromium.org/2166523003/diff/340001/content/public/browser...
content/public/browser/service_worker_context.h:70: virtual bool
IncrementPendingActivity(int64_t service_worker_version_id,
On 2016/10/07 18:50:10, lazyboy wrote:
> On 2016/10/07 15:13:22, Devlin wrote:
> > Drive-by: Since these are all associated with a specific uuid, it seems like
> > maybe this should be "AddPendingActivitiy" and "RemovePendingActivity" (and
> > maybe make activity_uuid, or replace activity with request in method
names?). 
> > Increment/Decrement makes it sound like there's a separate counter.
> > 
> > But this is really a content/ OWNERS choice, so I'm happy either way - just
my
> > $0.02.
> 
> OK, will do before sending out to content/public review.

i was thinking a rename would be good too, maybe match the naming in the impl

  StartingExternalRequest()
  FinishedExternalRequest()

and later, when needed

  ExtendingExternalRequest() to keep it alive for longer

https://codereview.chromium.org/2166523003/diff/340001/content/public/browser...
content/public/browser/service_worker_context.h:113: virtual void
GetPendingExternalRequestCountForTest(
and this could be something like CountExternalRequestsFrTest

[lazyboy, 2016-10-07 21:28:39]

lazyboy@chromium.org changed reviewers:
+ avi@chromium.org, mpearson@chromium.org, tsepez@chromium.org

[lazyboy, 2016-10-07 21:28:41]

+avi for content/public changes:
content/public/browser/service_worker_context.h
content/public/renderer/content_renderer_client.h

+tsepez for IPC changes:
extensions/common/extension_messages.h

+mpearson for histograms:
tools/metrics/histograms/histograms.xml

https://codereview.chromium.org/2166523003/diff/340001/content/public/browser...
File content/public/browser/service_worker_context.h (right):

https://codereview.chromium.org/2166523003/diff/340001/content/public/browser...
content/public/browser/service_worker_context.h:70: virtual bool
IncrementPendingActivity(int64_t service_worker_version_id,
On 2016/10/07 20:10:30, michaeln wrote:
> On 2016/10/07 18:50:10, lazyboy wrote:
> > On 2016/10/07 15:13:22, Devlin wrote:
> > > Drive-by: Since these are all associated with a specific uuid, it seems
like
> > > maybe this should be "AddPendingActivitiy" and "RemovePendingActivity"
(and
> > > maybe make activity_uuid, or replace activity with request in method
> names?). 
> > > Increment/Decrement makes it sound like there's a separate counter.
> > > 
> > > But this is really a content/ OWNERS choice, so I'm happy either way -
just
> my
> > > $0.02.
> > 
> > OK, will do before sending out to content/public review.
> 
> i was thinking a rename would be good too, maybe match the naming in the impl
> 
>   StartingExternalRequest()
>   FinishedExternalRequest()
> 
> and later, when needed
> 
>   ExtendingExternalRequest() to keep it alive for longer

Changed the names to reflect this is request instead of activity.

https://codereview.chromium.org/2166523003/diff/340001/content/public/browser...
content/public/browser/service_worker_context.h:113: virtual void
GetPendingExternalRequestCountForTest(
On 2016/10/07 20:10:30, michaeln wrote:
> and this could be something like CountExternalRequestsFrTest

Done.

https://codereview.chromium.org/2166523003/diff/360001/chrome/browser/extensi...
File chrome/browser/extensions/service_worker_apitest.cc (right):

https://codereview.chromium.org/2166523003/diff/360001/chrome/browser/extensi...
chrome/browser/extensions/service_worker_apitest.cc:178: base::Unretained(this),
run_loop.QuitClosure()));
On 2016/10/07 19:06:44, Devlin wrote:
> Optional nit: you might be able to do this in a lambda instead with something
> like
> 
> size_t ref_count = 0;
> base::RunLoop run_loop;
> auto set_ref_count = [&ref_count, &run_loop](size_t count) {
>   *ref_count = count;
>   run_loop->Quit();
> }
> 
> That might be a bit cleaner?  But I don't feel strongly either way.

Ya, less members and callback functions outside.
Done.

https://codereview.chromium.org/2166523003/diff/360001/chrome/browser/extensi...
chrome/browser/extensions/service_worker_apitest.cc:700: listener.Reply("Hello
world");
On 2016/10/07 19:06:44, Devlin wrote:
> TL;DR: Put the EXPECT_EQ(2u, GetWorkerRefCount(extension->url())) above the
> Reply() call.
> 
> this strikes me as conceptually racy, even though I don't think it technically
> is.  But since replying completes the sendMessage(), the ref count could
> potentially drop any time after this.  We don't have to worry since it's
handled
> on the renderer side and the process hop takes time, but if we moved the
> lifetime management to the browser (something I could see happening), this
would
> fail.

Good catch.
Done.

[Avi (use Gerrit), 2016-10-07 21:33:11]

content/public lgtm with nits

https://codereview.chromium.org/2166523003/diff/380001/content/public/browser...
File content/public/browser/service_worker_context.h (right):

https://codereview.chromium.org/2166523003/diff/380001/content/public/browser...
content/public/browser/service_worker_context.h:37:
CountExternalRequestsCallback;
Can we use the cool new

using CountExternalRequestsCallback = base::Callback<void(size_t
external_request_count)>;

syntax here and above? C++11 style guide calls it out as preferred.

[Tom Sepez, 2016-10-07 22:31:46]

https://codereview.chromium.org/2166523003/diff/380001/extensions/common/exte...
File extensions/common/extension_messages.h (right):

https://codereview.chromium.org/2166523003/diff/380001/extensions/common/exte...
extensions/common/extension_messages.h:891: // Asks the browser to increment the
pending activity count for
What happens browser-side if a compromised renderer sends more decrements than
increments, or continues to use in some manner a worker that has been
decremented to 0?

[lazyboy, 2016-10-07 22:31:49]

https://codereview.chromium.org/2166523003/diff/380001/content/public/browser...
File content/public/browser/service_worker_context.h (right):

https://codereview.chromium.org/2166523003/diff/380001/content/public/browser...
content/public/browser/service_worker_context.h:37:
CountExternalRequestsCallback;
On 2016/10/07 21:33:11, Avi wrote:
> Can we use the cool new
> 
> using CountExternalRequestsCallback = base::Callback<void(size_t
> external_request_count)>;
> 
> syntax here and above? C++11 style guide calls it out as preferred.

Done.

[lazyboy, 2016-10-07 23:16:59]

https://codereview.chromium.org/2166523003/diff/380001/extensions/common/exte...
File extensions/common/extension_messages.h (right):

https://codereview.chromium.org/2166523003/diff/380001/extensions/common/exte...
extensions/common/extension_messages.h:891: // Asks the browser to increment the
pending activity count for
On 2016/10/07 22:31:46, Tom Sepez wrote:
> What happens browser-side if a compromised renderer sends more decrements than
> increments, or continues to use in some manner a worker that has been
> decremented to 0?

This is expected to be unique, I've added a comment saying that.

A compromised renderer can keep sending same request_uuid, in that case:
For increment: if there's any request that was already pending with
|request_uuid|, browser will ignore it (I've not been doing that, added that in
latest patch, thanks).
For decrement: It will only work if browser has a pending request with
|request_uuid|, otherwise it will ignore the request.

[lazyboy, 2016-10-07 23:33:43]

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

[commit-bot: I haz the power, 2016-10-07 23:34:15]

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

[Tom Sepez, 2016-10-07 23:53:51]

> A compromised renderer can keep sending same request_uuid, in that case:
> For increment: if there's any request that was already pending with
> |request_uuid|, browser will ignore it (I've not been doing that, added that
in
> latest patch, thanks).
> For decrement: It will only work if browser has a pending request with
> |request_uuid|, otherwise it will ignore the request.
OK, LGTM then.

[michaeln, 2016-10-08 00:05:30]

still lgtm but would look even better with a commit message using "external
request" sort of language

[lazyboy, 2016-10-08 00:12:44]

Description was changed from

==========
Add ref count to service workers for extension API.

We need a way to keep a service worker alive
1) during extension function's request->response roundtrip completes.
2) when an event is about to be dispatched to a (stopped) service worker.

This CL shows a way to do #1. #2 can follow later.
The CL adds plumbing to expose functions to increment/decrement ref
counting to an EmbeddedWorkerInstance.

BUG=602442
Test=ServiceWorkerTest.TabsCreate in this CL shows a proof of concept.
Both the stop worker idle timeout and worker timeout timout is reduced to 3s.
The test calls chrome.tabs.update() function and the function's implemention is
modified to return after 7s delay (> 3s + 3s). When chrome.tabs.update() is
inflight, it increments the worker's ref count. The worker won't be stopped
because of the ref count (it will bail out from StopWorkerIfIdle()).
==========

to

==========
Add ref count to service workers for extension API.

We need a way to keep a service worker alive
1) during extension function's request->response roundtrip completes.
2) when an event is about to be dispatched to a (stopped) service worker.

This CL shows a way to do #1. #2 can follow later.

The CL adds plumbing to expose functions to increment/decrement ref
counting to an ServiceWorkerVersion. This is done by adding a way to
add "external requests" to a ServiceWorkerVersion: when a worker has
external requests pending, it will be considered to be in working state,
i.e ServiceWorkerVersion::HasWork() returns true.
The public interface is exposed through ServiceWorkerContext. And the
interface methods expect a GUID/nonce for each such requests from service
worker renderer.

BUG=602442
Test=ServiceWorkerTest.TabsCreate in this CL shows a proof of concept.
Both the stop worker idle timeout and worker timeout timout is reduced to 3s.
The test calls chrome.tabs.update() function and the function's implemention is
modified to return after 7s delay (> 3s + 3s). When chrome.tabs.update() is
inflight, it increments the worker's ref count. The worker won't be stopped
because of the ref count (it will bail out from StopWorkerIfIdle()).
==========

[lazyboy, 2016-10-08 00:17:15]

Description was changed from

==========
Add ref count to service workers for extension API.

We need a way to keep a service worker alive
1) during extension function's request->response roundtrip completes.
2) when an event is about to be dispatched to a (stopped) service worker.

This CL shows a way to do #1. #2 can follow later.

The CL adds plumbing to expose functions to increment/decrement ref
counting to an ServiceWorkerVersion. This is done by adding a way to
add "external requests" to a ServiceWorkerVersion: when a worker has
external requests pending, it will be considered to be in working state,
i.e ServiceWorkerVersion::HasWork() returns true.
The public interface is exposed through ServiceWorkerContext. And the
interface methods expect a GUID/nonce for each such requests from service
worker renderer.

BUG=602442
Test=ServiceWorkerTest.TabsCreate in this CL shows a proof of concept.
Both the stop worker idle timeout and worker timeout timout is reduced to 3s.
The test calls chrome.tabs.update() function and the function's implemention is
modified to return after 7s delay (> 3s + 3s). When chrome.tabs.update() is
inflight, it increments the worker's ref count. The worker won't be stopped
because of the ref count (it will bail out from StopWorkerIfIdle()).
==========

to

==========
Add ref count to service workers for extension API.

We need a way to keep a service worker alive
1) during extension function's request->response roundtrip completes.
2) when an event is about to be dispatched to a (stopped) service worker.

This CL shows a way to do #1. #2 can follow later.

The CL adds plumbing to expose functions to increment/decrement ref
counting to an ServiceWorkerVersion. This is done by adding a way to
add "external requests" to a ServiceWorkerVersion: when a worker has
external requests pending, it will be considered to be in working state,
i.e ServiceWorkerVersion::HasWork() will return true.
The public interface is exposed through ServiceWorkerContext. And the
interface methods expect a GUID/nonce for each such requests from service
worker renderer.

Extension APIs that are expected to be long running aren't handled in
this CL. For example: an extension API showing a dialog to user that
waits for user action.

BUG=602442
Test=ServiceWorkerTest.TabsCreate in this CL shows a proof of concept.
Both the stop worker idle timeout and worker timeout timout is reduced to 3s.
The test calls chrome.tabs.update() function and the function's implemention is
modified to return after 7s delay (> 3s + 3s). When chrome.tabs.update() is
inflight, it increments the worker's ref count. The worker won't be stopped
because of the ref count (it will bail out from StopWorkerIfIdle()).
==========

[lazyboy, 2016-10-08 00:22:11]

Description was changed from

==========
Add ref count to service workers for extension API.

We need a way to keep a service worker alive
1) during extension function's request->response roundtrip completes.
2) when an event is about to be dispatched to a (stopped) service worker.

This CL shows a way to do #1. #2 can follow later.

The CL adds plumbing to expose functions to increment/decrement ref
counting to an ServiceWorkerVersion. This is done by adding a way to
add "external requests" to a ServiceWorkerVersion: when a worker has
external requests pending, it will be considered to be in working state,
i.e ServiceWorkerVersion::HasWork() will return true.
The public interface is exposed through ServiceWorkerContext. And the
interface methods expect a GUID/nonce for each such requests from service
worker renderer.

Extension APIs that are expected to be long running aren't handled in
this CL. For example: an extension API showing a dialog to user that
waits for user action.

BUG=602442
Test=ServiceWorkerTest.TabsCreate in this CL shows a proof of concept.
Both the stop worker idle timeout and worker timeout timout is reduced to 3s.
The test calls chrome.tabs.update() function and the function's implemention is
modified to return after 7s delay (> 3s + 3s). When chrome.tabs.update() is
inflight, it increments the worker's ref count. The worker won't be stopped
because of the ref count (it will bail out from StopWorkerIfIdle()).
==========

to

==========
Add ref count to service workers for extension API.

We need a way to keep a service worker alive
1) during extension function's request->response roundtrip completes.
2) when an event is about to be dispatched to a (stopped) service worker.

This CL shows a way to do #1. #2 can follow later.

The CL adds plumbing to expose functions to increment/decrement ref
counting to an ServiceWorkerVersion. This is done by adding a way to
add "external requests" to a ServiceWorkerVersion: when a worker has
external requests pending, it will be considered to be in working state,
i.e ServiceWorkerVersion::HasWork() will return true.
The public interface is exposed through ServiceWorkerContext. And the
interface methods expect a GUID/nonce for each such requests from service
worker renderer.

Extension APIs that are expected to be long running aren't handled in
this CL. For example: an extension API showing a dialog to user that
waits for user action.

BUG=602442
Test=There's no easy way to test it without tweaking the code, I've
used the following steps to make sure that we keep SW alive when an extension
API is in-flight:
Change the stop worker idle timeout and worker timeout to sth small, e.g. 3s.
Call an extension function that runs for 7s (> 3s + 3s). Without the CL, the
extension function's callback won't be called because the worker would shut
down after 6s.
The added test ServiceWorkerTest.WorkerRefCount tests this at a bit lower level:
by checking ref count (= count of external requests for a ServiceWorkerVersion).
==========

[lazyboy, 2016-10-08 00:24:18]

Description was changed from

==========
Add ref count to service workers for extension API.

We need a way to keep a service worker alive
1) during extension function's request->response roundtrip completes.
2) when an event is about to be dispatched to a (stopped) service worker.

This CL shows a way to do #1. #2 can follow later.

The CL adds plumbing to expose functions to increment/decrement ref
counting to an ServiceWorkerVersion. This is done by adding a way to
add "external requests" to a ServiceWorkerVersion: when a worker has
external requests pending, it will be considered to be in working state,
i.e ServiceWorkerVersion::HasWork() will return true.
The public interface is exposed through ServiceWorkerContext. And the
interface methods expect a GUID/nonce for each such requests from service
worker renderer.

Extension APIs that are expected to be long running aren't handled in
this CL. For example: an extension API showing a dialog to user that
waits for user action.

BUG=602442
Test=There's no easy way to test it without tweaking the code, I've
used the following steps to make sure that we keep SW alive when an extension
API is in-flight:
Change the stop worker idle timeout and worker timeout to sth small, e.g. 3s.
Call an extension function that runs for 7s (> 3s + 3s). Without the CL, the
extension function's callback won't be called because the worker would shut
down after 6s.
The added test ServiceWorkerTest.WorkerRefCount tests this at a bit lower level:
by checking ref count (= count of external requests for a ServiceWorkerVersion).
==========

to

==========
Add ref count to service workers for extension API.

We need a way to keep a service worker alive
1) during extension function's request->response roundtrip completes.
2) when an event is about to be dispatched to a (stopped) service worker.

This CL shows a way to do #1. #2 can follow later.

The CL adds plumbing to expose functions to increment/decrement ref
counting to an ServiceWorkerVersion. This is done by adding a way to
add "external requests" to a ServiceWorkerVersion: when a worker has
external requests pending, it will be considered to be in working state,
i.e ServiceWorkerVersion::HasWork() will return true.
The public interface is exposed through ServiceWorkerContext. And the
interface methods expect a GUID/nonce for each such requests from service
worker renderer:
ServiceWorkerContext::StartingExternalRequest() and
ServiceWorkerContext::FinishedExternalRequest()

Extension APIs that are expected to be long running aren't handled in
this CL. For example: an extension API showing a dialog to user that
waits for user action.

BUG=602442
Test=There's no easy way to test it without tweaking the code, I've
used the following steps to make sure that we keep SW alive when an extension
API is in-flight:
Change the stop worker idle timeout and worker timeout to sth small, e.g. 3s.
Call an extension function that runs for 7s (> 3s + 3s). Without the CL, the
extension function's callback won't be called because the worker would shut
down after 6s.
The added test ServiceWorkerTest.WorkerRefCount tests this at a bit lower level:
by checking ref count (= count of external requests for a ServiceWorkerVersion).
==========

[lazyboy, 2016-10-08 00:24:54]

On 2016/10/08 00:05:30, michaeln wrote:
> still lgtm but would look even better with a commit message using "external
> request" sort of language

Tweaked the CL description a bit. Thanks!

[commit-bot: I haz the power, 2016-10-08 01:18:50]

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

[commit-bot: I haz the power, 2016-10-08 01:18:51]

Dry run: This issue passed the CQ dry run.

[horo, 2016-10-08 09:56:02]

lgtm

[falken, 2016-10-11 01:03:57]

On 2016/10/07 15:46:37, Devlin wrote:
>
https://codereview.chromium.org/2166523003/diff/340001/content/browser/servic...
> File content/browser/service_worker/service_worker_version.cc (right):
> 
>
https://codereview.chromium.org/2166523003/diff/340001/content/browser/servic...
> content/browser/service_worker/service_worker_version.cc:1804:
> external_request_uuid_to_request_id_.clear();
> On 2016/10/07 15:22:03, falken wrote:
> > On 2016/10/07 15:13:22, Devlin wrote:
> > > For my own edification, how can the SW be stopped when there are external
> > jobs?
> > 
> > It could be intentional: the browser process kills the SW if the request is
> > taking too long (5 min timeout) or if the SW is apparently in a busy loop
and
> > unresponsive to pings (30 sec timeout). It's also conceivable something went
> > awry and for whatever reason the renderer process or SW thread died
> > unexpectedly.
> 
> Couple of thoughts here (not necessarily ones that need to be addressed in
this
> CL).  What happens if an extension function round trip takes > 5 minutes? 
This
> could legitimately happen if e.g. the extension uses an API that shows a
dialog
> to the user and the user walks away.  For one thing, it sounds like the
browser
> would kill the SW then - which isn't necessarily the right thing to do.  For
> another, assuming the entire process isn't killed, the browser would respond
> once the user comes back, and the renderer might try to a) possibly call into
> the dead context and b) respond to decrement the (already dead) SW - which now
> would kill the renderer.

I'm not sure I like the direction this is heading: service workers are designed
to be very short-lived event-based workers and it looks like we're trying to
turn them into long-lived workers to fit the extension/background page model. We
disallow sync APIs in SW and the original idea was to make them lightweight
enough to be terminated and restarted several times a second. The five minute
timeout itself is already very generous for non-extension web platform use and
we're looking at ways to shorten it more.

We could allow a longer timeout or no timeout for extension-based service
workers but it's going to complicate the code. For example it'll confuse our
metrics about how long SW last unless we filter out extension SW specifically.

I think this particular patch can be landed but could we do a VC soon?

[lazyboy, 2016-10-14 00:00:58]

lazyboy@chromium.org changed reviewers:
+ asvitkine@chromium.org
- mpearson@chromium.org

[lazyboy, 2016-10-14 00:00:59]

+asvitkine for histograms.

@falken, chatting about long lived issue sg, would you have time chatting on
Friday late your time? (i.e. Friday earlier our time). Also, for apis like
notifications, how do service workers handle them in the same scenario?

e.g. in worker JS I do:
result = registration.showNotification('A', {body: 'B'});
e.waitUntil(result.then(function() { ... }));
Does the promise get rejected on timeout?

[falken, 2016-10-14 00:55:29]

On 2016/10/14 00:00:59, lazyboy wrote:
> +asvitkine for histograms.
> 
> @falken, chatting about long lived issue sg, would you have time chatting on
> Friday late your time? (i.e. Friday earlier our time).
 
That's a bit difficult... let me send you an email separately to schedule.

> Also, for apis like
> notifications, how do service workers handle them in the same scenario?
> 
> e.g. in worker JS I do:
> result = registration.showNotification('A', {body: 'B'});
> e.waitUntil(result.then(function() { ... }));
> Does the promise get rejected on timeout?

The worker gets forcibly killed on timeout. Note that the showNotification
promise resolves once the notification is shown (or rejects if it couldn't be).
When the user clicks, a notificationclick event is dispatched to the service
worker. The web developer should design for the service worker being terminated
and restarted between the events.

[Alexei Svitkine (slow), 2016-10-14 15:41:27]

histograms lgtm

[lazyboy, 2016-10-14 17:58:06]

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

[commit-bot: I haz the power, 2016-10-14 17:58:53]

Dry run: 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, 2016-10-14 19:43:38]

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

[commit-bot: I haz the power, 2016-10-14 19:43:39]

Dry run: This issue passed the CQ dry run.

[lazyboy, 2016-10-17 18:17:18]

lazyboy@chromium.org changed reviewers:
+ thestig@chromium.org

[lazyboy, 2016-10-17 18:17:20]

+thestig for  chrome/renderer/chrome_content_renderer_client.h
chrome/renderer/chrome_content_renderer_client.cc

[Lei Zhang, 2016-10-17 18:39:31]

On 2016/10/17 18:17:20, lazyboy wrote:
> +thestig for  chrome/renderer/chrome_content_renderer_client.h
> chrome/renderer/chrome_content_renderer_client.cc

lgtm

[lazyboy, 2016-10-17 22:22:18]

The CQ bit was checked by lazyboy@chromium.org

[lazyboy, 2016-10-17 22:22:19]

The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org,
rdevlin.cronin@chromium.org, avi@chromium.org, michaeln@chromium.org,
asvitkine@chromium.org, horo@chromium.org, tsepez@chromium.org
Link to the patchset: https://codereview.chromium.org/2166523003/#ps440001
(title: "sync@tott")

[commit-bot: I haz the power, 2016-10-17 22:23:05]

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, 2016-10-18 00:04:34]

Description was changed from

==========
Add ref count to service workers for extension API.

We need a way to keep a service worker alive
1) during extension function's request->response roundtrip completes.
2) when an event is about to be dispatched to a (stopped) service worker.

This CL shows a way to do #1. #2 can follow later.

The CL adds plumbing to expose functions to increment/decrement ref
counting to an ServiceWorkerVersion. This is done by adding a way to
add "external requests" to a ServiceWorkerVersion: when a worker has
external requests pending, it will be considered to be in working state,
i.e ServiceWorkerVersion::HasWork() will return true.
The public interface is exposed through ServiceWorkerContext. And the
interface methods expect a GUID/nonce for each such requests from service
worker renderer:
ServiceWorkerContext::StartingExternalRequest() and
ServiceWorkerContext::FinishedExternalRequest()

Extension APIs that are expected to be long running aren't handled in
this CL. For example: an extension API showing a dialog to user that
waits for user action.

BUG=602442
Test=There's no easy way to test it without tweaking the code, I've
used the following steps to make sure that we keep SW alive when an extension
API is in-flight:
Change the stop worker idle timeout and worker timeout to sth small, e.g. 3s.
Call an extension function that runs for 7s (> 3s + 3s). Without the CL, the
extension function's callback won't be called because the worker would shut
down after 6s.
The added test ServiceWorkerTest.WorkerRefCount tests this at a bit lower level:
by checking ref count (= count of external requests for a ServiceWorkerVersion).
==========

to

==========
Add ref count to service workers for extension API.

We need a way to keep a service worker alive
1) during extension function's request->response roundtrip completes.
2) when an event is about to be dispatched to a (stopped) service worker.

This CL shows a way to do #1. #2 can follow later.

The CL adds plumbing to expose functions to increment/decrement ref
counting to an ServiceWorkerVersion. This is done by adding a way to
add "external requests" to a ServiceWorkerVersion: when a worker has
external requests pending, it will be considered to be in working state,
i.e ServiceWorkerVersion::HasWork() will return true.
The public interface is exposed through ServiceWorkerContext. And the
interface methods expect a GUID/nonce for each such requests from service
worker renderer:
ServiceWorkerContext::StartingExternalRequest() and
ServiceWorkerContext::FinishedExternalRequest()

Extension APIs that are expected to be long running aren't handled in
this CL. For example: an extension API showing a dialog to user that
waits for user action.

BUG=602442
Test=There's no easy way to test it without tweaking the code, I've
used the following steps to make sure that we keep SW alive when an extension
API is in-flight:
Change the stop worker idle timeout and worker timeout to sth small, e.g. 3s.
Call an extension function that runs for 7s (> 3s + 3s). Without the CL, the
extension function's callback won't be called because the worker would shut
down after 6s.
The added test ServiceWorkerTest.WorkerRefCount tests this at a bit lower level:
by checking ref count (= count of external requests for a ServiceWorkerVersion).
==========

[commit-bot: I haz the power, 2016-10-18 00:04:36]

Committed patchset #21 (id:440001)

[commit-bot: I haz the power, 2016-10-18 00:07:48]

Description was changed from

==========
Add ref count to service workers for extension API.

We need a way to keep a service worker alive
1) during extension function's request->response roundtrip completes.
2) when an event is about to be dispatched to a (stopped) service worker.

This CL shows a way to do #1. #2 can follow later.

The CL adds plumbing to expose functions to increment/decrement ref
counting to an ServiceWorkerVersion. This is done by adding a way to
add "external requests" to a ServiceWorkerVersion: when a worker has
external requests pending, it will be considered to be in working state,
i.e ServiceWorkerVersion::HasWork() will return true.
The public interface is exposed through ServiceWorkerContext. And the
interface methods expect a GUID/nonce for each such requests from service
worker renderer:
ServiceWorkerContext::StartingExternalRequest() and
ServiceWorkerContext::FinishedExternalRequest()

Extension APIs that are expected to be long running aren't handled in
this CL. For example: an extension API showing a dialog to user that
waits for user action.

BUG=602442
Test=There's no easy way to test it without tweaking the code, I've
used the following steps to make sure that we keep SW alive when an extension
API is in-flight:
Change the stop worker idle timeout and worker timeout to sth small, e.g. 3s.
Call an extension function that runs for 7s (> 3s + 3s). Without the CL, the
extension function's callback won't be called because the worker would shut
down after 6s.
The added test ServiceWorkerTest.WorkerRefCount tests this at a bit lower level:
by checking ref count (= count of external requests for a ServiceWorkerVersion).
==========

to

==========
Add ref count to service workers for extension API.

We need a way to keep a service worker alive
1) during extension function's request->response roundtrip completes.
2) when an event is about to be dispatched to a (stopped) service worker.

This CL shows a way to do #1. #2 can follow later.

The CL adds plumbing to expose functions to increment/decrement ref
counting to an ServiceWorkerVersion. This is done by adding a way to
add "external requests" to a ServiceWorkerVersion: when a worker has
external requests pending, it will be considered to be in working state,
i.e ServiceWorkerVersion::HasWork() will return true.
The public interface is exposed through ServiceWorkerContext. And the
interface methods expect a GUID/nonce for each such requests from service
worker renderer:
ServiceWorkerContext::StartingExternalRequest() and
ServiceWorkerContext::FinishedExternalRequest()

Extension APIs that are expected to be long running aren't handled in
this CL. For example: an extension API showing a dialog to user that
waits for user action.

BUG=602442
Test=There's no easy way to test it without tweaking the code, I've
used the following steps to make sure that we keep SW alive when an extension
API is in-flight:
Change the stop worker idle timeout and worker timeout to sth small, e.g. 3s.
Call an extension function that runs for 7s (> 3s + 3s). Without the CL, the
extension function's callback won't be called because the worker would shut
down after 6s.
The added test ServiceWorkerTest.WorkerRefCount tests this at a bit lower level:
by checking ref count (= count of external requests for a ServiceWorkerVersion).

Committed: https://crrev.com/4c82177a8b45303c941ddbc469444f0bfc760de4
Cr-Commit-Position: refs/heads/master@{#425824}
==========

[commit-bot: I haz the power, 2016-10-18 00:07:50]

Patchset 21 (id:??) landed as
https://crrev.com/4c82177a8b45303c941ddbc469444f0bfc760de4
Cr-Commit-Position: refs/heads/master@{#425824}