Please find the log and Video @ http://go/chrome-androidlogs1/7/766167

ntfschr@  Might be it looks like this issue is related to your change. please look into once, if its not related to your change please reassign to me. 
Thanks!

It's a rescent regression marking RBD, If not a blocker please feel free to change label.

Thanks!

In case it helps with triage the error message page says: The web page at <https://login.yahoo.com/account/challenge/...long url...> could not be loaded because: net::ERR_NAME_NOT_RESOLVED.

will do a quick investigation to see how far I can go here.

actually I don't think this is plz navigate. I got the same error with 61.0.3163.81.

himm false excitement. I have the --enable-browser-side-navigation in my flags. :(

net::ERR_NAME_NOT_RESOLVED is a network layer error and shouldn't be changing whether it is PlzNavigate or not. Does Yahoo Mail do anything custom with DNS resolution or interception?

Not sure about that. Is there a code path you would expect it to hit? I can add a log.

In general, I'd capture output of chrome://net-internals, but not sure whether this works with WebView at all. Also, if there is an issue with DNS resolution, that could explain some of the other bugs where things don't show up. In one of the logs I can see this:

09-18 19:41:11.717 W/chromium(16693): [WARNING:dns_config_service_posix.cc(330)] Failed to read DnsConfig.

If indeed DNS config is incorrect, I would expect failures to not only navigate, but to load subresources (e.g. the Facebook ads missing case).

I think this bug is related to loaddatawithbaseurl being broken under some
circumstances with plNavigate.

Re c#14, I also see a call to loadDataWithBaseUrl. It's much earlier in the login flow, but maybe it's to blame.

#13: we don't have net-internals unfortunately. it is a project in my mind for a long time but never get to it.

The error seems to be originating for https://mobileexchange.yahoo.com
which actually is a dns error. Now the question is why does this show itself only for plznavigate.

I have added net-log to Webview, the CL is under review.

Attaching the files for both cases when plznavigate is enabled vs. disabled. As you can see there is a connection attemp to https://mobileexchange.yahoo.com/ which is causing the error when plznavigate is enabled. The next step is finding out why :)

Deleted: disable_plznavigate.json 280 KB

disable_plznavigate.json 280 KB View Download

Deleted: enable_plznavigate.json 264 KB

enable_plznavigate.json 264 KB View Download

When I compare both net-logs, of the differences I see is in a URLRequest that ends in a 302 (see ID#61 in disable_plznavigate and ID#64 in enable_plznavigate.)
[Deleted lots of stuff]
In disable_plznavigate, I see a HTTP/1.1 302
                            status: 302
                            location: https://mobileexchange.yahoo.com/?... 

which ends up in --> net_error = -3 (ERR_ABORTED)

The same for enable_plznavigate ends up in  net_error = -2. Because it seems like a GET for https://mobileexchange happens. 
RL_REQUEST_START_JOB  [dt=105+]
                    --> load_flags = 37121 (MAIN_FRAME_DEPRECATED | MAYBE_USER_GESTURE | VALIDATE_CACHE | VERIFY_EV_CERT)
                    --> method = "GET"
                    --> url = "https://mobileexchange.yahoo.com/?slcc=zUmNi.r.2QLA3Xx7wzbU2OMfSQ1JCwWTZFpQosp1UOgQ3ekjbYxvbDjOHCXKFct

I don't see the same in disable_plznavigate. Rather it says:

t=80270 [st=163]        DELEGATE_INFO  [dt=6]
                        --> delegate_blocked_by = "NavigationResourceThrottle"

Intercepting and logging WebView lifecycle callbacks show that the app is overriding the url loading for https://mobileexchange.yahoo.com


09-19 18:26:05.114 18778 18778 I cr_WebViewCallback: shouldOverrideUrlLoading=https://mobileexchange.yahoo.com/? [...]
09-19 18:26:05.118 18778 18778 D AccountUtils: logging event: asdk_web_sign_in_override_url_load
09-19 18:26:05.124 18778 18778 I cr_WebViewCallback: shouldoverrideurlloading returning true

so it seems like the changes somehow broke that API. This is probably the most important API of WebView :)

ah, ok from the above I suspect what's happening is that RenderFrameImpl::DecidePolicyForNavigation isn't being called at all with the URL when PlzNavigate is enabled (can you double check?). NavigationRequest::OnRequestFailed, on seeing an net::ERR_ABORTED, would not go to the renderer and just cancel the navigation. Right now, shouldOverrideUrlLoading is only called when a commit gets to the renderer.

A small (mergeable) fix may be to call AwContentsClientBridge::ShouldOverrideUrlLoading in NavigationRequest::OnRequestFailed when we see an net::ERR_ABORTED through a new ContentBrowserClient Android-only interface.

Longer term, perhaps we should always be calling AwContentsClientBridge::ShouldOverrideUrlLoading in the browser when PlzNavigate is on. There's a slight behavior change with PlzNavigate at this point, since we will start fetching a request before we call to the webview app. Without PlzNavigate, we wouldn't make a network request at first afaik.

sure, I will try that. It is likely that this will fix some of the other bugs too.

Am I correct to assume that ShouldOverrideUrlLoading should be called for all navigations, be it browser or renderer initiated? If that is true, then the correct place would be to call out in NavigationRequest::BeginNavigation, as it is the common point for all navigations in PlzNavigate. We would also avoid making a sync IPC call to the browser process as part of the DecidePolicyForNavigation code.

For the first question: yes it should be called for all navigations.

Regarding if NavigationRequest::BeginNavigation is enough, I believe all redirects should also call shouldOverrideUrlLoading (right Selim?). With the old navigation code path, DecidePolicyForNavigation is called for redirects.

Lastly we should see if there's a small fix; i.e. I agree we should avoid sync IPCs where possible. But given that currently happens, at this point I think we should focus on switching webview to plznavigate first. Then once that's done, we can clean it up by removing sync IPCs.

For redirects, we just add the same callout in NavigationRequest::OnRequestRedirected :). I'm all for making webview work, which is why I think this is the easiest way and the most minimal patch.

