[App banners] Start addressing race conditions

[App banners] Start addressing race conditions

DEPENDS ON 914403006

* If a user visits a page that triggers a banner icon download
  and immediately navigates away, the native BitmapFetcher will
  be deleted before its callback is handled, resulting in a crash.
  Address this by storing a vector of BitmapFetchers that are
  trimmed as fetches complete.

* Add basic unit test for ensuring that the race condition doesn't
  cause a crash anymore.

- There is still some timing wackiness that can go on here, like
  having the entire AppBannerManager go away before the fetch
  completes, but that requires further digging.

BUG=453170, 457414, 457413
TEST=AppBannerManagerTest

Committed: https://crrev.com/b4704eaa944bfd59f3397ef5ca9f892667a3bf87
Cr-Commit-Position: refs/heads/master@{#316875}

[gone, 2015-02-10 23:31:48]

dfalcantara@chromium.org changed reviewers:
+ newt@chromium.org

[gone, 2015-02-12 19:23:47]

Added more tests.

[gone, 2015-02-14 01:40:00]

dfalcantara@chromium.org changed reviewers:
+ benwells@chromium.org
- newt@chromium.org

[gone, 2015-02-14 01:40:01]

Sending to benwells@ instead.

[benwells, 2015-02-16 01:58:43]

https://codereview.chromium.org/914813002/diff/160001/chrome/browser/android/...
File chrome/browser/android/banners/app_banner_manager.cc (right):

https://codereview.chromium.org/914813002/diff/160001/chrome/browser/android/...
chrome/browser/android/banners/app_banner_manager.cc:58: void Cancel();
This doesn't appear to be called by anything.

https://codereview.chromium.org/914813002/diff/160001/chrome/browser/android/...
chrome/browser/android/banners/app_banner_manager.cc:225: DVLOG(1) << "Failed to
retrieve image: " << url;
Nit: This log message is kind of inaccurate if url != app_iconurl_.

https://codereview.chromium.org/914813002/diff/160001/chrome/browser/android/...
chrome/browser/android/banners/app_banner_manager.cc:310: return
active_fetchers_.size();
As far as I can tell, this is the only reason to have active_fetchers_. Could
you just keep track of a count instead, and count++ and count-- in the places
you currently add / remove to this list?

If you think you need the container, you could use a set instead as it doesn't
appear the ordering is necessary.

https://codereview.chromium.org/914813002/diff/160001/chrome/browser/android/...
File chrome/browser/android/banners/app_banner_manager.h (right):

https://codereview.chromium.org/914813002/diff/160001/chrome/browser/android/...
chrome/browser/android/banners/app_banner_manager.h:30: class
BannerBitmapFetcher;
Nit: I think it is nicer to put this inside AppBannerManager (as a nested
class). You can still have it only forward declared in the header.

[gone, 2015-02-17 22:56:52]

https://chromiumcodereview.appspot.com/914813002/diff/160001/chrome/browser/a...
File chrome/browser/android/banners/app_banner_manager.cc (right):

https://chromiumcodereview.appspot.com/914813002/diff/160001/chrome/browser/a...
chrome/browser/android/banners/app_banner_manager.cc:58: void Cancel();
On 2015/02/16 01:58:43, benwells wrote:
> This doesn't appear to be called by anything.

Fixed, good catch.

https://chromiumcodereview.appspot.com/914813002/diff/160001/chrome/browser/a...
chrome/browser/android/banners/app_banner_manager.cc:225: DVLOG(1) << "Failed to
retrieve image: " << url;
On 2015/02/16 01:58:43, benwells wrote:
> Nit: This log message is kind of inaccurate if url != app_iconurl_.

Just removed the message.  It's not useful in the current pipeline.

https://chromiumcodereview.appspot.com/914813002/diff/160001/chrome/browser/a...
chrome/browser/android/banners/app_banner_manager.cc:310: return
active_fetchers_.size();
On 2015/02/16 01:58:43, benwells wrote:
> As far as I can tell, this is the only reason to have active_fetchers_. Could
> you just keep track of a count instead, and count++ and count-- in the places
> you currently add / remove to this list?
> 
> If you think you need the container, you could use a set instead as it doesn't
> appear the ordering is necessary.

Started going the set route and realized that a weakly held pointer accomplished
the same thing.  Revamped in light of that.

https://chromiumcodereview.appspot.com/914813002/diff/160001/chrome/browser/a...
File chrome/browser/android/banners/app_banner_manager.h (right):

https://chromiumcodereview.appspot.com/914813002/diff/160001/chrome/browser/a...
chrome/browser/android/banners/app_banner_manager.h:30: class
BannerBitmapFetcher;
On 2015/02/16 01:58:43, benwells wrote:
> Nit: I think it is nicer to put this inside AppBannerManager (as a nested
> class). You can still have it only forward declared in the header. 

Done.

[benwells, 2015-02-18 06:01:18]

lgtm, with the class decl moved.

I think you should get someone who knows java better to review your test.

https://chromiumcodereview.appspot.com/914813002/diff/180001/chrome/browser/a...
File chrome/browser/android/banners/app_banner_manager.cc (right):

https://chromiumcodereview.appspot.com/914813002/diff/180001/chrome/browser/a...
chrome/browser/android/banners/app_banner_manager.cc:58:
banners::AppBannerManager* delegate);
Nit: calling this 'delegate' is a bit confusing, as this class is also a
delegate. Suggest changing the name of the param / field to manager.

