Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2005273002:
Move MimeTypeResourceHandler before ThrottlingResourceHandler (Closed)
|
Created:
4 years, 7 months ago by clamy Modified:
4 years, 3 months ago Reviewers:
asanka, jochen (gone - plz use gerrit), Randy Smith (Not in Mondays), nasko, mattm, Nathan Parker, mmenke CC:
asanka, carlosk, chromium-reviews, darin-cc_chromium.org, jam, loading-reviews_chromium.org, Mike West, Nathan Parker, yhirano Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMove MimeTypeResourceHandler before ThrottlingResourceHandler
This CL moves the MimeTypeResourceHandler before the ThrottlingResourceHandler.
This allows NavigationThrottles (called by the NavigationResourceThrottle) to
execute with full knowledge of the MIME type of a navigation. This is needed to
reland https://codereview.chromium.org/1617043002/, which needs knowledge of
downloads in the NavigationThrottles.
BUG=555418
Committed: https://crrev.com/0e3422671a1173d23d6a1e4dfb35c0499b401fb6
Cr-Commit-Position: refs/heads/master@{#416621}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #
Total comments: 3
Patch Set 5 : Back to one class #
Total comments: 3
Patch Set 6 : Rebase #Patch Set 7 : Split into 2 ResourceHandlers #Patch Set 8 : Removed compilation error in content_unittests #Patch Set 9 : Fixed gypi error #
Total comments: 38
Patch Set 10 : Rebase #Patch Set 11 : Addressed comments + new unit tests #Patch Set 12 : Rebase #Patch Set 13 : Fixed issue in ResourceDispatcherHostTests #
Total comments: 9
Patch Set 14 : Rebase #Patch Set 15 : Moved interception checks #Patch Set 16 : Fixed tests #Patch Set 17 : Rebase #
Total comments: 29
Patch Set 18 : Addressed comments #
Total comments: 23
Patch Set 19 : Rebase #Patch Set 20 : Addressed comments #
Total comments: 46
Patch Set 21 : Addressed comments + safe browsing issue #Patch Set 22 : Added missing file #
Total comments: 53
Patch Set 23 : Rebase #Patch Set 24 : Addressed comments #Patch Set 25 : Fixed compilation error on Mac #
Total comments: 78
Patch Set 26 : Addressed comments #
Total comments: 10
Patch Set 27 : Rebase #Patch Set 28 : Addressed comments #Messages
Total messages: 114 (47 generated)
Description was changed from ========== WIP Move MimeTypeResourceHandler before ThrottlingResourceHandler BUG= ========== to ========== Move MimeTypeResourceHandler before ThrottlingResourceHandler This CL moves the MimeTypeResourceHandler before the ThrottlingResourceHandler. This allows NavigationThrottles (called by the NavigationResourceThrottle) to execute with full knowledge of the MIME type of a navigation. This is needed to reland https://codereview.chromium.org/1617043002/, which needs knowledge of downloads in the NavigationThrottles. BUG=555418 ==========
Description was changed from ========== Move MimeTypeResourceHandler before ThrottlingResourceHandler This CL moves the MimeTypeResourceHandler before the ThrottlingResourceHandler. This allows NavigationThrottles (called by the NavigationResourceThrottle) to execute with full knowledge of the MIME type of a navigation. This is needed to reland https://codereview.chromium.org/1617043002/, which needs knowledge of downloads in the NavigationThrottles. BUG=555418 ========== to ========== Move MimeTypeResourceHandler before ThrottlingResourceHandler This CL moves the MimeTypeResourceHandler before the ThrottlingResourceHandler. This allows NavigationThrottles (called by the NavigationResourceThrottle) to execute with full knowledge of the MIME type of a navigation. This is needed to reland https://codereview.chromium.org/1617043002/, which needs knowledge of downloads in the NavigationThrottles. BUG=555418 ==========
clamy@chromium.org changed reviewers: + mmenke@chromium.org, nasko@chromium.org
@nasko, mmenke: PTAL This is a preliminary patch in order to reland https://codereview.chromium.org/1617043002/ and to move mixed content checks to the browser. For PlzNavigate, we want to move mixed content and XFO checks to the browser inside a NavigationThrottle. However, they shouldn't execute on downloads, which means we must know the mime type of the response before calling them. Therefore we discussed with nasko@ the possibility of moving the MimeTypeResourceHandler before the ThrottlingResourceHandler, which is what this patch is doing. Note: there are stille some unit tests failing, that require a rewrtie, which I will do in a later patch set. Also note that I split in the MimeTypeResourceHandler in two classes, a base one BufferedResourceHandler that deals with the buffering mechanisms, and MimeTypeResourceHandler which inherits from it and decides when to start/stop buffering. This is mostly to ensure that I would get the buffering right, but they can be merged back into one class. The actual logic for deciding when to buffer or to switch handler has not changed.However, the buffering logic has changed since it needs to handle more cases of request deferral from downstream handlers.
https://codereview.chromium.org/2005273002/diff/60001/content/browser/loader/... File content/browser/loader/buffered_resource_handler.h (right): https://codereview.chromium.org/2005273002/diff/60001/content/browser/loader/... content/browser/loader/buffered_resource_handler.h:31: class CONTENT_EXPORT BufferedResourceHandler : public LayeredResourceHandler, What's the motivation for separating this out, and should this be done in the same CL as changing ResourceHandler order? The CL description mentions nothing about this part of the change.
https://codereview.chromium.org/2005273002/diff/60001/content/browser/loader/... File content/browser/loader/buffered_resource_handler.h (right): https://codereview.chromium.org/2005273002/diff/60001/content/browser/loader/... content/browser/loader/buffered_resource_handler.h:31: class CONTENT_EXPORT BufferedResourceHandler : public LayeredResourceHandler, On 2016/05/26 17:30:22, mmenke wrote: > What's the motivation for separating this out, and should this be done in the > same CL as changing ResourceHandler order? The CL description mentions nothing > about this part of the change. My reason for asking is that I really don't want to encourage another other class to do this, and this is what 90+% of the CL is doing, so want to get that ironed out before reviewing the code.
I'd defer on this review to mmenke@, as he is more experienced in this area than myself.
https://codereview.chromium.org/2005273002/diff/60001/content/browser/loader/... File content/browser/loader/buffered_resource_handler.h (right): https://codereview.chromium.org/2005273002/diff/60001/content/browser/loader/... content/browser/loader/buffered_resource_handler.h:31: class CONTENT_EXPORT BufferedResourceHandler : public LayeredResourceHandler, On 2016/05/26 20:01:41, mmenke wrote: > On 2016/05/26 17:30:22, mmenke wrote: > > What's the motivation for separating this out, and should this be done in the > > same CL as changing ResourceHandler order? The CL description mentions > nothing > > about this part of the change. > > My reason for asking is that I really don't want to encourage another other > class to do this, and this is what 90+% of the CL is doing, so want to get that > ironed out before reviewing the code. As I explained in the review message, I split into 2 classes mostly so that I could understand the buffering mechanism and modify it properly. I'm remerging everything into one class in the next patchset.
PTAL at the latest patchset, where we're back to one class.
Just a couple of notes. I'm not sure I understand the changes in mime_type_resource_handler.cc well enough to be an useful reviewer :(. https://codereview.chromium.org/2005273002/diff/80001/content/browser/loader/... File content/browser/loader/layered_resource_handler.cc (right): https://codereview.chromium.org/2005273002/diff/80001/content/browser/loader/... content/browser/loader/layered_resource_handler.cc:99: // Simulate a read of payload_for_old_handler and the end of the response on This comment applies more to the else part of this statement. https://codereview.chromium.org/2005273002/diff/80001/content/browser/loader/... content/browser/loader/layered_resource_handler.cc:112: CHECK_GE(size, static_cast<int>(payload_for_old_handler.length())); It is better to use checked_cast<> from base/numerics/safe_conversions.h to avoid over/underflow.
On 2016/06/01 17:18:25, nasko (slow) wrote: > Just a couple of notes. I'm not sure I understand the changes in > mime_type_resource_handler.cc well enough to be an useful reviewer :(. > > https://codereview.chromium.org/2005273002/diff/80001/content/browser/loader/... > File content/browser/loader/layered_resource_handler.cc (right): > > https://codereview.chromium.org/2005273002/diff/80001/content/browser/loader/... > content/browser/loader/layered_resource_handler.cc:99: // Simulate a read of > payload_for_old_handler and the end of the response on > This comment applies more to the else part of this statement. > > https://codereview.chromium.org/2005273002/diff/80001/content/browser/loader/... > content/browser/loader/layered_resource_handler.cc:112: CHECK_GE(size, > static_cast<int>(payload_for_old_handler.length())); > It is better to use checked_cast<> from base/numerics/safe_conversions.h to > avoid over/underflow. This is going to require reading a lot of code, to make sure neither the reordering of the handler, nor "InstallNewLeafHandler" break anything. I may not have comments until Friday.
mmenke@chromium.org changed reviewers: + asanka@chromium.org, rdsmith@chromium.org
[+rdsmith]: FYI. This CL seems to have a pretty big footprint in terms of the ResourceLoader stack. [+asanka]: Also FYI, as this may have some impact on downloads.
Hrm...I wonder if this would be simpler if we split it in two: One to do mime sniffing, before the throttles, and one to do mime type dispatch, after the throttles. Downside is that the throttles get the response later than they do now, but this has that same issue, anyways. Upside is that it's slightly simpler. As-is, this CL makes my head spin even more than MimeTypeResourceHandler does already. I assume we can't just add a second ThrottlingResourceHandler after the MimeTypeResourceHandler for the AncestorThrottle - when responses are treated as "streams", they can be sent to plugins, and the throttle needs to be able to block those. https://codereview.chromium.org/2005273002/diff/80001/content/browser/loader/... File content/browser/loader/mime_type_resource_handler.cc (right): https://codereview.chromium.org/2005273002/diff/80001/content/browser/loader/... content/browser/loader/mime_type_resource_handler.cc:232: payload_for_old_handler); Why is this no longer always done synchronously?
On 2016/06/02 19:56:19, mmenke wrote: > Hrm...I wonder if this would be simpler if we split it in two: One to do mime > sniffing, before the throttles, and one to do mime type dispatch, after the > throttles. Downside is that the throttles get the response later than they do > now, but this has that same issue, anyways. Upside is that it's slightly > simpler. As-is, this CL makes my head spin even more than > MimeTypeResourceHandler does already. > > I assume we can't just add a second ThrottlingResourceHandler after the > MimeTypeResourceHandler for the AncestorThrottle - when responses are treated as > "streams", they can be sent to plugins, and the throttle needs to be able to > block those. "Can't just" meaning can't do it without the new insertion code this CL adds.
On 2016/06/02 19:56:57, mmenke wrote: > On 2016/06/02 19:56:19, mmenke wrote: > > Hrm...I wonder if this would be simpler if we split it in two: One to do mime > > sniffing, before the throttles, and one to do mime type dispatch, after the > > throttles. Downside is that the throttles get the response later than they do > > now, but this has that same issue, anyways. Upside is that it's slightly > > simpler. As-is, this CL makes my head spin even more than > > MimeTypeResourceHandler does already. > > > > I assume we can't just add a second ThrottlingResourceHandler after the > > MimeTypeResourceHandler for the AncestorThrottle - when responses are treated > as > > "streams", they can be sent to plugins, and the throttle needs to be able to > > block those. > > "Can't just" meaning can't do it without the new insertion code this CL adds. I'm not set on the approach I suggested, just trying to figure out how to make this code easier to deal with.
On 2016/06/02 19:58:42, mmenke wrote: > On 2016/06/02 19:56:57, mmenke wrote: > > On 2016/06/02 19:56:19, mmenke wrote: > > > Hrm...I wonder if this would be simpler if we split it in two: One to do > mime > > > sniffing, before the throttles, and one to do mime type dispatch, after the > > > throttles. Downside is that the throttles get the response later than they > do > > > now, but this has that same issue, anyways. Upside is that it's slightly > > > simpler. As-is, this CL makes my head spin even more than > > > MimeTypeResourceHandler does already. > > > > > > I assume we can't just add a second ThrottlingResourceHandler after the > > > MimeTypeResourceHandler for the AncestorThrottle - when responses are > treated > > as > > > "streams", they can be sent to plugins, and the throttle needs to be able to > > > block those. > > > > "Can't just" meaning can't do it without the new insertion code this CL adds. > > I'm not set on the approach I suggested, just trying to figure out how to make > this code easier to deal with. And yea, I realize that this is similar to the split you did earlier, at the BufferedResourceHandler layer, except it's BufferedResourceHAndler + Mime Sniffing in one - sorry for the run around here. Should have spent more time on this initially, but I've been slammed with reviews the last month.
@mmenke: PTAL. I split the handler in 2 as you suggested. If you're happy with the general architecture, I'll work on updating the unit tests (I had to delete some of them since they were testing functionality no longer in MimeTypeResourceHandler). https://codereview.chromium.org/2005273002/diff/160001/content/browser/loader... File content/browser/loader/DEPS (right): https://codereview.chromium.org/2005273002/diff/160001/content/browser/loader... content/browser/loader/DEPS:142: "+content/browser/loader/replacing_resource_handler.h", I imagine it would be there, but not sure about it.
On 2016/06/08 14:34:27, clamy wrote: > @mmenke: PTAL. I split the handler in 2 as you suggested. If you're happy with > the general architecture, I'll work on updating the unit tests (I had to delete > some of them since they were testing functionality no longer in > MimeTypeResourceHandler). Sorry I didn't get to this yesterday. This looks much better me. Going to try and do a full pass this afternoon.
mmenke@chromium.org changed reviewers: + mattm@chromium.org
https://codereview.chromium.org/2005273002/diff/160001/content/browser/loader... File content/browser/loader/mime_type_resource_handler.cc (right): https://codereview.chromium.org/2005273002/diff/160001/content/browser/loader... content/browser/loader/mime_type_resource_handler.cc:126: bool* defer) { DCHECK_EQ(STATE_STARTING, state_);? https://codereview.chromium.org/2005273002/diff/160001/content/browser/loader... content/browser/loader/mime_type_resource_handler.cc:236: DCHECK(state_ == STATE_STREAMING); DCHECK_EQ(STATE_STREAMING, state_); https://codereview.chromium.org/2005273002/diff/160001/content/browser/loader... content/browser/loader/mime_type_resource_handler.cc:272: DCHECK(state_ == STATE_BUFFERING); DCHECK_EQ https://codereview.chromium.org/2005273002/diff/160001/content/browser/loader... content/browser/loader/mime_type_resource_handler.cc:278: DCHECK(state_ == STATE_REPLAYING_RESPONSE_RECEIVED); DCHECK_EQ https://codereview.chromium.org/2005273002/diff/160001/content/browser/loader... File content/browser/loader/mime_type_resource_handler.h (right): https://codereview.chromium.org/2005273002/diff/160001/content/browser/loader... content/browser/loader/mime_type_resource_handler.h:35: class CONTENT_EXPORT MimeTypeResourceHandler : public LayeredResourceHandler, Think this can now be renamed to MimeSniffingResourceHandler, since that's all it does now. https://codereview.chromium.org/2005273002/diff/160001/content/browser/loader... content/browser/loader/mime_type_resource_handler.h:45: // acting as a blind pass-through ResourceHandler. It's not a blind pass-through ResourceHandler. It switches to STATE_BUFFERING in OnResponseStarted. https://codereview.chromium.org/2005273002/diff/160001/content/browser/loader... File content/browser/loader/replacing_resource_handler.cc (right): https://codereview.chromium.org/2005273002/diff/160001/content/browser/loader... content/browser/loader/replacing_resource_handler.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. Weird that git isn't picking up that this file was forked from mime_type_resource_handler. Could you try turning down the similarity threshold? Makes review easier. https://codereview.chromium.org/2005273002/diff/160001/content/browser/loader... content/browser/loader/replacing_resource_handler.cc:64: return true; BUG: This should be passed through to the next handler, and state should be set to State::CHOICE_MADE. Should have a test for that case. https://codereview.chromium.org/2005273002/diff/160001/content/browser/loader... content/browser/loader/replacing_resource_handler.cc:94: DCHECK(state_ == State::WAITING_FOR_BUFFER_COPY); DCHECK_EQ https://codereview.chromium.org/2005273002/diff/160001/content/browser/loader... File content/browser/loader/replacing_resource_handler.h (right): https://codereview.chromium.org/2005273002/diff/160001/content/browser/loader... content/browser/loader/replacing_resource_handler.h:28: class CONTENT_EXPORT ReplacingResourceHandler : public LayeredResourceHandler { I'm not a big fan of this name. ResourceHandlerReplacing[Resource]Handler seems unwieldy. RedirectingResourceHandler seems potentially confusing, given that "redirect" could mean redirect in an HTTP-sense. Maybe InterceptingResourceHandler? Open to other ideas. https://codereview.chromium.org/2005273002/diff/160001/content/browser/loader... content/browser/loader/replacing_resource_handler.h:38: enum class State { I think these states need some documentation. https://codereview.chromium.org/2005273002/diff/160001/content/browser/loader... content/browser/loader/replacing_resource_handler.h:40: WAITING_FOR_NEW_HANDLER, We only keep this state when we're waiting to get a copy of the plugin list, right? So maybe name this to WAITING_FOR_PLUGINS, and only set the state to this when the information returned by plugin_service_->GetPluginInfo is stale? https://codereview.chromium.org/2005273002/diff/160001/content/browser/loader... content/browser/loader/replacing_resource_handler.h:41: WAITING_FOR_BUFFER_COPY, This state is only needed because the MimeTypeResourceHandler calls OnWillRequest before it calls OnResponseStarted, think that warrants a mention. https://codereview.chromium.org/2005273002/diff/160001/content/browser/loader... content/browser/loader/replacing_resource_handler.h:42: CHOICE_MADE, We've also made the choice in WAITING_FOR_BUFFER_COPY. Maybe just call it PASS_THROUGH or something? Though that may imply we're passing through to the original handler.... https://codereview.chromium.org/2005273002/diff/160001/content/browser/loader... content/browser/loader/replacing_resource_handler.h:43: }; nit: Suggest a blank line here.
https://codereview.chromium.org/2005273002/diff/160001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2005273002/diff/160001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1731: // appropriate one based on the MIME type of the response if needed. Maybe mention that this must be at the end of the chain, just before |handler|?
The description says MimeTypeResourceHandler is moved before ThrottlingResourceHandler so that NavigationThrottles have knowledge of downloads, but the stuff about deciding if it's a download is moved into ReplacingResourceHandler which is still at the end. So that's a bit confusing. On 2016/06/09 16:12:17, mmenke wrote: > mailto:mmenke@chromium.org changed reviewers: > + mailto:mattm@chromium.org Was there anything in particular I should look at? https://codereview.chromium.org/2005273002/diff/160001/content/browser/loader... File content/browser/loader/mime_type_resource_handler.cc (right): https://codereview.chromium.org/2005273002/diff/160001/content/browser/loader... content/browser/loader/mime_type_resource_handler.cc:27: #include "content/public/browser/plugin_service.h" I think the plugin and download includes could be removed from this file? https://codereview.chromium.org/2005273002/diff/160001/content/browser/loader... File content/browser/loader/mime_type_resource_handler_unittest.cc (left): https://codereview.chromium.org/2005273002/diff/160001/content/browser/loader... content/browser/loader/mime_type_resource_handler_unittest.cc:459: #endif These tests are simply removed. Should they instead be moved into a replacing_resource_handler_unittest.cc ? https://codereview.chromium.org/2005273002/diff/160001/content/browser/loader... File content/browser/loader/replacing_resource_handler.cc (right): https://codereview.chromium.org/2005273002/diff/160001/content/browser/loader... content/browser/loader/replacing_resource_handler.cc:33: #include "net/base/mime_sniffer.h" not used here?
On 2016/06/09 22:55:33, mattm wrote: > The description says MimeTypeResourceHandler is moved before > ThrottlingResourceHandler so that NavigationThrottles have knowledge of > downloads, but the stuff about deciding if it's a download is moved into > ReplacingResourceHandler which is still at the end. So that's a bit confusing. > > On 2016/06/09 16:12:17, mmenke wrote: > > mailto:mmenke@chromium.org changed reviewers: > > + mailto:mattm@chromium.org > > Was there anything in particular I should look at? Oops...Meant to ask if there were any safe-browsing concerns here - it checks URLs before we send requests, and not response headers, right? We're moving buffering/mime sniffing before SafeBrowsing, so can populate the cache, etc, before it sees response headers. I don't think there are any issues here, but wanted to double-check.
On 2016/06/09 23:52:37, mmenke wrote: > On 2016/06/09 22:55:33, mattm wrote: > > The description says MimeTypeResourceHandler is moved before > > ThrottlingResourceHandler so that NavigationThrottles have knowledge of > > downloads, but the stuff about deciding if it's a download is moved into > > ReplacingResourceHandler which is still at the end. So that's a bit confusing. > > > > On 2016/06/09 16:12:17, mmenke wrote: > > > mailto:mmenke@chromium.org changed reviewers: > > > + mailto:mattm@chromium.org > > > > Was there anything in particular I should look at? > > Oops...Meant to ask if there were any safe-browsing concerns here - it checks > URLs before we send requests, and not response headers, right? We're moving > buffering/mime sniffing before SafeBrowsing, so can populate the cache, etc, > before it sees response headers. I don't think there are any issues here, but > wanted to double-check. Populate the cache with response bodies, that is.
On 2016/06/09 23:53:11, mmenke wrote: > On 2016/06/09 23:52:37, mmenke wrote: > > On 2016/06/09 22:55:33, mattm wrote: > > > The description says MimeTypeResourceHandler is moved before > > > ThrottlingResourceHandler so that NavigationThrottles have knowledge of > > > downloads, but the stuff about deciding if it's a download is moved into > > > ReplacingResourceHandler which is still at the end. So that's a bit > confusing. > > > > > > On 2016/06/09 16:12:17, mmenke wrote: > > > > mailto:mmenke@chromium.org changed reviewers: > > > > + mailto:mattm@chromium.org > > > > > > Was there anything in particular I should look at? > > > > Oops...Meant to ask if there were any safe-browsing concerns here - it checks > > URLs before we send requests, and not response headers, right? We're moving > > buffering/mime sniffing before SafeBrowsing, so can populate the cache, etc, > > before it sees response headers. I don't think there are any issues here, but > > wanted to double-check. > > Populate the cache with response bodies, that is. [+nparker fyi, and in case you wanted to add any comments about the mobile url checks situation.] On desktop, yes. The mobile case could be affected. SB Downloads: Don't think this would be different than before. Regular browsing, on desktop: URLs are checked before connection is started, so that should be fine. Regular browsing, on mobile: URL check and connection are started in parallel, and the request is deferred if the URL check isn't finished by the time SafeBrowsingResourceThrottle::WillProcessResponse occurs. So this would be different. (Mobile is different to mitigate the ipc delay using gmscore for safebrowsing checks.)
nparker@chromium.org changed reviewers: + nparker@chromium.org
Yes, on mobile we currently receive the headers before Safe Browsing looks at the request. Will we in any way trusting these headers, or writing things to cache, before safe browsing? I suspect this change is ok, as long as it doesn't allow downloading the body of the request (which I suspect it can't) w/o SB.
On 2016/06/10 16:45:14, Nathan Parker wrote: > Yes, on mobile we currently receive the headers before Safe Browsing looks at > the request. Will we in any way trusting these headers, or writing things to > cache, before safe browsing? I suspect this change is ok, as long as it doesn't > allow downloading the body of the request (which I suspect it can't) w/o SB. We've always written the headers to the cache before safebrowsing. Now we'll be writing (Some of, potentially all of) responses to the cache as well, if we have to sniff MIME type. Since the safebrowsing is between the cache and everything else, I *think* this is fine, but wanted to make sure you guys didn't have any concerns.
On 2016/06/10 16:46:50, mmenke wrote: > On 2016/06/10 16:45:14, Nathan Parker wrote: > > Yes, on mobile we currently receive the headers before Safe Browsing looks at > > the request. Will we in any way trusting these headers, or writing things to > > cache, before safe browsing? I suspect this change is ok, as long as it > doesn't > > allow downloading the body of the request (which I suspect it can't) w/o SB. > > We've always written the headers to the cache before safebrowsing. Now we'll be > writing (Some of, potentially all of) responses to the cache as well, if we have > to sniff MIME type. Since the safebrowsing is between the cache and everything > else, I *think* this is fine, but wanted to make sure you guys didn't have any > concerns. I was thinking about that. An issue is that if a compromised site is corrected and the SB list is updated to remove the site, the bad response could still be cached on the client and SB would then allow it.
On 2016/06/10 17:00:16, mattm wrote: > On 2016/06/10 16:46:50, mmenke wrote: > > On 2016/06/10 16:45:14, Nathan Parker wrote: > > > Yes, on mobile we currently receive the headers before Safe Browsing looks > at > > > the request. Will we in any way trusting these headers, or writing things > to > > > cache, before safe browsing? I suspect this change is ok, as long as it > > doesn't > > > allow downloading the body of the request (which I suspect it can't) w/o SB. > > > > We've always written the headers to the cache before safebrowsing. Now we'll > be > > writing (Some of, potentially all of) responses to the cache as well, if we > have > > to sniff MIME type. Since the safebrowsing is between the cache and > everything > > else, I *think* this is fine, but wanted to make sure you guys didn't have any > > concerns. > > I was thinking about that. An issue is that if a compromised site is corrected > and the SB list is updated to remove the site, the bad response could still be > cached on the client and SB would then allow it. Great point. So this is a potential problem on mobile.
On 2016/06/10 17:12:29, mmenke wrote: > On 2016/06/10 17:00:16, mattm wrote: > > On 2016/06/10 16:46:50, mmenke wrote: > > > On 2016/06/10 16:45:14, Nathan Parker wrote: > > > > Yes, on mobile we currently receive the headers before Safe Browsing looks > > at > > > > the request. Will we in any way trusting these headers, or writing things > > to > > > > cache, before safe browsing? I suspect this change is ok, as long as it > > > doesn't > > > > allow downloading the body of the request (which I suspect it can't) w/o > SB. > > > > > > We've always written the headers to the cache before safebrowsing. Now > we'll > > be > > > writing (Some of, potentially all of) responses to the cache as well, if we > > have > > > to sniff MIME type. Since the safebrowsing is between the cache and > > everything > > > else, I *think* this is fine, but wanted to make sure you guys didn't have > any > > > concerns. > > > > I was thinking about that. An issue is that if a compromised site is corrected > > and the SB list is updated to remove the site, the bad response could still be > > cached on the client and SB would then allow it. > > Great point. So this is a potential problem on mobile. If we're sufficiently concerned about this, we could consider ordering the ResourceHandlers like this: <main resource throttles> -> MimeTypeHandler -> NavigationResourceThrottle -> ReplacingResourceHAndler -> AsyncResourceHandler. i.e. create a second ThrottlingResourceHandler, just for navigation.
Thanks! I have updated the patch and added more unit tests. NavigationResourceThrottle has NavigationThrottles on the UI thread that should execute before SafeBrowsing, so the split proposed is not possible. Also, the goal for navigation is to move any ResourceThrottle that interacts with navigations on the UI thread to a NavigationThrottle. This includes SafeBrowsing. https://codereview.chromium.org/2005273002/diff/160001/content/browser/loader... File content/browser/loader/mime_type_resource_handler.cc (right): https://codereview.chromium.org/2005273002/diff/160001/content/browser/loader... content/browser/loader/mime_type_resource_handler.cc:27: #include "content/public/browser/plugin_service.h" On 2016/06/09 22:55:32, mattm wrote: > I think the plugin and download includes could be removed from this file? Done. Removed some more includes that were no longer needed. https://codereview.chromium.org/2005273002/diff/160001/content/browser/loader... content/browser/loader/mime_type_resource_handler.cc:126: bool* defer) { On 2016/06/09 18:09:29, mmenke wrote: > DCHECK_EQ(STATE_STARTING, state_);? Done. https://codereview.chromium.org/2005273002/diff/160001/content/browser/loader... content/browser/loader/mime_type_resource_handler.cc:236: DCHECK(state_ == STATE_STREAMING); On 2016/06/09 18:09:29, mmenke wrote: > DCHECK_EQ(STATE_STREAMING, state_); Done. https://codereview.chromium.org/2005273002/diff/160001/content/browser/loader... content/browser/loader/mime_type_resource_handler.cc:272: DCHECK(state_ == STATE_BUFFERING); On 2016/06/09 18:09:29, mmenke wrote: > DCHECK_EQ Done. https://codereview.chromium.org/2005273002/diff/160001/content/browser/loader... content/browser/loader/mime_type_resource_handler.cc:278: DCHECK(state_ == STATE_REPLAYING_RESPONSE_RECEIVED); On 2016/06/09 18:09:29, mmenke wrote: > DCHECK_EQ Done. https://codereview.chromium.org/2005273002/diff/160001/content/browser/loader... File content/browser/loader/mime_type_resource_handler.h (right): https://codereview.chromium.org/2005273002/diff/160001/content/browser/loader... content/browser/loader/mime_type_resource_handler.h:35: class CONTENT_EXPORT MimeTypeResourceHandler : public LayeredResourceHandler, On 2016/06/09 18:09:29, mmenke wrote: > Think this can now be renamed to MimeSniffingResourceHandler, since that's all > it does now. Done. https://codereview.chromium.org/2005273002/diff/160001/content/browser/loader... content/browser/loader/mime_type_resource_handler.h:45: // acting as a blind pass-through ResourceHandler. On 2016/06/09 18:09:29, mmenke wrote: > It's not a blind pass-through ResourceHandler. It switches to STATE_BUFFERING > in OnResponseStarted. Clarified this in the comment. https://codereview.chromium.org/2005273002/diff/160001/content/browser/loader... File content/browser/loader/replacing_resource_handler.cc (right): https://codereview.chromium.org/2005273002/diff/160001/content/browser/loader... content/browser/loader/replacing_resource_handler.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/06/09 18:09:29, mmenke wrote: > Weird that git isn't picking up that this file was forked from > mime_type_resource_handler. Could you try turning down the similarity > threshold? Makes review easier. Done. https://codereview.chromium.org/2005273002/diff/160001/content/browser/loader... content/browser/loader/replacing_resource_handler.cc:33: #include "net/base/mime_sniffer.h" On 2016/06/09 22:55:32, mattm wrote: > not used here? Done. https://codereview.chromium.org/2005273002/diff/160001/content/browser/loader... content/browser/loader/replacing_resource_handler.cc:64: return true; On 2016/06/09 18:09:29, mmenke wrote: > BUG: This should be passed through to the next handler, and state should be set > to State::CHOICE_MADE. Should have a test for that case. Done. https://codereview.chromium.org/2005273002/diff/160001/content/browser/loader... content/browser/loader/replacing_resource_handler.cc:94: DCHECK(state_ == State::WAITING_FOR_BUFFER_COPY); On 2016/06/09 18:09:29, mmenke wrote: > DCHECK_EQ Done. https://codereview.chromium.org/2005273002/diff/160001/content/browser/loader... File content/browser/loader/replacing_resource_handler.h (right): https://codereview.chromium.org/2005273002/diff/160001/content/browser/loader... content/browser/loader/replacing_resource_handler.h:28: class CONTENT_EXPORT ReplacingResourceHandler : public LayeredResourceHandler { On 2016/06/09 18:09:29, mmenke wrote: > I'm not a big fan of this name. ResourceHandlerReplacing[Resource]Handler seems > unwieldy. RedirectingResourceHandler seems potentially confusing, given that > "redirect" could mean redirect in an HTTP-sense. Maybe > InterceptingResourceHandler? Open to other ideas. Done. https://codereview.chromium.org/2005273002/diff/160001/content/browser/loader... content/browser/loader/replacing_resource_handler.h:38: enum class State { On 2016/06/09 18:09:29, mmenke wrote: > I think these states need some documentation. Done. https://codereview.chromium.org/2005273002/diff/160001/content/browser/loader... content/browser/loader/replacing_resource_handler.h:40: WAITING_FOR_NEW_HANDLER, On 2016/06/09 18:09:29, mmenke wrote: > We only keep this state when we're waiting to get a copy of the plugin list, > right? So maybe name this to WAITING_FOR_PLUGINS, and only set the state to > this when the information returned by plugin_service_->GetPluginInfo is stale? Done. https://codereview.chromium.org/2005273002/diff/160001/content/browser/loader... content/browser/loader/replacing_resource_handler.h:41: WAITING_FOR_BUFFER_COPY, On 2016/06/09 18:09:29, mmenke wrote: > This state is only needed because the MimeTypeResourceHandler calls > OnWillRequest before it calls OnResponseStarted, think that warrants a mention. Done. https://codereview.chromium.org/2005273002/diff/160001/content/browser/loader... content/browser/loader/replacing_resource_handler.h:42: CHOICE_MADE, On 2016/06/09 18:09:29, mmenke wrote: > We've also made the choice in WAITING_FOR_BUFFER_COPY. Maybe just call it > PASS_THROUGH or something? Though that may imply we're passing through to the > original handler.... I've called it DONE, since the InterceptingResourceHandler is no longer doing anything. I can change it to PASS_THROUGH if you prefer. https://codereview.chromium.org/2005273002/diff/160001/content/browser/loader... content/browser/loader/replacing_resource_handler.h:43: }; On 2016/06/09 18:09:29, mmenke wrote: > nit: Suggest a blank line here. Done. https://codereview.chromium.org/2005273002/diff/160001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2005273002/diff/160001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1731: // appropriate one based on the MIME type of the response if needed. On 2016/06/09 18:18:42, mmenke wrote: > Maybe mention that this must be at the end of the chain, just before |handler|? Done.
Going to take me a while to go through all the tests. I'm on network bug triage Thursday and Friday, so I may well not get to this until next week.
https://codereview.chromium.org/2005273002/diff/240001/content/browser/loader... File content/browser/loader/intercepting_resource_handler.cc (right): https://codereview.chromium.org/2005273002/diff/240001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:62: response->head.headers->response_code() == 304) { For HTTP(-like) responses, this class does the following: 304: Short-circuit everything and avoid any interception. 2xx: Intercept if necessary and swap the handler with the new one. everything else: Create a ResourceHandler for interception, but destroy it immediately (see UseAlternateResourceHandler below). Shall we centralize the interception policy calculation? Doing so would allow this class to decide whether an interception is allowed or not *before* creating any resource handlers. That would also indirectly fix the bug below where ResourceRequestInfo::is_download is set for a resource that's not a download if UseAlternateResourceHandler decides not to allow interception after all. https://codereview.chromium.org/2005273002/diff/240001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:248: next_handler_->OnWillRead(&buf, &size, -1); Is this correct? It seems that the old handler could see a sequence like the following: ->OnWillRead(...) // Due to MimeSniffingResourceHandler passing through OnWillRead() before OnResponseStarted() ->OnResponseStarted(...) // L235 above. ->OnWillRead(...) // L248 above. ->OnReadCompleted(...) // L252 below Are ResourceHandlers in general expected to be resilient to multiple OnWillRead() calls without intervening OnReadCompleted()?
https://codereview.chromium.org/2005273002/diff/240001/content/browser/loader... File content/browser/loader/intercepting_resource_handler.cc (right): https://codereview.chromium.org/2005273002/diff/240001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:62: response->head.headers->response_code() == 304) { On 2016/06/22 20:26:53, asanka wrote: > For HTTP(-like) responses, this class does the following: > > 304: Short-circuit everything and avoid any interception. > > 2xx: Intercept if necessary and swap the handler with the new one. > > everything else: Create a ResourceHandler for interception, but destroy it > immediately (see UseAlternateResourceHandler below). > > Shall we centralize the interception policy calculation? Doing so would allow > this class to decide whether an interception is allowed or not *before* creating > any resource handlers. If this is an <a download> link, but we get a 4xx error, don't we need to create a ResourceHandler to have a download error appear on the download shelf? At least I assumed that was the purpose of the "Everything else" behavior. > That would also indirectly fix the bug below where > ResourceRequestInfo::is_download is set for a resource that's not a download if > UseAlternateResourceHandler decides not to allow interception after all.
https://codereview.chromium.org/2005273002/diff/240001/content/browser/loader... File content/browser/loader/intercepting_resource_handler.cc (right): https://codereview.chromium.org/2005273002/diff/240001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:62: response->head.headers->response_code() == 304) { On 2016/06/22 at 20:31:18, mmenke wrote: > On 2016/06/22 20:26:53, asanka wrote: > > For HTTP(-like) responses, this class does the following: > > > > 304: Short-circuit everything and avoid any interception. > > > > 2xx: Intercept if necessary and swap the handler with the new one. > > > > everything else: Create a ResourceHandler for interception, but destroy it > > immediately (see UseAlternateResourceHandler below). > > > > Shall we centralize the interception policy calculation? Doing so would allow > > this class to decide whether an interception is allowed or not *before* creating > > any resource handlers. > > If this is an <a download> link, but we get a 4xx error, don't we need to create a ResourceHandler to have a download error appear on the download shelf? At least I assumed that was the purpose of the "Everything else" behavior. > <a download>s are explicit downloads and don't use the resource handler switching mechanism (previously known as BufferedResourceHandler and MimeTypeResourceHandler). Instead, for explicit downloads, ResourceDispatcherHostImpl constructs the ResourceHandler chain with DownloadResourceHandler already attached at the end and has to handle all responses including 404s. Hence for explicit downloads, the DownloadResourceHandler always gets called for error responses. (well, with the exception of any interference by one of the throttles on ThrottlingResourceHandler). The mechanism in the "Everything else" category above destroys the pending ResourceHandler chain without calling OnResponseStarted/OnResponseComplete. I.e. those handlers don't see a response. > > That would also indirectly fix the bug below where > > ResourceRequestInfo::is_download is set for a resource that's not a download if > > UseAlternateResourceHandler decides not to allow interception after all.
https://codereview.chromium.org/2005273002/diff/240001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2005273002/diff/240001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1728: handler.reset(new MimeSniffingResourceHandler(std::move(handler), request)); Note that this ResourceHandler is special. Every resource handler behind it has to be prepared to service OnWillRead *before* OnResponseStarted. It used to be an exception that only the last ResourceHandler needed to handle, but it's now the norm for all ResourceHandlers due to MimeSniffingResourceHandler being at the head of the chain. Maybe we should document it.
On 2016/06/23 02:30:13, asanka wrote: > https://codereview.chromium.org/2005273002/diff/240001/content/browser/loader... > File content/browser/loader/resource_dispatcher_host_impl.cc (right): > > https://codereview.chromium.org/2005273002/diff/240001/content/browser/loader... > content/browser/loader/resource_dispatcher_host_impl.cc:1728: handler.reset(new > MimeSniffingResourceHandler(std::move(handler), request)); > Note that this ResourceHandler is special. Every resource handler behind it has > to be prepared to service OnWillRead *before* OnResponseStarted. It used to be > an exception that only the last ResourceHandler needed to handle, but it's now > the norm for all ResourceHandlers due to MimeSniffingResourceHandler being at > the head of the chain. > > Maybe we should document it. Hrm...I wonder if it would make sense just to always call OnWillRead before OnResponseStarted. In the worst case, we allocate memory a little earlier, but OnWillRead should generally be pretty fast, I think. Well beyond the scope of this CL, of course.
Thanks! Working on some CL that depends on this, I realized that the interception checks need to be performed in MimeSniffingResourceHandler, otherwise the ResourceRequestInfo won't properly be updated. There's still one unit test failing, I will investigate tomorrow. https://codereview.chromium.org/2005273002/diff/240001/content/browser/loader... File content/browser/loader/intercepting_resource_handler.cc (right): https://codereview.chromium.org/2005273002/diff/240001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:62: response->head.headers->response_code() == 304) { On 2016/06/23 02:26:17, asanka wrote: > On 2016/06/22 at 20:31:18, mmenke wrote: > > On 2016/06/22 20:26:53, asanka wrote: > > > For HTTP(-like) responses, this class does the following: > > > > > > 304: Short-circuit everything and avoid any interception. > > > > > > 2xx: Intercept if necessary and swap the handler with the new one. > > > > > > everything else: Create a ResourceHandler for interception, but destroy it > > > immediately (see UseAlternateResourceHandler below). > > > > > > Shall we centralize the interception policy calculation? Doing so would > allow > > > this class to decide whether an interception is allowed or not *before* > creating > > > any resource handlers. > > > > If this is an <a download> link, but we get a 4xx error, don't we need to > create a ResourceHandler to have a download error appear on the download shelf? > At least I assumed that was the purpose of the "Everything else" behavior. > > > > <a download>s are explicit downloads and don't use the resource handler > switching mechanism (previously known as BufferedResourceHandler and > MimeTypeResourceHandler). Instead, for explicit downloads, > ResourceDispatcherHostImpl constructs the ResourceHandler chain with > DownloadResourceHandler already attached at the end and has to handle all > responses including 404s. Hence for explicit downloads, the > DownloadResourceHandler always gets called for error responses. (well, with the > exception of any interference by one of the throttles on > ThrottlingResourceHandler). > > The mechanism in the "Everything else" category above destroys the pending > ResourceHandler chain without calling OnResponseStarted/OnResponseComplete. I.e. > those handlers don't see a response. > > > > > That would also indirectly fix the bug below where > > > ResourceRequestInfo::is_download is set for a resource that's not a download > if > > > UseAlternateResourceHandler decides not to allow interception after all. > I've moved the interception choice case to the MimeSniffingResourceHandler so that the ResourceRequestInfo is properly updated (is_stream and is_download). There, I've put a specific method to check whether we should attempt interception or not at all. https://codereview.chromium.org/2005273002/diff/240001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:248: next_handler_->OnWillRead(&buf, &size, -1); On 2016/06/22 20:26:53, asanka wrote: > Is this correct? It seems that the old handler could see a sequence like the > following: > > ->OnWillRead(...) // Due to MimeSniffingResourceHandler passing through > OnWillRead() before OnResponseStarted() > > ->OnResponseStarted(...) // L235 above. > ->OnWillRead(...) // L248 above. > ->OnReadCompleted(...) // L252 below > > Are ResourceHandlers in general expected to be resilient to multiple > OnWillRead() calls without intervening OnReadCompleted()? I'm now doing a OnReadCompleted of size 0 so that we do OnWillRead OnResponseStarted OnReadCompleted OnWillReda OnReadCompleted. https://codereview.chromium.org/2005273002/diff/240001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2005273002/diff/240001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1728: handler.reset(new MimeSniffingResourceHandler(std::move(handler), request)); On 2016/06/23 02:30:13, asanka wrote: > Note that this ResourceHandler is special. Every resource handler behind it has > to be prepared to service OnWillRead *before* OnResponseStarted. It used to be > an exception that only the last ResourceHandler needed to handle, but it's now > the norm for all ResourceHandlers due to MimeSniffingResourceHandler being at > the head of the chain. > > Maybe we should document it. Added documentation about this behavior in the comment.
https://codereview.chromium.org/2005273002/diff/240001/content/browser/loader... File content/browser/loader/intercepting_resource_handler.cc (right): https://codereview.chromium.org/2005273002/diff/240001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:62: response->head.headers->response_code() == 304) { On 2016/06/28 16:20:41, clamy wrote: > On 2016/06/23 02:26:17, asanka wrote: > > On 2016/06/22 at 20:31:18, mmenke wrote: > > > On 2016/06/22 20:26:53, asanka wrote: > > > > For HTTP(-like) responses, this class does the following: > > > > > > > > 304: Short-circuit everything and avoid any interception. > > > > > > > > 2xx: Intercept if necessary and swap the handler with the new one. > > > > > > > > everything else: Create a ResourceHandler for interception, but destroy > it > > > > immediately (see UseAlternateResourceHandler below). > > > > > > > > Shall we centralize the interception policy calculation? Doing so would > > allow > > > > this class to decide whether an interception is allowed or not *before* > > creating > > > > any resource handlers. > > > > > > If this is an <a download> link, but we get a 4xx error, don't we need to > > create a ResourceHandler to have a download error appear on the download > shelf? > > At least I assumed that was the purpose of the "Everything else" behavior. > > > > > > > <a download>s are explicit downloads and don't use the resource handler > > switching mechanism (previously known as BufferedResourceHandler and > > MimeTypeResourceHandler). Instead, for explicit downloads, > > ResourceDispatcherHostImpl constructs the ResourceHandler chain with > > DownloadResourceHandler already attached at the end and has to handle all > > responses including 404s. Hence for explicit downloads, the > > DownloadResourceHandler always gets called for error responses. (well, with > the > > exception of any interference by one of the throttles on > > ThrottlingResourceHandler). > > > > The mechanism in the "Everything else" category above destroys the pending > > ResourceHandler chain without calling OnResponseStarted/OnResponseComplete. > I.e. > > those handlers don't see a response. > > > > > > > > That would also indirectly fix the bug below where > > > > ResourceRequestInfo::is_download is set for a resource that's not a > download > > if > > > > UseAlternateResourceHandler decides not to allow interception after all. > > > > I've moved the interception choice case to the MimeSniffingResourceHandler so > that the ResourceRequestInfo is properly updated (is_stream and is_download). > There, I've put a specific method to check whether we should attempt > interception or not at all. It appears that one of teh unit tests is (ForbiddenDownload) is enforcing the behavior of canceling with error if we get a non 2* response on a request we went to intercept. I put that back in place.
On 2016/06/29 13:35:57, clamy wrote: > https://codereview.chromium.org/2005273002/diff/240001/content/browser/loader... > File content/browser/loader/intercepting_resource_handler.cc (right): > > https://codereview.chromium.org/2005273002/diff/240001/content/browser/loader... > content/browser/loader/intercepting_resource_handler.cc:62: > response->head.headers->response_code() == 304) { > On 2016/06/28 16:20:41, clamy wrote: > > On 2016/06/23 02:26:17, asanka wrote: > > > On 2016/06/22 at 20:31:18, mmenke wrote: > > > > On 2016/06/22 20:26:53, asanka wrote: > > > > > For HTTP(-like) responses, this class does the following: > > > > > > > > > > 304: Short-circuit everything and avoid any interception. > > > > > > > > > > 2xx: Intercept if necessary and swap the handler with the new one. > > > > > > > > > > everything else: Create a ResourceHandler for interception, but > destroy > > it > > > > > immediately (see UseAlternateResourceHandler below). > > > > > > > > > > Shall we centralize the interception policy calculation? Doing so would > > > allow > > > > > this class to decide whether an interception is allowed or not *before* > > > creating > > > > > any resource handlers. > > > > > > > > If this is an <a download> link, but we get a 4xx error, don't we need to > > > create a ResourceHandler to have a download error appear on the download > > shelf? > > > At least I assumed that was the purpose of the "Everything else" behavior. > > > > > > > > > > <a download>s are explicit downloads and don't use the resource handler > > > switching mechanism (previously known as BufferedResourceHandler and > > > MimeTypeResourceHandler). Instead, for explicit downloads, > > > ResourceDispatcherHostImpl constructs the ResourceHandler chain with > > > DownloadResourceHandler already attached at the end and has to handle all > > > responses including 404s. Hence for explicit downloads, the > > > DownloadResourceHandler always gets called for error responses. (well, with > > the > > > exception of any interference by one of the throttles on > > > ThrottlingResourceHandler). > > > > > > The mechanism in the "Everything else" category above destroys the pending > > > ResourceHandler chain without calling OnResponseStarted/OnResponseComplete. > > I.e. > > > those handlers don't see a response. > > > > > > > > > > > That would also indirectly fix the bug below where > > > > > ResourceRequestInfo::is_download is set for a resource that's not a > > download > > > if > > > > > UseAlternateResourceHandler decides not to allow interception after all. > > > > > > > I've moved the interception choice case to the MimeSniffingResourceHandler so > > that the ResourceRequestInfo is properly updated (is_stream and is_download). > > There, I've put a specific method to check whether we should attempt > > interception or not at all. > > It appears that one of teh unit tests is (ForbiddenDownload) is enforcing the > behavior of canceling with error if we get a non 2* response on a request we > went to intercept. I put that back in place. Sorry for latency here, plan to do a full pass later today.
@mmenke: could you take a look at the latest patchset this week? I'm out until the end of the week, but I'd like to be able to iterate on it on Monday. This is starting to block a lot of my work.
On 2016/07/13 16:25:26, clamy (ooo until Jul 18) wrote: > @mmenke: could you take a look at the latest patchset this week? I'm out until > the end of the week, but I'd like to be able to iterate on it on Monday. This is > starting to block a lot of my work. Sorry for slowness - not a big fan of the last change, but haven't had time to dig into it enough to suggest an alternative. Every bot failing the last run also made it unclear if you were planning on fixing things.
The CQ bit was checked by clamy@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: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The redness was due to a patch conflict. Uploaded the rebase. There's still an issue with the destruction sequence of some of the newly added unittests (it passes locally). I will fix that in the next patch set (and the Android compilation error). Can you take a look at the current one?
Sorry it took me so long. I'm going to continue looking at the CL today, but this comment addresses the issue that it took me so long to get around to understanding. I expect any other issues I'll raise to be nits. https://codereview.chromium.org/2005273002/diff/320001/content/browser/loader... File content/browser/loader/mime_sniffing_resource_handler.cc (right): https://codereview.chromium.org/2005273002/diff/320001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler.cc:450: intercepting_handler_->UseNewHandler(std::move(handler), payload); I'm not a big fan of having this dig into intercepting_handler. It looks like all we need from this class that we weren't getting is the is_download bit? Given the "DCHECK(!info->allow_download());" in the plugin path, I think we can just have MimeSniffingResourceHandler::CheckForInterception set the is_download bit, and not set up the next handler, and then have the intercepting_handler use that bit itself, as well as checking for a plugin? Not great, but I think it's better than this.
https://codereview.chromium.org/2005273002/diff/320001/content/browser/loader... File content/browser/loader/intercepting_resource_handler.cc (right): https://codereview.chromium.org/2005273002/diff/320001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:8: #include <vector> Not used https://codereview.chromium.org/2005273002/diff/320001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:87: bytes_read_ += bytes_read; Seems like bytes_read_ isn't needed, given that this method is only called once. Can just pass it directly to CopyReadBufferToNextHandler, right? https://codereview.chromium.org/2005273002/diff/320001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:111: DCHECK(!defer_ignored); I don't understand this - why are we completing the read with 0 bytes, only to create a new buffer, when we presumably could have just use read_buffer_? https://codereview.chromium.org/2005273002/diff/320001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:114: // Get a new buffer iand use it to send |payload_for_old_handler_| to the nit: iand -> and https://codereview.chromium.org/2005273002/diff/320001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:115: // next ResourceHandler. "next ResourceHandler" is extremely confusing - this is the original ResourceHandler. https://codereview.chromium.org/2005273002/diff/320001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:132: bool InterceptingResourceHandler::CopyReadBufferToNextHandler() { Suggest just inlining this. The method it's called from is short enough, and the "state_ = State::DONE" transition seems a bit unexpected in here. THink it's clearer just to inline. https://codereview.chromium.org/2005273002/diff/320001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:141: state_ = State::DONE; Suppose it doesn't really matter what we do if OnWillRead fails, but seems most consistent, and least likely to cause big colorful explosions, if we move this line above the return false. https://codereview.chromium.org/2005273002/diff/320001/content/browser/loader... File content/browser/loader/intercepting_resource_handler.h (right): https://codereview.chromium.org/2005273002/diff/320001/content/browser/loader... content/browser/loader/intercepting_resource_handler.h:9: #include <vector> Not used. https://codereview.chromium.org/2005273002/diff/320001/content/browser/loader... content/browser/loader/intercepting_resource_handler.h:71: std::unique_ptr<ResourceHandler> new_handler_; include <memory> https://codereview.chromium.org/2005273002/diff/320001/content/browser/loader... content/browser/loader/intercepting_resource_handler.h:75: scoped_refptr<net::IOBuffer> read_buffer_; include net/base/io_buffer.h and base/memory/ref_counted.h https://codereview.chromium.org/2005273002/diff/320001/content/browser/loader... File content/browser/loader/mime_sniffing_resource_handler.cc (right): https://codereview.chromium.org/2005273002/diff/320001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler.cc:294: if (CanBeIntercepted() && !CheckForInterception(defer)) Maybe move CanBeIntercepted() to the first line of CheckForInterception? https://codereview.chromium.org/2005273002/diff/320001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler.cc:397: // This is request is a download, This comment seems weird, just above a "CheckInterceptionIsValid() -> fail request" call. https://codereview.chromium.org/2005273002/diff/320001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler.cc:398: if (!CheckInterceptionIsValid()) We have: CanBeIntercepted(), CheckForInterception(), CheckInterceptionIsValid(). These names seem very confusing to me. Maybe: CheckInterceptionIsValid() -> CheckResponseIsNotProvisional() [Per spec, 100 responses are "provisional". We may not even need the method, but getting rid of it is beyond the scope of this CL, of course]. May maybe even better, CheckResponseCanBeDownloaded(). Note that this check is just used for downloads, not plugin interceptions, so "CheckInterceptionIsValid()" seems inaccurate as well as confusing.
The CQ bit was checked by clamy@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: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Thanks! https://codereview.chromium.org/2005273002/diff/320001/content/browser/loader... File content/browser/loader/intercepting_resource_handler.cc (right): https://codereview.chromium.org/2005273002/diff/320001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:8: #include <vector> On 2016/07/18 18:50:42, mmenke wrote: > Not used Done. https://codereview.chromium.org/2005273002/diff/320001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:87: bytes_read_ += bytes_read; On 2016/07/18 18:50:42, mmenke wrote: > Seems like bytes_read_ isn't needed, given that this method is only called once. > Can just pass it directly to CopyReadBufferToNextHandler, right? Done. https://codereview.chromium.org/2005273002/diff/320001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:111: DCHECK(!defer_ignored); On 2016/07/18 18:50:42, mmenke wrote: > I don't understand this - why are we completing the read with 0 bytes, only to > create a new buffer, when we presumably could have just use read_buffer_? Because we may already have read something in read_buffer_ that we want to pass to the new handler, so we don't want to override it with the payload for the old handler. Rewrote the comment to make it clearer. https://codereview.chromium.org/2005273002/diff/320001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:114: // Get a new buffer iand use it to send |payload_for_old_handler_| to the On 2016/07/18 18:50:42, mmenke wrote: > nit: iand -> and Done. https://codereview.chromium.org/2005273002/diff/320001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:115: // next ResourceHandler. On 2016/07/18 18:50:42, mmenke wrote: > "next ResourceHandler" is extremely confusing - this is the original > ResourceHandler. Done. https://codereview.chromium.org/2005273002/diff/320001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:132: bool InterceptingResourceHandler::CopyReadBufferToNextHandler() { On 2016/07/18 18:50:42, mmenke wrote: > Suggest just inlining this. The method it's called from is short enough, and > the "state_ = State::DONE" transition seems a bit unexpected in here. THink > it's clearer just to inline. Done. https://codereview.chromium.org/2005273002/diff/320001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:141: state_ = State::DONE; On 2016/07/18 18:50:42, mmenke wrote: > Suppose it doesn't really matter what we do if OnWillRead fails, but seems most > consistent, and least likely to cause big colorful explosions, if we move this > line above the return false. Done. https://codereview.chromium.org/2005273002/diff/320001/content/browser/loader... File content/browser/loader/intercepting_resource_handler.h (right): https://codereview.chromium.org/2005273002/diff/320001/content/browser/loader... content/browser/loader/intercepting_resource_handler.h:9: #include <vector> On 2016/07/18 18:50:42, mmenke wrote: > Not used. Done. https://codereview.chromium.org/2005273002/diff/320001/content/browser/loader... content/browser/loader/intercepting_resource_handler.h:71: std::unique_ptr<ResourceHandler> new_handler_; On 2016/07/18 18:50:42, mmenke wrote: > include <memory> Done. https://codereview.chromium.org/2005273002/diff/320001/content/browser/loader... content/browser/loader/intercepting_resource_handler.h:75: scoped_refptr<net::IOBuffer> read_buffer_; On 2016/07/18 18:50:42, mmenke wrote: > include net/base/io_buffer.h and base/memory/ref_counted.h Done. https://codereview.chromium.org/2005273002/diff/320001/content/browser/loader... File content/browser/loader/mime_sniffing_resource_handler.cc (right): https://codereview.chromium.org/2005273002/diff/320001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler.cc:294: if (CanBeIntercepted() && !CheckForInterception(defer)) On 2016/07/18 18:50:42, mmenke wrote: > Maybe move CanBeIntercepted() to the first line of CheckForInterception? Done. https://codereview.chromium.org/2005273002/diff/320001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler.cc:397: // This is request is a download, On 2016/07/18 18:50:42, mmenke wrote: > This comment seems weird, just above a "CheckInterceptionIsValid() -> fail > request" call. Added a blank line to make it clearer that it's not related to the check itself. https://codereview.chromium.org/2005273002/diff/320001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler.cc:398: if (!CheckInterceptionIsValid()) On 2016/07/18 18:50:42, mmenke wrote: > We have: > > CanBeIntercepted(), CheckForInterception(), CheckInterceptionIsValid(). These > names seem very confusing to me. > > Maybe: > > CheckInterceptionIsValid() -> CheckResponseIsNotProvisional() [Per spec, 100 > responses are "provisional". We may not even need the method, but getting rid > of it is beyond the scope of this CL, of course]. May maybe even better, > CheckResponseCanBeDownloaded(). Note that this check is just used for > downloads, not plugin interceptions, so "CheckInterceptionIsValid()" seems > inaccurate as well as confusing. I went with CheckResponseIsNotProvisional. We do call the method in CheckForPluginHandler, so CheckResponseCanBeDownloaded seemed false. https://codereview.chromium.org/2005273002/diff/320001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler.cc:450: intercepting_handler_->UseNewHandler(std::move(handler), payload); On 2016/07/18 17:58:37, mmenke wrote: > I'm not a big fan of having this dig into intercepting_handler. > > It looks like all we need from this class that we weren't getting is the > is_download bit? Given the "DCHECK(!info->allow_download());" in the plugin > path, I think we can just have MimeSniffingResourceHandler::CheckForInterception > set the is_download bit, and not set up the next handler, and then have the > intercepting_handler use that bit itself, as well as checking for a plugin? > > Not great, but I think it's better than this. We also need the info->is_stream bit that is set in ResourceDispatcherHostImpl::MaybeInterceptAsStream which creates the ResourceHandler for the stream. So we need to call it as well, and I'm not sure if we can call it twice. Ideally, we would modify the public API so we can have a call to teh embedder to ask if this is a download or a stream, but which doesn't set up the new ResourceHandler. Then the intercepting_handler would make a call to a different function to actually get the new ResourceHandler. But the patch is already big enough (and I'm not very familiar with the chromium code) that I did not want to include this in the patch.
Quick followups. I'm pretty happy with the code, just want to resolve these issues, go over the tests, and give the CL a quick once over. Hopefully I'll get to some of the other stuff today, but wanted to make sure I get these out (Have a lot of reviews in my queue) https://codereview.chromium.org/2005273002/diff/320001/content/browser/loader... File content/browser/loader/mime_sniffing_resource_handler.cc (right): https://codereview.chromium.org/2005273002/diff/320001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler.cc:450: intercepting_handler_->UseNewHandler(std::move(handler), payload); On 2016/07/19 14:24:55, clamy wrote: > On 2016/07/18 17:58:37, mmenke wrote: > > I'm not a big fan of having this dig into intercepting_handler. > > > > It looks like all we need from this class that we weren't getting is the > > is_download bit? Given the "DCHECK(!info->allow_download());" in the plugin > > path, I think we can just have > MimeSniffingResourceHandler::CheckForInterception > > set the is_download bit, and not set up the next handler, and then have the > > intercepting_handler use that bit itself, as well as checking for a plugin? > > > > Not great, but I think it's better than this. > > We also need the info->is_stream bit that is set in > ResourceDispatcherHostImpl::MaybeInterceptAsStream which creates the > ResourceHandler for the stream. So we need to call it as well, and I'm not sure > if we can call it twice. > > Ideally, we would modify the public API so we can have a call to teh embedder to > ask if this is a download or a stream, but which doesn't set up the new > ResourceHandler. Then the intercepting_handler would make a call to a different > function to actually get the new ResourceHandler. But the patch is already big > enough (and I'm not very familiar with the chromium code) that I did not want to > include this in the patch. I'm not seeing where is_stream is used in this file. Do you mean you need the is_stream bit for the X-Frame-Options logic, and streams are exempt from X-Frame-Options? I certainly agree that we don't want to make this patch any bigger than it already is. https://codereview.chromium.org/2005273002/diff/340001/content/browser/loader... File content/browser/loader/intercepting_resource_handler.cc (right): https://codereview.chromium.org/2005273002/diff/340001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:118: // the old handler to create a new buffer to store the payload. This seems like a problem. There doesn't seem to be anything to stop the old next handler from re-using the read_buffer_ between the two calls, since we just told it we were done with the old buffer.
Still need a bit more digging into the sniffer tests. https://codereview.chromium.org/2005273002/diff/340001/content/browser/loader... File content/browser/loader/intercepting_resource_handler.h (right): https://codereview.chromium.org/2005273002/diff/340001/content/browser/loader... content/browser/loader/intercepting_resource_handler.h:75: scoped_refptr<net::IOBuffer> read_buffer_; Maybe call this first_read_buffer_, and mention that it's only the results of the first read (from this class's perspective) that may have to be passed to an alternate ResourceHandler instead of the original handler? https://codereview.chromium.org/2005273002/diff/340001/content/browser/loader... File content/browser/loader/intercepting_resource_handler_unittest.cc (right): https://codereview.chromium.org/2005273002/diff/340001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:93: TEST_F(InterceptingResourceHandlerTest, ResponseBodyHandling) { Seems like we should also test the non-intercept case, unexciting as it is. https://codereview.chromium.org/2005273002/diff/340001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:93: TEST_F(InterceptingResourceHandlerTest, ResponseBodyHandling) { Hrm...We aren't testing any of the failure cases (>OnResponseStarted returns false, any of the three calls to OnWillRead returning false, or any of the two calls to OnReadCompleted returning false). Guess testing them is more effort than it's worth. https://codereview.chromium.org/2005273002/diff/340001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:124: char data[] = "The data"; const char kData https://codereview.chromium.org/2005273002/diff/340001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:133: intercepting_handler->UseNewHandler(std::move(new_handler), std::string()); Should check that the old handler gets the abort message. Should also add a test where |payload_for_old_handler_| is non-empty, and make sure the old handler gets the data and the success status. https://codereview.chromium.org/2005273002/diff/340001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:142: EXPECT_FALSE(!memcmp(data, new_test_handler->buffer()->data(), sizeof(data))); Have we put any data in new_test_handler->buffer()->data() yet? I don't see us populating it with \0's anywhere, though I could be missing something. https://codereview.chromium.org/2005273002/diff/340001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:148: EXPECT_TRUE(!memcmp(data, new_test_handler->buffer()->data(), sizeof(data))); Seems like we should check a second read, make sure things behave as expected. https://codereview.chromium.org/2005273002/diff/340001/content/browser/loader... File content/browser/loader/mime_sniffing_resource_handler_unittest.cc (right): https://codereview.chromium.org/2005273002/diff/340001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler_unittest.cc:227: void Cancel() override { cancel_called_++; } night: Blank line before Cancel() https://codereview.chromium.org/2005273002/diff/340001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler_unittest.cc:236: int resume_called() const { return resume_called_; } Maybe call these something like times_blah_called(), blah_call_count(), etc, just to discourage using them with EXPECT_TRUE? https://codereview.chromium.org/2005273002/diff/340001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler_unittest.cc:359: std::unique_ptr<ResourceHandler> intercepting_handler( This can just be a std::unique_ptr<InterceptingResourceHandler>, no? https://codereview.chromium.org/2005273002/diff/340001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler_unittest.cc:380: EXPECT_LT(host.intercepted_as_stream_count(), 2); We don't check that we actually attach the alternate next handler?
I haven't followed the recent changes to this CL closely, but is there a solution to Matts' comments here: >> I was thinking about that. An issue is that if a >> compromised site is corrected and the SB list is >> updated to remove the site, the bad response could >> still be cached on the client and SB would then allow it. > Great point. So this is a potential problem on mobile.
On 2016/07/19 22:14:25, Nathan Parker wrote: > I haven't followed the recent changes to this CL closely, but is there a > solution to Matts' comments here: > > > >> I was thinking about that. An issue is that if a > >> compromised site is corrected and the SB list is > >> updated to remove the site, the bad response could > >> still be cached on the client and SB would then allow it. > > > Great point. So this is a potential problem on mobile. Thanks for raising this again, I completely forgot about it! No, there's no solution for that, currently.
The CQ bit was checked by clamy@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...
Thanks! Concerning the issue with SafeBrowsing, I have a few questions about the cache behavior: - do we keep the cache entry if the result of loading the request is a failure (ie if a ResourceHandler cancels the request before the end)? If so wouldn't that solve the problem, since even if SafeBrowsing cancels the load after the first read we discard the cache entry. There could still be a problem though, in the MimeSniffingResourceHandler we go to pass through mode when we get the ResponseComplete call and return true. We should probably defer ResponseComplete if we are still waiting on downstream handlers, in order to inform the URLRequest that the ResponseCompleteCall went wrong. In that case, would we still keep the cache entry, or would it be possible to discard it? https://codereview.chromium.org/2005273002/diff/340001/content/browser/loader... File content/browser/loader/intercepting_resource_handler.cc (right): https://codereview.chromium.org/2005273002/diff/340001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:118: // the old handler to create a new buffer to store the payload. On 2016/07/19 17:43:45, mmenke wrote: > This seems like a problem. There doesn't seem to be anything to stop the old > next handler from re-using the read_buffer_ between the two calls, since we just > told it we were done with the old buffer. I'm now making a copy of the buffer before sending the payload to the next handler. https://codereview.chromium.org/2005273002/diff/340001/content/browser/loader... File content/browser/loader/intercepting_resource_handler.h (right): https://codereview.chromium.org/2005273002/diff/340001/content/browser/loader... content/browser/loader/intercepting_resource_handler.h:75: scoped_refptr<net::IOBuffer> read_buffer_; On 2016/07/19 21:42:33, mmenke wrote: > Maybe call this first_read_buffer_, and mention that it's only the results of > the first read (from this class's perspective) that may have to be passed to an > alternate ResourceHandler instead of the original handler? Done. https://codereview.chromium.org/2005273002/diff/340001/content/browser/loader... File content/browser/loader/intercepting_resource_handler_unittest.cc (right): https://codereview.chromium.org/2005273002/diff/340001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:93: TEST_F(InterceptingResourceHandlerTest, ResponseBodyHandling) { On 2016/07/19 21:42:33, mmenke wrote: > Seems like we should also test the non-intercept case, unexciting as it is. Done. I only included some of the failure cases, since it turns out the current code ignores the return values of the old ResourceHandler when calling OnresponseStarted and OnReadCompleted. https://codereview.chromium.org/2005273002/diff/340001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:124: char data[] = "The data"; On 2016/07/19 21:42:33, mmenke wrote: > const char kData Done. https://codereview.chromium.org/2005273002/diff/340001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:133: intercepting_handler->UseNewHandler(std::move(new_handler), std::string()); On 2016/07/19 21:42:33, mmenke wrote: > Should check that the old handler gets the abort message. > > Should also add a test where |payload_for_old_handler_| is non-empty, and make > sure the old handler gets the data and the success status. Done. https://codereview.chromium.org/2005273002/diff/340001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:142: EXPECT_FALSE(!memcmp(data, new_test_handler->buffer()->data(), sizeof(data))); On 2016/07/19 21:42:33, mmenke wrote: > Have we put any data in new_test_handler->buffer()->data() yet? I don't see us > populating it with \0's anywhere, though I could be missing something. Done. The buffer is now populated with 0s at creation. https://codereview.chromium.org/2005273002/diff/340001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:148: EXPECT_TRUE(!memcmp(data, new_test_handler->buffer()->data(), sizeof(data))); On 2016/07/19 21:42:33, mmenke wrote: > Seems like we should check a second read, make sure things behave as expected. Done. https://codereview.chromium.org/2005273002/diff/340001/content/browser/loader... File content/browser/loader/mime_sniffing_resource_handler_unittest.cc (right): https://codereview.chromium.org/2005273002/diff/340001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler_unittest.cc:227: void Cancel() override { cancel_called_++; } On 2016/07/19 21:42:33, mmenke wrote: > night: Blank line before Cancel() Done. https://codereview.chromium.org/2005273002/diff/340001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler_unittest.cc:236: int resume_called() const { return resume_called_; } On 2016/07/19 21:42:33, mmenke wrote: > Maybe call these something like times_blah_called(), blah_call_count(), etc, > just to discourage using them with EXPECT_TRUE? Done. https://codereview.chromium.org/2005273002/diff/340001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler_unittest.cc:359: std::unique_ptr<ResourceHandler> intercepting_handler( On 2016/07/19 21:42:33, mmenke wrote: > This can just be a std::unique_ptr<InterceptingResourceHandler>, no? Done. https://codereview.chromium.org/2005273002/diff/340001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler_unittest.cc:380: EXPECT_LT(host.intercepted_as_stream_count(), 2); On 2016/07/19 21:42:33, mmenke wrote: > We don't check that we actually attach the alternate next handler? Done.
On 2016/07/25 16:58:26, clamy wrote: > Thanks! > > Concerning the issue with SafeBrowsing, I have a few questions about the cache > behavior: > - do we keep the cache entry if the result of loading the request is a failure > (ie if a ResourceHandler cancels the request before the end)? If so wouldn't > that solve the problem, since even if SafeBrowsing cancels the load after the > first read we discard the cache entry. > > There could still be a problem though, in the MimeSniffingResourceHandler we go > to pass through mode when we get the ResponseComplete call and return true. We > should probably defer ResponseComplete if we are still waiting on downstream > handlers, in order to inform the URLRequest that the ResponseCompleteCall went > wrong. In that case, would we still keep the cache entry, or would it be > possible to discard it? If we read any of the body from the URLRequest, it will have been written to the cache. If we've read the entire response body, the entire response will have been cached. If we've read just part of the response, I *think* there may be a path where we'll try and use range requests just get what we need when we request the resource again, but I'm not positive of that. Worth noting that even if we could discard the body if the security check fails, there's still the chance the request is cancelled before we hear back from the check. I think this problem is present in the old code, so I don't think it's an issue we should worry about (?). So maybe destroying the cache entry is enough. That having been said, maybe it would be simpler just to put the safe browsing throttle before the MimeSniffer? Not sure if we could just do that with all other Chrome-side throttles as well, or not.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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_...)
Hoping we can finally finish this up (Modulo the safe browsing issue) in just one more pass. Sorry for all my delays on this one. https://codereview.chromium.org/2005273002/diff/380001/content/browser/loader... File content/browser/loader/intercepting_resource_handler.cc (right): https://codereview.chromium.org/2005273002/diff/380001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:61: first_read_buffer_size_); Do our memory tools complain if we end up copying uninitialized memory here? Not that I see an easy way around that without modifying behavior. https://codereview.chromium.org/2005273002/diff/380001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:140: // on the old ResourceHandler. Does this case ever help? first_read_buffer_ was allocated from a previous call to next_handler_->OnWillRead(&buf, &size, -1);. Why would a new call with the exact same arguments allocate enough memory, if the first call did not? https://codereview.chromium.org/2005273002/diff/380001/content/browser/loader... File content/browser/loader/intercepting_resource_handler_unittest.cc (right): https://codereview.chromium.org/2005273002/diff/380001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:76: bool OnReadCompleted(int bytes_read, bool* defer) override { Seems like we should also record and check bytes_read here. https://codereview.chromium.org/2005273002/diff/380001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:97: DISALLOW_COPY_AND_ASSIGN(TestResourceHandler); nit: Blank line before DISALLOW_COPY_AND_ASSIGN. https://codereview.chromium.org/2005273002/diff/380001/content/browser/loader... File content/browser/loader/mime_sniffing_resource_handler_unittest.cc (right): https://codereview.chromium.org/2005273002/diff/380001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler_unittest.cc:41: bool defer_read_completed) Think these names can be clearer. Maybe: response_started_succeeds, defer_on_response_started, will_read_succeeds, read_completed_succeeds, defer_on_read_completed (response_started as a bool could mean whether or not the response is ever started, whether or not the handler should start the response, etc. defer_foo could mean we defer fooing, as opposed to meaning we defer advancing to the next state after fooing) https://codereview.chromium.org/2005273002/diff/380001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler_unittest.cc:97: DISALLOW_COPY_AND_ASSIGN(TestResourceHandler); nit: Blank line before disallow_copy_and_assign https://codereview.chromium.org/2005273002/diff/380001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler_unittest.cc:103: TestResourceDispatcherHostDelegate(bool must_download) explicit https://codereview.chromium.org/2005273002/diff/380001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler_unittest.cc:117: TestResourceDispatcherHost(bool stream_has_handler) explicit https://codereview.chromium.org/2005273002/diff/380001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler_unittest.cc:155: new TestResourceHandler(false, false, true, true, false)); Should merge the above two lines. https://codereview.chromium.org/2005273002/diff/380001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler_unittest.cc:157: static_cast<TestResourceHandler*>(new_resource_handler.get()); Rather than a static_cast, I think it's better to make new_resource_handler a std::unique_ptr<TestResourceHandler>, and return it with std::move(new_resource_handler); (May need to return it with std::move<ResourceHandler>(new_resource_handler), not sure) https://codereview.chromium.org/2005273002/diff/380001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler_unittest.cc:172: TestResourceHandler* last_resource_handler_; Maybe rename this to new_resource_handler_, and make clearer that it's the alternative (swapped in?) ResourceHandler? The RDH normally creates the original ResourceHandler, too, so seems a bit unclear, as-is. https://codereview.chromium.org/2005273002/diff/380001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler_unittest.cc:248: net::URLRequest* request); These need comments (What they do, what the return value is) https://codereview.chromium.org/2005273002/diff/380001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler_unittest.cc:262: ResourceType request_resource_type); Blank line below method. https://codereview.chromium.org/2005273002/diff/380001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler_unittest.cc:544: std::unique_ptr<ResourceHandler> intercepting_handler( Can make this an InterceptingResourceHandler and get rid of the static_cast. https://codereview.chromium.org/2005273002/diff/380001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler_unittest.cc:548: std::unique_ptr<ResourceHandler> handler(new MimeSniffingResourceHandler( Again, can make this a MimeSniffingResourceHandler and get rid of the static cast. In general, I think it's best to avoid static_casts unless we really need them. https://codereview.chromium.org/2005273002/diff/380001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler_unittest.cc:564: response->head.mime_type.assign("text/html"); I think this behavior is worth noting in the comment for this method. https://codereview.chromium.org/2005273002/diff/380001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler_unittest.cc:567: mime_sniffing_handler->state_); Do we really get anything out of testing the internal state? In general, it's best just to test the public APIs. Makes tests less likely to break across refactorings. https://codereview.chromium.org/2005273002/diff/380001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler_unittest.cc:574: CHECK_EQ(0, resource_controller.cancel_call_count()); EXPECT_EQ (Goes for most of this function. Some could be ASSERTS, but EXPECT seems fine for most of them) https://codereview.chromium.org/2005273002/diff/380001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler_unittest.cc:576: if (!response_started) { EXPECT_FALSE(defer)? https://codereview.chromium.org/2005273002/diff/380001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler_unittest.cc:577: content::RunAllPendingInMessageLoop(); Think these calls need comments as to why they're needed. https://codereview.chromium.org/2005273002/diff/380001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler_unittest.cc:603: if (!will_read) { EXPECT_FALSE(defer)? https://codereview.chromium.org/2005273002/diff/380001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler_unittest.cc:734: Why did you remove a test case here? https://codereview.chromium.org/2005273002/diff/380001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler_unittest.cc:762: TestHandlerNoSniffing(true, false, true, true, false); All of these argument need labels. As-is, it's just too hard to tell what these calls are doing. Alternatively, could add enum classes for each argument. Some goes for TestHandlerSniffing.
The CQ bit was checked by clamy@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...
Thanks! The latest patch set addresses the problem by having the SafeBrowsingResourceThrottle declares that it needs to be put before mime handling. We then create two sets of throttles that are put before and after mime handling. This was the simplest way I could see to solve the issue. Note: another option would be to ask the ResourceDispatcherhostDelegate explicitely for 2 sets of ResourceThrottles, but I think this is less constraining for the implementation. https://codereview.chromium.org/2005273002/diff/380001/content/browser/loader... File content/browser/loader/intercepting_resource_handler.cc (right): https://codereview.chromium.org/2005273002/diff/380001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:61: first_read_buffer_size_); On 2016/07/25 17:39:31, mmenke wrote: > Do our memory tools complain if we end up copying uninitialized memory here? > Not that I see an easy way around that without modifying behavior. Asan bot seems alright with it. To avoid that, the only solution would be to keep both handlers around until we get the OnReadCompleted where we know the data size. I'm not sure this is preferable. https://codereview.chromium.org/2005273002/diff/380001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:140: // on the old ResourceHandler. On 2016/07/25 17:39:31, mmenke wrote: > Does this case ever help? first_read_buffer_ was allocated from a previous call > to next_handler_->OnWillRead(&buf, &size, -1);. Why would a new call with the > exact same arguments allocate enough memory, if the first call did not? Good point. Removed this case. https://codereview.chromium.org/2005273002/diff/380001/content/browser/loader... File content/browser/loader/intercepting_resource_handler_unittest.cc (right): https://codereview.chromium.org/2005273002/diff/380001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:76: bool OnReadCompleted(int bytes_read, bool* defer) override { On 2016/07/25 17:39:31, mmenke wrote: > Seems like we should also record and check bytes_read here. Done. https://codereview.chromium.org/2005273002/diff/380001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:97: DISALLOW_COPY_AND_ASSIGN(TestResourceHandler); On 2016/07/25 17:39:31, mmenke wrote: > nit: Blank line before DISALLOW_COPY_AND_ASSIGN. Done. https://codereview.chromium.org/2005273002/diff/380001/content/browser/loader... File content/browser/loader/mime_sniffing_resource_handler_unittest.cc (right): https://codereview.chromium.org/2005273002/diff/380001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler_unittest.cc:41: bool defer_read_completed) On 2016/07/25 17:39:32, mmenke wrote: > Think these names can be clearer. Maybe: > > response_started_succeeds, > defer_on_response_started, > will_read_succeeds, > read_completed_succeeds, > defer_on_read_completed > > (response_started as a bool could mean whether or not the response is ever > started, whether or not the handler should start the response, etc. defer_foo > could mean we defer fooing, as opposed to meaning we defer advancing to the next > state after fooing) Done. https://codereview.chromium.org/2005273002/diff/380001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler_unittest.cc:97: DISALLOW_COPY_AND_ASSIGN(TestResourceHandler); On 2016/07/25 17:39:32, mmenke wrote: > nit: Blank line before disallow_copy_and_assign Done. https://codereview.chromium.org/2005273002/diff/380001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler_unittest.cc:103: TestResourceDispatcherHostDelegate(bool must_download) On 2016/07/25 17:39:31, mmenke wrote: > explicit Done. https://codereview.chromium.org/2005273002/diff/380001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler_unittest.cc:117: TestResourceDispatcherHost(bool stream_has_handler) On 2016/07/25 17:39:31, mmenke wrote: > explicit Done. https://codereview.chromium.org/2005273002/diff/380001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler_unittest.cc:155: new TestResourceHandler(false, false, true, true, false)); On 2016/07/25 17:39:32, mmenke wrote: > Should merge the above two lines. Done. https://codereview.chromium.org/2005273002/diff/380001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler_unittest.cc:157: static_cast<TestResourceHandler*>(new_resource_handler.get()); On 2016/07/25 17:39:32, mmenke wrote: > Rather than a static_cast, I think it's better to make new_resource_handler a > std::unique_ptr<TestResourceHandler>, and return it with > std::move(new_resource_handler); (May need to return it with > std::move<ResourceHandler>(new_resource_handler), not sure) Done. https://codereview.chromium.org/2005273002/diff/380001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler_unittest.cc:172: TestResourceHandler* last_resource_handler_; On 2016/07/25 17:39:31, mmenke wrote: > Maybe rename this to new_resource_handler_, and make clearer that it's the > alternative (swapped in?) ResourceHandler? The RDH normally creates the > original ResourceHandler, too, so seems a bit unclear, as-is. Done. https://codereview.chromium.org/2005273002/diff/380001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler_unittest.cc:248: net::URLRequest* request); On 2016/07/25 17:39:31, mmenke wrote: > These need comments (What they do, what the return value is) Done. https://codereview.chromium.org/2005273002/diff/380001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler_unittest.cc:262: ResourceType request_resource_type); On 2016/07/25 17:39:31, mmenke wrote: > Blank line below method. Done. https://codereview.chromium.org/2005273002/diff/380001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler_unittest.cc:544: std::unique_ptr<ResourceHandler> intercepting_handler( On 2016/07/25 17:39:32, mmenke wrote: > Can make this an InterceptingResourceHandler and get rid of the static_cast. Done. https://codereview.chromium.org/2005273002/diff/380001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler_unittest.cc:548: std::unique_ptr<ResourceHandler> handler(new MimeSniffingResourceHandler( On 2016/07/25 17:39:32, mmenke wrote: > Again, can make this a MimeSniffingResourceHandler and get rid of the static > cast. In general, I think it's best to avoid static_casts unless we really need > them. Yes. I'm not sure why this was a std::unique_ptr<ResourceHandler> (possibly a leftover from some version of the test where I needed it as a std::unique_ptr<ResourceHandler> to pass it to something). https://codereview.chromium.org/2005273002/diff/380001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler_unittest.cc:564: response->head.mime_type.assign("text/html"); On 2016/07/25 17:39:32, mmenke wrote: > I think this behavior is worth noting in the comment for this method. Done. https://codereview.chromium.org/2005273002/diff/380001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler_unittest.cc:567: mime_sniffing_handler->state_); On 2016/07/25 17:39:32, mmenke wrote: > Do we really get anything out of testing the internal state? In general, it's > best just to test the public APIs. Makes tests less likely to break across > refactorings. Done. https://codereview.chromium.org/2005273002/diff/380001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler_unittest.cc:574: CHECK_EQ(0, resource_controller.cancel_call_count()); On 2016/07/25 17:39:32, mmenke wrote: > EXPECT_EQ (Goes for most of this function. Some could be ASSERTS, but EXPECT > seems fine for most of them) Done. I had put CHECKs since I remembered there were some issues with EXPECTs in functions called by tests. Since these functions are actually part of the testing class that seems to work. https://codereview.chromium.org/2005273002/diff/380001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler_unittest.cc:576: if (!response_started) { On 2016/07/25 17:39:32, mmenke wrote: > EXPECT_FALSE(defer)? Done. https://codereview.chromium.org/2005273002/diff/380001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler_unittest.cc:577: content::RunAllPendingInMessageLoop(); On 2016/07/25 17:39:31, mmenke wrote: > Think these calls need comments as to why they're needed. Done. https://codereview.chromium.org/2005273002/diff/380001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler_unittest.cc:603: if (!will_read) { On 2016/07/25 17:39:32, mmenke wrote: > EXPECT_FALSE(defer)? We don't use defer in OnWillRead, so I added it to if (!read_completed) which uses it. https://codereview.chromium.org/2005273002/diff/380001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler_unittest.cc:734: On 2016/07/25 17:39:32, mmenke wrote: > Why did you remove a test case here? That wasn't intentional. It's now back. https://codereview.chromium.org/2005273002/diff/380001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler_unittest.cc:762: TestHandlerNoSniffing(true, false, true, true, false); On 2016/07/25 17:39:32, mmenke wrote: > All of these argument need labels. As-is, it's just too hard to tell what these > calls are doing. Alternatively, could add enum classes for each argument. Some > goes for TestHandlerSniffing. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by clamy@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: Try jobs failed on following builders: mac_chromium_gyp_rel on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gyp_...)
Ok, I'm happy with this. Not going to do another round of review from the original, just going to look at changes, from here on out. At long last, sorry for the delays on my part. https://codereview.chromium.org/2005273002/diff/420001/content/browser/loader... File content/browser/loader/intercepting_resource_handler.h (right): https://codereview.chromium.org/2005273002/diff/420001/content/browser/loader... content/browser/loader/intercepting_resource_handler.h:36: const std::string& payload_for_old_handler); Think this should be documented. Something like: Replaces the next handler with |new_handler|, sending |payload_for_old_handler| to the old handler. Must be called after OnWillStart and OnRequestRedirected and before OnResponseStarted. One OnWillRead call is permitted beforehand. |new_handler|'s OnWillStart and OnRequestRedirected methods will not be called. https://codereview.chromium.org/2005273002/diff/420001/content/browser/loader... content/browser/loader/intercepting_resource_handler.h:65: // ResourceHandler implementation: These should really be public, since they are in ResourceHandler as well. https://codereview.chromium.org/2005273002/diff/420001/content/browser/loader... File content/browser/loader/intercepting_resource_handler_unittest.cc (right): https://codereview.chromium.org/2005273002/diff/420001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:86: *request_status_ = status; Should we set an is_completed_ bool here to true, and EXPECT that it's false at the start of all these override methods? https://codereview.chromium.org/2005273002/diff/420001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:147: handler->OnWillRead(&read_buffer, &buf_size, -1); Seems like we should always call OnWillStart first, just to more accurately mimic what happens in production. It's less important in this test than others, but seems less regression prone. https://codereview.chromium.org/2005273002/diff/420001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:168: EXPECT_EQ(net::URLRequestStatus::CANCELED, old_handler_status.status()); EXPECT_FALSE(old_handler_status.is_success()); This avoids depending on the CANCELED/FAILED distinction. I eventually want to get rid of CANCELED. https://codereview.chromium.org/2005273002/diff/420001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:169: EXPECT_EQ(net::ERR_ABORTED, old_handler_status.error()); Should check the old handler hasn't received anything yet. https://codereview.chromium.org/2005273002/diff/420001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:181: EXPECT_EQ(sizeof(kData), new_test_handler->bytes_read()); This should probably go before comparing memory, and be an ASSERT_EQ. Same goes for the other instances of this pattern in this file. https://codereview.chromium.org/2005273002/diff/420001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:202: // ResourceHandler and the payload to the old ResourceHandler. nit: Maybe the payload -> "a payload", "the specified payload" or even just "|payload|". "the payload" alone seems a bit ambiguous. https://codereview.chromium.org/2005273002/diff/420001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:227: static_cast<InterceptingResourceHandler*>(handler.get()); Please try and remove all static_casts in this file. https://codereview.chromium.org/2005273002/diff/420001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:251: EXPECT_FALSE(!memcmp(kPayload, old_buffer->data(), sizeof(kPayload))); Seems safer the ASSERT bytes_read is 0 instead. https://codereview.chromium.org/2005273002/diff/420001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:260: EXPECT_TRUE(!memcmp(kPayload, old_buffer->data(), sizeof(kPayload))); Again, should ASSERT on size first. Alternatively, you could make TestResourceHandler::OnReadCompleted append the data that was read to a std::string, and just compare the string, instead of comparing bytes read + a memcmp. https://codereview.chromium.org/2005273002/diff/420001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:262: EXPECT_EQ(net::URLRequestStatus::SUCCESS, old_handler_status.status()); EXPECT_TRUE(old_handler_status.status().is_success()); is preferred. https://codereview.chromium.org/2005273002/diff/420001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:335: // The response is received. The new handler should not change. There is no new handler in this test, is there? https://codereview.chromium.org/2005273002/diff/420001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:385: new TestResourceHandler(&old_handler_status, true, false, true)); One all new TestResourceHandler calls, the bools should be labelled. https://codereview.chromium.org/2005273002/diff/420001/content/browser/loader... File content/browser/loader/mime_sniffing_resource_handler_unittest.cc (right): https://codereview.chromium.org/2005273002/diff/420001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler_unittest.cc:413: read_completed, defer_read_completed)), In this test and the next, should we check that the TestResourceHandler has seen all of the expected calls? (We see them, and we see them only once, in the right order, and we see the final read size?) The fact that we see the cancel/defer/resumes as expected is a pretty strong indicator we're calling the expected methods at least once, but may be nice to be a bit more thorough on that. https://codereview.chromium.org/2005273002/diff/420001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler_unittest.cc:423: // Simulate the response starting. We should start buffering, so the return nit: Avoid "we" in comments https://codereview.chromium.org/2005273002/diff/420001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler_unittest.cc:436: mime_sniffing_handler->OnWillRead(&read_buffer, &buf_size, -1)); Per comment in the other file, should probably start with WillStartRequest, just to better protect against changes there (Though they aren't too likely) https://codereview.chromium.org/2005273002/diff/420001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler_unittest.cc:440: EXPECT_FALSE(defer); Oops - WillRead doesn't take defer as an argument, and we already check it above, for the OnResponseStarted call. https://codereview.chromium.org/2005273002/diff/420001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler_unittest.cc:456: // If the next handler cancels the response start, we hsould be notified nit: hsould -> should https://codereview.chromium.org/2005273002/diff/420001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler_unittest.cc:456: // If the next handler cancels the response start, we hsould be notified nit: Avoid "we" in comments https://codereview.chromium.org/2005273002/diff/420001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler_unittest.cc:512: EXPECT_EQ(0, resource_controller.resume_call_count()); nit: Use braces https://codereview.chromium.org/2005273002/diff/420001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler_unittest.cc:937: EXPECT_TRUE(!new_handler); EXPECT_FALSE(new_handler)? Could also get rid of the new_handler local. https://codereview.chromium.org/2005273002/diff/420001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2005273002/diff/420001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1706: post_mime_sniffing_throttles.push_back(throttle); nit: Use braces with else-if. https://codereview.chromium.org/2005273002/diff/420001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/2005273002/diff/420001/content/browser/loader... content/browser/loader/resource_dispatcher_host_unittest.cc:2505: base::MessageLoop::current()->RunUntilIdle(); This is deprecated - use base::RunLoop().RunUntilIdle(). https://codereview.chromium.org/2005273002/diff/420001/content/public/browser... File content/public/browser/resource_throttle.h (right): https://codereview.chromium.org/2005273002/diff/420001/content/public/browser... content/public/browser/resource_throttle.h:51: virtual bool MustProcessResponseBeforeMimeSniffing(); Could name this be more general? MustProcessResponseBeforeReadingBody? I guess you're not just moving all embedded-supplied throttles before the the mime sniffer handler to maintain order as closely as possible? Maybe we should just move all throttles before the mime sniffer, and then add the x-frame-options one after it, when it's added? That does an even better job of maintaining throttle order.
https://codereview.chromium.org/2005273002/diff/420001/content/browser/loader... File content/browser/loader/mime_sniffing_resource_handler.cc (right): https://codereview.chromium.org/2005273002/diff/420001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler.cc:297: if (!CheckForInterception(defer)) And a question you missed: On 2016/07/19 17:43:45, mmenke wrote: > On 2016/07/19 14:24:55, clamy wrote: > > On 2016/07/18 17:58:37, mmenke wrote: > > > I'm not a big fan of having this dig into intercepting_handler. > > > > > > It looks like all we need from this class that we weren't getting is the > > > is_download bit? Given the "DCHECK(!info->allow_download());" in the plugin > > > path, I think we can just have > > MimeSniffingResourceHandler::CheckForInterception > > > set the is_download bit, and not set up the next handler, and then have the > > > intercepting_handler use that bit itself, as well as checking for a plugin? > > > > > > Not great, but I think it's better than this. > > > > We also need the info->is_stream bit that is set in > > ResourceDispatcherHostImpl::MaybeInterceptAsStream which creates the > > ResourceHandler for the stream. So we need to call it as well, and I'm not > sure > > if we can call it twice. > > > > Ideally, we would modify the public API so we can have a call to teh embedder > to > > ask if this is a download or a stream, but which doesn't set up the new > > ResourceHandler. Then the intercepting_handler would make a call to a > different > > function to actually get the new ResourceHandler. But the patch is already big > > enough (and I'm not very familiar with the chromium code) that I did not want > to > > include this in the patch. > > I'm not seeing where is_stream is used in this file. Do you mean you need the > is_stream bit for the X-Frame-Options logic, and streams are exempt from > X-Frame-Options? > > I certainly agree that we don't want to make this patch any bigger than it > already is.
The CQ bit was checked by clamy@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...
Thanks! https://codereview.chromium.org/2005273002/diff/420001/content/browser/loader... File content/browser/loader/intercepting_resource_handler.h (right): https://codereview.chromium.org/2005273002/diff/420001/content/browser/loader... content/browser/loader/intercepting_resource_handler.h:36: const std::string& payload_for_old_handler); On 2016/07/27 19:33:59, mmenke wrote: > Think this should be documented. > > Something like: Replaces the next handler with |new_handler|, sending > |payload_for_old_handler| to the old handler. Must be called after OnWillStart > and OnRequestRedirected and before OnResponseStarted. One OnWillRead call is > permitted beforehand. |new_handler|'s OnWillStart and OnRequestRedirected > methods will not be called. Done. https://codereview.chromium.org/2005273002/diff/420001/content/browser/loader... content/browser/loader/intercepting_resource_handler.h:65: // ResourceHandler implementation: On 2016/07/27 19:33:59, mmenke wrote: > These should really be public, since they are in ResourceHandler as well. Done. https://codereview.chromium.org/2005273002/diff/420001/content/browser/loader... File content/browser/loader/intercepting_resource_handler_unittest.cc (right): https://codereview.chromium.org/2005273002/diff/420001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:86: *request_status_ = status; On 2016/07/27 19:34:00, mmenke wrote: > Should we set an is_completed_ bool here to true, and EXPECT that it's false at > the start of all these override methods? Done. https://codereview.chromium.org/2005273002/diff/420001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:147: handler->OnWillRead(&read_buffer, &buf_size, -1); On 2016/07/27 19:34:00, mmenke wrote: > Seems like we should always call OnWillStart first, just to more accurately > mimic what happens in production. It's less important in this test than others, > but seems less regression prone. Done. https://codereview.chromium.org/2005273002/diff/420001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:168: EXPECT_EQ(net::URLRequestStatus::CANCELED, old_handler_status.status()); On 2016/07/27 19:33:59, mmenke wrote: > EXPECT_FALSE(old_handler_status.is_success()); > > This avoids depending on the CANCELED/FAILED distinction. I eventually want to > get rid of CANCELED. Done. https://codereview.chromium.org/2005273002/diff/420001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:169: EXPECT_EQ(net::ERR_ABORTED, old_handler_status.error()); On 2016/07/27 19:34:00, mmenke wrote: > Should check the old handler hasn't received anything yet. Done. https://codereview.chromium.org/2005273002/diff/420001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:181: EXPECT_EQ(sizeof(kData), new_test_handler->bytes_read()); On 2016/07/27 19:33:59, mmenke wrote: > This should probably go before comparing memory, and be an ASSERT_EQ. Same goes > for the other instances of this pattern in this file. Done. https://codereview.chromium.org/2005273002/diff/420001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:202: // ResourceHandler and the payload to the old ResourceHandler. On 2016/07/27 19:34:00, mmenke wrote: > nit: Maybe the payload -> "a payload", "the specified payload" or even just > "|payload|". "the payload" alone seems a bit ambiguous. Done. https://codereview.chromium.org/2005273002/diff/420001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:227: static_cast<InterceptingResourceHandler*>(handler.get()); On 2016/07/27 19:33:59, mmenke wrote: > Please try and remove all static_casts in this file. Done. https://codereview.chromium.org/2005273002/diff/420001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:251: EXPECT_FALSE(!memcmp(kPayload, old_buffer->data(), sizeof(kPayload))); On 2016/07/27 19:34:00, mmenke wrote: > Seems safer the ASSERT bytes_read is 0 instead. Done. https://codereview.chromium.org/2005273002/diff/420001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:260: EXPECT_TRUE(!memcmp(kPayload, old_buffer->data(), sizeof(kPayload))); On 2016/07/27 19:34:00, mmenke wrote: > Again, should ASSERT on size first. Alternatively, you could make > TestResourceHandler::OnReadCompleted append the data that was read to a > std::string, and just compare the string, instead of comparing bytes read + a > memcmp. Done. Went with the size ASSERT since the Handler can be deleted in the middle (but we keep a ref on the buffer). https://codereview.chromium.org/2005273002/diff/420001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:262: EXPECT_EQ(net::URLRequestStatus::SUCCESS, old_handler_status.status()); On 2016/07/27 19:34:00, mmenke wrote: > EXPECT_TRUE(old_handler_status.status().is_success()); is preferred. Done. https://codereview.chromium.org/2005273002/diff/420001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:335: // The response is received. The new handler should not change. On 2016/07/27 19:34:00, mmenke wrote: > There is no new handler in this test, is there? Yes. I meant "The handler should not change.". Corrected the comment. https://codereview.chromium.org/2005273002/diff/420001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:385: new TestResourceHandler(&old_handler_status, true, false, true)); On 2016/07/27 19:33:59, mmenke wrote: > One all new TestResourceHandler calls, the bools should be labelled. Done. https://codereview.chromium.org/2005273002/diff/420001/content/browser/loader... File content/browser/loader/mime_sniffing_resource_handler.cc (right): https://codereview.chromium.org/2005273002/diff/420001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler.cc:297: if (!CheckForInterception(defer)) On 2016/07/27 20:16:58, mmenke wrote: > And a question you missed: > > On 2016/07/19 17:43:45, mmenke wrote: > > On 2016/07/19 14:24:55, clamy wrote: > > > On 2016/07/18 17:58:37, mmenke wrote: > > > > I'm not a big fan of having this dig into intercepting_handler. > > > > > > > > It looks like all we need from this class that we weren't getting is the > > > > is_download bit? Given the "DCHECK(!info->allow_download());" in the > plugin > > > > path, I think we can just have > > > MimeSniffingResourceHandler::CheckForInterception > > > > set the is_download bit, and not set up the next handler, and then have > the > > > > intercepting_handler use that bit itself, as well as checking for a > plugin? > > > > > > > > Not great, but I think it's better than this. > > > > > > We also need the info->is_stream bit that is set in > > > ResourceDispatcherHostImpl::MaybeInterceptAsStream which creates the > > > ResourceHandler for the stream. So we need to call it as well, and I'm not > > sure > > > if we can call it twice. > > > > > > Ideally, we would modify the public API so we can have a call to teh > embedder > > to > > > ask if this is a download or a stream, but which doesn't set up the new > > > ResourceHandler. Then the intercepting_handler would make a call to a > > different > > > function to actually get the new ResourceHandler. But the patch is already > big > > > enough (and I'm not very familiar with the chromium code) that I did not > want > > to > > > include this in the patch. > > > > I'm not seeing where is_stream is used in this file. Do you mean you need the > > is_stream bit for the X-Frame-Options logic, and streams are exempt from > > X-Frame-Options? > > > > I certainly agree that we don't want to make this patch any bigger than it > > already is. > Actually we have another follow up patch planned to merge the CrossSiteResourceHandler with the NavigationResourceThrottle. The CrossSiteResourceHandler needs to know whether the navigation is a stream, hence why I need it there. https://codereview.chromium.org/2005273002/diff/420001/content/browser/loader... File content/browser/loader/mime_sniffing_resource_handler_unittest.cc (right): https://codereview.chromium.org/2005273002/diff/420001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler_unittest.cc:413: read_completed, defer_read_completed)), On 2016/07/27 19:34:00, mmenke wrote: > In this test and the next, should we check that the TestResourceHandler has seen > all of the expected calls? (We see them, and we see them only once, in the > right order, and we see the final read size?) > > The fact that we see the cancel/defer/resumes as expected is a pretty strong > indicator we're calling the expected methods at least once, but may be nice to > be a bit more thorough on that. Done. https://codereview.chromium.org/2005273002/diff/420001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler_unittest.cc:423: // Simulate the response starting. We should start buffering, so the return On 2016/07/27 19:34:00, mmenke wrote: > nit: Avoid "we" in comments Done. https://codereview.chromium.org/2005273002/diff/420001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler_unittest.cc:436: mime_sniffing_handler->OnWillRead(&read_buffer, &buf_size, -1)); On 2016/07/27 19:34:01, mmenke wrote: > Per comment in the other file, should probably start with WillStartRequest, just > to better protect against changes there (Though they aren't too likely) Done. https://codereview.chromium.org/2005273002/diff/420001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler_unittest.cc:440: EXPECT_FALSE(defer); On 2016/07/27 19:34:00, mmenke wrote: > Oops - WillRead doesn't take defer as an argument, and we already check it > above, for the OnResponseStarted call. Done. https://codereview.chromium.org/2005273002/diff/420001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler_unittest.cc:456: // If the next handler cancels the response start, we hsould be notified On 2016/07/27 19:34:01, mmenke wrote: > nit: hsould -> should Done. https://codereview.chromium.org/2005273002/diff/420001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler_unittest.cc:512: EXPECT_EQ(0, resource_controller.resume_call_count()); On 2016/07/27 19:34:00, mmenke wrote: > nit: Use braces Done. https://codereview.chromium.org/2005273002/diff/420001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler_unittest.cc:937: EXPECT_TRUE(!new_handler); On 2016/07/27 19:34:00, mmenke wrote: > EXPECT_FALSE(new_handler)? Could also get rid of the new_handler local. Done. https://codereview.chromium.org/2005273002/diff/420001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2005273002/diff/420001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1706: post_mime_sniffing_throttles.push_back(throttle); On 2016/07/27 19:34:01, mmenke wrote: > nit: Use braces with else-if. Done. https://codereview.chromium.org/2005273002/diff/420001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/2005273002/diff/420001/content/browser/loader... content/browser/loader/resource_dispatcher_host_unittest.cc:2505: base::MessageLoop::current()->RunUntilIdle(); On 2016/07/27 19:34:01, mmenke wrote: > This is deprecated - use base::RunLoop().RunUntilIdle(). Done. https://codereview.chromium.org/2005273002/diff/420001/content/public/browser... File content/public/browser/resource_throttle.h (right): https://codereview.chromium.org/2005273002/diff/420001/content/public/browser... content/public/browser/resource_throttle.h:51: virtual bool MustProcessResponseBeforeMimeSniffing(); On 2016/07/27 19:34:01, mmenke wrote: > Could name this be more general? MustProcessResponseBeforeReadingBody? > > I guess you're not just moving all embedded-supplied throttles before the the > mime sniffer handler to maintain order as closely as possible? Maybe we should > just move all throttles before the mime sniffer, and then add the > x-frame-options one after it, when it's added? That does an even better job of > maintaining throttle order. Done. To do what you suggest we have two options: - Move the NavigationResourceThrottle after the mime sniffer -> but this means that NavigationThrottles which execute first now execute last (ex: InterceptNavigationThrottle on Android). - Have two NavigationResourceThrottles: this adds one more thread hop, and also requires a lot of changes since the UI thread classes don't expect to be called twice. All in all, just moving the SafeBrowsingResourceThrottle up has the less disruption of the throttle order. Note that I think this is a somewhat temporary solution. since it would be better enventually to ensure that a request cancelled for security reasons can be removed form cache. This way we don't have to worry about parts of it having been cached.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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 clamy@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/2005273002/diff/420001/content/public/browser... File content/public/browser/resource_throttle.h (right): https://codereview.chromium.org/2005273002/diff/420001/content/public/browser... content/public/browser/resource_throttle.h:51: virtual bool MustProcessResponseBeforeMimeSniffing(); On 2016/08/17 12:47:35, clamy wrote: > On 2016/07/27 19:34:01, mmenke wrote: > > Could name this be more general? MustProcessResponseBeforeReadingBody? > > > > I guess you're not just moving all embedded-supplied throttles before the the > > mime sniffer handler to maintain order as closely as possible? Maybe we > should > > just move all throttles before the mime sniffer, and then add the > > x-frame-options one after it, when it's added? That does an even better job > of > > maintaining throttle order. > > Done. > > To do what you suggest we have two options: > - Move the NavigationResourceThrottle after the mime sniffer -> but this means > that NavigationThrottles which execute first now execute last (ex: > InterceptNavigationThrottle on Android). > - Have two NavigationResourceThrottles: this adds one more thread hop, and also > requires a lot of changes since the UI thread classes don't expect to be called > twice. > > All in all, just moving the SafeBrowsingResourceThrottle up has the less > disruption of the throttle order. Note that I think this is a somewhat temporary > solution. since it would be better enventually to ensure that a request > cancelled for security reasons can be removed form cache. This way we don't have > to worry about parts of it having been cached. I don't think this is going to happen, reliably - if a request is cancelled after it's been cached, but before it reaches the SafeBrowsingResourceThrottle, the bad response will still be cached. I consider safe browsing checks after any part of a response reaches the cache best-effort only. We also don't ensure the URLRequest isn't deleted in the middle of a safe browsing check. Nor do we ensure Chrome doesn't shut down in the meantime, etc. Just nothing about deleting a file in the cache in the safe browsing case can be considered reliable.
https://codereview.chromium.org/2005273002/diff/420001/content/public/browser... File content/public/browser/resource_throttle.h (right): https://codereview.chromium.org/2005273002/diff/420001/content/public/browser... content/public/browser/resource_throttle.h:51: virtual bool MustProcessResponseBeforeMimeSniffing(); On 2016/08/17 19:53:08, mmenke wrote: > On 2016/08/17 12:47:35, clamy wrote: > > On 2016/07/27 19:34:01, mmenke wrote: > > > Could name this be more general? MustProcessResponseBeforeReadingBody? > > > > > > I guess you're not just moving all embedded-supplied throttles before the > the > > > mime sniffer handler to maintain order as closely as possible? Maybe we > > should > > > just move all throttles before the mime sniffer, and then add the > > > x-frame-options one after it, when it's added? That does an even better job > > of > > > maintaining throttle order. > > > > Done. > > > > To do what you suggest we have two options: > > - Move the NavigationResourceThrottle after the mime sniffer -> but this means > > that NavigationThrottles which execute first now execute last (ex: > > InterceptNavigationThrottle on Android). > > - Have two NavigationResourceThrottles: this adds one more thread hop, and > also > > requires a lot of changes since the UI thread classes don't expect to be > called > > twice. > > > > All in all, just moving the SafeBrowsingResourceThrottle up has the less > > disruption of the throttle order. Note that I think this is a somewhat > temporary > > solution. since it would be better enventually to ensure that a request > > cancelled for security reasons can be removed form cache. This way we don't > have > > to worry about parts of it having been cached. > > I don't think this is going to happen, reliably - if a request is cancelled > after it's been cached, but before it reaches the SafeBrowsingResourceThrottle, > the bad response will still be cached. I consider safe browsing checks after > any part of a response reaches the cache best-effort only. We also don't ensure > the URLRequest isn't deleted in the middle of a safe browsing check. Nor do we > ensure Chrome doesn't shut down in the meantime, etc. Just nothing about > deleting a file in the cache in the safe browsing case can be considered > reliable. Acknowledged.
Not a full review (Sorry for all the slowness, 2,000 line CLs are just a pain to review) https://codereview.chromium.org/2005273002/diff/480001/chrome/browser/rendere... File chrome/browser/renderer_host/safe_browsing_resource_throttle.cc (right): https://codereview.chromium.org/2005273002/diff/480001/chrome/browser/rendere... chrome/browser/renderer_host/safe_browsing_resource_throttle.cc:173: return true; Think this needs an explanatory comment. https://codereview.chromium.org/2005273002/diff/480001/chrome/browser/rendere... chrome/browser/renderer_host/safe_browsing_resource_throttle.cc:173: return true; Also, if we have any SafeBrowsing integration tests for that pause requests in WillProcessResponse, and then cancel them, we should have an integration test where we go through that path for a cacheable resource, then change the safebrowsing result to not block the resource, turn off the test server, and re-send the request, making sure we don't get the cached result. If we *don't* have any integration tests for that SafeBrowsing path, then we really should, but adding them is well beyond the scope of this CL, and this doesn't make test coverage any worse. https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... File content/browser/loader/intercepting_resource_handler.cc (right): https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:49: // ResourceHandler. I think it's worth noting the weird magic here. "Despite not having been informed of any data being stored in first_read_buffer_, the MimeSniffingResourceHandler has read the data, it's just holding it back"...Hrm....That seems too weird. Could this copying be done by the MimeSniffingResourceHandler, before it calls into UseNewHandler? It even knows the actual amount of the buffer that's been populated. You'd need to make it call OnWillRead after calling UseNewHandler, but I don't think that's a problem? It also maintains the contract that OnReadCompleted will be called when the last data returned by OnWillRead will have been populated. This current approach breaks the contract, for both this class, and the ThrottlingResourceHandler, which seems really weird. Sorry for all the big[ish] suggestions, just suspect we'll be stuck with any ugliness this CL adds for quite a while. https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:61: // the original ResourceHandler. "This" seems unclear. Maybe "The original ResourceHandler is now no longer needed, so replace it with the new one, before sending the response to the new one."? https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... File content/browser/loader/intercepting_resource_handler.h (right): https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... content/browser/loader/intercepting_resource_handler.h:30: // If ENABLE_PLUGINS is defined, |plugin_service| must not be NULL. No longer accurate https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... content/browser/loader/intercepting_resource_handler.h:31: InterceptingResourceHandler(std::unique_ptr<ResourceHandler> next_handler, Should probably include ResourceHandler header. https://codereview.chromium.org/2005273002/diff/480001/content/public/browser... File content/public/browser/resource_throttle.h (right): https://codereview.chromium.org/2005273002/diff/480001/content/public/browser... content/public/browser/resource_throttle.h:51: virtual bool MustProcessResponseBeforeReadingBody(); Think we need test coverage of this method returning true - Can either cancel in WillProcessResponse and make sure we never read the body, or make a cacheable response, cancel there and make sure a second request doesn't use the cache. https://codereview.chromium.org/2005273002/diff/480001/content/public/browser... content/public/browser/resource_throttle.h:51: virtual bool MustProcessResponseBeforeReadingBody(); "no part of the response" -> "no part of the response body". I suspect we won't cache the headers either, but I'm not sure of that (And I don't think this code should implicitly make promises about caching stuff once we've pulled it from net)
https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... File content/browser/loader/intercepting_resource_handler.cc (right): https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:7: #include <utility> What's this needed for (Sorry if I already asked this - doesn't look like I did, though)? https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:100: CHECK((buf_len >= bytes_read) && (bytes_read >= 0)); Split this in two - makes it clearer which one failed. https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:101: CHECK_GE(first_read_buffer_size_, static_cast<size_t>(bytes_read)); CHECK -> DCHECK (x2) https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:106: return next_handler_->OnReadCompleted(bytes_read, defer); No unit tests make sure we pass through defer here...I think we're probably safe ignoring that case? https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:144: CHECK_GE(size, static_cast<int>(payload_for_old_handler_.length())); CHECK->DCHECK (x2) https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:147: next_handler_->OnReadCompleted(payload_for_old_handler_.length(), I assume payload_for_old_handler_ is short enough that keeping it around isn't a problem? (Worth noting that payload_for_old_handler_.clear() wouldn't actually free the memory it uses) https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... File content/browser/loader/intercepting_resource_handler.h (right): https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... content/browser/loader/intercepting_resource_handler.h:88: scoped_refptr<net::IOBuffer> first_read_buffer_copy_; Should probably include ref_counted.h https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... File content/browser/loader/intercepting_resource_handler_unittest.cc (right): https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:36: size_t* final_bytes_read) Think these should be documented. https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:47: bool on_read_completed) Maybe add _result to the last 3 values (And update comments labeling them around callsites)? Or document them, just want it to be clearer they're the return values of the corresponding functions. Or could make an enum FailureMode { FAIL_ON_RESPONSE_STARTED, FAIL_ON_WILL_READ, ... }. I'm happy with any option. https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:165: intercepting_handler->OnWillStart(GURL(), &defer); EXPECT_TRUE(intercepting_handler->OnWillStart(GURL(), &defer));? EXPECT_FALSE(&defer);? https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:166: intercepting_handler->OnWillRead(&read_buffer, &buf_size, -1); EXPECT_TRUE(intercepting_handler->OnWillRead(&read_buffer, &buf_size, -1)); Not going to go through and annotate all of these, but think we should check all return/buffer values (Not because defer isn't allowed to be set to true here, but the test fixture doesn't support it) https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:169: CHECK_EQ(read_buffer.get(), old_buffer.get()); ASSERT_EQ? (Same goes for other lines like this, further down) https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:169: CHECK_EQ(read_buffer.get(), old_buffer.get()); check buf_size as well? https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:194: !memcmp(kData, new_test_handler->buffer()->data(), sizeof(kData))); Check that the new handler hasn't read any bytes yet, too? https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:198: intercepting_handler->OnReadCompleted(sizeof(kData), &defer); EXPECT_TRUE https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:202: !memcmp(kData, new_test_handler->buffer()->data(), sizeof(kData))); Suggest "EXPECT_EQ(kData, std::string(new_test_handler->buffer()->data(), new_test_handler->bytes_read()));" That results in more useful output on failure. Or better, you could make new_test_handler have a method to return std:string(new_test_handler->buffer()->data(), new_test_handler->bytes_read()), and just use that. You could then also remove the "EXPECT_EQ(sizeof(kData), new_test_handler->bytes_read());" lines, since the comparisons now implicitly include that information. https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:204: // Make sure another read behave as expected. nit: behaves. https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:205: memset(new_test_handler->buffer()->data(), '\0', 2048); Should this be done in TestHandler::OnWillRead instead? https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:208: CHECK_EQ(read_buffer.get(), new_test_handler->buffer()); ASSERT_EQ https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:323: TEST_F(InterceptingResourceHandlerTest, NoSwitching) { nit: Suggest this test first, as the base / simple case. https://codereview.chromium.org/2005273002/diff/480001/content/public/browser... File content/public/browser/resource_throttle.h (right): https://codereview.chromium.org/2005273002/diff/480001/content/public/browser... content/public/browser/resource_throttle.h:51: virtual bool MustProcessResponseBeforeReadingBody(); Looks through all other throttles that implement WillProcessresponse, the only other change with this approach instead of moving all throttles but the navigation one before the mime sniffer looks to be that PowerSaveBlockResourceThrottle will block entering power save mode for longer (Normally blocks from upload start to headers received, now will wait until after mime type is sniffed). That doesn't exactly fill me with dread, though I'm still concerned we could be missing something. https://codereview.chromium.org/2005273002/diff/480001/content/public/browser... content/public/browser/resource_throttle.h:51: virtual bool MustProcessResponseBeforeReadingBody(); +const
No need to rush out a response - I'm planning on doing a pass on the sniffing half of the CL + its tests tomorrow.
On 2016/08/22 20:40:34, mmenke wrote: > No need to rush out a response - I'm planning on doing a pass on the sniffing > half of the CL + its tests tomorrow. Sorry, we now have a dev-blocker browser crasher in this area (https://crbug.com/640545), which needs to be fixed before major changes there (And since it's unclear where the issue is, that probably means I'll be the one investigating, which I'm going to prioritize above this review, given the looming M54 deadline, and the issue needs to be resolved before landing this, anyways.
Think it's finally about time to land this. https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... File content/browser/loader/mime_sniffing_resource_handler.cc (right): https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler.cc:272: bool MimeSniffingResourceHandler::ProcessReplay(bool* defer) { The "STATE_BUFFERING" case doesn't seem to have anything to do with processing a replay. Maybe just ProcessState (And ProceedWithReplay -> ProceedWithState? AdvanceState? ExecuteState?) https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler.cc:293: bool MimeSniffingResourceHandler::MaybeCheckForInterception(bool* defer) { This call always checks for Interception. MaybeIntercept? https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler.cc:322: read_buffer_ = NULL; nit: nullptr https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler.cc:413: intercepting_handler_->UseNewHandler(std::move(handler), std::string()); This seems like more than "checking for interception". MaybeStartInterception? https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... File content/browser/loader/mime_sniffing_resource_handler.h (right): https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler.h:98: void ProceedWithReplay(); Mind adding a comment that these return false if the request should be cancelled, true otherwise, and set defer to true if the request will advance to the next state asynchronously? https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler.h:101: bool GetFinalMimeType(bool* defer); Need to document these (At least the new ones) https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler.h:107: bool DetermineMimeType(); DetermineMimeType was removed. https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler.h:110: // download. Does more than just check, actually sets up the new handler. See also suggestion about renaming. https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler.h:111: bool CheckForInterception(bool* defer); Should document return value / defer https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler.h:113: // Determines whether a plugin will handle the current reques. Returns false nit: request
The CQ bit was checked by clamy@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...
Thanks! https://codereview.chromium.org/2005273002/diff/480001/chrome/browser/rendere... File chrome/browser/renderer_host/safe_browsing_resource_throttle.cc (right): https://codereview.chromium.org/2005273002/diff/480001/chrome/browser/rendere... chrome/browser/renderer_host/safe_browsing_resource_throttle.cc:173: return true; On 2016/08/19 16:33:02, mmenke wrote: > Think this needs an explanatory comment. Added a comment. https://codereview.chromium.org/2005273002/diff/480001/chrome/browser/rendere... chrome/browser/renderer_host/safe_browsing_resource_throttle.cc:173: return true; On 2016/08/19 16:33:02, mmenke wrote: > Also, if we have any SafeBrowsing integration tests for that pause requests in > WillProcessResponse, and then cancel them, we should have an integration test > where we go through that path for a cacheable resource, then change the > safebrowsing result to not block the resource, turn off the test server, and > re-send the request, making sure we don't get the cached result. > > If we *don't* have any integration tests for that SafeBrowsing path, then we > really should, but adding them is well beyond the scope of this CL, and this > doesn't make test coverage any worse. Considering that we now have a unit test to check for MustProcessResponseBeforeReadingBody and that this CL is big enough, I have marked adding a browser test as a TODO to do in a followup patch. https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... File content/browser/loader/intercepting_resource_handler.cc (right): https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:7: #include <utility> On 2016/08/22 20:39:03, mmenke wrote: > What's this needed for (Sorry if I already asked this - doesn't look like I did, > though)? It was no longer needed. Removed it. https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:49: // ResourceHandler. On 2016/08/19 16:33:02, mmenke wrote: > I think it's worth noting the weird magic here. "Despite not having been > informed of any data being stored in first_read_buffer_, the > MimeSniffingResourceHandler has read the data, it's just holding it > back"...Hrm....That seems too weird. Could this copying be done by the > MimeSniffingResourceHandler, before it calls into UseNewHandler? It even knows > the actual amount of the buffer that's been populated. > > You'd need to make it call OnWillRead after calling UseNewHandler, but I don't > think that's a problem? It also maintains the contract that OnReadCompleted > will be called when the last data returned by OnWillRead will have been > populated. This current approach breaks the contract, for both this class, and > the ThrottlingResourceHandler, which seems really weird. > > Sorry for all the big[ish] suggestions, just suspect we'll be stuck with any > ugliness this CL adds for quite a while. I've updated the comment and added a TODO to check if this should be moved to the MimeSniffingResourceHandler. Considering that this patch passes all tests and has testing coverage for this behavior, I'd like to land it ASAP to have it spend some time on trunk before branch point. Moving the copy to the MimeSniffingResourceHandler is non trivial, especially when it comes to the unit tests. I'd rather do it as a follow-up CL, so that I can unblock other CLs waiting on that one. https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:61: // the original ResourceHandler. On 2016/08/19 16:33:02, mmenke wrote: > "This" seems unclear. Maybe "The original ResourceHandler is now no longer > needed, so replace it with the new one, before sending the response to the new > one."? Done. https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:100: CHECK((buf_len >= bytes_read) && (bytes_read >= 0)); On 2016/08/22 20:39:04, mmenke wrote: > Split this in two - makes it clearer which one failed. Done. https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:101: CHECK_GE(first_read_buffer_size_, static_cast<size_t>(bytes_read)); On 2016/08/22 20:39:04, mmenke wrote: > CHECK -> DCHECK (x2) Considering we're about to write memory in the memcpy below, it seems safer from a security POV to keep a CHECK here in order to crash rather than risk a buffer overflow. Wdyt? https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:106: return next_handler_->OnReadCompleted(bytes_read, defer); On 2016/08/22 20:39:03, mmenke wrote: > No unit tests make sure we pass through defer here...I think we're probably safe > ignoring that case? Added a TODO. https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:144: CHECK_GE(size, static_cast<int>(payload_for_old_handler_.length())); On 2016/08/22 20:39:04, mmenke wrote: > CHECK->DCHECK (x2) Same comment as before. I left these as CHECKs because it checks for the validity of the memcpy. https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:147: next_handler_->OnReadCompleted(payload_for_old_handler_.length(), On 2016/08/22 20:39:03, mmenke wrote: > I assume payload_for_old_handler_ is short enough that keeping it around isn't a > problem? (Worth noting that payload_for_old_handler_.clear() wouldn't actually > free the memory it uses) I now reset it to an empty string. https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... File content/browser/loader/intercepting_resource_handler.h (right): https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... content/browser/loader/intercepting_resource_handler.h:30: // If ENABLE_PLUGINS is defined, |plugin_service| must not be NULL. On 2016/08/19 16:33:02, mmenke wrote: > No longer accurate Done. https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... content/browser/loader/intercepting_resource_handler.h:31: InterceptingResourceHandler(std::unique_ptr<ResourceHandler> next_handler, On 2016/08/19 16:33:02, mmenke wrote: > Should probably include ResourceHandler header. Done. https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... content/browser/loader/intercepting_resource_handler.h:88: scoped_refptr<net::IOBuffer> first_read_buffer_copy_; On 2016/08/22 20:39:04, mmenke wrote: > Should probably include ref_counted.h Already included. https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... File content/browser/loader/intercepting_resource_handler_unittest.cc (right): https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:36: size_t* final_bytes_read) On 2016/08/22 20:39:04, mmenke wrote: > Think these should be documented. Done. https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:47: bool on_read_completed) On 2016/08/22 20:39:04, mmenke wrote: > Maybe add _result to the last 3 values (And update comments labeling them around > callsites)? Or document them, just want it to be clearer they're the return > values of the corresponding functions. Or could make an enum FailureMode { > FAIL_ON_RESPONSE_STARTED, FAIL_ON_WILL_READ, ... }. I'm happy with any option. Done. https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:165: intercepting_handler->OnWillStart(GURL(), &defer); On 2016/08/22 20:39:04, mmenke wrote: > EXPECT_TRUE(intercepting_handler->OnWillStart(GURL(), &defer));? > EXPECT_FALSE(&defer);? Done. https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:166: intercepting_handler->OnWillRead(&read_buffer, &buf_size, -1); On 2016/08/22 20:39:04, mmenke wrote: > EXPECT_TRUE(intercepting_handler->OnWillRead(&read_buffer, &buf_size, -1)); > > Not going to go through and annotate all of these, but think we should check all > return/buffer values (Not because defer isn't allowed to be set to true here, > but the test fixture doesn't support it) Done. https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:169: CHECK_EQ(read_buffer.get(), old_buffer.get()); On 2016/08/22 20:39:04, mmenke wrote: > check buf_size as well? Done. https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:194: !memcmp(kData, new_test_handler->buffer()->data(), sizeof(kData))); On 2016/08/22 20:39:04, mmenke wrote: > Check that the new handler hasn't read any bytes yet, too? Done. https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:198: intercepting_handler->OnReadCompleted(sizeof(kData), &defer); On 2016/08/22 20:39:04, mmenke wrote: > EXPECT_TRUE Done. https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:202: !memcmp(kData, new_test_handler->buffer()->data(), sizeof(kData))); On 2016/08/22 20:39:04, mmenke wrote: > Suggest "EXPECT_EQ(kData, std::string(new_test_handler->buffer()->data(), > new_test_handler->bytes_read()));" That results in more useful output on > failure. > > Or better, you could make new_test_handler have a method to return > std:string(new_test_handler->buffer()->data(), new_test_handler->bytes_read()), > and just use that. > > You could then also remove the "EXPECT_EQ(sizeof(kData), > new_test_handler->bytes_read());" lines, since the comparisons now implicitly > include that information. This method doesn't work if we do more than one read, since bytes_read is the total amount of bytes read. This happens below. https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:204: // Make sure another read behave as expected. On 2016/08/22 20:39:04, mmenke wrote: > nit: behaves. Done. https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:205: memset(new_test_handler->buffer()->data(), '\0', 2048); On 2016/08/22 20:39:04, mmenke wrote: > Should this be done in TestHandler::OnWillRead instead? Done. https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:208: CHECK_EQ(read_buffer.get(), new_test_handler->buffer()); On 2016/08/22 20:39:04, mmenke wrote: > ASSERT_EQ Done. https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:323: TEST_F(InterceptingResourceHandlerTest, NoSwitching) { On 2016/08/22 20:39:04, mmenke wrote: > nit: Suggest this test first, as the base / simple case. Done. https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... File content/browser/loader/mime_sniffing_resource_handler.cc (right): https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler.cc:272: bool MimeSniffingResourceHandler::ProcessReplay(bool* defer) { On 2016/08/26 21:03:04, mmenke wrote: > The "STATE_BUFFERING" case doesn't seem to have anything to do with processing a > replay. Maybe just ProcessState (And ProceedWithReplay -> ProceedWithState? > AdvanceState? ExecuteState?) Done. https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler.cc:293: bool MimeSniffingResourceHandler::MaybeCheckForInterception(bool* defer) { On 2016/08/26 21:03:04, mmenke wrote: > This call always checks for Interception. MaybeIntercept? Done. https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler.cc:322: read_buffer_ = NULL; On 2016/08/26 21:03:04, mmenke wrote: > nit: nullptr Done. https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler.cc:413: intercepting_handler_->UseNewHandler(std::move(handler), std::string()); On 2016/08/26 21:03:04, mmenke wrote: > This seems like more than "checking for interception". MaybeStartInterception? Done. https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... File content/browser/loader/mime_sniffing_resource_handler.h (right): https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler.h:98: void ProceedWithReplay(); On 2016/08/26 21:03:04, mmenke wrote: > Mind adding a comment that these return false if the request should be > cancelled, true otherwise, and set defer to true if the request will advance to > the next state asynchronously? Done. https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler.h:101: bool GetFinalMimeType(bool* defer); On 2016/08/26 21:03:04, mmenke wrote: > Need to document these (At least the new ones) Done. https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler.h:107: bool DetermineMimeType(); On 2016/08/26 21:03:04, mmenke wrote: > DetermineMimeType was removed. Done. https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler.h:110: // download. On 2016/08/26 21:03:04, mmenke wrote: > Does more than just check, actually sets up the new handler. See also > suggestion about renaming. Done. https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler.h:111: bool CheckForInterception(bool* defer); On 2016/08/26 21:03:04, mmenke wrote: > Should document return value / defer Done. https://codereview.chromium.org/2005273002/diff/480001/content/browser/loader... content/browser/loader/mime_sniffing_resource_handler.h:113: // Determines whether a plugin will handle the current reques. Returns false On 2016/08/26 21:03:04, mmenke wrote: > nit: request Done. https://codereview.chromium.org/2005273002/diff/480001/content/public/browser... File content/public/browser/resource_throttle.h (right): https://codereview.chromium.org/2005273002/diff/480001/content/public/browser... content/public/browser/resource_throttle.h:51: virtual bool MustProcessResponseBeforeReadingBody(); On 2016/08/19 16:33:02, mmenke wrote: > "no part of the response" -> "no part of the response body". I suspect we won't > cache the headers either, but I'm not sure of that (And I don't think this code > should implicitly make promises about caching stuff once we've pulled it from > net) Done. https://codereview.chromium.org/2005273002/diff/480001/content/public/browser... content/public/browser/resource_throttle.h:51: virtual bool MustProcessResponseBeforeReadingBody(); On 2016/08/22 20:39:05, mmenke wrote: > +const See https://www.chromium.org/developers/content-module/content-api: "don't add the const identifier to interfaces. For interfaces implemented by the embedder, we can't make assumptions about what the embedder needs to implement it. For interfaces implemented by content, the implementation details doesn't have to be exposed.". I know we're doing it above, but we shouldn't. https://codereview.chromium.org/2005273002/diff/480001/content/public/browser... content/public/browser/resource_throttle.h:51: virtual bool MustProcessResponseBeforeReadingBody(); On 2016/08/19 16:33:02, mmenke wrote: > Think we need test coverage of this method returning true - Can either cancel in > WillProcessResponse and make sure we never read the body, or make a cacheable > response, cancel there and make sure a second request doesn't use the cache. I have added a unit tests that checks that if a ResourceThrottle has MustProcessResponseBeforeReadingBody and cancels the request in WillProcessResponse, we will never call ReadData on the URLRequestJob.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM https://codereview.chromium.org/2005273002/diff/500001/chrome/browser/rendere... File chrome/browser/renderer_host/safe_browsing_resource_throttle.cc (right): https://codereview.chromium.org/2005273002/diff/500001/chrome/browser/rendere... chrome/browser/renderer_host/safe_browsing_resource_throttle.cc:174: // response has been received. Therefore, not part of it should be cached nit: not -> no https://codereview.chromium.org/2005273002/diff/500001/content/browser/loader... File content/browser/loader/intercepting_resource_handler.cc (right): https://codereview.chromium.org/2005273002/diff/500001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:103: CHECK(bytes_read >= 0); This is checking input of this method - seems like it should be at the top of this method. https://codereview.chromium.org/2005273002/diff/500001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:104: CHECK_GE(first_read_buffer_size_, static_cast<size_t>(bytes_read)); On 2016/09/01 23:19:40, clamy (traveling until Sep 4) wrote: > On 2016/08/22 20:39:04, mmenke wrote: > > CHECK -> DCHECK (x2) > > Considering we're about to write memory in the memcpy below, it seems safer from > a security POV to keep a CHECK here in order to crash rather than risk a buffer > overflow. Wdyt? The problem is CHECKs bloat up the binary, so they're not free, and we should be frugal with them. The "CHECK(bytes_read >= 0);" doesn't seem to give us much, particularly. And the last CHECK also seems to just be CHECKing that the rest of the loader stack followed its API contract. Surely we don't want to have a CHECK at each layer of the network stack along similar lines? Why is this one special? https://codereview.chromium.org/2005273002/diff/500001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:149: CHECK_GE(size, static_cast<int>(payload_for_old_handler_.length())); On 2016/09/01 23:19:40, clamy (traveling until Sep 4) wrote: > On 2016/08/22 20:39:04, mmenke wrote: > > CHECK->DCHECK (x2) > > Same comment as before. I left these as CHECKs because it checks for the > validity of the memcpy. If the first CHECK fails, we're going to crash, so it doesn't get us anything too useful in production builds. https://codereview.chromium.org/2005273002/diff/500001/content/browser/loader... File content/browser/loader/intercepting_resource_handler_unittest.cc (right): https://codereview.chromium.org/2005273002/diff/500001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:287: !memcmp(kData, new_test_handler->buffer()->data(), sizeof(kData))); On 2016/09/01 23:19:41, clamy (traveling until Sep 4) wrote: > On 2016/08/22 20:39:04, mmenke wrote: > > Suggest "EXPECT_EQ(kData, std::string(new_test_handler->buffer()->data(), > > new_test_handler->bytes_read()));" That results in more useful output on > > failure. > > > > Or better, you could make new_test_handler have a method to return > > std:string(new_test_handler->buffer()->data(), > new_test_handler->bytes_read()), > > and just use that. > > > > You could then also remove the "EXPECT_EQ(sizeof(kData), > > new_test_handler->bytes_read());" lines, since the comparisons now implicitly > > include that information. > > This method doesn't work if we do more than one read, since bytes_read is the > total amount of bytes read. This happens below. Ok, so let's just switch to comparing strings, for more useful output on regression.
The CQ bit was checked by clamy@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...
clamy@chromium.org changed reviewers: + jochen@chromium.org
@mmenke: Thanks! @jochen: PTAL at the changes in chrome/browser/renderer_host https://codereview.chromium.org/2005273002/diff/500001/chrome/browser/rendere... File chrome/browser/renderer_host/safe_browsing_resource_throttle.cc (right): https://codereview.chromium.org/2005273002/diff/500001/chrome/browser/rendere... chrome/browser/renderer_host/safe_browsing_resource_throttle.cc:174: // response has been received. Therefore, not part of it should be cached On 2016/09/02 19:22:05, mmenke wrote: > nit: not -> no Done. https://codereview.chromium.org/2005273002/diff/500001/content/browser/loader... File content/browser/loader/intercepting_resource_handler.cc (right): https://codereview.chromium.org/2005273002/diff/500001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:103: CHECK(bytes_read >= 0); On 2016/09/02 19:22:05, mmenke wrote: > This is checking input of this method - seems like it should be at the top of > this method. Moved this one and converted it to a DCHECK. https://codereview.chromium.org/2005273002/diff/500001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:104: CHECK_GE(first_read_buffer_size_, static_cast<size_t>(bytes_read)); On 2016/09/02 19:22:05, mmenke wrote: > On 2016/09/01 23:19:40, clamy (traveling until Sep 4) wrote: > > On 2016/08/22 20:39:04, mmenke wrote: > > > CHECK -> DCHECK (x2) > > > > Considering we're about to write memory in the memcpy below, it seems safer > from > > a security POV to keep a CHECK here in order to crash rather than risk a > buffer > > overflow. Wdyt? > > The problem is CHECKs bloat up the binary, so they're not free, and we should be > frugal with them. > > The "CHECK(bytes_read >= 0);" doesn't seem to give us much, particularly. > > And the last CHECK also seems to just be CHECKing that the rest of the loader > stack followed its API contract. Surely we don't want to have a CHECK at each > layer of the network stack along similar lines? Why is this one special? I'm keeping these two here as they are trying to prevent a buffer overflow from happening. I agree that we should use CHECKs with parsimony, and converted the bytes_read >= 0 to a DCHECK. However I checked with Nasko for these ones, and anything that could result in a buffer overflow should be checked. https://codereview.chromium.org/2005273002/diff/500001/content/browser/loader... content/browser/loader/intercepting_resource_handler.cc:149: CHECK_GE(size, static_cast<int>(payload_for_old_handler_.length())); On 2016/09/02 19:22:05, mmenke wrote: > On 2016/09/01 23:19:40, clamy (traveling until Sep 4) wrote: > > On 2016/08/22 20:39:04, mmenke wrote: > > > CHECK->DCHECK (x2) > > > > Same comment as before. I left these as CHECKs because it checks for the > > validity of the memcpy. > > If the first CHECK fails, we're going to crash, so it doesn't get us anything > too useful in production builds. Converted it to a DCHECK. https://codereview.chromium.org/2005273002/diff/500001/content/browser/loader... File content/browser/loader/intercepting_resource_handler_unittest.cc (right): https://codereview.chromium.org/2005273002/diff/500001/content/browser/loader... content/browser/loader/intercepting_resource_handler_unittest.cc:287: !memcmp(kData, new_test_handler->buffer()->data(), sizeof(kData))); On 2016/09/02 19:22:05, mmenke wrote: > On 2016/09/01 23:19:41, clamy (traveling until Sep 4) wrote: > > On 2016/08/22 20:39:04, mmenke wrote: > > > Suggest "EXPECT_EQ(kData, std::string(new_test_handler->buffer()->data(), > > > new_test_handler->bytes_read()));" That results in more useful output on > > > failure. > > > > > > Or better, you could make new_test_handler have a method to return > > > std:string(new_test_handler->buffer()->data(), > > new_test_handler->bytes_read()), > > > and just use that. > > > > > > You could then also remove the "EXPECT_EQ(sizeof(kData), > > > new_test_handler->bytes_read());" lines, since the comparisons now > implicitly > > > include that information. > > > > This method doesn't work if we do more than one read, since bytes_read is the > > total amount of bytes read. This happens below. > > Ok, so let's just switch to comparing strings, for more useful output on > regression. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by clamy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/2005273002/#ps540001 (title: "Addressed comments")
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 ========== Move MimeTypeResourceHandler before ThrottlingResourceHandler This CL moves the MimeTypeResourceHandler before the ThrottlingResourceHandler. This allows NavigationThrottles (called by the NavigationResourceThrottle) to execute with full knowledge of the MIME type of a navigation. This is needed to reland https://codereview.chromium.org/1617043002/, which needs knowledge of downloads in the NavigationThrottles. BUG=555418 ========== to ========== Move MimeTypeResourceHandler before ThrottlingResourceHandler This CL moves the MimeTypeResourceHandler before the ThrottlingResourceHandler. This allows NavigationThrottles (called by the NavigationResourceThrottle) to execute with full knowledge of the MIME type of a navigation. This is needed to reland https://codereview.chromium.org/1617043002/, which needs knowledge of downloads in the NavigationThrottles. BUG=555418 ==========
Message was sent while issue was closed.
Committed patchset #28 (id:540001)
Message was sent while issue was closed.
Description was changed from ========== Move MimeTypeResourceHandler before ThrottlingResourceHandler This CL moves the MimeTypeResourceHandler before the ThrottlingResourceHandler. This allows NavigationThrottles (called by the NavigationResourceThrottle) to execute with full knowledge of the MIME type of a navigation. This is needed to reland https://codereview.chromium.org/1617043002/, which needs knowledge of downloads in the NavigationThrottles. BUG=555418 ========== to ========== Move MimeTypeResourceHandler before ThrottlingResourceHandler This CL moves the MimeTypeResourceHandler before the ThrottlingResourceHandler. This allows NavigationThrottles (called by the NavigationResourceThrottle) to execute with full knowledge of the MIME type of a navigation. This is needed to reland https://codereview.chromium.org/1617043002/, which needs knowledge of downloads in the NavigationThrottles. BUG=555418 Committed: https://crrev.com/0e3422671a1173d23d6a1e4dfb35c0499b401fb6 Cr-Commit-Position: refs/heads/master@{#416621} ==========
Message was sent while issue was closed.
Patchset 28 (id:??) landed as https://crrev.com/0e3422671a1173d23d6a1e4dfb35c0499b401fb6 Cr-Commit-Position: refs/heads/master@{#416621} |
