Nate, once please nav is enabled this can cause issues. Please own that and see what can be done.

estelle: as we ramp up from 5% to 10% and more, please continue to monitor the webview bugs for any weird app bugs. This may (we don't know) break things. However the fact that our QA did not see any issues on that gives me confidence.

I can still repro the bug in the latest canary, but only with --enable-browser-side-navigation.

Another difference is that shouldInterceptRequest gets called twice now: once for the base URL's favicon and once for the data. Without plz navigate, we only call it for the favicon. Not sure if this matters as much as onPageStarted.

himm, Jam I think you said it was enabled right. Was the flag actually flipped, or is it enabled through Finch?

It's enabled through Finch.

Webview does not have finch yet, unfortunately.

oh wow, that means that it's not turned on there and the only way to turn it on even for 62 will be to merge a cl.

When will plz navigate be turned on by default? It would be nice to enable this slowly with WebView finch (even after this bug is fixed).

@ntfschr: I'm confused, I thought sgurun@ said Webview doesn't have finch in comment 7?

We will turn it on trunk by default very soon. Once we know it stuck to M61, we will merge that to m62.

we do not have Finch. However we are working on it. I guess Nate meant can we wait for it to turn it on?

Even though we have a prototype that is ready, our actual release is not in the plans as a milestone yet. I am afraid that is going to be a unacceptable wait time for plznavigate people.

Nate can you please fix these two issues urgently. We have time to merge the fixes to M62.

Also in case things go south, we can add plumbing to turn it off for WebView only -this would be an acceptable escape hatch. We do have such kind of examples in the code.

We would strongly prefer that webview uses the same as the other platforms; we want to remove the old navigation code paths as soon as possible and we can't do that if it's in use in production.

Unfortunately I think we need to be extra careful with webview here. I can imagine plznavigate having a large impact on webview, because a lot of navigation events are exposed to apps, and it's pretty easy to imagine some subtle things changing breaking badly written apps.

Unfortunately, webview beta population is effectively negligible, and it's very possible regressions are only discovered in stable. So my general rule is hold old path for one additional release after new path goes to stable, just in case something goes wrong.

Also I guess plznavigate has never been enabled in webview? We probably should try doing that on trunk, and see if there any other issues besides this we can find before shipping to stable.

Jam: I don't think the goal is to remove old path in one milestone, is it? It would be nice to keep it at least till the M63 timeframe so we can fallback to it in case of a catastrophic problem and investigate issues in the meantime.

If we launch on m61 (but not webview, as that doesn't use finch), then we'd only want to support it for m62 at most.  if we need to revert it on m62, then we can do any fixes in 63 branch to turn it there again.

sgurun@, about comment #12, you mean if something goes bad we will able to stop the bleeding without shipping another build to stable? I am not very confident about turning on this feature on stable without a proper way to disable it in case something bad happens. 

What is the timeline of getting WebView support Finch? Does the plz team has to enable this in M62 absolutely? Could this wait until a proper mechanism(finch) to disable it in stable is established? 

+paulmiller@ to answer the finch question in c#17

@cmasso: we're turning PlzNavigate on M61 for all platforms other than webview (unless we see serious bugs next week as we ramp to 100%). Turning it on for webview at m62 is already delaying things for us (we just found out that webview doesn't use finch).

It seems the issue of webview not using finch is orthogonal from plznavigate? i.e. any new feature that is finch'd, sticks on, and then is turned on by code will have this same issue.

cmasso@ we cannot stop the bleeding without shipping any new build. There is no remote mechanism in webview now. It needs a CL.

there is no set timeline to have WebView finch now. We need to look at the design once again, and probably do a few experiments in an unstable channel to make sure things are working fine. Finch can be released after that.

cmasso: even if our beta community is tiny - I think we should be able to enable this for WebView in unstable channels now. It will give us extra information.

Makes sense. I approved this for non stable channel for now, hoping we get something out of it.

@cmasso: When PlzNavigate will have stuck on the rest of Chrome (which we hope will happen in M61) the longer we wait to enable it on WebView the more likely it is that bugs will arise.

First, all channels are currently running with PlzNavigate at 100%. This means that the Android Canary, Dev, Beta and Stable are not running the same navigation codepath as WebView. So if new bug arise in the old navigation codepath that would normally have been caught on those channels, we will not be able to catch it, and I imagine WebView will find about it when they do a release.

Second, some projects are doing development in a PlzNavigate only model, because the cost of developing for 2 codepaths is too high. While these projects may not affect WebView, some could. If such a project is turned on on trunk and is included in WebView which doesn't run with PlzNavigate, I don't know what will happen. This will likely cause a lot of maintainability issues.

Also, maintaining two codepaths for navigation is costly. Apart from added cost to developpers, we also need to maintain testing coverage of the old codepath. This requires us to run a lot of test suites with and without Plznavigate on CQ bots, which costs resources and lengthen CQ processing time. We would like this to stop as soon as possible when we launch PlzNavigate.

We didn't know that WebView didn't support Finch, otherwise we would have started talking with you folks a lot earlier. But we should aim towards having WebView as close as possible to the rest of Chrome.

In any case, we're prepared to help you with possible bugs that could arise with the launch of PlzNavigate :).

There's also a number of limitations in our planned finch implementation which mean that there will probably be some number of cases where webview isn't using updated finch settings and will rely in the defaults more frequently than Chrome does.

This means that we'd generally not want to rely on killswitches (feature being default enabled with the option to revert it via finch) for risky changes, and would rather it was the other way around.

We realise this is problematic for you to deal with, but changes to the ordering and behaviour of navigation callbacks in WebView have caused widespread application compatibility issues multiple times in the past and if there are known effects like the one in this bug then PlzNavigate is really risky for us. App implementations of navigation callbacks are often extremely fragile (and stateful) unfortunately.

So if I follow what you're saying, there's simply no mechanism for us to launch on WebView?

@torne: to clarify what you mean by "other way around": you mean disabled by default on webview, and when webview has finch we can use finch to enable it? Even though finch isn't always in use?

If I'm understanding what you wrote, there are two main problems I see with that:
1) if it's not on anywhere, that would mean old path is used sometimes. But we are planning on removing that path soon.
2) finch isn't available yet, so it's not an option.

