Issue metadata
Sign in to add a comment
|
NoStatePrefetch doesn't work on heavy pages
Reported by
ana...@yandex-team.ru,
Sep 28
|
||||||||||||||||||||||
Issue descriptionUserAgent: Mozilla/5.0 (Windows NT 10.0; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/69.0.3497.100 YaBrowser/18.10.1.226 (beta) Yowser/2.5 Safari/537.36 Steps to reproduce the problem: The reason it that the ResourceLoadScheduler reaches the kTightLimitForRendererSideResourceScheduler = 2. So when the page for prefetch has more that two resources for loading, NoStatePrefetch will never finish. When we getting new request which can be throttled, we ask: (running_throttleable_requests_.size() >= GetOutstandingLimit()) for decision: run it right now or put to throttleable_queue. And in case NoStatePrefetch our request never erase from running_throttleable_requests_, so request from throttleable_queue will never run. In normal pipeline (for exaple for background tabs) requests will be erase from queue here: https://cs.chromium.org/chromium/src/third_party/blink/renderer/platform/loader/fetch/resource_loader.cc?type=cs&g=0&l=901 from WebURLLoaderImpl::RequestPeerImpl::OnCompletedRequest. For NoStatePrefetch subresources we override this function and use SinkPeer:: OnCompletedRequest(), which cancels request for ResourceDispatcher, but not for ResourceLoadScheduler. I attach the page with example. You can check it in NoStatePrefetchBrowserTest. A simple fix for it: context_->OnCompletedRequest(status) in WebURLLoaderImpl::SinkPeer::OnCompletedRequest(); Thanks! What is the expected behavior? NoStatePrefetch finishes. What went wrong? NoStatePrefetch doesn't finish and doesn't fetch some resources. Did this work before? Yes I think this happened after this CL: https://chromium.googlesource.com/chromium/src/+/8eee3a363357656b645438dd3e98b2d309b0f37e%5E%21/#F0 Chrome version: 71.0.3563.0 Channel: stable OS Version: 10.0 Flash Version: Shockwave Flash 30.0 r0
,
Oct 1
anakir@ - Thanks for filing the issue...!! Could you please provide a screen cast/screenshot for better understanding of the issue. This will help us in triaging the issue further. Observed that on opening the prefetch_heavy_page.html in chrome, 20 images did not load as in the attached screen shot. Thanks...!!
,
Oct 1
Hi krajshree@! Sorry, this html was for adding to NoStatePrefetchBrowserTest for developers as simple way to reproduce the problem. The problem looks like: the first table from browser://net-internals/#prerender contains Prerender Page(with PrerenderMode=PREFETCH_ONLY) and it's live there very long time. In normal case it should finished in quite short time and moved to second table with status "NoStatePrefetch Finished". You can reproduce it in browser like this: 1. Set Flag NoStatePrefetch in chrome://flags 2. Make that in table in chrome://predictors line with some page on Facebook was green. In example it https://www.facebook.com/yandex For this you should go to https://www.facebook.com/yandex, and after that type "f" in omnibox, omnibox will be suggest to you "https://www.facebook.com/yandex", make it few times and in table you should see green line with "https://www.facebook.com/yandex" 3. After that open in new tab chrome://net-internals/#prerender (like on screenshot) and type "f" again. You will see, like in first table will live prerender. For check correct behavior you can make same steps but with light page (for example just for http://google.ru) Thanks!
,
Oct 1
Thank you for providing more feedback. Adding the requester to the cc list. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Oct 1
,
Oct 2
Sounds like we should probably disable the throttling for nostateprefetch requests.
,
Oct 2
I guess we can continue throttling but we should removing requests from queue, when requests finished. This starting work if we add here https://cs.chromium.org/chromium/src/content/renderer/loader/web_url_loader_impl.cc?type=cs&q=SinkPeer&g=0&l=508 this line: context_->OnCompletedRequest(status); for example.
,
Oct 11
pasko@ Could you please handle this issue?
,
Oct 11
awesome report with a suggested fix!! huge thanks! I think throttling in ResourceLoadScheduler is not too bad (prefetch requests are low priority anyway, so even if the RLS is not eating them, they'd be throttled). The idea to release the requests from SinkPeer looks good to me (though I am no expert in loader). I'll give it a try tomorrow, looks easy, will report on progress here.
,
Oct 11
This seems to be caused by crbug.com/729954 (Background throttling with Loading Dispatcher v0). Good catch!
,
Oct 12
proposed change: https://chromium-review.googlesource.com/c/chromium/src/+/1278799
,
Oct 25
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b637b7a7103c66f052d42b673f5cf14953f30ce3 commit b637b7a7103c66f052d42b673f5cf14953f30ce3 Author: Egor Pasko <pasko@chromium.org> Date: Thu Oct 25 09:44:10 2018 NoStatePrefetch: allow more than 2 requests Currently when the Background Tab Resource Load Throttling ([1]) is enabled, the nostate-prefetch webcontents is throttled to 2 requests at a time. When more than 2 requests are made by nostate-prefetch of a single page (including the main resource), the renderer hangs and cannot kill itself. It gets killed after a long timeout. Solution: Bypass the ResourceLoadScheduler for subresources of nostate-prefetch. These requests are of idle priority in the network stack. This means they will wait in the global queue and allowed only one at a time. This is more restrictive than the most aggressive ResourceLoadScheduler throttling policy. Also add two tests: 1. Make sure 4 image requests go through when the page is prefetched 2. Make sure the prefetched resources get reused from cache The bug has a good description of the problem in the very first comment. [1] Background Tab Resource Load Throttling https://www.chromestatus.com/feature/5527160148197376 Bug: 890337 Change-Id: Ifdb5300aba17d95dab14d32090dec4e786fffeb3 Reviewed-on: https://chromium-review.googlesource.com/c/1278799 Reviewed-by: Yutaka Hirano <yhirano@chromium.org> Reviewed-by: Kentaro Hara <haraken@chromium.org> Reviewed-by: Matthew Cary <mattcary@chromium.org> Commit-Queue: Egor Pasko <pasko@chromium.org> Cr-Commit-Position: refs/heads/master@{#602654} [modify] https://crrev.com/b637b7a7103c66f052d42b673f5cf14953f30ce3/chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc [add] https://crrev.com/b637b7a7103c66f052d42b673f5cf14953f30ce3/chrome/test/data/prerender/image2.png [add] https://crrev.com/b637b7a7103c66f052d42b673f5cf14953f30ce3/chrome/test/data/prerender/prefetch_page_bigger.html [modify] https://crrev.com/b637b7a7103c66f052d42b673f5cf14953f30ce3/content/renderer/loader/request_extra_data.cc [modify] https://crrev.com/b637b7a7103c66f052d42b673f5cf14953f30ce3/content/renderer/loader/request_extra_data.h [modify] https://crrev.com/b637b7a7103c66f052d42b673f5cf14953f30ce3/content/renderer/loader/web_url_loader_impl.cc [modify] https://crrev.com/b637b7a7103c66f052d42b673f5cf14953f30ce3/content/renderer/render_frame_impl.cc [modify] https://crrev.com/b637b7a7103c66f052d42b673f5cf14953f30ce3/third_party/blink/public/platform/web_url_request.h [modify] https://crrev.com/b637b7a7103c66f052d42b673f5cf14953f30ce3/third_party/blink/renderer/platform/exported/web_url_request.cc [modify] https://crrev.com/b637b7a7103c66f052d42b673f5cf14953f30ce3/third_party/blink/renderer/platform/loader/fetch/resource_loader.cc [modify] https://crrev.com/b637b7a7103c66f052d42b673f5cf14953f30ce3/third_party/blink/renderer/platform/loader/fetch/resource_request.cc [modify] https://crrev.com/b637b7a7103c66f052d42b673f5cf14953f30ce3/third_party/blink/renderer/platform/loader/fetch/resource_request.h
,
Nov 8
I'm hesitating between cherry-picking or not. If it shows up significant in the background throttling experiment, then I'd say cherry-pick, otherwise maybe dodge the risk of cherry-picks to Beta.. Asked in crbug.com/729954 |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by phanindra.mandapaka@chromium.org
, Sep 30