Sure, if making this call from NavigationRequest is sufficient to fix this and doesn't need a lot of other changes, I'm all for it. I was just worried that it would have other implications; but you're right we should check that first :)

btw bug 325351 may be relevant. It reimplemented this API using a resource throttle, but that failed because of timing differences that apparently aren't covered by tests (the bug mentioned things like about:blank). So it may not be a trivial change to change how embedder is called to do it in the browser vs the renderer, specifically due to some URLs which commit and we don't go to the browser for even with plznavigate.

It is called for redirects. It is however called for unsupported schemes, such as mydomain:// so the app has a chance to override. If  app starts fetching before app has  a chance to override, I am afraid this will break.

Many of the cornercases are in https://cs.chromium.org/chromium/src/android_webview/renderer/aw_content_renderer_client.cc?rcl=825d142b62c01685beecefc7ff8d86024a90fc97&l=108)

Yes bug/325351 is relevant. our attempts failed there unfortunately. CC'ing gustav@ as he is probably interested in this discussion.

actually we already have tests for custom schemes. they should be passing plznavigate. I will first try Jam's idea. 

I had discussion with ntfschr@ (thanks, Nate!), since it is blocking tomorrow's M63 Dev/Alpha push. Nate is still looking at it, doesn't have fix yet.

As WebView doesn't have specific Dev channel like Chrome and only the chance of external users seeing this issue seems to be very less. Users see this issue **only** if they switch WebView implementation to Chrome Dev. 

So we have decided to move to RBB (to unblock tmrw M63 Dev/Alpha push), assuming before next week's Dev push, we will have a fix.

Thanks!

I put a "WIP" CL together to see if Yahoo app works, and seemed that the login flow is now working. This is based on Nasko's idea but it only intercepts the NavigationRequest.OnRequestRedirected. This patch seems minimal but this API is super sensitive.

I am going to work on it tomorrow. 

https://chromium-review.googlesource.com/c/chromium/src/+/676478 is the WIP CL. you can put early comments if you want -:)

@sgurun: curious why you're also not hooking NavigationRequest::BeginNavigation and not calling it in the renderer if plznavigate is on?

