Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2222723002:
Avoid calling into the ContentBrowserClient interface from ResourceDispatcherHostImpl to determine … (Closed)
|
Created:
4 years, 4 months ago by ananta Modified:
4 years, 4 months ago CC:
chromium-reviews, extensions-reviews_chromium.org, Randy Smith (Not in Mondays), darin-cc_chromium.org, loading-reviews_chromium.org, chromium-apps-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAvoid calling into the ContentBrowserClient interface from ResourceDispatcherHostImpl to determine whether an origin is legal.
Continuing changes to remove usage of ContentBrowserClient from
ResourceDispatcherHostImpl.
This patch adds a facility to the ResourceDispatcherHost class to register
HTTP header based interceptors. The caller can specify a prefix for the header value it expects. This is matched in
ResourceDispatcherHostImpl before invoking the caller callback. We allow
callers to pass in empty strings for the header value, in which case they
receive notifications for all occurrences of that HTTP header.
I also removed the IsIllegalOrigin method from the ContentBrowserClient
interface
BUG=598073
Committed: https://crrev.com/a5721fb7ef4c4bd89e143bc5024bbd4b1e844fb6
Cr-Commit-Position: refs/heads/master@{#411114}
Patch Set 1 #Patch Set 2 : Revert header change and remove blank line #Patch Set 3 : Rebase to tip and git cl format #Patch Set 4 : Register the origin interceptor after RDH is created #Patch Set 5 : Invoke the correct callback on ChromeContentBrowserClientParts. #Patch Set 6 : Fix compile failures #Patch Set 7 : Fix compile failures #
Total comments: 14
Patch Set 8 : Address review comments #Patch Set 9 : Address review comments. Change interceptor to a callback #Patch Set 10 : Remove the extension origin interceptor class. #
Total comments: 7
Patch Set 11 : Address review comments #Patch Set 12 : Attempt to fix 64 bit windows redness #
Total comments: 6
Messages
Total messages: 51 (31 generated)
The CQ bit was checked by ananta@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 ananta@chromium.org
The CQ bit was checked by ananta@chromium.org to run a CQ dry run
ananta@chromium.org changed reviewers: + jam@chromium.org
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 ananta@chromium.org
Do we really need another RDH observer interface for this? We have delegate_->ShouldBeginRequest already. Seems like we could just piggyback these call on that.
On 2016/08/08 22:06:53, mmenke wrote: > Do we really need another RDH observer interface for this? We have > delegate_->ShouldBeginRequest already. Seems like we could just piggyback these > call on that. We wanted to avoid a delegate like pattern given that RDH and most of the code in content\browser\loader will eventually be moved into a network service.
The CQ bit was checked by ananta@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 ananta@chromium.org
The CQ bit was checked by ananta@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 ananta@chromium.org
great, just some nits mostly around avoiding adding another interface if we can avoid it https://codereview.chromium.org/2222723002/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc (right): https://codereview.chromium.org/2222723002/diff/120001/chrome/browser/extensi... chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:320: DCHECK_CURRENTLY_ON(BrowserThread::IO); no need to have this method anymore in this class, and it can just move the callback above https://codereview.chromium.org/2222723002/diff/120001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2222723002/diff/120001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1351: ContinuePendingBeginRequest(request_id, request_data, sync_result, route_id, nit: to avoid duplicating this from line 1337, can also do callback.Run() https://codereview.chromium.org/2222723002/diff/120001/content/public/browser... File content/public/browser/resource_dispatcher_host.h (right): https://codereview.chromium.org/2222723002/diff/120001/content/public/browser... content/public/browser/resource_dispatcher_host.h:71: virtual void UnregisterInterceptor( I think we don't need this? e.g. if we get rid of the ResourceDispatcherHostInterceptor interface and just have a callback. https://codereview.chromium.org/2222723002/diff/120001/content/public/browser... File content/public/browser/resource_dispatcher_host_interceptor.h (right): https://codereview.chromium.org/2222723002/diff/120001/content/public/browser... content/public/browser/resource_dispatcher_host_interceptor.h:36: OnHeaderProcessedCallback callback); per https://www.chromium.org/developers/content-module/content-api, since this is a single-method interface, can this just be a callback parameter to ResourceDispatcherHost? https://codereview.chromium.org/2222723002/diff/120001/extensions/browser/bad... File extensions/browser/bad_message.h (right): https://codereview.chromium.org/2222723002/diff/120001/extensions/browser/bad... extensions/browser/bad_message.h:31: INVALID_ORIGIN = 7, I'm curious what is the reason to start instrumenting this?
Description was changed from ========== Avoid calling into the ContentBrowserClient interface from ResourceDispatcherHostImpl to determine whether an origin is legal. Continuing changes to remove usage of ContentBrowserClient from ResourceDispatcherHostImpl. This patch adds a facility to the ResourceDispatcherHost class to register HTTP header based interceptors. The interface is defined in content/public/browser/resource_dispatcher_host_interceptor.h. The caller can specify a prefix for the header value it expects. This is matched in ResourceDispatcherHostImpl before invoking the caller interface. We allow callers to pass in empty strings for the header value, in which case they receive notifications for all occurrences of that HTTP header. I also removed the IsIllegalOrigin method from the ContentBrowserClient interface BUG=598073 ========== to ========== Avoid calling into the ContentBrowserClient interface from ResourceDispatcherHostImpl to determine whether an origin is legal. Continuing changes to remove usage of ContentBrowserClient from ResourceDispatcherHostImpl. This patch adds a facility to the ResourceDispatcherHost class to register HTTP header based interceptors. The interface is defined in content/public/browser/resource_dispatcher_host_interceptor.h. The caller can specify a prefix for the header value it expects. This is matched in ResourceDispatcherHostImpl before invoking the caller interface. We allow callers to pass in empty strings for the header value, in which case they receive notifications for all occurrences of that HTTP header. I added a method ResourceDispatcherHostDestroyed to the ContentBrowserClient class which is overridden by the ChromeContentBrowserClient class. This is invoked before the RDH is torn down. This is currently used to ensure that the callbacks on the RDH are unregistered. I also removed the IsIllegalOrigin method from the ContentBrowserClient interface BUG=598073 ==========
The CQ bit was checked by ananta@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 ananta@chromium.org
https://codereview.chromium.org/2222723002/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc (right): https://codereview.chromium.org/2222723002/diff/120001/chrome/browser/extensi... chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:320: DCHECK_CURRENTLY_ON(BrowserThread::IO); On 2016/08/09 01:36:24, jam wrote: > no need to have this method anymore in this class, and it can just move the > callback above Done. https://codereview.chromium.org/2222723002/diff/120001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2222723002/diff/120001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1351: ContinuePendingBeginRequest(request_id, request_data, sync_result, route_id, On 2016/08/09 01:36:24, jam wrote: > nit: to avoid duplicating this from line 1337, can also do callback.Run() Done. https://codereview.chromium.org/2222723002/diff/120001/content/public/browser... File content/public/browser/resource_dispatcher_host.h (right): https://codereview.chromium.org/2222723002/diff/120001/content/public/browser... content/public/browser/resource_dispatcher_host.h:71: virtual void UnregisterInterceptor( On 2016/08/09 01:36:24, jam wrote: > I think we don't need this? e.g. if we get rid of the > ResourceDispatcherHostInterceptor interface and just have a callback. Lets talk about this tomorrow. I was wondering whether with the network service eventually converting to mojo, it may be easier to have these callbacks exposed via a mojo interface on the client. If that does seem plausible, perhaps we could leave this as is. https://codereview.chromium.org/2222723002/diff/120001/content/public/browser... File content/public/browser/resource_dispatcher_host_interceptor.h (right): https://codereview.chromium.org/2222723002/diff/120001/content/public/browser... content/public/browser/resource_dispatcher_host_interceptor.h:36: OnHeaderProcessedCallback callback); On 2016/08/09 01:36:24, jam wrote: > per https://www.chromium.org/developers/content-module/content-api, since this > is a single-method interface, can this just be a callback parameter to > ResourceDispatcherHost? This could be a callback. Just thinking out aloud, if there would be a similar use case in RDH where we would need other callbacks. Lets talk tomorrow about this. https://codereview.chromium.org/2222723002/diff/120001/extensions/browser/bad... File extensions/browser/bad_message.h (right): https://codereview.chromium.org/2222723002/diff/120001/extensions/browser/bad... extensions/browser/bad_message.h:31: INVALID_ORIGIN = 7, On 2016/08/09 01:36:24, jam wrote: > I'm curious what is the reason to start instrumenting this? Reason being, to pass in a bad message code to content. The bad_message.h currently being used is not accessible outside content. Till now it wasn't an issue because we were returning true/false from IsIllegalOrigin from the embedder.
https://codereview.chromium.org/2222723002/diff/120001/content/public/browser... File content/public/browser/resource_dispatcher_host.h (right): https://codereview.chromium.org/2222723002/diff/120001/content/public/browser... content/public/browser/resource_dispatcher_host.h:71: virtual void UnregisterInterceptor( On 2016/08/09 05:47:48, ananta wrote: > On 2016/08/09 01:36:24, jam wrote: > > I think we don't need this? e.g. if we get rid of the > > ResourceDispatcherHostInterceptor interface and just have a callback. > > Lets talk about this tomorrow. I was wondering whether with the network service > eventually converting to mojo, it may be easier to have these callbacks exposed > via a mojo interface on the client. > > If that does seem plausible, perhaps we could leave this as is. The motivation is that eventually RDH will be the mojo interface to the network service. Mojo methods can take callbacks, so it seems sensible that this interceptor (mojo) method would be written that way. it also simplifies this change a little
Description was changed from ========== Avoid calling into the ContentBrowserClient interface from ResourceDispatcherHostImpl to determine whether an origin is legal. Continuing changes to remove usage of ContentBrowserClient from ResourceDispatcherHostImpl. This patch adds a facility to the ResourceDispatcherHost class to register HTTP header based interceptors. The interface is defined in content/public/browser/resource_dispatcher_host_interceptor.h. The caller can specify a prefix for the header value it expects. This is matched in ResourceDispatcherHostImpl before invoking the caller interface. We allow callers to pass in empty strings for the header value, in which case they receive notifications for all occurrences of that HTTP header. I added a method ResourceDispatcherHostDestroyed to the ContentBrowserClient class which is overridden by the ChromeContentBrowserClient class. This is invoked before the RDH is torn down. This is currently used to ensure that the callbacks on the RDH are unregistered. I also removed the IsIllegalOrigin method from the ContentBrowserClient interface BUG=598073 ========== to ========== Avoid calling into the ContentBrowserClient interface from ResourceDispatcherHostImpl to determine whether an origin is legal. Continuing changes to remove usage of ContentBrowserClient from ResourceDispatcherHostImpl. This patch adds a facility to the ResourceDispatcherHost class to register HTTP header based interceptors. . The caller can specify a prefix for the header value it expects. This is matched in ResourceDispatcherHostImpl before invoking the caller callback. We allow callers to pass in empty strings for the header value, in which case they receive notifications for all occurrences of that HTTP header. I also removed the IsIllegalOrigin method from the ContentBrowserClient interface BUG=598073 ==========
https://codereview.chromium.org/2222723002/diff/120001/content/public/browser... File content/public/browser/resource_dispatcher_host.h (right): https://codereview.chromium.org/2222723002/diff/120001/content/public/browser... content/public/browser/resource_dispatcher_host.h:71: virtual void UnregisterInterceptor( On 2016/08/09 19:58:41, jam wrote: > On 2016/08/09 05:47:48, ananta wrote: > > On 2016/08/09 01:36:24, jam wrote: > > > I think we don't need this? e.g. if we get rid of the > > > ResourceDispatcherHostInterceptor interface and just have a callback. > > > > Lets talk about this tomorrow. I was wondering whether with the network > service > > eventually converting to mojo, it may be easier to have these callbacks > exposed > > via a mojo interface on the client. > > > > If that does seem plausible, perhaps we could leave this as is. > > The motivation is that eventually RDH will be the mojo interface to the network > service. Mojo methods can take callbacks, so it seems sensible that this > interceptor (mojo) method would be written that way. it also simplifies this > change a little Done.
Description was changed from ========== Avoid calling into the ContentBrowserClient interface from ResourceDispatcherHostImpl to determine whether an origin is legal. Continuing changes to remove usage of ContentBrowserClient from ResourceDispatcherHostImpl. This patch adds a facility to the ResourceDispatcherHost class to register HTTP header based interceptors. . The caller can specify a prefix for the header value it expects. This is matched in ResourceDispatcherHostImpl before invoking the caller callback. We allow callers to pass in empty strings for the header value, in which case they receive notifications for all occurrences of that HTTP header. I also removed the IsIllegalOrigin method from the ContentBrowserClient interface BUG=598073 ========== to ========== Avoid calling into the ContentBrowserClient interface from ResourceDispatcherHostImpl to determine whether an origin is legal. Continuing changes to remove usage of ContentBrowserClient from ResourceDispatcherHostImpl. This patch adds a facility to the ResourceDispatcherHost class to register HTTP header based interceptors. The caller can specify a prefix for the header value it expects. This is matched in ResourceDispatcherHostImpl before invoking the caller callback. We allow callers to pass in empty strings for the header value, in which case they receive notifications for all occurrences of that HTTP header. I also removed the IsIllegalOrigin method from the ContentBrowserClient interface BUG=598073 ==========
https://codereview.chromium.org/2222723002/diff/120001/extensions/browser/bad... File extensions/browser/bad_message.h (right): https://codereview.chromium.org/2222723002/diff/120001/extensions/browser/bad... extensions/browser/bad_message.h:31: INVALID_ORIGIN = 7, On 2016/08/09 05:47:48, ananta wrote: > On 2016/08/09 01:36:24, jam wrote: > > I'm curious what is the reason to start instrumenting this? > > Reason being, to pass in a bad message code to content. The bad_message.h > currently being used is not accessible outside content. Till now it wasn't an > issue because we were returning true/false from IsIllegalOrigin from the > embedder. I'm not sure I follow. The bad_message::ReceivedBadMessage call is in content (ResourceDispatcherHostImpl::ContinuePendingBeginRequest), so why can't it just always call bad_message::ReceivedBadMessage(bad_message::RDH_ILLEGAL_ORIGIN) ? https://codereview.chromium.org/2222723002/diff/180001/chrome/browser/extensi... File chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc (right): https://codereview.chromium.org/2222723002/diff/180001/chrome/browser/extensi... chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:631: void ChromeContentBrowserClientExtensionsPart::OnHttpHeaderReceived( nit: why are these two methods part of ChromeContentBrowserClientExtensionsPart instead of being anonymous methods? they're not called by anyone outside this class directly, so it'd save having to write both a declaration and definition. https://codereview.chromium.org/2222723002/diff/180001/content/public/browser... File content/public/browser/content_browser_client.h (right): https://codereview.chromium.org/2222723002/diff/180001/content/public/browser... content/public/browser/content_browser_client.h:523: virtual void ResourceDispatcherHostDestroyed() {} don't need this https://codereview.chromium.org/2222723002/diff/180001/content/public/browser... File content/public/browser/resource_dispatcher_host.h (right): https://codereview.chromium.org/2222723002/diff/180001/content/public/browser... content/public/browser/resource_dispatcher_host.h:34: // notified of certain HTTP headers in outgoing requests. For e.g. Origin. nit: please document the parameters to this callback. ditto for above https://codereview.chromium.org/2222723002/diff/180001/content/public/browser... content/public/browser/resource_dispatcher_host.h:72: // strings for the |http_header| and |starts_with| parameters. nit: I'd remove these two lines since we don't need to document possible future APIs. Specifically, I don't think we'd ever want to filter for all requests since that would go against the goal of not slowing down the general code path
https://codereview.chromium.org/2222723002/diff/120001/extensions/browser/bad... File extensions/browser/bad_message.h (right): https://codereview.chromium.org/2222723002/diff/120001/extensions/browser/bad... extensions/browser/bad_message.h:31: INVALID_ORIGIN = 7, On 2016/08/10 00:19:26, jam wrote: > On 2016/08/09 05:47:48, ananta wrote: > > On 2016/08/09 01:36:24, jam wrote: > > > I'm curious what is the reason to start instrumenting this? > > > > Reason being, to pass in a bad message code to content. The bad_message.h > > currently being used is not accessible outside content. Till now it wasn't an > > issue because we were returning true/false from IsIllegalOrigin from the > > embedder. > > I'm not sure I follow. > > The bad_message::ReceivedBadMessage call is in content > (ResourceDispatcherHostImpl::ContinuePendingBeginRequest), so why can't it just > always call bad_message::ReceivedBadMessage(bad_message::RDH_ILLEGAL_ORIGIN) > ? We cannot assume RDH_ILLEGAL_ORIGIN all the time. The current use case, yes. However not if we consider other header inspectors going forward. https://codereview.chromium.org/2222723002/diff/180001/chrome/browser/extensi... File chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc (right): https://codereview.chromium.org/2222723002/diff/180001/chrome/browser/extensi... chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:631: void ChromeContentBrowserClientExtensionsPart::OnHttpHeaderReceived( On 2016/08/10 00:19:26, jam wrote: > nit: why are these two methods part of ChromeContentBrowserClientExtensionsPart > instead of being anonymous methods? they're not called by anyone outside this > class directly, so it'd save having to write both a declaration and definition. I had moved them to the namespace. Forgot to upload the patch. https://codereview.chromium.org/2222723002/diff/180001/content/public/browser... File content/public/browser/resource_dispatcher_host.h (right): https://codereview.chromium.org/2222723002/diff/180001/content/public/browser... content/public/browser/resource_dispatcher_host.h:34: // notified of certain HTTP headers in outgoing requests. For e.g. Origin. On 2016/08/10 00:19:26, jam wrote: > nit: please document the parameters to this callback. ditto for above Done. https://codereview.chromium.org/2222723002/diff/180001/content/public/browser... content/public/browser/resource_dispatcher_host.h:72: // strings for the |http_header| and |starts_with| parameters. On 2016/08/10 00:19:26, jam wrote: > nit: I'd remove these two lines since we don't need to document possible future > APIs. Specifically, I don't think we'd ever want to filter for all requests > since that would go against the goal of not slowing down the general code path Done.
The CQ bit was checked by ananta@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 ananta@chromium.org
lgtm
The CQ bit was checked by ananta@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.
The CQ bit was checked by ananta@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org Link to the patchset: https://codereview.chromium.org/2222723002/#ps220001 (title: "Attempt to fix 64 bit windows redness")
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 ========== Avoid calling into the ContentBrowserClient interface from ResourceDispatcherHostImpl to determine whether an origin is legal. Continuing changes to remove usage of ContentBrowserClient from ResourceDispatcherHostImpl. This patch adds a facility to the ResourceDispatcherHost class to register HTTP header based interceptors. The caller can specify a prefix for the header value it expects. This is matched in ResourceDispatcherHostImpl before invoking the caller callback. We allow callers to pass in empty strings for the header value, in which case they receive notifications for all occurrences of that HTTP header. I also removed the IsIllegalOrigin method from the ContentBrowserClient interface BUG=598073 ========== to ========== Avoid calling into the ContentBrowserClient interface from ResourceDispatcherHostImpl to determine whether an origin is legal. Continuing changes to remove usage of ContentBrowserClient from ResourceDispatcherHostImpl. This patch adds a facility to the ResourceDispatcherHost class to register HTTP header based interceptors. The caller can specify a prefix for the header value it expects. This is matched in ResourceDispatcherHostImpl before invoking the caller callback. We allow callers to pass in empty strings for the header value, in which case they receive notifications for all occurrences of that HTTP header. I also removed the IsIllegalOrigin method from the ContentBrowserClient interface BUG=598073 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Avoid calling into the ContentBrowserClient interface from ResourceDispatcherHostImpl to determine whether an origin is legal. Continuing changes to remove usage of ContentBrowserClient from ResourceDispatcherHostImpl. This patch adds a facility to the ResourceDispatcherHost class to register HTTP header based interceptors. The caller can specify a prefix for the header value it expects. This is matched in ResourceDispatcherHostImpl before invoking the caller callback. We allow callers to pass in empty strings for the header value, in which case they receive notifications for all occurrences of that HTTP header. I also removed the IsIllegalOrigin method from the ContentBrowserClient interface BUG=598073 ========== to ========== Avoid calling into the ContentBrowserClient interface from ResourceDispatcherHostImpl to determine whether an origin is legal. Continuing changes to remove usage of ContentBrowserClient from ResourceDispatcherHostImpl. This patch adds a facility to the ResourceDispatcherHost class to register HTTP header based interceptors. The caller can specify a prefix for the header value it expects. This is matched in ResourceDispatcherHostImpl before invoking the caller callback. We allow callers to pass in empty strings for the header value, in which case they receive notifications for all occurrences of that HTTP header. I also removed the IsIllegalOrigin method from the ContentBrowserClient interface BUG=598073 Committed: https://crrev.com/a5721fb7ef4c4bd89e143bc5024bbd4b1e844fb6 Cr-Commit-Position: refs/heads/master@{#411114} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/a5721fb7ef4c4bd89e143bc5024bbd4b1e844fb6 Cr-Commit-Position: refs/heads/master@{#411114}
Message was sent while issue was closed.
creis@chromium.org changed reviewers: + creis@chromium.org
Message was sent while issue was closed.
Sorry, please revert ASAP. We'll get corrupted data in the crash reports with this in place. (Otherwise I really like this approach and I'm happy we're going this way.) https://codereview.chromium.org/2222723002/diff/220001/chrome/browser/extensi... File chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc (right): https://codereview.chromium.org/2222723002/diff/220001/chrome/browser/extensi... chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:143: // committed. nit: Does "committed" fit on the previous line? It did before. https://codereview.chromium.org/2222723002/diff/220001/chrome/browser/extensi... chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:158: // are nit: Fix line wrap. https://codereview.chromium.org/2222723002/diff/220001/chrome/browser/extensi... chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:164: // given Same. https://codereview.chromium.org/2222723002/diff/220001/chrome/browser/extensi... chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:176: // information Same. https://codereview.chromium.org/2222723002/diff/220001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2222723002/diff/220001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1352: filter_, static_cast<bad_message::BadMessageReason>(error_code)); This is not a safe cast. The INVALID_ORIGIN code from extensions will collide with RFH_FILE_CHOOSER_PATH from content (7), and this kill will end up looking like the latter because we're calling the content version of ReceivedBadMessage here. I see that you're trying to generalize this to handle other types of kills, which is nice, but I'm not sure how to make that work here. https://codereview.chromium.org/2222723002/diff/220001/extensions/browser/bad... File extensions/browser/bad_message.h (right): https://codereview.chromium.org/2222723002/diff/220001/extensions/browser/bad... extensions/browser/bad_message.h:31: INVALID_ORIGIN = 7, As mentioned before, we can't define a code here and call ReceivedBadMessage from within content with it. (Also, I should point out that it's nice to use ReceivedBadMessage within content when possible, since we have better support for magic signatures there than in these other bad_message.h files, which get hit less often.)
Message was sent while issue was closed.
Instead of reverting just because of that one line, it's more lightweight to change the base_message call to bad_message::ReceivedBadMessage(filter_, bad_message::RDH_ILLEGAL_ORIGIN); and then we can asynchronously figure out how to fix it properly On 2016/08/10 20:44:02, Charlie Reis wrote: > Sorry, please revert ASAP. We'll get corrupted data in the crash reports with > this in place. > > (Otherwise I really like this approach and I'm happy we're going this way.) > > https://codereview.chromium.org/2222723002/diff/220001/chrome/browser/extensi... > File chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc > (right): > > https://codereview.chromium.org/2222723002/diff/220001/chrome/browser/extensi... > chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:143: > // committed. > nit: Does "committed" fit on the previous line? It did before. > > https://codereview.chromium.org/2222723002/diff/220001/chrome/browser/extensi... > chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:158: > // are > nit: Fix line wrap. > > https://codereview.chromium.org/2222723002/diff/220001/chrome/browser/extensi... > chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:164: > // given > Same. > > https://codereview.chromium.org/2222723002/diff/220001/chrome/browser/extensi... > chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:176: > // information > Same. > > https://codereview.chromium.org/2222723002/diff/220001/content/browser/loader... > File content/browser/loader/resource_dispatcher_host_impl.cc (right): > > https://codereview.chromium.org/2222723002/diff/220001/content/browser/loader... > content/browser/loader/resource_dispatcher_host_impl.cc:1352: filter_, > static_cast<bad_message::BadMessageReason>(error_code)); > This is not a safe cast. The INVALID_ORIGIN code from extensions will collide > with RFH_FILE_CHOOSER_PATH from content (7), and this kill will end up looking > like the latter because we're calling the content version of ReceivedBadMessage > here. > > I see that you're trying to generalize this to handle other types of kills, > which is nice, but I'm not sure how to make that work here. > > https://codereview.chromium.org/2222723002/diff/220001/extensions/browser/bad... > File extensions/browser/bad_message.h (right): > > https://codereview.chromium.org/2222723002/diff/220001/extensions/browser/bad... > extensions/browser/bad_message.h:31: INVALID_ORIGIN = 7, > As mentioned before, we can't define a code here and call ReceivedBadMessage > from within content with it. (Also, I should point out that it's nice to use > ReceivedBadMessage within content when possible, since we have better support > for magic signatures there than in these other bad_message.h files, which get > hit less often.)
Message was sent while issue was closed.
On 2016/08/10 20:52:16, jam wrote: > Instead of reverting just because of that one line, it's more lightweight to > change the base_message call to bad_message::ReceivedBadMessage(filter_, > bad_message::RDH_ILLEGAL_ORIGIN); > > and then we can asynchronously figure out how to fix it properly I'm ok with that, but we'll need to land something before 5 to avoid having bad data in tomorrow's canary.
Message was sent while issue was closed.
On 2016/08/10 20:58:51, Charlie Reis wrote: > On 2016/08/10 20:52:16, jam wrote: > > Instead of reverting just because of that one line, it's more lightweight to > > change the base_message call to bad_message::ReceivedBadMessage(filter_, > > bad_message::RDH_ILLEGAL_ORIGIN); > > > > and then we can asynchronously figure out how to fix it properly > > I'm ok with that, but we'll need to land something before 5 to avoid having bad > data in tomorrow's canary. I'll put together a CL, since Ananta isn't responding on chat.
Message was sent while issue was closed.
On 2016/08/10 21:03:26, Charlie Reis wrote: > On 2016/08/10 20:58:51, Charlie Reis wrote: > > On 2016/08/10 20:52:16, jam wrote: > > > Instead of reverting just because of that one line, it's more lightweight to > > > change the base_message call to bad_message::ReceivedBadMessage(filter_, > > > bad_message::RDH_ILLEGAL_ORIGIN); > > > > > > and then we can asynchronously figure out how to fix it properly > > > > I'm ok with that, but we'll need to land something before 5 to avoid having > bad > > data in tomorrow's canary. > > I'll put together a CL, since Ananta isn't responding on chat. Up for review and try jobs: https://codereview.chromium.org/2236503004/
Message was sent while issue was closed.
On 2016/08/08 23:32:42, ananta wrote: > On 2016/08/08 22:06:53, mmenke wrote: > > Do we really need another RDH observer interface for this? We have > > delegate_->ShouldBeginRequest already. Seems like we could just piggyback > these > > call on that. > > We wanted to avoid a delegate like pattern given that RDH and most of the code > in content\browser\loader will eventually be > moved into a network service. Still seems really problematic to me. Oh well. Would have followed up on this, but I somehow ended up off of the CC list (Maybe commenting on the issue removed me from it? Not sure).
Message was sent while issue was closed.
On 2016/08/11 20:09:39, mmenke wrote: > On 2016/08/08 23:32:42, ananta wrote: > > On 2016/08/08 22:06:53, mmenke wrote: > > > Do we really need another RDH observer interface for this? We have > > > delegate_->ShouldBeginRequest already. Seems like we could just piggyback > > these > > > call on that. > > > > We wanted to avoid a delegate like pattern given that RDH and most of the code > > in content\browser\loader will eventually be > > moved into a network service. > > Still seems really problematic to me. Oh well. Would have followed up on this, > but I somehow ended up off of the CC list (Maybe commenting on the issue removed > me from it? Not sure). Please send in your comments anyways. Will try to address your concerns in a followup. |
