Add enableReferrers and enableHyperlinkAuditing to contentSettings.misc API.

Add enableReferrers and enableHyperlinkAuditing to contentSettings.misc extension API. 

BUG=71067
TEST=ExtensionApiTest.*

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

[Bernhard Bauer, 2011-03-29 16:29:07]

Please review (jochen: extension stuff, john: content stuff).

[jam, 2011-03-29 18:17:16]

http://codereview.chromium.org/6773006/diff/2001/chrome/browser/renderer_host...
File chrome/browser/renderer_host/browser_render_process_host.h (right):

http://codereview.chromium.org/6773006/diff/2001/chrome/browser/renderer_host...
chrome/browser/renderer_host/browser_render_process_host.h:217: // Weak, owned
by |channel_|.
what happens if OnChannelError() is called?  channel_ will be reset and this
filter will be deleted.  so if you want to hold a pointer to it, use a
scoped_refptr

http://codereview.chromium.org/6773006/diff/2001/content/browser/renderer_hos...
File content/browser/renderer_host/resource_message_filter.cc (right):

http://codereview.chromium.org/6773006/diff/2001/content/browser/renderer_hos...
content/browser/renderer_host/resource_message_filter.cc:9: #include
"chrome/common/pref_names.h"
this isn't needed

http://codereview.chromium.org/6773006/diff/2001/content/browser/renderer_hos...
File content/browser/renderer_host/resource_message_filter.h (right):

http://codereview.chromium.org/6773006/diff/2001/content/browser/renderer_hos...
content/browser/renderer_host/resource_message_filter.h:63: bool
referrers_enabled() { return enableReferrers_; }
nit: const

http://codereview.chromium.org/6773006/diff/2001/content/browser/renderer_hos...
content/browser/renderer_host/resource_message_filter.h:83: bool
enableReferrers_;
camel caps for member variables, i.e. enable_referrers_.  same for the parameter
name for the setter

[Bernhard Bauer, 2011-03-29 19:26:57]

http://codereview.chromium.org/6773006/diff/2001/chrome/browser/renderer_host...
File chrome/browser/renderer_host/browser_render_process_host.h (right):

http://codereview.chromium.org/6773006/diff/2001/chrome/browser/renderer_host...
chrome/browser/renderer_host/browser_render_process_host.h:217: // Weak, owned
by |channel_|.
On 2011/03/29 18:17:16, John Abd-El-Malek wrote:
> what happens if OnChannelError() is called?  channel_ will be reset and this
> filter will be deleted.  so if you want to hold a pointer to it, use a
> scoped_refptr

I guess I should set the pointer to NULL in OnChannelError() then? I don't want
to hold on to the old ResourceMessageFilter after the channel is closed.

[jam, 2011-03-29 21:04:22]

On 2011/03/29 19:26:57, Bernhard Bauer wrote:
>
http://codereview.chromium.org/6773006/diff/2001/chrome/browser/renderer_host...
> File chrome/browser/renderer_host/browser_render_process_host.h (right):
> 
>
http://codereview.chromium.org/6773006/diff/2001/chrome/browser/renderer_host...
> chrome/browser/renderer_host/browser_render_process_host.h:217: // Weak, owned
> by |channel_|.
> On 2011/03/29 18:17:16, John Abd-El-Malek wrote:
> > what happens if OnChannelError() is called?  channel_ will be reset and this
> > filter will be deleted.  so if you want to hold a pointer to it, use a
> > scoped_refptr
> 
> I guess I should set the pointer to NULL in OnChannelError() then? I don't
want
> to hold on to the old ResourceMessageFilter after the channel is closed.

Thinking about it more, we really should avoid having to hold on to the
ResourceMessageFilter in BrowserRenderProcessHost.  RMF lives on the IO thread,
and BRPH lives on the UI thread.  Storing pointer from BRPH to RMF and using
locks to guard it is unwieldy.  Also watching the pref on the IO thread, and
keeping the pointer, strikes me as something that should be avoided.

Instead of storing this in the browser, and sending a referrer in the renderer
that then gets thrown out, how about just storing this in the renderer?  BRPH
can send a message when this pref changes, and either RD or another object can
store it.

[jochen (gone - plz use gerrit), 2011-03-30 08:29:10]

will you add some extension api tests for this?

will it be possible to e.g. disable referrers only for incognito profiles?

http://codereview.chromium.org/6773006/diff/2001/chrome/common/extensions/api...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/6773006/diff/2001/chrome/common/extensions/api...
chrome/common/extensions/api/extension_api.json:4785: "value": ["enablePing",
{"type":"boolean"}],
I think enabledHyperlinkAuditing would be a better name

[Bernhard Bauer, 2011-03-30 10:00:03]

On Wed, Mar 30, 2011 at 10:29,  <jochen@chromium.org> wrote:
> will you add some extension api tests for this?
>
> will it be possible to e.g. disable referrers only for incognito profiles?

It uses the standard preference API for extensions, so assuming that
the BrowserRenderProcessHost is hooked up to the OffTheRecordProfile,
it should all work automatically.

>
>
http://codereview.chromium.org/6773006/diff/2001/chrome/common/extensions/api...
> File chrome/common/extensions/api/extension_api.json (right):
>
>
http://codereview.chromium.org/6773006/diff/2001/chrome/common/extensions/api...
> chrome/common/extensions/api/extension_api.json:4785: "value":
> ["enablePing", {"type":"boolean"}],
> I think enabledHyperlinkAuditing would be a better name
>
> http://codereview.chromium.org/6773006/
>

[Bernhard Bauer, 2011-03-30 18:41:39]

On Tue, Mar 29, 2011 at 23:04,  <jam@chromium.org> wrote:
> On 2011/03/29 19:26:57, Bernhard Bauer wrote:
>
>
http://codereview.chromium.org/6773006/diff/2001/chrome/browser/renderer_host...
>>
>> File chrome/browser/renderer_host/browser_render_process_host.h (right):
>
>
>
http://codereview.chromium.org/6773006/diff/2001/chrome/browser/renderer_host...
>>
>> chrome/browser/renderer_host/browser_render_process_host.h:217: // Weak,
>> owned
>> by |channel_|.
>> On 2011/03/29 18:17:16, John Abd-El-Malek wrote:
>> > what happens if OnChannelError() is called?  channel_ will be reset and
>> > this
>> > filter will be deleted.  so if you want to hold a pointer to it, use a
>> > scoped_refptr
>
>> I guess I should set the pointer to NULL in OnChannelError() then? I don't
>
> want
>>
>> to hold on to the old ResourceMessageFilter after the channel is closed.
>
> Thinking about it more, we really should avoid having to hold on to the
> ResourceMessageFilter in BrowserRenderProcessHost.  RMF lives on the IO
> thread,
> and BRPH lives on the UI thread.  Storing pointer from BRPH to RMF and using
> locks to guard it is unwieldy.  Also watching the pref on the IO thread, and
> keeping the pointer, strikes me as something that should be avoided.
>
> Instead of storing this in the browser, and sending a referrer in the
> renderer
> that then gets thrown out, how about just storing this in the renderer?
>  BRPH
> can send a message when this pref changes, and either RD or another object
> can
> store it.

Hm, we already have (in this CL) an additional enable_referrers flag
in RendererPreferences (I figured referrers are not a chrome-specific
feature), and some debugging seems to imply that
RenderView::willSendRequest already gets to see the request (and
strips referrers), so doing the same thing in ResourceDispatcher might
not even be necessary. Of course, I don't know that much about the
request loading mechanisms; I basically just tried to replace all
occurrences of
|CommandLine::ForCurrentProcess()->HasSwitch(switches::kNoReferrers)|
with something which reads from preferences.