https://chromiumcodereview.appspot.com/914813002/diff/180001/chrome/browser/a...
File chrome/browser/android/banners/app_banner_manager.h (right):

https://chromiumcodereview.appspot.com/914813002/diff/180001/chrome/browser/a...
chrome/browser/android/banners/app_banner_manager.h:71: class
BannerBitmapFetcher;
This should be after 'public:', if it has to be public, or after 'private:' if
it can be.

[gone, 2015-02-18 17:48:25]

dfalcantara@chromium.org changed reviewers:
+ tedchoc@chromium.org

[gone, 2015-02-18 17:48:25]

dfalcantara@chromium.org changed reviewers:
+ tedchoc@chromium.org

[Ted C, 2015-02-18 17:54:47]

lgtm

[gone, 2015-02-18 18:33:04]

New patchsets have been uploaded after l-g-t-m from
benwells@chromium.org,tedchoc@chromium.org

[gone, 2015-02-18 18:33:29]

https://chromiumcodereview.appspot.com/914813002/diff/180001/chrome/browser/a...
File chrome/browser/android/banners/app_banner_manager.cc (right):

https://chromiumcodereview.appspot.com/914813002/diff/180001/chrome/browser/a...
chrome/browser/android/banners/app_banner_manager.cc:58:
banners::AppBannerManager* delegate);
On 2015/02/18 06:01:18, benwells wrote:
> Nit: calling this 'delegate' is a bit confusing, as this class is also a
> delegate. Suggest changing the name of the param / field to manager.

Done.

https://chromiumcodereview.appspot.com/914813002/diff/180001/chrome/browser/a...
File chrome/browser/android/banners/app_banner_manager.h (right):

https://chromiumcodereview.appspot.com/914813002/diff/180001/chrome/browser/a...
chrome/browser/android/banners/app_banner_manager.h:71: class
BannerBitmapFetcher;
On 2015/02/18 06:01:18, benwells wrote:
> This should be after 'public:', if it has to be public, or after 'private:' if
> it can be.

Couldn't put it under private because it's referenced by OnFetchComplete.  Just
moved it to under public:

[gone, 2015-02-18 18:33:33]

The CQ bit was checked by dfalcantara@chromium.org

[commit-bot: I haz the power, 2015-02-18 18:34:20]

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

[commit-bot: I haz the power, 2015-02-18 19:32:45]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2015-02-18 19:33:44]

Patchset 11 (id:??) landed as
https://crrev.com/b4704eaa944bfd59f3397ef5ca9f892667a3bf87
Cr-Commit-Position: refs/heads/master@{#316875}