Add ShellNetworkDelegate so content_shell can use cookies (again)

Add ShellNetworkDelegate so content_shell can use cookies (again)

BUG=none
TEST=try to log in to some site using content_shell

[jochen (gone - plz use gerrit), 2012-03-14 12:53:03]

plz review

this is necessary now that the default is to reject cookies

[jam, 2012-03-15 01:39:45]

sorry just saw this

why is the default that one needs to add a network delegate to get cookies
working? it seems that this should be the default behavior if one didn't specify
a network delegate. otherwise every embedder will run into this surprise and
have to waste time figuring out how to fix it.

[jochen (gone - plz use gerrit), 2012-03-15 06:12:10]

The default behavior of URLRequest has always been to discard cookies, it's
just that the no longer existing methods of URLRequest::Delegate defaulted
to allowing cookies. NetworkDelegate defaults to blocking to be consistent
with URLRequest.

An content embedder also needs to figure out that they need to create a
cookie monster, otherwise cookies also won't work (even before my changes
to NetworkDelegate)

[jam, 2012-03-15 06:29:28]

On 2012/03/15 06:12:10, jochen wrote:
> The default behavior of URLRequest has always been to discard cookies, it's
> just that the no longer existing methods of URLRequest::Delegate defaulted
> to allowing cookies. NetworkDelegate defaults to blocking to be consistent
> with URLRequest.
> 
> An content embedder also needs to figure out that they need to create a
> cookie monster, otherwise cookies also won't work (even before my changes
> to NetworkDelegate)

Figuring out that they need a cookie monster is apparent very quickly from what
I remember, because otherwise there's a crash. In this case, functionality just
breaks and the new embedder is left to figure out how to track this down.