We sympathize that navigation changes can have unintended consequences; that's why we're launching in September even though trybots were green in February.

To move forward, what was agreed yesterday with Selim and others is that it would get turned on by default on m62, and testers would check top 100 apps to make sure it works. Once m62 webview goes to stable, if during the rollout you hear of any critical bugs, you can disable it on m62 through a push (since finch isn't available). The bug fixes can be done on m63 branch.

If plznavigate sticks on 61 on all platforms other than webview, the old code path will be most likely be deleted when 63 branches (Oct 12).

Happy to VC to chat more about this.

To be clear, we do understand that there can be unintended effects: we did spend 3 years on that refactoring precisely because of that :). However as jam@ said, we simply cannot maintain the old navigation codepath for too long. We're very willing to work with you guys to find a way to launch on WebView and to deal with the potential bugs.

I'm happy to VC as well to discuss a way forward :).

What I mean is that even once WebView has finch support, the ideal rollout for features will be disabled by default with finch overriding it to be enabled, until it's been enabled for 100% of users for long enough that we're happy to just drop the "disabled" option entirely.

The intermediate step some features currently go through where they are enabled by default but there's still an option to disable them again via finch is what I'm saying isn't necessarily going to work for webview.

This wasn't specifically about this feature, just about finch support in webview for launching anything, and yes, this is only relevant in the future when we actually have finch support at all.

I have approved the merge to have plznavigate in M62. The plan is as described in comment 26. 

Issue 766298 has been merged into this issue.

Update: when blink handles navigation (no PlzNav), it doesn't create an onPageStarted event for loadDataWithBaseUrl. WebView manually creates this event here [1]. It appears that PlzNavigate, when loading the data, actually signals onPageStarted for the base URL (that's what we want).

I have a hack which skips our extra postOnPageStarted [1] if PlzNav is enabled.

Remaining concerns:

 1. onPageStarted: the URL can differ. Ex: I use "http://www.google.com" as the base URL (no trailing slash). PlzNav adds a trailing slash, but there is no trailing slash without PlzNav
 2. shouldInterceptRequest: PlzNav calls this for the data URI, which was formerly not the case
 3. onLoadResource: PlzNav calls this for the data URI, which was formerly not the case

Per concern 1, we don't actually make promises about the format of the URL for onPageStarted. So maybe this is an acceptable change in behavior.