> http://codereview.chromium.org/6773006/
>

[Bernhard Bauer, 2011-04-04 17:51:58]

OK, I now strip referrers in the renderer in RenderView (for WebKit) and in the
browser in ChromeNetworkDelegate (for requests which are not directly coming
from the renderer, like downloads). 

John, Jochen, could you take another look?

[jam, 2011-04-04 23:36:32]

lgtm but i'm not familiar with the browser/prefs code, so please have someone
more familiar with the API design of it look those changes over.  It seems that
prefs and profile are peers, and they shouldn't have to know about each other.

http://codereview.chromium.org/6773006/diff/12025/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_content_settings_apitest.cc (right):

http://codereview.chromium.org/6773006/diff/12025/chrome/browser/extensions/e...
chrome/browser/extensions/extension_content_settings_apitest.cc:27:
EXPECT_TRUE(pref_service->GetBoolean(prefs::kEnableReferrers));
nit: what is this testing?

[Bernhard Bauer, 2011-04-05 07:26:14]

On 2011/04/04 23:36:32, John Abd-El-Malek wrote:
> lgtm but i'm not familiar with the browser/prefs code, so please have someone
> more familiar with the API design of it look those changes over.  It seems
that
> prefs and profile are peers, and they shouldn't have to know about each other.

Mattias, could you take a look?

http://codereview.chromium.org/6773006/diff/12025/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_content_settings_apitest.cc (right):

http://codereview.chromium.org/6773006/diff/12025/chrome/browser/extensions/e...
chrome/browser/extensions/extension_content_settings_apitest.cc:27:
EXPECT_TRUE(pref_service->GetBoolean(prefs::kEnableReferrers));
On 2011/04/04 23:36:32, John Abd-El-Malek wrote:
> nit: what is this testing?

The test extension reads the preference value and tests that it is false, then
sets it to true, and we test it here.

[jochen (gone - plz use gerrit), 2011-04-05 07:47:30]

extension bits LGTM

for the network delegate, I'd propose to add willchan

[Mattias Nissler (ping if slow), 2011-04-05 08:48:27]