I don't see the motivation for having the default behavior of NetworkDelegate
(if it's not set afaik) to block cookies. In almost everything else, by default
we allow something unless the embedder disables it.

[jochen (gone - plz use gerrit), 2012-03-15 06:44:48]

The main advantage for me is that it's easier to audit - I can just search
for all places that implement a network delegate.

I'd propose to add a comment yo urlrequestcontext stating that you need a
network delegate in order to use cookies, maybe right next to the cookie
monster setter, wdyt
On Mar 15, 2012 7:29 AM, <jam@chromium.org> wrote:

> On 2012/03/15 06:12:10, jochen wrote:
>
>> The default behavior of URLRequest has always been to discard cookies,
>> it's
>> just that the no longer existing methods of URLRequest::Delegate defaulted
>> to allowing cookies. NetworkDelegate defaults to blocking to be consistent
>> with URLRequest.
>>
>
>  An content embedder also needs to figure out that they need to create a
>> cookie monster, otherwise cookies also won't work (even before my changes
>> to NetworkDelegate)
>>
>
> Figuring out that they need a cookie monster is apparent very quickly from
> what
> I remember, because otherwise there's a crash. In this case, functionality
> just
> breaks and the new embedder is left to figure out how to track this down.
>
> I don't see the motivation for having the default behavior of
> NetworkDelegate
> (if it's not set afaik) to block cookies. In almost everything else, by
> default
> we allow something unless the embedder disables it.
>
>
https://chromiumcodereview.**appspot.com/9701023/<https://chromiumcodereview....
>

[jam, 2012-03-15 06:48:57]

On 2012/03/15 06:44:48, jochen wrote:
> The main advantage for me is that it's easier to audit - I can just search
> for all places that implement a network delegate.

Is this a problem in practice? There's only one network delegate in chrome
afaik. It seems just as easy to audit that the one in chrome does what we want
by default.

> 
> I'd propose to add a comment yo urlrequestcontext stating that you need a
> network delegate in order to use cookies, maybe right next to the cookie
> monster setter, wdyt

I think if someone finds their way to the comment, they'd be mostly done
anyways. The issue is that it's hard to get to either of these places for a
newcomer.

[jam, 2012-03-15 06:54:23]

btw I looked at the change which switched from URLRequest::Delegate to
NetworkDelegate to block cookies at
https://chromiumcodereview.appspot.com/9572001/diff/41001/net/url_request/url....
so before we were allowing by default, and that change caused the default to be
false. I think we really want this to be changed to true.

[jochen (gone - plz use gerrit), 2012-03-15 06:55:36]

On Mar 15, 2012 7:48 AM, <jam@chromium.org> wrote:

On 2012/03/15 06:44:48, jochen wrote:

The main advantage for me is that it's easier to audit - I can just search
for all places that implement a network delegate.

Is this a problem in practice? There's only one network delegate in chrome
afaik. It seems just as easy to audit that the one in chrome does what we
want
by default.

The problem is finding all the places accessing cookies bypassing the
content settings. That's a huge problem in practice as cookies just worked
without any special code I could search for.

I'd propose to add a comment yo urlrequestcontext stating that you need a
network delegate in order to use cookies, maybe right next to the cookie
monster setter, wdyt

I think if someone finds their way to the comment, they'd be mostly done
anyways. The issue is that it's hard to get to either of these places for a
newcomer.

You'll get there once it crashes because you have no cookie monster.

I could also provide a default implementation of NetworkDelegate in net/
and just crash if there's no NetworkDelegate set.
https://chromiumcodereview.appspot.com/9701023/

[jochen (gone - plz use gerrit), 2012-03-15 07:08:24]

If you look at URLRequest (not delegate) you see that the default was
always false. And it was the declared target of the referenced bug to make
the default to block, so it's no accidential by-product

Jochen

[jochen (gone - plz use gerrit), 2012-03-15 08:24:29]

btw, content shell will need a more fancy cookie implementation anyway, to
support setting different cookie policies, see e.g. the TestShellNetworkDelegate
in simple_resource_loader_bridge.cc

[jam, 2012-03-15 14:58:48]

On Thu, Mar 15, 2012 at 1:24 AM, <jochen@chromium.org> wrote:

> btw, content shell will need a more fancy cookie implementation anyway, to
> support setting different cookie policies, see e.g. the
> TestShellNetworkDelegate
> in simple_resource_loader_bridge.**cc
>

My concern isn't really with content_shell, since that's a one-off which we
can easily fix. It's with every new embedder of content; i.e. how much work
they have to do.

On Thu, Mar 15, 2012 at 12:08 AM, Jochen Eisinger <jochen@chromium.org>
 wrote:

> If you look at URLRequest (not delegate) you see that the default was
> always false. And it was the declared target of the referenced bug to make
> the default to block, so it's no accidential by-product
>
For every embedder of content though, the default was to allow cookies
since the requests were either initiated by URLFetcher or
ResourceDispatcheHost, which were both URLRequest::Delegates and by default
allowed cookies.

On Wed, Mar 14, 2012 at 11:55 PM, Jochen Eisinger <jochen@chromium.org>wrote:

>
> On Mar 15, 2012 7:48 AM, <jam@chromium.org> wrote:
>
> On 2012/03/15 06:44:48, jochen wrote:
>
> The main advantage for me is that it's easier to audit - I can just search
> for all places that implement a network delegate.
>
> Is this a problem in practice? There's only one network delegate in chrome
> afaik. It seems just as easy to audit that the one in chrome does what we
> want
> by default.
>
> The problem is finding all the places accessing cookies bypassing the
> content settings. That's a huge problem in practice as cookies just worked
> without any special code I could search for.
>

When you say bypass content settings, all these still go through
ChromeNetworkDelegate though afaik. My point is that making the default of
net module to match chrome is unnecessary and adds more work to every
content embedder. They already go through a path where the default is no.
They don't need to go through two d

I'd propose to add a comment yo urlrequestcontext stating that you need a
> network delegate in order to use cookies, maybe right next to the cookie
> monster setter, wdyt
>
> I think if someone finds their way to the comment, they'd be mostly done
> anyways. The issue is that it's hard to get to either of these places for a
> newcomer.
>
> You'll get there once it crashes because you have no cookie monster.
>
> I could also provide a default implementation of NetworkDelegate in net/
> and just crash if there's no NetworkDelegate set.
> https://chromiumcodereview.appspot.com/9701023/
>

[jochen (gone - plz use gerrit), 2012-03-15 16:20:07]

On Thu, Mar 15, 2012 at 3:58 PM, John Abd-El-Malek <jam@chromium.org> wrote:

>
> On Thu, Mar 15, 2012 at 1:24 AM, <jochen@chromium.org> wrote:
>
> btw, content shell will need a more fancy cookie implementation anyway, to
>> support setting different cookie policies, see e.g. the
>> TestShellNetworkDelegate
>> in simple_resource_loader_bridge.**cc
>>
>
> My concern isn't really with content_shell, since that's a one-off which
> we can easily fix. It's with every new embedder of content; i.e. how much
> work they have to do.
>
> On Thu, Mar 15, 2012 at 12:08 AM, Jochen Eisinger <jochen@chromium.org>
>  wrote:
>
> If you look at URLRequest (not delegate) you see that the default was
>> always false. And it was the declared target of the referenced bug to make
>> the default to block, so it's no accidential by-product
>>
> For every embedder of content though, the default was to allow cookies
> since the requests were either initiated by URLFetcher or
> ResourceDispatcheHost, which were both URLRequest::Delegates and by default
> allowed cookies.
>

Sadly, that's not true. e.g. webkit/appcache uses raw URLRequests


>
> On Wed, Mar 14, 2012 at 11:55 PM, Jochen Eisinger <jochen@chromium.org>wrote:
>
>>
>> On Mar 15, 2012 7:48 AM, <jam@chromium.org> wrote:
>>
>> On 2012/03/15 06:44:48, jochen wrote:
>>
>> The main advantage for me is that it's easier to audit - I can just search
>> for all places that implement a network delegate.
>>
>> Is this a problem in practice? There's only one network delegate in chrome
>> afaik. It seems just as easy to audit that the one in chrome does what we
>> want
>> by default.
>>
>> The problem is finding all the places accessing cookies bypassing the
>> content settings. That's a huge problem in practice as cookies just worked
>> without any special code I could search for.
>>
>
> When you say bypass content settings, all these still go through
> ChromeNetworkDelegate though afaik. My point is that making the default of
> net module to match chrome is unnecessary and adds more work to every
> content embedder. They already go through a path where the default is no.
> They don't need to go through two d
>

Only if they use one of our URLRequestContexts defined on io_thread or one
of the profiles.

Again, that's not true for all places in chrome/ some sync code uses their
own URLRequestContexts that don't have the ChromeNetworkDelegate


>
> I'd propose to add a comment yo urlrequestcontext stating that you need a
>> network delegate in order to use cookies, maybe right next to the cookie
>> monster setter, wdyt
>>
>> I think if someone finds their way to the comment, they'd be mostly done
>> anyways. The issue is that it's hard to get to either of these places for
>> a
>> newcomer.
>>
>> You'll get there once it crashes because you have no cookie monster.
>>
>> I could also provide a default implementation of NetworkDelegate in net/
>> and just crash if there's no NetworkDelegate set.
>> https://chromiumcodereview.appspot.com/9701023/
>>
>
>

[jam, 2012-03-26 20:19:10]

lgtm with moving the delegate to a separate .h/.cc

[commit-bot: I haz the power, 2012-03-28 19:05:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jochen@chromium.org/9701023/9001

[commit-bot: I haz the power, 2012-03-28 22:11:23]

Try job failure for 9701023-9001 (retry) on win_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...