Make CookieMonster NonThreadSafe.

Make CookieMonster NonThreadSafe.
Made ExtensionFunction RefCountedThreadSafe so it can be posted to different threads.
Used WaitableEvent in AutomationProvider.
BUG=44083
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=50296

[willchan no longer on Chromium, 2010-06-09 01:22:03]

rdsmith: Please review CookieMonster changes and AutomationProvider stuff.
jochen: Please review the ExtensionDataDeleter changes
cindylau: Please review the CookieFunction changes
wtc: Please review cookie_policy_browsertest.cc

[jochen (gone - plz use gerrit), 2010-06-09 05:23:34]

shouldn't you also change cookie store then? it's ref counted thread safe..

please also run tryjobs on the valgrind bots.

On 2010/06/09 01:22:03, willchan wrote:
> rdsmith: Please review CookieMonster changes and AutomationProvider stuff.
> jochen: Please review the ExtensionDataDeleter changes
> cindylau: Please review the CookieFunction changes
> wtc: Please review cookie_policy_browsertest.cc

[Randy Smith (Not in Mondays), 2010-06-09 17:54:48]

I like the approach your taking, but have some comments inline.  Top level
comments:

-- Make sure to fix the presubmit checks; you've got some extra whitespace.

-- Does anything need to be done with making AutomationCookieStore
non-thread-safe, or is that outside the scope of this issue?  (It inherits from
CookieStore, not CookieMonster, so the answer may be "Nope; outside the scope
:-}"

http://codereview.chromium.org/2756003/diff/1/2
File chrome/browser/automation/automation_provider.cc (right):

http://codereview.chromium.org/2756003/diff/1/2#newcode1174
chrome/browser/automation/automation_provider.cc:1174: class SetCookiesTask :
public Task {
Shouldn't this be singular (SetCookieTask)?

http://codereview.chromium.org/2756003/diff/1/2#newcode1286
chrome/browser/automation/automation_provider.cc:1286:
tab->profile()->GetRequestContext()));
So I think this is a changed behavior, from synchronous to asynchronous.  Why? 
Are you sure that it's safe given the callers of AutomationProvider?  I'd think
keeping the behavior synchronous (i.e. patterning DeleteCookie after Set/Get)
would be safer.

http://codereview.chromium.org/2756003/diff/1/3
File chrome/browser/extensions/extension_cookies_api.cc (right):

http://codereview.chromium.org/2756003/diff/1/3#newcode382
chrome/browser/extensions/extension_cookies_api.cc:382: new
RemoveCookieTask(url, name, store_profile->GetRequestContext()));
Again, a change to asynchronous.  Could you reassure me (or future readers, in a
comment) as to why this is guaranteed safe?  

If there's no immediate reason to change to asynchronous, I'd recommend staying
synchronous; why risk subtle race conditions when we don't have to?

http://codereview.chromium.org/2756003/diff/1/4
File chrome/browser/extensions/extension_cookies_api.h (right):

http://codereview.chromium.org/2756003/diff/1/4#newcode36
chrome/browser/extensions/extension_cookies_api.h:36: // that occurs.
It can't return false since it doesn't return a boolean.  NULL?

http://codereview.chromium.org/2756003/diff/1/6
File chrome/browser/extensions/extension_cookies_helpers.h (right):

http://codereview.chromium.org/2756003/diff/1/6#newcode48
chrome/browser/extensions/extension_cookies_helpers.h:48: // URL. If the URL is
empty, all cookies in the cookie store are retrieved.
I'd add a comment here that this function must now be called on the IO thread. 
An assertion would be even better, but I don't think there's any harm in letting
the call fall through to the cookie monster and assert there.

http://codereview.chromium.org/2756003/diff/1/7
File chrome/browser/extensions/extension_data_deleter.cc (right):

http://codereview.chromium.org/2756003/diff/1/7#newcode47
chrome/browser/extensions/extension_data_deleter.cc:47:
cookie_monster->DeleteAllForURL(extension_url_, true);
I'm confused.  The posts above suggest that we're not on the IO thread; given
that, why is it safe to call cookie_monster methods?

http://codereview.chromium.org/2756003/diff/1/10
File net/base/cookie_monster.cc (right):

