DataURLNavigationThrottle is blocking this navigation because navigation_handle->IsRendererInitiated() is true for this case.

When |webView.loadUrl("javascript: alert(1);");| line is removed, IsRendererInitiated() returns false.

Nasko, CC'ing you in case you have any thoughts while I'm looking into this further.

I don't know why the javascript: URL will be considered at all by DataURLNavigationThrottle. Shouldn't it only affect data: URLs? The renderer vs browser initiated bit shouldn't really change. Did you check the bit on each navigation? Looking at the sample code, it looks to me that there are two navigations being performed - one to data: URL and another one to the pseudo-URL with "javascript:" scheme.

Here is what I've found so far:

The problem is that Android WebView's loadData() call followed with a loadURL("javascript:...") call starts a renderer-initiated data URL navigation. It should ideally start a browser initiated one. This in turn causes DataUrlNavigationThrottle to incorrectly block the data URL navigation.

This seems to be an existing bug that was manifested by data URL blocking.  For all data URLs, RenderFrameImpl::NavigateInternal() loads the data URL as a normal frame load (frame_->Load), instead of using LoadDataURL. This is because both common_params.base_url_for_data_url and request_params.data_url_as_string is empty [1]. |data_url_as_string| only exists on Android, and is set by LoadURLParams.java. However, LoadURLParams only fills it for createLoadDataParamsWithBaseUrl() calls and not for createLoadDataParams(). 

Two possible immediate fixes here:
1. Make RenderFrameImpl::NavigateInternal use LoadDataURL() for data URLs based on the URL scheme. This was previously the case until https://codereview.chromium.org/2037733002/diff/20001/content/renderer/render_frame_impl.cc changed it and removed the scheme check. 

2. Call params.setDataUrlAsString() in LoadURLParams.createLoadDataParams() to set the data_url_as_string parameter. This currently doesn't work because it hits a DCHECK in NavigationControllerAndroid (line 236).

It remains a mystery why removing the loadURL("javascript") call causes the first data URL to load fine. In this case, RenderFrameImpl still loads the data URL via frame_->Load() instead of LoadDataURL(), but this time the navigation correctly happens as a browser initiated one. I suspect the early return for javascript: scheme in WebLocalFrameImpl::Load() has something to do with why the data URL navigation turns into a renderer initiated one, but I don't quite understand why.

+clamy for https://codereview.chromium.org/2037733002: Is there a particular reason RenderFrameImpl shouldn't take the data URL scheme into account when deciding how to load the data URL?

[1] https://cs.chromium.org/chromium/src/content/renderer/render_frame_impl.cc?rcl=ce85174f53239960c7007e4b6c8bc6bde272f1a5&l=6137

More info on why javascript: URL affects data URL behavior:

NavigatorImpl::DidStartMainFrameNavigation checks for a browser initiated pending entry. If it can't find one, it creates a renderer initiated entry.

When a javascript: URL runs, the pending entry is cleared. I believe the reason is the special logic for javascript URLs at the end of NavigatorImpl::NavigateToEntry which returns false [1]. Further up the stack, NavigationControllerImpl::NavigateToPendingEntry uses this value as success code, and discards non committed entries. This means a javascript: navigation will always clear any pending entries.

Since there is now no pending entry, NavigatorImpl::DidStartMainFrameNavigation creates a new renderer initiated pending entry for the not-yet-completed data URL navigation -- even though it was originally created by a WebView. This in turn causes the data URL to be blocked by DataUrlNavigationThrottle.

[1] https://cs.chromium.org/chromium/src/content/browser/frame_host/navigator_impl.cc?rcl=c36c650cf291de889b00938e600e2bf1a54f5772&l=482

[2] https://cs.chromium.org/chromium/src/content/browser/frame_host/navigation_controller_impl.cc?rcl=c36c650cf291de889b00938e600e2bf1a54f5772&l=1964

