[ServiceWorker] Keep ServiceWorker alive while streaming the response for FetchEvent.

[ServiceWorker] Keep ServiceWorker alive while streaming the response for FetchEvent.

In current implementation the ServiceWorker is terminated 30 seconds after start loading the page even if the page is still loading.
(https://codereview.chromium.org/805083006/ changed the behavior of stopping the ServiceWorker.)

This cl changes ServiceWorkerVersion::HasInflightRequests() to check the existence of the active streaming ServiceWorkerURLRequestJobs.

BUG=445775
Committed: https://crrev.com/4bffcd598dd89e0016208ce9312a1f477ff105d1
Cr-Commit-Position: refs/heads/master@{#309970}

[horo, 2015-01-05 06:56:52]

Patchset #1 (id:1) has been deleted

[horo, 2015-01-05 06:56:58]

Patchset #1 (id:20001) has been deleted

[horo, 2015-01-05 06:58:52]

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

[horo, 2015-01-05 06:58:52]

kinuko@
Could you please review this?

[kinuko, 2015-01-05 08:26:04]

Thanks for fixing this!

https://codereview.chromium.org/832813002/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_url_request_job.cc (right):

https://codereview.chromium.org/832813002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_url_request_job.cc:542:
provider_host_->active_version()->AddStreamingURLRequestJob(this);
Should we just always keep alive the version while we're serving from
ServiceWorker?

[horo, 2015-01-05 08:31:33]

https://codereview.chromium.org/832813002/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_url_request_job.cc (right):

https://codereview.chromium.org/832813002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_url_request_job.cc:542:
provider_host_->active_version()->AddStreamingURLRequestJob(this);
On 2015/01/05 08:26:04, kinuko wrote:
> Should we just always keep alive the version while we're serving from
> ServiceWorker?

We don't need to keep alive the version if ServiceWorkerURLRequestJob is reading
the data from a blob.

[kinuko, 2015-01-05 14:08:21]

https://codereview.chromium.org/832813002/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_url_request_job.cc (right):

https://codereview.chromium.org/832813002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_url_request_job.cc:542:
provider_host_->active_version()->AddStreamingURLRequestJob(this);
On 2015/01/05 08:31:32, horo wrote:
> On 2015/01/05 08:26:04, kinuko wrote:
> > Should we just always keep alive the version while we're serving from
> > ServiceWorker?
> 
> We don't need to keep alive the version if ServiceWorkerURLRequestJob is
reading
> the data from a blob.

Ok. I just thought it might be simpler and ok enough, but there's no reason to
keep versions alive longer than necessary. lgtm

[horo, 2015-01-05 22:07:28]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2015-01-05 22:08:54]

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

[commit-bot: I haz the power, 2015-01-05 22:21:27]

Committed patchset #1 (id:40001)

[commit-bot: I haz the power, 2015-01-05 22:22:35]

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

[michaeln, 2015-01-07 21:48:45]

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

[michaeln, 2015-01-07 21:48:46]

https://codereview.chromium.org/832813002/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_url_request_job.cc (right):

https://codereview.chromium.org/832813002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_url_request_job.cc:87: if
(ServiceWorkerVersion* active_version = provider_host_->active_version())
couple of things i wonder about?

provider_host_ is a weakptr, i'm not sure there's anything preventing the host
from being deleted prior to this being called

is Kill() called in all cases? this class does not itself do anything to ensure
that it is.

[horo, 2015-01-08 04:04:03]

https://codereview.chromium.org/832813002/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_url_request_job.cc (right):

https://codereview.chromium.org/832813002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_url_request_job.cc:87: if
(ServiceWorkerVersion* active_version = provider_host_->active_version())
On 2015/01/07 21:48:46, michaeln wrote:
> couple of things i wonder about?
> 
> provider_host_ is a weakptr, i'm not sure there's anything preventing the host
> from being deleted prior to this being called
> 
> is Kill() called in all cases? this class does not itself do anything to
ensure
> that it is.

ServiceWorkerURLRequestJob::Kill() must be called in the destructor of the
URLRequest.
 URLRequest::~URLRequest() calls URLRequest::OrphanJob().
 URLRequest::OrphanJob() calls ServiceWorkerURLRequestJob::Kill().


ServiceWorkerProviderHost is deleted when:
 1. the browser process recieves ServiceWorkerHostMsg_ProviderDestroyed message.
or
 2. the IPC connection to the renderer is closed because of the crash of the
renderer process.

1.1. ServiceWorkerHostMsg_ProviderDestroyed is sent from the renderer when
ServiceWorkerNetworkProvider in the renderer is deleted.
1.2. ServiceWorkerNetworkProvider is deleted when DocumentLoader is deleted in
FrameLoader.
1.3. DocumentLoader::detachFromFrame() must be called before DocumentLoader is
deleted. (see FrameLoader.cpp)
1.4. DocumentLoader::detachFromFrame() calcels the all pending requests. So
ResourceHostMsg_CancelRequest for the all pending requests are sent.
1.5. When the browser process recieves ResourceHostMsg_CancelRequest, URLRequest
is deleted.


2.1. ChannelProxy::Context::OnChannelClosed() is called when the IPC connection
to the renderer is closed.
2.2. ResourceMessageFilter::OnChannelClosing() is caleld before
ServiceWorkerDispatcherHost::OnFilterRemoved() is called in
ChannelProxy::Context::OnChannelClosed(). It is because ResourceMessageFilter is
added before ServiceWorkerDispatcherHost in
RenderProcessHostImpl::CreateMessageFilters().
2.3. ResourceMessageFilter::OnChannelClosing() deletes the all pending
URLRequest from the renderer.
2.4. ServiceWorkerDispatcherHost::OnFilterRemoved() deletes
ServiceWorkerProviderHost.

So, ServiceWorkerURLRequestJob::Kill() must be called before
ServiceWorkerProviderHost is deleted.


Is my understanding incorrect?

[michaeln, 2015-01-08 23:04:25]

> Is my understanding incorrect?

Maybe, hard to be sure? I guess in the expected case it works out. No data on
various test harnesses or in exceptional cases or in the face future changes to
various far removed classes.

Depending on the detailed behavior you described of a long series of distant
classes seems brittle. Looks like the code can be rearranged without too much
trouble to not rely on Kill() always being called and always prior to the host
being deleted. I think it'd be an improvement to make it more obviously air
tight. Wdyt?

[horo, 2015-01-09 04:57:52]

On 2015/01/08 23:04:25, michaeln wrote:
> > Is my understanding incorrect?
> 
> Maybe, hard to be sure? I guess in the expected case it works out. No data on
> various test harnesses or in exceptional cases or in the face future changes
to
> various far removed classes.
> 
> Depending on the detailed behavior you described of a long series of distant
> classes seems brittle. Looks like the code can be rearranged without too much
> trouble to not rely on Kill() always being called and always prior to the host
> being deleted. I think it'd be an improvement to make it more obviously air
> tight. Wdyt?

Yes, I agree.
I created a patch: https://codereview.chromium.org/844913002/
Please review it.