[Android WebView] Filter out error page URL from onPageFinished to un-flake tests

[Android WebView] Filter out error page URL from onPageFinished to un-flake tests

The three tests being flaky (testContentUrlAccessWithTwoViews, testFileUrlAccessWithTwoViews and testCacheModeWithTwoViews) are all implicitly rely on the fact that when a page load error happens, onPageFinished is only called once. This behaviour had been broken since about M33, but due to happy timing, it just didn't cause the tests to fail.

The solution is to filter onPageFinished callbacks on AwContentsClient side, since the flow of passing the URL of the error page appears perfectly normal in the context of Chrome.

Other changes:

  -- testContentUrlAccessWithTwoViews was in fact incorrect, as it wasn't expecting an error in case when access to content: URLs is blocked;

  -- modified ClientOnPageFinishedTest.testOnPageFinishedCalledAfterError to catch situations when onPageFinished / onReceivedError is called twice, to avoid future regressions;

  -- added ClientOnPageStartedTest to catch similar problems with onPageStarted.

BUG=370950, 371983
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=277787

[mkosiba (inactive), 2014-05-12 14:41:42]

Maybe I'm misunderstanding your explanation but it seems to me that these aren't
just test flakes. If the embedder sets the setting to a particular value and
then calls loadUrl they will expect the setting to affect the subsequent loadUrl
calls.

In terms of memory consistency 
 - T1 sets settings.value to v2
 - T1 calls loadUrl by posting to T2
 - T2 reads settings.value
   T2 should observe settings.value == v2 since the read happens after the post
which happens after the write. So I don't think there is any good reason for the
throttle to be seeing the wrong value.

[mnaganov (inactive), 2014-05-13 05:47:10]

On 2014/05/12 14:41:42, mkosiba wrote:
> Maybe I'm misunderstanding your explanation but it seems to me that these
aren't
> just test flakes. If the embedder sets the setting to a particular value and
> then calls loadUrl they will expect the setting to affect the subsequent
loadUrl
> calls.
> 
> In terms of memory consistency 
>  - T1 sets settings.value to v2
>  - T1 calls loadUrl by posting to T2
>  - T2 reads settings.value
>    T2 should observe settings.value == v2 since the read happens after the
post
> which happens after the write. So I don't think there is any good reason for
the
> throttle to be seeing the wrong value.

Yeah, sorry, I have made a wrong assumption. It's all fine with checking flag
values in our throttles.

Anyway, loading "about:blank" seems to help, as well as a delay :). Looks like
there is a race somewhere causing resource loader to drop our stream reading
requests. I'm investigating.

[mnaganov (inactive), 2014-06-06 10:53:38]

Marcin, PTAL!

This time looks like a working solution to return back to behaviour of M30.

[mnaganov (inactive), 2014-06-06 13:35:38]

On 2014/06/06 10:53:38, Mikhail Naganov (Cr) wrote:
> Marcin, PTAL!
> 
> This time looks like a working solution to return back to behaviour of M30.

OK, not as easy as I was hoping...

[mkosiba (inactive), 2014-06-06 14:25:04]

lgtm

https://codereview.chromium.org/281563002/diff/20001/android_webview/javatest...
File
android_webview/javatests/src/org/chromium/android_webview/test/ClientOnPageFinishedTest.java
(right):