2 & 3 require further investigation. It looks like we must be sending each URL through the navigation pipeline, or we're double-posting somewhere along the line.

[1] https://cs.chromium.org/chromium/src/android_webview/java/src/org/chromium/android_webview/AwContents.java?type=cs&q=awcontents.java+postonpagestarted&sq=package:chromium&l=1662

If we change the formats of URLs in callbacks I would expect apps to break, even if the URL would parse identically: it's likely that some apps rely on exact string matching in their logic.

I guess we should consider any points where we synthesize an navigation status callback like onPageStarted in webview suspicious and audit all of them.

Presumably the URL identity here is something that is not guaranteed in general for non data URLs, given that they will go through other Chrome code. Doubtless this change will break some app somewhere, but it seems like something that most apps wouldn't rely upon.

For 2 & 3, I guess we could just drop data URIs at the point we receive them. Presumably we need to disable all these special cases dependent on the app targeting P, and document the change.

Does shouldInterceptRequest now see redirects? Because that could also break apps (although it's a feature that developers have asked for...).

> Does shouldInterceptRequest now see redirects? Because that could also break apps (although it's a feature that developers have asked for...).

Didn't it do that before?

I tried this with http bin's redirect URL. I see changes in the *order* of callbacks, but shouldOverrideUrlLoading is still called for each redirect (w/ and w/o PlzNav). Is this not expected?

Anyway, I tested redirects with loadUrl("https://httpbin.org/redirect/6") (but see the description on https://httpbin.org/ before clicking).

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

commit 5b685a090dde7a9d0e0e3f409d66a9016cb4e8a2
Author: Nate Fischer <ntfschr@chromium.org>
Date: Tue Sep 26 18:55:09 2017

AW: only call onPageStarted once for loadDataWithBaseUrl

Previously, WebView would explicitly post onPageStarted for
loadDataWithBaseUrl, since this navigation was handled by blink and it
did not generate the event.

With browser-side navigation (PlzNavigate), this is no longer the case
and the navigation code will post this event under the normal pipeline.
For this reason, we need to disable the extra postOnPageStarted when
browser-side is turned on.

As part of the implementation, this adds a Java wrapper around
IsBrowserSideNavigationEnabled.

This also modifies an integration test to check that we only get
onPageStarted called once.

Bug:  702785 
Test: run_webview_instrumentation_test_apk -f LoadDataWithBaseUrlTest#testloadDataWithBaseUrlCallsOnPageStarted
Change-Id: I3abc7b3503feee26e5b2438f9b039be30750e4ef
Reviewed-on: https://chromium-review.googlesource.com/683634
Reviewed-by: Bo <boliu@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Commit-Queue: Nate Fischer <ntfschr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#504441}
[modify] https://crrev.com/5b685a090dde7a9d0e0e3f409d66a9016cb4e8a2/android_webview/java/src/org/chromium/android_webview/AwContents.java
[modify] https://crrev.com/5b685a090dde7a9d0e0e3f409d66a9016cb4e8a2/android_webview/javatests/src/org/chromium/android_webview/test/LoadDataWithBaseUrlTest.java
[modify] https://crrev.com/5b685a090dde7a9d0e0e3f409d66a9016cb4e8a2/content/common/BUILD.gn
[add] https://crrev.com/5b685a090dde7a9d0e0e3f409d66a9016cb4e8a2/content/common/android/browser_side_navigation_policy_android.cc
[modify] https://crrev.com/5b685a090dde7a9d0e0e3f409d66a9016cb4e8a2/content/public/android/BUILD.gn
[add] https://crrev.com/5b685a090dde7a9d0e0e3f409d66a9016cb4e8a2/content/public/android/java/src/org/chromium/content_public/common/BrowserSideNavigationPolicy.java

I'm branching off separate issues into separate bugs:

 - shouldInterceptRequest is called for the data URL for loadDataWithBaseUrl ( issue 769126 )
 - investigate shouldInterceptRequest behavior with redirects ( issue 768631 )

---

That being said, the core issue (onPageStarted called twice) should now be fixed.

There's no obvious way to manually verify this, but this CL has automated test coverage.

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

Bulk edit: marking stale 'fixed' bugs as 'verified' since they don't need verification at this point.