Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2645493002:
Increase the lifetime of Navigation Preload related objects. (Closed)
|
Created:
3 years, 11 months ago by horo Modified:
3 years, 11 months ago CC:
chromium-reviews, michaeln, jsbell+serviceworker_chromium.org, kenjibaheux+watch_chromium.org, mlamouri+watch-content_chromium.org, shimazu+serviceworker_chromium.org, serviceworker-reviews, jam, nhiroki, kinuko+serviceworker, jshin+watch_chromium.org, horo+watch_chromium.org, blink-reviews, darin-cc_chromium.org, kinuko+watch, tzik, falken+watch_chromium.org, blink-worker-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIncrease the lifetime of Navigation Preload related objects.
In the current implementation, the navigation preload requests are canceled
when the FetchEvent finished. But in this behavior,
"event.respondWith(event.preloadResponse);" doesn't work well if the response is
bigger than the buffer size or the response stream was flushed.
To fix this, this cl changes the lifetime of the
the ServiceWorkerContextClient::NavigationPreloadRequest in the Service Worker
thread and the mojom::URLLoader related objects in the browser process.
ServiceWorkerContextClient::NavigationPreloadRequest will be deleted when
it's OnReceiveRedirect() or OnComplete() are called.
The mojom::URLLoader related objects in the browser process will be deleted when
both the ServiceWorkerURLRequestJob and the FetchEvent finished.
BUG=649558
Review-Url: https://codereview.chromium.org/2645493002
Cr-Commit-Position: refs/heads/master@{#444690}
Committed: https://chromium.googlesource.com/chromium/src/+/6d38a91f37e0a80f708320ca7fb47aabc0288b16
Patch Set 1 #Patch Set 2 : check fetchEvent in ServiceWorkerGlobalScopeProxy::onNavigationPreloadError #
Total comments: 22
Patch Set 3 : incorporated falken's comment #
Total comments: 4
Patch Set 4 : incorporated falken's comment #Patch Set 5 : send abort error and fix flaky test and add abort-when-responded layouttest #
Total comments: 2
Patch Set 6 : Keep the URL loader related assetsalive while the FetchEvent is onging. #
Total comments: 8
Patch Set 7 : incorporated falken's comment #Patch Set 8 : fix broken-chunked-encoding-worker.js #
Total comments: 2
Patch Set 9 : fix broken-chunked-encoding-worker.js #Dependent Patchsets: Messages
Total messages: 72 (53 generated)
The CQ bit was checked by horo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by horo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:1) has been deleted
The CQ bit was checked by horo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:20001) has been deleted
Description was changed from ========== Keep NavPreload related objects BUG= ========== to ========== Increase the lifetime of Navigation Preload related objects. In the current implementation, the navigation preload requests are canceled when the FetchEvent finished. But this behavior is not written in the Service Worker spec. And also "event.respondWith(event.preloadResponse);" doesn't work if the response is bigger than the buffer size or the response stream was flushed. To fix this, this cl changes the lifetime of the ServiceWorkerContextClient::NavigationPreloadRequest in the Service Worker thread and the mojom::URLLoader related objects in the browser process. ServiceWorkerContextClient::NavigationPreloadRequest will be deleted when it's OnReceiveRedirect() or OnComplete() are called. mojom::URLLoader related objects in the browser process will be deleted when the ServiceWorkerURLRequestJob finished. BUG=649558 ==========
Description was changed from ========== Increase the lifetime of Navigation Preload related objects. In the current implementation, the navigation preload requests are canceled when the FetchEvent finished. But this behavior is not written in the Service Worker spec. And also "event.respondWith(event.preloadResponse);" doesn't work if the response is bigger than the buffer size or the response stream was flushed. To fix this, this cl changes the lifetime of the ServiceWorkerContextClient::NavigationPreloadRequest in the Service Worker thread and the mojom::URLLoader related objects in the browser process. ServiceWorkerContextClient::NavigationPreloadRequest will be deleted when it's OnReceiveRedirect() or OnComplete() are called. mojom::URLLoader related objects in the browser process will be deleted when the ServiceWorkerURLRequestJob finished. BUG=649558 ========== to ========== Increase the lifetime of Navigation Preload related objects. In the current implementation, the navigation preload requests are canceled when the FetchEvent finished. But this behavior is not written in the Service Worker spec. And also "event.respondWith(event.preloadResponse);" doesn't work if the response is bigger than the buffer size or the response stream was flushed. To fix this, this cl changes the lifetime of the ServiceWorkerContextClient::NavigationPreloadRequest in the Service Worker thread and the mojom::URLLoader related objects in the browser process. ServiceWorkerContextClient::NavigationPreloadRequest will be deleted when it's OnReceiveRedirect() or OnComplete() are called. The mojom::URLLoader related objects in the browser process will be deleted when the ServiceWorkerURLRequestJob finished. BUG=649558 ==========
Description was changed from ========== Increase the lifetime of Navigation Preload related objects. In the current implementation, the navigation preload requests are canceled when the FetchEvent finished. But this behavior is not written in the Service Worker spec. And also "event.respondWith(event.preloadResponse);" doesn't work if the response is bigger than the buffer size or the response stream was flushed. To fix this, this cl changes the lifetime of the ServiceWorkerContextClient::NavigationPreloadRequest in the Service Worker thread and the mojom::URLLoader related objects in the browser process. ServiceWorkerContextClient::NavigationPreloadRequest will be deleted when it's OnReceiveRedirect() or OnComplete() are called. The mojom::URLLoader related objects in the browser process will be deleted when the ServiceWorkerURLRequestJob finished. BUG=649558 ========== to ========== Increase the lifetime of Navigation Preload related objects. In the current implementation, the navigation preload requests are canceled when the FetchEvent finished. But this behavior is not written in the Service Worker spec. And also "event.respondWith(event.preloadResponse);" doesn't work if the response is bigger than the buffer size or the response stream was flushed. To fix this, this cl changes the lifetime of the the ServiceWorkerContextClient::NavigationPreloadRequest in the Service Worker thread and the mojom::URLLoader related objects in the browser process. ServiceWorkerContextClient::NavigationPreloadRequest will be deleted when it's OnReceiveRedirect() or OnComplete() are called. The mojom::URLLoader related objects in the browser process will be deleted when the ServiceWorkerURLRequestJob finished. BUG=649558 ==========
horo@chromium.org changed reviewers: + falken@chromium.org
falken@ Could you please review this?
Patchset #2 (id:60001) has been deleted
The CQ bit was checked by horo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2645493002/diff/80001/content/browser/service... File content/browser/service_worker/service_worker_url_request_job.cc (right): https://codereview.chromium.org/2645493002/diff/80001/content/browser/service... content/browser/service_worker/service_worker_url_request_job.cc:568: // Do not clear |fetch_dispatcher_| if it has dispached a navigation prelaod nit: dispatched https://codereview.chromium.org/2645493002/diff/80001/content/browser/service... content/browser/service_worker/service_worker_url_request_job.cc:569: // request to keep the mojom::URLLoader related objects in it. Ah, I see... can you add something to the comment like "because the preload response might still need to be streamed even after calling respondWith()." If that's correct? https://codereview.chromium.org/2645493002/diff/80001/content/renderer/servic... File content/renderer/service_worker/service_worker_context_client.cc (right): https://codereview.chromium.org/2645493002/diff/80001/content/renderer/servic... content/renderer/service_worker/service_worker_context_client.cc:287: // This will delete itself. nit: Slightly prefer |this| instead of "itself" https://codereview.chromium.org/2645493002/diff/80001/content/renderer/servic... content/renderer/service_worker/service_worker_context_client.cc:310: ReportErrorToClient("Service Worker navigation preload network error."); Does this also delete |this|? https://codereview.chromium.org/2645493002/diff/80001/content/renderer/servic... content/renderer/service_worker/service_worker_context_client.cc:1259: context_->preload_requests.Remove(fetch_event_id); So either OnError or OnComplete must be called, but not both, right? Could be worth in the header file. https://codereview.chromium.org/2645493002/diff/80001/content/renderer/servic... File content/renderer/service_worker/service_worker_context_client.h (right): https://codereview.chromium.org/2645493002/diff/80001/content/renderer/servic... content/renderer/service_worker/service_worker_context_client.h:274: void OnNavigationPreloadComplete(int fetch_event_id); Maybe it's time for a short comment for the diff between Response, Error, and Complete. // Called to resolve the FetchEvent.preloadResponse promise. void OnResponse // Called to reject the FetchEvent.preloadResponse promise. void OnError // Called to....?? void OnComplete https://codereview.chromium.org/2645493002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/serviceworker/navigation-preload/broken-chunked-encoding.html (right): https://codereview.chromium.org/2645493002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/navigation-preload/broken-chunked-encoding.html:13: registration = reg; add_completion_callback to unregister https://codereview.chromium.org/2645493002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/serviceworker/navigation-preload/chunked-encoding.html (right): https://codereview.chromium.org/2645493002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/navigation-preload/chunked-encoding.html:13: registration = reg; ditto: unregister https://codereview.chromium.org/2645493002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/serviceworker/navigation-preload/resources/broken-chunked-encoding-worker.js (right): https://codereview.chromium.org/2645493002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/navigation-preload/resources/broken-chunked-encoding-worker.js:9: .then(text => new Response('Done'))); Do we care what |text| is? Doesn't matter as long as it resolves? https://codereview.chromium.org/2645493002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/serviceworker/navigation-preload/resources/chunked-encoding-scope.php (right): https://codereview.chromium.org/2645493002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/navigation-preload/resources/chunked-encoding-scope.php:10: usleep(10000); Can you comment why we need usleep and how you chose 10,000? BTW https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/http/test... used 1000 us.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
https://codereview.chromium.org/2645493002/diff/80001/content/browser/service... File content/browser/service_worker/service_worker_url_request_job.cc (right): https://codereview.chromium.org/2645493002/diff/80001/content/browser/service... content/browser/service_worker/service_worker_url_request_job.cc:568: // Do not clear |fetch_dispatcher_| if it has dispached a navigation prelaod On 2017/01/18 14:24:58, falken wrote: > nit: dispatched Done. https://codereview.chromium.org/2645493002/diff/80001/content/browser/service... content/browser/service_worker/service_worker_url_request_job.cc:569: // request to keep the mojom::URLLoader related objects in it. On 2017/01/18 14:24:58, falken wrote: > Ah, I see... can you add something to the comment like "because the preload > response might still need to be streamed even after calling respondWith()." If > that's correct? Yes. Done. https://codereview.chromium.org/2645493002/diff/80001/content/renderer/servic... File content/renderer/service_worker/service_worker_context_client.cc (right): https://codereview.chromium.org/2645493002/diff/80001/content/renderer/servic... content/renderer/service_worker/service_worker_context_client.cc:287: // This will delete itself. On 2017/01/18 14:24:58, falken wrote: > nit: Slightly prefer |this| instead of "itself" Done. https://codereview.chromium.org/2645493002/diff/80001/content/renderer/servic... content/renderer/service_worker/service_worker_context_client.cc:310: ReportErrorToClient("Service Worker navigation preload network error."); On 2017/01/18 14:24:58, falken wrote: > Does this also delete |this|? Done. https://codereview.chromium.org/2645493002/diff/80001/content/renderer/servic... content/renderer/service_worker/service_worker_context_client.cc:1259: context_->preload_requests.Remove(fetch_event_id); On 2017/01/18 14:24:58, falken wrote: > So either OnError or OnComplete must be called, but not both, right? Could be > worth in the header file. Done. https://codereview.chromium.org/2645493002/diff/80001/content/renderer/servic... File content/renderer/service_worker/service_worker_context_client.h (right): https://codereview.chromium.org/2645493002/diff/80001/content/renderer/servic... content/renderer/service_worker/service_worker_context_client.h:274: void OnNavigationPreloadComplete(int fetch_event_id); On 2017/01/18 14:24:59, falken wrote: > Maybe it's time for a short comment for the diff between Response, Error, and > Complete. > > // Called to resolve the FetchEvent.preloadResponse promise. > void OnResponse > // Called to reject the FetchEvent.preloadResponse promise. > void OnError > // Called to....?? > void OnComplete Done. https://codereview.chromium.org/2645493002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/serviceworker/navigation-preload/broken-chunked-encoding.html (right): https://codereview.chromium.org/2645493002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/navigation-preload/broken-chunked-encoding.html:13: registration = reg; On 2017/01/18 14:24:59, falken wrote: > add_completion_callback to unregister Done. https://codereview.chromium.org/2645493002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/serviceworker/navigation-preload/chunked-encoding.html (right): https://codereview.chromium.org/2645493002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/navigation-preload/chunked-encoding.html:13: registration = reg; On 2017/01/18 14:24:59, falken wrote: > ditto: unregister Done. https://codereview.chromium.org/2645493002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/serviceworker/navigation-preload/resources/broken-chunked-encoding-worker.js (right): https://codereview.chromium.org/2645493002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/navigation-preload/resources/broken-chunked-encoding-worker.js:9: .then(text => new Response('Done'))); On 2017/01/18 14:24:59, falken wrote: > Do we care what |text| is? Doesn't matter as long as it resolves? "event.respondWith(event.preloadResponse)" will result with a blank page. We can't know whether the blank page is the expected behavior or not. https://codereview.chromium.org/2645493002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/serviceworker/navigation-preload/resources/chunked-encoding-scope.php (right): https://codereview.chromium.org/2645493002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/navigation-preload/resources/chunked-encoding-scope.php:10: usleep(10000); On 2017/01/18 14:24:59, falken wrote: > Can you comment why we need usleep and how you chose 10,000? BTW > https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/http/test... > used 1000 us. Done. But I have no specific reason to chose 10,000.
lgtm, thanks https://codereview.chromium.org/2645493002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/serviceworker/navigation-preload/resources/broken-chunked-encoding-worker.js (right): https://codereview.chromium.org/2645493002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/navigation-preload/resources/broken-chunked-encoding-worker.js:9: .then(text => new Response('Done'))); On 2017/01/18 16:14:57, horo wrote: > On 2017/01/18 14:24:59, falken wrote: > > Do we care what |text| is? Doesn't matter as long as it resolves? > > "event.respondWith(event.preloadResponse)" will result with a blank page. > > We can't know whether the blank page is the expected behavior or not. You could do something like: if (text === '') return new Response('Done'); else return new Response('fail: got text: ' + text); Or if we really don't care, it's fine as is, I probably would do a comment like "Don't bother testing |text| since we only care if the test crashes or not". https://codereview.chromium.org/2645493002/diff/100001/content/renderer/servi... File content/renderer/service_worker/service_worker_context_client.cc (right): https://codereview.chromium.org/2645493002/diff/100001/content/renderer/servi... content/renderer/service_worker/service_worker_context_client.cc:342: // This will delete itself. nit: |this| https://codereview.chromium.org/2645493002/diff/100001/content/renderer/servi... File content/renderer/service_worker/service_worker_context_client.h (right): https://codereview.chromium.org/2645493002/diff/100001/content/renderer/servi... content/renderer/service_worker/service_worker_context_client.h:276: // Called when an error occured while recieving the response of the navigation nit: occurred, receiving
The CQ bit was checked by horo@chromium.org to run a CQ dry run
Thank you! https://codereview.chromium.org/2645493002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/serviceworker/navigation-preload/resources/broken-chunked-encoding-worker.js (right): https://codereview.chromium.org/2645493002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/navigation-preload/resources/broken-chunked-encoding-worker.js:9: .then(text => new Response('Done'))); On 2017/01/18 16:32:57, falken wrote: > On 2017/01/18 16:14:57, horo wrote: > > On 2017/01/18 14:24:59, falken wrote: > > > Do we care what |text| is? Doesn't matter as long as it resolves? > > > > "event.respondWith(event.preloadResponse)" will result with a blank page. > > > > We can't know whether the blank page is the expected behavior or not. > > You could do something like: > if (text === '') return new Response('Done'); > else return new Response('fail: got text: ' + text); > > Or if we really don't care, it's fine as is, I probably would do a comment like > "Don't bother testing |text| since we only care if the test crashes or not". Done. https://codereview.chromium.org/2645493002/diff/100001/content/renderer/servi... File content/renderer/service_worker/service_worker_context_client.cc (right): https://codereview.chromium.org/2645493002/diff/100001/content/renderer/servi... content/renderer/service_worker/service_worker_context_client.cc:342: // This will delete itself. On 2017/01/18 16:32:58, falken wrote: > nit: |this| Done. https://codereview.chromium.org/2645493002/diff/100001/content/renderer/servi... File content/renderer/service_worker/service_worker_context_client.h (right): https://codereview.chromium.org/2645493002/diff/100001/content/renderer/servi... content/renderer/service_worker/service_worker_context_client.h:276: // Called when an error occured while recieving the response of the navigation On 2017/01/18 16:32:58, falken wrote: > nit: occurred, receiving Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
horo@chromium.org changed reviewers: + haraken@chromium.org
haraken@ Could you please review third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp?
The CQ bit was checked by horo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #5 (id:140001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Description was changed from ========== Increase the lifetime of Navigation Preload related objects. In the current implementation, the navigation preload requests are canceled when the FetchEvent finished. But this behavior is not written in the Service Worker spec. And also "event.respondWith(event.preloadResponse);" doesn't work if the response is bigger than the buffer size or the response stream was flushed. To fix this, this cl changes the lifetime of the the ServiceWorkerContextClient::NavigationPreloadRequest in the Service Worker thread and the mojom::URLLoader related objects in the browser process. ServiceWorkerContextClient::NavigationPreloadRequest will be deleted when it's OnReceiveRedirect() or OnComplete() are called. The mojom::URLLoader related objects in the browser process will be deleted when the ServiceWorkerURLRequestJob finished. BUG=649558 ========== to ========== Change the lifetime of Navigation Preload related objects. In the current implementation, the navigation preload requests are canceled when the FetchEvent finished. But this behavior is not written in the Service Worker spec. And also "event.respondWith(event.preloadResponse);" doesn't work if the response is bigger than the buffer size or the response stream was flushed. To fix this, this cl changes the lifetime of the the ServiceWorkerContextClient::NavigationPreloadRequest in the Service Worker thread and the mojom::URLLoader related objects in the browser process. ServiceWorkerContextClient::NavigationPreloadRequest will be deleted when it's OnReceiveRedirect() or OnComplete() are called. The mojom::URLLoader related objects in the browser process will be deleted when the ServiceWorkerURLRequestJob finished. BUG=649558 ==========
The CQ bit was checked by horo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
falken@ I noticed that Patch Set 3 has a bug that ServiceWorkerContextClient::NavigationPreloadRequest is not released when the page load finished before Chrome receives the preloaded response. I uploaded Patch Set 4 to fix the bug and fix the flaky ServiceWorkerNavigationPreloadTests and add LayoutTest for that situation. Could you please review Patch Set 4?
LGTM
https://codereview.chromium.org/2645493002/diff/160001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/serviceworker/navigation-preload/abort-when-responded.html (right): https://codereview.chromium.org/2645493002/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/serviceworker/navigation-preload/abort-when-responded.html:34: 'network error.'); Testing the error string isn't right for an WPT test. Also the language of "abort" seems implementation-specific, it shoudl say something like "the preloadResponse promise rejects when the page load finished". https://codereview.chromium.org/2645493002/diff/160001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/serviceworker/navigation-preload/resources/abort-when-responded-worker.js (right): https://codereview.chromium.org/2645493002/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/serviceworker/navigation-preload/resources/abort-when-responded-worker.js:12: e => preload_resolve('REJECTED: ' + e.toString()))); Does the spec require this? It seems reasonable for an implementation to still wait on |preloadResponse| and resolve it normally since the JS called waitUntil(preloadResponse). The JS could still do something like put it in the cache for a next fetch. If we land this, I think it should be a chromium test unless we think the spec will require rejecting the preload.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by horo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #6 (id:180001) has been deleted
The CQ bit was checked by horo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Change the lifetime of Navigation Preload related objects. In the current implementation, the navigation preload requests are canceled when the FetchEvent finished. But this behavior is not written in the Service Worker spec. And also "event.respondWith(event.preloadResponse);" doesn't work if the response is bigger than the buffer size or the response stream was flushed. To fix this, this cl changes the lifetime of the the ServiceWorkerContextClient::NavigationPreloadRequest in the Service Worker thread and the mojom::URLLoader related objects in the browser process. ServiceWorkerContextClient::NavigationPreloadRequest will be deleted when it's OnReceiveRedirect() or OnComplete() are called. The mojom::URLLoader related objects in the browser process will be deleted when the ServiceWorkerURLRequestJob finished. BUG=649558 ========== to ========== Increase the lifetime of Navigation Preload related objects. In the current implementation, the navigation preload requests are canceled when the FetchEvent finished. But in this behavior, "event.respondWith(event.preloadResponse);" doesn't work well if the response is bigger than the buffer size or the response stream was flushed. To fix this, this cl changes the lifetime of the the ServiceWorkerContextClient::NavigationPreloadRequest in the Service Worker thread and the mojom::URLLoader related objects in the browser process. ServiceWorkerContextClient::NavigationPreloadRequest will be deleted when it's OnReceiveRedirect() or OnComplete() are called. The mojom::URLLoader related objects in the browser process will be deleted when both the ServiceWorkerURLRequestJob and the FetchEvent finished. BUG=649558 ==========
Patchset #6 (id:200001) has been deleted
The CQ bit was checked by horo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I changed ServiceWorkerFetchDispatcher to keep the URL loader related assets alive while the FetchEvent is onging. And uploaded Patch Set 6. Please review it.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2645493002/diff/220001/content/browser/servic... File content/browser/service_worker/service_worker_fetch_dispatcher.cc (right): https://codereview.chromium.org/2645493002/diff/220001/content/browser/servic... content/browser/service_worker/service_worker_fetch_dispatcher.cc:356: // assets alive while the FetchEvent is onging in the service worker. ongoing https://codereview.chromium.org/2645493002/diff/220001/content/browser/servic... content/browser/service_worker/service_worker_fetch_dispatcher.cc:489: new DelegatingURLLoader(std::move(url_loader_associated_ptr))); nit: this could be MakeUnique(std::move()) https://codereview.chromium.org/2645493002/diff/220001/content/browser/servic... content/browser/service_worker/service_worker_fetch_dispatcher.cc:494: std::move(url_loader_client)); nit MakeUnique https://codereview.chromium.org/2645493002/diff/220001/content/browser/servic... File content/browser/service_worker/service_worker_url_request_job.cc (right): https://codereview.chromium.org/2645493002/diff/220001/content/browser/servic... content/browser/service_worker/service_worker_url_request_job.cc:568: // Do not clear |fetch_dispatcher_| if it has dispatched a navigation prelaod preload
Thank you https://codereview.chromium.org/2645493002/diff/220001/content/browser/servic... File content/browser/service_worker/service_worker_fetch_dispatcher.cc (right): https://codereview.chromium.org/2645493002/diff/220001/content/browser/servic... content/browser/service_worker/service_worker_fetch_dispatcher.cc:356: // assets alive while the FetchEvent is onging in the service worker. On 2017/01/19 05:26:47, falken wrote: > ongoing Done. https://codereview.chromium.org/2645493002/diff/220001/content/browser/servic... content/browser/service_worker/service_worker_fetch_dispatcher.cc:489: new DelegatingURLLoader(std::move(url_loader_associated_ptr))); On 2017/01/19 05:26:46, falken wrote: > nit: this could be MakeUnique(std::move()) Done. https://codereview.chromium.org/2645493002/diff/220001/content/browser/servic... content/browser/service_worker/service_worker_fetch_dispatcher.cc:494: std::move(url_loader_client)); On 2017/01/19 05:26:47, falken wrote: > nit MakeUnique It's not a unique_ptr. https://codereview.chromium.org/2645493002/diff/220001/content/browser/servic... File content/browser/service_worker/service_worker_url_request_job.cc (right): https://codereview.chromium.org/2645493002/diff/220001/content/browser/servic... content/browser/service_worker/service_worker_url_request_job.cc:568: // Do not clear |fetch_dispatcher_| if it has dispatched a navigation prelaod On 2017/01/19 05:26:47, falken wrote: > preload Done.
The CQ bit was checked by horo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, falken@chromium.org Link to the patchset: https://codereview.chromium.org/2645493002/#ps240001 (title: "incorporated falken's comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by horo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #8 (id:150015) has been deleted
The CQ bit was checked by horo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Ah, after this patch https://codereview.chromium.org/2633123002/, OnComplete() with an error is called before OnStartLoadingResponseBody() in broken-chunked-encoding.html test. So I updated the expected behavior in broken-chunked-encoding-worker.js. I think this is more appropriate expected behavior. falken@ Can you review Patch Set8.
lgtm https://codereview.chromium.org/2645493002/diff/270001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/serviceworker/navigation-preload/resources/broken-chunked-encoding-worker.js (right): https://codereview.chromium.org/2645493002/diff/270001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/serviceworker/navigation-preload/resources/broken-chunked-encoding-worker.js:9: .catch(_ => new Response('Done'))); I'd slightly prefer writing this such that 'Done' happens only if |preloadResponse| rejects (and not |res|), but not a big deal.
Thank you! https://codereview.chromium.org/2645493002/diff/270001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/serviceworker/navigation-preload/resources/broken-chunked-encoding-worker.js (right): https://codereview.chromium.org/2645493002/diff/270001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/serviceworker/navigation-preload/resources/broken-chunked-encoding-worker.js:9: .catch(_ => new Response('Done'))); On 2017/01/19 08:09:52, falken wrote: > I'd slightly prefer writing this such that 'Done' happens only if > |preloadResponse| rejects (and not |res|), but not a big deal. Done.
The CQ bit was checked by horo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, falken@chromium.org Link to the patchset: https://codereview.chromium.org/2645493002/#ps290001 (title: "fix broken-chunked-encoding-worker.js")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 290001, "attempt_start_ts": 1484813530114430,
"parent_rev": "e92e9a9d5c688c348a8260c6bcdea2214755014d", "commit_rev":
"6d38a91f37e0a80f708320ca7fb47aabc0288b16"}
Message was sent while issue was closed.
Description was changed from ========== Increase the lifetime of Navigation Preload related objects. In the current implementation, the navigation preload requests are canceled when the FetchEvent finished. But in this behavior, "event.respondWith(event.preloadResponse);" doesn't work well if the response is bigger than the buffer size or the response stream was flushed. To fix this, this cl changes the lifetime of the the ServiceWorkerContextClient::NavigationPreloadRequest in the Service Worker thread and the mojom::URLLoader related objects in the browser process. ServiceWorkerContextClient::NavigationPreloadRequest will be deleted when it's OnReceiveRedirect() or OnComplete() are called. The mojom::URLLoader related objects in the browser process will be deleted when both the ServiceWorkerURLRequestJob and the FetchEvent finished. BUG=649558 ========== to ========== Increase the lifetime of Navigation Preload related objects. In the current implementation, the navigation preload requests are canceled when the FetchEvent finished. But in this behavior, "event.respondWith(event.preloadResponse);" doesn't work well if the response is bigger than the buffer size or the response stream was flushed. To fix this, this cl changes the lifetime of the the ServiceWorkerContextClient::NavigationPreloadRequest in the Service Worker thread and the mojom::URLLoader related objects in the browser process. ServiceWorkerContextClient::NavigationPreloadRequest will be deleted when it's OnReceiveRedirect() or OnComplete() are called. The mojom::URLLoader related objects in the browser process will be deleted when both the ServiceWorkerURLRequestJob and the FetchEvent finished. BUG=649558 Review-Url: https://codereview.chromium.org/2645493002 Cr-Commit-Position: refs/heads/master@{#444690} Committed: https://chromium.googlesource.com/chromium/src/+/6d38a91f37e0a80f708320ca7fb4... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:290001) as https://chromium.googlesource.com/chromium/src/+/6d38a91f37e0a80f708320ca7fb4... |
