Chromium Code Reviews
Help | Chromium Project | Sign in
(475)

Issue 2756003: Make CookieMonster NonThreadSafe. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 years, 10 months ago by willchan
Modified:
2 years, 11 months ago
CC:
chromium-reviews_chromium.org, Matt Perry
Base URL:
http://src.chromium.org/git/chromium.git
Visibility:
Public.

Description

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

Patch Set 1 #

Total comments: 17

Patch Set 2 : Address comments. #

Total comments: 7

Patch Set 3 : Address cindylau's comments. #

Total comments: 12

Patch Set 4 : Address cindy's comments. #

Patch Set 5 : Oops, build fix and address eric's nits. #

Patch Set 6 : Use async functions instead. Make ExtensionFunction RefCountedThreadSafe. #

Patch Set 7 : Update includes. #

Total comments: 3

Patch Set 8 : Oops, posted to wrong thread. #

Patch Set 9 : Address eroman comments. #

Total comments: 22

Patch Set 10 : Address eroman's and cindylau's comments. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+459 lines, -130 lines) Lint Patch
M chrome/browser/automation/automation_provider.cc View 1 2 3 4 5 6 7 8 9 5 chunks +114 lines, -11 lines 0 comments ? errors Download
M chrome/browser/extensions/extension_cookies_api.h View 1 2 3 4 5 6 7 8 9 4 chunks +65 lines, -14 lines 0 comments ? errors Download
M chrome/browser/extensions/extension_cookies_api.cc View 1 2 3 4 5 6 7 8 9 7 chunks +178 lines, -74 lines 0 comments ? errors Download
M chrome/browser/extensions/extension_cookies_helpers.h View 1 2 chunks +3 lines, -1 line 0 comments ? errors Download
M chrome/browser/extensions/extension_cookies_helpers.cc View 1 chunk +2 lines, -3 lines 0 comments ? errors Download
M chrome/browser/extensions/extension_data_deleter.h View 1 chunk +4 lines, -0 lines 0 comments ? errors Download
M chrome/browser/extensions/extension_data_deleter.cc View 1 chunk +18 lines, -8 lines 0 comments ? errors Download
M chrome/browser/extensions/extension_function.h View 2 chunks +2 lines, -5 lines 0 comments ? errors Download
M chrome/browser/net/cookie_policy_browsertest.cc View 6 chunks +50 lines, -12 lines 0 comments ? errors Download
M net/base/cookie_monster.h View 1 2 3 4 5 6 7 8 5 chunks +10 lines, -2 lines 0 comments ? errors Download
M net/base/cookie_monster.cc View 1 12 chunks +13 lines, -0 lines 0 comments ? errors Download
Commit:

Messages

Total messages: 24
willchan
rdsmith: Please review CookieMonster changes and AutomationProvider stuff. jochen: Please review the ExtensionDataDeleter changes cindylau: ...
3 years, 10 months ago #1
jochen
shouldn't you also change cookie store then? it's ref counted thread safe.. please also run ...
3 years, 10 months ago #2
rdsmith
I like the approach your taking, but have some comments inline. Top level comments: -- ...
3 years, 10 months ago #3
willchan
On 2010/06/09 05:23:34, jochen wrote: > shouldn't you also change cookie store then? it's ref ...
3 years, 10 months ago #4
rdsmith
On 2010/06/09 19:36:50, willchan wrote: > On 2010/06/09 05:23:34, jochen wrote: > > shouldn't you ...
3 years, 10 months ago #5
willchan
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, ...
3 years, 10 months ago #6
rdsmith
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 ...
3 years, 10 months ago #7
Cindy Lau
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 ...
3 years, 10 months ago #8
willchan
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: > ...
3 years, 10 months ago #9
wtc
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?
3 years, 10 months ago #10
Cindy Lau
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 { ...
3 years, 10 months ago #11
eroman
The blocking wait being done here in the extension API is pretty nasty. It seems ...
3 years, 10 months ago #12
jochen
On 2010/06/10 20:23:26, eroman wrote: > The blocking wait being done here in the extension ...
3 years, 10 months ago #13
willchan
On 2010/06/14 10:53:57, jochen wrote: > On 2010/06/10 20:23:26, eroman wrote: > > The blocking ...
3 years, 10 months ago #14
eroman
I think the ideal is to be able to register sync callbacks, but tell the ...
3 years, 10 months ago #15
willchan
I chatted with aa about this. He said that all the extension functions are async ...
3 years, 10 months ago #16
willchan
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): ...
3 years, 10 months ago #17
eroman
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 ...
3 years, 10 months ago #18
willchan
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 ...
3 years, 10 months ago #19
Cindy Lau
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, ...
3 years, 10 months ago #20
willchan
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: ...
3 years, 10 months ago #21
eroman
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 ...
3 years, 10 months ago #22
Cindy Lau
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: ...
3 years, 10 months ago #23
willchan
3 years, 10 months ago #24
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1280:2d3e6564b7b6