https://codereview.chromium.org/281563002/diff/20001/android_webview/javatest...
android_webview/javatests/src/org/chromium/android_webview/test/ClientOnPageFinishedTest.java:54:
class OurTestAwContentsClient extends TestAwContentsClient {
nit: maybe LocalTestAwContentsClient? or just LocalTestClient ?

[mnaganov (inactive), 2014-06-16 15:00:42]

https://codereview.chromium.org/281563002/diff/20001/android_webview/javatest...
File
android_webview/javatests/src/org/chromium/android_webview/test/ClientOnPageFinishedTest.java
(right):

https://codereview.chromium.org/281563002/diff/20001/android_webview/javatest...
android_webview/javatests/src/org/chromium/android_webview/test/ClientOnPageFinishedTest.java:54:
class OurTestAwContentsClient extends TestAwContentsClient {
On 2014/06/06 14:25:04, mkosiba wrote:
> nit: maybe LocalTestAwContentsClient? or just LocalTestClient ?

Done.

[mnaganov (inactive), 2014-06-16 18:16:03]

Marcus, can you please do an OWNER's review for content/browser/android and
content/public/android?

[bulach, 2014-06-17 00:42:13]

some comments below:

https://codereview.chromium.org/281563002/diff/40001/android_webview/java/src...
File android_webview/java/src/org/chromium/android_webview/AwContentsClient.java
(right):

https://codereview.chromium.org/281563002/diff/40001/android_webview/java/src...
android_webview/java/src/org/chromium/android_webview/AwContentsClient.java:65:
if (isMainFrame && !isErrorPage) {
see below, I suppose we can filter at this level just based on validatedUrl?

https://codereview.chromium.org/281563002/diff/40001/content/browser/android/...
File content/browser/android/web_contents_observer_android.cc (right):

https://codereview.chromium.org/281563002/diff/40001/content/browser/android/...
content/browser/android/web_contents_observer_android.cc:227:
ConvertUTF8ToJavaString(env, url_string));
hmm.... I'm not quite sure about the approach here... the param "is_error_page"
on a method called "didFinishLoad" can be *fairly* ambiguous, I was originally
thinking about interstitial pages, and what not...
since it's just testing the particular value for the URL it receives on tests,
rather than changing the API at this level, how about just changing higher up to
detect something like "isUnreacheableWebDataUrl()" or something? would that
simplify things?

[mnaganov (inactive), 2014-06-17 09:52:15]

Thanks, Marcus!

...and with your comments addressed, I don't need your owner's approval anymore
:P

https://codereview.chromium.org/281563002/diff/40001/android_webview/java/src...
File android_webview/java/src/org/chromium/android_webview/AwContentsClient.java
(right):

https://codereview.chromium.org/281563002/diff/40001/android_webview/java/src...
android_webview/java/src/org/chromium/android_webview/AwContentsClient.java:65:
if (isMainFrame && !isErrorPage) {
On 2014/06/17 00:42:12, bulach wrote:
> see below, I suppose we can filter at this level just based on validatedUrl?

Done.

https://codereview.chromium.org/281563002/diff/40001/content/browser/android/...
File content/browser/android/web_contents_observer_android.cc (right):

https://codereview.chromium.org/281563002/diff/40001/content/browser/android/...
content/browser/android/web_contents_observer_android.cc:227:
ConvertUTF8ToJavaString(env, url_string));
On 2014/06/17 00:42:12, bulach wrote:
> hmm.... I'm not quite sure about the approach here... the param
"is_error_page"
> on a method called "didFinishLoad" can be *fairly* ambiguous, I was originally
> thinking about interstitial pages, and what not...
> since it's just testing the particular value for the URL it receives on tests,
> rather than changing the API at this level, how about just changing higher up
to
> detect something like "isUnreacheableWebDataUrl()" or something? would that
> simplify things?

Yes, sounds reasonable. Sorry for not making that initially.

[mnaganov (inactive), 2014-06-17 09:52:37]

The CQ bit was checked by mnaganov@chromium.org

[commit-bot: I haz the power, 2014-06-17 09:54:44]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mnaganov@chromium.org/281563002/60001

[commit-bot: I haz the power, 2014-06-17 12:42:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-17 12:42:54]

Try jobs failed on following builders:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)

[mnaganov (inactive), 2014-06-17 12:50:42]

The CQ bit was checked by mnaganov@chromium.org

[commit-bot: I haz the power, 2014-06-17 12:51:20]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mnaganov@chromium.org/281563002/80001

[bulach, 2014-06-17 16:26:37]

On 2014/06/17 09:52:15, Mikhail Naganov (Cr) wrote:
> Thanks, Marcus!
> 
> ...and with your comments addressed, I don't need your owner's approval
anymore
> :P

:)

oh, and of course, lgtm, thanks!

[commit-bot: I haz the power, 2014-06-17 17:00:45]

Change committed as 277787

[boliu, 2014-06-17 21:22:56]

Getting some flakes with
ClientOnPageFinishedTest#testOnPageFinishedCalledAfterError but maybe not enough
to disable yet:

http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...
http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...

