Convert URLFetcher::Delegates to use an interface in content/public/common. Also remove the old U...

Convert URLFetcher::Delegates to use an interface in content/public/common. Also remove the old URLFetcher delegate callback while I'm touching all of them.

BUG=98716, 83592
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=106949

[Ilya Sherman, 2011-10-23 00:20:11]

http://codereview.chromium.org/8373021/diff/2197/chrome/browser/autofill/auto...
File chrome/browser/autofill/autofill_download.cc (right):

http://codereview.chromium.org/8373021/diff/2197/chrome/browser/autofill/auto...
chrome/browser/autofill/autofill_download.cc:322:
CHECK(source->GetResponseAsString(&response_body));
nit: Do we really need a CHECK() here?  Per Brett's recent email, I'd expect a
DCHECK to be preferable.

[jam, 2011-10-23 01:29:18]

http://codereview.chromium.org/8373021/diff/2197/chrome/browser/autofill/auto...
File chrome/browser/autofill/autofill_download.cc (right):

http://codereview.chromium.org/8373021/diff/2197/chrome/browser/autofill/auto...
chrome/browser/autofill/autofill_download.cc:322:
CHECK(source->GetResponseAsString(&response_body));
On 2011/10/23 00:20:12, Ilya Sherman wrote:
> nit: Do we really need a CHECK() here?  Per Brett's recent email, I'd expect a
> DCHECK to be preferable.

brett's email was about logging. do CHECKs also add that much code? I doubt it.

also, I put a CHECK because that's how the code used to do it before my change
in the GetResponseStringRef implementation

[Ilya Sherman, 2011-10-23 09:17:16]

On 2011/10/23 01:29:18, John Abd-El-Malek wrote:
>
http://codereview.chromium.org/8373021/diff/2197/chrome/browser/autofill/auto...
> File chrome/browser/autofill/autofill_download.cc (right):
> 
>
http://codereview.chromium.org/8373021/diff/2197/chrome/browser/autofill/auto...
> chrome/browser/autofill/autofill_download.cc:322:
> CHECK(source->GetResponseAsString(&response_body));
> On 2011/10/23 00:20:12, Ilya Sherman wrote:
> > nit: Do we really need a CHECK() here?  Per Brett's recent email, I'd expect
a
> > DCHECK to be preferable.
> 
> brett's email was about logging. do CHECKs also add that much code? I doubt
it.
> 
> also, I put a CHECK because that's how the code used to do it before my change
> in the GetResponseStringRef implementation

From Brett's email:

"Reviewers should please challenge all non-"D" logging statements. We
should treat these as something exceptional and temporary, like we do
when we're trying to track down an obscure crash and somebody will put
a bunch of extra CHECKs in. If you have a real need for non-debug
logging, there should be a reason for why you need it and a bug for
removing it when this reason is over.

Continue to use CHECK/DLOG(FATAL) for security sensitive issues where
we want to terminate the program. But these should probably be the
only case and should be rare."

Maybe check with him?  (No pun intended.)

[jam, 2011-10-23 22:01:45]

ah thanks, I hadn't read that thread closely. I didn't know that CHECKs
would also have a cost.

I'm fine with taking them out altogether. Will: do you have an opinion on
this?

Adding DCHECKs would make the code ugly, since
DCHECK(source->GetResponseAsString..) wouldn't work because in release it
would be compiled out. The code previously used to CHECK in that function,
and wouldn't return if it failed, so I'm hesitant to change things to early
return.

On Sun, Oct 23, 2011 at 2:17 AM, <isherman@chromium.org> wrote:

