🛀 Fix ApplicationStatus.isEveryActivityDestroyed() = false within callback

Fix ApplicationStatus.isEveryActivityDestroyed() = false within callbacks

isEveryActivityDestroyed was not being updated until after callbacks
were called, so querying it from within a callback would always return
false.

There is currently only one use of this function within a callback:
ChromeApplication.onForegroundActivityDestroyed()

This change fixes isEveryActivityDestroyed(), and also updates
onForegroundActivityDestroyed() so that it doesn't do bad things. Namely:

1) DevTools server shouldn't ever be taken down once it's been initialized in
initializeSharedClasses(), which is only called once and would cause the
DevTools server to never start up again.

2) stopApplicationActivityTracker() would require another call to
startApplicationActivityTracker() once an activity is again created. However,
the only call to it is within initializeSharedClasses(), which happens only during
cold start.

3) CombinedPolicyProvider.get().destroy() actually led me to a separate bug:
https://codereview.chromium.org/2184203004/

BUG=632042
Committed: https://crrev.com/f8daf5cd73ee366de2d409a18509af2058f922f0
Cr-Commit-Position: refs/heads/master@{#408555}

[agrieve, 2016-07-27 19:03:27]

agrieve@chromium.org changed reviewers:
+ dfalcantara@chromium.org, twellington@chromium.org

[agrieve, 2016-07-27 19:03:48]

Description was changed from

==========
Fix ApplicationStatus.isEveryActivityDestroyed() = false within callback

The ActivityInfo was being removed *after* calling callbacks, causing
isEveryActivityDestroyed()'s isEmpty() check to return false, even
though the map contains just a single destroyed activity.

BUG=none
==========

to

==========
Fix ApplicationStatus.isEveryActivityDestroyed() = false within callback

The ActivityInfo was being removed *after* calling callbacks, causing
isEveryActivityDestroyed()'s isEmpty() check to return false, even
though the map contains just a single destroyed activity.

BUG=632042
==========

[agrieve, 2016-07-27 19:03:56]

The CQ bit was checked by agrieve@chromium.org to run a CQ dry run

[agrieve, 2016-07-27 19:04:26]

agrieve@chromium.org changed reviewers:
+ nyquist@chromium.org

[commit-bot: I haz the power, 2016-07-27 19:04:35]

Dry run: 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-27 19:46:54]

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

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

Dry run: This issue passed the CQ dry run.

[gone, 2016-07-28 16:09:57]

The lines you took out aren't mentioned in the CL description, so I'm a bit
confused.

https://chromiumcodereview.appspot.com/2188723003/diff/1/chrome/android/java/...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java
(left):

