PrecacheFetcher::CancelPrecaching should run in the main UI thread

PrecacheFetcher::CancelPrecaching should run in the main UI thread

Reducing the possibilities of crash in CancelPrecaching.

BUG=619111
Committed: https://crrev.com/28e7a0cc33dafc8719c4b9c740d79a47ff90f067
Cr-Commit-Position: refs/heads/master@{#404792}

[Raj, 2016-07-08 17:57:34]

rajendrant@chromium.org changed reviewers:
+ bengr@google.com

[Raj, 2016-07-08 17:57:35]

ptal

https://codereview.chromium.org/2137593002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheController.java
(right):

https://codereview.chromium.org/2137593002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheController.java:417:
private void cancelPrecaching(final int event) {
This could be called from PrecacheManager::Shutdown() or other places where it
could be a different thread.

https://codereview.chromium.org/2137593002/diff/1/components/precache/core/pr...
File components/precache/core/precache_fetcher.cc (right):

https://codereview.chromium.org/2137593002/diff/1/components/precache/core/pr...
components/precache/core/precache_fetcher.cc:363: return nullptr;
CancelPrecaching() could get called multiple times, though I do not see an
apparent codepath for multiple calls.
But it should be handled gracefully.

[Raj, 2016-07-08 17:57:45]

rajendrant@chromium.org changed reviewers:
+ bengr@chromium.org
- bengr@google.com

[Raj, 2016-07-08 17:57:45]

ptal

[Raj, 2016-07-11 18:11:21]

Description was changed from

==========
PrecacheFetcher::CancelPrecaching should run in the main UI thread

Reducing the possibilities of crash in CancelPrecaching.

BUG=619111
==========

to

==========
PrecacheFetcher::CancelPrecaching should run in the main UI thread

Reducing the possibilities of crash in CancelPrecaching.

BUG=619111
==========

[Raj, 2016-07-11 18:11:22]

rajendrant@chromium.org changed reviewers:
+ sclittle@chromium.org
- bengr@chromium.org

[Raj, 2016-07-11 18:11:36]

+ sclittle@ since bengr@ is OOO

[sclittle, 2016-07-11 18:16:51]

LGTM % nits about comments

https://codereview.chromium.org/2137593002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheController.java
(right):

https://codereview.chromium.org/2137593002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheController.java:417:
private void cancelPrecaching(final int event) {
On 2016/07/08 17:57:35, Raj wrote:
> This could be called from PrecacheManager::Shutdown() or other places where it
> could be a different thread.

Interesting, could you put this comment in the code? If it's worth mentioning
here then it seems like it would probably be useful to future readers of this
code.

https://codereview.chromium.org/2137593002/diff/1/components/precache/core/pr...
File components/precache/core/precache_fetcher.cc (right):

https://codereview.chromium.org/2137593002/diff/1/components/precache/core/pr...
components/precache/core/precache_fetcher.cc:363: return nullptr;
On 2016/07/08 17:57:35, Raj wrote:
> CancelPrecaching() could get called multiple times, though I do not see an
> apparent codepath for multiple calls.
> But it should be handled gracefully.

Could you add this comment to the code too?

[Raj, 2016-07-12 00:32:20]

The CQ bit was checked by rajendrant@chromium.org

[Raj, 2016-07-12 00:32:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
sclittle@chromium.org
Link to the patchset: https://codereview.chromium.org/2137593002/#ps40001
(title: "Fixed unittest")

[Raj, 2016-07-12 00:32:26]

https://codereview.chromium.org/2137593002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheController.java
(right):

https://codereview.chromium.org/2137593002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheController.java:417:
private void cancelPrecaching(final int event) {
On 2016/07/11 18:16:51, sclittle wrote:
> On 2016/07/08 17:57:35, Raj wrote:
> > This could be called from PrecacheManager::Shutdown() or other places where
it
> > could be a different thread.
> 
> Interesting, could you put this comment in the code? If it's worth mentioning
> here then it seems like it would probably be useful to future readers of this
> code.

Done.

[commit-bot: I haz the power, 2016-07-12 00:34:02]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-12 03:18:32]

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

[commit-bot: I haz the power, 2016-07-12 03:18:33]

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

[Raj, 2016-07-12 04:32:12]

The CQ bit was checked by rajendrant@chromium.org

[Raj, 2016-07-12 04:32:13]

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

[commit-bot: I haz the power, 2016-07-12 04:33:16]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-12 07:24:45]

Description was changed from

==========
PrecacheFetcher::CancelPrecaching should run in the main UI thread

Reducing the possibilities of crash in CancelPrecaching.

BUG=619111
==========

to

==========
PrecacheFetcher::CancelPrecaching should run in the main UI thread

Reducing the possibilities of crash in CancelPrecaching.

BUG=619111
==========

[commit-bot: I haz the power, 2016-07-12 07:24:46]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-07-12 07:24:50]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-12 07:27:55]

Description was changed from

==========
PrecacheFetcher::CancelPrecaching should run in the main UI thread

Reducing the possibilities of crash in CancelPrecaching.

BUG=619111
==========

to

==========
PrecacheFetcher::CancelPrecaching should run in the main UI thread

Reducing the possibilities of crash in CancelPrecaching.

BUG=619111

Committed: https://crrev.com/28e7a0cc33dafc8719c4b9c740d79a47ff90f067
Cr-Commit-Position: refs/heads/master@{#404792}
==========

[commit-bot: I haz the power, 2016-07-12 07:27:56]

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

[picksi1, 2016-07-12 14:25:24]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/2147543002/ by picksi@chromium.org.

The reason for reverting is: Android Test (dbg) has turned red, with failures:
C  860.211s Main  [  FAILED  ]
org.chromium.chrome.browser.precache.PrecacheControllerTest#testDeviceStateChangeCancels
C  860.211s Main  [  FAILED  ]
org.chromium.chrome.browser.precache.PrecacheControllerTest#testTimeoutCancelsPrecaching
C  860.211s Main  [  FAILED  ]
org.chromium.chrome.browser.precache.PrecacheControllerTest#testTimeoutDoesNotCancelIfNotPrecaching
(UNKNOWN)
C  860.211s Main  [  FAILED  ]
org.chromium.chrome.browser.precache.PrecacheLauncherTest#testUpdateEnabled_Disabled_ThenEnabled
(UNKNOWN)
C  860.211s Main  [  FAILED  ]
org.chromium.chrome.browser.precache.PrecacheLauncherTest#testUpdateEnabled_SyncNotReady_ThenEnabled
(UNKNOWN)

 Link to bot:

https://uberchromegw.corp.google.com/i/chromium.linux/builders/Android%20Test...
.