nasko: Is my understanding of javascript: navigations clearing pending entries accurate?

@meacer: do you mean why don't we do should_load_data_url = url.SchemeIsData()?
For historical reasons. This is how it was written, and I didn't want to modify behavior.

+creis for issue with history
I think your understanding of the issue with the pending entry is correct. Could you try running the code with PlzNavigate enabled? We use a different way to check if a navigation is browser-initiated or not, which shouldn't be affected by this issue. (run with the command flag --enable-browser-side-navigation).

> @meacer: do you mean why don't we do should_load_data_url = url.SchemeIsData()?

That seemed to be the case before crrev.com/2037733002, so I was curious if there is a particular reason not to do that -- at least for browser initiated navigations.

> Could you try running the code with PlzNavigate enabled? We use a different way to check if a navigation is browser-initiated or not, which shouldn't be affected by this issue. (run with the command flag --enable-browser-side-navigation).

Yes, with PlzNavigate the bug indeed disappears.

Do you mind if I assign this to you as it's navigation related? Here is a CL with a test that could help: https://codereview.chromium.org/2944803003

clamy, nasko, creis: Ping :) Can I assign this shiny release blocker to one of you?

nasko, creis: Since the problem is fixed with PlzNavigate, should we revert the data url block and wait until PlzNavigate lands? It seems to me that modifying the code around the pending nav entry cancellation for javascript navs might be hard.

I would like us to keep the data: URL blocking, as it is addressing a very real threat to our users. One thing I don't understand yet - what prevents us from exploring suggestion 1 in comment 4?

Part of suggestion 1 from comment 4 is not accurate. The CL in question only affected PlzNavigate. In regular navigations, we have never used a scheme check to determine if a url should be loaded as a data url. This was only done in PlzNavigate, and we removed it because some tests expected it to go through the normal codepath instead.

On top of that, I'm not even sure this would be correct for Android WebView. We do have code that sends all data urls to the browser right now on Android because Android WebView needs to check them. If they are always sent to the browser, they would be checked by the DataUrlNavigationThrottle.

It seems to me then that the proper fix is to have the webView.loadData() API create a browser initiated navigation, which will not be blocked by DataUrlNavigationThrottle. The API I assume originates in the Android app Java code, so it is semantically equivalent to the browser process in Chrome, so it makes overall sense to me.

But they are creating a browser-initiated navigation in the first place. The issue is that they issue a navigation to a Javascript URL just next. This has the side effect of discarding the pending NavigationEntry. By the time we get the DidStartProvisionalLoad corresponding to the browser-initiated navigation to a data URL, we no longer have a pending NavigationEntry for this navigation. So when we create the NavigationHandle, we misclassify it as renderer-initiated. And it is then blocked by the DataUrlNavigationThrottle.

The problem is that we don't have a better way of classifying something as browser-intiated in the current navigation architecture than checking if we have a matching pending entry. But it can go away.

nasko, clamy: Is PlzNavigate going to be pushed to 100% Stable in M60? If so, it sounds like the problem will automatically go away, is that correct?

Also I don't think I'm the right owner for this, as it's more about navigation and less about data URL blocking. clamy: Can I assign it to you?

The PlzNavigate on in M60 is still unclear and won't be clear until closer to M60 becoming stable. However, it looks like it is not easy to fix the data: URL blocking to work properly with WebView and the old navigation codepath. As such, I think it is your call meacer@ as the owner of the data: URL blocking feature whether to disable it until PlzNavigate is the default.

OK, looks like we have some time to decide until PlzNavigate situation is clear.

In any case I don't think we should revert blocking for everyone just because of this bug. However we can delay Android blockage by a single milestone if PlzNavigate doesn't ship fully in M60.

emilyschechter: FYI, this is a data URL blocking stable blocker on Android WebView. It's fixed by PlzNavigate, but it's not clear yet if PlzNavigate is shipping in M60. If it doesn't, I'm planning to disable data URL blocking on Android. Does that sound okay?

