ChromeFrame cookie requests would incorrectly get routed to the first host br...

ChromeFrame cookie requests would incorrectly get routed to the first host browser connected to the renderer. This basically
meant that if the browser process exited then other popups could not query cookies from the host.

Part of the fix for http://b/issue?id=2277519

Bug=2277519
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=53757

[amit, 2010-07-26 21:28:25]

[+cc ericdingle, Ananta figured out that the routing id is available in cookie
messages so this should fix your issue?]
 
lgtm with a few nits.

http://codereview.chromium.org/3066004/diff/7001/8002
File chrome/browser/automation/automation_resource_message_filter.cc (right):

http://codereview.chromium.org/3066004/diff/7001/8002#newcode361
chrome/browser/automation/automation_resource_message_filter.cc:361:
automation_details_iter->second.filter->Send(
if (filter) and maybe a NOTREACHED, just to be safe?

http://codereview.chromium.org/3066004/diff/7001/8002#newcode409
chrome/browser/automation/automation_resource_message_filter.cc:409:
automation_details_iter->second.filter->Send(
if ...?

http://codereview.chromium.org/3066004/diff/7001/8005
File chrome/browser/renderer_host/resource_message_filter.h (right):

http://codereview.chromium.org/3066004/diff/7001/8005#newcode476
chrome/browser/renderer_host/resource_message_filter.h:476: class
GetCookiesCompletion : public net::CompletionCallback {
GetCookiesCompletion adn GetRawCookiesCompletion are 99% identical so maybe just
an additional arg to the ctor like 'bool raw_cookies' should work?

[ananta, 2010-07-26 21:39:08]

http://codereview.chromium.org/3066004/diff/7001/8002
File chrome/browser/automation/automation_resource_message_filter.cc (right):

http://codereview.chromium.org/3066004/diff/7001/8002#newcode361
chrome/browser/automation/automation_resource_message_filter.cc:361:
automation_details_iter->second.filter->Send(
On 2010/07/26 21:28:25, amit wrote:
> if (filter) and maybe a NOTREACHED, just to be safe? 
There is a DCHECK for filter in this function. Added an if check.

http://codereview.chromium.org/3066004/diff/7001/8002#newcode409
chrome/browser/automation/automation_resource_message_filter.cc:409:
automation_details_iter->second.filter->Send(
On 2010/07/26 21:28:25, amit wrote:
> if ...?

Done.

http://codereview.chromium.org/3066004/diff/7001/8005
File chrome/browser/renderer_host/resource_message_filter.h (right):

http://codereview.chromium.org/3066004/diff/7001/8005#newcode476
chrome/browser/renderer_host/resource_message_filter.h:476: class
GetCookiesCompletion : public net::CompletionCallback {
On 2010/07/26 21:28:25, amit wrote:
> GetCookiesCompletion adn GetRawCookiesCompletion are 99% identical so maybe
just
> an additional arg to the ctor like 'bool raw_cookies' should work?

Done.

[Jeff Timanus, 2010-07-27 18:14:12]

Eric passed this review on to me.

Looks good, but I don't think it will solve the cookie routing issues fixed in a
separate CL, here:  http://codereview.chromium.org/2961007/show

This will ensure that the correct host browser will receive the cookie request,
but not that Chrome will provide the proper routing-id when making cookie
requests via Javascript.

http://codereview.chromium.org/3066004/diff/14001/15002
File chrome/browser/automation/automation_resource_message_filter.cc (right):

http://codereview.chromium.org/3066004/diff/14001/15002#newcode348
chrome/browser/automation/automation_resource_message_filter.cc:348:
get_cookies_callback->render_view_id())));
Nit:  4 space indents.

http://codereview.chromium.org/3066004/diff/14001/15004
File chrome/browser/renderer_host/resource_message_filter.cc (right):

http://codereview.chromium.org/3066004/diff/14001/15004#newcode618
chrome/browser/renderer_host/resource_message_filter.cc:618: reply_msg, this,
context, true);
This will correctly pass the renderer identification to the cookie routing
mechanism, but there is still a problem here - multiple different RenderViews
may share the same ResourceMessageFilter.  The first to install the context will
win - for example, an extension background page that does not have the
AutomationMessageFilter configured will force all subsequent users of this
ResourceMessageFilter to use the Chrome cookie jar.

http://codereview.chromium.org/3066004/diff/14001/15005
File chrome/browser/renderer_host/resource_message_filter.h (right):

http://codereview.chromium.org/3066004/diff/14001/15005#newcode473
chrome/browser/renderer_host/resource_message_filter.h:473:
scoped_refptr<ChromeURLRequestContext> context_;
Nit:  Disallow copy & assign?

http://codereview.chromium.org/3066004/diff/14001/15005#newcode502
chrome/browser/renderer_host/resource_message_filter.h:502: bool raw_cookies_;
Disallow copy and assign here too?

[ananta, 2010-07-27 19:15:53]

Hi Twiz

I will address these comments in a followup CL.

Thanks
Ananta

On Tue, Jul 27, 2010 at 11:14 AM, <twiz@chromium.org> wrote:

> Eric passed this review on to me.
>
> Looks good, but I don't think it will solve the cookie routing issues fixed
> in a
> separate CL, here:  http://codereview.chromium.org/2961007/show
>
> This will ensure that the correct host browser will receive the cookie
> request,
> but not that Chrome will provide the proper routing-id when making cookie
> requests via Javascript.
>
>
>
> http://codereview.chromium.org/3066004/diff/14001/15002
>
> File chrome/browser/automation/automation_resource_message_filter.cc
> (right):
>
> http://codereview.chromium.org/3066004/diff/14001/15002#newcode348
> chrome/browser/automation/automation_resource_message_filter.cc:348:
> get_cookies_callback->render_view_id())));
> Nit:  4 space indents.
>
> http://codereview.chromium.org/3066004/diff/14001/15004
> File chrome/browser/renderer_host/resource_message_filter.cc (right):
>
> http://codereview.chromium.org/3066004/diff/14001/15004#newcode618
> chrome/browser/renderer_host/resource_message_filter.cc:618: reply_msg,
> this, context, true);
> This will correctly pass the renderer identification to the cookie
> routing mechanism, but there is still a problem here - multiple
> different RenderViews may share the same ResourceMessageFilter.  The
> first to install the context will win - for example, an extension
> background page that does not have the AutomationMessageFilter
> configured will force all subsequent users of this ResourceMessageFilter
> to use the Chrome cookie jar.
>
> http://codereview.chromium.org/3066004/diff/14001/15005
>
> File chrome/browser/renderer_host/resource_message_filter.h (right):
>
> http://codereview.chromium.org/3066004/diff/14001/15005#newcode473
> chrome/browser/renderer_host/resource_message_filter.h:473:
> scoped_refptr<ChromeURLRequestContext> context_;
> Nit:  Disallow copy & assign?
>
> http://codereview.chromium.org/3066004/diff/14001/15005#newcode502
> chrome/browser/renderer_host/resource_message_filter.h:502: bool
> raw_cookies_;
> Disallow copy and assign here too?
>
>
> http://codereview.chromium.org/3066004/show
>