Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2810373002:
InstalledAppProviderImpl: Assert that code is run off the UI thread. (Closed)
|
Created:
3 years, 8 months ago by Matt Giuca Modified:
3 years, 7 months ago CC:
chromium-reviews, jam, darin-cc_chromium.org, agrieve+watch_chromium.org, chrome-apps-syd-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionInstalledAppProviderImpl: Assert that code is run off the UI thread.
Adds methods to ThreadUtils to support this: a new
assertOnBackgroundThread to complement assertOnUiThread, as well as a
way to disable these checks for testing (since the tests are
single-threaded).
BUG=710745
Review-Url: https://codereview.chromium.org/2810373002
Cr-Commit-Position: refs/heads/master@{#470786}
Committed: https://chromium.googlesource.com/chromium/src/+/25a9390dce3a08a787c852a3d381322767dace3c
Patch Set 1 #Patch Set 2 : Rebase. #Patch Set 3 : Added ThreadUtils methods for assertions. Use that instead of Mockito UI thread shenanigans. #Patch Set 4 : Use real assert instead of exception (apparently this works now). #
Total comments: 6
Patch Set 5 : Switch setThreadAssertsEnabled to Disabled (for default initialization). #Patch Set 6 : Renamed to 'ForTesting' and removed @VisibleForTesting. #Patch Set 7 : Rebase. #Patch Set 8 : Fix spelling of UI. #
Messages
Total messages: 30 (17 generated)
mgiuca@chromium.org changed reviewers: + tedchoc@chromium.org
Follow-up to discussion on previous CL: https://codereview.chromium.org/2813153002/#msg14 I've cargo-culted the implementation from VariationsSeedFetcherTest.java that you pointed me to. This doesn't feel "great" to me; unlike VSFT which is running code *entirely* on a background thread (so it's genuine to have the test pretend to be on a different thread), the test here is a bit deceptive. Because Robolectric runs all the code on one thread, we basically have to have the test either pretend to be on the UI thread, or off it. In this CL, I make it pretend to be off the UI thread, but that means the code in filterInstalledApps is "not on the UI thread" either. This would prevent me from (legitimately) adding an "assert ThreadUtils.runningOnUiThread();" in that method. So, yeah this doesn't feel exactly right, but I'm happy to land it if you think it's better to have that assert than not have it.
The CQ bit was checked by mgiuca@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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
On 2017/04/13 03:08:09, Matt Giuca wrote: > Follow-up to discussion on previous CL: > https://codereview.chromium.org/2813153002/#msg14 > > I've cargo-culted the implementation from VariationsSeedFetcherTest.java that > you pointed me to. > > This doesn't feel "great" to me; unlike VSFT which is running code *entirely* on > a background thread (so it's genuine to have the test pretend to be on a > different thread), the test here is a bit deceptive. > > Because Robolectric runs all the code on one thread, we basically have to have > the test either pretend to be on the UI thread, or off it. In this CL, I make it > pretend to be off the UI thread, but that means the code in filterInstalledApps > is "not on the UI thread" either. This would prevent me from (legitimately) > adding an "assert ThreadUtils.runningOnUiThread();" in that method. > > So, yeah this doesn't feel exactly right, but I'm happy to land it if you think > it's better to have that assert than not have it. Hmm...yeah that is less than ideal. Maybe a "better" fix would be to add a new method to ThreadUtils like assertOnBackgroundThread to not have to overload assertOnUiThread. Then we could just add a static method to ThreadUtils like disableThreadAssertsForTest which would allow us to have all the asserts we'd like in code and the ability to disable them for junit tests. Also if we go down this path, we can just use assert !runningOnUiThread() : "Must be called on background thread"; instead of the exception throwing. agrieve made asserts work on all android versions so that weird exception thing I added can be simplified. Does this seem more tenable?
Description was changed from ========== InstalledAppProviderImpl: Assert that code is run off the UI thread. Updated tests to pretend to be off the UI thread, so that the assert doesn't fail on the tests. BUG=710745 ========== to ========== InstalledAppProviderImpl: Assert that code is run off the UI thread. Adds methods to ThreadUtils to support this: a new assertOnBackgroundThread to complement assertOnUiThread, as well as a way to disable these checks for testing (since the tests are single-threaded). BUG=710745 ==========
mgiuca@chromium.org changed reviewers: + agrieve@chromium.org
agrieve@chromium.org: Please review changes in ThreadUtils.java. Note: Ted told me (in #7) that you've made asserts work everywhere now, so we can replace the if-then-throw logic with an assert. If this isn't correct, or if you think we should do this in a separate CL, we can do Patch Set 3 which keeps the existing logic.
lgtm https://codereview.chromium.org/2810373002/diff/60001/base/android/java/src/o... File base/android/java/src/org/chromium/base/ThreadUtils.java (right): https://codereview.chromium.org/2810373002/diff/60001/base/android/java/src/o... base/android/java/src/org/chromium/base/ThreadUtils.java:229: public static void setThreadAssertsEnabled(boolean enabled) { I would add "ForTesting" to the end of the name to make sure this doesn't leak into prod code usage.
lgtm /w nits. Asserts work fine now across all API levels, but they are still stripped in release mode. https://codereview.chromium.org/2810373002/diff/60001/base/android/java/src/o... File base/android/java/src/org/chromium/base/ThreadUtils.java (right): https://codereview.chromium.org/2810373002/diff/60001/base/android/java/src/o... base/android/java/src/org/chromium/base/ThreadUtils.java:28: private static boolean sThreadAssertsEnabled = true; nit: Generally better to have things default-initialized if possible, since it requires less code to run on start-up. E.g. could just call this sThreadAssertsDisabled, then wouldn't need to be initialized. https://codereview.chromium.org/2810373002/diff/60001/base/android/java/src/o... base/android/java/src/org/chromium/base/ThreadUtils.java:229: public static void setThreadAssertsEnabled(boolean enabled) { On 2017/05/05 14:09:39, Ted C wrote: > I would add "ForTesting" to the end of the name to make sure this doesn't leak > into prod code usage. Along the same lines, probably would drop "VisibleForTesting", since the visibility has not been altered for tests (it exists only for tests, which is what changing the name will convey)
https://codereview.chromium.org/2810373002/diff/60001/base/android/java/src/o... File base/android/java/src/org/chromium/base/ThreadUtils.java (right): https://codereview.chromium.org/2810373002/diff/60001/base/android/java/src/o... base/android/java/src/org/chromium/base/ThreadUtils.java:28: private static boolean sThreadAssertsEnabled = true; On 2017/05/05 15:00:48, agrieve wrote: > nit: Generally better to have things default-initialized if possible, since it > requires less code to run on start-up. E.g. could just call this > sThreadAssertsDisabled, then wouldn't need to be initialized. Done. https://codereview.chromium.org/2810373002/diff/60001/base/android/java/src/o... base/android/java/src/org/chromium/base/ThreadUtils.java:229: public static void setThreadAssertsEnabled(boolean enabled) { I'm confused about both of these suggestions... you're the experts on Java-in-Chrome so I'll defer to you, but my understanding was that in Java we use @VisibleForTesting as a more formalized alternative to a "ForTesting" name suffix. On 2017/05/05 15:00:49, agrieve wrote: > On 2017/05/05 14:09:39, Ted C wrote: > > I would add "ForTesting" to the end of the name to make sure this doesn't leak > > into prod code usage. > > Along the same lines, probably would drop "VisibleForTesting", since the > visibility has not been altered for tests (it exists only for tests, which is > what changing the name will convey) Are you suggesting that if a method is used *both* internally as well as being exposed publicly for testing, it should have @VisibleForTesting, but if a method is *only* exposed for testing (and isn't otherwise used internally), it should have a "ForTesting" suffix instead, and not the annotation? That seems like a pretty arbitrary distinction; from an external perspective, it doesn't matter whether it's used internally or not, and surely we should have a consistent way to convey something shouldn't be used other than by tests? Also, there are three other methods in this class (setUiThread, runOnUiThreadBlockingNoException and postOnUiThreadDelayed) which are annotated @VisibleForTesting but aren't used internally. I'm changing it but please confirm this is OK because it seems wrong to me.
The CQ bit was checked by mgiuca@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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
https://codereview.chromium.org/2810373002/diff/60001/base/android/java/src/o... File base/android/java/src/org/chromium/base/ThreadUtils.java (right): https://codereview.chromium.org/2810373002/diff/60001/base/android/java/src/o... base/android/java/src/org/chromium/base/ThreadUtils.java:229: public static void setThreadAssertsEnabled(boolean enabled) { On 2017/05/08 01:18:10, Matt Giuca wrote: > I'm confused about both of these suggestions... you're the experts on > Java-in-Chrome so I'll defer to you, but my understanding was that in Java we > use @VisibleForTesting as a more formalized alternative to a "ForTesting" name > suffix. > > On 2017/05/05 15:00:49, agrieve wrote: > > On 2017/05/05 14:09:39, Ted C wrote: > > > I would add "ForTesting" to the end of the name to make sure this doesn't > leak > > > into prod code usage. > > > > Along the same lines, probably would drop "VisibleForTesting", since the > > visibility has not been altered for tests (it exists only for tests, which is > > what changing the name will convey) > > Are you suggesting that if a method is used *both* internally as well as being > exposed publicly for testing, it should have @VisibleForTesting, but if a method > is *only* exposed for testing (and isn't otherwise used internally), it should > have a "ForTesting" suffix instead, and not the annotation? > > That seems like a pretty arbitrary distinction; from an external perspective, it > doesn't matter whether it's used internally or not, and surely we should have a > consistent way to convey something shouldn't be used other than by tests? > > Also, there are three other methods in this class (setUiThread, > runOnUiThreadBlockingNoException and postOnUiThreadDelayed) which are annotated > @VisibleForTesting but aren't used internally. I'm changing it but please > confirm this is OK because it seems wrong to me. Consistency is certainly not our strong suite. To me, seeing @VisibleForTesting does not answer whether the function is used outside of tests, but the name suffix makes it crystal clear. Thinking about this change again though... Is there a reason the tests can't just run on the UI thread? It's generally better to have tests match reality as much as possible. E.g. just call ThreadUtils.runOnUiThreadBlocking() within the tests.
On 2017/05/08 14:18:15, agrieve wrote: > Consistency is certainly not our strong suite. To me, seeing @VisibleForTesting > does not answer whether the function is used outside of tests, but the name > suffix makes it crystal clear. > > Thinking about this change again though... Is there a reason the tests can't > just run on the UI thread? It's generally better to have tests match reality as > much as possible. E.g. just call ThreadUtils.runOnUiThreadBlocking() within the > tests. It's a Robolectric test, and Robolectric fakes out the whole threading system. The tests *are* running on the UI thread. The problem is that the code under test calls AsyncTask.executeOnExecutor(THREAD_POOL_EXECUTOR) then asserts that it runs off the UI thread. In production, some of this code runs on the UI thread and some runs on the background thread. In the test, Robolectric hooks AsyncTask to make it a dummy operation and the executeOnExecutor code runs on the UI thread as well. That's why we have to disable the thread checks; since ThreadUtils.assertOnBackgroundThread() just checks that it's not on the UI thread, that would fail in Robolectric.
On 2017/05/09 00:26:29, Matt Giuca wrote: > On 2017/05/08 14:18:15, agrieve wrote: > > Consistency is certainly not our strong suite. To me, seeing > @VisibleForTesting > > does not answer whether the function is used outside of tests, but the name > > suffix makes it crystal clear. > > > > Thinking about this change again though... Is there a reason the tests can't > > just run on the UI thread? It's generally better to have tests match reality > as > > much as possible. E.g. just call ThreadUtils.runOnUiThreadBlocking() within > the > > tests. > > It's a Robolectric test, and Robolectric fakes out the whole threading system. > > The tests *are* running on the UI thread. The problem is that the code under > test calls AsyncTask.executeOnExecutor(THREAD_POOL_EXECUTOR) then asserts that > it runs off the UI thread. In production, some of this code runs on the UI > thread and some runs on the background thread. In the test, Robolectric hooks > AsyncTask to make it a dummy operation and the executeOnExecutor code runs on > the UI thread as well. That's why we have to disable the thread checks; since > ThreadUtils.assertOnBackgroundThread() just checks that it's not on the UI > thread, that would fail in Robolectric. Wow, crazy business! Just searched for how this works, and wonder if it's actually just a case of using: https://cs.chromium.org/chromium/src/testing/android/junit/java/src/org/chrom... rather than: CustomShadowAsyncTask.java (I don't see why we have both tbh)
The CQ bit was checked by mgiuca@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 2017/05/09 01:45:52, agrieve wrote: > On 2017/05/09 00:26:29, Matt Giuca wrote: > > On 2017/05/08 14:18:15, agrieve wrote: > > > Consistency is certainly not our strong suite. To me, seeing > > @VisibleForTesting > > > does not answer whether the function is used outside of tests, but the name > > > suffix makes it crystal clear. > > > > > > Thinking about this change again though... Is there a reason the tests can't > > > just run on the UI thread? It's generally better to have tests match reality > > as > > > much as possible. E.g. just call ThreadUtils.runOnUiThreadBlocking() within > > the > > > tests. > > > > It's a Robolectric test, and Robolectric fakes out the whole threading system. > > > > The tests *are* running on the UI thread. The problem is that the code under > > test calls AsyncTask.executeOnExecutor(THREAD_POOL_EXECUTOR) then asserts that > > it runs off the UI thread. In production, some of this code runs on the UI > > thread and some runs on the background thread. In the test, Robolectric hooks > > AsyncTask to make it a dummy operation and the executeOnExecutor code runs on > > the UI thread as well. That's why we have to disable the thread checks; since > > ThreadUtils.assertOnBackgroundThread() just checks that it's not on the UI > > thread, that would fail in Robolectric. > > Wow, crazy business! Just searched for how this works, and wonder if it's > actually just a case of using: > https://cs.chromium.org/chromium/src/testing/android/junit/java/src/org/chrom... > > rather than: CustomShadowAsyncTask.java > > (I don't see why we have both tbh) I just subbed in BackgroundShadowAsyncTask for CustomShadowAsyncTask, and all the tests crashed in the background thread with NullPointerException... in many different ways depending on the tests. For starters, mContext.getPackageManager() is returning null when called on the background thread. Other tests are just crashing in ShadowAsyncTask itself. I don't really know what's going on, but I also don't have time to dig into this. It isn't necessary for these unit tests to run multi-threaded. They are testing the behaviour of the code. Note there was a small merge conflict in ThreadUtils: Ted added a new method checkUiThread, so I have also made that a no-op if thread asserts are disabled by the tests. It's a bit weird now that the assert methods use assert, but the check method still uses an if statement + exception (because asserts are disabled in Release builds). Should be the correct behaviour though.
The CQ bit was checked by mgiuca@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org, agrieve@chromium.org Link to the patchset: https://codereview.chromium.org/2810373002/#ps140001 (title: "Fix spelling of UI.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/05/11 01:21:05, commit-bot: I haz the power wrote: > CQ is trying da patch. > > Follow status at: > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... Sounds good. Thanks for spending as much extra time on it as you did!
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1494465566686510,
"parent_rev": "1b2f92e7444305d3132176edc5709b0805c9847a", "commit_rev":
"25a9390dce3a08a787c852a3d381322767dace3c"}
Message was sent while issue was closed.
Description was changed from ========== InstalledAppProviderImpl: Assert that code is run off the UI thread. Adds methods to ThreadUtils to support this: a new assertOnBackgroundThread to complement assertOnUiThread, as well as a way to disable these checks for testing (since the tests are single-threaded). BUG=710745 ========== to ========== InstalledAppProviderImpl: Assert that code is run off the UI thread. Adds methods to ThreadUtils to support this: a new assertOnBackgroundThread to complement assertOnUiThread, as well as a way to disable these checks for testing (since the tests are single-threaded). BUG=710745 Review-Url: https://codereview.chromium.org/2810373002 Cr-Commit-Position: refs/heads/master@{#470786} Committed: https://chromium.googlesource.com/chromium/src/+/25a9390dce3a08a787c852a3d381... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/25a9390dce3a08a787c852a3d381... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