http://codereview.chromium.org/2756003/diff/1/10#newcode552
net/base/cookie_monster.cc:552: DCHECK(CalledOnValidThread());
How did you choose which functions to put this into?  Public methods which don't
have it: DeleteCookie (three argument version), SetCookieWithCreationTime
(redundant with SetCookieWithCreationTimeAndOptions but would be consistent with
presence in SetCookieWithOptions, DeleteAllCreatedAfter), GetCookieMonster
(though that's probably overkill).

[willchan no longer on Chromium, 2010-06-09 19:36:50]

On 2010/06/09 05:23:34, jochen wrote:
> shouldn't you also change cookie store then? it's ref counted thread safe..

Hm, I'll file a bug for that and do it in a later changelist.

> 
> please also run tryjobs on the valgrind bots.
> 
> On 2010/06/09 01:22:03, willchan wrote:
> > rdsmith: Please review CookieMonster changes and AutomationProvider stuff.
> > jochen: Please review the ExtensionDataDeleter changes
> > cindylau: Please review the CookieFunction changes
> > wtc: Please review cookie_policy_browsertest.cc

[Randy Smith (Not in Mondays), 2010-06-09 19:43:32]

On 2010/06/09 19:36:50, willchan wrote:
> On 2010/06/09 05:23:34, jochen wrote:
> > shouldn't you also change cookie store then? it's ref counted thread safe..
> 
> Hm, I'll file a bug for that and do it in a later changelist.

For what it's worth, I like the idea of getting some "soak time" where we'll get
DCHECK's if the IO thread restriction is violated in a debug build, but releases
are safe because the class is still RefCountedThreadSafe.  You may not have
intended to remove the RefCountedThreadSafe in the later changelist, but if you
did, I'd suggest filing a bug and not fixing it for a little while :-}.

> 
> > 
> > please also run tryjobs on the valgrind bots.
> > 
> > On 2010/06/09 01:22:03, willchan wrote:
> > > rdsmith: Please review CookieMonster changes and AutomationProvider stuff.
> > > jochen: Please review the ExtensionDataDeleter changes
> > > cindylau: Please review the CookieFunction changes
> > > wtc: Please review cookie_policy_browsertest.cc

[willchan no longer on Chromium, 2010-06-09 19:45:40]

http://codereview.chromium.org/2756003/diff/1/2
File chrome/browser/automation/automation_provider.cc (right):

http://codereview.chromium.org/2756003/diff/1/2#newcode1174
chrome/browser/automation/automation_provider.cc:1174: class SetCookiesTask :
public Task {
On 2010/06/09 17:54:49, rdsmith wrote:
> Shouldn't this be singular (SetCookieTask)?

Done.

http://codereview.chromium.org/2756003/diff/1/2#newcode1286
chrome/browser/automation/automation_provider.cc:1286:
tab->profile()->GetRequestContext()));
On 2010/06/09 17:54:49, rdsmith wrote:
> So I think this is a changed behavior, from synchronous to asynchronous.  Why?

> Are you sure that it's safe given the callers of AutomationProvider?  I'd
think
> keeping the behavior synchronous (i.e. patterning DeleteCookie after Set/Get)
> would be safer.

Here's my justification for why it's safe:
Everything is ordered on the IO thread.  This is a write operation.  Any other
read/write operation on the CookieMonster will happen on the IO thread,
therefore they are guaranteed to see this.

Also, what kind of race condition are you worried about?  Everything's being
done on the IO thread, so there shouldn't be any corruption of the CookieMonster
itself.  If you're worried about the UI thread consumer making sure that the
delete happens before proceeding with any other activity, I don't know that
that's a safe assumption to make, since something else may have modified the
CookieMonster in the meantime anyway, so I don't think there are any reasonable
guarantees on the UI thread in the first place.

Let me know what you think about that justification.

http://codereview.chromium.org/2756003/diff/1/3
File chrome/browser/extensions/extension_cookies_api.cc (right):

http://codereview.chromium.org/2756003/diff/1/3#newcode382
chrome/browser/extensions/extension_cookies_api.cc:382: new
RemoveCookieTask(url, name, store_profile->GetRequestContext()));
On 2010/06/09 17:54:49, rdsmith wrote:
> Again, a change to asynchronous.  Could you reassure me (or future readers, in
a
> comment) as to why this is guaranteed safe?  
> 
> If there's no immediate reason to change to asynchronous, I'd recommend
staying
> synchronous; why risk subtle race conditions when we don't have to?

Not blocking the UI thread is a performance win.  Any delay on it will result in
all pending UI events in the queue being delayed.  The delay would be roughly
equal to a single iteration through the IO thread.

I think it's safe, due to the justification I provided in
automation_provider.cc.  Let me know what you think about it.  I am going to
loop in eroman for his opinion here too.

http://codereview.chromium.org/2756003/diff/1/4
File chrome/browser/extensions/extension_cookies_api.h (right):

http://codereview.chromium.org/2756003/diff/1/4#newcode36
chrome/browser/extensions/extension_cookies_api.h:36: // that occurs.
On 2010/06/09 17:54:49, rdsmith wrote:
> It can't return false since it doesn't return a boolean.  NULL?
> 

Done.

http://codereview.chromium.org/2756003/diff/1/6
File chrome/browser/extensions/extension_cookies_helpers.h (right):

http://codereview.chromium.org/2756003/diff/1/6#newcode48
chrome/browser/extensions/extension_cookies_helpers.h:48: // URL. If the URL is
empty, all cookies in the cookie store are retrieved.
On 2010/06/09 17:54:49, rdsmith wrote:
> I'd add a comment here that this function must now be called on the IO thread.

> An assertion would be even better, but I don't think there's any harm in
letting
> the call fall through to the cookie monster and assert there.
> 

Done.

http://codereview.chromium.org/2756003/diff/1/7
File chrome/browser/extensions/extension_data_deleter.cc (right):

http://codereview.chromium.org/2756003/diff/1/7#newcode47
chrome/browser/extensions/extension_data_deleter.cc:47:
cookie_monster->DeleteAllForURL(extension_url_, true);
On 2010/06/09 17:54:49, rdsmith wrote:
> I'm confused.  The posts above suggest that we're not on the IO thread; given
> that, why is it safe to call cookie_monster methods?
> 

I think you might be misreading it?  StartDeleting() is called on the UI thread.
 It posts a call to DeleteCookiesOnIOThread() to the IO thread.  Note the
DCHECK(ChromeThread::CurrentlyOn(ChromeThread::IO)); at the beginning of the
function which proves we're on the IO thread.

http://codereview.chromium.org/2756003/diff/1/10
File net/base/cookie_monster.cc (right):