1a8bb:  06-17 20:54:31.848 22795 22795 W System.err:
junit.framework.AssertionFailedError: onPageFinished called twice for
data:text/html,chromewebdata expected:<false> but was:<true>
1a8bb:  06-17 20:54:31.848 22795 22795 W System.err: 	at
junit.framework.Assert.fail(Assert.java:50)
1a8bb:  06-17 20:54:31.848 22795 22795 W System.err: 	at
junit.framework.Assert.failNotEquals(Assert.java:287)
1a8bb:  06-17 20:54:31.848 22795 22795 W System.err: 	at
junit.framework.Assert.assertEquals(Assert.java:67)
1a8bb:  06-17 20:54:31.858 22795 22795 W System.err: 	at
junit.framework.Assert.assertEquals(Assert.java:147)
1a8bb:  06-17 20:54:31.858 22795 22795 W System.err: 	at
org.chromium.android_webview.test.ClientOnPageFinishedTest$1LocalTestClient.onPageFinished(ClientOnPageFinishedTest.java:75)
1a8bb:  06-17 20:54:31.858 22795 22795 W System.err: 	at
org.chromium.android_webview.AwContentsClient$AwWebContentsObserver.didFailLoad(AwContentsClient.java:89)
1a8bb:  06-17 20:54:31.858 22795 22795 W System.err: 	at
org.chromium.base.SystemMessageHandler.nativeDoRunLoopOnce(Native Method)
1a8bb:  06-17 20:54:31.858 22795 22795 W System.err: 	at
org.chromium.base.SystemMessageHandler.handleMessage(SystemMessageHandler.java:28)
1a8bb:  06-17 20:54:31.858 22795 22795 W System.err: 	at
android.os.Handler.dispatchMessage(Handler.java:102)
1a8bb:  06-17 20:54:31.858 22795 22795 W System.err: 	at
android.os.Looper.loop(Looper.java:136)
1a8bb:  06-17 20:54:31.858 22795 22795 W System.err: 	at
android.app.ActivityThread.main(ActivityThread.java:5017)
1a8bb:  06-17 20:54:31.858 22795 22795 W System.err: 	at
java.lang.reflect.Method.invokeNative(Native Method)
1a8bb:  06-17 20:54:31.858 22795 22795 W System.err: 	at
java.lang.reflect.Method.invoke(Method.java:515)
1a8bb:  06-17 20:54:31.858 22795 22795 W System.err: 	at
com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:779)
1a8bb:  06-17 20:54:31.858 22795 22795 W System.err: 	at
com.android.internal.os.ZygoteInit.main(ZygoteInit.java:595)
1a8bb:  06-17 20:54:31.858 22795 22795 W System.err: 	at
dalvik.system.NativeStart.main(Native Method)

[mnaganov (inactive), 2014-06-18 08:33:20]

Yes, I see 2 failures on the flakiness dashboard. I'll try doing an
endurance run for the test, maybe it will reveal something.


On Tue, Jun 17, 2014 at 10:22 PM, <boliu@chromium.org> wrote:

> Getting some flakes with
> ClientOnPageFinishedTest#testOnPageFinishedCalledAfterError but maybe not
> enough
> to disable yet:
>
> http://build.chromium.org/p/tryserver.chromium/builders/
> android_dbg_triggered_tests/builds/163323
> http://build.chromium.org/p/tryserver.chromium/builders/
> android_dbg_triggered_tests/builds/163330
>
> 1a8bb:  06-17 20:54:31.848 22795 22795 W System.err:
> junit.framework.AssertionFailedError: onPageFinished called twice for
> data:text/html,chromewebdata expected:<false> but was:<true>
> 1a8bb:  06-17 20:54:31.848 22795 22795 W System.err:    at
> junit.framework.Assert.fail(Assert.java:50)
> 1a8bb:  06-17 20:54:31.848 22795 22795 W System.err:    at
> junit.framework.Assert.failNotEquals(Assert.java:287)
> 1a8bb:  06-17 20:54:31.848 22795 22795 W System.err:    at
> junit.framework.Assert.assertEquals(Assert.java:67)
> 1a8bb:  06-17 20:54:31.858 22795 22795 W System.err:    at
> junit.framework.Assert.assertEquals(Assert.java:147)
> 1a8bb:  06-17 20:54:31.858 22795 22795 W System.err:    at
> org.chromium.android_webview.test.ClientOnPageFinishedTest$
> 1LocalTestClient.onPageFinished(ClientOnPageFinishedTest.java:75)
> 1a8bb:  06-17 20:54:31.858 22795 22795 W System.err:    at
> org.chromium.android_webview.AwContentsClient$AwWebContentsObserver.
> didFailLoad(AwContentsClient.java:89)
> 1a8bb:  06-17 20:54:31.858 22795 22795 W System.err:    at
> org.chromium.base.SystemMessageHandler.nativeDoRunLoopOnce(Native Method)
> 1a8bb:  06-17 20:54:31.858 22795 22795 W System.err:    at
> org.chromium.base.SystemMessageHandler.handleMessage(
> SystemMessageHandler.java:28)
> 1a8bb:  06-17 20:54:31.858 22795 22795 W System.err:    at
> android.os.Handler.dispatchMessage(Handler.java:102)
> 1a8bb:  06-17 20:54:31.858 22795 22795 W System.err:    at
> android.os.Looper.loop(Looper.java:136)
> 1a8bb:  06-17 20:54:31.858 22795 22795 W System.err:    at
> android.app.ActivityThread.main(ActivityThread.java:5017)
> 1a8bb:  06-17 20:54:31.858 22795 22795 W System.err:    at
> java.lang.reflect.Method.invokeNative(Native Method)
> 1a8bb:  06-17 20:54:31.858 22795 22795 W System.err:    at
> java.lang.reflect.Method.invoke(Method.java:515)
> 1a8bb:  06-17 20:54:31.858 22795 22795 W System.err:    at
> com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(
> ZygoteInit.java:779)
> 1a8bb:  06-17 20:54:31.858 22795 22795 W System.err:    at
> com.android.internal.os.ZygoteInit.main(ZygoteInit.java:595)
> 1a8bb:  06-17 20:54:31.858 22795 22795 W System.err:    at
> dalvik.system.NativeStart.main(Native Method)
>
> https://codereview.chromium.org/281563002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mnaganov (inactive), 2014-06-19 07:16:41]