LG, just some nits I found while looking at chrome/browser/prefs/* and friends.

http://codereview.chromium.org/6773006/diff/12025/chrome/browser/prefs/pref_m...
File chrome/browser/prefs/pref_member.cc (right):

http://codereview.chromium.org/6773006/diff/12025/chrome/browser/prefs/pref_m...
chrome/browser/prefs/pref_member.cc:77: }
No need for curlies (or make it consistent with the NOTREACHED below?)

http://codereview.chromium.org/6773006/diff/12025/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_io_data.h (right):

http://codereview.chromium.org/6773006/diff/12025/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_io_data.h:117: scoped_ptr<BooleanPrefMember>
enable_referrers;
Any reason this can't be a regular data member, Destroy()ing it when done?

http://codereview.chromium.org/6773006/diff/12025/chrome/common/pref_names.h
File chrome/common/pref_names.h (right):

http://codereview.chromium.org/6773006/diff/12025/chrome/common/pref_names.h#...
chrome/common/pref_names.h:475: extern const char kEnableReferrers[];
Note that the lower part of this file is for local state prefs, while profile
prefs go in the top part. I always tell people, but nobody seems to care :(

[Bernhard Bauer, 2011-04-05 10:48:34]

http://codereview.chromium.org/6773006/diff/12025/chrome/browser/prefs/pref_m...
File chrome/browser/prefs/pref_member.cc (right):

http://codereview.chromium.org/6773006/diff/12025/chrome/browser/prefs/pref_m...
chrome/browser/prefs/pref_member.cc:77: }
On 2011/04/05 08:48:27, Mattias Nissler wrote:
> No need for curlies (or make it consistent with the NOTREACHED below?)

Done.

http://codereview.chromium.org/6773006/diff/12025/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_io_data.h (right):

http://codereview.chromium.org/6773006/diff/12025/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_io_data.h:117: scoped_ptr<BooleanPrefMember>
enable_referrers;
On 2011/04/05 08:48:27, Mattias Nissler wrote:
> Any reason this can't be a regular data member, Destroy()ing it when done?

I initialize it on the UI thread, and then pass it to the ChromeNetworkDelegate,
which accesses it from the IO thread, so I need to pass the pointer around.

http://codereview.chromium.org/6773006/diff/12025/chrome/common/pref_names.h
File chrome/common/pref_names.h (right):

http://codereview.chromium.org/6773006/diff/12025/chrome/common/pref_names.h#...
chrome/common/pref_names.h:475: extern const char kEnableReferrers[];
On 2011/04/05 08:48:27, Mattias Nissler wrote:
> Note that the lower part of this file is for local state prefs, while profile
> prefs go in the top part. I always tell people, but nobody seems to care :(

:-( I didn't realize that. Added a comment to make it a bit clearer.

[Mattias Nissler (ping if slow), 2011-04-05 10:57:46]

Pref stuff LGTM (with a nit).

http://codereview.chromium.org/6773006/diff/12025/chrome/common/pref_names.h
File chrome/common/pref_names.h (right):

http://codereview.chromium.org/6773006/diff/12025/chrome/common/pref_names.h#...
chrome/common/pref_names.h:475: extern const char kEnableReferrers[];
On 2011/04/05 10:48:34, Bernhard Bauer wrote:
> On 2011/04/05 08:48:27, Mattias Nissler wrote:
> > Note that the lower part of this file is for local state prefs, while
profile
> > prefs go in the top part. I always tell people, but nobody seems to care :(
> 
> :-( I didn't realize that. Added a comment to make it a bit clearer.

Thanks. I guess the state it's currently makes it very hard to figure out what's
the right thing. I filed a bug to clean this up:
http://code.google.com/p/chromium/issues/detail?id=78415

http://codereview.chromium.org/6773006/diff/14026/chrome/common/pref_names.h#...
chrome/common/pref_names.h:15: // Profile prefs
Grammar.

[Mattias Nissler (ping if slow), 2011-04-05 10:57:47]

Pref stuff LGTM (with a nit).

http://codereview.chromium.org/6773006/diff/12025/chrome/common/pref_names.h
File chrome/common/pref_names.h (right):

http://codereview.chromium.org/6773006/diff/12025/chrome/common/pref_names.h#...
chrome/common/pref_names.h:475: extern const char kEnableReferrers[];
On 2011/04/05 10:48:34, Bernhard Bauer wrote:
> On 2011/04/05 08:48:27, Mattias Nissler wrote:
> > Note that the lower part of this file is for local state prefs, while
profile
> > prefs go in the top part. I always tell people, but nobody seems to care :(
> 
> :-( I didn't realize that. Added a comment to make it a bit clearer.

Thanks. I guess the state it's currently makes it very hard to figure out what's
the right thing. I filed a bug to clean this up:
http://code.google.com/p/chromium/issues/detail?id=78415

http://codereview.chromium.org/6773006/diff/14026/chrome/common/pref_names.h#...
chrome/common/pref_names.h:15: // Profile prefs
Grammar.

[Bernhard Bauer, 2011-04-05 11:34:31]

On 2011/04/05 07:47:30, jochen wrote:
> extension bits LGTM
> 
> for the network delegate, I'd propose to add willchan

Will, could you take a look at the network delegate part?

http://codereview.chromium.org/6773006/diff/14026/chrome/common/pref_names.h
File chrome/common/pref_names.h (right):

http://codereview.chromium.org/6773006/diff/14026/chrome/common/pref_names.h#...
chrome/common/pref_names.h:15: // Profile prefs. Please Local State prefs below
instead.
On 2011/04/05 10:57:51, Mattias Nissler wrote:
> Grammar.

Whoops, forgot to save.

[willchan no longer on Chromium, 2011-04-05 13:10:40]

http://codereview.chromium.org/6773006/diff/12028/chrome/browser/net/chrome_n...
File chrome/browser/net/chrome_network_delegate.h (right):

http://codereview.chromium.org/6773006/diff/12028/chrome/browser/net/chrome_n...
chrome/browser/net/chrome_network_delegate.h:26: // PrefService and moved to the
IO thread. It can be NULL. This object will
Why are you allowing it to be NULL? Does it crash otherwise in tests? If
possible, it'd be nice if it were always non-NULL and you added a DCHECK to
enforce this. It makes new readers of the code have to think less and avoids
having to do NULL checks everywhere.

OIC why, it looks like you are passing in NULL for the system
ChromeNetworkDelegate. Is that really what you want to do? Should we be setting
the referer for system requests if an extension disables them?

http://codereview.chromium.org/6773006/diff/12028/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_io_data.cc (right):

http://codereview.chromium.org/6773006/diff/12028/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_io_data.cc:137:
params->enable_referrers->ObserveProfileDestruction(profile);
Why is this necessary? The associated ProfileIOData / ChromeNetworkDelegate
should die soon after the Profile dies.

http://codereview.chromium.org/6773006/diff/12028/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_io_data.h (right):

http://codereview.chromium.org/6773006/diff/12028/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_io_data.h:116: // Initialized on the UI thread.
No need for comment. Everything in this struct has to be initialized on the UI
thread. See the comment above.

[Bernhard Bauer, 2011-04-05 14:49:02]

Thanks for the review!

http://codereview.chromium.org/6773006/diff/12028/chrome/browser/net/chrome_n...
File chrome/browser/net/chrome_network_delegate.h (right):

http://codereview.chromium.org/6773006/diff/12028/chrome/browser/net/chrome_n...
chrome/browser/net/chrome_network_delegate.h:26: // PrefService and moved to the
IO thread. It can be NULL. This object will
On 2011/04/05 13:10:41, willchan wrote:
> Why are you allowing it to be NULL? Does it crash otherwise in tests? If
> possible, it'd be nice if it were always non-NULL and you added a DCHECK to
> enforce this. It makes new readers of the code have to think less and avoids
> having to do NULL checks everywhere.
> 
> OIC why, it looks like you are passing in NULL for the system
> ChromeNetworkDelegate. Is that really what you want to do? Should we be
setting
> the referer for system requests if an extension disables them?

Yeah, I thought about adding support for the pref in the system network delegate
as well, I just decided against doing it right now, as the preference is not
registered in Local State, and there is no client which would set it either. If
you think it's worthwhile though, I can re-add it.

It's also somewhat unlikely that not stripping the referrer from system requests
is going to leak information, as the referrers would probably have to be
explicitly specified anyway.

http://codereview.chromium.org/6773006/diff/12028/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_io_data.cc (right):

http://codereview.chromium.org/6773006/diff/12028/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_io_data.cc:137:
params->enable_referrers->ObserveProfileDestruction(profile);
On 2011/04/05 13:10:41, willchan wrote:
> Why is this necessary? The associated ProfileIOData / ChromeNetworkDelegate
> should die soon after the Profile dies.

Sadly, not soon enough. When the PrefMember is destroyed, it has to be
unregistered from observing pref changes, which has to happen a) on the UI
thread, and b) before the PrefService is destroyed (which is not guaranteed to
be after the ChromeNetworkDelegate is destroyed).

http://codereview.chromium.org/6773006/diff/12028/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_io_data.h (right):

http://codereview.chromium.org/6773006/diff/12028/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_io_data.h:116: // Initialized on the UI thread.
On 2011/04/05 13:10:41, willchan wrote:
> No need for comment. Everything in this struct has to be initialized on the UI
> thread. See the comment above.

Easy enough :-) Done.

[willchan no longer on Chromium, 2011-04-05 15:06:18]

http://codereview.chromium.org/6773006/diff/12028/chrome/browser/net/chrome_n...
File chrome/browser/net/chrome_network_delegate.h (right):

http://codereview.chromium.org/6773006/diff/12028/chrome/browser/net/chrome_n...
chrome/browser/net/chrome_network_delegate.h:26: // PrefService and moved to the
IO thread. It can be NULL. This object will
On 2011/04/05 14:49:02, Bernhard Bauer wrote:
> On 2011/04/05 13:10:41, willchan wrote:
> > Why are you allowing it to be NULL? Does it crash otherwise in tests? If
> > possible, it'd be nice if it were always non-NULL and you added a DCHECK to
> > enforce this. It makes new readers of the code have to think less and avoids
> > having to do NULL checks everywhere.
> > 
> > OIC why, it looks like you are passing in NULL for the system
> > ChromeNetworkDelegate. Is that really what you want to do? Should we be
> setting
> > the referer for system requests if an extension disables them?
> 
> Yeah, I thought about adding support for the pref in the system network
delegate
> as well, I just decided against doing it right now, as the preference is not
> registered in Local State, and there is no client which would set it either.
If
> you think it's worthwhile though, I can re-add it.

I think I'd prefer this, but I don't feel too strongly. I mostly like avoiding
NULL checks where possible since people often forget when they are necessary.

> 
> It's also somewhat unlikely that not stripping the referrer from system
requests
> is going to leak information, as the referrers would probably have to be
> explicitly specified anyway.

Heh, you never know :) What if we have a DomUI page for configuring system
settings (profile-agnostic), and it uses XHR? That would probably be a
non-profile associated URLRequestContext that might have a referrer.

http://codereview.chromium.org/6773006/diff/12028/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_io_data.cc (right):

http://codereview.chromium.org/6773006/diff/12028/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_io_data.cc:137:
params->enable_referrers->ObserveProfileDestruction(profile);
On 2011/04/05 14:49:02, Bernhard Bauer wrote:
> On 2011/04/05 13:10:41, willchan wrote:
> > Why is this necessary? The associated ProfileIOData / ChromeNetworkDelegate
> > should die soon after the Profile dies.
> 
> Sadly, not soon enough. When the PrefMember is destroyed, it has to be
> unregistered from observing pref changes, which has to happen a) on the UI
> thread, and b) before the PrefService is destroyed (which is not guaranteed to
> be after the ChromeNetworkDelegate is destroyed).

Wait, but this BooleanPrefMember lives in the ChromeNetworkDelegate which lives
on the IO thread. We'd accessing this without locks? Is this safe? It seems a
bit dodgy.

[Bernhard Bauer, 2011-04-05 16:12:58]

http://codereview.chromium.org/6773006/diff/12028/chrome/browser/net/chrome_n...
File chrome/browser/net/chrome_network_delegate.h (right):

http://codereview.chromium.org/6773006/diff/12028/chrome/browser/net/chrome_n...
chrome/browser/net/chrome_network_delegate.h:26: // PrefService and moved to the
IO thread. It can be NULL. This object will
On 2011/04/05 15:06:18, willchan wrote:
> On 2011/04/05 14:49:02, Bernhard Bauer wrote:
> > On 2011/04/05 13:10:41, willchan wrote:
> > > Why are you allowing it to be NULL? Does it crash otherwise in tests? If
> > > possible, it'd be nice if it were always non-NULL and you added a DCHECK
to
> > > enforce this. It makes new readers of the code have to think less and
avoids
> > > having to do NULL checks everywhere.
> > > 
> > > OIC why, it looks like you are passing in NULL for the system
> > > ChromeNetworkDelegate. Is that really what you want to do? Should we be
> > setting
> > > the referer for system requests if an extension disables them?
> > 
> > Yeah, I thought about adding support for the pref in the system network
> delegate
> > as well, I just decided against doing it right now, as the preference is not
> > registered in Local State, and there is no client which would set it either.
> If
> > you think it's worthwhile though, I can re-add it.
> 
> I think I'd prefer this, but I don't feel too strongly. I mostly like avoiding
> NULL checks where possible since people often forget when they are necessary.
> 
> > 
> > It's also somewhat unlikely that not stripping the referrer from system
> requests
> > is going to leak information, as the referrers would probably have to be
> > explicitly specified anyway.
> 
> Heh, you never know :) What if we have a DomUI page for configuring system
> settings (profile-agnostic), and it uses XHR? That would probably be a
> non-profile associated URLRequestContext that might have a referrer.

Fair enough. Done.

http://codereview.chromium.org/6773006/diff/12028/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_io_data.cc (right):

http://codereview.chromium.org/6773006/diff/12028/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_io_data.cc:137:
params->enable_referrers->ObserveProfileDestruction(profile);
On 2011/04/05 15:06:18, willchan wrote:
> On 2011/04/05 14:49:02, Bernhard Bauer wrote:
> > On 2011/04/05 13:10:41, willchan wrote:
> > > Why is this necessary? The associated ProfileIOData /
ChromeNetworkDelegate
> > > should die soon after the Profile dies.
> > 
> > Sadly, not soon enough. When the PrefMember is destroyed, it has to be
> > unregistered from observing pref changes, which has to happen a) on the UI
> > thread, and b) before the PrefService is destroyed (which is not guaranteed
to
> > be after the ChromeNetworkDelegate is destroyed).
> 
> Wait, but this BooleanPrefMember lives in the ChromeNetworkDelegate which
lives
> on the IO thread. We'd accessing this without locks? Is this safe? It seems a
> bit dodgy.

We specifically added support for reading prefs without locks on other threads,
by observing pref changes on the UI thread and then updating the cached pref
value on the IO thread.

[willchan no longer on Chromium, 2011-04-05 17:12:51]

http://codereview.chromium.org/6773006/diff/12028/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_io_data.cc (right):

http://codereview.chromium.org/6773006/diff/12028/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_io_data.cc:137:
params->enable_referrers->ObserveProfileDestruction(profile);
On 2011/04/05 16:12:59, Bernhard Bauer wrote:
> On 2011/04/05 15:06:18, willchan wrote:
> > On 2011/04/05 14:49:02, Bernhard Bauer wrote:
> > > On 2011/04/05 13:10:41, willchan wrote:
> > > > Why is this necessary? The associated ProfileIOData /
> ChromeNetworkDelegate
> > > > should die soon after the Profile dies.
> > > 
> > > Sadly, not soon enough. When the PrefMember is destroyed, it has to be
> > > unregistered from observing pref changes, which has to happen a) on the UI
> > > thread, and b) before the PrefService is destroyed (which is not
guaranteed
> to
> > > be after the ChromeNetworkDelegate is destroyed).

Which PrefService? Sorry, there seem to be a whole bunch and the shutdown
ordering is not obvious to me.

> > 
> > Wait, but this BooleanPrefMember lives in the ChromeNetworkDelegate which
> lives
> > on the IO thread. We'd accessing this without locks? Is this safe? It seems
a
> > bit dodgy.
> 
> We specifically added support for reading prefs without locks on other
threads,
> by observing pref changes on the UI thread and then updating the cached pref
> value on the IO thread. 

I see, sounds great.

I see that the PROFILE_DESTROYED notification is issued at the start of
~OffTheRecordProfile and ~ProfileImpl. What guarantee is there that this will
hold true? Is this documented somewhere? What happens if a profile releases the
last reference to ProfileIOData before this notification is issued? Currently
the reference is held as a member of a Profile subclass, so it should be
destroyed after the destructor body has run. We don't have any tests enforcing
any ordering, nor any comments. If you're adding an ordering dependency, some
sort of enforcement would be nice.

[Bernhard Bauer, 2011-04-05 18:24:47]

http://codereview.chromium.org/6773006/diff/12028/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_io_data.cc (right):

http://codereview.chromium.org/6773006/diff/12028/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_io_data.cc:137:
params->enable_referrers->ObserveProfileDestruction(profile);
On 2011/04/05 17:12:51, willchan wrote:
> On 2011/04/05 16:12:59, Bernhard Bauer wrote:
> > On 2011/04/05 15:06:18, willchan wrote:
> > > On 2011/04/05 14:49:02, Bernhard Bauer wrote:
> > > > On 2011/04/05 13:10:41, willchan wrote:
> > > > > Why is this necessary? The associated ProfileIOData /
> > ChromeNetworkDelegate
> > > > > should die soon after the Profile dies.
> > > > 
> > > > Sadly, not soon enough. When the PrefMember is destroyed, it has to be
> > > > unregistered from observing pref changes, which has to happen a) on the
UI
> > > > thread, and b) before the PrefService is destroyed (which is not
> guaranteed
> > to
> > > > be after the ChromeNetworkDelegate is destroyed).
> 
> Which PrefService? Sorry, there seem to be a whole bunch and the shutdown
> ordering is not obvious to me.

The one returned by |GetPrefs()| (which is the incognito PrefService for an
OffTheRecordProfile and the regular one for a regular ProfileImpl), and it is
destroyed via the ProfileManager in the destructor of BrowserProcessImpl (before
the IO thread is stopped in the same destructor).

> > > 
> > > Wait, but this BooleanPrefMember lives in the ChromeNetworkDelegate which
> > lives
> > > on the IO thread. We'd accessing this without locks? Is this safe? It
seems
> a
> > > bit dodgy.
> > 
> > We specifically added support for reading prefs without locks on other
> threads,
> > by observing pref changes on the UI thread and then updating the cached pref
> > value on the IO thread. 
> 
> I see, sounds great.
> 
> I see that the PROFILE_DESTROYED notification is issued at the start of
> ~OffTheRecordProfile and ~ProfileImpl. What guarantee is there that this will
> hold true? 

I guess none apart from the fact that probably a lot of other stuff will start
failing if we don't send the notification ;-)

I discussed this with Mattias and we agreed that in this case it's probably
easier to watch for a notification than to explicitly |Destroy()| the
PrefMember, as it is pretty hidden in the ChromeNetworkDelegate.

> Is this documented somewhere? What happens if a profile releases the
> last reference to ProfileIOData before this notification is issued? Currently
> the reference is held as a member of a Profile subclass, so it should be
> destroyed after the destructor body has run. 

So we should be fine at the moment, right?

> We don't have any tests enforcing
> any ordering, nor any comments. If you're adding an ordering dependency, some
> sort of enforcement would be nice.

Would you be okay with me just adding a comment which states that the
notification should be sent before the other stuff is destroyed? The
use-after-free we'd possibly get when getting it wrong would probably be hard to
reliably test for :-/

[Bernhard Bauer, 2011-04-08 12:54:05]

On Tue, Apr 5, 2011 at 20:24,  <bauerb@chromium.org> wrote:
>
>
http://codereview.chromium.org/6773006/diff/12028/chrome/browser/profiles/pro...
> File chrome/browser/profiles/profile_io_data.cc (right):
>
>
http://codereview.chromium.org/6773006/diff/12028/chrome/browser/profiles/pro...
> chrome/browser/profiles/profile_io_data.cc:137:
> params->enable_referrers->ObserveProfileDestruction(profile);
> On 2011/04/05 17:12:51, willchan wrote:
>>
>> On 2011/04/05 16:12:59, Bernhard Bauer wrote:
>> > On 2011/04/05 15:06:18, willchan wrote:
>> > > On 2011/04/05 14:49:02, Bernhard Bauer wrote:
>> > > > On 2011/04/05 13:10:41, willchan wrote:
>> > > > > Why is this necessary? The associated ProfileIOData /
>> > ChromeNetworkDelegate
>> > > > > should die soon after the Profile dies.
>> > > >
>> > > > Sadly, not soon enough. When the PrefMember is destroyed, it has
>
> to be
>>
>> > > > unregistered from observing pref changes, which has to happen a)
>
> on the UI
>>
>> > > > thread, and b) before the PrefService is destroyed (which is not
>> guaranteed
>> > to
>> > > > be after the ChromeNetworkDelegate is destroyed).
>
>> Which PrefService? Sorry, there seem to be a whole bunch and the
>
> shutdown
>>
>> ordering is not obvious to me.
>
> The one returned by |GetPrefs()| (which is the incognito PrefService for
> an OffTheRecordProfile and the regular one for a regular ProfileImpl),
> and it is destroyed via the ProfileManager in the destructor of
> BrowserProcessImpl (before the IO thread is stopped in the same
> destructor).
>
>> > >
>> > > Wait, but this BooleanPrefMember lives in the
>
> ChromeNetworkDelegate which
>>
>> > lives
>> > > on the IO thread. We'd accessing this without locks? Is this safe?
>
> It seems
>>
>> a
>> > > bit dodgy.
>> >
>> > We specifically added support for reading prefs without locks on
>
> other
>>
>> threads,
>> > by observing pref changes on the UI thread and then updating the
>
> cached pref
>>
>> > value on the IO thread.
>
>> I see, sounds great.
>
>> I see that the PROFILE_DESTROYED notification is issued at the start
>
> of
>>
>> ~OffTheRecordProfile and ~ProfileImpl. What guarantee is there that
>
> this will
>>
>> hold true?
>
> I guess none apart from the fact that probably a lot of other stuff will
> start failing if we don't send the notification ;-)
>
> I discussed this with Mattias and we agreed that in this case it's
> probably easier to watch for a notification than to explicitly
> |Destroy()| the PrefMember, as it is pretty hidden in the
> ChromeNetworkDelegate.
>
>> Is this documented somewhere? What happens if a profile releases the
>> last reference to ProfileIOData before this notification is issued?
>
> Currently
>>
>> the reference is held as a member of a Profile subclass, so it should
>
> be
>>
>> destroyed after the destructor body has run.
>
> So we should be fine at the moment, right?
>
>> We don't have any tests enforcing
>> any ordering, nor any comments. If you're adding an ordering
>
> dependency, some
>>
>> sort of enforcement would be nice.
>
> Would you be okay with me just adding a comment which states that the
> notification should be sent before the other stuff is destroyed? The
> use-after-free we'd possibly get when getting it wrong would probably be
> hard to reliably test for :-/

Ping? I added comments to the destructors of ProfileImpl and
OffTheRecordProfileImpl right before they send the notification,
explaining that this should happen first thing.

I realize that the shutdown code is pretty fragile (to say the least;
your recent mail to chromium-dev pretty much hit the nail on the
head), but I'm not sure how I could otherwise improve the situation
here.

Bernhard.

> http://codereview.chromium.org/6773006/
>

[willchan no longer on Chromium, 2011-04-08 19:10:36]

http://codereview.chromium.org/6773006/diff/12028/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_io_data.cc (right):

http://codereview.chromium.org/6773006/diff/12028/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_io_data.cc:137:
params->enable_referrers->ObserveProfileDestruction(profile);
On 2011/04/05 18:24:47, Bernhard Bauer wrote:
> On 2011/04/05 17:12:51, willchan wrote:
> > On 2011/04/05 16:12:59, Bernhard Bauer wrote:
> > > On 2011/04/05 15:06:18, willchan wrote:
> > > > On 2011/04/05 14:49:02, Bernhard Bauer wrote:
> > > > > On 2011/04/05 13:10:41, willchan wrote:
> > > > > > Why is this necessary? The associated ProfileIOData /
> > > ChromeNetworkDelegate
> > > > > > should die soon after the Profile dies.
> > > > > 
> > > > > Sadly, not soon enough. When the PrefMember is destroyed, it has to be
> > > > > unregistered from observing pref changes, which has to happen a) on
the
> UI
> > > > > thread, and b) before the PrefService is destroyed (which is not
> > guaranteed
> > > to
> > > > > be after the ChromeNetworkDelegate is destroyed).

I don't think I like adding implicit notifications like this. First off, it's
dependent on the Profile. It's possible that a good chunk of this code will move
into content/ which doesn't know about Chrome profiles. I prefer explicit
shutdown notifications. I'd rather you add a Shutdown member function to
ProfileImplIOData::Handle(), which calls ProfileImplIOData::Shutdown(), which
notifies objects as necessary. This way, if we need to forward such shutdown
notifications to the content/ code, we won't have to hook the Profile destroyed
notification.

> > 
> > Which PrefService? Sorry, there seem to be a whole bunch and the shutdown
> > ordering is not obvious to me.
> 
> The one returned by |GetPrefs()| (which is the incognito PrefService for an
> OffTheRecordProfile and the regular one for a regular ProfileImpl), and it is
> destroyed via the ProfileManager in the destructor of BrowserProcessImpl
(before
> the IO thread is stopped in the same destructor).
> 
> > > > 
> > > > Wait, but this BooleanPrefMember lives in the ChromeNetworkDelegate
which
> > > lives
> > > > on the IO thread. We'd accessing this without locks? Is this safe? It
> seems
> > a
> > > > bit dodgy.
> > > 
> > > We specifically added support for reading prefs without locks on other
> > threads,
> > > by observing pref changes on the UI thread and then updating the cached
pref
> > > value on the IO thread. 
> > 
> > I see, sounds great.
> > 
> > I see that the PROFILE_DESTROYED notification is issued at the start of
> > ~OffTheRecordProfile and ~ProfileImpl. What guarantee is there that this
will
> > hold true? 
> 
> I guess none apart from the fact that probably a lot of other stuff will start
> failing if we don't send the notification ;-)
> 
> I discussed this with Mattias and we agreed that in this case it's probably
> easier to watch for a notification than to explicitly |Destroy()| the
> PrefMember, as it is pretty hidden in the ChromeNetworkDelegate.
> 
> > Is this documented somewhere? What happens if a profile releases the
> > last reference to ProfileIOData before this notification is issued?
Currently
> > the reference is held as a member of a Profile subclass, so it should be
> > destroyed after the destructor body has run. 
> 
> So we should be fine at the moment, right?
> 
> > We don't have any tests enforcing
> > any ordering, nor any comments. If you're adding an ordering dependency,
some
> > sort of enforcement would be nice.
> 
> Would you be okay with me just adding a comment which states that the
> notification should be sent before the other stuff is destroyed? The
> use-after-free we'd possibly get when getting it wrong would probably be hard
to
> reliably test for :-/

http://codereview.chromium.org/6773006/diff/24001/chrome/browser/io_thread.h
File chrome/browser/io_thread.h (right):

http://codereview.chromium.org/6773006/diff/24001/chrome/browser/io_thread.h#...
chrome/browser/io_thread.h:211: // Weak, owned by the ChromeNetworkDelegate
(which is transitively owned
Why don't you just put this in the ChromeNetworkDelegate destructor? Oh I see,
it's because the destructor is shared between the system one and the profile
specific ChromeNetworkDelegate. We should just refactor them to be different
classes. Using this weak pointer like this is a bit risky.

[Bernhard Bauer, 2011-04-11 14:16:15]

On Fri, Apr 8, 2011 at 21:10,  <willchan@chromium.org> wrote:
>
>
http://codereview.chromium.org/6773006/diff/12028/chrome/browser/profiles/pro...
> File chrome/browser/profiles/profile_io_data.cc (right):
>
>
http://codereview.chromium.org/6773006/diff/12028/chrome/browser/profiles/pro...
> chrome/browser/profiles/profile_io_data.cc:137:
> params->enable_referrers->ObserveProfileDestruction(profile);
> On 2011/04/05 18:24:47, Bernhard Bauer wrote:
>>
>> On 2011/04/05 17:12:51, willchan wrote:
>> > On 2011/04/05 16:12:59, Bernhard Bauer wrote:
>> > > On 2011/04/05 15:06:18, willchan wrote:
>> > > > On 2011/04/05 14:49:02, Bernhard Bauer wrote:
>> > > > > On 2011/04/05 13:10:41, willchan wrote:
>> > > > > > Why is this necessary? The associated ProfileIOData /
>> > > ChromeNetworkDelegate
>> > > > > > should die soon after the Profile dies.
>> > > > >
>> > > > > Sadly, not soon enough. When the PrefMember is destroyed, it
>
> has to be
>>
>> > > > > unregistered from observing pref changes, which has to happen
>
> a) on the
>>
>> UI
>> > > > > thread, and b) before the PrefService is destroyed (which is
>
> not
>>
>> > guaranteed
>> > > to
>> > > > > be after the ChromeNetworkDelegate is destroyed).
>
> I don't think I like adding implicit notifications like this. First off,
> it's dependent on the Profile. It's possible that a good chunk of this
> code will move into content/ which doesn't know about Chrome profiles. I
> prefer explicit shutdown notifications. I'd rather you add a Shutdown
> member function to ProfileImplIOData::Handle(), which calls
> ProfileImplIOData::Shutdown(), which notifies objects as necessary. This
> way, if we need to forward such shutdown notifications to the content/
> code, we won't have to hook the Profile destroyed notification.

Ok, I added Shutdown() methods to the ProfileIOData subclasses and a
ShutdownOnUIThread() method to ChromeNetworkDelegate. PTAL.

>
>> >
>> > Which PrefService? Sorry, there seem to be a whole bunch and the
>
> shutdown
>>
>> > ordering is not obvious to me.
>
>> The one returned by |GetPrefs()| (which is the incognito PrefService
>
> for an
>>
>> OffTheRecordProfile and the regular one for a regular ProfileImpl),
>
> and it is
>>
>> destroyed via the ProfileManager in the destructor of
>
> BrowserProcessImpl (before
>>
>> the IO thread is stopped in the same destructor).
>
>> > > >
>> > > > Wait, but this BooleanPrefMember lives in the
>
> ChromeNetworkDelegate which
>>
>> > > lives
>> > > > on the IO thread. We'd accessing this without locks? Is this
>
> safe? It
>>
>> seems
>> > a
>> > > > bit dodgy.
>> > >
>> > > We specifically added support for reading prefs without locks on
>
> other
>>
>> > threads,
>> > > by observing pref changes on the UI thread and then updating the
>
> cached pref
>>
>> > > value on the IO thread.
>> >
>> > I see, sounds great.
>> >
>> > I see that the PROFILE_DESTROYED notification is issued at the start
>
> of
>>
>> > ~OffTheRecordProfile and ~ProfileImpl. What guarantee is there that
>
> this will
>>
>> > hold true?
>
>> I guess none apart from the fact that probably a lot of other stuff
>
> will start
>>
>> failing if we don't send the notification ;-)
>
>> I discussed this with Mattias and we agreed that in this case it's
>
> probably
>>
>> easier to watch for a notification than to explicitly |Destroy()| the
>> PrefMember, as it is pretty hidden in the ChromeNetworkDelegate.
>
>> > Is this documented somewhere? What happens if a profile releases the
>> > last reference to ProfileIOData before this notification is issued?
>
> Currently
>>
>> > the reference is held as a member of a Profile subclass, so it
>
> should be
>>
>> > destroyed after the destructor body has run.
>
>> So we should be fine at the moment, right?
>
>> > We don't have any tests enforcing
>> > any ordering, nor any comments. If you're adding an ordering
>
> dependency, some
>>
>> > sort of enforcement would be nice.
>
>> Would you be okay with me just adding a comment which states that the
>> notification should be sent before the other stuff is destroyed? The
>> use-after-free we'd possibly get when getting it wrong would probably
>
> be hard to
>>
>> reliably test for :-/
>
> http://codereview.chromium.org/6773006/diff/24001/chrome/browser/io_thread.h
> File chrome/browser/io_thread.h (right):
>
>
http://codereview.chromium.org/6773006/diff/24001/chrome/browser/io_thread.h#...
> chrome/browser/io_thread.h:211: // Weak, owned by the
> ChromeNetworkDelegate (which is transitively owned
> Why don't you just put this in the ChromeNetworkDelegate destructor? Oh
> I see, it's because the destructor is shared between the system one and
> the profile specific ChromeNetworkDelegate. We should just refactor them
> to be different classes. Using this weak pointer like this is a bit
> risky.

This is now taken care of by ChromeNetworkDelegate::ShutdownOnUIThread().

> http://codereview.chromium.org/6773006/
>

[willchan no longer on Chromium, 2011-04-11 15:21:41]

http://codereview.chromium.org/6773006/diff/28030/chrome/browser/profiles/off...
File chrome/browser/profiles/off_the_record_profile_io_data.cc (right):

http://codereview.chromium.org/6773006/diff/28030/chrome/browser/profiles/off...
chrome/browser/profiles/off_the_record_profile_io_data.cc:104: if
(network_delegate_.get())
This is called on the UI thread, but |network_delegate_| may be being
constructed. This is a race.

Can we do something similar to what you had with the notification based
approach, where we post a shutdown task to the IO thread?

[Bernhard Bauer, 2011-04-11 15:56:02]

http://codereview.chromium.org/6773006/diff/28030/chrome/browser/profiles/off...
File chrome/browser/profiles/off_the_record_profile_io_data.cc (right):

http://codereview.chromium.org/6773006/diff/28030/chrome/browser/profiles/off...
chrome/browser/profiles/off_the_record_profile_io_data.cc:104: if
(network_delegate_.get())
On 2011/04/11 15:21:41, willchan wrote:
> This is called on the UI thread, but |network_delegate_| may be being
> constructed. This is a race.
> 
> Can we do something similar to what you had with the notification based
> approach, where we post a shutdown task to the IO thread?

Ultimately, we need to unregister the PrefMember from its PrefService, which has
to happen on the UI thread :-(

[willchan no longer on Chromium, 2011-04-11 16:00:20]

http://codereview.chromium.org/6773006/diff/28030/chrome/browser/profiles/off...
File chrome/browser/profiles/off_the_record_profile_io_data.cc (right):

http://codereview.chromium.org/6773006/diff/28030/chrome/browser/profiles/off...
chrome/browser/profiles/off_the_record_profile_io_data.cc:104: if
(network_delegate_.get())
On 2011/04/11 15:56:02, Bernhard Bauer wrote:
> On 2011/04/11 15:21:41, willchan wrote:
> > This is called on the UI thread, but |network_delegate_| may be being
> > constructed. This is a race.
> > 
> > Can we do something similar to what you had with the notification based
> > approach, where we post a shutdown task to the IO thread?
> 
> Ultimately, we need to unregister the PrefMember from its PrefService, which
has
> to happen on the UI thread :-(
> 

Ugh, I'm sorry! I had thought your previous CL did a roundtrip from UI=>IO=>UI.
I misread that. Lemme think for a sec. Sorry also if I seem to be jerking you
around over a small detail, I definitely was not planning on that.

[Bernhard Bauer, 2011-04-11 16:19:25]

http://codereview.chromium.org/6773006/diff/28030/chrome/browser/profiles/off...
File chrome/browser/profiles/off_the_record_profile_io_data.cc (right):

http://codereview.chromium.org/6773006/diff/28030/chrome/browser/profiles/off...
chrome/browser/profiles/off_the_record_profile_io_data.cc:104: if
(network_delegate_.get())
On 2011/04/11 16:00:20, willchan wrote:
> On 2011/04/11 15:56:02, Bernhard Bauer wrote:
> > On 2011/04/11 15:21:41, willchan wrote:
> > > This is called on the UI thread, but |network_delegate_| may be being
> > > constructed. This is a race.
> > > 
> > > Can we do something similar to what you had with the notification based
> > > approach, where we post a shutdown task to the IO thread?
> > 
> > Ultimately, we need to unregister the PrefMember from its PrefService, which
> has
> > to happen on the UI thread :-(
> > 
> 
> Ugh, I'm sorry! I had thought your previous CL did a roundtrip from
UI=>IO=>UI.
> I misread that. Lemme think for a sec. Sorry also if I seem to be jerking you
> around over a small detail, I definitely was not planning on that.

Uh, no, I totally realize that we should do something about it.

I think we'd also get a problem if we shut down after InitializeProfileParams
has been called (so the PrefMember is initialized), but before the
NetworkDelegate has been created, so it would still live in the LazyParams :-/

[willchan no longer on Chromium, 2011-04-11 16:24:37]

http://codereview.chromium.org/6773006/diff/28030/chrome/browser/profiles/off...
File chrome/browser/profiles/off_the_record_profile_io_data.cc (right):

http://codereview.chromium.org/6773006/diff/28030/chrome/browser/profiles/off...
chrome/browser/profiles/off_the_record_profile_io_data.cc:104: if
(network_delegate_.get())
On 2011/04/11 16:00:20, willchan wrote:
> On 2011/04/11 15:56:02, Bernhard Bauer wrote:
> > On 2011/04/11 15:21:41, willchan wrote:
> > > This is called on the UI thread, but |network_delegate_| may be being
> > > constructed. This is a race.
> > > 
> > > Can we do something similar to what you had with the notification based
> > > approach, where we post a shutdown task to the IO thread?
> > 
> > Ultimately, we need to unregister the PrefMember from its PrefService, which
> has
> > to happen on the UI thread :-(
> > 
> 
> Ugh, I'm sorry! I had thought your previous CL did a roundtrip from
UI=>IO=>UI.
> I misread that. Lemme think for a sec. Sorry also if I seem to be jerking you
> around over a small detail, I definitely was not planning on that.

The only other solution I can come up with involves adding the PrefMember
directly into the ProfileIOData, so we know it exists, and then have the
ChromeNetworkDelegate keep a raw pointer to it. I'm not sure how much I like
this solution either =/

It's not clear to me which is better. Choose whichever you prefer (your previous
solution with the profile observing, or my other proposal where you add the
PrefMember directly into the ProfileIOData).

[Bernhard Bauer, 2011-04-12 15:11:16]

On Mon, Apr 11, 2011 at 18:24, <willchan@chromium.org> wrote:
>
>
http://codereview.chromium.org/6773006/diff/28030/chrome/browser/profiles/off...
> File chrome/browser/profiles/off_the_record_profile_io_data.cc (right):
>
>
http://codereview.chromium.org/6773006/diff/28030/chrome/browser/profiles/off...
> chrome/browser/profiles/off_the_record_profile_io_data.cc:104: if
> (network_delegate_.get())
> On 2011/04/11 16:00:20, willchan wrote:
>>
>> On 2011/04/11 15:56:02, Bernhard Bauer wrote:
>> > On 2011/04/11 15:21:41, willchan wrote:
>> > > This is called on the UI thread, but |network_delegate_| may be
>
> being
>>
>> > > constructed. This is a race.
>> > >
>> > > Can we do something similar to what you had with the notification
>
> based
>>
>> > > approach, where we post a shutdown task to the IO thread?
>> >
>> > Ultimately, we need to unregister the PrefMember from its
>
> PrefService, which
>>
>> has
>> > to happen on the UI thread :-(
>> >
>
>> Ugh, I'm sorry! I had thought your previous CL did a roundtrip from
>
> UI=>IO=>UI.
>>
>> I misread that. Lemme think for a sec. Sorry also if I seem to be
>
> jerking you
>>
>> around over a small detail, I definitely was not planning on that.
>
> The only other solution I can come up with involves adding the
> PrefMember directly into the ProfileIOData, so we know it exists, and
> then have the ChromeNetworkDelegate keep a raw pointer to it. I'm not
> sure how much I like this solution either =/

Done! I chose this solution, because the other one seemed like too
much action at a distance to me, the more I thought about it.

> It's not clear to me which is better. Choose whichever you prefer (your
> previous solution with the profile observing, or my other proposal where
> you add the PrefMember directly into the ProfileIOData).
>
> http://codereview.chromium.org/6773006/

[willchan no longer on Chromium, 2011-04-12 16:34:17]

http://codereview.chromium.org/6773006/diff/33031/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_io_data.cc (right):

http://codereview.chromium.org/6773006/diff/33031/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_io_data.cc:284: void ProfileIOData::Shutdown() {
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));

http://codereview.chromium.org/6773006/diff/33031/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_io_data.h (right):

http://codereview.chromium.org/6773006/diff/33031/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_io_data.h:122: explicit ProfileIOData(Profile*
profile, bool is_incognito);
Not explicit anymore.

I have to say, I resisted adding Profile for a long time. The reason is because
ProfileIOData will live on the IO thread. We really don't want to cache the
|profile| pointer, because any use on the IO thread is a bug.

I've been trying to pass the explicit dependency (the initialized
BooleanPrefMember in this case), although I acknowledge that would introduce a
bit of code redundancy. I'm also worried about the ProfileIOData subclasses
caching Profile*. I'm not sure how draconian I should be. I guess I'd be fine
with it if there were comments in the constructors indicating not to cache the
Profile*.

http://codereview.chromium.org/6773006/diff/33031/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_io_data.h:138: protected:
Look at line 73, there's already a protected section. Actually, everything below
should be private. Other than the new member functions you've added.

http://codereview.chromium.org/6773006/diff/33031/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_io_data.h:167: void Shutdown();
ShutdownOnUI()

http://codereview.chromium.org/6773006/diff/33031/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_io_data.h:174: scoped_ptr<BooleanPrefMember>
enable_referrers_;
Why use a pointer? Can't we just directly embed the BooleanPrefMember and
construct it in the initializer list?

[Bernhard Bauer, 2011-04-12 20:26:04]

(I'll work on the other comments tomorrow, when I'm back at the office)

http://codereview.chromium.org/6773006/diff/33031/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_io_data.h (right):

http://codereview.chromium.org/6773006/diff/33031/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_io_data.h:122: explicit ProfileIOData(Profile*
profile, bool is_incognito);
On 2011/04/12 16:34:18, willchan wrote:
> Not explicit anymore.
> 
> I have to say, I resisted adding Profile for a long time. The reason is
because
> ProfileIOData will live on the IO thread. We really don't want to cache the
> |profile| pointer, because any use on the IO thread is a bug.
> 
> I've been trying to pass the explicit dependency (the initialized
> BooleanPrefMember in this case), although I acknowledge that would introduce a
> bit of code redundancy. I'm also worried about the ProfileIOData subclasses
> caching Profile*. I'm not sure how draconian I should be. I guess I'd be fine
> with it if there were comments in the constructors indicating not to cache the
> Profile*.

Hmm, I could initialize the BooleanPrefMember in the ::Handle constructor,
right? There we already have the Profile, and I could even extract a static
helper method similar to InitializeProfileParams() that does the intialization.

[willchan no longer on Chromium, 2011-04-12 22:43:07]

http://codereview.chromium.org/6773006/diff/33031/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_io_data.h (right):

http://codereview.chromium.org/6773006/diff/33031/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_io_data.h:122: explicit ProfileIOData(Profile*
profile, bool is_incognito);
On 2011/04/12 20:26:05, Bernhard Bauer wrote:
> On 2011/04/12 16:34:18, willchan wrote:
> > Not explicit anymore.
> > 
> > I have to say, I resisted adding Profile for a long time. The reason is
> because
> > ProfileIOData will live on the IO thread. We really don't want to cache the
> > |profile| pointer, because any use on the IO thread is a bug.
> > 
> > I've been trying to pass the explicit dependency (the initialized
> > BooleanPrefMember in this case), although I acknowledge that would introduce
a
> > bit of code redundancy. I'm also worried about the ProfileIOData subclasses
> > caching Profile*. I'm not sure how draconian I should be. I guess I'd be
fine
> > with it if there were comments in the constructors indicating not to cache
the
> > Profile*.
> 
> Hmm, I could initialize the BooleanPrefMember in the ::Handle constructor,
> right? There we already have the Profile, and I could even extract a static
> helper method similar to InitializeProfileParams() that does the
intialization.

Yes, I would be fine with that. That's what I referred to in causing code
redundancy, since It'd have to be done for both ProfileImplIOData::Handle and
OffTheRecordProfileIOData::Handle.

[Bernhard Bauer, 2011-04-13 11:17:19]

http://codereview.chromium.org/6773006/diff/33031/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_io_data.cc (right):

http://codereview.chromium.org/6773006/diff/33031/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_io_data.cc:284: void ProfileIOData::Shutdown() {
On 2011/04/12 16:34:18, willchan wrote:
> DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));

Done.

http://codereview.chromium.org/6773006/diff/33031/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_io_data.h (right):

http://codereview.chromium.org/6773006/diff/33031/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_io_data.h:122: explicit ProfileIOData(Profile*
profile, bool is_incognito);
On 2011/04/12 22:43:07, willchan wrote:
> On 2011/04/12 20:26:05, Bernhard Bauer wrote:
> > On 2011/04/12 16:34:18, willchan wrote:
> > > Not explicit anymore.
> > > 
> > > I have to say, I resisted adding Profile for a long time. The reason is
> > because
> > > ProfileIOData will live on the IO thread. We really don't want to cache
the
> > > |profile| pointer, because any use on the IO thread is a bug.
> > > 
> > > I've been trying to pass the explicit dependency (the initialized
> > > BooleanPrefMember in this case), although I acknowledge that would
introduce
> a
> > > bit of code redundancy. I'm also worried about the ProfileIOData
subclasses
> > > caching Profile*. I'm not sure how draconian I should be. I guess I'd be
> fine
> > > with it if there were comments in the constructors indicating not to cache
> the
> > > Profile*.
> > 
> > Hmm, I could initialize the BooleanPrefMember in the ::Handle constructor,
> > right? There we already have the Profile, and I could even extract a static
> > helper method similar to InitializeProfileParams() that does the
> intialization.
> 
> Yes, I would be fine with that. That's what I referred to in causing code
> redundancy, since It'd have to be done for both ProfileImplIOData::Handle and
> OffTheRecordProfileIOData::Handle.

OK, done (in ChromeNetworkDelegate now, so it can be used by IOThread as well).

http://codereview.chromium.org/6773006/diff/33031/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_io_data.h:138: protected:
On 2011/04/12 16:34:18, willchan wrote:
> Look at line 73, there's already a protected section. Actually, everything
below
> should be private. Other than the new member functions you've added.

Oops, done.

http://codereview.chromium.org/6773006/diff/33031/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_io_data.h:167: void Shutdown();
On 2011/04/12 16:34:18, willchan wrote:
> ShutdownOnUI()

Done.

http://codereview.chromium.org/6773006/diff/33031/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_io_data.h:174: scoped_ptr<BooleanPrefMember>
enable_referrers_;
On 2011/04/12 16:34:18, willchan wrote:
> Why use a pointer? Can't we just directly embed the BooleanPrefMember and
> construct it in the initializer list?

Very nice! Done.

[willchan no longer on Chromium, 2011-04-13 14:59:00]

LGTM!

http://codereview.chromium.org/6773006/diff/38001/chrome/browser/io_thread.h
File chrome/browser/io_thread.h (right):

http://codereview.chromium.org/6773006/diff/38001/chrome/browser/io_thread.h#...
chrome/browser/io_thread.h:185: BooleanPrefMember enable_referrers_;
Nit: maybe we should call this system_enable_referrers_, to indicate that this
is Profile agnostic. Up to you!

http://codereview.chromium.org/6773006/diff/38001/chrome/browser/net/chrome_n...
File chrome/browser/net/chrome_network_delegate.cc (right):

http://codereview.chromium.org/6773006/diff/38001/chrome/browser/net/chrome_n...
chrome/browser/net/chrome_network_delegate.cc:57:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
Please include browser_thread.h for this.

[Bernhard Bauer, 2011-04-13 15:14:50]

Committing now. Thanks for your patience with this CL!

http://codereview.chromium.org/6773006/diff/38001/chrome/browser/io_thread.h
File chrome/browser/io_thread.h (right):

http://codereview.chromium.org/6773006/diff/38001/chrome/browser/io_thread.h#...
chrome/browser/io_thread.h:185: BooleanPrefMember enable_referrers_;
On 2011/04/13 14:59:00, willchan wrote:
> Nit: maybe we should call this system_enable_referrers_, to indicate that this
> is Profile agnostic. Up to you!

Sounds good.

http://codereview.chromium.org/6773006/diff/38001/chrome/browser/net/chrome_n...
File chrome/browser/net/chrome_network_delegate.cc (right):

http://codereview.chromium.org/6773006/diff/38001/chrome/browser/net/chrome_n...
chrome/browser/net/chrome_network_delegate.cc:57:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
On 2011/04/13 14:59:00, willchan wrote:
> Please include browser_thread.h for this.

Done.

[willchan no longer on Chromium, 2011-04-13 15:18:06]

Hah, thanks for your patience in putting up with my
nittipickiness/strictness :)

On Wed, Apr 13, 2011 at 5:14 PM, <bauerb@chromium.org> wrote:

> Committing now. Thanks for your patience with this CL!
>
>
>
>
> http://codereview.chromium.org/6773006/diff/38001/chrome/browser/io_thread.h
> File chrome/browser/io_thread.h (right):
>
>
>
http://codereview.chromium.org/6773006/diff/38001/chrome/browser/io_thread.h#...
> chrome/browser/io_thread.h:185: BooleanPrefMember enable_referrers_;
> On 2011/04/13 14:59:00, willchan wrote:
>
>> Nit: maybe we should call this system_enable_referrers_, to indicate
>>
> that this
>
>> is Profile agnostic. Up to you!
>>
>
> Sounds good.
>
>
>
>
http://codereview.chromium.org/6773006/diff/38001/chrome/browser/net/chrome_n...
> File chrome/browser/net/chrome_network_delegate.cc (right):
>
>
>
http://codereview.chromium.org/6773006/diff/38001/chrome/browser/net/chrome_n...
> chrome/browser/net/chrome_network_delegate.cc:57:
> DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
> On 2011/04/13 14:59:00, willchan wrote:
>
>> Please include browser_thread.h for this.
>>
>
> Done.
>
>
> http://codereview.chromium.org/6773006/
>