only because I did not have enough time to play with it (I was the chromium sheriff today).  
That is exactly what I am going to look at tomorrow - use BeginNavigation and disable the callpath from DecidePolicyForNavigation if plznavigate is enabled. 

One question: do all navigations end up in NavigationRequest or are some completely handled in Renderer? because that would be another variable to worry, I think.

Navigations hitting the network stack do get a NavigationRequest. javascript: "navigations" for example don't get that. You can take a look at IsURLHandledByNetworkStack to get an idea what are the conditions.

Just to expand on what Nasko said, if javascript URLs are expected to go through shouldOverrideUrlLoading perhaps with PlzNavigate the sync IPC call path should be taken if IsURLHandledByNetworkStack returns true?

javascript: does not seem to be bubbled up to WebView in renderer initiated navigation anyway. As far as I can see, we are safe there.

I am looking for how to figure out if navigation is initiated by application (via load url) or by content in navigationrequest. That is a parameter to shouldoverrideurlloading.

Curious where that is used? Looking at
https://cs.chromium.org/chromium/src/android_webview/browser/aw_contents_client_bridge.cc?rcl=cbba184f42c6800badcae347562f87d5c1123d80&l=372
the parameters are
jurl, has_user_gesture, is_redirect, is_main_frame
?

Curious where that is used? Looking at
https://cs.chromium.org/chromium/src/android_webview/browser/aw_contents_client_bridge.cc?rcl=cbba184f42c6800badcae347562f87d5c1123d80&l=372
the parameters are
jurl, has_user_gesture, is_redirect, is_main_frame
?

well, not a parameter there but see below for some of the conditions:

 https://cs.chromium.org/chromium/src/android_webview/renderer/aw_content_renderer_client.cc?rcl=825d142b62c01685beecefc7ff8d86024a90fc97&l=108

I should not call it for application initiated navigations or go back/forward but I should be able to call it redirects, even if it is application initiated. 

it seems that navigation request already has API for that - I don't know how I missed it:

  bool browser_initiated() const { return browser_initiated_ ; }

turned out I forgot about about:blank navigations - which are bubbled to shouldoverrideurlloading if they are top navigations. 

so I still had to rely on IsURLHandledByNetworkStack

I have finished an implementation -but still there are a few details to iron out- it is passing all the tests under AwContentsClientShouldOverrideLoadingTest except testCalledOn302Redirect(). In this case the behavior change comes from the user gesture flag in one case. 

The particular failure is due to:
https://cs.chromium.org/chromium/src/android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientShouldOverrideUrlLoadingTest.java?rcl=579f2d08bd77ed88f6986f30429622e0ecbe1762&l=710


that is if the user touches a link, that is redirected by the server, the redirected url does not retain has_user_gesture in case of renderer side navigation, but it carries has_user_gesture in case of browser side navigation. I am fairly confident the difference is not important enough to be noticed by the applications. 


Next step is to run CTS tests, all the Aw tests, ironing out a few more things, and perhaps writing a few more tests.

Finished running Aw tests. 5 tests failed, 2 of these are due to has_user_gesture that was mentioned in #45

issionRequestTest#testGrantAccess__multiprocess_mode
C 6942.795s Main  [  FAILED  ] 5 tests, listed below:
C 6942.795s Main  [  FAILED  ] org.chromium.android_webview.test.AwContentsClientShouldOverrideUrlLoadingTest#testCalledOn302Redirect
C 6942.795s Main  [  FAILED  ] org.chromium.android_webview.test.AwContentsClientShouldOverrideUrlLoadingTest#testCalledOn302Redirect__multiprocess_mode
C 6942.795s Main  [  FAILED  ] org.chromium.android_webview.test.ClientOnPageFinishedTest#testCalledAfterRedirectedUrlIsOverridden
C 6942.795s Main  [  FAILED  ] org.chromium.android_webview.test.ClientOnPageFinishedTest#testCalledAfterRedirectedUrlIsOverridden__multiprocess_mode
C 6942.795s Main  [  FAILED  ] org.chromium.android_webview.test.ClientOnPageFinishedTest#testCalledOnCancelingProvisionalLoad
C 6942.795s Main  
C 6942.795s Main  5 FAILED TESTS
C 6942.795s Main  **************************


