[AndroidWebView] Add instrumentation test to catch StrictMode violations

[AndroidWebView] Add instrumentation test to catch StrictMode violations

Since WebView is an Android component we don't want WebView startup to
cause any StrictMode violations.

BUG=628650
Committed: https://crrev.com/872e1ba031bf6c7ea6ae39f10a23f80e34db6791
Cr-Commit-Position: refs/heads/master@{#424148}

[gsennton, 2016-09-14 17:17:58]

gsennton@chromium.org changed reviewers:
+ torne@chromium.org

[gsennton, 2016-09-14 17:18:05]

AFAICT we are running each instrumentation test case in a different process - is
this guaranteed? (to ensure we aren't falling into the trap of some state
already being initialized during subsequent tests, like we did for CTS :P).

Note: testStartup currently passes while testStartupWithoutPresetAppRestrictions
 fails because of crbug.com/628627 so if you (Nicolas or Anthony) want to fix
crbug.com/628627 you can use these tests to check whether your changes fix the
strictmode violation.


Btw, do we care about strictmode violation during use of WebView? I.e. not only
startup - should we call some random apis to try to catch such violations?

https://codereview.chromium.org/2337983004/diff/1/android_webview/javatests/s...
File
android_webview/javatests/src/org/chromium/android_webview/test/AwStartupStrictModeTest.java
(right):

https://codereview.chromium.org/2337983004/diff/1/android_webview/javatests/s...
android_webview/javatests/src/org/chromium/android_webview/test/AwStartupStrictModeTest.java:39:
AbstractAppRestrictionsProvider.setTestRestrictions(null);
Nicolas, is this valid use of
AbstractAppRestrictionsProvider.setTestRestrictions?
The current strictmode violation over in crbug.com/628627 is only triggered when
test restrictions are null so I believe we should include this test case?

https://codereview.chromium.org/2337983004/diff/1/android_webview/javatests/s...
android_webview/javatests/src/org/chromium/android_webview/test/AwStartupStrictModeTest.java:51:
.detectDiskReads()
Only detecting disk reads here... should we detect other stuff as well? (e.g.
disk writes)

https://codereview.chromium.org/2337983004/diff/1/android_webview/javatests/s...
android_webview/javatests/src/org/chromium/android_webview/test/AwStartupStrictModeTest.java:57:
AwBrowserProcess.start();
Torne, I'm not sure whether there is anything else we should be calling here to
simulate WebView startup as well as possible, any thoughts?

[Torne, 2016-09-20 13:41:20]

> AFAICT we are running each instrumentation test case in a different process -
is
> this guaranteed? (to ensure we aren't falling into the trap of some state
> already being initialized during subsequent tests, like we did for CTS :P).

I think this is supposed to be the case but I am not sure how this is
guaranteed. What happens if AwBrowserProcess gets started twice? If that already
crashes then we don't have to worry about this test passing incorrectly. :)

> Btw, do we care about strictmode violation during use of WebView? I.e. not
only
> startup - should we call some random apis to try to catch such violations?

Yes, we do; we should probably trigger a navigation at least, dunno how much
it's worth calling random stuff as we won't ever have a lot of coverage, but it
doesn't hurt to try a few things.

https://codereview.chromium.org/2337983004/diff/1/android_webview/javatests/s...
File
android_webview/javatests/src/org/chromium/android_webview/test/AwStartupStrictModeTest.java
(right):

https://codereview.chromium.org/2337983004/diff/1/android_webview/javatests/s...
android_webview/javatests/src/org/chromium/android_webview/test/AwStartupStrictModeTest.java:51:
.detectDiskReads()
On 2016/09/14 17:18:04, gsennton_OOO_until_9.21 wrote:
> Only detecting disk reads here... should we detect other stuff as well? (e.g.
> disk writes)

Yeah, apps might have *everything* enabled, not just disk access. We have bugs
about detectLeakedCloseableObjects for example.

For thread policy it's probably okay to just do detectAll(). For VM policy I'm
not so sure as I'm not familiar with what those do, but we could try detectAll
and see what happens?

https://codereview.chromium.org/2337983004/diff/1/android_webview/javatests/s...
android_webview/javatests/src/org/chromium/android_webview/test/AwStartupStrictModeTest.java:57:
AwBrowserProcess.start();
On 2016/09/14 17:18:04, gsennton_OOO_until_9.21 wrote:
> Torne, I'm not sure whether there is anything else we should be calling here
to
> simulate WebView startup as well as possible, any thoughts?

Not sure; we should just imitate as best we can what other tests do that do
basic startup + initial navigation maybe?

[dgn, 2016-09-22 09:37:40]

dgn@chromium.org changed reviewers:
+ dgn@chromium.org

[dgn, 2016-09-22 09:37:47]

https://codereview.chromium.org/2337983004/diff/1/android_webview/javatests/s...
File
android_webview/javatests/src/org/chromium/android_webview/test/AwStartupStrictModeTest.java
(right):

https://codereview.chromium.org/2337983004/diff/1/android_webview/javatests/s...
android_webview/javatests/src/org/chromium/android_webview/test/AwStartupStrictModeTest.java:39:
AbstractAppRestrictionsProvider.setTestRestrictions(null);
On 2016/09/14 17:18:04, gsennton wrote:
> Nicolas, is this valid use of
> AbstractAppRestrictionsProvider.setTestRestrictions?
> The current strictmode violation over in crbug.com/628627 is only triggered
when
> test restrictions are null so I believe we should include this test case?

As discussed, it's probably better to force the tests to use the default
behaviour everywhere, so setting the test bundle to null instead of empty in all
tests by default. I should just be a matter of returning null instead of an
empty map in Policies#getPolicies() when the Policy annotation is not provided,
and reflecting it through PolicyData#asBundle. This is so that
Policy.RegistrationHook#run automatically sets the test bundle to null

[gsennton, 2016-09-22 12:34:47]

Calling AwBrowserProcess.start() twice indeed causes a crash
(java.nio.channels.OverlappingFileLockException), so that's fine :).

https://codereview.chromium.org/2337983004/diff/1/android_webview/javatests/s...
File
android_webview/javatests/src/org/chromium/android_webview/test/AwStartupStrictModeTest.java
(right):

https://codereview.chromium.org/2337983004/diff/1/android_webview/javatests/s...
android_webview/javatests/src/org/chromium/android_webview/test/AwStartupStrictModeTest.java:39:
AbstractAppRestrictionsProvider.setTestRestrictions(null);
On 2016/09/22 09:37:47, dgn wrote:
> On 2016/09/14 17:18:04, gsennton wrote:
> > Nicolas, is this valid use of
> > AbstractAppRestrictionsProvider.setTestRestrictions?
> > The current strictmode violation over in crbug.com/628627 is only triggered
> when
> > test restrictions are null so I believe we should include this test case?
> 
> As discussed, it's probably better to force the tests to use the default
> behaviour everywhere, so setting the test bundle to null instead of empty in
all
> tests by default. I should just be a matter of returning null instead of an
> empty map in Policies#getPolicies() when the Policy annotation is not
provided,
> and reflecting it through PolicyData#asBundle. This is so that
> Policy.RegistrationHook#run automatically sets the test bundle to null 

Done (not exactly as you describe here, but with the same outcome).

https://codereview.chromium.org/2337983004/diff/1/android_webview/javatests/s...
android_webview/javatests/src/org/chromium/android_webview/test/AwStartupStrictModeTest.java:51:
.detectDiskReads()
On 2016/09/20 13:41:20, Torne wrote:
> On 2016/09/14 17:18:04, gsennton_OOO_until_9.21 wrote:
> > Only detecting disk reads here... should we detect other stuff as well?
(e.g.
> > disk writes)
> 
> Yeah, apps might have *everything* enabled, not just disk access. We have bugs
> about detectLeakedCloseableObjects for example.
> 
> For thread policy it's probably okay to just do detectAll(). For VM policy I'm
> not so sure as I'm not familiar with what those do, but we could try detectAll
> and see what happens?

Doing detectAll on both of these seems to work (i.e. the tests pass).
It is weird that this doesn't trigger the detectLeakedCloseableObjects bug
though... (crbug.com/647291)

https://codereview.chromium.org/2337983004/diff/1/android_webview/javatests/s...
android_webview/javatests/src/org/chromium/android_webview/test/AwStartupStrictModeTest.java:57:
AwBrowserProcess.start();
On 2016/09/20 13:41:20, Torne wrote:
> On 2016/09/14 17:18:04, gsennton_OOO_until_9.21 wrote:
> > Torne, I'm not sure whether there is anything else we should be calling here
> to
> > simulate WebView startup as well as possible, any thoughts?
> 
> Not sure; we should just imitate as best we can what other tests do that do
> basic startup + initial navigation maybe?

Doing startup + loadData + setJavaScriptEnabled.

https://codereview.chromium.org/2337983004/diff/40001/android_webview/javates...
File
android_webview/javatests/src/org/chromium/android_webview/test/AwStartupStrictModeTest.java
(right):

https://codereview.chromium.org/2337983004/diff/40001/android_webview/javates...
android_webview/javatests/src/org/chromium/android_webview/test/AwStartupStrictModeTest.java:28:
// activity calls AwBrowserProcess.loadLibrary()
This is annoying (and I don't know why this doesn't happen in the real-world
case yet).
This points to yet another way we could miss a real-world strictmode violation
in an instrumentation test like this.

Maybe we should add some tests to Tim's WebView layout tests as well to catch
any glue-layer strictmode violations? (and have those act as a fallback that
runs on a waterfall, while these instrumentation tests catch as much as they can
on the CQ).

https://codereview.chromium.org/2337983004/diff/40001/android_webview/javates...
android_webview/javatests/src/org/chromium/android_webview/test/AwStartupStrictModeTest.java:109:
mAwTestContainerView = createAwTestContainerView(mContentsClient);
These objects are being set on the application main thread - but after calling
startEverythingSync we might want to use these objects on the instrumentation
thread.
Will the synchronization statements used to implement runOnMainSync (i.e.
waiting for some synchronized boolean which is set when the given runnable is
done) cause these changes to these objects to be correctly visible on the
instrumentation thread, or do I need to add explicit synchronization statements
here?

[gsennton, 2016-09-23 15:14:23]

Hah! Almost done here.. just need to figure out why we aren't hitting the
detectLeakedCloseableObjects thingy in these tests and whether the different
objects being set in during synchronous calls to the main thread in
AwStartupStrictModeTest require explicit synchronized-statements.

https://codereview.chromium.org/2337983004/diff/40001/android_webview/javates...
File
android_webview/javatests/src/org/chromium/android_webview/test/AwStartupStrictModeTest.java
(right):

https://codereview.chromium.org/2337983004/diff/40001/android_webview/javates...
android_webview/javatests/src/org/chromium/android_webview/test/AwStartupStrictModeTest.java:28:
// activity calls AwBrowserProcess.loadLibrary()
On 2016/09/22 12:34:47, gsennton wrote:
> This is annoying (and I don't know why this doesn't happen in the real-world
> case yet).
> This points to yet another way we could miss a real-world strictmode violation
> in an instrumentation test like this.
> 
> Maybe we should add some tests to Tim's WebView layout tests as well to catch
> any glue-layer strictmode violations? (and have those act as a fallback that
> runs on a waterfall, while these instrumentation tests catch as much as they
can
> on the CQ).

Alright, as discussed offline we don't hit in real-world cases because in
WebViewFactory we surround the entire WebViewChromiumFactoryProvider
initialization with StrictMode.allowDiskReads.

I've now surrounded the call to AwBrowserProcess.loadLibrary from
AwTestRunnerActivity with allowDiskReads.

[dgn, 2016-10-03 16:14:26]

https://codereview.chromium.org/2337983004/diff/1/android_webview/javatests/s...
File
android_webview/javatests/src/org/chromium/android_webview/test/AwStartupStrictModeTest.java
(right):

https://codereview.chromium.org/2337983004/diff/1/android_webview/javatests/s...
android_webview/javatests/src/org/chromium/android_webview/test/AwStartupStrictModeTest.java:39:
AbstractAppRestrictionsProvider.setTestRestrictions(null);
On 2016/09/22 12:34:46, gsennton wrote:
> On 2016/09/22 09:37:47, dgn wrote:
> > On 2016/09/14 17:18:04, gsennton wrote:
> > > Nicolas, is this valid use of
> > > AbstractAppRestrictionsProvider.setTestRestrictions?
> > > The current strictmode violation over in crbug.com/628627 is only
triggered
> > when
> > > test restrictions are null so I believe we should include this test case?
> > 
> > As discussed, it's probably better to force the tests to use the default
> > behaviour everywhere, so setting the test bundle to null instead of empty in
> all
> > tests by default. I should just be a matter of returning null instead of an
> > empty map in Policies#getPolicies() when the Policy annotation is not
> provided,
> > and reflecting it through PolicyData#asBundle. This is so that
> > Policy.RegistrationHook#run automatically sets the test bundle to null 
> 
> Done (not exactly as you describe here, but with the same outcome).

All right, I wanted to allow different behaviours if the annoation is not there
vs if it is there but withtout any data, but we don't need it for now so no need
to make thing unecesarily complicated. non owner lgtm for *policies*

[gsennton, 2016-10-07 16:40:37]

gsennton@chromium.org changed reviewers:
+ pastarmovj@chromium.org

[gsennton, 2016-10-07 16:40:38]

Adding pastarmovj@ for OWNER review of components/policy/

I think we can land this now.
This test doesn't hit the violation in crbug.com/647291, but I'm not convinced
we can hit that bug.. anyway if it turns out we can hit that violation in a test
we can just add that test then :P

[pastarmovj, 2016-10-10 10:35:37]

lgtm

[Torne, 2016-10-10 12:09:41]

lgtm

[gsennton, 2016-10-10 13:44:57]

The CQ bit was checked by gsennton@chromium.org

[gsennton, 2016-10-10 13:44:58]

The patchset sent to the CQ was uploaded after l-g-t-m from dgn@chromium.org
Link to the patchset: https://codereview.chromium.org/2337983004/#ps80001
(title: "Minor changes - class renaming + comment removal.")

[commit-bot: I haz the power, 2016-10-10 13:45:09]

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

[commit-bot: I haz the power, 2016-10-10 14:32:32]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-10-10 14:34:23]

Description was changed from

==========
[AndroidWebView] Add instrumentation test to catch StrictMode violations

Since WebView is an Android component we don't want WebView startup to
cause any StrictMode violations.

BUG=628650
==========

to

==========
[AndroidWebView] Add instrumentation test to catch StrictMode violations

Since WebView is an Android component we don't want WebView startup to
cause any StrictMode violations.

BUG=628650

Committed: https://crrev.com/872e1ba031bf6c7ea6ae39f10a23f80e34db6791
Cr-Commit-Position: refs/heads/master@{#424148}
==========

[commit-bot: I haz the power, 2016-10-10 14:34:24]

Patchset 5 (id:??) landed as
https://crrev.com/872e1ba031bf6c7ea6ae39f10a23f80e34db6791
Cr-Commit-Position: refs/heads/master@{#424148}