https://chromiumcodereview.appspot.com/2188723003/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java:338:
if (mDevToolsServer != null) {
Why'd these lines get taken out?

[agrieve, 2016-07-28 16:38:25]

Description was changed from

==========
Fix ApplicationStatus.isEveryActivityDestroyed() = false within callback

The ActivityInfo was being removed *after* calling callbacks, causing
isEveryActivityDestroyed()'s isEmpty() check to return false, even
though the map contains just a single destroyed activity.

BUG=632042
==========

to

==========
Fix ApplicationStatus.isEveryActivityDestroyed() = false within callback

The ActivityInfo was being removed *after* calling callbacks, causing
isEveryActivityDestroyed()'s isEmpty() check to return false, even
though the map contains just a single destroyed activity.

There is currently only one use of this function within a callback:
ChromeApplication.onForegroundActivityDestroyed()

Of the logic in there, the following actually break things with this fix:
mDevToolsServer.destroy(): causes remote inspecting to not
    work after all activities are destroyed
stopApplicationActivityTracker: causes activity tracking to never
    happen after all activities are destroyed.

BUG=632042
==========

[agrieve, 2016-07-28 16:39:27]

https://chromiumcodereview.appspot.com/2188723003/diff/1/chrome/android/java/...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java
(left):

https://chromiumcodereview.appspot.com/2188723003/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java:338:
if (mDevToolsServer != null) {
On 2016/07/28 16:09:57, dfalcantara wrote:
> Why'd these lines get taken out?

I mentioned it in the bug, but have now updated the CL description.

[agrieve, 2016-07-28 16:40:16]

Description was changed from

==========
Fix ApplicationStatus.isEveryActivityDestroyed() = false within callback

The ActivityInfo was being removed *after* calling callbacks, causing
isEveryActivityDestroyed()'s isEmpty() check to return false, even
though the map contains just a single destroyed activity.

There is currently only one use of this function within a callback:
ChromeApplication.onForegroundActivityDestroyed()

Of the logic in there, the following actually break things with this fix:
mDevToolsServer.destroy(): causes remote inspecting to not
    work after all activities are destroyed
stopApplicationActivityTracker: causes activity tracking to never
    happen after all activities are destroyed.

BUG=632042
==========

to

==========
Fix ApplicationStatus.isEveryActivityDestroyed() = false within callback

The ActivityInfo was being removed *after* calling callbacks, causing
isEveryActivityDestroyed()'s isEmpty() check to return false, even
though the map contains just a single destroyed activity.

There is currently only one use of this function within a callback:
ChromeApplication.onForegroundActivityDestroyed()

Of the logic in there, the following actually break things with this fix:
mDevToolsServer.destroy(): causes remote inspecting to not
    work after all activities are destroyed
stopApplicationActivityTracker: causes activity tracking to never
    happen after all activities are destroyed.
CombinedPolicyProvider.get().destroy(): This is singleton initialized
    in ChromeBrowserInitializer, so will stop working if destroyed.

BUG=632042
==========

[gone, 2016-07-28 16:41:45]

Cool, thanks.  Is there going to be a follow up to fix the breakages on the same
bug?

[agrieve, 2016-07-28 18:00:35]

On 2016/07/28 16:41:45, dfalcantara wrote:
> Cool, thanks.  Is there going to be a follow up to fix the breakages on the
same
> bug?

I wasn't planning on a follow-up. I believe all of the breakages pointed out on
the bug are addressed in this patch.

I was at first going to just delete onForegroundActivityDestroyed(), since the
code in it hasn't been running for over a year, but I think it's marginally
better to leave a few of the calls that are in there that are easy to validate
they're safe.

[gone, 2016-07-28 18:12:05]

You should change the CL description to say that removing these lines fix things
then; it's currently worded to say that things break when you land this patch.

Specifically:
1) DevTools server shouldn't ever be taken down once it's been initialized in
initializeSharedClasses(), which is only called once and would cause the
DevTools server to never start up again.

2) stopApplicationActivityTracker() is used by ChromeApplicationInternal, but
its only use is to call PlayLogTrackerInternal.stopPlayLogger().  It's weird
that this was called from here at all because it's only initialized in
initializeSharedClasses(), which is only called once on a cold start and would
never restart the PlayLogTracker.

3) CombinedPolicyProvider.get().destroy() is a weird one.  Not familiar with it
but I'm hoping you've looked into it, at least.

lgtm

[nyquist, 2016-07-28 18:34:48]

overall lgtm, but cl description is weird

[Theresa, 2016-07-29 01:02:42]

+1 to the commit message rewording, otherwise lgtm

[agrieve, 2016-07-29 01:33:57]

Description was changed from

==========
Fix ApplicationStatus.isEveryActivityDestroyed() = false within callback

The ActivityInfo was being removed *after* calling callbacks, causing
isEveryActivityDestroyed()'s isEmpty() check to return false, even
though the map contains just a single destroyed activity.

There is currently only one use of this function within a callback:
ChromeApplication.onForegroundActivityDestroyed()

Of the logic in there, the following actually break things with this fix:
mDevToolsServer.destroy(): causes remote inspecting to not
    work after all activities are destroyed
stopApplicationActivityTracker: causes activity tracking to never
    happen after all activities are destroyed.
CombinedPolicyProvider.get().destroy(): This is singleton initialized
    in ChromeBrowserInitializer, so will stop working if destroyed.

BUG=632042
==========

to

==========
Fix ApplicationStatus.isEveryActivityDestroyed() = false within callbacks

isEveryActivityDestroyed was not being updated until after callbacks
were called, so querying it from within a callback would always return
false.

There is currently only one use of this function within a callback:
ChromeApplication.onForegroundActivityDestroyed()

This change fixes isEveryActivityDestroyed(), and also updates
onForegroundActivityDestroyed() so that it doesn't do bad things. Namely:

1) DevTools server shouldn't ever be taken down once it's been initialized in
initializeSharedClasses(), which is only called once and would cause the
DevTools server to never start up again.

2) stopApplicationActivityTracker() would require another call to
startApplicationActivityTracker() once an activity is again created. However,
the only call to it is within initializeSharedClasses(), which happens only
during
cold start.

3) CombinedPolicyProvider.get().destroy() actually led me to a separate bug:
https://codereview.chromium.org/2184203004/

BUG=632042
==========

