Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2201783003:
Add test to ensure shouldOverrideUrlLoading throws Java exception (Closed)
DescriptionAdd test to ensure shouldOverrideUrlLoading throws Java exception
[Can't land before https://codereview.chromium.org/2169553002/ - the
added test won't pass]
Add test that creates a WebView in a service in a separate process and
makes the its shouldOverrideUrlLoading callback throw a run-time
exception. Then check that the uncaughtExceptionHandler for the UI
thread of the service sees this exception.
Refactoring some instrumentation test utility files so that they can be
reached from the webview shell component (where the service is defined).
BUG=592556
Patch Set 1 #
Total comments: 1
Patch Set 2 : Reupload PS1 with similarity 40% to show JSUtils.java as moved file. #
Total comments: 34
Patch Set 3 : Simplify navigation logic, revert some utility code changes. #Patch Set 4 : Fix some comments. #
Total comments: 26
Patch Set 5 : Add testbase for separate-service tests, use this from AwSecondBrowserTest. #
Total comments: 31
Messages
Total messages: 33 (2 generated)
gsennton@chromium.org changed reviewers: + boliu@chromium.org, torne@chromium.org
Heheyyy, this is an instrumentation test that starts a service in a separate process and then throws a Java exception in ShouldOverrideUrlLoading. The service registers an unCaughtExceptionHandler on its UI thread and when that handler is invoked it then signals the original test to pass. The test works fine locally with the patch in https://codereview.chromium.org/2169553002/ (and crashes without that CL, as it should). Does this look sane?
On 2016/08/01 17:52:43, gsennton wrote: > Heheyyy, this is an instrumentation test that starts a service in a separate > process and then throws a Java exception in ShouldOverrideUrlLoading. The > service registers an unCaughtExceptionHandler on its UI thread and when that > handler is invoked it then signals the original test to pass. > > The test works fine locally with the patch in > https://codereview.chromium.org/2169553002/ > (and crashes without that CL, as it should). > > Does this look sane? (added both Torne and Bo as reviewers since Bo was involved in my description of the approach to this test in the corresponding bug).
Looks okay in general but at least one of the moved files you haven't changed the package declaration even though you changed its path.. https://codereview.chromium.org/2201783003/diff/1/android_webview/javatests/s... File android_webview/javatests/src/org/chromium/android_webview/test/JniCrashTest.java (right): https://codereview.chromium.org/2201783003/diff/1/android_webview/javatests/s... android_webview/javatests/src/org/chromium/android_webview/test/JniCrashTest.java:78: @Feature({"AndroidWebView", "Navigation"}) // TODO what does this Feature stuff mean? I think you can use it to filter which tests to run, but it doesn't appear to be applied very consistently and so may not actually be useful :)
started reading line by line, and then realized there are still lots of TODOs :/ https://codereview.chromium.org/2201783003/diff/20001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/JniCrashTest.java (right): https://codereview.chromium.org/2201783003/diff/20001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/JniCrashTest.java:67: private void sendMessengerToService(Messenger serviceMessenger, Messenger messenger) { can this be part of MyServiceConnection? then MyServiceConnection can be static? https://codereview.chromium.org/2201783003/diff/20001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/JniCrashTest.java:101: assertNotNull(getActivity().startService(intent)); not super familiar with services, is there something we listen to for service crash? if the reply does not arrive first, that would be an explicit failure case instead of waiting for this to timeout? https://codereview.chromium.org/2201783003/diff/20001/android_webview/test/BU... File android_webview/test/BUILD.gn (right): https://codereview.chromium.org/2201783003/diff/20001/android_webview/test/BU... android_webview/test/BUILD.gn:210: android_library("android_webview_java_test_utils") { maybe leave a comment why this is needed? https://codereview.chromium.org/2201783003/diff/20001/android_webview/test/sh... File android_webview/test/shell/src/org/chromium/android_webview/test/SecondBrowserProcess.java (right): https://codereview.chromium.org/2201783003/diff/20001/android_webview/test/sh... android_webview/test/shell/src/org/chromium/android_webview/test/SecondBrowserProcess.java:57: static void startBrowserProcess(Context context) throws Exception { uhh, move this somewhere that appears more "shared", another file? actually, one service per test sucks, can we re-use the same service (give it a more generic name), and just work out what to do depending on what message is received? https://codereview.chromium.org/2201783003/diff/20001/android_webview/test/sh... File android_webview/test/shell/src/org/chromium/android_webview/test/SeparateProcessWebViewService.java (right): https://codereview.chromium.org/2201783003/diff/20001/android_webview/test/sh... android_webview/test/shell/src/org/chromium/android_webview/test/SeparateProcessWebViewService.java:36: public static final int RECEIVER_MESSAGE_ID = 1; these message ID names seem way too generic, maybe put the name of the test in there? this one seems like a "START_TEST"? https://codereview.chromium.org/2201783003/diff/20001/android_webview/test/sh... android_webview/test/shell/src/org/chromium/android_webview/test/SeparateProcessWebViewService.java:37: public static final int RETURN_MESSAGE_ID = 42; this one seems like "success" message, ie we got the exception? https://codereview.chromium.org/2201783003/diff/20001/android_webview/test/sh... android_webview/test/shell/src/org/chromium/android_webview/test/SeparateProcessWebViewService.java:58: Log.e(TAG, "Caught exception in testShouldOverrideUrlLoadingCrash: " + e); pass e as third parameter? https://codereview.chromium.org/2201783003/diff/20001/android_webview/test/sh... android_webview/test/shell/src/org/chromium/android_webview/test/SeparateProcessWebViewService.java:75: Log.e(TAG, "in uncaughtException with throwable " + e this is not an error https://codereview.chromium.org/2201783003/diff/20001/android_webview/test/sh... android_webview/test/shell/src/org/chromium/android_webview/test/SeparateProcessWebViewService.java:160: JSUtils.clickOnLinkUsingJsFromNonUiThread(awContents, you can just post back to UI asynchronously? https://codereview.chromium.org/2201783003/diff/20001/android_webview/test/sh... File android_webview/test/shell/src/org/chromium/android_webview/test/util/JSUtils.java (right): https://codereview.chromium.org/2201783003/diff/20001/android_webview/test/sh... android_webview/test/shell/src/org/chromium/android_webview/test/util/JSUtils.java:81: public static void clickOnLinkUsingJsFromNonUiThread(final AwContents awContents, seems complicated, can you just navigate from js directly? https://codereview.chromium.org/2201783003/diff/20001/android_webview/test/sh... android_webview/test/shell/src/org/chromium/android_webview/test/util/JSUtils.java:118: public static String executeJavaScriptOnUiThreadAndWaitForResult(final AwContents awContents, this seems copied? reuse JavaScriptUtils. executeJavaScriptAndWaitForResult?
On 2016/08/02 20:16:00, boliu wrote: > started reading line by line, and then realized there are still lots of TODOs :/ > > https://codereview.chromium.org/2201783003/diff/20001/android_webview/javates... > File > android_webview/javatests/src/org/chromium/android_webview/test/JniCrashTest.java > (right): > > https://codereview.chromium.org/2201783003/diff/20001/android_webview/javates... > android_webview/javatests/src/org/chromium/android_webview/test/JniCrashTest.java:67: > private void sendMessengerToService(Messenger serviceMessenger, Messenger > messenger) { > can this be part of MyServiceConnection? then MyServiceConnection can be static? > > https://codereview.chromium.org/2201783003/diff/20001/android_webview/javates... > android_webview/javatests/src/org/chromium/android_webview/test/JniCrashTest.java:101: > assertNotNull(getActivity().startService(intent)); > not super familiar with services, is there something we listen to for service > crash? if the reply does not arrive first, that would be an explicit failure > case instead of waiting for this to timeout? > > https://codereview.chromium.org/2201783003/diff/20001/android_webview/test/BU... > File android_webview/test/BUILD.gn (right): > > https://codereview.chromium.org/2201783003/diff/20001/android_webview/test/BU... > android_webview/test/BUILD.gn:210: > android_library("android_webview_java_test_utils") { > maybe leave a comment why this is needed? > > https://codereview.chromium.org/2201783003/diff/20001/android_webview/test/sh... > File > android_webview/test/shell/src/org/chromium/android_webview/test/SecondBrowserProcess.java > (right): > > https://codereview.chromium.org/2201783003/diff/20001/android_webview/test/sh... > android_webview/test/shell/src/org/chromium/android_webview/test/SecondBrowserProcess.java:57: > static void startBrowserProcess(Context context) throws Exception { > uhh, move this somewhere that appears more "shared", another file? > > actually, one service per test sucks, can we re-use the same service (give it a > more generic name), and just work out what to do depending on what message is > received? > > https://codereview.chromium.org/2201783003/diff/20001/android_webview/test/sh... > File > android_webview/test/shell/src/org/chromium/android_webview/test/SeparateProcessWebViewService.java > (right): > > https://codereview.chromium.org/2201783003/diff/20001/android_webview/test/sh... > android_webview/test/shell/src/org/chromium/android_webview/test/SeparateProcessWebViewService.java:36: > public static final int RECEIVER_MESSAGE_ID = 1; > these message ID names seem way too generic, maybe put the name of the test in > there? > > this one seems like a "START_TEST"? > > https://codereview.chromium.org/2201783003/diff/20001/android_webview/test/sh... > android_webview/test/shell/src/org/chromium/android_webview/test/SeparateProcessWebViewService.java:37: > public static final int RETURN_MESSAGE_ID = 42; > this one seems like "success" message, ie we got the exception? > > https://codereview.chromium.org/2201783003/diff/20001/android_webview/test/sh... > android_webview/test/shell/src/org/chromium/android_webview/test/SeparateProcessWebViewService.java:58: > Log.e(TAG, "Caught exception in testShouldOverrideUrlLoadingCrash: " + e); > pass e as third parameter? > > https://codereview.chromium.org/2201783003/diff/20001/android_webview/test/sh... > android_webview/test/shell/src/org/chromium/android_webview/test/SeparateProcessWebViewService.java:75: > Log.e(TAG, "in uncaughtException with throwable " + e > this is not an error > > https://codereview.chromium.org/2201783003/diff/20001/android_webview/test/sh... > android_webview/test/shell/src/org/chromium/android_webview/test/SeparateProcessWebViewService.java:160: > JSUtils.clickOnLinkUsingJsFromNonUiThread(awContents, > you can just post back to UI asynchronously? > > https://codereview.chromium.org/2201783003/diff/20001/android_webview/test/sh... > File > android_webview/test/shell/src/org/chromium/android_webview/test/util/JSUtils.java > (right): > > https://codereview.chromium.org/2201783003/diff/20001/android_webview/test/sh... > android_webview/test/shell/src/org/chromium/android_webview/test/util/JSUtils.java:81: > public static void clickOnLinkUsingJsFromNonUiThread(final AwContents > awContents, > seems complicated, can you just navigate from js directly? > > https://codereview.chromium.org/2201783003/diff/20001/android_webview/test/sh... > android_webview/test/shell/src/org/chromium/android_webview/test/util/JSUtils.java:118: > public static String executeJavaScriptOnUiThreadAndWaitForResult(final > AwContents awContents, > this seems copied? reuse JavaScriptUtils. executeJavaScriptAndWaitForResult? Yeah, I figured it might need some work - that's one of the reasons I didn't want to put this in https://codereview.chromium.org/2169553002/ (at least until this is cleaned up).
https://codereview.chromium.org/2201783003/diff/20001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/JniCrashTest.java (right): https://codereview.chromium.org/2201783003/diff/20001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/JniCrashTest.java:67: private void sendMessengerToService(Messenger serviceMessenger, Messenger messenger) { On 2016/08/02 20:15:59, boliu wrote: > can this be part of MyServiceConnection? then MyServiceConnection can be static? True, will fix. https://codereview.chromium.org/2201783003/diff/20001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/JniCrashTest.java:101: assertNotNull(getActivity().startService(intent)); On 2016/08/02 20:15:59, boliu wrote: > not super familiar with services, is there something we listen to for service > crash? if the reply does not arrive first, that would be an explicit failure > case instead of waiting for this to timeout? There is the onServiceDisconnected callback above we could use to notify the test when we lose the connection to the service. https://codereview.chromium.org/2201783003/diff/20001/android_webview/test/sh... File android_webview/test/shell/src/org/chromium/android_webview/test/SecondBrowserProcess.java (right): https://codereview.chromium.org/2201783003/diff/20001/android_webview/test/sh... android_webview/test/shell/src/org/chromium/android_webview/test/SecondBrowserProcess.java:57: static void startBrowserProcess(Context context) throws Exception { On 2016/08/02 20:15:59, boliu wrote: > uhh, move this somewhere that appears more "shared", another file? > > actually, one service per test sucks, can we re-use the same service (give it a > more generic name), and just work out what to do depending on what message is > received? Sure, will do :) https://codereview.chromium.org/2201783003/diff/20001/android_webview/test/sh... File android_webview/test/shell/src/org/chromium/android_webview/test/SeparateProcessWebViewService.java (right): https://codereview.chromium.org/2201783003/diff/20001/android_webview/test/sh... android_webview/test/shell/src/org/chromium/android_webview/test/SeparateProcessWebViewService.java:58: Log.e(TAG, "Caught exception in testShouldOverrideUrlLoadingCrash: " + e); On 2016/08/02 20:16:00, boliu wrote: > pass e as third parameter? Will do https://codereview.chromium.org/2201783003/diff/20001/android_webview/test/sh... android_webview/test/shell/src/org/chromium/android_webview/test/SeparateProcessWebViewService.java:75: Log.e(TAG, "in uncaughtException with throwable " + e On 2016/08/02 20:16:00, boliu wrote: > this is not an error Ooops, just used it as debug logging. https://codereview.chromium.org/2201783003/diff/20001/android_webview/test/sh... File android_webview/test/shell/src/org/chromium/android_webview/test/util/JSUtils.java (right): https://codereview.chromium.org/2201783003/diff/20001/android_webview/test/sh... android_webview/test/shell/src/org/chromium/android_webview/test/util/JSUtils.java:81: public static void clickOnLinkUsingJsFromNonUiThread(final AwContents awContents, On 2016/08/02 20:16:00, boliu wrote: > seems complicated, can you just navigate from js directly? You mean rather than waiting for the link to show up and then clicking on that? https://codereview.chromium.org/2201783003/diff/20001/android_webview/test/sh... android_webview/test/shell/src/org/chromium/android_webview/test/util/JSUtils.java:118: public static String executeJavaScriptOnUiThreadAndWaitForResult(final AwContents awContents, On 2016/08/02 20:16:00, boliu wrote: > this seems copied? reuse JavaScriptUtils. executeJavaScriptAndWaitForResult? Yeah, both of these methods are just copied from the existing ones in this class. The difference is that we are not running an InstrumentationTestCase inside the Service so I'm not using functionality like testCase.getInstrumentation().runOnMainSync(...) in these new methods. It does indeed look like this method can just be replaced with JavaScriptUtils.executeJavaScriptAndWaitForResult, thanks!
https://codereview.chromium.org/2201783003/diff/20001/android_webview/test/sh... File android_webview/test/shell/src/org/chromium/android_webview/test/util/JSUtils.java (right): https://codereview.chromium.org/2201783003/diff/20001/android_webview/test/sh... android_webview/test/shell/src/org/chromium/android_webview/test/util/JSUtils.java:81: public static void clickOnLinkUsingJsFromNonUiThread(final AwContents awContents, On 2016/08/02 20:33:57, gsennton wrote: > On 2016/08/02 20:16:00, boliu wrote: > > seems complicated, can you just navigate from js directly? > > You mean rather than waiting for the link to show up and then clicking on that? Yeah, then you don't even need to load a real page, just load about:blank
https://codereview.chromium.org/2201783003/diff/20001/android_webview/test/BU... File android_webview/test/BUILD.gn (right): https://codereview.chromium.org/2201783003/diff/20001/android_webview/test/BU... android_webview/test/BUILD.gn:210: android_library("android_webview_java_test_utils") { On 2016/08/02 20:15:59, boliu wrote: > maybe leave a comment why this is needed? Oooh, right, I have just been assuming we didn't want the shell/ directory to depend on javatests/. I was meaning to ask you: do we have any kind of logic as to which classes live under shell/src/org/chromium/android_webview/test/util/ and which classes live under javatests/src/org/chromium/android_webview/test/ ? (especially considering utility classes).
https://codereview.chromium.org/2201783003/diff/20001/android_webview/test/BU... File android_webview/test/BUILD.gn (right): https://codereview.chromium.org/2201783003/diff/20001/android_webview/test/BU... android_webview/test/BUILD.gn:210: android_library("android_webview_java_test_utils") { On 2016/08/02 20:55:15, gsennton wrote: > On 2016/08/02 20:15:59, boliu wrote: > > maybe leave a comment why this is needed? > > Oooh, right, I have just been assuming we didn't want the shell/ directory to > depend on javatests/. > I was meaning to ask you: > do we have any kind of logic as to which classes live under > shell/src/org/chromium/android_webview/test/util/ > and which classes live under > javatests/src/org/chromium/android_webview/test/ > ? > > (especially considering utility classes). Layering is android_webview_test_apk (javatests) can depend on android_webview_apk (shell), but not the other way around. So if something is needed in the shell, like in your case, you have to move it to shell. I think what happens here is that this target will be linked to both the shell apk and the test apk, right?
On 2016/08/03 05:12:37, boliu wrote: > https://codereview.chromium.org/2201783003/diff/20001/android_webview/test/BU... > File android_webview/test/BUILD.gn (right): > > https://codereview.chromium.org/2201783003/diff/20001/android_webview/test/BU... > android_webview/test/BUILD.gn:210: > android_library("android_webview_java_test_utils") { > On 2016/08/02 20:55:15, gsennton wrote: > > On 2016/08/02 20:15:59, boliu wrote: > > > maybe leave a comment why this is needed? > > > > Oooh, right, I have just been assuming we didn't want the shell/ directory to > > depend on javatests/. > > I was meaning to ask you: > > do we have any kind of logic as to which classes live under > > shell/src/org/chromium/android_webview/test/util/ > > and which classes live under > > javatests/src/org/chromium/android_webview/test/ > > ? > > > > (especially considering utility classes). > > Layering is android_webview_test_apk (javatests) can depend on > android_webview_apk (shell), but not the other way around. So if something is > needed in the shell, like in your case, you have to move it to shell. > > I think what happens here is that this target will be linked to both the shell > apk and the test apk, right? Yepp, we could just move these utility methods into the shell apk instead (it just seemed clearer to have them in their own target given that they don't really depend on other stuff within the shell apk). Is there a specific rule for what exactly should go into the shell apk? E.g. putting SecondBrowserProcess.java (and SeparateProcessWebViewService.java) there seems a bit arbitrary.
On 2016/08/03 08:40:51, gsennton_OOO_back_8.15 wrote: > On 2016/08/03 05:12:37, boliu wrote: > > > https://codereview.chromium.org/2201783003/diff/20001/android_webview/test/BU... > > File android_webview/test/BUILD.gn (right): > > > > > https://codereview.chromium.org/2201783003/diff/20001/android_webview/test/BU... > > android_webview/test/BUILD.gn:210: > > android_library("android_webview_java_test_utils") { > > On 2016/08/02 20:55:15, gsennton wrote: > > > On 2016/08/02 20:15:59, boliu wrote: > > > > maybe leave a comment why this is needed? > > > > > > Oooh, right, I have just been assuming we didn't want the shell/ directory > to > > > depend on javatests/. > > > I was meaning to ask you: > > > do we have any kind of logic as to which classes live under > > > shell/src/org/chromium/android_webview/test/util/ > > > and which classes live under > > > javatests/src/org/chromium/android_webview/test/ > > > ? > > > > > > (especially considering utility classes). > > > > Layering is android_webview_test_apk (javatests) can depend on > > android_webview_apk (shell), but not the other way around. So if something is > > needed in the shell, like in your case, you have to move it to shell. > > > > I think what happens here is that this target will be linked to both the shell > > apk and the test apk, right? > > Yepp, we could just move these utility methods into the shell apk instead (it > just seemed clearer to have them in their own target given that they don't > really depend on other stuff within the shell apk). Is there a specific rule for > what exactly should go into the shell apk? I don't anyone gave any deep thought into that. I think general rule should be if something can live in the test apk, then it should live there (general principle of having code living in the higher layer, if it's only needed in higher layer) > E.g. putting SecondBrowserProcess.java (and SeparateProcessWebViewService.java) > there seems a bit arbitrary. Are you sure they are ok living in the test apk? If yes, we should do that. But I suspect it's actually not ok?
> > Are you sure they are ok living in the test apk? If yes, we should do that. But > I suspect it's actually not ok? Ah, right, I think we need to declare the Services in the AndroidManifest of the apk we are instrumenting so that's why SecondBrowserProcess needs to live in the shell apk.
Question: how does the multi-process flag work? (how are we passing the flag to WebView / the shell?) - some of the test annotations we use won't be applied to a WebView started in a separate service since that service doesn't use our instrumentation test runner. https://codereview.chromium.org/2201783003/diff/20001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/JniCrashTest.java (right): https://codereview.chromium.org/2201783003/diff/20001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/JniCrashTest.java:67: private void sendMessengerToService(Messenger serviceMessenger, Messenger messenger) { On 2016/08/02 20:33:57, gsennton wrote: > On 2016/08/02 20:15:59, boliu wrote: > > can this be part of MyServiceConnection? then MyServiceConnection can be > static? > > True, will fix. Done. https://codereview.chromium.org/2201783003/diff/20001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/JniCrashTest.java:101: assertNotNull(getActivity().startService(intent)); On 2016/08/02 20:33:57, gsennton wrote: > On 2016/08/02 20:15:59, boliu wrote: > > not super familiar with services, is there something we listen to for service > > crash? if the reply does not arrive first, that would be an explicit failure > > case instead of waiting for this to timeout? > > There is the onServiceDisconnected callback above we could use to notify the > test when we lose the connection to the service. Though I think the onServiceDisconnected callback would just mean that the service got killed - it might still be restarted again to complete the test. Now I'm returning a message from the service telling us that we failed. But that only covers the case when the service throws a java exception - not the other cases where the service can crash. https://codereview.chromium.org/2201783003/diff/20001/android_webview/test/BU... File android_webview/test/BUILD.gn (right): https://codereview.chromium.org/2201783003/diff/20001/android_webview/test/BU... android_webview/test/BUILD.gn:210: android_library("android_webview_java_test_utils") { On 2016/08/03 05:12:37, boliu wrote: > On 2016/08/02 20:55:15, gsennton wrote: > > On 2016/08/02 20:15:59, boliu wrote: > > > maybe leave a comment why this is needed? > > > > Oooh, right, I have just been assuming we didn't want the shell/ directory to > > depend on javatests/. > > I was meaning to ask you: > > do we have any kind of logic as to which classes live under > > shell/src/org/chromium/android_webview/test/util/ > > and which classes live under > > javatests/src/org/chromium/android_webview/test/ > > ? > > > > (especially considering utility classes). > > Layering is android_webview_test_apk (javatests) can depend on > android_webview_apk (shell), but not the other way around. So if something is > needed in the shell, like in your case, you have to move it to shell. > > I think what happens here is that this target will be linked to both the shell > apk and the test apk, right? Yeah, I've just put these in the shell apk instead now. https://codereview.chromium.org/2201783003/diff/20001/android_webview/test/sh... File android_webview/test/shell/src/org/chromium/android_webview/test/SecondBrowserProcess.java (right): https://codereview.chromium.org/2201783003/diff/20001/android_webview/test/sh... android_webview/test/shell/src/org/chromium/android_webview/test/SecondBrowserProcess.java:57: static void startBrowserProcess(Context context) throws Exception { On 2016/08/02 20:33:57, gsennton wrote: > On 2016/08/02 20:15:59, boliu wrote: > > uhh, move this somewhere that appears more "shared", another file? > > > > actually, one service per test sucks, can we re-use the same service (give it > a > > more generic name), and just work out what to do depending on what message is > > received? > > Sure, will do :) Actually, IMO we shouldn't merge these services because SecondBrowserProcess is simpler than SeparateProcessWebViewService - the latter needs to communicate with the corresponding test long after the initial setup-communication is done (i.e. the communication needs to happen in the uncaughtExceptionHandler) while SecondBrowserProcess can just run the code it wants to test and then return the result directly. So to communicate in the new Service I'm using Messengers while SecondBrowserProcess just uses the onTransact method. SecondBrowserProcess is easier to read/interpret as is IMO (actually, the primary reason I don't want to merge the services is because then I would have to re-run/fix the tests that use SecondBrowserProcess to ensure they work correctly). Does this make sense? (merging them shouldn't be too difficult, it just seems like extra work for very little gain). If we don't merge them I'll move the startBrowserProcess method somewhere else. https://codereview.chromium.org/2201783003/diff/20001/android_webview/test/sh... File android_webview/test/shell/src/org/chromium/android_webview/test/SeparateProcessWebViewService.java (right): https://codereview.chromium.org/2201783003/diff/20001/android_webview/test/sh... android_webview/test/shell/src/org/chromium/android_webview/test/SeparateProcessWebViewService.java:36: public static final int RECEIVER_MESSAGE_ID = 1; On 2016/08/02 20:15:59, boliu wrote: > these message ID names seem way too generic, maybe put the name of the test in > there? > > this one seems like a "START_TEST"? Done. https://codereview.chromium.org/2201783003/diff/20001/android_webview/test/sh... android_webview/test/shell/src/org/chromium/android_webview/test/SeparateProcessWebViewService.java:37: public static final int RETURN_MESSAGE_ID = 42; On 2016/08/02 20:16:00, boliu wrote: > this one seems like "success" message, ie we got the exception? Done. https://codereview.chromium.org/2201783003/diff/20001/android_webview/test/sh... android_webview/test/shell/src/org/chromium/android_webview/test/SeparateProcessWebViewService.java:58: Log.e(TAG, "Caught exception in testShouldOverrideUrlLoadingCrash: " + e); On 2016/08/02 20:33:57, gsennton wrote: > On 2016/08/02 20:16:00, boliu wrote: > > pass e as third parameter? > > Will do Done. https://codereview.chromium.org/2201783003/diff/20001/android_webview/test/sh... android_webview/test/shell/src/org/chromium/android_webview/test/SeparateProcessWebViewService.java:160: JSUtils.clickOnLinkUsingJsFromNonUiThread(awContents, On 2016/08/02 20:16:00, boliu wrote: > you can just post back to UI asynchronously? Yeah, this code isn't needed either when just navigating through JS >.< https://codereview.chromium.org/2201783003/diff/20001/android_webview/test/sh... File android_webview/test/shell/src/org/chromium/android_webview/test/util/JSUtils.java (right): https://codereview.chromium.org/2201783003/diff/20001/android_webview/test/sh... android_webview/test/shell/src/org/chromium/android_webview/test/util/JSUtils.java:81: public static void clickOnLinkUsingJsFromNonUiThread(final AwContents awContents, On 2016/08/02 20:33:57, gsennton wrote: > On 2016/08/02 20:16:00, boliu wrote: > > seems complicated, can you just navigate from js directly? > > You mean rather than waiting for the link to show up and then clicking on that? Huh, I just assumed I needed to click a link to trigger shouldOverrideUrlLoading - didn't think about just navigating directly through JS.... this is SO much simpler now. Don't even need the JSUtils class :D https://codereview.chromium.org/2201783003/diff/60001/android_webview/test/sh... File android_webview/test/shell/src/org/chromium/android_webview/test/SeparateProcessWebViewService.java (right): https://codereview.chromium.org/2201783003/diff/60001/android_webview/test/sh... android_webview/test/shell/src/org/chromium/android_webview/test/SeparateProcessWebViewService.java:117: private AwTestContainerView createDetachedAwTestContainerView( This method is essentially a copy-paste from AwTestBase - I don't really like this, but on the other hand it's a bit fiddly to put more AwTestBase logic in the shell-apk. Do we care about the copy-pasting here?
On 2016/09/06 15:26:08, gsennton wrote: > Question: how does the multi-process flag work? (how are we passing the flag to > WebView / the shell?) - some of the test annotations we use won't be applied to > a WebView started in a separate service since that service doesn't use our > instrumentation test runner. For production webview, we just add the command line on start up: https://cs.chromium.org/chromium/src/android_webview/glue/java/src/com/androi... I think for the test annotations, the test script actually reads the annotation, do the appropriate thing (in this case add command line switch), and then start the test. If you add the annotation on the browser side of the test, it should just work, since it's just a file on disk *I think*.
https://codereview.chromium.org/2201783003/diff/60001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/JniCrashTest.java (right): https://codereview.chromium.org/2201783003/diff/60001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/JniCrashTest.java:31: private AwTestContainerView mTestContainerView; these all appear unused https://codereview.chromium.org/2201783003/diff/60001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/JniCrashTest.java:84: ThreadUtils.getUiThreadLooper(), new Handler.Callback() { ui looper sounds like a worse option though, because then you risk triggering ANRs, use a background thread https://codereview.chromium.org/2201783003/diff/60001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/JniCrashTest.java:93: return true; why would you not notify the latch here? are you expecting the instrumentation thread to wait forever here? how does the test handler handle that? https://codereview.chromium.org/2201783003/diff/60001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/JniCrashTest.java:95: return false; can this ever happen? ban it if not https://codereview.chromium.org/2201783003/diff/60001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/JniCrashTest.java:112: AwTestBase.WAIT_TIMEOUT_MS, TimeUnit.MILLISECONDS)); this is bound to be flaky if you are relying on a timeout. waiting can't be avoided here, but should minimize the cases where timeout happens by design what if RETURN_JNI_CRASH_TEST_FAILED is received? what if service crashes unexpectedly? at least both of those cases can be notified without timeout https://codereview.chromium.org/2201783003/diff/60001/android_webview/test/sh... File android_webview/test/shell/src/org/chromium/android_webview/test/SecondBrowserProcess.java (right): https://codereview.chromium.org/2201783003/diff/60001/android_webview/test/sh... android_webview/test/shell/src/org/chromium/android_webview/test/SecondBrowserProcess.java:57: static void startBrowserProcess(Context context) throws Exception { even if you are not merging the two services, this is still not ok because the dependency makes no sense. factor it out into a separate util class or something like that. https://codereview.chromium.org/2201783003/diff/60001/android_webview/test/sh... File android_webview/test/shell/src/org/chromium/android_webview/test/SeparateProcessWebViewService.java (right): https://codereview.chromium.org/2201783003/diff/60001/android_webview/test/sh... android_webview/test/shell/src/org/chromium/android_webview/test/SeparateProcessWebViewService.java:31: public static final int RETURN_JNI_CRASH_TEST_SUCCESS = 2; this isn't actually in a set with START_JNI_CRASH_TEST, right? can you add spacing and/or comments etc to make that clear? https://codereview.chromium.org/2201783003/diff/60001/android_webview/test/sh... android_webview/test/shell/src/org/chromium/android_webview/test/SeparateProcessWebViewService.java:59: super.handleMessage(msg); again, can this happen? if not, ban it https://codereview.chromium.org/2201783003/diff/60001/android_webview/test/sh... android_webview/test/shell/src/org/chromium/android_webview/test/SeparateProcessWebViewService.java:78: if (e instanceof MySpecialException) { in theory you could be more strict and check for the specific instance of the exception, but this is probably good enough in practice https://codereview.chromium.org/2201783003/diff/60001/android_webview/test/sh... android_webview/test/shell/src/org/chromium/android_webview/test/SeparateProcessWebViewService.java:85: Log.e(TAG, "RemoteException in uncaughtExceptionHangler", re); crash here? https://codereview.chromium.org/2201783003/diff/60001/android_webview/test/sh... android_webview/test/shell/src/org/chromium/android_webview/test/SeparateProcessWebViewService.java:88: originalUncaughtExceptionHandler.uncaughtException(t, e); won't this race with send above? https://codereview.chromium.org/2201783003/diff/60001/android_webview/test/sh... android_webview/test/shell/src/org/chromium/android_webview/test/SeparateProcessWebViewService.java:117: private AwTestContainerView createDetachedAwTestContainerView( On 2016/09/06 15:26:08, gsennton wrote: > This method is essentially a copy-paste from AwTestBase - I don't really like > this, but on the other hand it's a bit fiddly to put more AwTestBase logic in > the shell-apk. > Do we care about the copy-pasting here? Yes
https://codereview.chromium.org/2201783003/diff/20001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/JniCrashTest.java (right): https://codereview.chromium.org/2201783003/diff/20001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/JniCrashTest.java:101: assertNotNull(getActivity().startService(intent)); On 2016/09/06 15:26:08, gsennton wrote: > On 2016/08/02 20:33:57, gsennton wrote: > > On 2016/08/02 20:15:59, boliu wrote: > > > not super familiar with services, is there something we listen to for > service > > > crash? if the reply does not arrive first, that would be an explicit failure > > > case instead of waiting for this to timeout? > > > > There is the onServiceDisconnected callback above we could use to notify the > > test when we lose the connection to the service. > > Though I think the onServiceDisconnected callback would just mean that the > service got killed - it might still be restarted again to complete the test. > > Now I'm returning a message from the service telling us that we failed. But that > only covers the case when the service throws a java exception - not the other > cases where the service can crash. Sounds like should definitely listen to onServiceDisconnected in this test then, to avoid timeouts. https://codereview.chromium.org/2201783003/diff/20001/android_webview/test/sh... File android_webview/test/shell/src/org/chromium/android_webview/test/SecondBrowserProcess.java (right): https://codereview.chromium.org/2201783003/diff/20001/android_webview/test/sh... android_webview/test/shell/src/org/chromium/android_webview/test/SecondBrowserProcess.java:57: static void startBrowserProcess(Context context) throws Exception { On 2016/09/06 15:26:08, gsennton wrote: > On 2016/08/02 20:33:57, gsennton wrote: > > On 2016/08/02 20:15:59, boliu wrote: > > > uhh, move this somewhere that appears more "shared", another file? > > > > > > actually, one service per test sucks, can we re-use the same service (give > it > > a > > > more generic name), and just work out what to do depending on what message > is > > > received? > > > > Sure, will do :) > > Actually, IMO we shouldn't merge these services because SecondBrowserProcess is > simpler than SeparateProcessWebViewService - the latter needs to communicate > with the corresponding test long after the initial setup-communication is done > (i.e. the communication needs to happen in the uncaughtExceptionHandler) while > SecondBrowserProcess can just run the code it wants to test and then return the > result directly. So to communicate in the new Service I'm using Messengers while > SecondBrowserProcess just uses the onTransact method. > SecondBrowserProcess is easier to read/interpret as is IMO (actually, the > primary reason I don't want to merge the services is because then I would have > to re-run/fix the tests that use SecondBrowserProcess to ensure they work > correctly). > > Does this make sense? (merging them shouldn't be too difficult, it just seems > like extra work for very little gain). If we don't merge them I'll move the > startBrowserProcess method somewhere else. Always optimize for code that's easy to understand rather than code that's easy to write. Code is read a lot more often than it's written. I'll leave the decision to you, but sounds like this case, SecondBrowserProcess can be easily rewritten on top of Messenger. But having two tests living in the same file might look messy and confusing too. Maybe something that sets up the service, which calls out to test bodies in separate files. It's like you are writing junit on top of android services.
Woooow, this CL grew a lot! :/ Anyway, the test-class logic is simplified while most of the Service-handling logic now lives in SeparateServiceTestBase.java. I only use the Service SeparateProcessWebViewService.java (SecondBrowserProcess.java is deleted) now, and it calls into children of "ServiceTestRunner" which run the code each test wants to run in the Service - this code must send a response from the Service back to the test process for the test to finish. https://codereview.chromium.org/2201783003/diff/20001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/JniCrashTest.java (right): https://codereview.chromium.org/2201783003/diff/20001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/JniCrashTest.java:101: assertNotNull(getActivity().startService(intent)); On 2016/09/07 18:32:40, boliu wrote: > On 2016/09/06 15:26:08, gsennton wrote: > > On 2016/08/02 20:33:57, gsennton wrote: > > > On 2016/08/02 20:15:59, boliu wrote: > > > > not super familiar with services, is there something we listen to for > > service > > > > crash? if the reply does not arrive first, that would be an explicit > failure > > > > case instead of waiting for this to timeout? > > > > > > There is the onServiceDisconnected callback above we could use to notify the > > > test when we lose the connection to the service. > > > > Though I think the onServiceDisconnected callback would just mean that the > > service got killed - it might still be restarted again to complete the test. > > > > Now I'm returning a message from the service telling us that we failed. But > that > > only covers the case when the service throws a java exception - not the other > > cases where the service can crash. > > Sounds like should definitely listen to onServiceDisconnected in this test then, > to avoid timeouts. Wouldn't that be a possible cause for flakiness? (if the service gets killed for whatever reason, instead of crashing). I'm not sure here - atm I've added a check that crashes here if we disconnect Before we receive a response from the Service. https://codereview.chromium.org/2201783003/diff/20001/android_webview/test/sh... File android_webview/test/shell/src/org/chromium/android_webview/test/SecondBrowserProcess.java (right): https://codereview.chromium.org/2201783003/diff/20001/android_webview/test/sh... android_webview/test/shell/src/org/chromium/android_webview/test/SecondBrowserProcess.java:57: static void startBrowserProcess(Context context) throws Exception { On 2016/09/07 18:32:40, boliu wrote: > On 2016/09/06 15:26:08, gsennton wrote: > > On 2016/08/02 20:33:57, gsennton wrote: > > > On 2016/08/02 20:15:59, boliu wrote: > > > > uhh, move this somewhere that appears more "shared", another file? > > > > > > > > actually, one service per test sucks, can we re-use the same service (give > > it > > > a > > > > more generic name), and just work out what to do depending on what message > > is > > > > received? > > > > > > Sure, will do :) > > > > Actually, IMO we shouldn't merge these services because SecondBrowserProcess > is > > simpler than SeparateProcessWebViewService - the latter needs to communicate > > with the corresponding test long after the initial setup-communication is done > > (i.e. the communication needs to happen in the uncaughtExceptionHandler) while > > SecondBrowserProcess can just run the code it wants to test and then return > the > > result directly. So to communicate in the new Service I'm using Messengers > while > > SecondBrowserProcess just uses the onTransact method. > > SecondBrowserProcess is easier to read/interpret as is IMO (actually, the > > primary reason I don't want to merge the services is because then I would have > > to re-run/fix the tests that use SecondBrowserProcess to ensure they work > > correctly). > > > > Does this make sense? (merging them shouldn't be too difficult, it just seems > > like extra work for very little gain). If we don't merge them I'll move the > > startBrowserProcess method somewhere else. > > Always optimize for code that's easy to understand rather than code that's easy > to write. Code is read a lot more often than it's written. > > I'll leave the decision to you, but sounds like this case, SecondBrowserProcess > can be easily rewritten on top of Messenger. But having two tests living in the > same file might look messy and confusing too. Maybe something that sets up the > service, which calls out to test bodies in separate files. It's like you are > writing junit on top of android services. Yes, this - having a general-purpose Service as base and building tests in separate files on top - sounds like the best solution :) (this solution should make it easy to add new similar tests in the future). PS5 implements this. It would be even better if we could put the "junit on top of android services" classes in the test apk and keep the service in the shell apk - in that way all the testing functionality could still remain in the test apk (though I can't think of decent way of implementing this in such a way - since the Service itself lives in the shell apk and needs access to the class where we put our code). https://codereview.chromium.org/2201783003/diff/60001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/JniCrashTest.java (right): https://codereview.chromium.org/2201783003/diff/60001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/JniCrashTest.java:31: private AwTestContainerView mTestContainerView; On 2016/09/07 18:24:07, boliu wrote: > these all appear unused Done. https://codereview.chromium.org/2201783003/diff/60001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/JniCrashTest.java:84: ThreadUtils.getUiThreadLooper(), new Handler.Callback() { On 2016/09/07 18:24:07, boliu wrote: > ui looper sounds like a worse option though, because then you risk triggering > ANRs, use a background thread Done - created a HandlerThread. https://codereview.chromium.org/2201783003/diff/60001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/JniCrashTest.java:93: return true; On 2016/09/07 18:24:07, boliu wrote: > why would you not notify the latch here? are you expecting the instrumentation > thread to wait forever here? how does the test handler handle that? Done. https://codereview.chromium.org/2201783003/diff/60001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/JniCrashTest.java:95: return false; On 2016/09/07 18:24:07, boliu wrote: > can this ever happen? ban it if not Done. https://codereview.chromium.org/2201783003/diff/60001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/JniCrashTest.java:112: AwTestBase.WAIT_TIMEOUT_MS, TimeUnit.MILLISECONDS)); On 2016/09/07 18:24:07, boliu wrote: > this is bound to be flaky if you are relying on a timeout. waiting can't be > avoided here, but should minimize the cases where timeout happens by design > > what if RETURN_JNI_CRASH_TEST_FAILED is received? > what if service crashes unexpectedly? > > at least both of those cases can be notified without timeout How do you mean this would be flaky? If everything is fine we won't hit the timeout (given that we can start a service with webview in it before we time out). I've added failure-checking for the two cases you mention above. https://codereview.chromium.org/2201783003/diff/60001/android_webview/test/sh... File android_webview/test/shell/src/org/chromium/android_webview/test/SecondBrowserProcess.java (right): https://codereview.chromium.org/2201783003/diff/60001/android_webview/test/sh... android_webview/test/shell/src/org/chromium/android_webview/test/SecondBrowserProcess.java:57: static void startBrowserProcess(Context context) throws Exception { On 2016/09/07 18:24:07, boliu wrote: > even if you are not merging the two services, this is still not ok because the > dependency makes no sense. factor it out into a separate util class or something > like that. It's now in AwTestBaseUtility. https://codereview.chromium.org/2201783003/diff/60001/android_webview/test/sh... File android_webview/test/shell/src/org/chromium/android_webview/test/SeparateProcessWebViewService.java (right): https://codereview.chromium.org/2201783003/diff/60001/android_webview/test/sh... android_webview/test/shell/src/org/chromium/android_webview/test/SeparateProcessWebViewService.java:31: public static final int RETURN_JNI_CRASH_TEST_SUCCESS = 2; On 2016/09/07 18:24:07, boliu wrote: > this isn't actually in a set with START_JNI_CRASH_TEST, right? can you add > spacing and/or comments etc to make that clear? Removed START_JNI_CRASH_TEST. https://codereview.chromium.org/2201783003/diff/60001/android_webview/test/sh... android_webview/test/shell/src/org/chromium/android_webview/test/SeparateProcessWebViewService.java:59: super.handleMessage(msg); On 2016/09/07 18:24:08, boliu wrote: > again, can this happen? if not, ban it Done. https://codereview.chromium.org/2201783003/diff/60001/android_webview/test/sh... android_webview/test/shell/src/org/chromium/android_webview/test/SeparateProcessWebViewService.java:85: Log.e(TAG, "RemoteException in uncaughtExceptionHangler", re); On 2016/09/07 18:24:08, boliu wrote: > crash here? "void uncaughtException(Thread t, Throwable e) Method invoked when the given thread terminates due to the given uncaught exception. Any exception thrown by this method will be ignored by the Java Virtual Machine." https://codereview.chromium.org/2201783003/diff/60001/android_webview/test/sh... android_webview/test/shell/src/org/chromium/android_webview/test/SeparateProcessWebViewService.java:88: originalUncaughtExceptionHandler.uncaughtException(t, e); On 2016/09/07 18:24:08, boliu wrote: > won't this race with send above? Yup, as we talked about offline I now just avoid calling the original uncaughtExceptionHandler and instead kill the service when the test is done (to ensure the message does get sent back to the test before the service dies). https://codereview.chromium.org/2201783003/diff/60001/android_webview/test/sh... android_webview/test/shell/src/org/chromium/android_webview/test/SeparateProcessWebViewService.java:117: private AwTestContainerView createDetachedAwTestContainerView( On 2016/09/07 18:24:07, boliu wrote: > On 2016/09/06 15:26:08, gsennton wrote: > > This method is essentially a copy-paste from AwTestBase - I don't really like > > this, but on the other hand it's a bit fiddly to put more AwTestBase logic in > > the shell-apk. > > Do we care about the copy-pasting here? > > Yes I added a static method in AwTestBaseUtility that is used by AwTestBase as well.
you still think all this complexity and maintenance cost is worth adding this test..? https://codereview.chromium.org/2201783003/diff/60001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/JniCrashTest.java (right): https://codereview.chromium.org/2201783003/diff/60001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/JniCrashTest.java:112: AwTestBase.WAIT_TIMEOUT_MS, TimeUnit.MILLISECONDS)); On 2016/09/14 13:51:52, gsennton wrote: > On 2016/09/07 18:24:07, boliu wrote: > > this is bound to be flaky if you are relying on a timeout. waiting can't be > > avoided here, but should minimize the cases where timeout happens by design > > > > what if RETURN_JNI_CRASH_TEST_FAILED is received? > > what if service crashes unexpectedly? > > > > at least both of those cases can be notified without timeout > > How do you mean this would be flaky? If everything is fine we won't hit the > timeout (given that we can start a service with webview in it before we time > out). > > I've added failure-checking for the two cases you mention above. Well, this code assumes the test finishes in 15 seconds. But how do you really know if that's long enough? What if this is running on a low end device and it has to evict a lot of other things to start the service, something like that etc.. Another problem with timeout is that it's completely useless for debugging when it happens. So if you can make things be an expectation failure (ie RETURN_JNI_CRASH_TEST_FAILED) rather than assertion failure (ie timeout), it's a much better test. https://codereview.chromium.org/2201783003/diff/60001/android_webview/test/sh... File android_webview/test/shell/src/org/chromium/android_webview/test/SeparateProcessWebViewService.java (right): https://codereview.chromium.org/2201783003/diff/60001/android_webview/test/sh... android_webview/test/shell/src/org/chromium/android_webview/test/SeparateProcessWebViewService.java:85: Log.e(TAG, "RemoteException in uncaughtExceptionHangler", re); On 2016/09/14 13:51:52, gsennton wrote: > On 2016/09/07 18:24:08, boliu wrote: > > crash here? > > "void uncaughtException(Thread t, > Throwable e) > Method invoked when the given thread terminates due to the given uncaught > exception. > Any exception thrown by this method will be ignored by the Java Virtual > Machine." System.exit? Basically anything that lets the other side notice the process is gone before the timeout happens. https://codereview.chromium.org/2201783003/diff/80001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/AwSecondBrowserProcessTest.java (right): https://codereview.chromium.org/2201783003/diff/80001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwSecondBrowserProcessTest.java:24: private static final String TAG = AwSecondBrowserProcessTest.class.getSimpleName(); this is not used https://codereview.chromium.org/2201783003/diff/80001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwSecondBrowserProcessTest.java:39: @DisabledTest don't drop the message also can you make sure these two tests still works ok? I think this one is disabled because we actually have to make the crash a warning instead https://codereview.chromium.org/2201783003/diff/80001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwSecondBrowserProcessTest.java:72: }, false /* unBindAndStopService */); hmmmm, this unBindAndStopService thing kinda smells bad, since spreading clean up code is generally not a good idea you can refactor this test to execute code while the service is up by doing adding code to handler above when a message is received https://codereview.chromium.org/2201783003/diff/80001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/SeparateServiceTestBase.java (right): https://codereview.chromium.org/2201783003/diff/80001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/SeparateServiceTestBase.java:31: private static final String TAG = SeparateServiceTestBase.class.getSimpleName(); unused https://codereview.chromium.org/2201783003/diff/80001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/SeparateServiceTestBase.java:53: mSeparateProcessThread = new HandlerThread("SeparateWebViewProcessThread"); do you need a new thread? I think you don't actually do any blocking thing on this thread, so just using the main thread should be fine, right? one less thread to keep track of https://codereview.chromium.org/2201783003/diff/80001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/SeparateServiceTestBase.java:126: assertEquals("Service disconnected before we received a response", (long) 0, 0L https://codereview.chromium.org/2201783003/diff/80001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/SeparateServiceTestBase.java:145: // TODO do we care about handler leaks in a test class? no https://codereview.chromium.org/2201783003/diff/80001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/SeparateServiceTestBase.java:154: mProcessPidMessageReceivedLatch.countDown(); It's really odd that we are the one starting a service process, and we have to ask the service process what its pid is. There should be a unspoofable way to get that without having the service to provide it, surely.. Actually, looking at how ChildProcessConnection in clank works, it uses an aidl interface over binder, but it basically just asks the service for its pid too. o_O how's that safe...?! https://codereview.chromium.org/2201783003/diff/80001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/SeparateServiceTestBase.java:158: mMessageReceivedLatch.countDown(); I'm confused what the return value of mTestMessageHandler.handleMessage means do you mean for returning true to mean end the test? but then it's not possible to encode unhandled vs handled but don't mark test as done. https://codereview.chromium.org/2201783003/diff/80001/android_webview/test/sh... File android_webview/test/shell/src/org/chromium/android_webview/test/SecondBrowserProcessTestRunner.java (right): https://codereview.chromium.org/2201783003/diff/80001/android_webview/test/sh... android_webview/test/shell/src/org/chromium/android_webview/test/SecondBrowserProcessTestRunner.java:17: public class SecondBrowserProcessTestRunner extends ServiceTestRunner { can this just be an inner class of AwSecondBrowserProcessTest? https://codereview.chromium.org/2201783003/diff/80001/android_webview/test/sh... File android_webview/test/shell/src/org/chromium/android_webview/test/SeparateProcessWebViewService.java (right): https://codereview.chromium.org/2201783003/diff/80001/android_webview/test/sh... android_webview/test/shell/src/org/chromium/android_webview/test/SeparateProcessWebViewService.java:27: public static final String TEST_CODE_BUNDLE_TAG = "test_code_bundle_tag"; all caps https://codereview.chromium.org/2201783003/diff/80001/android_webview/test/sh... File android_webview/test/shell/src/org/chromium/android_webview/test/ServiceTestRunner.java (right): https://codereview.chromium.org/2201783003/diff/80001/android_webview/test/sh... android_webview/test/shell/src/org/chromium/android_webview/test/ServiceTestRunner.java:14: * This class is Serializable so that it can't be passed through IPC from a test process to a "can't" or can? https://codereview.chromium.org/2201783003/diff/80001/android_webview/test/sh... android_webview/test/shell/src/org/chromium/android_webview/test/ServiceTestRunner.java:17: public abstract class ServiceTestRunner implements Serializable { interface? https://codereview.chromium.org/2201783003/diff/80001/android_webview/test/sh... File android_webview/test/shell/src/org/chromium/android_webview/test/ShouldOverrideUrlLoadingJniCrashTestRunner.java (right): https://codereview.chromium.org/2201783003/diff/80001/android_webview/test/sh... android_webview/test/shell/src/org/chromium/android_webview/test/ShouldOverrideUrlLoadingJniCrashTestRunner.java:22: public class ShouldOverrideUrlLoadingJniCrashTestRunner extends ServiceTestRunner { ditto, can this live in JniCrashTest?
Not sure whether it is still worth it :/ It feels like supporting both mine and Mikhail's tests in the same Service is too complex - i.e. the code in SeparateServiceTestBase.java. Reverting back to having one Service for Mikhail's tests and one for mine might be sustainable. I might be looking at this the wrong way: my current POV is basically - given that it shouldn't require much work to finish the test now (ignoring the work I put into the current PS) we could just commit it and if it breaks anything we'll just revert it? (or disable it for low-end devices). But I might be underestimating the time consumption of finishing the test and/or observing whether it makes bots fail, WDYT? https://codereview.chromium.org/2201783003/diff/80001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/AwSecondBrowserProcessTest.java (right): https://codereview.chromium.org/2201783003/diff/80001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwSecondBrowserProcessTest.java:39: @DisabledTest On 2016/09/15 04:57:53, boliu wrote: > don't drop the message > > also can you make sure these two tests still works ok? I think this one is > disabled because we actually have to make the crash a warning instead Both of these fail in the assertFalse(tryStartingBrowserProcess()) statement since (as you said) the crash we are looking for is just a log message now. This is true before and after my change. https://codereview.chromium.org/2201783003/diff/80001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/SeparateServiceTestBase.java (right): https://codereview.chromium.org/2201783003/diff/80001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/SeparateServiceTestBase.java:53: mSeparateProcessThread = new HandlerThread("SeparateWebViewProcessThread"); On 2016/09/15 04:57:53, boliu wrote: > do you need a new thread? I think you don't actually do any blocking thing on > this thread, so just using the main thread should be fine, right? one less > thread to keep track of Right, this thread just handles messages and unblocks the instrumentation thread. https://codereview.chromium.org/2201783003/diff/80001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/SeparateServiceTestBase.java:154: mProcessPidMessageReceivedLatch.countDown(); On 2016/09/15 04:57:53, boliu wrote: > It's really odd that we are the one starting a service process, and we have to > ask the service process what its pid is. There should be a unspoofable way to > get that without having the service to provide it, surely.. > > Actually, looking at how ChildProcessConnection in clank works, it uses an aidl > interface over binder, but it basically just asks the service for its pid too. > o_O how's that safe...?! Oooooh, you mean in case the child decides to return the wrong pid? https://codereview.chromium.org/2201783003/diff/80001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/SeparateServiceTestBase.java:158: mMessageReceivedLatch.countDown(); On 2016/09/15 04:57:53, boliu wrote: > I'm confused what the return value of mTestMessageHandler.handleMessage means > > do you mean for returning true to mean end the test? but then it's not possible > to encode unhandled vs handled but don't mark test as done. Yeah, this implementation is assuming that you either 1. don't handle a message, or 2. you handle it and then the test finishes. I guess you could just change the interface to return a value representing more state than a boolean but the current tests all finish after one message so I didn't see a reason for making this change currently (especially since, as you pointed out, it is not clear whether we want to support this test :/). https://codereview.chromium.org/2201783003/diff/80001/android_webview/test/sh... File android_webview/test/shell/src/org/chromium/android_webview/test/SecondBrowserProcessTestRunner.java (right): https://codereview.chromium.org/2201783003/diff/80001/android_webview/test/sh... android_webview/test/shell/src/org/chromium/android_webview/test/SecondBrowserProcessTestRunner.java:17: public class SecondBrowserProcessTestRunner extends ServiceTestRunner { On 2016/09/15 04:57:53, boliu wrote: > can this just be an inner class of AwSecondBrowserProcessTest? Since the Service lives in the shell apk (which doesn't depend on the test-apk) and the AwSecondBrowserProcessTest lives in the test-apk we can't instantiate a class from the test-apk in the Service where we want to run this code (because that class won't exist in the classloader used by the Service). https://codereview.chromium.org/2201783003/diff/80001/android_webview/test/sh... File android_webview/test/shell/src/org/chromium/android_webview/test/ServiceTestRunner.java (right): https://codereview.chromium.org/2201783003/diff/80001/android_webview/test/sh... android_webview/test/shell/src/org/chromium/android_webview/test/ServiceTestRunner.java:14: * This class is Serializable so that it can't be passed through IPC from a test process to a On 2016/09/15 04:57:53, boliu wrote: > "can't" or can? Can :P
https://codereview.chromium.org/2201783003/diff/80001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/SeparateServiceTestBase.java (right): https://codereview.chromium.org/2201783003/diff/80001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/SeparateServiceTestBase.java:154: mProcessPidMessageReceivedLatch.countDown(); On 2016/09/15 04:57:53, boliu wrote: > It's really odd that we are the one starting a service process, and we have to > ask the service process what its pid is. There should be a unspoofable way to > get that without having the service to provide it, surely.. Not really; Android doesn't really think there's any reason to care what the pid of a service you're bound to is; it's none of your business, and in the general case (not your own package) it will be running as a different UID so you can't use the pid for anything anyway. > Actually, looking at how ChildProcessConnection in clank works, it uses an aidl > interface over binder, but it basically just asks the service for its pid too. > o_O how's that safe...?! It's safe because it happens at startup, before the service has loaded any content. https://codereview.chromium.org/2201783003/diff/80001/android_webview/test/sh... File android_webview/test/shell/src/org/chromium/android_webview/test/ShouldOverrideUrlLoadingJniCrashTestRunner.java (right): https://codereview.chromium.org/2201783003/diff/80001/android_webview/test/sh... android_webview/test/shell/src/org/chromium/android_webview/test/ShouldOverrideUrlLoadingJniCrashTestRunner.java:49: // We don't call the original uncaughtExceptionHandler here - instead we kill the This will mean that the current thread dies, but the process doesn't (the current thread *always* exits when there's an uncaught exception, no matter what the handler does or whether you call the original one) - so if that's the UI thread, the system is going to ANR you five seconds after the next time it sends you a message. The original handler kills the whole process when this happens and that's the expected behaviour *on android* - the system expects this and if you're continuing to run without a UI thread then weird behaviour is likely..
https://codereview.chromium.org/2201783003/diff/80001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/SeparateServiceTestBase.java (right): https://codereview.chromium.org/2201783003/diff/80001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/SeparateServiceTestBase.java:154: mProcessPidMessageReceivedLatch.countDown(); On 2016/09/15 10:26:43, Torne wrote: > On 2016/09/15 04:57:53, boliu wrote: > > It's really odd that we are the one starting a service process, and we have to > > ask the service process what its pid is. There should be a unspoofable way to > > get that without having the service to provide it, surely.. > > Not really; Android doesn't really think there's any reason to care what the pid > of a service you're bound to is; it's none of your business, and in the general > case (not your own package) it will be running as a different UID so you can't > use the pid for anything anyway. > > > Actually, looking at how ChildProcessConnection in clank works, it uses an > aidl > > interface over binder, but it basically just asks the service for its pid too. > > o_O how's that safe...?! > > It's safe because it happens at startup, before the service has loaded any > content. Yeah that's sort of what I assumed too. Still super brittle though. meh https://codereview.chromium.org/2201783003/diff/80001/android_webview/test/sh... File android_webview/test/shell/src/org/chromium/android_webview/test/ShouldOverrideUrlLoadingJniCrashTestRunner.java (right): https://codereview.chromium.org/2201783003/diff/80001/android_webview/test/sh... android_webview/test/shell/src/org/chromium/android_webview/test/ShouldOverrideUrlLoadingJniCrashTestRunner.java:49: // We don't call the original uncaughtExceptionHandler here - instead we kill the On 2016/09/15 10:26:44, Torne wrote: > This will mean that the current thread dies, but the process doesn't (the > current thread *always* exits when there's an uncaught exception, no matter what > the handler does or whether you call the original one) That was the original code. Then I assumed killing the process here will race with sending the message (probably?). If there is a way to ensure the message is sent won't get dropped from killing here, or if there is a way to wait for it to be sent, then just calling the original handler is fine. > - so if that's the UI > thread, the system is going to ANR you five seconds after the next time it sends > you a message. I remember asking something to be moved off the UI thread earlier... now I can't remember where I said that :| ANR won't happen in service I think. But the other point still stands I guess. > > The original handler kills the whole process when this happens and that's the > expected behaviour *on android* - the system expects this and if you're > continuing to run without a UI thread then weird behaviour is likely..
> ANR won't happen in service I think. *Processes* ANR, not activities/services. If the system sends a message to ActivityThread (the class that handles binder messages on the UI thread) and doesn't get a response inside five seconds, the process ANRs. Services get these messages too: for example, all the service lifecycle callbacks, broadcasts, etc.
On 2016/09/15 16:53:48, Torne wrote: > > ANR won't happen in service I think. > > *Processes* ANR, not activities/services. If the system sends a message to > ActivityThread (the class that handles binder messages on the UI thread) and > doesn't get a response inside five seconds, the process ANRs. Services get these > messages too: for example, all the service lifecycle callbacks, broadcasts, etc. Wait.. service main thread is the same as the UI thread..? Well, either way, if the service main thread is dead, there won't be any response either I guess.
On 2016/09/15 16:57:53, boliu wrote: > On 2016/09/15 16:53:48, Torne wrote: > > > ANR won't happen in service I think. > > > > *Processes* ANR, not activities/services. If the system sends a message to > > ActivityThread (the class that handles binder messages on the UI thread) and > > doesn't get a response inside five seconds, the process ANRs. Services get > these > > messages too: for example, all the service lifecycle callbacks, broadcasts, > etc. > > Wait.. service main thread is the same as the UI thread..? Well, either way, if > the service main thread is dead, there won't be any response either I guess. Services handle requests on various binder thread pool threads. I'm not actually sure if the service lifecycle callbacks happen on the main looper or on a binder thread pool thread, but, yeah, if one of the threads is dead then things are gonna start not being answered, probably ;)
https://codereview.chromium.org/2201783003/diff/80001/android_webview/test/sh... File android_webview/test/shell/src/org/chromium/android_webview/test/ShouldOverrideUrlLoadingJniCrashTestRunner.java (right): https://codereview.chromium.org/2201783003/diff/80001/android_webview/test/sh... android_webview/test/shell/src/org/chromium/android_webview/test/ShouldOverrideUrlLoadingJniCrashTestRunner.java:49: // We don't call the original uncaughtExceptionHandler here - instead we kill the On 2016/09/15 16:47:51, boliu wrote: > On 2016/09/15 10:26:44, Torne wrote: > > This will mean that the current thread dies, but the process doesn't (the > > current thread *always* exits when there's an uncaught exception, no matter > what > > the handler does or whether you call the original one) > > That was the original code. Then I assumed killing the process here will race > with sending the message (probably?). > > If there is a way to ensure the message is sent won't get dropped from killing > here, or if there is a way to wait for it to be sent, then just calling the > original handler is fine. > > > - so if that's the UI > > thread, the system is going to ANR you five seconds after the next time it > sends > > you a message. > > I remember asking something to be moved off the UI thread earlier... now I can't > remember where I said that :| > > ANR won't happen in service I think. But the other point still stands I guess. > > > > > The original handler kills the whole process when this happens and that's the > > expected behaviour *on android* - the system expects this and if you're > > continuing to run without a UI thread then weird behaviour is likely.. > Yeah, if the thread + process don't die while this UEH call is still active then we could just block here until we receive a message from the test-process ensuring us that the test has received the successMessage.. is this true though? Can we even receive binder calls at this point? (before the UEH finishes)
https://codereview.chromium.org/2201783003/diff/80001/android_webview/test/sh... File android_webview/test/shell/src/org/chromium/android_webview/test/ShouldOverrideUrlLoadingJniCrashTestRunner.java (right): https://codereview.chromium.org/2201783003/diff/80001/android_webview/test/sh... android_webview/test/shell/src/org/chromium/android_webview/test/ShouldOverrideUrlLoadingJniCrashTestRunner.java:49: // We don't call the original uncaughtExceptionHandler here - instead we kill the On 2016/09/15 17:08:35, gsennton wrote: > On 2016/09/15 16:47:51, boliu wrote: > > On 2016/09/15 10:26:44, Torne wrote: > > > This will mean that the current thread dies, but the process doesn't (the > > > current thread *always* exits when there's an uncaught exception, no matter > > what > > > the handler does or whether you call the original one) > > > > That was the original code. Then I assumed killing the process here will race > > with sending the message (probably?). > > > > If there is a way to ensure the message is sent won't get dropped from killing > > here, or if there is a way to wait for it to be sent, then just calling the > > original handler is fine. > > > > > - so if that's the UI > > > thread, the system is going to ANR you five seconds after the next time it > > sends > > > you a message. > > > > I remember asking something to be moved off the UI thread earlier... now I > can't > > remember where I said that :| > > > > ANR won't happen in service I think. But the other point still stands I guess. > > > > > > > > The original handler kills the whole process when this happens and that's > the > > > expected behaviour *on android* - the system expects this and if you're > > > continuing to run without a UI thread then weird behaviour is likely.. > > > > Yeah, if the thread + process don't die while this UEH call is still active then > we could just block here until we receive a message from the test-process > ensuring us that the test has received the successMessage.. is this true though? > Can we even receive binder calls at this point? (before the UEH finishes) You are talking about nested looper. And I have no idea if that's supported on android or not. Either way, should be avoided if possible..
https://codereview.chromium.org/2201783003/diff/80001/android_webview/test/sh... File android_webview/test/shell/src/org/chromium/android_webview/test/ShouldOverrideUrlLoadingJniCrashTestRunner.java (right): https://codereview.chromium.org/2201783003/diff/80001/android_webview/test/sh... android_webview/test/shell/src/org/chromium/android_webview/test/ShouldOverrideUrlLoadingJniCrashTestRunner.java:49: // We don't call the original uncaughtExceptionHandler here - instead we kill the On 2016/09/15 17:13:12, boliu wrote: > On 2016/09/15 17:08:35, gsennton wrote: > > On 2016/09/15 16:47:51, boliu wrote: > > > On 2016/09/15 10:26:44, Torne wrote: > > > > This will mean that the current thread dies, but the process doesn't (the > > > > current thread *always* exits when there's an uncaught exception, no > matter > > > what > > > > the handler does or whether you call the original one) > > > > > > That was the original code. Then I assumed killing the process here will > race > > > with sending the message (probably?). > > > > > > If there is a way to ensure the message is sent won't get dropped from > killing > > > here, or if there is a way to wait for it to be sent, then just calling the > > > original handler is fine. > > > > > > > - so if that's the UI > > > > thread, the system is going to ANR you five seconds after the next time it > > > sends > > > > you a message. > > > > > > I remember asking something to be moved off the UI thread earlier... now I > > can't > > > remember where I said that :| > > > > > > ANR won't happen in service I think. But the other point still stands I > guess. > > > > > > > > > > > The original handler kills the whole process when this happens and that's > > the > > > > expected behaviour *on android* - the system expects this and if you're > > > > continuing to run without a UI thread then weird behaviour is likely.. > > > > > > > Yeah, if the thread + process don't die while this UEH call is still active > then > > we could just block here until we receive a message from the test-process > > ensuring us that the test has received the successMessage.. is this true > though? > > Can we even receive binder calls at this point? (before the UEH finishes) > > You are talking about nested looper. And I have no idea if that's supported on > android or not. Either way, should be avoided if possible.. I don't see the connection between this case and a nested looper, can you explain? :P
https://codereview.chromium.org/2201783003/diff/80001/android_webview/test/sh... File android_webview/test/shell/src/org/chromium/android_webview/test/ShouldOverrideUrlLoadingJniCrashTestRunner.java (right): https://codereview.chromium.org/2201783003/diff/80001/android_webview/test/sh... android_webview/test/shell/src/org/chromium/android_webview/test/ShouldOverrideUrlLoadingJniCrashTestRunner.java:49: // We don't call the original uncaughtExceptionHandler here - instead we kill the On 2016/09/15 17:17:21, gsennton wrote: > On 2016/09/15 17:13:12, boliu wrote: > > On 2016/09/15 17:08:35, gsennton wrote: > > > On 2016/09/15 16:47:51, boliu wrote: > > > > On 2016/09/15 10:26:44, Torne wrote: > > > > > This will mean that the current thread dies, but the process doesn't > (the > > > > > current thread *always* exits when there's an uncaught exception, no > > matter > > > > what > > > > > the handler does or whether you call the original one) > > > > > > > > That was the original code. Then I assumed killing the process here will > > race > > > > with sending the message (probably?). > > > > > > > > If there is a way to ensure the message is sent won't get dropped from > > killing > > > > here, or if there is a way to wait for it to be sent, then just calling > the > > > > original handler is fine. > > > > > > > > > - so if that's the UI > > > > > thread, the system is going to ANR you five seconds after the next time > it > > > > sends > > > > > you a message. > > > > > > > > I remember asking something to be moved off the UI thread earlier... now I > > > can't > > > > remember where I said that :| > > > > > > > > ANR won't happen in service I think. But the other point still stands I > > guess. > > > > > > > > > > > > > > The original handler kills the whole process when this happens and > that's > > > the > > > > > expected behaviour *on android* - the system expects this and if you're > > > > > continuing to run without a UI thread then weird behaviour is likely.. > > > > > > > > > > Yeah, if the thread + process don't die while this UEH call is still active > > then > > > we could just block here until we receive a message from the test-process > > > ensuring us that the test has received the successMessage.. is this true > > though? > > > Can we even receive binder calls at this point? (before the UEH finishes) > > > > You are talking about nested looper. And I have no idea if that's supported on > > android or not. Either way, should be avoided if possible.. > > I don't see the connection between this case and a nested looper, can you > explain? :P You can't block the thread and then wait for a event that will be posted to the thread you just blocked.. assuming the event does get added to the queue, you could say, lets just keep running through the event queue, and that's a nested looper (or nested message loop in chrome parlance..) You can probably structure the thing such that the thread that just got nuked is *not* the thread that receives messages. Then you can probably wait for it. But it's like all so complex....
On 2016/09/15 10:02:04, gsennton wrote: > Not sure whether it is still worth it :/ > It feels like supporting both mine and Mikhail's tests in the same Service is > too complex - i.e. the code in SeparateServiceTestBase.java. Reverting back to > having one Service for Mikhail's tests and one for mine might be sustainable. By sustainable, you really mean having this CL be a manageable size. You could always do the refactor for the existing test first, then add the new test. Or split this into smaller pieces. > > I might be looking at this the wrong way: my current POV is basically - given > that it shouldn't require much work to finish the test now (ignoring the work I > put into the current PS) we could just commit it and if it breaks anything we'll > just revert it? (or disable it for low-end devices). A flaky test is worse than no test, because it consumes compute and human resources, and has no benefit. > But I might be underestimating the time consumption of finishing the test and/or > observing whether it makes bots fail, WDYT? Not ok. Do not commit flaky tests. https://codereview.chromium.org/2201783003/diff/80001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/AwSecondBrowserProcessTest.java (right): https://codereview.chromium.org/2201783003/diff/80001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwSecondBrowserProcessTest.java:39: @DisabledTest On 2016/09/15 10:02:04, gsennton wrote: > On 2016/09/15 04:57:53, boliu wrote: > > don't drop the message > > > > also can you make sure these two tests still works ok? I think this one is > > disabled because we actually have to make the crash a warning instead > > Both of these fail in the assertFalse(tryStartingBrowserProcess()) statement > since (as you said) the crash we are looking for is just a log message now. This > is true before and after my change. Can you turn on the crash and make sure the tests pass? and repeat like 20 times to make sure they are not flaky? (I guess do that when everything else in this CL is ready, since things are still changing it looks like) https://codereview.chromium.org/2201783003/diff/80001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/SeparateServiceTestBase.java (right): https://codereview.chromium.org/2201783003/diff/80001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/SeparateServiceTestBase.java:158: mMessageReceivedLatch.countDown(); On 2016/09/15 10:02:04, gsennton wrote: > On 2016/09/15 04:57:53, boliu wrote: > > I'm confused what the return value of mTestMessageHandler.handleMessage means > > > > do you mean for returning true to mean end the test? but then it's not > possible > > to encode unhandled vs handled but don't mark test as done. > > Yeah, this implementation is assuming that you either 1. don't handle a message, > or 2. you handle it and then the test finishes. > I guess you could just change the interface to return a value representing more > state than a boolean but the current tests all finish after one message so I > didn't see a reason for making this change currently (especially since, as you > pointed out, it is not clear whether we want to support this test :/). Another option is you can give the latch up to the test cast and let it decide when to unblock. https://codereview.chromium.org/2201783003/diff/80001/android_webview/test/sh... File android_webview/test/shell/src/org/chromium/android_webview/test/SecondBrowserProcessTestRunner.java (right): https://codereview.chromium.org/2201783003/diff/80001/android_webview/test/sh... android_webview/test/shell/src/org/chromium/android_webview/test/SecondBrowserProcessTestRunner.java:17: public class SecondBrowserProcessTestRunner extends ServiceTestRunner { On 2016/09/15 10:02:04, gsennton wrote: > On 2016/09/15 04:57:53, boliu wrote: > > can this just be an inner class of AwSecondBrowserProcessTest? > > Since the Service lives in the shell apk (which doesn't depend on the test-apk) > and the AwSecondBrowserProcessTest lives in the test-apk we can't instantiate a > class from the test-apk in the Service where we want to run this code (because > that class won't exist in the classloader used by the Service). Oh so serializable isn't magic :p I'm almost tempted to say move the test to the shell, but that's probably another can of worms, so never mind :/
I think I'd like to implement the 'block in UEH to wait for message from test-code before dying' solution (which shouldn't be too difficult) and see if that works. If so, I'll run the tests lots of times to ensure they don't flake, and then probably split this CL in two. SG? On 2016/09/15 18:04:04, boliu wrote: > On 2016/09/15 10:02:04, gsennton wrote: > > Not sure whether it is still worth it :/ > > It feels like supporting both mine and Mikhail's tests in the same Service is > > too complex - i.e. the code in SeparateServiceTestBase.java. Reverting back to > > having one Service for Mikhail's tests and one for mine might be sustainable. > > By sustainable, you really mean having this CL be a manageable size. You could > always do the refactor for the existing test first, then add the new test. Or > split this into smaller pieces. > > > > > I might be looking at this the wrong way: my current POV is basically - given > > that it shouldn't require much work to finish the test now (ignoring the work > I > > put into the current PS) we could just commit it and if it breaks anything > we'll > > just revert it? (or disable it for low-end devices). > > A flaky test is worse than no test, because it consumes compute and human > resources, and has no benefit. > > > But I might be underestimating the time consumption of finishing the test > and/or > > observing whether it makes bots fail, WDYT? > > Not ok. Do not commit flaky tests.
I'm gonna close this so it won't show up in your codereview-queue. IIRC I gave up here because the test was flaky - and that was probably because waiting within an UncaughtExceptionHandler on the UI thread (while trying to communicate over binder with another current process) is not really something that is supported.
Description was changed from ========== Add test to ensure shouldOverrideUrlLoading throws Java exception [Can't land before https://codereview.chromium.org/2169553002/ - the added test won't pass] Add test that creates a WebView in a service in a separate process and makes the its shouldOverrideUrlLoading callback throw a run-time exception. Then check that the uncaughtExceptionHandler for the UI thread of the service sees this exception. Refactoring some instrumentation test utility files so that they can be reached from the webview shell component (where the service is defined). BUG=592556 ========== to ========== Add test to ensure shouldOverrideUrlLoading throws Java exception [Can't land before https://codereview.chromium.org/2169553002/ - the added test won't pass] Add test that creates a WebView in a service in a separate process and makes the its shouldOverrideUrlLoading callback throw a run-time exception. Then check that the uncaughtExceptionHandler for the UI thread of the service sees this exception. Refactoring some instrumentation test utility files so that they can be reached from the webview shell component (where the service is defined). BUG=592556 ========== |