This leaves 2 distinct tests to deal with.

Maria just told me that the org.chromium.android_webview.test.ClientOnPageFinishedTest#testCalledOnCancelingProvisionalLoad was in fact flaky as she as a sheriff is disabling it.

https://bugs.chromium.org/p/chromium/issues/detail?id=683384

That is probably why it failed here, because it does not fail in multiprocess mode.

Thanks for pushing on this sgurun@. Do I understand it correctly that the failures you've hit are acceptable/expected and the change is mostly ready for review? Or is it that the failures in comment 46 are significant?

Getting close but not yet. I want to run WebView CTS tests.

Also I need to look at some code paths in Webview that are easily forgotten and weak in tests - such as popups. 

I debugged the failing test with Jam@ and updated the plance in onrequestredirected() where I was calling shouldoverrideurlloading.

The test is now passing. I also run CTS tests locally and all of them except 2 are passing. The failing ones are unrelated as they were already failing and disabled here:

https://cs.chromium.org/chromium/src/android_webview/tools/cts_config/expected_failure_on_bot.json?q=expected_failure_on_bot.json&sq=package:chromium&dr

The trybots are testing the webview tests now with the latest change.

That is as far as we can go with WebView's automated testing. As a next step I am going to look at our popup path since it has a distinct behavior and is not well tested. After that the CL will be ready to review!

Deleted: test_result_failures.html 3.8 KB

test_result_failures.html 3.8 KB View Download

Fixing this bug will somewhat unblock https://bugs.chromium.org/p/chromium/issues/detail?id=325351 which is an existing bug for a long time.

The new plan is I think what mnaganov mentioned in his comment
https://bugs.chromium.org/p/chromium/issues/detail?id=462306#c23

"Yeah, but abarth@ suggested that "this will break the engine." Hmmm, that's pretty gross. We probably should leave the renderer path for "about:blank" only, and exclusively for supporting legacy apps (based on the target API level). Then eventually we will be able to remove this path."

let's leave sync IPC for about:blank as I am experimenting with, disable old way of handling shouldoverrideurlloading in popups. This at least makes the one CTS test we wrote many years ago (and was never enabled as we were not handling this properly), passing. So it is a net gain in our side as far as I can see.

I have tested about:blank navigation via window.open for popups. Currently, if this happens, no webview callbacks such as onPageFinished or onReceivedTitle are received. the behavior stays same with the suggested change in comment #51.

I added some number of tests for popup scenario and shouldoverrideurlloading. 
https://chromium-review.googlesource.com/c/chromium/src/+/685750

The tests are passing with the new plznavigate suggestion. 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/166024323466d3b1edb3d763a6f2811db1df2cf4

commit 166024323466d3b1edb3d763a6f2811db1df2cf4
Author: John Abd-El-Malek <jam@chromium.org>
Date: Wed Sep 27 02:46:09 2017

Fix issues with shouldOverrideUrlLoading and PlzNavigate.

There were a few problems:
-shouldOverrideUrlLoading wasn't called for requests to invalid schemes or origins that don't exist, since the navigation was aborted in the browser
-the request went to the network before the application got a chance to override it

The fix is to call the application in the browser when PlzNavigate. The exception is for URLs that don't go to the browser, and for that we use the old sync IPC code path.

This is based on sgurun's patch https://chromium-review.googlesource.com/c/chromium/src/+/676478
Bug:766167

