Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2143203002:
🎨 Tidy up of TabPersistentStore's clean-up logic (Closed)
DescriptionTidy up of TabPersistentStore's clean-up logic
* Clean files on every activity start rather than just the first
* Don't delete files owned by other TabModels
* Don't delete icons used by other TabModels
* Adds test for incognito tab files being removed appropraitely
BUG=626629
Committed: https://crrev.com/b9ef755c73b5852c66a20b41ddc8f3624ee85ef6
Cr-Commit-Position: refs/heads/master@{#408566}
Patch Set 1 #
Total comments: 4
Patch Set 2 : a more better fix #Patch Set 3 : remove debug logs #
Total comments: 4
Patch Set 4 : Don't restore incognito explicitly #
Total comments: 16
Patch Set 5 : Added TabWindowManager.tabExistsInAnySelector() #
Total comments: 8
Patch Set 6 : rebased & TabWindowManager test #Patch Set 7 : Fix incognito tabs persisting when cct service is active #Patch Set 8 : fix tests fingers crossed! #Patch Set 9 : Fix TabPersistentStoreTest #
Total comments: 27
Patch Set 10 : review comments #Patch Set 11 : review comments #
Total comments: 8
Patch Set 12 : Fix incognito tabs persisting when cct service is active #Depends on Patchset: Messages
Total messages: 69 (35 generated)
agrieve@chromium.org changed reviewers: + dfalcantara@chromium.org
The CQ bit was checked by agrieve@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org to run a CQ dry run
Dry run: This issue passed the CQ dry run.
twellington@chromium.org changed reviewers: + twellington@chromium.org
https://codereview.chromium.org/2143203002/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/crypto/CipherFactory.java (right): https://codereview.chromium.org/2143203002/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/crypto/CipherFactory.java:275: // Application instance is kept alive by a Chrome Custom Tab. Will you please test this on Android N+ multi-window (as a sanity check)? It's expected that the two instances share an encryption key so incognito tabs can be moved between the windows.
https://codereview.chromium.org/2143203002/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/crypto/CipherFactory.java (right): https://codereview.chromium.org/2143203002/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/crypto/CipherFactory.java:275: // Application instance is kept alive by a Chrome Custom Tab. On 2016/07/12 23:29:24, Theresa Wellington wrote: > Will you please test this on Android N+ multi-window (as a sanity check)? It's > expected that the two instances share an encryption key so incognito tabs can be > moved between the windows. Yeah, this is broken for multi-window because ChromeTabbedActivity unconditionally calls restoreFromBundle() in ChromeTabbedActivity#initializeState(). If the second ChromeTabbedActivity doesn't have a bundle and is started while the first ChromeTabbedActivity has incognito tabs, it'll still wipe out the cipher key. https://codereview.chromium.org/2143203002/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/crypto/CipherFactory.java:277: mData = null; Should mData be set to null in all cases where this returns false?
https://codereview.chromium.org/2143203002/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/crypto/CipherFactory.java (right): https://codereview.chromium.org/2143203002/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/crypto/CipherFactory.java:275: // Application instance is kept alive by a Chrome Custom Tab. On 2016/07/13 17:15:16, dfalcantara wrote: > On 2016/07/12 23:29:24, Theresa Wellington wrote: > > Will you please test this on Android N+ multi-window (as a sanity check)? It's > > expected that the two instances share an encryption key so incognito tabs can > be > > moved between the windows. > > Yeah, this is broken for multi-window because ChromeTabbedActivity > unconditionally calls restoreFromBundle() in > ChromeTabbedActivity#initializeState(). If the second ChromeTabbedActivity > doesn't have a bundle and is started while the first ChromeTabbedActivity has > incognito tabs, it'll still wipe out the cipher key. Yikes! Thanks for noticing this Theresa! I think I've got a better fix now, but the tests took me a long time to figure out. ptal.
https://chromiumcodereview.appspot.com/2143203002/diff/40001/chrome/android/j... File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (right): https://chromiumcodereview.appspot.com/2143203002/diff/40001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1314: mTabModelSelectorImpl.getModel(true /* ingognito */) ingognito? https://chromiumcodereview.appspot.com/2143203002/diff/40001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1315: .closeAllTabs(false /* allowDelegation */, true /*uponExit */); This just straight up closes all incognito tabs whenever the ChromeTabbedActivity is destroyed. What causes this pathway to get called? Does this happen when the Activity is killed in the background due to memory pressure? If so, that case would normally allow the user to restore their incognito tabs on the next startup. This also prevents incognito tabs from being merged over to the other ChromeTabbedActivity.
Also, your commit message needs an update. https://chromiumcodereview.appspot.com/2143203002/diff/40001/chrome/android/j... File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (right): https://chromiumcodereview.appspot.com/2143203002/diff/40001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1315: .closeAllTabs(false /* allowDelegation */, true /*uponExit */); On 2016/07/15 00:04:48, dfalcantara wrote: > This just straight up closes all incognito tabs whenever the > ChromeTabbedActivity is destroyed. What causes this pathway to get called? > Does this happen when the Activity is killed in the background due to memory > pressure? If so, that case would normally allow the user to restore their > incognito tabs on the next startup. This also prevents incognito tabs from > being merged over to the other ChromeTabbedActivity. Yeah, this is a step backward from current functionality. onDestroyInternal is called when the user backgrounds Chrome (turn on Don't Keep Activities to test). This means the users will never get to keep their incognito tabs when the process dies.
https://chromiumcodereview.appspot.com/2143203002/diff/40001/chrome/android/j... File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (right): https://chromiumcodereview.appspot.com/2143203002/diff/40001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1315: .closeAllTabs(false /* allowDelegation */, true /*uponExit */); On 2016/07/15 00:13:41, dfalcantara wrote: > On 2016/07/15 00:04:48, dfalcantara wrote: > > This just straight up closes all incognito tabs whenever the > > ChromeTabbedActivity is destroyed. What causes this pathway to get called? > > Does this happen when the Activity is killed in the background due to memory > > pressure? If so, that case would normally allow the user to restore their > > incognito tabs on the next startup. This also prevents incognito tabs from > > being merged over to the other ChromeTabbedActivity. > > Yeah, this is a step backward from current functionality. onDestroyInternal is > called when the user backgrounds Chrome (turn on Don't Keep Activities to test). > This means the users will never get to keep their incognito tabs when the > process dies. Hmm... tricky tricky. So, before my change that was the regression, we were clearing all incognito state files when an activity started without a savedInstanceState. I think this would also be trouble for multi-window, so we don't want to do this. The approach in this PS is to clear the tabs whenever the activity goes away. This doesn't normally happen (I think) in an OOM situation (its process just gets zapped), but you're right that it happens with dontKeepActivities enabled (so yeah, no good). If we clear tabs when the activity is created without a savedInstanceState, but only clear them if no other activities exist, this is a problem: - split screen is active. One incognito, one not - swipe away incognito - use existing other window to re-create 2nd activity - observe it revives the swiped away activity. If we clear tabs when the activity is created without a savedInstanceState, but only clear those that don't belong to an existing activity, this is a problem: - Create 2 windows, one incognito, one not - Swipe away incognito - Send first window to background with dontKeepActivities - Re-open Chrome, and observe that a savedInstanceState exists since the most-recently closed activity had one Another approach: - Only save the cipherdata to the bundle on saveinstancestate for an activity that contains an incognito tab? - When restored without a savedInstanceState, clear all incognito tabs that don't belong to an existing activity. - The problem I see here is: - 2 activities, one incognito, one not - Send the incognito one to background with dontKeepActivities. Saves its state. - Swipe away the other one, no saved state - Restore Chrome, find no bundleData & no existing activities. Delete all tabs. Maybe that's the closest to what we want? Any other ideas? What do you mean by tab merging? The "Move to other window" button?
On 2016/07/15 13:45:42, agrieve wrote: > https://chromiumcodereview.appspot.com/2143203002/diff/40001/chrome/android/j... > File > chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java > (right): > > https://chromiumcodereview.appspot.com/2143203002/diff/40001/chrome/android/j... > chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:1315: > .closeAllTabs(false /* allowDelegation */, true /*uponExit */); > On 2016/07/15 00:13:41, dfalcantara wrote: > > On 2016/07/15 00:04:48, dfalcantara wrote: > > > This just straight up closes all incognito tabs whenever the > > > ChromeTabbedActivity is destroyed. What causes this pathway to get called? > > > Does this happen when the Activity is killed in the background due to memory > > > pressure? If so, that case would normally allow the user to restore their > > > incognito tabs on the next startup. This also prevents incognito tabs from > > > being merged over to the other ChromeTabbedActivity. > > > > Yeah, this is a step backward from current functionality. onDestroyInternal > is > > called when the user backgrounds Chrome (turn on Don't Keep Activities to > test). > > This means the users will never get to keep their incognito tabs when the > > process dies. > > Hmm... tricky tricky. > > So, before my change that was the regression, we were clearing all incognito > state files when an activity started without a savedInstanceState. I think this > would also be trouble for multi-window, so we don't want to do this. > > The approach in this PS is to clear the tabs whenever the activity goes away. > This doesn't normally happen (I think) in an OOM situation (its process just > gets zapped), but you're right that it happens with dontKeepActivities enabled > (so yeah, no good). > > If we clear tabs when the activity is created without a savedInstanceState, but > only clear them if no other activities exist, this is a problem: > - split screen is active. One incognito, one not > - swipe away incognito > - use existing other window to re-create 2nd activity > - observe it revives the swiped away activity. > > If we clear tabs when the activity is created without a savedInstanceState, but > only clear those that don't belong to an existing activity, this is a problem: > - Create 2 windows, one incognito, one not > - Swipe away incognito > - Send first window to background with dontKeepActivities > - Re-open Chrome, and observe that a savedInstanceState exists since the > most-recently closed activity had one > > Another approach: > - Only save the cipherdata to the bundle on saveinstancestate for an activity > that contains an incognito tab? > - When restored without a savedInstanceState, clear all incognito tabs that > don't belong to an existing activity. > - The problem I see here is: > - 2 activities, one incognito, one not > - Send the incognito one to background with dontKeepActivities. Saves its > state. > - Swipe away the other one, no saved state > - Restore Chrome, find no bundleData & no existing activities. Delete all > tabs. > > > Maybe that's the closest to what we want? Any other ideas? > > What do you mean by tab merging? The "Move to other window" button? The final UX for multi-instance is going to be merging the two tab models together when the user goes back to full screen. It's not implemented yet, but is in progress. Since multi-instance can only be accessed from multi-window, we don't want users to lose tabs if they close one of the instances in multi-window, then go back to full screen where they can't create a second instance.
On 2016/07/15 13:45:42, agrieve wrote: .... > If we clear tabs when the activity is created without a savedInstanceState, but > only clear those that don't belong to an existing activity, this is a problem: > - Create 2 windows, one incognito, one not > - Swipe away incognito > - Send first window to background with dontKeepActivities > - Re-open Chrome, and observe that a savedInstanceState exists since the > most-recently closed activity had one "Re-open Chrome" should just bring the first window to the foreground (unless I'm misinterpreting re-open?). If so, we don't want to clear the first window's incognito tabs. > Another approach: > - Only save the cipherdata to the bundle on saveinstancestate for an activity > that contains an incognito tab? I don't think we should do this. I have a (lower priority) open bug to handle a side case related to incognito in mw & activities dying; the plan there is to create an incognito cipher key when the second activity is created to include in both activity's saved instance states. crbug.com/618892 It's worth noting that I just tried reproducing and couldn't. > - When restored without a savedInstanceState, clear all incognito tabs that > don't belong to an existing activity. > - The problem I see here is: > - 2 activities, one incognito, one not > - Send the incognito one to background with dontKeepActivities. Saves its > state. > - Swipe away the other one, no saved state > - Restore Chrome, find no bundleData & no existing activities. Delete all > tabs. This surprises me. If it's the remaining window that wasn't swiped away getting refocused/restored, it should have a saved instance state. The two ChromeTabbedActivities shouldn't share saved instance states since they're different instances. > Maybe that's the closest to what we want? Any other ideas? I think clearing tabs when the activity is created without a saved instance state and there are no other ChromeTabbedActivity tasks (* not activities -- tasks exist even if the activity is destroyed so long as there is an entry for it in Android recents) is an easy first solution. I think that only having incognito tabs persist when removing Chrome from recents then recreating the activity in multi-window (vs any time there's a Chrome process still alive) is preferable to losing incognito tabs when an activity gets sent to the background and dies (others may disagree). I'm optimistic we can find a better solution, though.
On 2016/07/15 22:52:10, Theresa Wellington wrote:
> On 2016/07/15 13:45:42, agrieve wrote:
> ....
> > If we clear tabs when the activity is created without a savedInstanceState,
> but
> > only clear those that don't belong to an existing activity, this is a
problem:
> > - Create 2 windows, one incognito, one not
> > - Swipe away incognito
> > - Send first window to background with dontKeepActivities
> > - Re-open Chrome, and observe that a savedInstanceState exists since the
> > most-recently closed activity had one
>
> "Re-open Chrome" should just bring the first window to the foreground (unless
> I'm misinterpreting re-open?). If so, we don't want to clear the first
window's
> incognito tabs.
>
>
> > Another approach:
> > - Only save the cipherdata to the bundle on saveinstancestate for an
activity
> > that contains an incognito tab?
>
> I don't think we should do this. I have a (lower priority) open bug to handle
a
> side case related to incognito in mw & activities dying; the plan there is to
> create an incognito cipher key when the second activity is created to include
in
> both activity's saved instance states. crbug.com/618892 It's worth noting that
I
> just tried reproducing and couldn't.
>
> > - When restored without a savedInstanceState, clear all incognito tabs that
> > don't belong to an existing activity.
> > - The problem I see here is:
> > - 2 activities, one incognito, one not
> > - Send the incognito one to background with dontKeepActivities. Saves its
> > state.
> > - Swipe away the other one, no saved state
> > - Restore Chrome, find no bundleData & no existing activities. Delete all
> > tabs.
>
> This surprises me. If it's the remaining window that wasn't swiped away
getting
> refocused/restored, it should have a saved instance state. The two
> ChromeTabbedActivities shouldn't share saved instance states since they're
> different instances.
>
>
> > Maybe that's the closest to what we want? Any other ideas?
>
> I think clearing tabs when the activity is created without a saved instance
> state and there are no other ChromeTabbedActivity tasks (* not activities --
> tasks exist even if the activity is destroyed so long as there is an entry for
> it in Android recents) is an easy first solution. I think that only having
> incognito tabs persist when removing Chrome from recents then recreating the
> activity in multi-window (vs any time there's a Chrome process still alive) is
> preferable to losing incognito tabs when an activity gets sent to the
background
> and dies (others may disagree).
>
> I'm optimistic we can find a better solution, though.
Thanks Theresa! I didn't really get that the two windows were different activity
classes, and so would have different saved instance states.
How about:
- If started without a savedInstanceState:
- If there's no other tabbed activity running:
- Clear cipher (and let normal clean-up asynctask delete the state files)
- If there is another tabbed activity running:
- Skip trying to load tabs from disk (and let normal clean-up asynctask
delete the state files)
On 2016/07/18 19:36:24, agrieve wrote: > Thanks Theresa! I didn't really get that the two windows were different activity > classes, and so would have different saved instance states. Yep :) They would have different saved instance states even if they used the same activity class since they're two separate instances. > How about: > - If started without a savedInstanceState: > - If there's no other tabbed activity running: > - Clear cipher (and let normal clean-up asynctask delete the state files) We'll also want to make sure the cipher key gets initialized before any incognito tabs are created if we clear the cipher. > - If there is another tabbed activity running: > - Skip trying to load tabs from disk (and let normal clean-up asynctask > delete the state files) As long as this is "skip trying to load incognito tabs from disk" that seems reasonable. Another option (that may have less cases/conditions) is just to skip loading incognito when there isn't a saved instance state for the activity we're currently loading regardless of whether there are other activities running.
On 2016/07/18 20:19:11, Theresa Wellington wrote: > On 2016/07/18 19:36:24, agrieve wrote: > > Thanks Theresa! I didn't really get that the two windows were different > activity > > classes, and so would have different saved instance states. > > Yep :) They would have different saved instance states even if they used the > same > activity class since they're two separate instances. > > > How about: > > - If started without a savedInstanceState: > > - If there's no other tabbed activity running: > > - Clear cipher (and let normal clean-up asynctask delete the state files) > > We'll also want to make sure the cipher key gets initialized before any > incognito tabs are created if we clear the cipher. > > > - If there is another tabbed activity running: > > - Skip trying to load tabs from disk (and let normal clean-up asynctask > > delete the state files) > > As long as this is "skip trying to load incognito tabs from disk" that seems > reasonable. Another option (that may have less cases/conditions) is just to skip > loading incognito when there isn't a saved instance state for the activity we're > currently loading regardless of whether there are other activities running. I went to implement the two cases, and ended up concluding exactly what you spelled out here - that there's actually no difference when the other activity exists or not :P. ptal
The CQ bit was checked by agrieve@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
https://codereview.chromium.org/2143203002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelSelectorImpl.java (right): https://codereview.chromium.org/2143203002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelSelectorImpl.java:278: public void loadState(boolean ignoreIncognitoFiles) { nit: add JavaDoc for this new param https://codereview.chromium.org/2143203002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java (left): https://codereview.chromium.org/2143203002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:996: && TabWindowManager.getInstance().getNumberOfAssignedTabModelSelectors() == 1) { I added this intentionally. If both tab models are loaded, they could both be changing e.g. 1. CleanUpTabStateDataTask gets created & started. 2. In the background getTabsFromOtherStateFiles() gets called; it returns tab5, tab6, tab7 as belonging to the other state file. 3. A new tab8 is created in the other window. 4. CleanUpTabSTateDataTask#doInBackground() calls getStateDirectory.list(), which lists tab5, tab6, tab7, tab8 5. tab8 gets deleted While this interleaving is unlikely, it does seem possible. If we're going to clear state while more than one activity is running, we should be checking that activity's tab model rather than reading it's metadata file to determine which tabs are still alive. If we leave this on application cold start only, for now we may end up with extra incognito files that won't get cleared out until the next application start. Eventually we'll merge tabs on application cold start and this will be a non-issue. https://codereview.chromium.org/2143203002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java (right): https://codereview.chromium.org/2143203002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:365: public void loadState(boolean ignoreIncognitoFiles) { nit: add JavaDoc for this new param here too https://codereview.chromium.org/2143203002/diff/60001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/TabsTest.java (right): https://codereview.chromium.org/2143203002/diff/60001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/TabsTest.java:1660: public void testIncognitoTabsNotRestoredAfterSwipe() throws Exception { Yay tests! https://codereview.chromium.org/2143203002/diff/60001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/TabsTest.java:1679: getActivity().finish(); finishAndRemoveTask() will actually remove this from recents. finish() just calls destroy, but the activity remains in recents. https://codereview.chromium.org/2143203002/diff/60001/chrome/test/android/jav... File chrome/test/android/javatests/src/org/chromium/chrome/test/util/ApplicationTestUtils.java (right): https://codereview.chromium.org/2143203002/diff/60001/chrome/test/android/jav... chrome/test/android/javatests/src/org/chromium/chrome/test/util/ApplicationTestUtils.java:148: public static void waitForActivityOnDestroy(final Activity targetActivity, long timeoutMillis) Instead of creating a new method here, you may be able to just use CriteriaHelper with getStateForActivity(). MultiWindowUtilsTest.java has an example of this: https://cs.chromium.org/chromium/src/chrome/android/javatests/src/org/chromiu... If you do want to use the ActivityStateListener, I suggest doing something like this in TabsTest.java: 1. Create CallbackHelper to be notified when the activity is destroyed 2. Create an ActivityStateListener; onActivityStateChanged() notifies callback 3. Register ActivityStateListener 4. Call finishAndRemoveTask() on the activity 5. callbackHelper#waitForCallback() to wait until the activity is destroyed
I'm OOO for likely the reset of the day, but this change is still top of my list! Thanks for the quick review! https://codereview.chromium.org/2143203002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java (left): https://codereview.chromium.org/2143203002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:996: && TabWindowManager.getInstance().getNumberOfAssignedTabModelSelectors() == 1) { On 2016/07/19 15:29:41, Theresa Wellington wrote: > I added this intentionally. If both tab models are loaded, they could both be > changing e.g. > > 1. CleanUpTabStateDataTask gets created & started. > 2. In the background getTabsFromOtherStateFiles() gets called; it returns tab5, > tab6, tab7 as belonging to the other state file. > 3. A new tab8 is created in the other window. > 4. CleanUpTabSTateDataTask#doInBackground() calls getStateDirectory.list(), > which lists tab5, tab6, tab7, tab8 > 5. tab8 gets deleted > > While this interleaving is unlikely, it does seem possible. If we're going to > clear state while more than one activity is running, we should be checking that > activity's tab model rather than reading it's metadata file to determine which > tabs are still alive. > > > If we leave this on application cold start only, for now we may end up with > extra incognito files that won't get cleared out until the next application > start. Eventually we'll merge tabs on application cold start and this will be a > non-issue. Hrm, I was hoping to avoid the case where you skip loading them this time, they are never cleaned up, then the task is closed OOM killed & re-opened with bundle, then the tabs are reloaded. WDYT about if we change the logic in CleanUpTabSTateDataTask to consult with TabWindowManager to check against all in-memory models in order to avoid this race? https://codereview.chromium.org/2143203002/diff/60001/chrome/test/android/jav... File chrome/test/android/javatests/src/org/chromium/chrome/test/util/ApplicationTestUtils.java (right): https://codereview.chromium.org/2143203002/diff/60001/chrome/test/android/jav... chrome/test/android/javatests/src/org/chromium/chrome/test/util/ApplicationTestUtils.java:148: public static void waitForActivityOnDestroy(final Activity targetActivity, long timeoutMillis) On 2016/07/19 15:29:42, Theresa Wellington wrote: > Instead of creating a new method here, you may be able to just use > CriteriaHelper with getStateForActivity(). MultiWindowUtilsTest.java has an > example of this: > https://cs.chromium.org/chromium/src/chrome/android/javatests/src/org/chromiu... Yeah, I was hoping to avoid polling. Although it's definitely much less code (I'm torn). > > > If you do want to use the ActivityStateListener, I suggest doing something like > this in TabsTest.java: > 1. Create CallbackHelper to be notified when the activity is destroyed > 2. Create an ActivityStateListener; onActivityStateChanged() notifies callback > 3. Register ActivityStateListener > 4. Call finishAndRemoveTask() on the activity > 5. callbackHelper#waitForCallback() to wait until the activity is destroyed Cool! Didn't know about CallbackHelper, and it looks useful to help with this. I'll try to use it here.
https://codereview.chromium.org/2143203002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java (left): https://codereview.chromium.org/2143203002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:996: && TabWindowManager.getInstance().getNumberOfAssignedTabModelSelectors() == 1) { On 2016/07/19 16:11:23, agrieve wrote: > On 2016/07/19 15:29:41, Theresa Wellington wrote: > > I added this intentionally. If both tab models are loaded, they could both be > > changing e.g. > > > > 1. CleanUpTabStateDataTask gets created & started. > > 2. In the background getTabsFromOtherStateFiles() gets called; it returns > tab5, > > tab6, tab7 as belonging to the other state file. > > 3. A new tab8 is created in the other window. > > 4. CleanUpTabSTateDataTask#doInBackground() calls getStateDirectory.list(), > > which lists tab5, tab6, tab7, tab8 > > 5. tab8 gets deleted > > > > While this interleaving is unlikely, it does seem possible. If we're going to > > clear state while more than one activity is running, we should be checking > that > > activity's tab model rather than reading it's metadata file to determine which > > tabs are still alive. > > > > > > If we leave this on application cold start only, for now we may end up with > > extra incognito files that won't get cleared out until the next application > > start. Eventually we'll merge tabs on application cold start and this will be > a > > non-issue. > > Hrm, I was hoping to avoid the case where you skip loading them this time, they > are never cleaned up, then the task is closed OOM killed & re-opened with > bundle, then the tabs are reloaded. The tab state gets saved in onStopWithNative() so it *should* (but it's Android, so I won't make any strong promises) get written out when the activity is sent to the background before it's OOM killed. I think it'll work like this: 1. Load state for second window, incognito tabs don't get cleared 2. Do some stuff 3. Send second window to background 5. onStopWithNative() saves state (including tab metadata file) for second window 6. Second window killed by OOM 7. Second window restored, has saved instance state, incognito tab files are still present but tab metadata file doesn't reference them anymore so they aren't loaded. > WDYT about if we change the logic in CleanUpTabSTateDataTask to consult with > TabWindowManager to check against all in-memory models in order to avoid this > race? That would work too. I had opted not to do it initially because it's more complex and skipping cleanup when the second activity was created seemed fine at the time, but we could certainly add a method to TabWindowManager that checks if a tab with a given id exists in any model. Another option would be to ignore the incognito tabs in #getTabsFromOtherStateFiles() call to readSavedStateFile() (which generates the list of tab ids in the other state file(s)).
The CQ bit was checked by agrieve@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2143203002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelSelectorImpl.java (right): https://codereview.chromium.org/2143203002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelSelectorImpl.java:278: public void loadState(boolean ignoreIncognitoFiles) { On 2016/07/19 15:29:41, Theresa Wellington wrote: > nit: add JavaDoc for this new param Done. https://codereview.chromium.org/2143203002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java (left): https://codereview.chromium.org/2143203002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:996: && TabWindowManager.getInstance().getNumberOfAssignedTabModelSelectors() == 1) { On 2016/07/19 17:02:47, Theresa Wellington wrote: > On 2016/07/19 16:11:23, agrieve wrote: > > On 2016/07/19 15:29:41, Theresa Wellington wrote: > > > I added this intentionally. If both tab models are loaded, they could both > be > > > changing e.g. > > > > > > 1. CleanUpTabStateDataTask gets created & started. > > > 2. In the background getTabsFromOtherStateFiles() gets called; it returns > > tab5, > > > tab6, tab7 as belonging to the other state file. > > > 3. A new tab8 is created in the other window. > > > 4. CleanUpTabSTateDataTask#doInBackground() calls getStateDirectory.list(), > > > which lists tab5, tab6, tab7, tab8 > > > 5. tab8 gets deleted > > > > > > While this interleaving is unlikely, it does seem possible. If we're going > to > > > clear state while more than one activity is running, we should be checking > > that > > > activity's tab model rather than reading it's metadata file to determine > which > > > tabs are still alive. > > > > > > > > > If we leave this on application cold start only, for now we may end up with > > > extra incognito files that won't get cleared out until the next application > > > start. Eventually we'll merge tabs on application cold start and this will > be > > a > > > non-issue. > > > > Hrm, I was hoping to avoid the case where you skip loading them this time, > they > > are never cleaned up, then the task is closed OOM killed & re-opened with > > bundle, then the tabs are reloaded. > > The tab state gets saved in onStopWithNative() so it *should* (but it's Android, > so I won't make any strong promises) get written out when the activity is sent > to the background before it's OOM killed. > > I think it'll work like this: > 1. Load state for second window, incognito tabs don't get cleared > 2. Do some stuff > 3. Send second window to background > 5. onStopWithNative() saves state (including tab metadata file) for second > window > 6. Second window killed by OOM > 7. Second window restored, has saved instance state, incognito tab files are > still present but tab metadata file doesn't reference them anymore so they > aren't loaded. > > > WDYT about if we change the logic in CleanUpTabSTateDataTask to consult with > > TabWindowManager to check against all in-memory models in order to avoid this > > race? > > That would work too. I had opted not to do it initially because it's more > complex and skipping cleanup when the second activity was created seemed fine at > the time, but we could certainly add a method to TabWindowManager that checks if > a tab with a given id exists in any model. > > Another option would be to ignore the incognito tabs in > #getTabsFromOtherStateFiles() call to readSavedStateFile() (which generates the > list of tab ids in the other state file(s)). I've gone with adding TabWindowManager.tabExistsInAnySelector() to try and work around this problem. Also fixed a TODO(BUG) in the thumbnails related cleanup code by having it use the same "is this tab deletable" logic. https://codereview.chromium.org/2143203002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java (right): https://codereview.chromium.org/2143203002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:365: public void loadState(boolean ignoreIncognitoFiles) { On 2016/07/19 15:29:41, Theresa Wellington wrote: > nit: add JavaDoc for this new param here too Done. https://codereview.chromium.org/2143203002/diff/60001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/TabsTest.java (right): https://codereview.chromium.org/2143203002/diff/60001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/TabsTest.java:1679: getActivity().finish(); On 2016/07/19 15:29:41, Theresa Wellington wrote: > finishAndRemoveTask() will actually remove this from recents. finish() just > calls destroy, but the activity remains in recents. Tried switching to this method, but it turns out that it's not available pre-21, and also doesn't actually result in onDestroy() being called! https://codereview.chromium.org/2143203002/diff/60001/chrome/test/android/jav... File chrome/test/android/javatests/src/org/chromium/chrome/test/util/ApplicationTestUtils.java (right): https://codereview.chromium.org/2143203002/diff/60001/chrome/test/android/jav... chrome/test/android/javatests/src/org/chromium/chrome/test/util/ApplicationTestUtils.java:148: public static void waitForActivityOnDestroy(final Activity targetActivity, long timeoutMillis) On 2016/07/19 16:11:23, agrieve wrote: > On 2016/07/19 15:29:42, Theresa Wellington wrote: > > Instead of creating a new method here, you may be able to just use > > CriteriaHelper with getStateForActivity(). MultiWindowUtilsTest.java has an > > example of this: > > > https://cs.chromium.org/chromium/src/chrome/android/javatests/src/org/chromiu... > > Yeah, I was hoping to avoid polling. Although it's definitely much less code > (I'm torn). > > > > > > > If you do want to use the ActivityStateListener, I suggest doing something > like > > this in TabsTest.java: > > 1. Create CallbackHelper to be notified when the activity is destroyed > > 2. Create an ActivityStateListener; onActivityStateChanged() notifies callback > > 3. Register ActivityStateListener > > 4. Call finishAndRemoveTask() on the activity > > 5. callbackHelper#waitForCallback() to wait until the activity is destroyed > > Cool! Didn't know about CallbackHelper, and it looks useful to help with this. > I'll try to use it here. > Ended up just switching this to use finishAllChromeTasks(), and converting finishAllChromeTasks() to use CallbackHelper to avoid polling.
https://codereview.chromium.org/2143203002/diff/60001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/TabsTest.java (right): https://codereview.chromium.org/2143203002/diff/60001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/TabsTest.java:1679: getActivity().finish(); On 2016/07/20 17:32:27, agrieve wrote: > On 2016/07/19 15:29:41, Theresa Wellington wrote: > > finishAndRemoveTask() will actually remove this from recents. finish() just > > calls destroy, but the activity remains in recents. > > Tried switching to this method, but it turns out that it's not available pre-21, > and also doesn't actually result in onDestroy() being called! ApiCompatibilityUtils.finishAndRemoveTask(getActivity()) is our workaround for pre-21.
https://codereview.chromium.org/2143203002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java (right): https://codereview.chromium.org/2143203002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:1112: && TabWindowManager.getInstance().getNumberOfAssignedTabModelSelectors() == 1) { This is only happening on application cold start... which seems to negate the need for a method in TabWindowManager to check all existing models. https://codereview.chromium.org/2143203002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabWindowManager.java (right): https://codereview.chromium.org/2143203002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabWindowManager.java:163: if (TabModelUtils.getTabById(selector.getModel(false), tabId) != null) { This only checks the normal model. If the cleanup persistent data task is going to run more often than application cold start, then this needs to check the incognito tab models too or we'll incorrectly trash incognito tab files and thumbnails. https://codereview.chromium.org/2143203002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabWindowManager.java:169: // Account for tabs that were recently reparented and hasn't been attached to its new nit: s/hasn't/haven't s/its/their s/activity/activities since the sentence is talking about plural tabs and they may not all be getting attached to the same activity.
https://codereview.chromium.org/2143203002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/content/TabContentManager.java (left): https://codereview.chromium.org/2143203002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/content/TabContentManager.java:319: public void cleanUpPersistentData(final TabModelSelector modelSelector) { Thank you for addressing this long standing bug! I think for an M52 fix, we should focus just on deleting incognito files when there's no saved instance state and follow up with a better fix that addresses this thumbnail deletion bug & a global method to look for existing tabs for M53+ since I'd like to get more test coverage on that.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by agrieve@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by agrieve@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by agrieve@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by agrieve@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/07/20 17:57:46, Theresa Wellington wrote: > https://codereview.chromium.org/2143203002/diff/80001/chrome/android/java/src... > File > chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/content/TabContentManager.java > (left): > > https://codereview.chromium.org/2143203002/diff/80001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/content/TabContentManager.java:319: > public void cleanUpPersistentData(final TabModelSelector modelSelector) { > Thank you for addressing this long standing bug! > > I think for an M52 fix, we should focus just on deleting incognito files when > there's no saved instance state and follow up with a better fix that addresses > this thumbnail deletion bug & a global method to look for existing tabs for M53+ > since I'd like to get more test coverage on that. Tests finally passing. Woo! PTAL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2143203002/diff/160001/base/android/java/src/... File base/android/java/src/org/chromium/base/ApplicationStatus.java (right): https://codereview.chromium.org/2143203002/diff/160001/base/android/java/src/... base/android/java/src/org/chromium/base/ApplicationStatus.java:199: // this was the last activity. This generally seems more correct than removing it after notifying listeners, but please check places currently listening for destroyed activity's to make sure this doesn't break anything. https://codereview.chromium.org/2143203002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java (right): https://codereview.chromium.org/2143203002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:1256: || (!tabWindowManager.tabExistsInAnySelector(tabId) nit: make a shouldDeleteTabFile() method that can be called here and above? https://codereview.chromium.org/2143203002/diff/160001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/TabsTest.java (right): https://codereview.chromium.org/2143203002/diff/160001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/TabsTest.java:1675: ApplicationTestUtils.finishActivity(getActivity()); This test should have passed without your new changes as long as there no other Chrome processes were running. I think it would be more robust to spin up a second activity and leave it alive, finish the first activity and assert its incognito tabs are gone. https://codereview.chromium.org/2143203002/diff/160001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/TabPersistentStoreTest.java (right): https://codereview.chromium.org/2143203002/diff/160001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/TabPersistentStoreTest.java:251: private final TabWindowManager.TabModelSelectorFactory mMockTabModelSelectorFactory = nit: add a blank line before this one https://codereview.chromium.org/2143203002/diff/160001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/TabPersistentStoreTest.java:628: mFakeChromeActivity, ActivityState.DESTROYED); Why does the activity need to be destroyed here? https://codereview.chromium.org/2143203002/diff/160001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/TabWindowManagerTest.java (right): https://codereview.chromium.org/2143203002/diff/160001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/TabWindowManagerTest.java:237: * {@link Activity}s assigned {@link TabModelSelector}s. This comment doesn't seem to line up with what's being tested. https://codereview.chromium.org/2143203002/diff/160001/chrome/test/android/ja... File chrome/test/android/javatests/src/org/chromium/chrome/test/util/ApplicationTestUtils.java (right): https://codereview.chromium.org/2143203002/diff/160001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/util/ApplicationTestUtils.java:144: activity.finish(); Will this actually remove the task on Pre-L devices? https://codereview.chromium.org/2143203002/diff/160001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/util/ApplicationTestUtils.java:148: callbackHelper.waitUntilCriteria(new Criteria() { What's the advantage of using the CallbackHelper instead of CriteriaHelper?
https://codereview.chromium.org/2143203002/diff/60001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/TabsTest.java (right): https://codereview.chromium.org/2143203002/diff/60001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/TabsTest.java:1679: getActivity().finish(); On 2016/07/20 17:34:08, dfalcantara wrote: > On 2016/07/20 17:32:27, agrieve wrote: > > On 2016/07/19 15:29:41, Theresa Wellington wrote: > > > finishAndRemoveTask() will actually remove this from recents. finish() just > > > calls destroy, but the activity remains in recents. > > > > Tried switching to this method, but it turns out that it's not available > pre-21, > > and also doesn't actually result in onDestroy() being called! > > ApiCompatibilityUtils.finishAndRemoveTask(getActivity()) is our workaround for > pre-21. Done. https://codereview.chromium.org/2143203002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java (right): https://codereview.chromium.org/2143203002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:1112: && TabWindowManager.getInstance().getNumberOfAssignedTabModelSelectors() == 1) { On 2016/07/20 17:50:09, Theresa Wellington wrote: > This is only happening on application cold start... which seems to negate the > need for a method in TabWindowManager to check all existing models. :( I think I messed this up when merging. https://codereview.chromium.org/2143203002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabWindowManager.java (right): https://codereview.chromium.org/2143203002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabWindowManager.java:163: if (TabModelUtils.getTabById(selector.getModel(false), tabId) != null) { On 2016/07/20 17:50:09, Theresa Wellington wrote: > This only checks the normal model. If the cleanup persistent data task is going > to run more often than application cold start, then this needs to check the > incognito tab models too or we'll incorrectly trash incognito tab files and > thumbnails. Done (and added test). https://codereview.chromium.org/2143203002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabWindowManager.java:169: // Account for tabs that were recently reparented and hasn't been attached to its new On 2016/07/20 17:50:09, Theresa Wellington wrote: > nit: s/hasn't/haven't s/its/their s/activity/activities since the sentence is > talking about plural tabs and they may not all be getting attached to the same > activity. Done. https://codereview.chromium.org/2143203002/diff/80001/chrome/test/android/jav... File chrome/test/android/javatests/src/org/chromium/chrome/test/util/ApplicationTestUtils.java (left): https://codereview.chromium.org/2143203002/diff/80001/chrome/test/android/jav... chrome/test/android/javatests/src/org/chromium/chrome/test/util/ApplicationTestUtils.java:127: task.finishAndRemoveTask(); Figured out why this wasn't working for me - getAppTasks() is returning an empty list, so nothing gets finished :( https://codereview.chromium.org/2143203002/diff/160001/base/android/java/src/... File base/android/java/src/org/chromium/base/ApplicationStatus.java (right): https://codereview.chromium.org/2143203002/diff/160001/base/android/java/src/... base/android/java/src/org/chromium/base/ApplicationStatus.java:199: // this was the last activity. On 2016/07/26 18:48:17, Theresa Wellington wrote: > This generally seems more correct than removing it after notifying listeners, > but please check places currently listening for destroyed activity's to make > sure this doesn't break anything. Looks like it will probably affect this line: https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... It appears as though the condition would never have evaluated to true before. I'll pull this out into a separate change to call out this difference.
https://codereview.chromium.org/2143203002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java (right): https://codereview.chromium.org/2143203002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:1256: || (!tabWindowManager.tabExistsInAnySelector(tabId) On 2016/07/26 18:48:17, Theresa Wellington wrote: > nit: make a shouldDeleteTabFile() method that can be called here and above? Done. https://codereview.chromium.org/2143203002/diff/160001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/TabsTest.java (right): https://codereview.chromium.org/2143203002/diff/160001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/TabsTest.java:1675: ApplicationTestUtils.finishActivity(getActivity()); On 2016/07/26 18:48:17, Theresa Wellington wrote: > This test should have passed without your new changes as long as there no other > Chrome processes were running. I think it would be more robust to spin up a > second activity and leave it alive, finish the first activity and assert its > incognito tabs are gone. I verified that the test fails without my M52 change (since incognito files are never deleted). The test as-is does test the reported bug scenario: that if you swipe away an incognito-holding chrome activity and the Application is kept alive, then the files should be gone on next activity start. I also don't have much more time to spend on this :S https://codereview.chromium.org/2143203002/diff/160001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/TabPersistentStoreTest.java (right): https://codereview.chromium.org/2143203002/diff/160001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/TabPersistentStoreTest.java:251: private final TabWindowManager.TabModelSelectorFactory mMockTabModelSelectorFactory = On 2016/07/26 18:48:17, Theresa Wellington wrote: > nit: add a blank line before this one Done. https://codereview.chromium.org/2143203002/diff/160001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/TabPersistentStoreTest.java:628: mFakeChromeActivity, ActivityState.DESTROYED); On 2016/07/26 18:48:17, Theresa Wellington wrote: > Why does the activity need to be destroyed here? Added a comment. https://codereview.chromium.org/2143203002/diff/160001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/TabWindowManagerTest.java (right): https://codereview.chromium.org/2143203002/diff/160001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/TabWindowManagerTest.java:237: * {@link Activity}s assigned {@link TabModelSelector}s. On 2016/07/26 18:48:17, Theresa Wellington wrote: > This comment doesn't seem to line up with what's being tested. Done. https://codereview.chromium.org/2143203002/diff/160001/chrome/test/android/ja... File chrome/test/android/javatests/src/org/chromium/chrome/test/util/ApplicationTestUtils.java (right): https://codereview.chromium.org/2143203002/diff/160001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/util/ApplicationTestUtils.java:144: activity.finish(); On 2016/07/26 18:48:17, Theresa Wellington wrote: > Will this actually remove the task on Pre-L devices? If it's the only activity, then I think it will. If there's multiple activities, there's also .finishTask() that we could use (probably that'd be a better fallback for our ApiCompatibilityUtils method). This function only promises to finish the activity though, not the task. https://codereview.chromium.org/2143203002/diff/160001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/util/ApplicationTestUtils.java:148: callbackHelper.waitUntilCriteria(new Criteria() { On 2016/07/26 18:48:17, Theresa Wellington wrote: > What's the advantage of using the CallbackHelper instead of CriteriaHelper? CriteriaHelper polls & CallbackHelper doesn't.
https://codereview.chromium.org/2143203002/diff/160001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/TabsTest.java (right): https://codereview.chromium.org/2143203002/diff/160001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/TabsTest.java:1675: ApplicationTestUtils.finishActivity(getActivity()); On 2016/07/27 19:59:17, agrieve wrote: > On 2016/07/26 18:48:17, Theresa Wellington wrote: > > This test should have passed without your new changes as long as there no > other > > Chrome processes were running. I think it would be more robust to spin up a > > second activity and leave it alive, finish the first activity and assert its > > incognito tabs are gone. > > I verified that the test fails without my M52 change (since incognito files are > never deleted). Incognito files should have been getting deleted prior to your M52 changes as part of the normal clean up process, right? > The test as-is does test the reported bug scenario: that if you swipe away an > incognito-holding chrome activity and the Application is kept alive, then the > files should be gone on next activity start. Is the application really being kept alive in this scenario, though? The process may stay alive for a tad bit longer (up to Android when exactly to get rid of it), but I don't think that's guaranteed. It's really easy to start a new activity - MultiWindowUtilsTest#createSecondChromeTabbedActivity() https://cs.chromium.org/chromium/src/chrome/android/javatests/src/org/chromiu... > I also don't have much more time to spend on this :S https://codereview.chromium.org/2143203002/diff/160001/chrome/test/android/ja... File chrome/test/android/javatests/src/org/chromium/chrome/test/util/ApplicationTestUtils.java (right): https://codereview.chromium.org/2143203002/diff/160001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/util/ApplicationTestUtils.java:144: activity.finish(); On 2016/07/27 19:59:17, agrieve wrote: > On 2016/07/26 18:48:17, Theresa Wellington wrote: > > Will this actually remove the task on Pre-L devices? > > If it's the only activity, then I think it will. Sort of orthogonal, since we apparently don't actually need to remove the task - As far as I know, it does not remove the task; finish() pre-L just minimizes the app. We've run into this numerous times working on other features. The documentation says: finish(): Call this when your activity is done and should be closed. finishAndRemoveTask(): Call this when your activity is done and should be closed and the task should be completely removed as a part of finishing the root activity of the task. I don't think only having one activity running affects the behavior of either of these methods. > If there's multiple activities, there's also .finishTask() that we could use (probably that'd be a better fallback for our ApiCompatibilityUtils method). > > This function only promises to finish the activity though, not the task. Since the test is passing, I'm fine with leaving it as finish(). I had assumed that leaving the task around left a saved instance state, but apparently that is not the case. https://codereview.chromium.org/2143203002/diff/160001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/util/ApplicationTestUtils.java:148: callbackHelper.waitUntilCriteria(new Criteria() { On 2016/07/27 19:59:17, agrieve wrote: > On 2016/07/26 18:48:17, Theresa Wellington wrote: > > What's the advantage of using the CallbackHelper instead of CriteriaHelper? > > CriteriaHelper polls & CallbackHelper doesn't. CallbackHelper#waitUntilCriteria blocks - why is that desired over polling?
https://codereview.chromium.org/2143203002/diff/160001/chrome/test/android/ja... File chrome/test/android/javatests/src/org/chromium/chrome/test/util/ApplicationTestUtils.java (right): https://codereview.chromium.org/2143203002/diff/160001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/util/ApplicationTestUtils.java:148: callbackHelper.waitUntilCriteria(new Criteria() { On 2016/07/27 22:53:59, Theresa Wellington wrote: > On 2016/07/27 19:59:17, agrieve wrote: > > On 2016/07/26 18:48:17, Theresa Wellington wrote: > > > What's the advantage of using the CallbackHelper instead of CriteriaHelper? > > > > CriteriaHelper polls & CallbackHelper doesn't. > > CallbackHelper#waitUntilCriteria blocks - why is that desired over polling? Instead of using waitUntilCriteria here, can we rename the incoming variable to something like activityToDestroy and do this: @Override public void onActivityStateChange(Activity activity, int newState) { if (activity == activityToDestroy && newState == ActivityState.DESTROYED) { callbackHelper.notifyCalled(); } } And then the callback helper can just wait for a callback instead of blocking until the criteria is satisfied.
https://codereview.chromium.org/2143203002/diff/160001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/TabsTest.java (right): https://codereview.chromium.org/2143203002/diff/160001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/TabsTest.java:1675: ApplicationTestUtils.finishActivity(getActivity()); On 2016/07/27 22:53:59, Theresa Wellington wrote: > On 2016/07/27 19:59:17, agrieve wrote: > > On 2016/07/26 18:48:17, Theresa Wellington wrote: > > > This test should have passed without your new changes as long as there no > > other > > > Chrome processes were running. I think it would be more robust to spin up a > > > second activity and leave it alive, finish the first activity and assert its > > > incognito tabs are gone. > > > > I verified that the test fails without my M52 change (since incognito files > are > > never deleted). > > Incognito files should have been getting deleted prior to your M52 changes as > part of the normal clean up process, right? > > > The test as-is does test the reported bug scenario: that if you swipe away an > > incognito-holding chrome activity and the Application is kept alive, then the > > files should be gone on next activity start. > > Is the application really being kept alive in this scenario, though? The process > may stay alive for a tad bit longer (up to Android when exactly to get rid of > it), but I don't think that's guaranteed. > > It's really easy to start a new activity - > MultiWindowUtilsTest#createSecondChromeTabbedActivity() > https://cs.chromium.org/chromium/src/chrome/android/javatests/src/org/chromiu... > > > I also don't have much more time to spend on this :S > It's guaranteed in these tests because they live in the same process as Chrome. Before my M52 change, the assertFalse on 1679 will fail because the tab is restored due to the cipher data still kicking around (due to it being stored in a static field) https://codereview.chromium.org/2143203002/diff/160001/chrome/test/android/ja... File chrome/test/android/javatests/src/org/chromium/chrome/test/util/ApplicationTestUtils.java (right): https://codereview.chromium.org/2143203002/diff/160001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/util/ApplicationTestUtils.java:144: activity.finish(); On 2016/07/27 22:53:59, Theresa Wellington wrote: > On 2016/07/27 19:59:17, agrieve wrote: > > On 2016/07/26 18:48:17, Theresa Wellington wrote: > > > Will this actually remove the task on Pre-L devices? > > > > If it's the only activity, then I think it will. > > Sort of orthogonal, since we apparently don't actually need to remove the task - > As far as I know, it does not remove the task; finish() pre-L just minimizes the > app. We've run into this numerous times working on other features. > > The documentation says: > > finish(): Call this when your activity is done and should be closed. > > finishAndRemoveTask(): Call this when your activity is done and should be closed > and the task should be completely removed as a part of finishing the root > activity of the task. > > I don't think only having one activity running affects the behavior of either of > these methods. > > > If there's multiple activities, there's also .finishTask() that we could use > (probably that'd be a better fallback for our ApiCompatibilityUtils method). > > > > This function only promises to finish the activity though, not the task. > > Since the test is passing, I'm fine with leaving it as finish(). I had assumed > that leaving the task around left a saved instance state, but apparently that is > not the case. Okay, great to know! Yeah, from what I've googled, it's actually not an easy thing to get Activities within instrumentation tests to have a restored bundle. https://codereview.chromium.org/2143203002/diff/160001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/util/ApplicationTestUtils.java:148: callbackHelper.waitUntilCriteria(new Criteria() { On 2016/07/27 22:53:59, Theresa Wellington wrote: > On 2016/07/27 19:59:17, agrieve wrote: > > On 2016/07/26 18:48:17, Theresa Wellington wrote: > > > What's the advantage of using the CallbackHelper instead of CriteriaHelper? > > > > CriteriaHelper polls & CallbackHelper doesn't. > > CallbackHelper#waitUntilCriteria blocks - why is that desired over polling? Because it's faster. https://codereview.chromium.org/2143203002/diff/160001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/util/ApplicationTestUtils.java:148: callbackHelper.waitUntilCriteria(new Criteria() { On 2016/07/27 23:19:27, Theresa Wellington wrote: > On 2016/07/27 22:53:59, Theresa Wellington wrote: > > On 2016/07/27 19:59:17, agrieve wrote: > > > On 2016/07/26 18:48:17, Theresa Wellington wrote: > > > > What's the advantage of using the CallbackHelper instead of > CriteriaHelper? > > > > > > CriteriaHelper polls & CallbackHelper doesn't. > > > > CallbackHelper#waitUntilCriteria blocks - why is that desired over polling? > > Instead of using waitUntilCriteria here, can we rename the incoming variable to > something like activityToDestroy and do this: > > @Override > public void onActivityStateChange(Activity activity, int newState) { > if (activity == activityToDestroy && newState == ActivityState.DESTROYED) { > callbackHelper.notifyCalled(); > } > } > > And then the callback helper can just wait for a callback instead of blocking > until the criteria is satisfied. Done.
lgtm https://codereview.chromium.org/2143203002/diff/160001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/TabsTest.java (right): https://codereview.chromium.org/2143203002/diff/160001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/TabsTest.java:1675: ApplicationTestUtils.finishActivity(getActivity()); On 2016/07/28 00:54:25, agrieve wrote: > On 2016/07/27 22:53:59, Theresa Wellington wrote: > > On 2016/07/27 19:59:17, agrieve wrote: > > > On 2016/07/26 18:48:17, Theresa Wellington wrote: > > > > This test should have passed without your new changes as long as there no > > > other > > > > Chrome processes were running. I think it would be more robust to spin up > a > > > > second activity and leave it alive, finish the first activity and assert > its > > > > incognito tabs are gone. > > > > > > I verified that the test fails without my M52 change (since incognito files > > are > > > never deleted). > > > > Incognito files should have been getting deleted prior to your M52 changes as > > part of the normal clean up process, right? > > > > > The test as-is does test the reported bug scenario: that if you swipe away > an > > > incognito-holding chrome activity and the Application is kept alive, then > the > > > files should be gone on next activity start. > > > > Is the application really being kept alive in this scenario, though? The > process > > may stay alive for a tad bit longer (up to Android when exactly to get rid of > > it), but I don't think that's guaranteed. > > > > It's really easy to start a new activity - > > MultiWindowUtilsTest#createSecondChromeTabbedActivity() > > > https://cs.chromium.org/chromium/src/chrome/android/javatests/src/org/chromiu... > > > > > I also don't have much more time to spend on this :S > > > > It's guaranteed in these tests because they live in the same process as Chrome. > > Before my M52 change, the assertFalse on 1679 will fail because the tab is > restored due to the cipher data still kicking around (due to it being stored in > a static field) Okay, makes sense. Will you please add a comment to that effect? https://codereview.chromium.org/2143203002/diff/160001/chrome/test/android/ja... File chrome/test/android/javatests/src/org/chromium/chrome/test/util/ApplicationTestUtils.java (right): https://codereview.chromium.org/2143203002/diff/160001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/util/ApplicationTestUtils.java:148: callbackHelper.waitUntilCriteria(new Criteria() { On 2016/07/28 00:54:25, agrieve wrote: > On 2016/07/27 22:53:59, Theresa Wellington wrote: > > On 2016/07/27 19:59:17, agrieve wrote: > > > On 2016/07/26 18:48:17, Theresa Wellington wrote: > > > > What's the advantage of using the CallbackHelper instead of > CriteriaHelper? > > > > > > CriteriaHelper polls & CallbackHelper doesn't. > > > > CallbackHelper#waitUntilCriteria blocks - why is that desired over polling? > > Because it's faster. Ah, that's a good reason. Thanks for changing it. https://codereview.chromium.org/2143203002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java (right): https://codereview.chromium.org/2143203002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:1219: private boolean shouldDeleteTabFile(int tabId, TabWindowManager tabWindowManager) { nit: put this after onPostExecute() https://codereview.chromium.org/2143203002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:1220: return mDeleteAllFiles || (!tabWindowManager.tabExistsInAnySelector(tabId) nit: does the entire || fit on the second line? https://codereview.chromium.org/2143203002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabWindowManager.java (right): https://codereview.chromium.org/2143203002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabWindowManager.java:159: public boolean tabExistsInAnySelector(int tabId) { nit: JavaDoc for tabId (even though it's obvious) https://codereview.chromium.org/2143203002/diff/200001/chrome/test/android/ja... File chrome/test/android/javatests/src/org/chromium/chrome/test/util/ApplicationTestUtils.java (right): https://codereview.chromium.org/2143203002/diff/200001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/util/ApplicationTestUtils.java:125: public static void finishActivity(final Activity activityToDestroy) throws Exception { nit: this can just be activity since there's no need to check if the activities are == in onActivityStateChange (forgot it was a listener for the specific activity).
https://codereview.chromium.org/2143203002/diff/160001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/TabsTest.java (right): https://codereview.chromium.org/2143203002/diff/160001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/TabsTest.java:1675: ApplicationTestUtils.finishActivity(getActivity()); On 2016/07/28 01:19:46, Theresa Wellington wrote: > On 2016/07/28 00:54:25, agrieve wrote: > > On 2016/07/27 22:53:59, Theresa Wellington wrote: > > > On 2016/07/27 19:59:17, agrieve wrote: > > > > On 2016/07/26 18:48:17, Theresa Wellington wrote: > > > > > This test should have passed without your new changes as long as there > no > > > > other > > > > > Chrome processes were running. I think it would be more robust to spin > up > > a > > > > > second activity and leave it alive, finish the first activity and assert > > its > > > > > incognito tabs are gone. > > > > > > > > I verified that the test fails without my M52 change (since incognito > files > > > are > > > > never deleted). > > > > > > Incognito files should have been getting deleted prior to your M52 changes > as > > > part of the normal clean up process, right? > > > > > > > The test as-is does test the reported bug scenario: that if you swipe away > > an > > > > incognito-holding chrome activity and the Application is kept alive, then > > the > > > > files should be gone on next activity start. > > > > > > Is the application really being kept alive in this scenario, though? The > > process > > > may stay alive for a tad bit longer (up to Android when exactly to get rid > of > > > it), but I don't think that's guaranteed. > > > > > > It's really easy to start a new activity - > > > MultiWindowUtilsTest#createSecondChromeTabbedActivity() > > > > > > https://cs.chromium.org/chromium/src/chrome/android/javatests/src/org/chromiu... > > > > > > > I also don't have much more time to spend on this :S > > > > > > > It's guaranteed in these tests because they live in the same process as > Chrome. > > > > Before my M52 change, the assertFalse on 1679 will fail because the tab is > > restored due to the cipher data still kicking around (due to it being stored > in > > a static field) > > Okay, makes sense. Will you please add a comment to that effect? Done. https://codereview.chromium.org/2143203002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java (right): https://codereview.chromium.org/2143203002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:1219: private boolean shouldDeleteTabFile(int tabId, TabWindowManager tabWindowManager) { On 2016/07/28 01:19:46, Theresa Wellington wrote: > nit: put this after onPostExecute() Done. https://codereview.chromium.org/2143203002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java:1220: return mDeleteAllFiles || (!tabWindowManager.tabExistsInAnySelector(tabId) On 2016/07/28 01:19:46, Theresa Wellington wrote: > nit: does the entire || fit on the second line? not quite :( https://codereview.chromium.org/2143203002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabWindowManager.java (right): https://codereview.chromium.org/2143203002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabWindowManager.java:159: public boolean tabExistsInAnySelector(int tabId) { On 2016/07/28 01:19:46, Theresa Wellington wrote: > nit: JavaDoc for tabId (even though it's obvious) Done. https://codereview.chromium.org/2143203002/diff/200001/chrome/test/android/ja... File chrome/test/android/javatests/src/org/chromium/chrome/test/util/ApplicationTestUtils.java (right): https://codereview.chromium.org/2143203002/diff/200001/chrome/test/android/ja... chrome/test/android/javatests/src/org/chromium/chrome/test/util/ApplicationTestUtils.java:125: public static void finishActivity(final Activity activityToDestroy) throws Exception { On 2016/07/28 01:19:46, Theresa Wellington wrote: > nit: this can just be activity since there's no need to check if the activities > are == in onActivityStateChange (forgot it was a listener for the specific > activity). Done.
Commit message needs a serious update.
Description was changed from ========== Fix incognito tabs persisting when cct service is active Swiping away the Chrome activity does not cause the process to be killed if a Chrome Custom Tab is open somewhere. In this case, when Chrome is started again, the CipherData static still has the old Activity's incognito key stored in it and the tabs are unexpectedly restored. This changes the start-up behaviour to explicitly clear any existing cipher data when an activity starts without any bundle data. BUG=626629 ========== to ========== Tidy up of TabPersistentStore's clean-up logic * Clean files on every activity start rather than just the first * Don't delete files owned by other TabModels * Don't delete icons used by other TabModels * Adds test for incognito tab files being removed appropraitely BUG=626629 ==========
On 2016/07/28 16:36:10, dfalcantara wrote: > Commit message needs a serious update. Updated seriously.
Lol, the description change explains why the code didn't make any sense when I looked it over. I updated the subject of the CL to match. lgtm
The CQ bit was checked by agrieve@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from twellington@chromium.org Link to the patchset: https://codereview.chromium.org/2143203002/#ps220001 (title: "Fix incognito tabs persisting when cct service is active")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Tidy up of TabPersistentStore's clean-up logic * Clean files on every activity start rather than just the first * Don't delete files owned by other TabModels * Don't delete icons used by other TabModels * Adds test for incognito tab files being removed appropraitely BUG=626629 ========== to ========== Tidy up of TabPersistentStore's clean-up logic * Clean files on every activity start rather than just the first * Don't delete files owned by other TabModels * Don't delete icons used by other TabModels * Adds test for incognito tab files being removed appropraitely BUG=626629 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Tidy up of TabPersistentStore's clean-up logic * Clean files on every activity start rather than just the first * Don't delete files owned by other TabModels * Don't delete icons used by other TabModels * Adds test for incognito tab files being removed appropraitely BUG=626629 ========== to ========== Tidy up of TabPersistentStore's clean-up logic * Clean files on every activity start rather than just the first * Don't delete files owned by other TabModels * Don't delete icons used by other TabModels * Adds test for incognito tab files being removed appropraitely BUG=626629 Committed: https://crrev.com/b9ef755c73b5852c66a20b41ddc8f3624ee85ef6 Cr-Commit-Position: refs/heads/master@{#408566} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/b9ef755c73b5852c66a20b41ddc8f3624ee85ef6 Cr-Commit-Position: refs/heads/master@{#408566} |
