Requests proxied by Service Worker breaks HTTP/2 priorities
Reported by
erik.wi...@googlemail.com,
Aug 9
|
||||||||||||
Issue descriptionUserAgent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/68.0.3440.84 Safari/537.36 Steps to reproduce the problem: 1. Load https://testpage-speedkit.app.baqend.com/ an observe that the font.css (which is imported in the main style.css file) is loaded with high priority (i.e. HTTP/2 Stream: 169, weight 256, depends on 0). Alternatively see https://www.webpagetest.org/result/180809_J9_ce853b30bac6f9b1d0674213e9681b2c/1/details/#waterfall_view_step2 2. Install a simple service worker on the domain by going to https://testpage-speedkit.app.baqend.com/install-simple-sw.html the installed Service Worker will look like this: addEventListener('fetch', event => { event.respondWith(fetch(event.request)); }); 3. Load https://testpage-speedkit.app.baqend.com/ again and observe that the font.css is loaded with low priority (i.e. HTTP/2 Stream: 171, weight 220, depends on 169). Alternatively, see https://www.webpagetest.org/result/180809_0W_19b7ffebc47212324553fa6e5c5a5b57/1/details/#waterfall_view_step2 As an effect of the lost priority (i.e. dependency and weight), rendering in blocked by the font.css and therefore takes 6 seconds instead of 3 seconds in the example. I tried to explain the issue in more detail here: https://medium.baqend.com/chromes-service-workers-break-http-2-priorities-649c4e0fa930 What is the expected behavior? The expected behavior is that the request priority is preserved when passing through the Service Worker. What went wrong? The priority of the request is overridden when it passes through the Service Worker. Only the order in which the requests pass through the Service Worker decides their priority. Did this work before? No Does this work in other browsers? No Firefox appears to have a similarly problematic approach. Every request that is proxied through the Service Worker gets the same priority that looks like this: HTTP/2 Stream: X, weight 22, depends on 11 Sadly since the release of the new WebPagetest agent, we cannot see the HTTP/2 priorities in the waterfalls, but still, here they are without Service Worker and with Service Worker. Chrome version: 67.0.3396.99 Channel: n/a OS Version: Flash Version: This issue appears to be described briefly in https://www.researchgate.net/publication/324514529_HTTP2_Prioritization_and_its_Impact_on_Web_Performance section 4.4. However, I could not find any issue here describing the problem, yet.
,
Aug 9
,
Aug 9
,
Aug 9
,
Aug 10
,
Aug 10
If I read the thread right a PR's coming, then we can fix this in impl too.
,
Aug 11
Took a look at this a bit. At first I assumed the network::ResourceRequest we'd be getting from the SW FetchEvent would contain the correct priority, and that we were just failing to copy it over to FetchRequestData when we create a blink::Request to pass into fetch(). From my digging around and debugging, I observe the network::ResourceRequest received in [1] always has priority of net::IDLE however (default upon creation). Assuming this is the ~correct place to be looking, not sure why the priority would not be preserved. Going to look a little more in the mojom files and see if I can find anything with how the DispatchFetchEventParams is constructed that might lead to this. Will be delayed a bit, as I'm moving over the next ~day or 2. Beyond this, I wonder if preserving and transferring the priority will be enough here. IIUC, we will always compute the priority down in ResourceFetcher [2], so even if we preserve the priority early on on the blink::ResourceRequest, we may overwrite it with a priority computed for fetch(), which is _always_ kHigh. Wondering if we might want a way to say "Hey, this resource request already has a priority associated from it, we don't want to generate one here again (which could generate a different priority)". This would be similar to what the Fetch Standard does in [3] > 1 > 5. [1]: https://cs.chromium.org/chromium/src/content/renderer/service_worker/service_worker_context_client.cc?sq=package:chromium&g=0&l=1782 [2]: https://cs.chromium.org/chromium/src/third_party/blink/renderer/platform/loader/fetch/resource_fetcher.cc?sq=package:chromium&g=0&l=679 [3]: https://fetch.spec.whatwg.org/#concept-fetch
,
Aug 13
Maybe we should wait for the upstream fetch standard issue to be resolved? https://github.com/whatwg/fetch/pull/785 Assigning to jakearchibald to make sure this doesn't get dropped. You can send it back for triage when the standard issue is resolved.
,
Aug 27
,
Aug 29
I've got a fix for this at http://crrev.com/c/1192353. A WPT run with the custom browser and the initial script test Erik provided in the initial comment [1] should show that the priority regression is fixed with my change, and > MEDIUM net priorities are preserved when passing through a Service Worker's Fetch Event handler. I'll make a follow-up CL that will ensure intended-LOWEST-priority requests do not get upgraded to net MEDIUM (because of fetch()) priority when passing through a Service Worker's FetchEvent handler (this currently happens now). Thanks for handling the spec change jakearchibald@. [1]: https://www.webpagetest.org/result/180829_JH_cc841bb75e030394c1c99c7f7a4ae211/1/details/#waterfall_view_step2
,
Aug 30
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f141070af9bec5def36d4ca51e2f6c45451875fa commit f141070af9bec5def36d4ca51e2f6c45451875fa Author: Dominic Farolino <domfarolino@gmail.com> Date: Thu Aug 30 06:26:50 2018 Preserve Service Worker fetch priorities This CL preserves the priorities of requests that go through a Service Worker's onfetch event and are fetch()ed from there. Before this CL, these priorities were lost and re-generated later in ResourceFetcher::ComputeLoadPriority with the priority we naturally assign fetch()es (kHigh). This meant that requests for things like render-blocking CSS were downgraded from kVeryHigh => kHigh, possibly causing loading regressions. After this CL, the priorities of these requests are preserved and communicated to the ResourceRequest for the corresponding fetch(). ResourceFetcher::ComputeLoadPriority is still entered, and takes the maximum of the following two priorities: - The one we would naturally generate (kHigh, due to fetch()) - The one _already on_ the ResourceRequest. This fixes priority regressions from kVeryHigh => kHigh, however what it does not do is stop desired low-priority requests from being upgraded to kHigh. For example, if a SW's onfetch event.request is for an image, with kLow priority, ResourceFetcher::ComputeLoadPriority will "upgrade" this request to kHigh. This is probably undesirable, and will need a follow-up CL. R=kinuko@chromium.org, kouhei@chromium.org, yhirano@chromium.org, yoav@yoav.ws Bug: 872776 Change-Id: Iaa6b5420b65bc6004b5b7a2d1f8d0eb5453df872 Reviewed-on: https://chromium-review.googlesource.com/1192353 Commit-Queue: Dominic Farolino <domfarolino@gmail.com> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Reviewed-by: Makoto Shimazu <shimazu@chromium.org> Reviewed-by: Yutaka Hirano <yhirano@chromium.org> Cr-Commit-Position: refs/heads/master@{#587457} [modify] https://crrev.com/f141070af9bec5def36d4ca51e2f6c45451875fa/content/renderer/loader/web_url_loader_impl.cc [modify] https://crrev.com/f141070af9bec5def36d4ca51e2f6c45451875fa/content/renderer/service_worker/service_worker_context_client.cc [modify] https://crrev.com/f141070af9bec5def36d4ca51e2f6c45451875fa/third_party/blink/public/platform/modules/service_worker/web_service_worker_request.h [modify] https://crrev.com/f141070af9bec5def36d4ca51e2f6c45451875fa/third_party/blink/renderer/core/fetch/fetch_manager.cc [modify] https://crrev.com/f141070af9bec5def36d4ca51e2f6c45451875fa/third_party/blink/renderer/core/fetch/fetch_request_data.cc [modify] https://crrev.com/f141070af9bec5def36d4ca51e2f6c45451875fa/third_party/blink/renderer/core/fetch/fetch_request_data.h [modify] https://crrev.com/f141070af9bec5def36d4ca51e2f6c45451875fa/third_party/blink/renderer/core/fetch/request.cc [modify] https://crrev.com/f141070af9bec5def36d4ca51e2f6c45451875fa/third_party/blink/renderer/platform/exported/web_service_worker_request.cc
,
Sep 21
With the above CL, the Blink priority of font.css is kVeryHigh ("Highest" in DevTools, "HIGHEST" in content/net layer), and locally I observe the stream weight being set to 256 in a netlog capture, however subsequent runs on WPT in Canary [1] are not showing the priority any higher, which is strange. Looking into it.
,
Sep 21
,
Sep 21
Any chance you have the link to the WebPageTest result? The HTTP/2 stream weight should give you the most accurate read of the underlying priority that actually went out. The displayed "Priority" comes from the dev tools events but the HTTP/2 details should come from the netlog trace events.
,
Sep 21
Ah sorry, forgot to include it. It also has a a netlog captured which seems to show "HIGHEST" priority, and then "MEDIUM" for some reason (could this be the request for font.css before and after going through SW, would netlog capture this)? https://www.webpagetest.org/result/180921_PP_90d757bd3e66a16f9cdec132d13173f2/ Oddly enough, I see only "HIGHEST" in a netlog captured locally.
,
Sep 28
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f95c163a6592fd71f2c455e3c5906cc0aa8bce87 commit f95c163a6592fd71f2c455e3c5906cc0aa8bce87 Author: Dominic Farolino <domfarolino@gmail.com> Date: Fri Sep 28 14:58:34 2018 Add getResourcePriority to WorkerInternals This CL adds the getResourcePriority method to the WorkerInternals interface via the partial interface defined in core/fetch. This will help testing resource priorities fetched from worker contexts (i.e., Service Workers). Before this CL, getResourcePriority was only exposed on Internals, and thus all ResourceFetchers associated with WorkerGlobalScopes could not be queried. R=kinuko@chromium.org, yhirano@chromium.org, yoav@yoav.ws Bug: 872776 Change-Id: I010b95441c191cf90dfda7b0d5f58c7e7d6c7c21 Reviewed-on: https://chromium-review.googlesource.com/1249882 Reviewed-by: Yutaka Hirano <yhirano@chromium.org> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Commit-Queue: Dominic Farolino <domfarolino@gmail.com> Cr-Commit-Position: refs/heads/master@{#595091} [modify] https://crrev.com/f95c163a6592fd71f2c455e3c5906cc0aa8bce87/third_party/blink/renderer/core/fetch/testing/worker_internals_fetch.cc [modify] https://crrev.com/f95c163a6592fd71f2c455e3c5906cc0aa8bce87/third_party/blink/renderer/core/fetch/testing/worker_internals_fetch.h [modify] https://crrev.com/f95c163a6592fd71f2c455e3c5906cc0aa8bce87/third_party/blink/renderer/core/fetch/testing/worker_internals_fetch.idl
,
Nov 9
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0f0d0b74734bf82737b893d03ed334ef360f9b0b commit 0f0d0b74734bf82737b893d03ed334ef360f9b0b Author: Dominic Farolino <domfarolino@gmail.com> Date: Fri Nov 09 01:18:46 2018 Add resource load priority Service Worker tests This CL adds resource load priority tests ensuring that the priority of requests that pass through a Service Worker is preserved. R=kinuko@chromium.org, kouhei@chromium.org, yhirano@chromium.org Bug: 872776 Change-Id: I74f334359b3255fcac5c13fbdb9f1e601af0f8f0 Reviewed-on: https://chromium-review.googlesource.com/c/1316104 Commit-Queue: Dominic Farolino <domfarolino@gmail.com> Reviewed-by: Matt Falkenhagen <falken@chromium.org> Reviewed-by: Kouhei Ueno <kouhei@chromium.org> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Reviewed-by: Yutaka Hirano <yhirano@chromium.org> Cr-Commit-Position: refs/heads/master@{#606689} [add] https://crrev.com/0f0d0b74734bf82737b893d03ed334ef360f9b0b/third_party/WebKit/LayoutTests/http/tests/priorities/resource-load-priorities-service-worker.https-expected.txt [add] https://crrev.com/0f0d0b74734bf82737b893d03ed334ef360f9b0b/third_party/WebKit/LayoutTests/http/tests/priorities/resource-load-priorities-service-worker.https.html [modify] https://crrev.com/0f0d0b74734bf82737b893d03ed334ef360f9b0b/third_party/WebKit/LayoutTests/http/tests/priorities/resources/common.js [add] https://crrev.com/0f0d0b74734bf82737b893d03ed334ef360f9b0b/third_party/WebKit/LayoutTests/http/tests/priorities/resources/service-worker/async-script.html [add] https://crrev.com/0f0d0b74734bf82737b893d03ed334ef360f9b0b/third_party/WebKit/LayoutTests/http/tests/priorities/resources/service-worker/defer-script.html [add] https://crrev.com/0f0d0b74734bf82737b893d03ed334ef360f9b0b/third_party/WebKit/LayoutTests/http/tests/priorities/resources/service-worker/fetch.html [add] https://crrev.com/0f0d0b74734bf82737b893d03ed334ef360f9b0b/third_party/WebKit/LayoutTests/http/tests/priorities/resources/service-worker/module-script.html [add] https://crrev.com/0f0d0b74734bf82737b893d03ed334ef360f9b0b/third_party/WebKit/LayoutTests/http/tests/priorities/resources/service-worker/off-screen-image.html [add] https://crrev.com/0f0d0b74734bf82737b893d03ed334ef360f9b0b/third_party/WebKit/LayoutTests/http/tests/priorities/resources/service-worker/parser-blocking-script.html [add] https://crrev.com/0f0d0b74734bf82737b893d03ed334ef360f9b0b/third_party/WebKit/LayoutTests/http/tests/priorities/resources/service-worker/prefetch.html [add] https://crrev.com/0f0d0b74734bf82737b893d03ed334ef360f9b0b/third_party/WebKit/LayoutTests/http/tests/priorities/resources/service-worker/render-blocking-stylesheet.html [add] https://crrev.com/0f0d0b74734bf82737b893d03ed334ef360f9b0b/third_party/WebKit/LayoutTests/http/tests/priorities/resources/service-worker/xhr.html [add] https://crrev.com/0f0d0b74734bf82737b893d03ed334ef360f9b0b/third_party/WebKit/LayoutTests/http/tests/priorities/service-worker-get-priority.js
,
Nov 9
,
Nov 9
,
Nov 9
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c01e9b4c3145dbde2c4d472ab75dd00937663981 commit c01e9b4c3145dbde2c4d472ab75dd00937663981 Author: Corentin Wallez <cwallez@chromium.org> Date: Fri Nov 09 13:57:13 2018 Revert "Add resource load priority Service Worker tests" This reverts commit 0f0d0b74734bf82737b893d03ed334ef360f9b0b. Reason for revert: super flaky on win7_chromium_rel_ng BUG= chromium:872776 Original change's description: > Add resource load priority Service Worker tests > > This CL adds resource load priority tests ensuring that the priority of > requests that pass through a Service Worker is preserved. > > R=kinuko@chromium.org, kouhei@chromium.org, yhirano@chromium.org > > Bug: 872776 > Change-Id: I74f334359b3255fcac5c13fbdb9f1e601af0f8f0 > Reviewed-on: https://chromium-review.googlesource.com/c/1316104 > Commit-Queue: Dominic Farolino <domfarolino@gmail.com> > Reviewed-by: Matt Falkenhagen <falken@chromium.org> > Reviewed-by: Kouhei Ueno <kouhei@chromium.org> > Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> > Reviewed-by: Yutaka Hirano <yhirano@chromium.org> > Cr-Commit-Position: refs/heads/master@{#606689} TBR=falken@chromium.org,kinuko@chromium.org,yhirano@chromium.org,kouhei@chromium.org,domfarolino@gmail.com Change-Id: I6d997795ffc64f1005ed3821799300cd3401eea9 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 872776 Reviewed-on: https://chromium-review.googlesource.com/c/1329245 Reviewed-by: Corentin Wallez <cwallez@chromium.org> Commit-Queue: Corentin Wallez <cwallez@chromium.org> Cr-Commit-Position: refs/heads/master@{#606822} [delete] https://crrev.com/089805f4087998a3c196305ad8f13cc488bb7655/third_party/WebKit/LayoutTests/http/tests/priorities/resource-load-priorities-service-worker.https-expected.txt [delete] https://crrev.com/089805f4087998a3c196305ad8f13cc488bb7655/third_party/WebKit/LayoutTests/http/tests/priorities/resource-load-priorities-service-worker.https.html [modify] https://crrev.com/c01e9b4c3145dbde2c4d472ab75dd00937663981/third_party/WebKit/LayoutTests/http/tests/priorities/resources/common.js [delete] https://crrev.com/089805f4087998a3c196305ad8f13cc488bb7655/third_party/WebKit/LayoutTests/http/tests/priorities/resources/service-worker/async-script.html [delete] https://crrev.com/089805f4087998a3c196305ad8f13cc488bb7655/third_party/WebKit/LayoutTests/http/tests/priorities/resources/service-worker/defer-script.html [delete] https://crrev.com/089805f4087998a3c196305ad8f13cc488bb7655/third_party/WebKit/LayoutTests/http/tests/priorities/resources/service-worker/fetch.html [delete] https://crrev.com/089805f4087998a3c196305ad8f13cc488bb7655/third_party/WebKit/LayoutTests/http/tests/priorities/resources/service-worker/module-script.html [delete] https://crrev.com/089805f4087998a3c196305ad8f13cc488bb7655/third_party/WebKit/LayoutTests/http/tests/priorities/resources/service-worker/off-screen-image.html [delete] https://crrev.com/089805f4087998a3c196305ad8f13cc488bb7655/third_party/WebKit/LayoutTests/http/tests/priorities/resources/service-worker/parser-blocking-script.html [delete] https://crrev.com/089805f4087998a3c196305ad8f13cc488bb7655/third_party/WebKit/LayoutTests/http/tests/priorities/resources/service-worker/prefetch.html [delete] https://crrev.com/089805f4087998a3c196305ad8f13cc488bb7655/third_party/WebKit/LayoutTests/http/tests/priorities/resources/service-worker/render-blocking-stylesheet.html [delete] https://crrev.com/089805f4087998a3c196305ad8f13cc488bb7655/third_party/WebKit/LayoutTests/http/tests/priorities/resources/service-worker/xhr.html [delete] https://crrev.com/089805f4087998a3c196305ad8f13cc488bb7655/third_party/WebKit/LayoutTests/http/tests/priorities/service-worker-get-priority.js |
||||||||||||
►
Sign in to add a comment |
||||||||||||
Comment 1 by viswa.karala@chromium.org
, Aug 9