Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2691073003:
Add javatests for the WebView SafeBrowsing feature (Closed)
|
Created:
3 years, 10 months ago by Nate Fischer Modified:
3 years, 10 months ago CC:
android-webview-reviews_chromium.org, chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd javatests for the WebView SafeBrowsing feature
This adds javatests for the SafeBrowsing feature in webview. It tests:
* Loading a safe URL does not show an interstitial
* Loading a malicious URL does show an interstitial
* Loading a page with a malicious subresource does show an interstitial
BUG=676737
Review-Url: https://codereview.chromium.org/2691073003
Cr-Commit-Position: refs/heads/master@{#452385}
Committed: https://chromium.googlesource.com/chromium/src/+/f7c3ed28c88cf8f5e30a3879c44005d527ea11a6
Patch Set 1 #
Total comments: 11
Patch Set 2 : No sleeps, reduce duplicated code, switch to EmbeddedTestServer, etc. #
Total comments: 10
Patch Set 3 : Refactoring #Patch Set 4 : Fix DEPS issue #Patch Set 5 : The callback helper approach didn't work, so just more carefully checks pixel color #
Total comments: 2
Patch Set 6 : No polling, only waitforcallback #Patch Set 7 : Make the iframe html gray instead of black, change assertFalse(... == ...) to assertTrue(... != ...… #Patch Set 8 : Changing the pages from using body.background-color to creating a full-size div with the background… #
Total comments: 2
Patch Set 9 : Move getPixelColorAtCenterOfView to GraphicsTestUtils #Messages
Total messages: 53 (38 generated)
The CQ bit was checked by ntfschr@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...
ntfschr@chromium.org changed reviewers: + boliu@chromium.org
PTAL. This relies on TestWebServer, which is deprecated in favor of EmbeddedTestServer. If this is an issue, I can switch to the new test server (but this would require saving the html as files in the source tree).
> This relies on TestWebServer, which is deprecated in favor of > EmbeddedTestServer. If this is an issue, I can switch to the new test server > (but this would require saving the html as files in the source tree). shenghuazhang is actively removing all TestWebServer usage. Don't add more https://codereview.chromium.org/2691073003/diff/1/android_webview/javatests/s... File android_webview/javatests/src/org/chromium/android_webview/test/SafeBrowsingTest.java (right): https://codereview.chromium.org/2691073003/diff/1/android_webview/javatests/s... android_webview/javatests/src/org/chromium/android_webview/test/SafeBrowsingTest.java:67: return Pattern.compile(".*" + MALWARE_URL).matcher(uri).matches(); endsWith good enough? https://codereview.chromium.org/2691073003/diff/1/android_webview/javatests/s... android_webview/javatests/src/org/chromium/android_webview/test/SafeBrowsingTest.java:101: final Context appContext = getInstrumentation().getTargetContext().getApplicationContext(); this is a lot of copied code, meaning AwTestBase is doing things wrong, duplicated code should be in a private method in AwTestBase, and then have the pref and context passed into the protected method https://codereview.chromium.org/2691073003/diff/1/android_webview/javatests/s... android_webview/javatests/src/org/chromium/android_webview/test/SafeBrowsingTest.java:113: return (bitmap.getPixel(0, 0) == INTERSTITIAL_COLOR); where is this color from? https://codereview.chromium.org/2691073003/diff/1/android_webview/javatests/s... android_webview/javatests/src/org/chromium/android_webview/test/SafeBrowsingTest.java:141: Thread.sleep(WAIT_TIMEOUT_MS); no sleeps you can wait for startUriLookup to be called https://codereview.chromium.org/2691073003/diff/1/android_webview/javatests/s... android_webview/javatests/src/org/chromium/android_webview/test/SafeBrowsingTest.java:167: Thread.sleep(WAIT_TIMEOUT_MS); ditto
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by ntfschr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
PTAL. I'm still looking into the trybot failures. https://codereview.chromium.org/2691073003/diff/1/android_webview/javatests/s... File android_webview/javatests/src/org/chromium/android_webview/test/SafeBrowsingTest.java (right): https://codereview.chromium.org/2691073003/diff/1/android_webview/javatests/s... android_webview/javatests/src/org/chromium/android_webview/test/SafeBrowsingTest.java:67: return Pattern.compile(".*" + MALWARE_URL).matcher(uri).matches(); On 2017/02/14 at 02:34:32, boliu wrote: > endsWith good enough? That's better, thanks. Done https://codereview.chromium.org/2691073003/diff/1/android_webview/javatests/s... android_webview/javatests/src/org/chromium/android_webview/test/SafeBrowsingTest.java:101: final Context appContext = getInstrumentation().getTargetContext().getApplicationContext(); On 2017/02/14 at 02:34:31, boliu wrote: > this is a lot of copied code, meaning AwTestBase is doing things wrong, duplicated code should be in a private method in AwTestBase, and then have the pref and context passed into the protected method I broke it into 3 methods. The intermediate method is overridden in the sub class. By splitting it in 3, I was able to get rid of the MockAwBrowserContext member variable in the sub class. https://codereview.chromium.org/2691073003/diff/1/android_webview/javatests/s... android_webview/javatests/src/org/chromium/android_webview/test/SafeBrowsingTest.java:113: return (bitmap.getPixel(0, 0) == INTERSTITIAL_COLOR); On 2017/02/14 at 02:34:31, boliu wrote: > where is this color from? It's the background color of the interstitial HTML. I found it in this css file: https://cs.chromium.org/chromium/src/components/security_interstitials/core/b... 0xffce3426 is the hex representation of rgb(206, 52, 38) (the 'ff' is the alpha channel). I rewrote it as `Color.rgb(206, 52, 38)` and added an explanatory comment to improve clarity. Is there any way I can make this better? https://codereview.chromium.org/2691073003/diff/1/android_webview/javatests/s... android_webview/javatests/src/org/chromium/android_webview/test/SafeBrowsingTest.java:141: Thread.sleep(WAIT_TIMEOUT_MS); On 2017/02/14 at 02:34:32, boliu wrote: > no sleeps > > you can wait for startUriLookup to be called I wasn't sure how to implement what you suggested, so I wrote up something else that I think will be sufficient. This is replaced with waitForInterstitial, which polls until it sees an interstitial. This is based off AwTestBase#waitForPixelColorAtCenterOfView. https://codereview.chromium.org/2691073003/diff/1/android_webview/javatests/s... android_webview/javatests/src/org/chromium/android_webview/test/SafeBrowsingTest.java:167: Thread.sleep(WAIT_TIMEOUT_MS); On 2017/02/14 at 02:34:32, boliu wrote: > ditto Done
https://codereview.chromium.org/2691073003/diff/1/android_webview/javatests/s... File android_webview/javatests/src/org/chromium/android_webview/test/SafeBrowsingTest.java (right): https://codereview.chromium.org/2691073003/diff/1/android_webview/javatests/s... android_webview/javatests/src/org/chromium/android_webview/test/SafeBrowsingTest.java:113: return (bitmap.getPixel(0, 0) == INTERSTITIAL_COLOR); On 2017/02/14 17:17:55, Nate Fischer wrote: > On 2017/02/14 at 02:34:31, boliu wrote: > > where is this color from? > > It's the background color of the interstitial HTML. I found it in this css file: > > https://cs.chromium.org/chromium/src/components/security_interstitials/core/b... > > 0xffce3426 is the hex representation of rgb(206, 52, 38) (the 'ff' is the alpha > channel). > > I rewrote it as `Color.rgb(206, 52, 38)` and added an explanatory comment to > improve clarity. Is there any way I can make this better? It's not ok to depend on arbitrary values in far off code like that. Imagine if someone is redesigning the interstitial later and suddenly these tests start breaking. Besides, how do you even know 0,0 is the background color and not something else? If you just want to check interstitial exists, then you want to wait for the signal that an interstitial is created. If you really want to check interstitial is being rendered, then you'll have to somehow override the interstitial html in the tes here. https://codereview.chromium.org/2691073003/diff/20001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/AwTestBase.java (right): https://codereview.chromium.org/2691073003/diff/20001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwTestBase.java:97: protected void createAwBrowserContext() { ideally this would be made private as well, are there tests that legitimately can't be moved to createAwBrowserContextHelper? https://codereview.chromium.org/2691073003/diff/20001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwTestBase.java:112: protected void createAwBrowserContextHelper( s/Helper/onUiThread/ https://codereview.chromium.org/2691073003/diff/20001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwTestBase.java:117: protected void setBrowserContext(AwBrowserContext browserContext) { this should be private (or shouldn't exist at all), just have createAwBrowserContextHelper return the context, and set it in createAwBrowserContext https://codereview.chromium.org/2691073003/diff/20001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/SafeBrowsingTest.java (right): https://codereview.chromium.org/2691073003/diff/20001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/SafeBrowsingTest.java:117: pollUiThread(new Callable<Boolean>() { Always prefer the CallbackHelper pattern, which just waits for a signal, over polling.
Thanks for explanation. As we discussed in person, I think I actually need to check that the interstitial is being rendered, or that we're using the right compositor, or something like that. I will investigate the options and see what I can come up with. I will ping you when I have found a more robust way to check for interstitials. https://codereview.chromium.org/2691073003/diff/20001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/AwTestBase.java (right): https://codereview.chromium.org/2691073003/diff/20001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwTestBase.java:97: protected void createAwBrowserContext() { On 2017/02/14 at 18:31:40, boliu wrote: > ideally this would be made private as well, are there tests that legitimately can't be moved to createAwBrowserContextHelper? No other test overrides this function, but AwStrictModeTest.java invokes this function. I can't think of a way to make this private without adding obscurity. If you have a suggestion in mind, please let me know. https://codereview.chromium.org/2691073003/diff/20001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwTestBase.java:112: protected void createAwBrowserContextHelper( On 2017/02/14 at 18:31:40, boliu wrote: > s/Helper/onUiThread/ Done https://codereview.chromium.org/2691073003/diff/20001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwTestBase.java:117: protected void setBrowserContext(AwBrowserContext browserContext) { On 2017/02/14 at 18:31:40, boliu wrote: > this should be private (or shouldn't exist at all), just have createAwBrowserContextHelper return the context, and set it in createAwBrowserContext Done https://codereview.chromium.org/2691073003/diff/20001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/SafeBrowsingTest.java (right): https://codereview.chromium.org/2691073003/diff/20001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/SafeBrowsingTest.java:117: pollUiThread(new Callable<Boolean>() { On 2017/02/14 at 18:31:40, boliu wrote: > Always prefer the CallbackHelper pattern, which just waits for a signal, over polling. Ack. If I've understood you correctly, polling is necessary if I'm checking actual rgb values, while CallbackHelper is preferred for other use cases. I will try to control the rgb value in the interstitial, otherwise I will find a way to switch to waiting for the signal. If I've misunderstood you, and I should use CallbackHelper even for checking rgb values, please correct me.
The CQ bit was checked by ntfschr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2691073003/diff/20001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/AwTestBase.java (right): https://codereview.chromium.org/2691073003/diff/20001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwTestBase.java:97: protected void createAwBrowserContext() { On 2017/02/14 19:42:55, Nate Fischer wrote: > On 2017/02/14 at 18:31:40, boliu wrote: > > ideally this would be made private as well, are there tests that legitimately > can't be moved to createAwBrowserContextHelper? > > No other test overrides this function, but AwStrictModeTest.java invokes this > function. I can't think of a way to make this private without adding obscurity. > If you have a suggestion in mind, please let me know. Ok. This is fine then. https://codereview.chromium.org/2691073003/diff/20001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/SafeBrowsingTest.java (right): https://codereview.chromium.org/2691073003/diff/20001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/SafeBrowsingTest.java:117: pollUiThread(new Callable<Boolean>() { On 2017/02/14 19:42:55, Nate Fischer wrote: > On 2017/02/14 at 18:31:40, boliu wrote: > > Always prefer the CallbackHelper pattern, which just waits for a signal, over > polling. > > Ack. If I've understood you correctly, polling is necessary if I'm checking > actual rgb values, while CallbackHelper is preferred for other use cases. I will > try to control the rgb value in the interstitial, otherwise I will find a way to > switch to waiting for the signal. > > If I've misunderstood you, and I should use CallbackHelper even for checking rgb > values, please correct me. Yep that's all correct
The CQ bit was checked by ntfschr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by ntfschr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by ntfschr@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...
PTAL. No more polling. The malicious-subresource test is broken now though. The target page is visible instead of the interstitial. https://codereview.chromium.org/2691073003/diff/80001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/SafeBrowsingTest.java (right): https://codereview.chromium.org/2691073003/diff/80001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/SafeBrowsingTest.java:170: waitForPageToNotBeColor(COLOR_BLACK); It's necessary to wait for both colors to disappear, since for subresources we typically load the main frame successfully and then create an interstitial after realizing the subresource is malicious. If we only waitForPageToNotBeColor(COLOR_GREEN) then we'll still be seeing the target page, not the interstitial.
Sorry, the last reply included a draft I forgot to remove https://codereview.chromium.org/2691073003/diff/80001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/SafeBrowsingTest.java (right): https://codereview.chromium.org/2691073003/diff/80001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/SafeBrowsingTest.java:170: waitForPageToNotBeColor(COLOR_BLACK); On 2017/02/15 at 22:53:20, Nate Fischer wrote: > It's necessary to wait for both colors to disappear, since for subresources we typically load the main frame successfully and then create an interstitial after realizing the subresource is malicious. If we only waitForPageToNotBeColor(COLOR_GREEN) then we'll still be seeing the target page, not the interstitial. Ignore this comment. It's from a previous patchset.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by ntfschr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by ntfschr@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...
ntfschr@chromium.org changed reviewers: + vakh@chromium.org
+vakh@ for the DEPS change (our safebrowsing tests need to depend on components/safe_browsing_db) boliu@: PTAL. I switched to the approach we talked about with divs instead of body.background-color. I also decided to change the iframe to be super-small, and now I can safely read the pixel value from the center of the view. I noticed that chrome adds a slight border around the div, so depending on the center sounds safer than depending on the corner. Let me know if you think `getPixelColorAtCenterOfView()` should be protected, or should be private within SafeBrowsingTest. I just followed `waitForPixelColorAtCenterOfView()`.
https://codereview.chromium.org/2691073003/diff/140001/android_webview/javate... File android_webview/javatests/src/org/chromium/android_webview/test/AwTestBase.java (right): https://codereview.chromium.org/2691073003/diff/140001/android_webview/javate... android_webview/javatests/src/org/chromium/android_webview/test/AwTestBase.java:375: public int getPixelColorAtCenterOfView( I think this can just be static, in which case just put it in GraphicsTestUtils
On 2017/02/21 21:37:48, boliu wrote: > https://codereview.chromium.org/2691073003/diff/140001/android_webview/javate... > File > android_webview/javatests/src/org/chromium/android_webview/test/AwTestBase.java > (right): > > https://codereview.chromium.org/2691073003/diff/140001/android_webview/javate... > android_webview/javatests/src/org/chromium/android_webview/test/AwTestBase.java:375: > public int getPixelColorAtCenterOfView( > I think this can just be static, in which case just put it in GraphicsTestUtils and lgtm after that
The CQ bit was checked by ntfschr@chromium.org to run a CQ dry run
https://codereview.chromium.org/2691073003/diff/140001/android_webview/javate... File android_webview/javatests/src/org/chromium/android_webview/test/AwTestBase.java (right): https://codereview.chromium.org/2691073003/diff/140001/android_webview/javate... android_webview/javatests/src/org/chromium/android_webview/test/AwTestBase.java:375: public int getPixelColorAtCenterOfView( On 2017/02/21 at 21:37:47, boliu wrote: > I think this can just be static, in which case just put it in GraphicsTestUtils Done
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: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
DEPS lgtm
The CQ bit was checked by ntfschr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org Link to the patchset: https://codereview.chromium.org/2691073003/#ps160001 (title: "Move getPixelColorAtCenterOfView to GraphicsTestUtils")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1487808958558710,
"parent_rev": "863728ec53cb174d1329fae8770b1cd7ac51a26d", "commit_rev":
"f7c3ed28c88cf8f5e30a3879c44005d527ea11a6"}
Message was sent while issue was closed.
Description was changed from ========== Add javatests for the WebView SafeBrowsing feature This adds javatests for the SafeBrowsing feature in webview. It tests: * Loading a safe URL does not show an interstitial * Loading a malicious URL does show an interstitial * Loading a page with a malicious subresource does show an interstitial BUG=676737 ========== to ========== Add javatests for the WebView SafeBrowsing feature This adds javatests for the SafeBrowsing feature in webview. It tests: * Loading a safe URL does not show an interstitial * Loading a malicious URL does show an interstitial * Loading a page with a malicious subresource does show an interstitial BUG=676737 Review-Url: https://codereview.chromium.org/2691073003 Cr-Commit-Position: refs/heads/master@{#452385} Committed: https://chromium.googlesource.com/chromium/src/+/f7c3ed28c88cf8f5e30a3879c440... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/f7c3ed28c88cf8f5e30a3879c440... |