Change-Id: Iadb467cb89f00bed5a2e359b277b403fc4a8f1e9
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_site_isolation
Reviewed-on: https://chromium-review.googlesource.com/683795
Commit-Queue: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Selim Gurun <sgurun@chromium.org>
Cr-Commit-Position: refs/heads/master@{#504557}
[modify] https://crrev.com/166024323466d3b1edb3d763a6f2811db1df2cf4/android_webview/browser/aw_content_browser_client.cc
[modify] https://crrev.com/166024323466d3b1edb3d763a6f2811db1df2cf4/android_webview/browser/aw_content_browser_client.h
[modify] https://crrev.com/166024323466d3b1edb3d763a6f2811db1df2cf4/content/browser/frame_host/navigation_request.cc
[modify] https://crrev.com/166024323466d3b1edb3d763a6f2811db1df2cf4/content/browser/frame_host/render_frame_host_impl.cc
[modify] https://crrev.com/166024323466d3b1edb3d763a6f2811db1df2cf4/content/browser/frame_host/render_frame_host_impl.h
[modify] https://crrev.com/166024323466d3b1edb3d763a6f2811db1df2cf4/content/common/frame_messages.h
[modify] https://crrev.com/166024323466d3b1edb3d763a6f2811db1df2cf4/content/public/browser/content_browser_client.cc
[modify] https://crrev.com/166024323466d3b1edb3d763a6f2811db1df2cf4/content/public/browser/content_browser_client.h
[modify] https://crrev.com/166024323466d3b1edb3d763a6f2811db1df2cf4/content/renderer/render_frame_impl.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0c329dc3e3bc480720d2a0f2cbfe68f9f991ad2c

commit 0c329dc3e3bc480720d2a0f2cbfe68f9f991ad2c
Author: Selim Gurun <sgurun@google.com>
Date: Wed Sep 27 18:52:48 2017

Add tests for ShouldOverrideUrlLoading callback behavior in popups.

WebView did not have tests for shouldoverrideurlloading in popups. 
This code path is different from main code path and needs to be tested.
These tests only add a few basic ones. 

Bug:  766167 
Change-Id: Ic9223ca602d82cf5f2902e0db599af463e703077
Reviewed-on: https://chromium-review.googlesource.com/685750
Reviewed-by: Bo <boliu@chromium.org>
Commit-Queue: Selim Gurun <sgurun@chromium.org>
Cr-Commit-Position: refs/heads/master@{#504741}
[modify] https://crrev.com/0c329dc3e3bc480720d2a0f2cbfe68f9f991ad2c/android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientShouldOverrideUrlLoadingTest.java

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1ea0edbb6c8d845aa5d63d1298f591d74d26088c

commit 1ea0edbb6c8d845aa5d63d1298f591d74d26088c
Author: Selim Gurun <sgurun@google.com>
Date: Wed Sep 27 23:09:16 2017

Disable old shouldoverrideurlloading path for PlzNavigate

When Webview moved to Chromium based code, we invented a "side"
shouldoverrideurlloading path for popups because by the time
sync ipc's were received, Webview was not ready to receive
the shouldoverrideurlloading callback (Popups require embedding
app to create them).

With PlzNav, we don't use sync IPC's anymore (except 
renderer-initiated navs, which is mainly about:blank), and now can 
disable that path. Note that this is not a super well tested path.
Some popup tests were added recently and they seem to be working
ok with this change.

Bug:  766167 
Change-Id: I061ab5de7296f7f3d59fb5833ece3f867b2e1c03
Reviewed-on: https://chromium-review.googlesource.com/688474
Reviewed-by: Bo <boliu@chromium.org>
Commit-Queue: Selim Gurun <sgurun@chromium.org>
Cr-Commit-Position: refs/heads/master@{#504792}
[modify] https://crrev.com/1ea0edbb6c8d845aa5d63d1298f591d74d26088c/android_webview/java/src/org/chromium/android_webview/AwContents.java

now we disabled the old pop up path, I am closing the bug as fixed. There are potentially some other tests and improvements that can be done for plznavigate.

I will file bugs for the things I know.

Fix issue verified on Pixel XL / OPR1.170623.011 version 63.0.3226.0

Thanks!

Thanks for verifying! Requesting merge of the CL landed in comment 53.

This bug requires manual review: M62 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

We will only get to merge PlzNavigate fixes into M62 when 766083 is fixed since it is currently blocking M62 beta.

I had a long meeting with Gustav about shouldoverrideurlloading path today and we discussed various cornercases and test needs. He can file any future bugs to fix this path. 

Thanks Gustav.