Bug 202787 - Do not timeout a load intercepted by service worker that receives a response
Summary: Do not timeout a load intercepted by service worker that receives a response
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Service Workers (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-10-10 02:57 PDT by youenn fablet
Modified: 2019-10-10 12:53 PDT (History)
6 users (show)

See Also:


Attachments
Patch (15.43 KB, patch)
2019-10-10 03:47 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (15.45 KB, patch)
2019-10-10 11:58 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2019-10-10 02:57:08 PDT
Do not timeout a load intercepted by service worker that receives a response.
Comment 1 youenn fablet 2019-10-10 03:47:30 PDT
Created attachment 380628 [details]
Patch
Comment 2 Chris Dumez 2019-10-10 08:48:00 PDT
Comment on attachment 380628 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=380628&action=review

> Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.cpp:94
>      m_timeoutTimer.stop();

Do we need this? Can didFinish() be called without didReceiveResponse() getting called first? Maybe this could be an assertion?
Comment 3 youenn fablet 2019-10-10 08:51:40 PDT
(In reply to Chris Dumez from comment #2)
> Comment on attachment 380628 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=380628&action=review
> 
> > Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.cpp:94
> >      m_timeoutTimer.stop();
> 
> Do we need this? Can didFinish() be called without didReceiveResponse()
> getting called first? Maybe this could be an assertion?

Rob told me that this happens in a few case apparently and works fine for ResourceLoader.

Stopping it there is mostly for consistency and is not strictly needed since didFinish will synchronously destroy the ServiceWorkerFetchTask.
Comment 4 youenn fablet 2019-10-10 08:52:32 PDT
(In reply to youenn fablet from comment #3)
> (In reply to Chris Dumez from comment #2)
> > Comment on attachment 380628 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=380628&action=review
> > 
> > > Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.cpp:94
> > >      m_timeoutTimer.stop();
> > 
> > Do we need this? Can didFinish() be called without didReceiveResponse()
> > getting called first? Maybe this could be an assertion?
> 
> Rob told me that this happens in a few case apparently and works fine for
> ResourceLoader.

That said, our service worker fetch handler is probably enforcing to either send a didReceiveResponse or didFail.
Will add an ASSERT.
Comment 5 youenn fablet 2019-10-10 11:58:08 PDT
Created attachment 380669 [details]
Patch for landing
Comment 6 WebKit Commit Bot 2019-10-10 12:52:11 PDT
Comment on attachment 380669 [details]
Patch for landing

Clearing flags on attachment: 380669

Committed r250985: <https://trac.webkit.org/changeset/250985>
Comment 7 WebKit Commit Bot 2019-10-10 12:52:12 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Radar WebKit Bug Importer 2019-10-10 12:53:19 PDT
<rdar://problem/56165481>