NoStatePrefetch running scripts |
|||
Issue descriptionSee bug 819937 where with nostateprefetch there were sync XHRs being created. My understanding, verified by Matt, is that there should be no scripts running.
,
Mar 19 2018
(copying question from other bug) How do you know it's XHR---it's a sync request, but it seems that a sync request could occur in other places, eg while loading an XML document with XLT. Although, I tried to make an XML document with CSS and I didn't see a sync request, so maybe not.
,
Mar 19 2018
Sync XHR is the only sync request that I know of. I'm not aware of others, which doesn't mean that I'm not missing something :) If you have any repro that'd be great. The crashes do have URLs of pages that were being loaded but I never managed to figure out how to get prefetch to trigger for a url.
,
Mar 19 2018
btw I loaded the first 30 urls from the crash reports and the only sync call I saw was sync xhr
,
Mar 20 2018
Ah! I didn't know that URLs were there. The ones off the "Fields" tab are the right ones? The easiest way I've found to trigger nostate is to use a page with <link rel="prerender">. I did that with 3 urls taken from the crash report (2 tumblr, one sarkariresult) and so no sync loads (by adding logging to FetchParameters). I did notice that on the tumblr pages, PrerenderDispatcher::DecrementPrefetchCount doesn't get the prefetch count below 10, and so the prefetch renderer doesn't get destroyed. On the sarkariresult site, however, it behaves as expected. Eyeballing the netlog requests and comparing against actually navigating to the site seems to confirm that the prefetcher is behaving as expected: there are many js and ad resources fetched only on the explicit navigation and not on the prefetch. It is the case that PrerenderDispatcher::DecrementPrefetchCount is called after the prefetch job is itself finished: [1:1:0320/114756.563710:ERROR:prerender_dispatcher.cc(48)] PrerenderDispatcher::DecrementPrefetchCount 16 1 [1:1:0320/114756.564266:ERROR:prerender_dispatcher.cc(48)] PrerenderDispatcher::DecrementPrefetchCount 15 1 [1:1:0320/114805.394574:ERROR:FetchParameters.cpp(44)] FetchParameters::FetchParameters 1: "https://www.tumblr.com/services/cslog" [122636:122636:0320/114807.991500:ERROR:prerender_histograms.cc(77)] PrerenderHistograms::RecordFinalStatus origin: 8 final status: 1 [1:1:0320/114807.993005:ERROR:prerender_dispatcher.cc(48)] PrerenderDispatcher::DecrementPrefetchCount 14 1 [1:1:0320/114807.993527:ERROR:prerender_dispatcher.cc(48)] PrerenderDispatcher::DecrementPrefetchCount 13 1 In the above, the two numbers after DecrementPrefetchCount are prefetch_count_ and prefetch_finished_, respectively. So possibly the stack traces with the sync loads are spurious---the old throttles still hanging on from the prefetch just happen to get destroyed during the explicit page load when a sync load is occurring?
,
Mar 20 2018
Yep, Fields tab then URLs field. I'm not sure I follow what you mean about spurious stack traces. The throttle would only get created if the RenderFrame says it's being prefetched. That RenderFrame or RenderView wouldn't get reused for a non-prefetch load.
,
Mar 20 2018
To make it clear, this is my test case: <html> <head> <link rel="prerender" href="somewhere"> </head> <body> <a href="somewhere">zzz</a> </body> </html> I load this page, kicking off no-state prefetch. Then I click on the link. It appears that the same process is used for both the prefetch and the full navigation. I'm not sure of that, though, I'll have to look in more detail tomorrow in the office. That's not to say that the same renderframe* is used, of course, I'm not familiar with those mechanics. But it does certainly appear that the same PrerenderDispatcher is alive for both those steps, the logs above surround the click of the link. Since, IIUC, DecrementPrefetchCount is fired when the UrlLoaderThrottle is destroyed, is it possible that those instances persist across the navigation? That seems odd to me since this is a cross-site navigation.
,
Mar 21 2018
Well, it's certainly not running in the same process after testing on linux. So that's all probably a red herring. I still haven't had any success at reproducing a sync request from prefetch. I'll look into adding logging of some sort to see if we can get information from the field.
,
Mar 21 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/943f2a2b2927680ff4098d4f568ad59157db88ab commit 943f2a2b2927680ff4098d4f568ad59157db88ab Author: John Abd-El-Malek <jam@chromium.org> Date: Wed Mar 21 17:06:50 2018 Fix crash when NoStatePrefetch is running a sync XHR. I'm not sure why scripts are executing, but until that's figured out this should fix the symptom of the null dereference (and usage of PrerenderDispatcher on the wrong thread). Bug: 819937, 823306 Change-Id: I30cd24c7b5275a8f1a46babc437a9bcf7f6e8348 Reviewed-on: https://chromium-review.googlesource.com/969082 Reviewed-by: Matthew Cary <mattcary@chromium.org> Commit-Queue: John Abd-El-Malek <jam@chromium.org> Cr-Commit-Position: refs/heads/master@{#544743} [modify] https://crrev.com/943f2a2b2927680ff4098d4f568ad59157db88ab/chrome/renderer/url_loader_throttle_provider_impl.cc
,
Mar 21 2018
Still can't reproduce, but I think I've found a possible explanation. See crrev.com/c/973613 for details. I'll have to run it by someone who knows something about parsing to ensure it makes sense.
,
Mar 22 2018
After further examination into the tumblr dashboard page, it looks like this end-of-parsing problem was it. The only sync requests are some sort of exception or teardown handling, which could reasonably be triggered from parse finish handling.
,
Mar 22 2018
Sorry I just saw the last few comments. Please feel free to revert my cl when yours lands, then we can see on canary that the crash doesn't happen anymore without my workaround.
,
Mar 23 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/17a0a62ce923bd9c4101c87f6fb279530b645ba5 commit 17a0a62ce923bd9c4101c87f6fb279530b645ba5 Author: Matthew Cary <mattcary@chromium.org> Date: Fri Mar 23 17:06:07 2018 Prerender: Do not tokenize when ending parser if prefetching. NoState Prefetch is a version of prerendering that should only run the preload scanner on a document. It appears that it's possible for parsing to occurr, however, when the document parser is torn down. This change prevents that from happening and adds a a DCHECK to make sure it doesn't. Bug: 823306 Change-Id: I541515e3f75d726b56f132c1c010367a4aa829d8 Reviewed-on: https://chromium-review.googlesource.com/973613 Commit-Queue: Matthew Cary <mattcary@chromium.org> Reviewed-by: Charlie Harrison <csharrison@chromium.org> Reviewed-by: Kouhei Ueno <kouhei@chromium.org> Cr-Commit-Position: refs/heads/master@{#545491} [modify] https://crrev.com/17a0a62ce923bd9c4101c87f6fb279530b645ba5/third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp [modify] https://crrev.com/17a0a62ce923bd9c4101c87f6fb279530b645ba5/third_party/WebKit/Source/core/html/parser/HTMLDocumentParserTest.cpp
,
Mar 23 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e689ce0554361d3adca55ec3029f2b35d11ecb42 commit e689ce0554361d3adca55ec3029f2b35d11ecb42 Author: Matthew Cary <mattcary@chromium.org> Date: Fri Mar 23 18:35:44 2018 Revert "Fix crash when NoStatePrefetch is running a sync XHR." As per jam@'s suggestion, this should no longer be necessary after crrev.com/c/973613. Reverting this and watching for crashes on canary will confirm this. @jam: It still seems like a good defensive idea to make sure the dispatcher is running on the right thread. But you may have more context. This reverts commit 943f2a2b2927680ff4098d4f568ad59157db88ab. Reason for revert: <INSERT REASONING HERE> Original change's description: > Fix crash when NoStatePrefetch is running a sync XHR. > > I'm not sure why scripts are executing, but until that's figured out this should fix the symptom of the null dereference (and usage of PrerenderDispatcher on the wrong thread). > > Bug: 819937, 823306 > Change-Id: I30cd24c7b5275a8f1a46babc437a9bcf7f6e8348 > Reviewed-on: https://chromium-review.googlesource.com/969082 > Reviewed-by: Matthew Cary <mattcary@chromium.org> > Commit-Queue: John Abd-El-Malek <jam@chromium.org> > Cr-Commit-Position: refs/heads/master@{#544743} TBR=jam@chromium.org,mattcary@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 819937, 823306 Change-Id: Ic6c792e4b0be23e1ce515b2b3aacef0e8b4e8d89 Reviewed-on: https://chromium-review.googlesource.com/977829 Reviewed-by: Matthew Cary <mattcary@chromium.org> Commit-Queue: Matthew Cary <mattcary@chromium.org> Cr-Commit-Position: refs/heads/master@{#545528} [modify] https://crrev.com/e689ce0554361d3adca55ec3029f2b35d11ecb42/chrome/renderer/url_loader_throttle_provider_impl.cc
,
Apr 5 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e028ced7a69551928a5bd5e5a3e619308ae91937 commit e028ced7a69551928a5bd5e5a3e619308ae91937 Author: Matthew Cary <mattcary@chromium.org> Date: Thu Apr 05 19:38:49 2018 Prerender: Do not tokenize when ending parser if prefetching. NoState Prefetch is a version of prerendering that should only run the preload scanner on a document. It appears that it's possible for parsing to occurr, however, when the document parser is torn down. This change prevents that from happening and adds a a DCHECK to make sure it doesn't. Bug: 823306 ,819937 Change-Id: I541515e3f75d726b56f132c1c010367a4aa829d8 Reviewed-on: https://chromium-review.googlesource.com/973613 Commit-Queue: Matthew Cary <mattcary@chromium.org> Reviewed-by: Charlie Harrison <csharrison@chromium.org> Reviewed-by: Kouhei Ueno <kouhei@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#545491}(cherry picked from commit 17a0a62ce923bd9c4101c87f6fb279530b645ba5) Reviewed-on: https://chromium-review.googlesource.com/998653 Reviewed-by: John Abd-El-Malek <jam@chromium.org> Cr-Commit-Position: refs/branch-heads/3359@{#592} Cr-Branched-From: 66afc5e5d10127546cc4b98b9117aff588b5e66b-refs/heads/master@{#540276} [modify] https://crrev.com/e028ced7a69551928a5bd5e5a3e619308ae91937/third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp [modify] https://crrev.com/e028ced7a69551928a5bd5e5a3e619308ae91937/third_party/WebKit/Source/core/html/parser/HTMLDocumentParserTest.cpp
,
May 4 2018
|
|||
►
Sign in to add a comment |
|||
Comment 1 by jam@chromium.org
, Mar 19 2018