> On 2011/10/23 01:29:18, John Abd-El-Malek wrote:
>
> http://codereview.chromium.**org/8373021/diff/2197/chrome/**
>
browser/autofill/autofill_**download.cc<http://codereview.chromium.org/8373021/diff/2197/chrome/browser/autofill/autofill_download.cc>
>
>> File chrome/browser/autofill/**autofill_download.cc (right):
>>
>
>
> http://codereview.chromium.**org/8373021/diff/2197/chrome/**
>
browser/autofill/autofill_**download.cc#newcode322<http://codereview.chromium.org/8373021/diff/2197/chrome/browser/autofill/autofill_download.cc#newcode322>
>
>> chrome/browser/autofill/**autofill_download.cc:322:
>> CHECK(source->**GetResponseAsString(&response_**body));
>> On 2011/10/23 00:20:12, Ilya Sherman wrote:
>> > nit: Do we really need a CHECK() here?  Per Brett's recent email, I'd
>> expect
>>
> a
>
>> > DCHECK to be preferable.
>>
>
>  brett's email was about logging. do CHECKs also add that much code? I
>> doubt
>>
> it.
>
>  also, I put a CHECK because that's how the code used to do it before my
>> change
>> in the GetResponseStringRef implementation
>>
>
> From Brett's email:
>
> "Reviewers should please challenge all non-"D" logging statements. We
> should treat these as something exceptional and temporary, like we do
> when we're trying to track down an obscure crash and somebody will put
> a bunch of extra CHECKs in. If you have a real need for non-debug
> logging, there should be a reason for why you need it and a bug for
> removing it when this reason is over.
>
> Continue to use CHECK/DLOG(FATAL) for security sensitive issues where
> we want to terminate the program. But these should probably be the
> only case and should be rare."
>
> Maybe check with him?  (No pun intended.)
>
>
http://codereview.chromium.**org/8373021/<http://codereview.chromium.org/8373...
>

[jam, 2011-10-24 02:30:30]

On 2011/10/23 22:01:45, John Abd-El-Malek wrote:
> ah thanks, I hadn't read that thread closely. I didn't know that CHECKs
> would also have a cost.
> 
> I'm fine with taking them out altogether. Will: do you have an opinion on
> this?
> 
> Adding DCHECKs would make the code ugly, since
> DCHECK(source->GetResponseAsString..) wouldn't work because in release it
> would be compiled out. The code previously used to CHECK in that function,
> and wouldn't return if it failed, so I'm hesitant to change things to early
> return.

I took a look through the crash reports, and this CHECK was never hit anyways,
so I just took it out.

[willchan no longer on Chromium, 2011-10-24 17:12:33]

LGTM

http://codereview.chromium.org/8373021/diff/5226/content/public/common/url_fe...
File content/public/common/url_fetcher_delegate.h (right):

http://codereview.chromium.org/8373021/diff/5226/content/public/common/url_fe...
content/public/common/url_fetcher_delegate.h:18: class URLRequestStatus;
Are these two guys needed?

[jam, 2011-10-24 17:37:10]

http://codereview.chromium.org/8373021/diff/5226/content/public/common/url_fe...
File content/public/common/url_fetcher_delegate.h (right):

http://codereview.chromium.org/8373021/diff/5226/content/public/common/url_fe...
content/public/common/url_fetcher_delegate.h:18: class URLRequestStatus;
On 2011/10/24 17:12:33, willchan wrote:
> Are these two guys needed?

Done. I had those left over from when I had the old OnURLFetchComplete method.
also removed GURL

[Sam Kerner (Chrome), 2011-10-24 18:36:25]

http://codereview.chromium.org/8373021/diff/10062/content/common/net/url_fetc...
File content/common/net/url_fetcher.h (left):

http://codereview.chromium.org/8373021/diff/10062/content/common/net/url_fetc...
content/common/net/url_fetcher.h:251: virtual const std::string&
GetResponseStringRef() const;
Does GetResponseAsString() cause a copy of the string to be made?  It depends on
how std::string is implemented.  If 

std::string a = "long long long long string";
std::string b = a;

Is the string in memory twice?  I ask because some of the items fetched by
URLFetcher are large, and making an extra copy might be an issue.  Passing the
reference avoids this.

[willchan no longer on Chromium, 2011-10-24 18:42:12]

http://codereview.chromium.org/8373021/diff/10062/content/common/net/url_fetc...
File content/common/net/url_fetcher.h (left):

http://codereview.chromium.org/8373021/diff/10062/content/common/net/url_fetc...
content/common/net/url_fetcher.h:251: virtual const std::string&
GetResponseStringRef() const;
On 2011/10/24 18:36:25, Sam Kerner (Chrome) wrote:
> Does GetResponseAsString() cause a copy of the string to be made?  It depends
on
> how std::string is implemented.  If 
> 
> std::string a = "long long long long string";
> std::string b = a;
> 
> Is the string in memory twice?  I ask because some of the items fetched by
> URLFetcher are large, and making an extra copy might be an issue.  Passing the
> reference avoids this.

I don't think using URLFetcher for fetching large items is particularly
advisable. It's been done for simplicity's sake, but it's a bit disturbing to me
that we're requiring MB-sized contiguous memory buffers.

That said, I only looked at the url_fetcher changes and neglected to look at the
consumers to see how much of a regression this change would entail. It might
indeed be a blocker for this change.

[Sam Kerner (Chrome), 2011-10-24 19:43:11]

On 2011/10/24 18:42:12, willchan wrote:
>
http://codereview.chromium.org/8373021/diff/10062/content/common/net/url_fetc...
> File content/common/net/url_fetcher.h (left):
> 
>
http://codereview.chromium.org/8373021/diff/10062/content/common/net/url_fetc...
> content/common/net/url_fetcher.h:251: virtual const std::string&
> GetResponseStringRef() const;
> On 2011/10/24 18:36:25, Sam Kerner (Chrome) wrote:
> > Does GetResponseAsString() cause a copy of the string to be made?  It
depends
> on
> > how std::string is implemented.  If 
> > 
> > std::string a = "long long long long string";
> > std::string b = a;
> > 
> > Is the string in memory twice?  I ask because some of the items fetched by
> > URLFetcher are large, and making an extra copy might be an issue.  Passing
the
> > reference avoids this.
> 
> I don't think using URLFetcher for fetching large items is particularly
> advisable. It's been done for simplicity's sake, but it's a bit disturbing to
me
> that we're requiring MB-sized contiguous memory buffers.

I added the ability to fetch to a file so that the extension updater would not
make a URLFetcher hold the whole extension in memory.  Perhaps we should find
other users who are fetching large amounts of data and get them to fetch to a
file instead of a string.

A histogram showing response size for each user of a urlfetcher would allow us
to see which ones need attention.  Keying that histogram on the code that
instantiated the URLFetcher would make it easy to assign blame.  Could we add a
FROM_HERE parameter to the URLFetcher factory, or is there an easier way?

> That said, I only looked at the url_fetcher changes and neglected to look at
the
> consumers to see how much of a regression this change would entail. It might
> indeed be a blocker for this change.

[jam, 2011-10-24 20:04:29]

I agree that people fetching large data shouldn't be storing it in strings.
I'm not familiar enough with every consumer to know when this is the case. I
had removed the function which returns a reference per 83592 which you had
filed.

On Mon, Oct 24, 2011 at 12:43 PM, <skerner@chromium.org> wrote:

> On 2011/10/24 18:42:12, willchan wrote:
>
> http://codereview.chromium.**org/8373021/diff/10062/**
>
content/common/net/url_**fetcher.h<http://codereview.chromium.org/8373021/diff/10062/content/common/net/url_fetcher.h>
>
>> File content/common/net/url_**fetcher.h (left):
>>
>
>
> http://codereview.chromium.**org/8373021/diff/10062/**
>
content/common/net/url_**fetcher.h#oldcode251<http://codereview.chromium.org/8373021/diff/10062/content/common/net/url_fetcher.h#oldcode251>
>
>> content/common/net/url_**fetcher.h:251: virtual const std::string&
>> GetResponseStringRef() const;
>> On 2011/10/24 18:36:25, Sam Kerner (Chrome) wrote:
>> > Does GetResponseAsString() cause a copy of the string to be made?  It
>>
> depends
>
>> on
>> > how std::string is implemented.  If
>> >
>> > std::string a = "long long long long string";
>> > std::string b = a;
>> >
>> > Is the string in memory twice?  I ask because some of the items fetched
>> by
>> > URLFetcher are large, and making an extra copy might be an issue.
>>  Passing
>>
> the
>
>> > reference avoids this.
>>
>
>  I don't think using URLFetcher for fetching large items is particularly
>> advisable. It's been done for simplicity's sake, but it's a bit disturbing
>> to
>>
> me
>
>> that we're requiring MB-sized contiguous memory buffers.
>>
>
> I added the ability to fetch to a file so that the extension updater would
> not
> make a URLFetcher hold the whole extension in memory.  Perhaps we should
> find
> other users who are fetching large amounts of data and get them to fetch to
> a
> file instead of a string.
>
> A histogram showing response size for each user of a urlfetcher would allow
> us
> to see which ones need attention.  Keying that histogram on the code that
> instantiated the URLFetcher would make it easy to assign blame.  Could we
> add a
> FROM_HERE parameter to the URLFetcher factory, or is there an easier way?
>
>
>  That said, I only looked at the url_fetcher changes and neglected to look
>> at
>>
> the
>
>> consumers to see how much of a regression this change would entail. It
>> might
>> indeed be a blocker for this change.
>>
>
>
>
>
http://codereview.chromium.**org/8373021/<http://codereview.chromium.org/8373...
>

[willchan no longer on Chromium, 2011-10-25 18:09:39]

http://codereview.chromium.org/8373021/diff/10062/chrome/browser/metrics/metr...
File chrome/browser/metrics/metrics_service.cc (right):

http://codereview.chromium.org/8373021/diff/10062/chrome/browser/metrics/metr...
chrome/browser/metrics/metrics_service.cc:1076: current_fetch_.reset(NULL);  //
We're not allowed to re-use it.
Here's the URLFetcher getting deleted. current_fetch_ == source. This is the
result of the use-after-free crashes.