Fix incognito tabs persisting when cct service is active

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 ignore incognito tab
state files when no bundle is passed to the activity's onCreate.

BUG=626629
Committed: https://crrev.com/eb712422db840465b17318a3fefc99a63e2cc734
Cr-Commit-Position: refs/heads/master@{#406747}

[agrieve, 2016-07-20 20:00:25]

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

[agrieve, 2016-07-20 20:00:26]

Created a new review to have just the changes for M52. Will rebase
https://codereview.chromium.org/2143203002 ontop of it.

[Theresa, 2016-07-20 20:19:14]

lgtm % I think we should restrict testIncognitoTabsNotRestoredAfterSwipe() to L+

https://codereview.chromium.org/2166943002/diff/1/chrome/test/android/javates...
File
chrome/test/android/javatests/src/org/chromium/chrome/test/util/ApplicationTestUtils.java
(right):

https://codereview.chromium.org/2166943002/diff/1/chrome/test/android/javates...
chrome/test/android/javatests/src/org/chromium/chrome/test/util/ApplicationTestUtils.java:137:
ApiCompatibilityUtils.finishAndRemoveTask(activity);
Just changing to ApiCompatiabilityUtils#finishAndRemoveTask() doesn't mean the
new test will pass pass on pre-L devices. That method just calls
Activity#finish() pre-L; since the task isn't removed from recents, I think the
activity will still have a saved instance state.

The current implementation's way of finishing a;l chrome tasks was also more
robust. A task may still be running even if the activity is not. e.g. The test
creates two activities, one immediately dies in the background.
ActivityManager#getAppTasks() will still return a task for it, but
ApplicationStatus.getRunningActivities() will not return an entry for the
activity since it was destroyed.

My preference is to revert the changes to this method and just restrict
testIncognitoTabsNotRestoredAfterSwipe() to L+ (@MinAndroidSdkLevel annotation)
(at least for now).

[gone, 2016-07-20 20:29:21]

lgtm % Teresa's comments about the test change.  ApplicationTestUtils#setUp
affects every single test, which we probably shouldn't do for M52.

https://codereview.chromium.org/2166943002/diff/1/chrome/android/javatests/sr...
File chrome/android/javatests/src/org/chromium/chrome/browser/TabsTest.java
(right):

https://codereview.chromium.org/2166943002/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/TabsTest.java:1658:
@Smoke
Don't think anyone cares about the Smoke test distinction anymore.

[agrieve, 2016-07-20 20:41:15]

WDYT about just deleting the test for this patch and adding it in for the next
one (so that we change as little as possible)?

https://codereview.chromium.org/2166943002/diff/1/chrome/test/android/javates...
File
chrome/test/android/javatests/src/org/chromium/chrome/test/util/ApplicationTestUtils.java
(right):

https://codereview.chromium.org/2166943002/diff/1/chrome/test/android/javates...
chrome/test/android/javatests/src/org/chromium/chrome/test/util/ApplicationTestUtils.java:137:
ApiCompatibilityUtils.finishAndRemoveTask(activity);
On 2016/07/20 20:19:14, Theresa Wellington wrote:
> Just changing to ApiCompatiabilityUtils#finishAndRemoveTask() doesn't mean the
> new test will pass pass on pre-L devices. That method just calls
> Activity#finish() pre-L; since the task isn't removed from recents, I think
the
> activity will still have a saved instance state.

I've tested that the test passes when using .finish(). No bundleData is ever
passed for the next onCreate().

> 
> The current implementation's way of finishing a;l chrome tasks was also more
> robust. A task may still be running even if the activity is not. e.g. The test
> creates two activities, one immediately dies in the background.
> ActivityManager#getAppTasks() will still return a task for it, but
> ApplicationStatus.getRunningActivities() will not return an entry for the
> activity since it was destroyed.
> 
> My preference is to revert the changes to this method and just restrict
> testIncognitoTabsNotRestoredAfterSwipe() to L+ (@MinAndroidSdkLevel
annotation)
> (at least for now).

task.finishAndRemoveTask() never calls onDestroy() for the Activity, so the test
actually doesn't work when using this call.

[agrieve, 2016-07-20 20:42:26]

On 2016/07/20 20:41:15, agrieve wrote:
> WDYT about just deleting the test for this patch and adding it in for the next
> one (so that we change as little as possible)?
> 
>
https://codereview.chromium.org/2166943002/diff/1/chrome/test/android/javates...
> File
>
chrome/test/android/javatests/src/org/chromium/chrome/test/util/ApplicationTestUtils.java
> (right):
> 
>
https://codereview.chromium.org/2166943002/diff/1/chrome/test/android/javates...
>
chrome/test/android/javatests/src/org/chromium/chrome/test/util/ApplicationTestUtils.java:137:
> ApiCompatibilityUtils.finishAndRemoveTask(activity);
> On 2016/07/20 20:19:14, Theresa Wellington wrote:
> > Just changing to ApiCompatiabilityUtils#finishAndRemoveTask() doesn't mean
the
> > new test will pass pass on pre-L devices. That method just calls
> > Activity#finish() pre-L; since the task isn't removed from recents, I think
> the
> > activity will still have a saved instance state.
> 
> I've tested that the test passes when using .finish(). No bundleData is ever
> passed for the next onCreate().
> 
> > 
> > The current implementation's way of finishing a;l chrome tasks was also more
> > robust. A task may still be running even if the activity is not. e.g. The
test
> > creates two activities, one immediately dies in the background.
> > ActivityManager#getAppTasks() will still return a task for it, but
> > ApplicationStatus.getRunningActivities() will not return an entry for the
> > activity since it was destroyed.
> > 
> > My preference is to revert the changes to this method and just restrict
> > testIncognitoTabsNotRestoredAfterSwipe() to L+ (@MinAndroidSdkLevel
> annotation)
> > (at least for now).
> 
> task.finishAndRemoveTask() never calls onDestroy() for the Activity, so the
test
> actually doesn't work when using this call.

PS - heading home now. Will be back at it around 10pm / 7pm tonight, so if the
urgency is there, feel free to clone & land. Otherwise I'll get to it tonight.

[Theresa, 2016-07-20 20:43:50]

On 2016/07/20 20:41:15, agrieve wrote:
> WDYT about just deleting the test for this patch and adding it in for the next
> one (so that we change as little as possible)?

That's fine with me.

> task.finishAndRemoveTask() never calls onDestroy() for the Activity, so the
test
> actually doesn't work when using this call.

Huh. The activity still gets destroyed though, so when it's started back up, it
shouldn't have a saved instance state and the test should pass.

[Theresa, 2016-07-20 20:43:51]

On 2016/07/20 20:41:15, agrieve wrote:
> WDYT about just deleting the test for this patch and adding it in for the next
> one (so that we change as little as possible)?

That's fine with me.

> task.finishAndRemoveTask() never calls onDestroy() for the Activity, so the
test
> actually doesn't work when using this call.

Huh. The activity still gets destroyed though, so when it's started back up, it
shouldn't have a saved instance state and the test should pass.

[agrieve, 2016-07-21 02:10:37]

The CQ bit was checked by agrieve@chromium.org

[agrieve, 2016-07-21 02:10:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
twellington@chromium.org, dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/2166943002/#ps20001
(title: "remove tests")

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

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

[agrieve, 2016-07-21 02:20:16]

On 2016/07/20 20:43:51, Theresa Wellington wrote:
> On 2016/07/20 20:41:15, agrieve wrote:
> > WDYT about just deleting the test for this patch and adding it in for the
next
> > one (so that we change as little as possible)?
> 
> That's fine with me.
> 
> > task.finishAndRemoveTask() never calls onDestroy() for the Activity, so the
> test
> > actually doesn't work when using this call.
> 
> Huh. The activity still gets destroyed though, so when it's started back up,
it
> shouldn't have a saved instance state and the test should pass.

Test change removed & submitting away!

This issue I was seeing with onDestroy not being called, is that the
CallbackHelper never resolves, so the test times out. I'll try increasing the
timeout just to verify it's not super delayed.

[commit-bot: I haz the power, 2016-07-21 03:07:10]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-07-21 03:08:31]

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 ignore incognito tab
state files when no bundle is passed to the activity's onCreate.

BUG=626629
==========

to

==========
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 ignore incognito tab
state files when no bundle is passed to the activity's onCreate.

BUG=626629

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

[commit-bot: I haz the power, 2016-07-21 03:08:32]

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