Fix race condition in WebstoreInstallHelper

Fix race condition in WebstoreInstallHelper.

Before this CL, several member variables were accessed concurrently on the UI and IO threads. Fix this by using the SafeJsonParser helper class which handles the thread hopping, so that the WebstoreInstallHelper can live completely on the UI thread.

Committed: https://crrev.com/8457c218e1df6bf2789658cd9c6205c08c2f1d41
Cr-Commit-Position: refs/heads/master@{#332171}

[Marc Treib, 2015-05-27 08:23:03]

treib@chromium.org changed reviewers:
+ asargent@chromium.org

[Marc Treib, 2015-05-27 08:23:04]

PTAL!

https://codereview.chromium.org/1153143002/diff/1/chrome/browser/bitmap_fetch...
File chrome/browser/bitmap_fetcher/bitmap_fetcher.cc (left):

https://codereview.chromium.org/1153143002/diff/1/chrome/browser/bitmap_fetch...
chrome/browser/bitmap_fetcher/bitmap_fetcher.cc:40:
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
I don't think this DCHECK should be here. It's the only thread check in the
whole file, and I see no reason why this should only be allowed on the UI
thread.

[asargent_no_longer_on_chrome, 2015-05-27 16:49:24]

See inline comment for a potential problem with the URLRequestContextGetter.

Also, if we stick with this approach, a small nit is that it would be nice to
include comments in the .h file to make it clear which variables should be used
on which thread; perhaps grouping their declarations together.

We might even consider making it more self-documenting by doing something like
having a separate private struct for the data that should be used on the IO
thread, with a name like "IOThreadData" or whatever, so that you'd see code
like:


iothread_data_->utility_host->Send(...)

https://codereview.chromium.org/1153143002/diff/1/chrome/browser/extensions/w...
File chrome/browser/extensions/webstore_install_helper.cc (right):

https://codereview.chromium.org/1153143002/diff/1/chrome/browser/extensions/w...
chrome/browser/extensions/webstore_install_helper.cc:71: context_getter_,
std::string(),
There's a comment in the .h file for context_getter_ that says "Only usable on
UI thread". Perhaps that's a problem? I read the comment on
BrowserContext::GetRequestContext which says:

// Returns the request context information associated with this context.  Call
// this only on the UI thread, since it can send notifications that should
// happen on the UI thread.

It's not totally clear to me if this implies that it's ok to *use* the result on
another thread after having gotten it on the UI thread. It might be worth
checking with some of the folks on the networking team.

[Marc Treib, 2015-05-28 10:03:50]

I've found a better way of doing the whole thing: Using SafeJsonParser. Now
WebstoreInstallHelper doesn't have to handle the yucky UtilityProcessHost stuff
and thread hopping, and can live completely on the UI thread :)

https://codereview.chromium.org/1153143002/diff/1/chrome/browser/extensions/w...
File chrome/browser/extensions/webstore_install_helper.cc (right):

https://codereview.chromium.org/1153143002/diff/1/chrome/browser/extensions/w...
chrome/browser/extensions/webstore_install_helper.cc:71: context_getter_,
std::string(),
On 2015/05/27 16:49:23, Antony Sargent wrote:
> There's a comment in the .h file for context_getter_ that says "Only usable on
> UI thread". Perhaps that's a problem? I read the comment on
> BrowserContext::GetRequestContext which says:
> 
> // Returns the request context information associated with this context.  Call
> // this only on the UI thread, since it can send notifications that should
> // happen on the UI thread.
> 
> It's not totally clear to me if this implies that it's ok to *use* the result
on
> another thread after having gotten it on the UI thread. It might be worth
> checking with some of the folks on the networking team. 

Good catch! Looks like the URL fetching should really live on the UI thread.

https://codereview.chromium.org/1153143002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/webstore_install_helper.h (right):

https://codereview.chromium.org/1153143002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/webstore_install_helper.h:38: : public
base::RefCountedThreadSafe<WebstoreInstallHelper>,
Before, we inherited that via UtilityProcessHostClient. I'm not sure if the
"ThreadSafe" is really needed - seems a bit pointless since the class can only
be used on the UI thread anyway?

[asargent_no_longer_on_chrome, 2015-05-28 17:08:47]

nice, lgtm

https://codereview.chromium.org/1153143002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/webstore_install_helper.h (right):

https://codereview.chromium.org/1153143002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/webstore_install_helper.h:38: : public
base::RefCountedThreadSafe<WebstoreInstallHelper>,
On 2015/05/28 10:03:49, Marc Treib wrote:
> Before, we inherited that via UtilityProcessHostClient. I'm not sure if the
> "ThreadSafe" is really needed - seems a bit pointless since the class can only
> be used on the UI thread anyway?

Agreed - feel free to switch to plain RefCounted.

[Marc Treib, 2015-05-29 07:48:49]

https://codereview.chromium.org/1153143002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/webstore_install_helper.h (right):

https://codereview.chromium.org/1153143002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/webstore_install_helper.h:38: : public
base::RefCountedThreadSafe<WebstoreInstallHelper>,
On 2015/05/28 17:08:47, Antony Sargent wrote:
> On 2015/05/28 10:03:49, Marc Treib wrote:
> > Before, we inherited that via UtilityProcessHostClient. I'm not sure if the
> > "ThreadSafe" is really needed - seems a bit pointless since the class can
only
> > be used on the UI thread anyway?
> 
> Agreed - feel free to switch to plain RefCounted. 

Done.

[Marc Treib, 2015-05-29 07:48:55]

The CQ bit was checked by treib@chromium.org

[Marc Treib, 2015-05-29 07:48:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
asargent@chromium.org
Link to the patchset: https://codereview.chromium.org/1153143002/#ps40001
(title: "RefCounted non-ThreadSafe")

[commit-bot: I haz the power, 2015-05-29 07:49:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1153143002/40001

[commit-bot: I haz the power, 2015-05-29 09:18:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-29 09:18:45]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Marc Treib, 2015-05-29 11:56:13]

I've uploaded a new patchset that fixes the memleak. What happened was:
When constructing the SafeJsonParser, we pass it two callbacks
(success/failure). Each of those adds a reference to us, but only one of them
ever gets called, so one ref remains and we're never deleted.
I've fixed it by using base::Unretained and manually calling AddRef/Release. Not
too pretty, but oh well.

[Marc Treib, 2015-06-01 07:48:56]

The CQ bit was checked by treib@chromium.org

[Marc Treib, 2015-06-01 07:48:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
asargent@chromium.org
Link to the patchset: https://codereview.chromium.org/1153143002/#ps60001
(title: "fix memleak")

[commit-bot: I haz the power, 2015-06-01 07:49:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1153143002/60001

[commit-bot: I haz the power, 2015-06-01 09:05:51]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-06-01 09:06:52]

Patchset 4 (id:??) landed as
https://crrev.com/8457c218e1df6bf2789658cd9c6205c08c2f1d41
Cr-Commit-Position: refs/heads/master@{#332171}