[agrieve, 2016-07-29 01:34:26]

On 2016/07/29 01:02:42, Theresa Wellington wrote:
> +1 to the commit message rewording, otherwise lgtm

Thanks all! Hopefully the message is better now.

[agrieve, 2016-07-29 01:34:32]

The CQ bit was checked by agrieve@chromium.org

[commit-bot: I haz the power, 2016-07-29 01:34:47]

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-29 02:07:55]

Description was changed from

==========
Fix ApplicationStatus.isEveryActivityDestroyed() = false within callbacks

isEveryActivityDestroyed was not being updated until after callbacks
were called, so querying it from within a callback would always return
false.

There is currently only one use of this function within a callback:
ChromeApplication.onForegroundActivityDestroyed()

This change fixes isEveryActivityDestroyed(), and also updates
onForegroundActivityDestroyed() so that it doesn't do bad things. Namely:

1) DevTools server shouldn't ever be taken down once it's been initialized in
initializeSharedClasses(), which is only called once and would cause the
DevTools server to never start up again.

2) stopApplicationActivityTracker() would require another call to
startApplicationActivityTracker() once an activity is again created. However,
the only call to it is within initializeSharedClasses(), which happens only
during
cold start.

3) CombinedPolicyProvider.get().destroy() actually led me to a separate bug:
https://codereview.chromium.org/2184203004/

BUG=632042
==========

to

==========
Fix ApplicationStatus.isEveryActivityDestroyed() = false within callbacks

isEveryActivityDestroyed was not being updated until after callbacks
were called, so querying it from within a callback would always return
false.

There is currently only one use of this function within a callback:
ChromeApplication.onForegroundActivityDestroyed()

This change fixes isEveryActivityDestroyed(), and also updates
onForegroundActivityDestroyed() so that it doesn't do bad things. Namely:

1) DevTools server shouldn't ever be taken down once it's been initialized in
initializeSharedClasses(), which is only called once and would cause the
DevTools server to never start up again.

2) stopApplicationActivityTracker() would require another call to
startApplicationActivityTracker() once an activity is again created. However,
the only call to it is within initializeSharedClasses(), which happens only
during
cold start.

3) CombinedPolicyProvider.get().destroy() actually led me to a separate bug:
https://codereview.chromium.org/2184203004/

BUG=632042
==========

[commit-bot: I haz the power, 2016-07-29 02:07:56]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-07-29 02:09:57]

Description was changed from

==========
Fix ApplicationStatus.isEveryActivityDestroyed() = false within callbacks

isEveryActivityDestroyed was not being updated until after callbacks
were called, so querying it from within a callback would always return
false.

There is currently only one use of this function within a callback:
ChromeApplication.onForegroundActivityDestroyed()

This change fixes isEveryActivityDestroyed(), and also updates
onForegroundActivityDestroyed() so that it doesn't do bad things. Namely:

1) DevTools server shouldn't ever be taken down once it's been initialized in
initializeSharedClasses(), which is only called once and would cause the
DevTools server to never start up again.

2) stopApplicationActivityTracker() would require another call to
startApplicationActivityTracker() once an activity is again created. However,
the only call to it is within initializeSharedClasses(), which happens only
during
cold start.

3) CombinedPolicyProvider.get().destroy() actually led me to a separate bug:
https://codereview.chromium.org/2184203004/

BUG=632042
==========

to

==========
Fix ApplicationStatus.isEveryActivityDestroyed() = false within callbacks

isEveryActivityDestroyed was not being updated until after callbacks
were called, so querying it from within a callback would always return
false.

There is currently only one use of this function within a callback:
ChromeApplication.onForegroundActivityDestroyed()

This change fixes isEveryActivityDestroyed(), and also updates
onForegroundActivityDestroyed() so that it doesn't do bad things. Namely:

1) DevTools server shouldn't ever be taken down once it's been initialized in
initializeSharedClasses(), which is only called once and would cause the
DevTools server to never start up again.

2) stopApplicationActivityTracker() would require another call to
startApplicationActivityTracker() once an activity is again created. However,
the only call to it is within initializeSharedClasses(), which happens only
during
cold start.

3) CombinedPolicyProvider.get().destroy() actually led me to a separate bug:
https://codereview.chromium.org/2184203004/

BUG=632042

Committed: https://crrev.com/f8daf5cd73ee366de2d409a18509af2058f922f0
Cr-Commit-Position: refs/heads/master@{#408555}
==========

[commit-bot: I haz the power, 2016-07-29 02:09:58]

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