OK, so onPageFinished is definitely flaky: http://code.google.com/p/
chromium/issues/detail?id=386300


On Wed, Jun 18, 2014 at 9:32 AM, Mikhail Naganov <mnaganov@chromium.org>
wrote:

> Yes, I see 2 failures on the flakiness dashboard. I'll try doing an
> endurance run for the test, maybe it will reveal something.
>
>
> On Tue, Jun 17, 2014 at 10:22 PM, <boliu@chromium.org> wrote:
>
>> Getting some flakes with
>> ClientOnPageFinishedTest#testOnPageFinishedCalledAfterError but maybe
>> not enough
>> to disable yet:
>>
>> http://build.chromium.org/p/tryserver.chromium/builders/
>> android_dbg_triggered_tests/builds/163323
>> http://build.chromium.org/p/tryserver.chromium/builders/
>> android_dbg_triggered_tests/builds/163330
>>
>> 1a8bb:  06-17 20:54:31.848 22795 22795 W System.err:
>> junit.framework.AssertionFailedError: onPageFinished called twice for
>> data:text/html,chromewebdata expected:<false> but was:<true>
>> 1a8bb:  06-17 20:54:31.848 22795 22795 W System.err:    at
>> junit.framework.Assert.fail(Assert.java:50)
>> 1a8bb:  06-17 20:54:31.848 22795 22795 W System.err:    at
>> junit.framework.Assert.failNotEquals(Assert.java:287)
>> 1a8bb:  06-17 20:54:31.848 22795 22795 W System.err:    at
>> junit.framework.Assert.assertEquals(Assert.java:67)
>> 1a8bb:  06-17 20:54:31.858 22795 22795 W System.err:    at
>> junit.framework.Assert.assertEquals(Assert.java:147)
>> 1a8bb:  06-17 20:54:31.858 22795 22795 W System.err:    at
>> org.chromium.android_webview.test.ClientOnPageFinishedTest$
>> 1LocalTestClient.onPageFinished(ClientOnPageFinishedTest.java:75)
>> 1a8bb:  06-17 20:54:31.858 22795 22795 W System.err:    at
>> org.chromium.android_webview.AwContentsClient$AwWebContentsObserver.
>> didFailLoad(AwContentsClient.java:89)
>> 1a8bb:  06-17 20:54:31.858 22795 22795 W System.err:    at
>> org.chromium.base.SystemMessageHandler.nativeDoRunLoopOnce(Native Method)
>> 1a8bb:  06-17 20:54:31.858 22795 22795 W System.err:    at
>> org.chromium.base.SystemMessageHandler.handleMessage(
>> SystemMessageHandler.java:28)
>> 1a8bb:  06-17 20:54:31.858 22795 22795 W System.err:    at
>> android.os.Handler.dispatchMessage(Handler.java:102)
>> 1a8bb:  06-17 20:54:31.858 22795 22795 W System.err:    at
>> android.os.Looper.loop(Looper.java:136)
>> 1a8bb:  06-17 20:54:31.858 22795 22795 W System.err:    at
>> android.app.ActivityThread.main(ActivityThread.java:5017)
>> 1a8bb:  06-17 20:54:31.858 22795 22795 W System.err:    at
>> java.lang.reflect.Method.invokeNative(Native Method)
>> 1a8bb:  06-17 20:54:31.858 22795 22795 W System.err:    at
>> java.lang.reflect.Method.invoke(Method.java:515)
>> 1a8bb:  06-17 20:54:31.858 22795 22795 W System.err:    at
>> com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(
>> ZygoteInit.java:779)
>> 1a8bb:  06-17 20:54:31.858 22795 22795 W System.err:    at
>> com.android.internal.os.ZygoteInit.main(ZygoteInit.java:595)
>> 1a8bb:  06-17 20:54:31.858 22795 22795 W System.err:    at
>> dalvik.system.NativeStart.main(Native Method)
>>
>> https://codereview.chromium.org/281563002/
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.