That sounds fine to me, thanks for investigating.

Just curious: Would you be able to disable it just for Android WebView and not Chrome on Android?

I was planning to #ifdefing it for Android.

But yes it's possible if we are okay with adding a temporary method such as RenderFrameHost::AllowDataUrlNavigationsForAndroidWebView.

FWIW there is already a RenderFrameHost::AllowInjectingJavaScriptForAndroidWebView() method.

if there is no other way, I guess this is acceptable temporarily.

What is the status here?  We need this addressed by end of next week at the absolute latest.

The CL to re-enable data URLs on Android Webview is WIP: https://codereview.chromium.org/2969473003/

I'll see if I can land it today or on Monday.

That is: sgurun@ and I decided not to wait until the decision to ship PlzNavigate happens. We'll just reenable navigations on Android Webview for now.

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

commit e23f989bb00a9207fd3dc67cc27f725c32b2be77
Author: meacer <meacer@chromium.org>
Date: Wed Jul 12 21:44:43 2017

Allow data URL navigations for Android WebView until PlzNavigate ships

BUG= 732976 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2969473003
Cr-Commit-Position: refs/heads/master@{#486092}

[modify] https://crrev.com/e23f989bb00a9207fd3dc67cc27f725c32b2be77/android_webview/browser/aw_browser_main_parts.cc
[modify] https://crrev.com/e23f989bb00a9207fd3dc67cc27f725c32b2be77/android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java
[modify] https://crrev.com/e23f989bb00a9207fd3dc67cc27f725c32b2be77/content/browser/frame_host/data_url_navigation_throttle.cc
[modify] https://crrev.com/e23f989bb00a9207fd3dc67cc27f725c32b2be77/content/browser/frame_host/render_frame_host_impl.cc
[modify] https://crrev.com/e23f989bb00a9207fd3dc67cc27f725c32b2be77/content/public/browser/render_frame_host.h

Merge approved for M60 branch 3112.

Forked the root cause of the problem as bug 743115.

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

commit 45e4c0d828d5b470172bfe7b13128f45d4a6bee7
Author: Mustafa Acer <meacer@chromium.org>
Date: Fri Jul 14 18:38:32 2017

[Merge to M-60] Allow data URL navigations for Android WebView until PlzNavigate ships

BUG= 732976 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
TBR=meacer@chromium.org,clamy@chromium.org,sgurun@chromium.org

(cherry picked from commit e23f989bb00a9207fd3dc67cc27f725c32b2be77)

Review-Url: https://codereview.chromium.org/2969473003
Cr-Original-Commit-Position: refs/heads/master@{#486092}
Change-Id: I147a7e68e44b805d1151dce2c0b62b67b625787a
Reviewed-on: https://chromium-review.googlesource.com/571515
Reviewed-by: Selim Gurun <sgurun@chromium.org>
Reviewed-by: Mustafa Emre Acer <meacer@chromium.org>
Cr-Commit-Position: refs/branch-heads/3112@{#609}
Cr-Branched-From: b6460e24cf59f429d69de255538d0fc7a425ccf9-refs/heads/master@{#474897}
[modify] https://crrev.com/45e4c0d828d5b470172bfe7b13128f45d4a6bee7/android_webview/browser/aw_browser_main_parts.cc
[modify] https://crrev.com/45e4c0d828d5b470172bfe7b13128f45d4a6bee7/android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java
[modify] https://crrev.com/45e4c0d828d5b470172bfe7b13128f45d4a6bee7/content/browser/frame_host/data_url_navigation_throttle.cc
[modify] https://crrev.com/45e4c0d828d5b470172bfe7b13128f45d4a6bee7/content/browser/frame_host/render_frame_host_impl.cc
[modify] https://crrev.com/45e4c0d828d5b470172bfe7b13128f45d4a6bee7/content/public/browser/render_frame_host.h