Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2262183002:
[NoStatePrefetch] Do not send responses to renderer in prefetch mode. (Closed)
|
Created:
4 years, 4 months ago by droger Modified:
4 years, 3 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, loading-reviews_chromium.org, pasko Base URL:
https://chromium.googlesource.com/chromium/src.git@prefetchProto Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[NoStatePrefetch] Do not send responses to renderer in prefetch mode.
Only the main frame is sent, to allow running the preload scanner on it,
other resources are not.
The renderer does not need this data, and so this CL avoids having them
in the renderer memory unnnecessarily.
This is done by creating the DetachableResourceHandler in detached mode
when such a blocked resource is found.
BUG=632361
Committed: https://crrev.com/308308a95a348f446a25de9c9bd789c5a94c92c4
Cr-Commit-Position: refs/heads/master@{#417926}
Patch Set 1 : comments #
Total comments: 5
Patch Set 2 : Renderer side implementation #Patch Set 3 : Fix uninitialized variable #
Total comments: 4
Patch Set 4 : move more logic to renderer #Patch Set 5 : Cancel requests #Patch Set 6 : unittest #Patch Set 7 : Cleanup #
Total comments: 5
Patch Set 8 : Capitalization #Patch Set 9 : remove TODO #
Total comments: 8
Patch Set 10 : fix typo #Patch Set 11 : remove WebDocument api #Messages
Total messages: 91 (39 generated)
Patchset #1 (id:1) has been deleted
The CQ bit was checked by droger@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...
droger@chromium.org changed reviewers: + clamy@chromium.org, mattcary@chromium.org
This is how I intend to prevent prefetches to go into blink memory cache. This CL is blocked on several other CLs, so no need to do a detailed review yet, but comments welcome!
rdsmith@chromium.org changed reviewers: + rdsmith@chromium.org
(Drive by, not intending to do a thorough review.) What were your thoughts about the tradeoff of this approach versus a new IPC message/adding a boolean to ResourceHostMsg_RequestResource?
Description was changed from ========== [NoStatePrefetch] Do not send responses to renderer in prefetch mode. Only the main frame is sent, to allow running the preload scanner on it, other resources are not. The renderer does not need this data, and so this CL avoids having them in the renderer memory unnnecessarily. This is done by creating the DetachableResourceHandler in detached mode when such a blocked resource is found. A new ResourceDispatcherHostDelegate method is added to allow Chrome to tell content that the resource should be blocked. ========== to ========== [NoStatePrefetch] Do not send responses to renderer in prefetch mode. Only the main frame is sent, to allow running the preload scanner on it, other resources are not. The renderer does not need this data, and so this CL avoids having them in the renderer memory unnnecessarily. This is done by creating the DetachableResourceHandler in detached mode when such a blocked resource is found. A new ResourceDispatcherHostDelegate method is added to allow Chrome to tell content that the resource should be blocked. ==========
rdsmith@chromium.org changed reviewers: - rdsmith@chromium.org
On 2016/08/22 13:59:15, Randy Smith - Not in Fridays wrote: > (Drive by, not intending to do a thorough review.) What were your thoughts > about the tradeoff of this approach versus a new IPC message/adding a boolean to > ResourceHostMsg_RequestResource? This was indeed considered (with clamy@ in particular), and I think both could work. If see an advantage to doing this renderer-side, please let me know. I think the two (weak) reasons we chose this way are: - doing things renderer side might cause additional security considerations in general - the implementation from the browser side seemed quite straightforward, whereas the implementation from the renderer side seemed less clear (although I admit we did not dig a lot).
mmenke@chromium.org changed reviewers: + mmenke@chromium.org
https://codereview.chromium.org/2262183002/diff/20001/content/browser/loader/... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2262183002/diff/20001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:1641: const bool detached = style: const is generally not used for stack variables that aren't compile-time constants. https://codereview.chromium.org/2262183002/diff/20001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:1642: delegate()->ShouldSinkResponse(request, request_data.resource_type); sink / detach: Should use consistent naming. https://codereview.chromium.org/2262183002/diff/20001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:1646: (!sync_result && IsDetachableResourceType(request_data.resource_type))) { Should we instead introduce a resource type for this instead? https://codereview.chromium.org/2262183002/diff/20001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:1646: (!sync_result && IsDetachableResourceType(request_data.resource_type))) { BUG: The sync_result check should cover the case detached is true - we don't want to let the embedder cause the entire renderer to hang, no matter how much it may want to.
https://codereview.chromium.org/2262183002/diff/20001/content/browser/loader/... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2262183002/diff/20001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:1646: (!sync_result && IsDetachableResourceType(request_data.resource_type))) { On 2016/08/22 14:08:22, mmenke wrote: > Should we instead introduce a resource type for this instead? Moreover, do we really want the embedder controlling behavior here? The new magic prerender type has logic in content/, so I assume content or blink knows that this is a not a real navigation, and the renderer doesn't really need the data. So it doesn't seem like the embedder should be controlling this.
On 2016/08/22 14:11:00, mmenke wrote: > https://codereview.chromium.org/2262183002/diff/20001/content/browser/loader/... > File content/browser/loader/resource_dispatcher_host_impl.cc (right): > > https://codereview.chromium.org/2262183002/diff/20001/content/browser/loader/... > content/browser/loader/resource_dispatcher_host_impl.cc:1646: (!sync_result && > IsDetachableResourceType(request_data.resource_type))) { > On 2016/08/22 14:08:22, mmenke wrote: > > Should we instead introduce a resource type for this instead? > > > Moreover, do we really want the embedder controlling behavior here? The new > magic prerender type has logic in content/, so I assume content or blink knows > that this is a not a real navigation, and the renderer doesn't really need the > data. So it doesn't seem like the embedder should be controlling this. You're right that piping this information from the renderer, we should be able to avoid the hop to the embedder here, which seems a clear benefit here. I'm going to look into how a renderer-side implementation would look like.
On 2016/08/22 14:06:46, droger wrote: > On 2016/08/22 13:59:15, Randy Smith - Not in Fridays wrote: > > (Drive by, not intending to do a thorough review.) What were your thoughts > > about the tradeoff of this approach versus a new IPC message/adding a boolean > to > > ResourceHostMsg_RequestResource? > > This was indeed considered (with clamy@ in particular), and I think both could > work. If see an advantage to doing this renderer-side, please let me know. No strong ones; it's just a tradeoff of where you put the interface complexity. Adding a bit in the request from the renderer meaning "fire and forget" seemed ot me more in keeping with what that interface is targeted for, whereas adding a new function on the RDH delegate seems like more of a bag on the side. But I agree about the security point, and at least some of what's going on is that I have more investment in keeping the RDH interface clean than the IPC interface (which should be discounted as an argument). Thanks for the summary. Carry on :-}. > > I think the two (weak) reasons we chose this way are: > - doing things renderer side might cause additional security considerations in > general > - the implementation from the browser side seemed quite straightforward, whereas > the implementation from the renderer side seemed less clear (although I admit we > did not dig a lot).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
Patchset #2 (id:40001) has been deleted
mmenke, rdsmith: changed to send the information from the renderer, as requested. This is probably cleaner indeed. PTAL. Note that it requires exposing the blink function Document::IsPrefetchOnly() to the public blink interface.
The CQ bit was checked by droger@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 ========== [NoStatePrefetch] Do not send responses to renderer in prefetch mode. Only the main frame is sent, to allow running the preload scanner on it, other resources are not. The renderer does not need this data, and so this CL avoids having them in the renderer memory unnnecessarily. This is done by creating the DetachableResourceHandler in detached mode when such a blocked resource is found. A new ResourceDispatcherHostDelegate method is added to allow Chrome to tell content that the resource should be blocked. ========== to ========== [NoStatePrefetch] Do not send responses to renderer in prefetch mode. Only the main frame is sent, to allow running the preload scanner on it, other resources are not. The renderer does not need this data, and so this CL avoids having them in the renderer memory unnnecessarily. This is done by creating the DetachableResourceHandler in detached mode when such a blocked resource is found. ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by droger@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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2262183002/diff/80001/content/browser/loader/... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2262183002/diff/80001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:1642: bool start_detached = request_data.is_prefetch_only && |is_prefetch_only| seems confusing, given that we also have RESOURCE_TYPE_PREFETCH, which also seems to indicate a request is only a prefetch. I guess we can't just make a new resource type, since we want to get the accept header right, which is set based on the resource type, but I think we at least need to switch to different terminology. |do_not_send_to_renderer|? |download_to_[network_]cache_only|? https://codereview.chromium.org/2262183002/diff/80001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:1643: request_data.resource_type != RESOURCE_TYPE_MAIN_FRAME; I'm not a fan of this MAIN_FRAME logic here. Perhaps it should be in the renderer, based on the main frame being one of the new prerender types? https://codereview.chromium.org/2262183002/diff/80001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:1651: start_detached ? nullptr : std::move(handler))); Could you update the DetachableResourceHandler docs to mention this works? https://codereview.chromium.org/2262183002/diff/80001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:1651: start_detached ? nullptr : std::move(handler))); We need to tell the renderer the request was aborted. Otherwise, it will keep on waiting for a response. We should have a unit test for this (And it should also make sure the entire resource is read).
Patchset #5 (id:120001) has been deleted
The CQ bit was checked by droger@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...
All done. This is not completely polished yet (will do another pass at comments, formats and naming) but the logic should be there.
On 2016/08/24 15:56:31, droger wrote: > All done. This is not completely polished yet (will do another pass at comments, > formats and naming) but the logic should be there. Let's not land this until https://crbug.com/640545 is resolved. Don't want to make any changes to the loader stack until we understand what's going on there.
Description was changed from ========== [NoStatePrefetch] Do not send responses to renderer in prefetch mode. Only the main frame is sent, to allow running the preload scanner on it, other resources are not. The renderer does not need this data, and so this CL avoids having them in the renderer memory unnnecessarily. This is done by creating the DetachableResourceHandler in detached mode when such a blocked resource is found. ========== to ========== [NoStatePrefetch] Do not send responses to renderer in prefetch mode. Only the main frame is sent, to allow running the preload scanner on it, other resources are not. The renderer does not need this data, and so this CL avoids having them in the renderer memory unnnecessarily. This is done by creating the DetachableResourceHandler in detached mode when such a blocked resource is found. BUG=632361 ==========
The CQ bit was checked by droger@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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
This is now ready for review.
LGTM, thanks for doing this! (The crasher I mentioned earlier has mysteriously vanished, so find to land this whenever, from my perspective). https://codereview.chromium.org/2262183002/diff/180001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/2262183002/diff/180001/content/browser/loader... content/browser/loader/resource_dispatcher_host_unittest.cc:1525: // there are two requests, so we should have gotten them classified as such. nit: Capitalize "There"
https://codereview.chromium.org/2262183002/diff/180001/content/child/web_url_... File content/child/web_url_request_util.cc (right): https://codereview.chromium.org/2262183002/diff/180001/content/child/web_url_... content/child/web_url_request_util.cc:224: load_flags |= net::LOAD_PREFETCH; Wait....This looks problematic. Could we remove this part of the CL? It looks to me like the LOAD_PREFETCH check in net/ fails to distinguish between CORS and non-CORS requests, which seems bad to me. If we do a prefetch in a way that sends cookies, and don't use a request, and then another page requests the resource in a way that does not, it can get the response with the cookies, even if there's a "Vary: Cookie" header, which seems problematic. I could be missing some protection against this, but I'm not seeing one.
On 2016/08/25 19:41:09, mmenke (busy) wrote: > https://codereview.chromium.org/2262183002/diff/180001/content/child/web_url_... > File content/child/web_url_request_util.cc (right): > > https://codereview.chromium.org/2262183002/diff/180001/content/child/web_url_... > content/child/web_url_request_util.cc:224: load_flags |= net::LOAD_PREFETCH; > Wait....This looks problematic. Could we remove this part of the CL? It looks > to me like the LOAD_PREFETCH check in net/ fails to distinguish between CORS and > non-CORS requests, which seems bad to me. > > If we do a prefetch in a way that sends cookies, and don't use a request, and > then another page requests the resource in a way that does not, it can get the > response with the cookies, even if there's a "Vary: Cookie" header, which seems > problematic. I could be missing some protection against this, but I'm not > seeing one. This is a good point, but I think that our argument here is that the current prerender already has this behavior, and thus we can have the same for no-state prefetch. I can move this part to a separate CL but in my understanding the LOAD_PREFETCH flag is essential for the feature.
On 2016/08/25 19:47:51, droger wrote: > On 2016/08/25 19:41:09, mmenke (busy) wrote: > > > https://codereview.chromium.org/2262183002/diff/180001/content/child/web_url_... > > File content/child/web_url_request_util.cc (right): > > > > > https://codereview.chromium.org/2262183002/diff/180001/content/child/web_url_... > > content/child/web_url_request_util.cc:224: load_flags |= net::LOAD_PREFETCH; > > Wait....This looks problematic. Could we remove this part of the CL? It > looks > > to me like the LOAD_PREFETCH check in net/ fails to distinguish between CORS > and > > non-CORS requests, which seems bad to me. > > > > If we do a prefetch in a way that sends cookies, and don't use a request, and > > then another page requests the resource in a way that does not, it can get the > > response with the cookies, even if there's a "Vary: Cookie" header, which > seems > > problematic. I could be missing some protection against this, but I'm not > > seeing one. > > This is a good point, but I think that our argument here is that the current > prerender already has this behavior, and thus we can have the same for no-state > prefetch. > I can move this part to a separate CL but in my understanding the LOAD_PREFETCH > flag is essential for the feature. I don't think prerender has that behavior (Yes, you can prerender any page you want, but this is specifically allowing us to service a request sent without cookies with a response sent with them, which prerender doesn't do. This seems a bug with the implementation of LOAD_PREFETCH)
On 2016/08/25 19:50:48, mmenke (busy) wrote: > On 2016/08/25 19:47:51, droger wrote: > > On 2016/08/25 19:41:09, mmenke (busy) wrote: > > > > > > https://codereview.chromium.org/2262183002/diff/180001/content/child/web_url_... > > > File content/child/web_url_request_util.cc (right): > > > > > > > > > https://codereview.chromium.org/2262183002/diff/180001/content/child/web_url_... > > > content/child/web_url_request_util.cc:224: load_flags |= net::LOAD_PREFETCH; > > > Wait....This looks problematic. Could we remove this part of the CL? It > > looks > > > to me like the LOAD_PREFETCH check in net/ fails to distinguish between CORS > > and > > > non-CORS requests, which seems bad to me. > > > > > > If we do a prefetch in a way that sends cookies, and don't use a request, > and > > > then another page requests the resource in a way that does not, it can get > the > > > response with the cookies, even if there's a "Vary: Cookie" header, which > > seems > > > problematic. I could be missing some protection against this, but I'm not > > > seeing one. > > > > This is a good point, but I think that our argument here is that the current > > prerender already has this behavior, and thus we can have the same for > no-state > > prefetch. > > I can move this part to a separate CL but in my understanding the > LOAD_PREFETCH > > flag is essential for the feature. > > I don't think prerender has that behavior (Yes, you can prerender any page you > want, but this is specifically allowing us to service a request sent without > cookies with a response sent with them, which prerender doesn't do. This seems > a bug with the implementation of LOAD_PREFETCH) I've filed a bug about this: https://crbug.com/641103. Mind holding off on landing this until we figure that out? I'm not an authority on CORS, so could well be missing something.
On 2016/08/25 20:01:01, mmenke (busy) wrote: > On 2016/08/25 19:50:48, mmenke (busy) wrote: > > On 2016/08/25 19:47:51, droger wrote: > > > On 2016/08/25 19:41:09, mmenke (busy) wrote: > > > > > > > > > > https://codereview.chromium.org/2262183002/diff/180001/content/child/web_url_... > > > > File content/child/web_url_request_util.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2262183002/diff/180001/content/child/web_url_... > > > > content/child/web_url_request_util.cc:224: load_flags |= > net::LOAD_PREFETCH; > > > > Wait....This looks problematic. Could we remove this part of the CL? It > > > looks > > > > to me like the LOAD_PREFETCH check in net/ fails to distinguish between > CORS > > > and > > > > non-CORS requests, which seems bad to me. > > > > > > > > If we do a prefetch in a way that sends cookies, and don't use a request, > > and > > > > then another page requests the resource in a way that does not, it can get > > the > > > > response with the cookies, even if there's a "Vary: Cookie" header, which > > > seems > > > > problematic. I could be missing some protection against this, but I'm not > > > > seeing one. > > > > > > This is a good point, but I think that our argument here is that the current > > > > prerender already has this behavior, and thus we can have the same for > > no-state > > > prefetch. > > > I can move this part to a separate CL but in my understanding the > > LOAD_PREFETCH > > > flag is essential for the feature. > > > > I don't think prerender has that behavior (Yes, you can prerender any page you > > want, but this is specifically allowing us to service a request sent without > > cookies with a response sent with them, which prerender doesn't do. This > seems > > a bug with the implementation of LOAD_PREFETCH) > > I've filed a bug about this: https://crbug.com/641103. Mind holding off on > landing this until we figure that out? I'm not an authority on CORS, so could > well be missing something. Yes,sure. Thanks for detecting and pointing this issue.. I admit I don't understand the problem completely yet. Let's resolve this before landing. If it turns out to be complex I'll split the load flags change to another CL.
On 2016/08/25 20:29:11, droger wrote: > On 2016/08/25 20:01:01, mmenke (busy) wrote: > > On 2016/08/25 19:50:48, mmenke (busy) wrote: > > > On 2016/08/25 19:47:51, droger wrote: > > > > On 2016/08/25 19:41:09, mmenke (busy) wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/2262183002/diff/180001/content/child/web_url_... > > > > > File content/child/web_url_request_util.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2262183002/diff/180001/content/child/web_url_... > > > > > content/child/web_url_request_util.cc:224: load_flags |= > > net::LOAD_PREFETCH; > > > > > Wait....This looks problematic. Could we remove this part of the CL? > It > > > > looks > > > > > to me like the LOAD_PREFETCH check in net/ fails to distinguish between > > CORS > > > > and > > > > > non-CORS requests, which seems bad to me. > > > > > > > > > > If we do a prefetch in a way that sends cookies, and don't use a > request, > > > and > > > > > then another page requests the resource in a way that does not, it can > get > > > the > > > > > response with the cookies, even if there's a "Vary: Cookie" header, > which > > > > seems > > > > > problematic. I could be missing some protection against this, but I'm > not > > > > > seeing one. > > > > > > > > This is a good point, but I think that our argument here is that the > current > > > > > > prerender already has this behavior, and thus we can have the same for > > > no-state > > > > prefetch. > > > > I can move this part to a separate CL but in my understanding the > > > LOAD_PREFETCH > > > > flag is essential for the feature. > > > > > > I don't think prerender has that behavior (Yes, you can prerender any page > you > > > want, but this is specifically allowing us to service a request sent without > > > cookies with a response sent with them, which prerender doesn't do. This > > seems > > > a bug with the implementation of LOAD_PREFETCH) > > > > I've filed a bug about this: https://crbug.com/641103. Mind holding off on > > landing this until we figure that out? I'm not an authority on CORS, so could > > well be missing something. > > Yes,sure. Thanks for detecting and pointing this issue.. I admit I don't > understand the problem completely yet. Let's resolve this before landing. If it > turns out to be complex I'll split the load flags change to another CL. Did a little more investigation, which I'm still concerned about an issue here, it affects less cases than I thought, so I'm good. content/browser/loader LGTM, and didn't see any other issues in the content/child code.
Thanks! The code in content/ looks very reasonable. One question below. https://codereview.chromium.org/2262183002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebDocument.cpp (right): https://codereview.chromium.org/2262183002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebDocument.cpp:142: return constUnwrap<Document>()->isPrefetchOnly(); How is isPrefetchOnly set on the document? Is this only true for no-state prefetch?
https://codereview.chromium.org/2262183002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebDocument.cpp (right): https://codereview.chromium.org/2262183002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebDocument.cpp:142: return constUnwrap<Document>()->isPrefetchOnly(); On 2016/08/25 22:34:59, clamy wrote: > How is isPrefetchOnly set on the document? Is this only true for no-state > prefetch? Yes, only true for no-state prefetch currently. It's set from the PrerenderContents on the browser via the render frame, see here: https://cs.chromium.org/chromium/src/chrome/browser/prerender/prerender_conte... Currently prerender_contents is only set to be prefetch via a command-line flag. There is discussion that as the nostate prefetch experiment evolves, we may set it conditionally based on the prerender origin. Regardless, it's the prerender manager/prerender contents that controls isPrefetchOnly.
The CQ bit was checked by droger@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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
clamy: ping. If you don't have time to do this review, let me know and I'll find someone else.
On 2016/08/25 22:34:59, clamy wrote: > Thanks! The code in content/ looks very reasonable. One question below. > > https://codereview.chromium.org/2262183002/diff/180001/third_party/WebKit/Sou... > File third_party/WebKit/Source/web/WebDocument.cpp (right): > > https://codereview.chromium.org/2262183002/diff/180001/third_party/WebKit/Sou... > third_party/WebKit/Source/web/WebDocument.cpp:142: return > constUnwrap<Document>()->isPrefetchOnly(); > How is isPrefetchOnly set on the document? Is this only true for no-state > prefetch? Yes, this is only for no-state prefetch. It is sent from the browser using IPC. On the renderer side, the IPC is caught by chrome/renderer/prerender/prerender_helper.cc
lgtm No useful comments from me, just questions... https://codereview.chromium.org/2262183002/diff/220001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2262183002/diff/220001/content/renderer/rende... content/renderer/render_frame_impl.cc:4076: WebURLRequestToResourceType(request) != RESOURCE_TYPE_MAIN_FRAME); Why do we not set this for main frame resources?
https://codereview.chromium.org/2262183002/diff/220001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2262183002/diff/220001/content/renderer/rende... content/renderer/render_frame_impl.cc:4076: WebURLRequestToResourceType(request) != RESOURCE_TYPE_MAIN_FRAME); On 2016/08/29 09:27:54, mattcary wrote: > Why do we not set this for main frame resources? This flags prevents the responses from going back to the renderer. When we do a NoStatePrefetch, we want the main resource to go to the renderer, to be able to run the preload scanner on it.
https://codereview.chromium.org/2262183002/diff/220001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2262183002/diff/220001/content/renderer/rende... content/renderer/render_frame_impl.cc:4076: WebURLRequestToResourceType(request) != RESOURCE_TYPE_MAIN_FRAME); On 2016/08/29 09:32:42, droger wrote: > On 2016/08/29 09:27:54, mattcary wrote: > > Why do we not set this for main frame resources? > > This flags prevents the responses from going back to the renderer. > > When we do a NoStatePrefetch, we want the main resource to go to the renderer, > to be able to run the preload scanner on it. Basically I use this to distinguish the main HTML document from the sub-resources like images and javascripts. I'm not using this to make a distinction main-frame / sub-frame. Let me know if you think this is a bad use of the flag.
On 2016/08/29 09:35:55, droger wrote: > https://codereview.chromium.org/2262183002/diff/220001/content/renderer/rende... > File content/renderer/render_frame_impl.cc (right): > > https://codereview.chromium.org/2262183002/diff/220001/content/renderer/rende... > content/renderer/render_frame_impl.cc:4076: WebURLRequestToResourceType(request) > != RESOURCE_TYPE_MAIN_FRAME); > On 2016/08/29 09:32:42, droger wrote: > > On 2016/08/29 09:27:54, mattcary wrote: > > > Why do we not set this for main frame resources? > > > > This flags prevents the responses from going back to the renderer. > > > > When we do a NoStatePrefetch, we want the main resource to go to the renderer, > > to be able to run the preload scanner on it. > > Basically I use this to distinguish the main HTML document from the > sub-resources like images and javascripts. I'm not using this to make a > distinction main-frame / sub-frame. Let me know if you think this is a bad use > of the flag. It's subtle---RESOURCE_TYPE_MAIN_FRAME doesn't exactly shout "source for preload scanner". But I can't comment on if there's a more straightforward way to do it.
On 2016/08/29 09:22:17, droger wrote: > clamy: ping. If you don't have time to do this review, let me know and I'll find > someone else. I'm still waiting for you to address mmenke's comment in content/child/web_url_request_util.cc in patch set 7. Other than that the code looks fine.
On 2016/08/29 17:39:33, clamy wrote: > On 2016/08/29 09:22:17, droger wrote: > > clamy: ping. If you don't have time to do this review, let me know and I'll > find > > someone else. > > I'm still waiting for you to address mmenke's comment in > content/child/web_url_request_util.cc in patch set 7. Other than that the code > looks fine. Sorry, I looked into that and determined it's not actually a problem. Made comments as standalone messages, rather than commenting inline in the CL.
https://codereview.chromium.org/2262183002/diff/180001/content/child/web_url_... File content/child/web_url_request_util.cc (right): https://codereview.chromium.org/2262183002/diff/180001/content/child/web_url_... content/child/web_url_request_util.cc:224: load_flags |= net::LOAD_PREFETCH; On 2016/08/25 19:41:09, mmenke wrote: > Wait....This looks problematic. Could we remove this part of the CL? It looks > to me like the LOAD_PREFETCH check in net/ fails to distinguish between CORS and > non-CORS requests, which seems bad to me. > > If we do a prefetch in a way that sends cookies, and don't use a request, and > then another page requests the resource in a way that does not, it can get the > response with the cookies, even if there's a "Vary: Cookie" header, which seems > problematic. I could be missing some protection against this, but I'm not > seeing one. As discussed offline, this has been resolved (see bug https://crbug.com/641103).
Thanks! The code in content/ looks mostly good, except for an issue with the ResourceType. I've added a reviewer for the IPC & blink changes. @dcheng: PTAL at the changes in resource_messages.h & blink https://codereview.chromium.org/2262183002/diff/220001/content/common/resourc... File content/common/resource_request.h (right): https://codereview.chromium.org/2262183002/diff/220001/content/common/resourc... content/common/resource_request.h:177: // The response should be downloaded and stored in the network cache, but the nit: remove unneeded the https://codereview.chromium.org/2262183002/diff/220001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2262183002/diff/220001/content/renderer/rende... content/renderer/render_frame_impl.cc:4076: WebURLRequestToResourceType(request) != RESOURCE_TYPE_MAIN_FRAME); Do we want iframes to also be downloaded to cache? If yes, this works, otherwise you should use IsResourceTypeFrame to also exclude iframes (which have RESOURCE_TYPE_SUB_FRAME). Not that I wonder how this would work with more exotic types, such as RESOURCE_TYPE_PING or RESOURCE_TYPE_SERVICE_WORKER. Do we want to try to download them to cache, or just stop te request?
clamy@chromium.org changed reviewers: + dcheng@chromium.org
@dcheng: PTAL at the changes in resource_messages.h & Blink.
I'm confused. What is prefetch? How does it work? Is it related to prerendering?
This is for no-state prefetch (design doc here: https://docs.google.com/document/d/16VCYGGWau483IMSxODpg5faZny1FJ6vNK2v-BuM5E...). Roughly, the goal is to eventually replace prerender with an extended version of prefetch that downloads all subresources found by the html parser to cache (and doesn't do a full rendering of the page).
Thanks for the review! https://codereview.chromium.org/2262183002/diff/220001/content/common/resourc... File content/common/resource_request.h (right): https://codereview.chromium.org/2262183002/diff/220001/content/common/resourc... content/common/resource_request.h:177: // The response should be downloaded and stored in the network cache, but the On 2016/08/30 18:16:13, clamy wrote: > nit: remove unneeded the Done. https://codereview.chromium.org/2262183002/diff/220001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2262183002/diff/220001/content/renderer/rende... content/renderer/render_frame_impl.cc:4076: WebURLRequestToResourceType(request) != RESOURCE_TYPE_MAIN_FRAME); On 2016/08/30 18:16:13, clamy wrote: > Do we want iframes to also be downloaded to cache? If yes, this works, otherwise > you should use IsResourceTypeFrame to also exclude iframes (which have > RESOURCE_TYPE_SUB_FRAME). > > Not that I wonder how this would work with more exotic types, such as > RESOURCE_TYPE_PING or RESOURCE_TYPE_SERVICE_WORKER. Do we want to try to > download them to cache, or just stop te request? For now we want to only let the main request go through, and everything else goes to cache. We may add support for sub frames later. About the more exotic requests, it's not very clear what kind of side effect these can have. The current behavior with prerendering is not cancelling them, so I think we can treat them like the other resources for now and see what happens. Does that sounds good?
Thanks! Lgtm with one comment. https://codereview.chromium.org/2262183002/diff/220001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2262183002/diff/220001/content/renderer/rende... content/renderer/render_frame_impl.cc:4076: WebURLRequestToResourceType(request) != RESOURCE_TYPE_MAIN_FRAME); On 2016/09/01 09:37:32, droger wrote: > On 2016/08/30 18:16:13, clamy wrote: > > Do we want iframes to also be downloaded to cache? If yes, this works, > otherwise > > you should use IsResourceTypeFrame to also exclude iframes (which have > > RESOURCE_TYPE_SUB_FRAME). > > > > Not that I wonder how this would work with more exotic types, such as > > RESOURCE_TYPE_PING or RESOURCE_TYPE_SERVICE_WORKER. Do we want to try to > > download them to cache, or just stop te request? > > For now we want to only let the main request go through, and everything else > goes to cache. We may add support for sub frames later. > > About the more exotic requests, it's not very clear what kind of side effect > these can have. The current behavior with prerendering is not cancelling them, > so I think we can treat them like the other resources for now and see what > happens. Does that sounds good? Could you add a comment explaining this? This way people reading the code will know that it is intentional that subframes follow this as well. Maybe add a TODO as well to check if all types of resources should go to cache.
OK, so just so I understand: For prefetching, we still create a Page / LocalFrame / Document? And it has the corresponding content side RenderView/RenderFrame? Does content know when it's creating a prefetch only document? I'm just kind of curious why we have to plumb it all the way back out (and why it's tracked at the Document level; it seems like the entire Page must be for prefetch, really, even though we only do prefetch on the main frame atm)
On 2016/09/02 08:49:48, dcheng wrote: > OK, so just so I understand: > > For prefetching, we still create a Page / LocalFrame / Document? And it has the > corresponding content side RenderView/RenderFrame? Yes. > > Does content know when it's creating a prefetch only document? I'm just kind of > curious why we have to plumb it all the way back out (and why it's tracked at > the Document level; it seems like the entire Page must be for prefetch, really, > even though we only do prefetch on the main frame atm) I'm not sure exactly how the document creation works and what is the exact sequence of events. It works exactly like prerendering: right after creating the renderer, content sends a IPC to tell the renderer that the page is in prefetch mode. We use the existing plumbing for prerendering to do this. Content knows about this: chrome/renderer/prerender/prerender_helper.h has a prerender mode now that is used for this. But blink also needs to know about it: the core of the feature is implemented in HTMLDocumentParser. When in prefetch mode, the parser just runs the preload scanner and stops ; it never actually parses the page. The main CL for the feature in blink was https://codereview.chromium.org/2172613002/
On 2016/09/02 09:03:48, droger wrote: > On 2016/09/02 08:49:48, dcheng wrote: > > OK, so just so I understand: > > > > For prefetching, we still create a Page / LocalFrame / Document? And it has > the > > corresponding content side RenderView/RenderFrame? > > Yes. > > > > > Does content know when it's creating a prefetch only document? I'm just kind > of > > curious why we have to plumb it all the way back out (and why it's tracked at > > the Document level; it seems like the entire Page must be for prefetch, > really, > > even though we only do prefetch on the main frame atm) > > I'm not sure exactly how the document creation works and what is the exact > sequence of events. > It works exactly like prerendering: right after creating the renderer, content > sends a IPC to tell the renderer that the page is in prefetch mode. > We use the existing plumbing for prerendering to do this. > > Content knows about this: chrome/renderer/prerender/prerender_helper.h has a > prerender mode now that is used for this. > > But blink also needs to know about it: the core of the feature is implemented in > HTMLDocumentParser. When in prefetch mode, the parser just runs the preload > scanner and stops ; it never actually parses the page. > > The main CL for the feature in blink was > https://codereview.chromium.org/2172613002/ Right, but it looks like this CL is not exposing it from content to BLink: it's exposing from BLink back into content.
On 2016/09/02 09:06:57, dcheng wrote: > On 2016/09/02 09:03:48, droger wrote: > > On 2016/09/02 08:49:48, dcheng wrote: > > > OK, so just so I understand: > > > > > > For prefetching, we still create a Page / LocalFrame / Document? And it has > > the > > > corresponding content side RenderView/RenderFrame? > > > > Yes. > > > > > > > > Does content know when it's creating a prefetch only document? I'm just kind > > of > > > curious why we have to plumb it all the way back out (and why it's tracked > at > > > the Document level; it seems like the entire Page must be for prefetch, > > really, > > > even though we only do prefetch on the main frame atm) > > > > I'm not sure exactly how the document creation works and what is the exact > > sequence of events. > > It works exactly like prerendering: right after creating the renderer, content > > sends a IPC to tell the renderer that the page is in prefetch mode. > > We use the existing plumbing for prerendering to do this. > > > > Content knows about this: chrome/renderer/prerender/prerender_helper.h has a > > prerender mode now that is used for this. > > > > But blink also needs to know about it: the core of the feature is implemented > in > > HTMLDocumentParser. When in prefetch mode, the parser just runs the preload > > scanner and stops ; it never actually parses the page. > > > > The main CL for the feature in blink was > > https://codereview.chromium.org/2172613002/ > > Right, but it looks like this CL is not exposing it from content to BLink: it's > exposing from BLink back into content. content -> blink goes through the prerenderer client (in the other CL) blink->content goes through Document / WebDocument (in this CL) Is your point that we should try to avoid exposing it in WebDocument?
On 2016/09/02 09:23:23, droger wrote: > On 2016/09/02 09:06:57, dcheng wrote: > > On 2016/09/02 09:03:48, droger wrote: > > > On 2016/09/02 08:49:48, dcheng wrote: > > > > OK, so just so I understand: > > > > > > > > For prefetching, we still create a Page / LocalFrame / Document? And it > has > > > the > > > > corresponding content side RenderView/RenderFrame? > > > > > > Yes. > > > > > > > > > > > Does content know when it's creating a prefetch only document? I'm just > kind > > > of > > > > curious why we have to plumb it all the way back out (and why it's tracked > > at > > > > the Document level; it seems like the entire Page must be for prefetch, > > > really, > > > > even though we only do prefetch on the main frame atm) > > > > > > I'm not sure exactly how the document creation works and what is the exact > > > sequence of events. > > > It works exactly like prerendering: right after creating the renderer, > content > > > sends a IPC to tell the renderer that the page is in prefetch mode. > > > We use the existing plumbing for prerendering to do this. > > > > > > Content knows about this: chrome/renderer/prerender/prerender_helper.h has a > > > prerender mode now that is used for this. > > > > > > But blink also needs to know about it: the core of the feature is > implemented > > in > > > HTMLDocumentParser. When in prefetch mode, the parser just runs the preload > > > scanner and stops ; it never actually parses the page. > > > > > > The main CL for the feature in blink was > > > https://codereview.chromium.org/2172613002/ > > > > Right, but it looks like this CL is not exposing it from content to BLink: > it's > > exposing from BLink back into content. > > content -> blink goes through the prerenderer client (in the other CL) > blink->content goes through Document / WebDocument (in this CL) > > Is your point that we should try to avoid exposing it in WebDocument? Yes. Basically, I'm wondering why we have to tell content if content already knows. If we don't need to expose it through WebDocument, that might be nicer.
On 2016/09/02 09:49:12, dcheng wrote: > Yes. Basically, I'm wondering why we have to tell content if content already > knows. If we don't need to expose it through WebDocument, that might be nicer. So I looked more into this. This is not straightforward to do, since the information whether the document is in prefetch mode is actually in chrome, not in content. Blink already has a way to query this from chrome using the blink::PrerendererClient, but content has no way to do this currently. I made a new version that adds a new way to get the information through The ChromeContentRendererClient instead, since this is what nearby code uses. Let me know if you like it better.
Patchset #11 (id:260001) has been deleted
On 2016/09/02 14:01:28, droger wrote: > On 2016/09/02 09:49:12, dcheng wrote: > > Yes. Basically, I'm wondering why we have to tell content if content already > > knows. If we don't need to expose it through WebDocument, that might be nicer. > > So I looked more into this. > > This is not straightforward to do, since the information whether the document is > in prefetch mode is actually in chrome, not in content. > Blink already has a way to query this from chrome using the > blink::PrerendererClient, but content has no way to do this currently. > > I made a new version that adds a new way to get the information through The > ChromeContentRendererClient instead, since this is what nearby code uses. Let me > know if you like it better. dcheng: Ping. Please take a look at patch 11 and tell me if you think it's better (changing content client API instead of blink API). I can revert to patch 10 otherwise.
dcheng: ping
On 2016/09/08 07:51:42, droger wrote: > dcheng: ping To me, it feels weird that the layer above //content (//chrome) and below //content (Blink) all know about this prefetch thing, but content doesn't. Will prefetch make sense as a //content thing in the future?
On 2016/09/09 04:42:21, dcheng wrote: > On 2016/09/08 07:51:42, droger wrote: > > dcheng: ping > > To me, it feels weird that the layer above //content (//chrome) and below > //content (Blink) all know about this prefetch thing, but content doesn't. Will > prefetch make sense as a //content thing in the future? Possibly. Currently we're trying to implement prefetching using the existing prerendering code (which is at chrome level) without changing too much the architecture until we're sure that we will really ship it. At this point, we will remove the existing prerendering code and keep only prefetching, and also simplify the architecture at the maximum. The simplification of the architecture is actually our main goal, since the main drawback of prerendering is its complexity, and prefetching is supposed to address this by being simpler. When we'll be at that point we can consider moving more code into content. If I understand correctly, you would prefer that the "prefetch" bit would be stored in the content layer instead of the chrome layer as it currently is. (Note that is is NOT currently stored in the blink layer, blink uses the WebPrerendererClient to get it from chrome). One way to do this may be to move the PrerenderHelper class into content, maybe. However, I would like to avoid doing this for now, as many of this code may go away when we delete the prerendering code path. Would it be OK if I land patch 9 (the one going through webdocument) or 11 (the one going through content client) for now?
On 2016/09/09 09:07:26, droger wrote: > On 2016/09/09 04:42:21, dcheng wrote: > > On 2016/09/08 07:51:42, droger wrote: > > > dcheng: ping > > > > To me, it feels weird that the layer above //content (//chrome) and below > > //content (Blink) all know about this prefetch thing, but content doesn't. > Will > > prefetch make sense as a //content thing in the future? > > Possibly. > > Currently we're trying to implement prefetching using the existing prerendering > code (which is at chrome level) without changing too much the architecture until > we're sure that we will really ship it. > At this point, we will remove the existing prerendering code and keep only > prefetching, and also simplify the architecture at the maximum. > > The simplification of the architecture is actually our main goal, since the main > drawback of prerendering is its complexity, and prefetching is supposed to > address this by being simpler. > When we'll be at that point we can consider moving more code into content. > > If I understand correctly, you would prefer that the "prefetch" bit would be > stored in the content layer instead of the chrome layer as it currently is. > (Note that is is NOT currently stored in the blink layer, blink uses the > WebPrerendererClient to get it from chrome). One way to do this may be to move > the PrerenderHelper class into content, maybe. > However, I would like to avoid doing this for now, as many of this code may go > away when we delete the prerendering code path. > > Would it be OK if I land patch 9 (the one going through webdocument) or 11 (the > one going through content client) for now? From a plumbing perspective, PS11 LGTM if the content owners are OK with it (it is a bit more code). The reason is it looks a bit unusual to plumb a bit all the way down into Blink via WebPrerenderClient, and then for content to query that bit back out of Blink: it just feels like our bookkeeping is amiss somewhere =)
droger@chromium.org changed reviewers: + jochen@chromium.org
Thanks for the reviews. +jochen as OWNER for content_renderer_client.
lgtm
The CQ bit was checked by droger@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org, mattcary@chromium.org, clamy@chromium.org Link to the patchset: https://codereview.chromium.org/2262183002/#ps280001 (title: "remove WebDocument api")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [NoStatePrefetch] Do not send responses to renderer in prefetch mode. Only the main frame is sent, to allow running the preload scanner on it, other resources are not. The renderer does not need this data, and so this CL avoids having them in the renderer memory unnnecessarily. This is done by creating the DetachableResourceHandler in detached mode when such a blocked resource is found. BUG=632361 ========== to ========== [NoStatePrefetch] Do not send responses to renderer in prefetch mode. Only the main frame is sent, to allow running the preload scanner on it, other resources are not. The renderer does not need this data, and so this CL avoids having them in the renderer memory unnnecessarily. This is done by creating the DetachableResourceHandler in detached mode when such a blocked resource is found. BUG=632361 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:280001)
Message was sent while issue was closed.
Description was changed from ========== [NoStatePrefetch] Do not send responses to renderer in prefetch mode. Only the main frame is sent, to allow running the preload scanner on it, other resources are not. The renderer does not need this data, and so this CL avoids having them in the renderer memory unnnecessarily. This is done by creating the DetachableResourceHandler in detached mode when such a blocked resource is found. BUG=632361 ========== to ========== [NoStatePrefetch] Do not send responses to renderer in prefetch mode. Only the main frame is sent, to allow running the preload scanner on it, other resources are not. The renderer does not need this data, and so this CL avoids having them in the renderer memory unnnecessarily. This is done by creating the DetachableResourceHandler in detached mode when such a blocked resource is found. BUG=632361 Committed: https://crrev.com/308308a95a348f446a25de9c9bd789c5a94c92c4 Cr-Commit-Position: refs/heads/master@{#417926} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/308308a95a348f446a25de9c9bd789c5a94c92c4 Cr-Commit-Position: refs/heads/master@{#417926} |
