New issue
Advanced search Search tips

Issue 632042 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 2016
Cc:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 3
Type: Bug



Sign in to add a comment

ChromeActivity.onForegroundActivityDestroyed() is never called

Project Member Reported by agrieve@chromium.org, Jul 27 2016

Issue description

Version: 54.0.2810.0
OS: Android

What steps will reproduce the problem?
(1) Turn on "Don't keep activities" developer option
(2) Open chrome
(3) Return to homescreen

What is the expected output?
Logic within ChromeApplication.onForegroundActivityDestroyed() should run


What do you see instead?
ApplicationStatus.isEveryActivityDestroyed() always returns false within status callbacks, so the logic is never being executed.


This has been broken for at least a year (didn't bother to look back further), so either the logic in there isn't necessary, or it's entirely unwanted.

Here's the logic:

            mBackgroundProcessing.onDestroy();
            if (mDevToolsServer != null) {
                mDevToolsServer.destroy();
                mDevToolsServer = null;
            }
            stopApplicationActivityTracker();
            PartnerBrowserCustomizations.destroy();
            ShareHelper.clearSharedImages(this);
            CombinedPolicyProvider.get().destroy();

 
Audited the code to find:

Probably okay:
mBackgroundProcessing: Safe to call start() again after onDestroy().
PartnerBrowserCustomizations: Re-initialized on every activity start.
ShareHelper: Just clearing, not destroying. fine.
CombinedPolicyProvider.get().destroy(): Initialized in ChromeBrowserInitializer()


Probably not okay (both singleton initialized in ChromeAppliction.initializeSharedClasses():
mDevToolsServer
stopApplicationActivityTracker: startApplicationActivityTracker() will never be called again.

Confirmed that right now the CombinedPolicyProvider not being destroyed is indeed bad. Every time there's a new activity, we re-register 2 new (same) policy providers.

Confirmed that DevTools never comes back if you destroy it as is being done here.
Do you think this is related to  https://crbug.com/579363 ?
I don't think so, but I did see that exact symptom when trying to implement my finishAllChromeActivities(). Left a comment on the bug.
Project Member

Comment 5 by bugdroid1@chromium.org, Jul 29 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f8daf5cd73ee366de2d409a18509af2058f922f0

commit f8daf5cd73ee366de2d409a18509af2058f922f0
Author: agrieve <agrieve@chromium.org>
Date: Fri Jul 29 02:07:39 2016

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 

Review-Url: https://codereview.chromium.org/2188723003
Cr-Commit-Position: refs/heads/master@{#408555}

[modify] https://crrev.com/f8daf5cd73ee366de2d409a18509af2058f922f0/base/android/java/src/org/chromium/base/ApplicationStatus.java
[modify] https://crrev.com/f8daf5cd73ee366de2d409a18509af2058f922f0/chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java

Status: Fixed (was: Started)
Project Member

Comment 7 by bugdroid1@chromium.org, Aug 5 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/86c77fcc18da9e983c9d0b8f80de71809d4daac0

commit 86c77fcc18da9e983c9d0b8f80de71809d4daac0
Author: agrieve <agrieve@chromium.org>
Date: Fri Aug 05 20:42:21 2016

Register policy providers only once

They were being re-registered for each new activity and never
unregistered.

BUG= 632042 

Review-Url: https://codereview.chromium.org/2184203004
Cr-Commit-Position: refs/heads/master@{#410157}

[modify] https://crrev.com/86c77fcc18da9e983c9d0b8f80de71809d4daac0/chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java

Sign in to add a comment