Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2847263003:
Add tests to make sure closed CCT activities are not leaked. (Closed)
|
Created:
3 years, 7 months ago by DmitrySkiba Modified:
3 years, 7 months ago CC:
chromium-reviews, danakj+watch_chromium.org, agrieve+watch_chromium.org, lizeb+watch-custom-tabs_chromium.org, vmpstr+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd tests to make sure closed CCT activities are not leaked.
BUG=715724, 710612
Review-Url: https://codereview.chromium.org/2847263003
Cr-Commit-Position: refs/heads/master@{#470057}
Committed: https://chromium.googlesource.com/chromium/src/+/d50d8466d0bdae0b5ae8af4108ecaa1376ed5f21
Patch Set 1 #Patch Set 2 : Add comments; simplify logic #Patch Set 3 : Comment getUiThreadHandler #
Total comments: 24
Patch Set 4 : Address comments #
Total comments: 1
Patch Set 5 : Start extra non-CCT activity #Patch Set 6 : Debugging failure on a bot #Patch Set 7 : Revert debugging #
Messages
Total messages: 29 (14 generated)
The CQ bit was checked by dskiba@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...)
dskiba@chromium.org changed reviewers: + tedchoc@chromium.org, yusufo@chromium.org
Guys PTAL. I checked that reverting last two leak-related makes tests fail.
dskiba@chromium.org changed reviewers: + torne@chromium.org
torne@: please review changes in ThreadUtils.java - I had to make getUiThreadHandler() public in order to cancel a task in ToolbarManager - otherwise it interferes with leak tests by keeping a reference to the activity for 30s.
https://codereview.chromium.org/2847263003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java (right): https://codereview.chromium.org/2847263003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java:827: if (mDeferredStartupRunnable != null) { Is this function always running on the UI thread?
On 2017/05/01 19:23:47, Torne wrote: > https://codereview.chromium.org/2847263003/diff/40001/chrome/android/java/src... > File > chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java > (right): > > https://codereview.chromium.org/2847263003/diff/40001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java:827: > if (mDeferredStartupRunnable != null) { > Is this function always running on the UI thread? Yes, it's called directly from ChromeActivity.onDestroy()
lgtm
https://codereview.chromium.org/2847263003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java (right): https://codereview.chromium.org/2847263003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java:1136: onDeferredStartup(activityCreationTimeMs, activityName); you'll need to set mDeferredStartupRunnable = null here to avoid double running it in destroy() https://codereview.chromium.org/2847263003/diff/40001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java (right): https://codereview.chromium.org/2847263003/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java:2347: @SmallTest each of these should be LargeTest (at least!) https://codereview.chromium.org/2847263003/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java:2402: final ArrayList<WeakReference<CustomTabActivity>> activityRefs = new ArrayList<>(); we should use List<WeakRef instead of ArrayList<WeakRef https://codereview.chromium.org/2847263003/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java:2416: assertNotNull("Activity is prematurely GCed", activity); I don't think you have any guarantee to ensure this. I think it is actually quite likely that on many devices nothing but the foreground activity might be destroyed. I think we should allow this case to happen and not trigger the close (IMO). https://codereview.chromium.org/2847263003/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java:2426: // Note that we're not checking the last activity. That's because something hmm...what happens in the reverse(activityRefs) case? Doesn't that mess up which is the "last" activity? https://codereview.chromium.org/2847263003/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java:2430: // anyway. So we're simply not checking the last activity. Can we just create another Activity to launch at the end to not have to do this? We add activities just in our test code for this: CustomTabExternalNavigationTest$DummyActivityForSpecialScheme for example https://codereview.chromium.org/2847263003/diff/40001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTestBase.java (right): https://codereview.chromium.org/2847263003/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTestBase.java:57: protected CustomTabActivity launchCustomTabActivityCompletely(Intent intent) { static? https://codereview.chromium.org/2847263003/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTestBase.java:98: protected void waitForCustomTab(final CustomTabActivity activity) throws InterruptedException { can this be static? https://codereview.chromium.org/2847263003/diff/40001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabsTestUtils.java (right): https://codereview.chromium.org/2847263003/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabsTestUtils.java:33: builder.setShowTitle(true); I don't know if this is minimal anymore. We might want to add this in the tests explicitly because this could get removed and maybe that causes our tests to start passing incorrectly. https://codereview.chromium.org/2847263003/diff/40001/chrome/test/android/jav... File chrome/test/android/javatests/src/org/chromium/chrome/test/util/ApplicationTestUtils.java (right): https://codereview.chromium.org/2847263003/diff/40001/chrome/test/android/jav... chrome/test/android/javatests/src/org/chromium/chrome/test/util/ApplicationTestUtils.java:138: public interface ActivityCloser<A extends Activity> { void close(A activity); } maybe we could just use Callback<Activity> to avoid the additional interface?
https://codereview.chromium.org/2847263003/diff/40001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java (right): https://codereview.chromium.org/2847263003/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java:2448: assertNull("Activity is leaked", ref.get()); should this be a poll instead of assert? Are we sure this will happen syncronously?
https://codereview.chromium.org/2847263003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java (right): https://codereview.chromium.org/2847263003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java:1136: onDeferredStartup(activityCreationTimeMs, activityName); On 2017/05/02 17:48:10, Ted C wrote: > you'll need to set mDeferredStartupRunnable = null here to avoid double running > it in destroy() Done. https://codereview.chromium.org/2847263003/diff/40001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java (right): https://codereview.chromium.org/2847263003/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java:2347: @SmallTest On 2017/05/02 17:48:10, Ted C wrote: > each of these should be LargeTest (at least!) Done. https://codereview.chromium.org/2847263003/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java:2402: final ArrayList<WeakReference<CustomTabActivity>> activityRefs = new ArrayList<>(); On 2017/05/02 17:48:10, Ted C wrote: > we should use List<WeakRef instead of ArrayList<WeakRef Done. https://codereview.chromium.org/2847263003/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java:2416: assertNotNull("Activity is prematurely GCed", activity); On 2017/05/02 17:48:10, Ted C wrote: > I don't think you have any guarantee to ensure this. I think it is actually > quite likely that on many devices nothing but the foreground activity might be > destroyed. > > I think we should allow this case to happen and not trigger the close (IMO). Done. Checked, and it works when activities are not kept. https://codereview.chromium.org/2847263003/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java:2426: // Note that we're not checking the last activity. That's because something On 2017/05/02 17:48:10, Ted C wrote: > hmm...what happens in the reverse(activityRefs) case? Doesn't that mess up > which is the "last" activity? Nothing, the last activity here refers to the last one passed to closeActivity() above. I'll update the comment. https://codereview.chromium.org/2847263003/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java:2430: // anyway. So we're simply not checking the last activity. On 2017/05/02 17:48:10, Ted C wrote: > Can we just create another Activity to launch at the end to not have to do this? > > We add activities just in our test code for this: > CustomTabExternalNavigationTest$DummyActivityForSpecialScheme > for example No, it's something in ApplicationTestUtils.closeActivity() - which we need to call. I.e. we can create one more activity, and then close it, but then it becomes similar to what we're already doing. https://codereview.chromium.org/2847263003/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java:2448: assertNull("Activity is leaked", ref.get()); On 2017/05/02 17:56:19, Yusuf wrote: > should this be a poll instead of assert? Are we sure this will happen > syncronously? Yes, System.gc() is synchronous, and it *should* collect all the available garbage. We'll see how flaky it is (not flaky in my tests), and then poll if needed. https://codereview.chromium.org/2847263003/diff/40001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTestBase.java (right): https://codereview.chromium.org/2847263003/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTestBase.java:57: protected CustomTabActivity launchCustomTabActivityCompletely(Intent intent) { On 2017/05/02 17:48:10, Ted C wrote: > static? Nope, it calls getInstrumentation(), which is not static. https://codereview.chromium.org/2847263003/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTestBase.java:98: protected void waitForCustomTab(final CustomTabActivity activity) throws InterruptedException { On 2017/05/02 17:48:10, Ted C wrote: > can this be static? Done. https://codereview.chromium.org/2847263003/diff/40001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabsTestUtils.java (right): https://codereview.chromium.org/2847263003/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabsTestUtils.java:33: builder.setShowTitle(true); On 2017/05/02 17:48:10, Ted C wrote: > I don't know if this is minimal anymore. We might want to add this in the tests > explicitly because this could get removed and maybe that causes our tests to > start passing incorrectly. Oops, this was a leftover from an earlier experiment. https://codereview.chromium.org/2847263003/diff/40001/chrome/test/android/jav... File chrome/test/android/javatests/src/org/chromium/chrome/test/util/ApplicationTestUtils.java (right): https://codereview.chromium.org/2847263003/diff/40001/chrome/test/android/jav... chrome/test/android/javatests/src/org/chromium/chrome/test/util/ApplicationTestUtils.java:138: public interface ActivityCloser<A extends Activity> { void close(A activity); } On 2017/05/02 17:48:10, Ted C wrote: > maybe we could just use Callback<Activity> to avoid the additional interface? Yes, it's possible, but calling Callback.onResult(activity) to close the activity feels wrong. How big of a problem additional interface is?
I'm slightly worried that if we can't solve the issue where the last activity isn't checked for leaks then we'll have the same issue where classes can hold onto a stale activity longer than they should (which is one of the leaks we did fix recently). https://codereview.chromium.org/2847263003/diff/40001/chrome/test/android/jav... File chrome/test/android/javatests/src/org/chromium/chrome/test/util/ApplicationTestUtils.java (right): https://codereview.chromium.org/2847263003/diff/40001/chrome/test/android/jav... chrome/test/android/javatests/src/org/chromium/chrome/test/util/ApplicationTestUtils.java:138: public interface ActivityCloser<A extends Activity> { void close(A activity); } On 2017/05/03 17:53:32, DmitrySkiba wrote: > On 2017/05/02 17:48:10, Ted C wrote: > > maybe we could just use Callback<Activity> to avoid the additional interface? > > Yes, it's possible, but calling Callback.onResult(activity) to close the > activity feels wrong. How big of a problem additional interface is? A new interface isn't a big deal, but we don't really want a proliferation of one method single param interfaces in the codebase. I do think onResult is a bit weird of the callback param. I'll defer to you here, but I would still likely use a Callback purely to avoid writing it, but you already wrote it so hey :-P https://codereview.chromium.org/2847263003/diff/60001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java (right): https://codereview.chromium.org/2847263003/diff/60001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java:2431: // Note that we're not checking the last closed activity. That's because would more calls to .gc() fix this? I wonder if having things being accessed across scope using final delays the GC of the outer referenced objects.
Hmm, you are right, that last unchecked activity will mask issues where last opened activity is leaked. I'll rework the code to use unrelated test activity instead (as you suggested).
Done, PTAL.
The CQ bit was checked by dskiba@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...
lgtm
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 dskiba@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from torne@chromium.org, tedchoc@chromium.org Link to the patchset: https://codereview.chromium.org/2847263003/#ps120001 (title: "Revert debugging")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1494265608355740,
"parent_rev": "2fc984150a60b860103aa4d2f67d9c1686d340da", "commit_rev":
"d50d8466d0bdae0b5ae8af4108ecaa1376ed5f21"}
Message was sent while issue was closed.
Description was changed from ========== Add tests to make sure closed CCT activities are not leaked. BUG=715724, 710612 ========== to ========== Add tests to make sure closed CCT activities are not leaked. BUG=715724, 710612 Review-Url: https://codereview.chromium.org/2847263003 Cr-Commit-Position: refs/heads/master@{#470057} Committed: https://chromium.googlesource.com/chromium/src/+/d50d8466d0bdae0b5ae8af4108ec... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/d50d8466d0bdae0b5ae8af4108ec...
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:120001) has been created in https://codereview.chromium.org/2866213003/ by olka@chromium.org. The reason for reverting is: org.chromium.chrome.browser.customtabs.CustomTabActivityTest#testClosedBackActivitiesNotLeaked flaky on chromium.linux/Android Tests See Issue 719909. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
