Fixed a few valgrind tests by not assuming the UrlFetcher arrives with the same URL.

Fixed a few valgrind tests by not assuming the UrlFetcher arrives with the same URL.

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

[MAD, 2011-06-23 17:47:40]

I've changed the way we handle the URLFetches hoping it will fix the Valgrind
issues...

If you think someone else should look at it, please let me know...

[MAD, 2011-06-23 18:50:28]

On 2011/06/23 17:47:40, MAD wrote:
> I've changed the way we handle the URLFetches hoping it will fix the Valgrind
> issues...
> 
> If you think someone else should look at it, please let me know...

OK, this doesn't work for some tests which endup releasing the UrlFetcher object
in wrong thread... working on an alternate solution... :-(

[MAD, 2011-06-23 22:49:12]

OK, I think it's fine now... Please take a look...

Thanks!

BYE
MAD

[Timur Iskhodzhanov, 2011-06-24 16:04:12]

deferring to Chris (he's the current memory sheriff) instead of Alexander

[cbentzel, 2011-06-24 19:56:08]

LGTM

http://codereview.chromium.org/7246010/diff/1009/chrome/browser/translate/tra...
File chrome/browser/translate/translate_manager.cc (right):

http://codereview.chromium.org/7246010/diff/1009/chrome/browser/translate/tra...
chrome/browser/translate/translate_manager.cc:354: // Looks like crash on Mac is
possibly caused with callback entering here
Should you add a CHECK here so we can see failures? This is a definite
programming problem if it happens.

[cbentzel, 2011-06-24 20:02:04]

Also - it looks like this fixes a potential use-after-free of TranslateManager.

The UrlFetcherDelegate does not unregister as delegate from the UrlFetcher in
it's destructor - in fact, it looks like there is no way to change the delegate
post-construction - so we need to be doing the scoped_ptr's here to be safe. 

I'm worried that the incorrect-source issue is a red herring - but OK with
keeping it in if you augment with a CHECK.

[MAD, 2011-06-27 18:42:40]

BTW, the check was taken from here:
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/autofi...

which was added by George in rev:
43923<http://src.chromium.org/viewvc/chrome?view=rev&revision=43923>

I didn't get to add the DCHECK before you committed the change, is it worth
adding now as an after thought? Should I also add it to the
autofil_download.cc file?

Thanks!

On Fri, Jun 24, 2011 at 4:02 PM, <cbentzel@chromium.org> wrote:

> Also - it looks like this fixes a potential use-after-free of
> TranslateManager.
>
> The UrlFetcherDelegate does not unregister as delegate from the UrlFetcher
> in
> it's destructor - in fact, it looks like there is no way to change the
> delegate
> post-construction - so we need to be doing the scoped_ptr's here to be
> safe.
>
> I'm worried that the incorrect-source issue is a red herring - but OK with
> keeping it in if you augment with a CHECK.
>
>
>
http://codereview.chromium.**org/7246010/<http://codereview.chromium.org/7246...
>