http://codereview.chromium.org/2756003/diff/1/10#newcode552
net/base/cookie_monster.cc:552: DCHECK(CalledOnValidThread());
On 2010/06/09 17:54:49, rdsmith wrote:
> How did you choose which functions to put this into?  Public methods which
don't
> have it: DeleteCookie (three argument version), SetCookieWithCreationTime
> (redundant with SetCookieWithCreationTimeAndOptions but would be consistent
with
> presence in SetCookieWithOptions, DeleteAllCreatedAfter), GetCookieMonster
> (though that's probably overkill). 

I was trying to do all public methods.  Looks like I missed some.  Thanks for
pointing them out.  I've updated them now.

[Randy Smith (Not in Mondays), 2010-06-09 20:12:14]

LGTM

http://codereview.chromium.org/2756003/diff/1/2
File chrome/browser/automation/automation_provider.cc (right):

http://codereview.chromium.org/2756003/diff/1/2#newcode1286
chrome/browser/automation/automation_provider.cc:1286:
tab->profile()->GetRequestContext()));
On 2010/06/09 19:45:41, willchan wrote:
> On 2010/06/09 17:54:49, rdsmith wrote:
> > So I think this is a changed behavior, from synchronous to asynchronous. 
Why?
> 
> > Are you sure that it's safe given the callers of AutomationProvider?  I'd
> think
> > keeping the behavior synchronous (i.e. patterning DeleteCookie after
Set/Get)
> > would be safer.
> 
> Here's my justification for why it's safe:
> Everything is ordered on the IO thread.  This is a write operation.  Any other
> read/write operation on the CookieMonster will happen on the IO thread,
> therefore they are guaranteed to see this.

That's valid as long as when the PostTask returns, any future PostTask will be
enqueued after it in the IO thread queue.  That strikes me as a believable
guarantee from PostTask, which means I'm cool with this justification, but I
wanted to make explicit my reasoning in case I was making an invalid assumption;
I don't know this code well.

> 
> Also, what kind of race condition are you worried about?  

Most of my poking at this is out of pure paranoia.  OTOH, that paranoia's the
result of several years of multi-threaded programming :-}.  I also tend to worry
more than many people about really low likelihood bugs, because when they do
happen, they're almost impossible to reproduce and then to fix.  

> Everything's being
> done on the IO thread, so there shouldn't be any corruption of the
CookieMonster
> itself.  If you're worried about the UI thread consumer making sure that the
> delete happens before proceeding with any other activity, I don't know that
> that's a safe assumption to make, since something else may have modified the
> CookieMonster in the meantime anyway, so I don't think there are any
reasonable
> guarantees on the UI thread in the first place.

That's the type of thing I was worried about; if the only user of this interface
is the UI Cookie interface, I think that's a valid argument (and I think your IO
thread serialization argument is also valid, so you've got belt-and-suspenders).
 Thanks for engaging with my paranoia.

> 
> Let me know what you think about that justification.

http://codereview.chromium.org/2756003/diff/1/3
File chrome/browser/extensions/extension_cookies_api.cc (right):

http://codereview.chromium.org/2756003/diff/1/3#newcode382
chrome/browser/extensions/extension_cookies_api.cc:382: new
RemoveCookieTask(url, name, store_profile->GetRequestContext()));
On 2010/06/09 19:45:41, willchan wrote:
> On 2010/06/09 17:54:49, rdsmith wrote:
> > Again, a change to asynchronous.  Could you reassure me (or future readers,
in
> a
> > comment) as to why this is guaranteed safe?  
> > 
> > If there's no immediate reason to change to asynchronous, I'd recommend
> staying
> > synchronous; why risk subtle race conditions when we don't have to?
> 
> Not blocking the UI thread is a performance win.  Any delay on it will result
in
> all pending UI events in the queue being delayed.  The delay would be roughly
> equal to a single iteration through the IO thread.
> 
> I think it's safe, due to the justification I provided in
> automation_provider.cc.  Let me know what you think about it.  I am going to
> loop in eroman for his opinion here too.

That argument makes sense to me.  I'm still internalizing the performance
requirements of this project, but thinking about it, speeding up the UI thread
is definitely a good thing.

http://codereview.chromium.org/2756003/diff/1/7
File chrome/browser/extensions/extension_data_deleter.cc (right):

http://codereview.chromium.org/2756003/diff/1/7#newcode47
chrome/browser/extensions/extension_data_deleter.cc:47:
cookie_monster->DeleteAllForURL(extension_url_, true);
Sorry, quite right; my eyes glazed over the boundary between the functions
because of the diff.

[Cindy Lau, 2010-06-10 00:02:21]

http://codereview.chromium.org/2756003/diff/11001/12002
File chrome/browser/extensions/extension_cookies_api.cc (right):

