Add SafeBrowsingURLRequestContext which stores cookies.

Add SafeBrowsingURLRequestContext which stores cookies.

Rename OnIOInitialize and OnIOShutdown methods to more accurately match their current use.

BUG=103243
TEST=tcpdump it up, run chrome twice in a row (waiting for safebrowsing updates to occur), verify that cookies from both connections were the same.


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

[willchan no longer on Chromium, 2012-02-04 22:30:20]

I did a quick skim. I ought to look at this more closely later on, the shutdown
semantics are always tricky.

http://codereview.chromium.org/9310020/diff/1/chrome/browser/safe_browsing/sa...
File chrome/browser/safe_browsing/safe_browsing_service.cc (right):

http://codereview.chromium.org/9310020/diff/1/chrome/browser/safe_browsing/sa...
chrome/browser/safe_browsing/safe_browsing_service.cc:108: class
SafeBrowsingURLRequestContext : public net::URLRequestContext {
Can you give this guy a LeakTracker too, and in IOThread::CleanUp make sure that
it hasn't been leaked? All URLRequests that SafeBrowsingService started up
should have been canceled by this point, right?

http://codereview.chromium.org/9310020/diff/1/chrome/browser/safe_browsing/sa...
chrome/browser/safe_browsing/safe_browsing_service.cc:247:
BrowserThread::PostTask(
I don't know if this is safe.  It'll AddRef() SafeBrowsingService (which hasn't
been put into a scoped_refptr yet, since we're in the constructor), and then
there's a possible race if the IO thread task finishes before we exit the
constructor and the scoped_refptr gets to AddRef() this object.

http://codereview.chromium.org/9310020/diff/1/chrome/browser/safe_browsing/sa...
File chrome/browser/safe_browsing/safe_browsing_service.h (right):

http://codereview.chromium.org/9310020/diff/1/chrome/browser/safe_browsing/sa...
chrome/browser/safe_browsing/safe_browsing_service.h:356: const std::string&
wrapped_key);
indentation off

[mattm, 2012-02-07 01:28:01]

http://codereview.chromium.org/9310020/diff/1/chrome/browser/safe_browsing/sa...
File chrome/browser/safe_browsing/safe_browsing_service.cc (right):

http://codereview.chromium.org/9310020/diff/1/chrome/browser/safe_browsing/sa...
chrome/browser/safe_browsing/safe_browsing_service.cc:108: class
SafeBrowsingURLRequestContext : public net::URLRequestContext {
/On 2012/02/04 22:30:20, willchan wrote:
> Can you give this guy a LeakTracker too, and in IOThread::CleanUp make sure
that
> it hasn't been leaked?

done

> All URLRequests that SafeBrowsingService started up
> should have been canceled by this point, right?

Yeah.

http://codereview.chromium.org/9310020/diff/1/chrome/browser/safe_browsing/sa...
chrome/browser/safe_browsing/safe_browsing_service.cc:247:
BrowserThread::PostTask(
On 2012/02/04 22:30:20, willchan wrote:
> I don't know if this is safe.  It'll AddRef() SafeBrowsingService (which
hasn't
> been put into a scoped_refptr yet, since we're in the constructor), and then
> there's a possible race if the IO thread task finishes before we exit the
> constructor and the scoped_refptr gets to AddRef() this object.

Ah, good catch! (though in patch set 2 I moved it into Initialize to fix another
problem).

http://codereview.chromium.org/9310020/diff/1/chrome/browser/safe_browsing/sa...
File chrome/browser/safe_browsing/safe_browsing_service.h (right):

http://codereview.chromium.org/9310020/diff/1/chrome/browser/safe_browsing/sa...
chrome/browser/safe_browsing/safe_browsing_service.h:356: const std::string&
wrapped_key);
On 2012/02/04 22:30:20, willchan wrote:
> indentation off

Done.

http://codereview.chromium.org/9310020/diff/8001/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/safe_browsing_service.cc (right):

http://codereview.chromium.org/9310020/diff/8001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/safe_browsing_service.cc:626: if
(system_url_request_context_getter)
Had to update DownloadProtectionServiceTest to call Initialize and ShutDown so
that sb_service_->download_protection_service() is created, which introduces the
system_url_request_context_getter being NULL problem again.  This fix doesn't
seem great but let me know if you have a better idea.

[rvargas (doing something else), 2012-02-08 00:09:46]

Drive-by:

This adds persistent storage that is not tied to any UI control to delete it
(browsing data remover), so make sure that the privacy group is OK with it.

[willchan no longer on Chromium, 2012-02-09 19:41:30]

lgtm

http://codereview.chromium.org/9310020/diff/8001/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/safe_browsing_service.cc (right):

http://codereview.chromium.org/9310020/diff/8001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/safe_browsing_service.cc:309:
BrowserThread::PostTask(
I think this is right, but I just want to point out that this will add a
refcount that will be decremented on the IO thread. SafeBrowsingService appears
to be RefCountedThreadSafe with deletion on the UI thread. It's not clear to me
how it ever gets deleted, since hasn't the UI thread already stopped by this
point? Is this just always leaked?

That said, I don't think this is a big change in behavior because Stop() already
seems to post to the IO thread. Anyway, I found it intriguing. I'm fine with
this as long as you safe browsing folks are.

[mattm, 2012-02-09 22:40:09]

http://codereview.chromium.org/9310020/diff/8001/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/safe_browsing_service.cc (right):

http://codereview.chromium.org/9310020/diff/8001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/safe_browsing_service.cc:309:
BrowserThread::PostTask(
On 2012/02/09 19:41:30, willchan wrote:
> I think this is right, but I just want to point out that this will add a
> refcount that will be decremented on the IO thread. SafeBrowsingService
appears
> to be RefCountedThreadSafe with deletion on the UI thread. It's not clear to
me
> how it ever gets deleted, since hasn't the UI thread already stopped by this
> point? Is this just always leaked?

I think this works out in practice, since BrowserProcessImpl maintains a
reference to the SafeBrowsingService.  By the time BrowserProcessImpl gets
destructed, the tasks with references to SBService on the IOthread have already
been run, so the BrowserProcessImpl should be the one holding the last ref, and
the message loop doesn't need to be running if the last ref is released on the
correct thread.  (I'm not 100% sure if there isn't some case where something
else might still hold a ref to it, but in a few quick tests it did get
destructed.)

> That said, I don't think this is a big change in behavior because Stop()
already
> seems to post to the IO thread. Anyway, I found it intriguing. I'm fine with
> this as long as you safe browsing folks are.

[willchan no longer on Chromium, 2012-02-09 22:49:54]

OK, this should not be refcounted :P Obviously still LGTM, and if you had
spare cycles, it'd be great to kill off the refcounting in another CL.

On Thu, Feb 9, 2012 at 2:40 PM, <mattm@chromium.org> wrote:

>
> http://codereview.chromium.**org/9310020/diff/8001/chrome/**
>
browser/safe_browsing/safe_**browsing_service.cc<http://codereview.chromium.org/9310020/diff/8001/chrome/browser/safe_browsing/safe_browsing_service.cc>
> File chrome/browser/safe_browsing/**safe_browsing_service.cc (right):
>
> http://codereview.chromium.**org/9310020/diff/8001/chrome/**
>
browser/safe_browsing/safe_**browsing_service.cc#newcode309<http://codereview.chromium.org/9310020/diff/8001/chrome/browser/safe_browsing/safe_browsing_service.cc#newcode309>
> chrome/browser/safe_browsing/**safe_browsing_service.cc:309:
> BrowserThread::PostTask(
> On 2012/02/09 19:41:30, willchan wrote:
>
>> I think this is right, but I just want to point out that this will add
>>
> a
>
>> refcount that will be decremented on the IO thread.
>>
> SafeBrowsingService appears
>
>> to be RefCountedThreadSafe with deletion on the UI thread. It's not
>>
> clear to me
>
>> how it ever gets deleted, since hasn't the UI thread already stopped
>>
> by this
>
>> point? Is this just always leaked?
>>
>
> I think this works out in practice, since BrowserProcessImpl maintains a
> reference to the SafeBrowsingService.  By the time BrowserProcessImpl
> gets destructed, the tasks with references to SBService on the IOthread
> have already been run, so the BrowserProcessImpl should be the one
> holding the last ref, and the message loop doesn't need to be running if
> the last ref is released on the correct thread.  (I'm not 100% sure if
> there isn't some case where something else might still hold a ref to it,
> but in a few quick tests it did get destructed.)
>
>
>  That said, I don't think this is a big change in behavior because
>>
> Stop() already
>
>> seems to post to the IO thread. Anyway, I found it intriguing. I'm
>>
> fine with
>
>> this as long as you safe browsing folks are.
>>
>
>
http://codereview.chromium.**org/9310020/<http://codereview.chromium.org/9310...
>

[commit-bot: I haz the power, 2012-02-13 21:47:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mattm@chromium.org/9310020/8001

[commit-bot: I haz the power, 2012-02-13 22:34:29]

Try job failure for 9310020-8001 (retry) on linux_clang for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...

[commit-bot: I haz the power, 2012-02-13 23:00:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mattm@chromium.org/9310020/8001

[commit-bot: I haz the power, 2012-02-14 00:56:45]

Change committed as 121794