http://codereview.chromium.org/2756003/diff/11001/12002#newcode76
chrome/browser/extensions/extension_cookies_api.cc:76: namespace {
Could you please move all this anonymous namespace code in this file to
extension_cookies_helpers.h/.cc so it can be unit tested?

http://codereview.chromium.org/2756003/diff/11001/12002#newcode136
chrome/browser/extensions/extension_cookies_api.cc:136: Profile* store_profile =
GetStoreProfile(details);
The purpose of ParseCookieStore was to refactor common code; it is meant to be
analogous to ParseUrl in that it takes an API argument and returns the useful
data associated with it.  Here you've broken out the common code and are
repeating it multiple times.  Seems like you could move the
GetStoreIdFromProfile back to where it was, and instead of ParseCookieStore you
could say something like ParseStoreContext and obtain the
URLRequestContextGetter, since you never use the store profile for anything else
in this file.

[willchan no longer on Chromium, 2010-06-10 00:43:31]

http://codereview.chromium.org/2756003/diff/11001/12002
File chrome/browser/extensions/extension_cookies_api.cc (right):

http://codereview.chromium.org/2756003/diff/11001/12002#newcode76
chrome/browser/extensions/extension_cookies_api.cc:76: namespace {
On 2010/06/10 00:02:21, Cindy Lau wrote:
> Could you please move all this anonymous namespace code in this file to
> extension_cookies_helpers.h/.cc so it can be unit tested?

Is there a reason you want to unit test the implementation directly rather than
through the public interface?  Is the public interface insufficient for any
reason?

http://codereview.chromium.org/2756003/diff/11001/12002#newcode136
chrome/browser/extensions/extension_cookies_api.cc:136: Profile* store_profile =
GetStoreProfile(details);
On 2010/06/10 00:02:21, Cindy Lau wrote:
> The purpose of ParseCookieStore was to refactor common code; it is meant to be
> analogous to ParseUrl in that it takes an API argument and returns the useful
> data associated with it.  Here you've broken out the common code and are
> repeating it multiple times.  Seems like you could move the
> GetStoreIdFromProfile back to where it was, and instead of ParseCookieStore
you
> could say something like ParseStoreContext and obtain the
> URLRequestContextGetter, since you never use the store profile for anything
else
> in this file.

Done.

[wtc, 2010-06-10 03:14:32]

LGTM on cookie_policy_browsertest.cc.

http://codereview.chromium.org/2756003/diff/22001/10009
File chrome/browser/net/cookie_policy_browsertest.cc (right):

http://codereview.chromium.org/2756003/diff/22001/10009#newcode15
chrome/browser/net/cookie_policy_browsertest.cc:15: namespace {
Is this necessary?

[Cindy Lau, 2010-06-10 19:51:20]

Thanks. LGTM aside from some minor things.

http://codereview.chromium.org/2756003/diff/11001/12002
File chrome/browser/extensions/extension_cookies_api.cc (right):

http://codereview.chromium.org/2756003/diff/11001/12002#newcode76
chrome/browser/extensions/extension_cookies_api.cc:76: namespace {
On 2010/06/10 00:43:32, willchan wrote:
> On 2010/06/10 00:02:21, Cindy Lau wrote:
> > Could you please move all this anonymous namespace code in this file to
> > extension_cookies_helpers.h/.cc so it can be unit tested?
> 
> Is there a reason you want to unit test the implementation directly rather
than
> through the public interface?  Is the public interface insufficient for any
> reason?

I guess you may be able to use static mocks to mock out the task threading here;
I was concerned that you wouldn't be able to do so.  Feel free to leave it as is
if that's the case.

http://codereview.chromium.org/2756003/diff/22001/10003#newcode148
chrome/browser/extensions/extension_cookies_api.cc:148: // Return the first
matching cookie. Relies on the fact that the
Please add the DCHECK back in from the original code.

http://codereview.chromium.org/2756003/diff/22001/10003#newcode183
chrome/browser/extensions/extension_cookies_api.cc:183: ListValue* matching_list
= new ListValue();
Please restore the original DCHECK.

http://codereview.chromium.org/2756003/diff/22001/10003#newcode328
chrome/browser/extensions/extension_cookies_api.cc:328: error_ =
ExtensionErrorUtils::FormatErrorMessage(
DCHECK

http://codereview.chromium.org/2756003/diff/22001/10003#newcode383
chrome/browser/extensions/extension_cookies_api.cc:383: bool rv =
ChromeThread::PostTask(
DCHECK

http://codereview.chromium.org/2756003/diff/22001/10004
File chrome/browser/extensions/extension_cookies_api.h (right):

http://codereview.chromium.org/2756003/diff/22001/10004#newcode33
chrome/browser/extensions/extension_cookies_api.h:33: // the cookie store
context.  If the 'storeId' value isn't found in the
Please add the comments back in about the store_id output parameter from the
original code.

[eroman, 2010-06-10 20:23:26]

The blocking wait being done here in the extension API is pretty nasty.

It seems the right solution is to instead register the extension callbacks as
running on the IO thread rather than the UI thread.

That way the callback will be directly invoked on the IO thread and this
threading issue won't arise.

mpcomplete: Is there an existing mechanism in the extension framework to specify
which thread the callbacks should be invoked on?

If not, it seems like an important feature to add, since we will be hitting this
problem for any other future network extension API that we want to add (as they
all need to run code on the IO thread and not the UI thread)... And wrapping
them all in this way is not a viable option IMO.

http://codereview.chromium.org/2756003/diff/11001/12001
File chrome/browser/automation/automation_provider.cc (right):

http://codereview.chromium.org/2756003/diff/11001/12001#newcode1177
chrome/browser/automation/automation_provider.cc:1177: const std::string& value,
indentatin.

http://codereview.chromium.org/2756003/diff/22001/10011
File net/base/cookie_monster.h (right):

http://codereview.chromium.org/2756003/diff/22001/10011#newcode291
net/base/cookie_monster.h:291: Lock lock_;
Not getting rid of the lock?

[jochen (gone - plz use gerrit), 2010-06-14 10:53:57]

On 2010/06/10 20:23:26, eroman wrote:
> The blocking wait being done here in the extension API is pretty nasty.
> 
> It seems the right solution is to instead register the extension callbacks as
> running on the IO thread rather than the UI thread.
> 
> That way the callback will be directly invoked on the IO thread and this
> threading issue won't arise.
> 
> mpcomplete: Is there an existing mechanism in the extension framework to
specify
> which thread the callbacks should be invoked on?

the cookies extension api uses sync callbacks. you can use async callbacks
instead, so there's no need to block when waiting for results from other
threads.

> 
> If not, it seems like an important feature to add, since we will be hitting
this
> problem for any other future network extension API that we want to add (as
they
> all need to run code on the IO thread and not the UI thread)... And wrapping
> them all in this way is not a viable option IMO.
> 
> http://codereview.chromium.org/2756003/diff/11001/12001
> File chrome/browser/automation/automation_provider.cc (right):
> 
> http://codereview.chromium.org/2756003/diff/11001/12001#newcode1177
> chrome/browser/automation/automation_provider.cc:1177: const std::string&
value,
> indentatin.
> 
> http://codereview.chromium.org/2756003/diff/22001/10011
> File net/base/cookie_monster.h (right):
> 
> http://codereview.chromium.org/2756003/diff/22001/10011#newcode291
> net/base/cookie_monster.h:291: Lock lock_;
> Not getting rid of the lock?

[willchan no longer on Chromium, 2010-06-15 17:10:29]

On 2010/06/14 10:53:57, jochen wrote:
> On 2010/06/10 20:23:26, eroman wrote:
> > The blocking wait being done here in the extension API is pretty nasty.
> > 
> > It seems the right solution is to instead register the extension callbacks
as
> > running on the IO thread rather than the UI thread.
> > 
> > That way the callback will be directly invoked on the IO thread and this
> > threading issue won't arise.
> > 
> > mpcomplete: Is there an existing mechanism in the extension framework to
> specify
> > which thread the callbacks should be invoked on?
> 
> the cookies extension api uses sync callbacks. you can use async callbacks
> instead, so there's no need to block when waiting for results from other
> threads.

Are you sure this is reasonable?  I'm not an extension expert here, so I defer
to the reviewers, but it doesn't seem ideal to make these cookie functions
async.  But I'm happy to make that change if it's ok with you guys.

> 
> > 
> > If not, it seems like an important feature to add, since we will be hitting
> this
> > problem for any other future network extension API that we want to add (as
> they
> > all need to run code on the IO thread and not the UI thread)... And wrapping
> > them all in this way is not a viable option IMO.
> > 
> > http://codereview.chromium.org/2756003/diff/11001/12001
> > File chrome/browser/automation/automation_provider.cc (right):
> > 
> > http://codereview.chromium.org/2756003/diff/11001/12001#newcode1177
> > chrome/browser/automation/automation_provider.cc:1177: const std::string&
> value,
> > indentatin.
> > 
> > http://codereview.chromium.org/2756003/diff/22001/10011
> > File net/base/cookie_monster.h (right):
> > 
> > http://codereview.chromium.org/2756003/diff/22001/10011#newcode291
> > net/base/cookie_monster.h:291: Lock lock_;
> > Not getting rid of the lock?

[eroman, 2010-06-15 20:36:12]

I think the ideal is to be able to register sync callbacks, but tell the
extension system to call the sync handlers on the IO thread rather than the UI
thread.

I am not familiar with the extension framework, but this seems like a very
reasonable thing for it to support.

[willchan no longer on Chromium, 2010-06-15 21:50:56]

I chatted with aa about this.  He said that all the extension
functions are async anyway from the extension's point of view.
async/sync only refers to how it's handled in the browser's UI thread.
 So, yeah, the simplest thing to do is make this async.  I'll make
that change and re-ping when ready for review.

On Tue, Jun 15, 2010 at 1:36 PM,  <eroman@chromium.org> wrote:
> I think the ideal is to be able to register sync callbacks, but tell the
> extension system to call the sync handlers on the IO thread rather than the
> UI
> thread.
>
> I am not familiar with the extension framework, but this seems like a very
> reasonable thing for it to support.
>
> http://codereview.chromium.org/2756003/show
>

[willchan no longer on Chromium, 2010-06-16 00:52:22]

Ok, the extension_cookie_api.* stuff is redone now and needs a re-review.

http://codereview.chromium.org/2756003/diff/11001/12001
File chrome/browser/automation/automation_provider.cc (right):

http://codereview.chromium.org/2756003/diff/11001/12001#newcode1177
chrome/browser/automation/automation_provider.cc:1177: const std::string& value,
On 2010/06/10 20:23:26, eroman wrote:
> indentatin.

Done.

http://codereview.chromium.org/2756003/diff/22001/10004
File chrome/browser/extensions/extension_cookies_api.h (right):

http://codereview.chromium.org/2756003/diff/22001/10004#newcode33
chrome/browser/extensions/extension_cookies_api.h:33: // the cookie store
context.  If the 'storeId' value isn't found in the
On 2010/06/10 19:51:23, Cindy Lau wrote:
> Please add the comments back in about the store_id output parameter from the
> original code.

Done.

http://codereview.chromium.org/2756003/diff/22001/10009
File chrome/browser/net/cookie_policy_browsertest.cc (right):

http://codereview.chromium.org/2756003/diff/22001/10009#newcode15
chrome/browser/net/cookie_policy_browsertest.cc:15: namespace {
On 2010/06/10 03:14:32, wtc wrote:
> Is this necessary?

I'm not sure.  The GetCookies() task might conflict, I dunno.  But I don't think
there's any reason for test code to be in the global namespace, since no one
should be calling into it.  Let me know if you care about this.

http://codereview.chromium.org/2756003/diff/22001/10011
File net/base/cookie_monster.h (right):

http://codereview.chromium.org/2756003/diff/22001/10011#newcode291
net/base/cookie_monster.h:291: Lock lock_;
On 2010/06/10 20:23:26, eroman wrote:
> Not getting rid of the lock?

Not yet.  I want to make sure I didn't miss anything.  I figured I'd do it in a
second pass after letting things bake.  Maybe I'm being overly paranoid?

[eroman, 2010-06-16 01:24:31]

http://codereview.chromium.org/2756003/diff/22001/10011
File net/base/cookie_monster.h (right):

http://codereview.chromium.org/2756003/diff/22001/10011#newcode291
net/base/cookie_monster.h:291: Lock lock_;
On 2010/06/16 00:52:22, willchan wrote:
> On 2010/06/10 20:23:26, eroman wrote:
> > Not getting rid of the lock?
> 
> Not yet.  I want to make sure I didn't miss anything.  I figured I'd do it in
a
> second pass after letting things bake.  Maybe I'm being overly paranoid?

I am fine with that. Can you add a TODO for removing it in that case?

http://codereview.chromium.org/2756003/diff/38001/39002
File chrome/browser/extensions/extension_cookies_api.cc (right):

http://codereview.chromium.org/2756003/diff/38001/39002#newcode188
chrome/browser/extensions/extension_cookies_api.cc:188: void
GetCookieFunction::RespondOnUIThread() {
The split of logic between GetXXXOnIOThread() and ResponseOnUIThread() seems
like it can be simplified.

How about putting all of the logic into GetXXXOnIOThread(), so
ResponseONUIThread() can be a light wrapper, say:

void ResponseOnUIThread(Value* value, bool success) {
  result_.reset(value);
  SendResponse(success);
}

Moreover, you could probably eliminate most of the additional parameters by
making them arguments to GetCookiesOnIOThead() instead.

http://codereview.chromium.org/2756003/diff/38001/39002#newcode245
chrome/browser/extensions/extension_cookies_api.cc:245: void
GetAllCookiesFunction::RespondOnUIThread() {
Same comment as above. Seems unecessary to add store_id_, url_, details_,
cookie_list_ as members when they could be arguments to GetCookiesOnIOThread(),
and let it do the creation of the ListValue*.

[willchan no longer on Chromium, 2010-06-16 20:34:50]

http://codereview.chromium.org/2756003/diff/22001/10011
File net/base/cookie_monster.h (right):

http://codereview.chromium.org/2756003/diff/22001/10011#newcode291
net/base/cookie_monster.h:291: Lock lock_;
On 2010/06/16 01:24:31, eroman wrote:
> On 2010/06/16 00:52:22, willchan wrote:
> > On 2010/06/10 20:23:26, eroman wrote:
> > > Not getting rid of the lock?
> > 
> > Not yet.  I want to make sure I didn't miss anything.  I figured I'd do it
in
> a
> > second pass after letting things bake.  Maybe I'm being overly paranoid?
> 
> I am fine with that. Can you add a TODO for removing it in that case?

Done.

http://codereview.chromium.org/2756003/diff/38001/39002
File chrome/browser/extensions/extension_cookies_api.cc (right):

http://codereview.chromium.org/2756003/diff/38001/39002#newcode245
chrome/browser/extensions/extension_cookies_api.cc:245: void
GetAllCookiesFunction::RespondOnUIThread() {
On 2010/06/16 01:24:31, eroman wrote:
> Same comment as above. Seems unecessary to add store_id_, url_, details_,
> cookie_list_ as members when they could be arguments to
GetCookiesOnIOThread(),
> and let it do the creation of the ListValue*.

I've address some bits of this, but I've kept most of it the same.  I didn't
implement it due to some complications.

* GetExtension() pulls the extension from the Profile.  This needs to happen on
the UI thread.
* parameter storage isn't well defined in tasks.  I'd have to copy all these
values to the IO thread, which isn't the end of the world.  The one
inconsistency, is that we'd also have to copy the scoped_refptr for the
URLRequestContextGetter, otherwise, the reference may disappear from underneath
us.  This is non-obvious to many people.
* My solution addresses the storage and ref counting issues by making the
function object hold everything.

Let me know what you think about this.  I have a preference for my way, but it's
not strong.  If you feel strongly, I am happy to switch to your suggestion
(other than GetExtension()).

[Cindy Lau, 2010-06-16 21:12:29]

http://codereview.chromium.org/2756003/diff/51001/52002
File chrome/browser/extensions/extension_cookies_api.cc (right):

http://codereview.chromium.org/2756003/diff/51001/52002#newcode176
chrome/browser/extensions/extension_cookies_api.cc:176: void
GetCookieFunction::GetCookiesOnIOThread() {
This function name could be improved, since you're basically implementing the
entire API in this function.  This latest change is kind of strange IMHO,
because now you've got all of GetCookieFunction implemented here, whereas
GetAllCookiesFunction splits the work differently because of GetExtension(). 
More consistency would be nice.

[willchan no longer on Chromium, 2010-06-16 21:16:08]

http://codereview.chromium.org/2756003/diff/51001/52002
File chrome/browser/extensions/extension_cookies_api.cc (right):

http://codereview.chromium.org/2756003/diff/51001/52002#newcode176
chrome/browser/extensions/extension_cookies_api.cc:176: void
GetCookieFunction::GetCookiesOnIOThread() {
On 2010/06/16 21:12:30, Cindy Lau wrote:
> This function name could be improved, since you're basically implementing the

I'm not sure what a better name would be.  Can you suggest one?  I think the
function name states pretty clearly what it's doing.

> entire API in this function.  This latest change is kind of strange IMHO,
> because now you've got all of GetCookieFunction implemented here, whereas
> GetAllCookiesFunction splits the work differently because of GetExtension(). 
> More consistency would be nice.

I agree more consistency would be nice.  I had adopted Eric's suggestion, but
ran into the GetExtension() change later.  Rather than make it consistent again
and risk having to re-write it all over again, I wanted to request comments
first.  Do you want to step in and state your preference here?  I'm going to
wait until Eric weighs in too before I proceed.

[eroman, 2010-06-18 00:22:34]

LGTM.

Thanks for addressing the comments from so many reviewers.

http://codereview.chromium.org/2756003/diff/51001/52001
File chrome/browser/automation/automation_provider.cc (right):

http://codereview.chromium.org/2756003/diff/51001/52001#newcode1167
chrome/browser/automation/automation_provider.cc:1167: ChromeThread::PostTask(
Can you catch the return value for PostTask, and then either CHECK() that it is
OK, or early return?

I suggest this as a safety net to avoid hanging forever in case the Post fails.

http://codereview.chromium.org/2756003/diff/51001/52001#newcode1170
chrome/browser/automation/automation_provider.cc:1170: event.Wait();
Do you know if testing is the only consumer of this? Or does ChromeFrame use it
too?

If the latter, I suggest adding a TODO to make the upper layer interface async,
so we don't need to block here.

http://codereview.chromium.org/2756003/diff/51001/52001#newcode1177
chrome/browser/automation/automation_provider.cc:1177: const std::string& value,
nit:indentation

http://codereview.chromium.org/2756003/diff/51001/52001#newcode1209
chrome/browser/automation/automation_provider.cc:1209: ChromeThread::PostTask(
Same comment as above, please bullet-proof this against hangs (my preference is
to convert the hang into a crash).

http://codereview.chromium.org/2756003/diff/51001/52002
File chrome/browser/extensions/extension_cookies_api.cc (right):

http://codereview.chromium.org/2756003/diff/51001/52002#newcode282
chrome/browser/extensions/extension_cookies_api.cc:282:
EXTENSION_FUNCTION_VALIDATE(details->GetBoolean(keys::kSecureKey, &secure_));
>80c

http://codereview.chromium.org/2756003/diff/51001/52003
File chrome/browser/extensions/extension_cookies_api.h (right):

http://codereview.chromium.org/2756003/diff/51001/52003#newcode64
chrome/browser/extensions/extension_cookies_api.h:64: // and are not threadsafe.
They modify the result_ member variable directly;
might want to clarify what it means now to be "not threadsafe", since the class
does in fact run on multiple threads.

http://codereview.chromium.org/2756003/diff/51001/52003#newcode96
chrome/browser/extensions/extension_cookies_api.h:96: void
GetCookiesOnIOThread();
nit: I wander if for consistency this should be called GetCookieOnIOThread()?
(singular).

http://codereview.chromium.org/2756003/diff/51001/52003#newcode113
chrome/browser/extensions/extension_cookies_api.h:113: void
GetCookiesOnIOThread();
nit: for consistency, I wander if this should be called
GetAllCookiesOnIOThread()?

[Cindy Lau, 2010-06-18 18:04:04]

LGTM

http://codereview.chromium.org/2756003/diff/51001/52002
File chrome/browser/extensions/extension_cookies_api.cc (right):

http://codereview.chromium.org/2756003/diff/51001/52002#newcode176
chrome/browser/extensions/extension_cookies_api.cc:176: void
GetCookieFunction::GetCookiesOnIOThread() {
On 2010/06/16 21:16:08, willchan wrote:
> On 2010/06/16 21:12:30, Cindy Lau wrote:
> > This function name could be improved, since you're basically implementing
the
> 
> I'm not sure what a better name would be.  Can you suggest one?  I think the
> function name states pretty clearly what it's doing.
> 
> > entire API in this function.  This latest change is kind of strange IMHO,
> > because now you've got all of GetCookieFunction implemented here, whereas
> > GetAllCookiesFunction splits the work differently because of GetExtension().

> > More consistency would be nice.
> 
> I agree more consistency would be nice.  I had adopted Eric's suggestion, but
> ran into the GetExtension() change later.  Rather than make it consistent
again
> and risk having to re-write it all over again, I wanted to request comments
> first.  Do you want to step in and state your preference here?  I'm going to
> wait until Eric weighs in too before I proceed.

My preference would be to put the bulk of the implementations in
RespondOnUIThread for consistency at this point. If you had ended up also using
Eric's suggestion of sending the input data as params to GetCookiesOnIOThread()
instead of member vars, it would have made more sense to try and put the
implementations there, but the storage/refcounting issues you brought up make
sense.

But we're splitting hairs here, I think. I'm fine with whatever you decide to
do, but if you leave the implementation as is then I second Eric's function name
suggestions.

http://codereview.chromium.org/2756003/diff/51001/52003
File chrome/browser/extensions/extension_cookies_api.h (right):

http://codereview.chromium.org/2756003/diff/51001/52003#newcode96
chrome/browser/extensions/extension_cookies_api.h:96: void
GetCookiesOnIOThread();
On 2010/06/18 00:22:34, eroman wrote:
> nit: I wander if for consistency this should be called GetCookieOnIOThread()?
> (singular).

+1 on this name suggestion

http://codereview.chromium.org/2756003/diff/51001/52003#newcode113
chrome/browser/extensions/extension_cookies_api.h:113: void
GetCookiesOnIOThread();
On 2010/06/18 00:22:34, eroman wrote:
> nit: for consistency, I wander if this should be called
> GetAllCookiesOnIOThread()?

+1

[willchan no longer on Chromium, 2010-06-18 21:46:17]

OK, I think I've nailed everything now.  Since I've got the LGTMs, I'm going to
land this once trybots look good, unless anyone else has any other comments.

http://codereview.chromium.org/2756003/diff/51001/52001
File chrome/browser/automation/automation_provider.cc (right):

http://codereview.chromium.org/2756003/diff/51001/52001#newcode1167
chrome/browser/automation/automation_provider.cc:1167: ChromeThread::PostTask(
On 2010/06/18 00:22:34, eroman wrote:
> Can you catch the return value for PostTask, and then either CHECK() that it
is
> OK, or early return?
> 
> I suggest this as a safety net to avoid hanging forever in case the Post
fails.

Done.

http://codereview.chromium.org/2756003/diff/51001/52001#newcode1170
chrome/browser/automation/automation_provider.cc:1170: event.Wait();
On 2010/06/18 00:22:34, eroman wrote:
> Do you know if testing is the only consumer of this? Or does ChromeFrame use
it
> too?

After tracing through the code, it looks like ChromeFrame uses 
InternetGetCookieA() or npapi::GetValueForURL(instance_, NPNURLVCookie, ...).

> 
> If the latter, I suggest adding a TODO to make the upper layer interface
async,
> so we don't need to block here.

http://codereview.chromium.org/2756003/diff/51001/52001#newcode1177
chrome/browser/automation/automation_provider.cc:1177: const std::string& value,
On 2010/06/18 00:22:34, eroman wrote:
> nit:indentation

Done.

http://codereview.chromium.org/2756003/diff/51001/52001#newcode1209
chrome/browser/automation/automation_provider.cc:1209: ChromeThread::PostTask(
On 2010/06/18 00:22:34, eroman wrote:
> Same comment as above, please bullet-proof this against hangs (my preference
is
> to convert the hang into a crash).

Done.

http://codereview.chromium.org/2756003/diff/51001/52002
File chrome/browser/extensions/extension_cookies_api.cc (right):

http://codereview.chromium.org/2756003/diff/51001/52002#newcode176
chrome/browser/extensions/extension_cookies_api.cc:176: void
GetCookieFunction::GetCookiesOnIOThread() {
On 2010/06/18 18:04:04, Cindy Lau wrote:
> On 2010/06/16 21:16:08, willchan wrote:
> > On 2010/06/16 21:12:30, Cindy Lau wrote:
> > > This function name could be improved, since you're basically implementing
> the
> > 
> > I'm not sure what a better name would be.  Can you suggest one?  I think the
> > function name states pretty clearly what it's doing.
> > 
> > > entire API in this function.  This latest change is kind of strange IMHO,
> > > because now you've got all of GetCookieFunction implemented here, whereas
> > > GetAllCookiesFunction splits the work differently because of
GetExtension().
> 
> > > More consistency would be nice.
> > 
> > I agree more consistency would be nice.  I had adopted Eric's suggestion,
but
> > ran into the GetExtension() change later.  Rather than make it consistent
> again
> > and risk having to re-write it all over again, I wanted to request comments
> > first.  Do you want to step in and state your preference here?  I'm going to
> > wait until Eric weighs in too before I proceed.
> 
> My preference would be to put the bulk of the implementations in
> RespondOnUIThread for consistency at this point. If you had ended up also
using
> Eric's suggestion of sending the input data as params to
GetCookiesOnIOThread()
> instead of member vars, it would have made more sense to try and put the
> implementations there, but the storage/refcounting issues you brought up make
> sense.
> 
> But we're splitting hairs here, I think. I'm fine with whatever you decide to
> do, but if you leave the implementation as is then I second Eric's function
name
> suggestions.

Done.

http://codereview.chromium.org/2756003/diff/51001/52002#newcode282
chrome/browser/extensions/extension_cookies_api.cc:282:
EXTENSION_FUNCTION_VALIDATE(details->GetBoolean(keys::kSecureKey, &secure_));
On 2010/06/18 00:22:34, eroman wrote:
> >80c

Done.

http://codereview.chromium.org/2756003/diff/51001/52003
File chrome/browser/extensions/extension_cookies_api.h (right):

http://codereview.chromium.org/2756003/diff/51001/52003#newcode64
chrome/browser/extensions/extension_cookies_api.h:64: // and are not threadsafe.
They modify the result_ member variable directly;
On 2010/06/18 00:22:34, eroman wrote:
> might want to clarify what it means now to be "not threadsafe", since the
class
> does in fact run on multiple threads.

Done.

http://codereview.chromium.org/2756003/diff/51001/52003#newcode96
chrome/browser/extensions/extension_cookies_api.h:96: void
GetCookiesOnIOThread();
On 2010/06/18 18:04:04, Cindy Lau wrote:
> On 2010/06/18 00:22:34, eroman wrote:
> > nit: I wander if for consistency this should be called
GetCookieOnIOThread()?
> > (singular).
> 
> +1 on this name suggestion

Done.

http://codereview.chromium.org/2756003/diff/51001/52003#newcode113
chrome/browser/extensions/extension_cookies_api.h:113: void
GetCookiesOnIOThread();
On 2010/06/18 18:04:04, Cindy Lau wrote:
> On 2010/06/18 00:22:34, eroman wrote:
> > nit: for consistency, I wander if this should be called
> > GetAllCookiesOnIOThread()?
> 
> +1

Done.