Commit aec60579 authored by Lukasz Anforowicz's avatar Lukasz Anforowicz Committed by Commit Bot

Fixup URLs passed through the public Android WebView API.

This CL changes AwContents.loadUrl(LoadUrlParams), so that URLs passing
through WebView APIs are fixed - partially restoring the behavior prior
to r818969.

Before r818969 (which landed in 88.0.4298.0), the URL passed to
WebView.loadUrl (and postUrl, loadData, loadDataWithBaseURL) would get
fixed via url_formatter::FixupURL when the //content layer was
processing the navigation (e.g. from within WillHandleBrowserAboutURL or
BrowserURLHandlerImpl::FixupURLBeforeRewrite - both of these callsites
have been removed in r818969).  Such rewrite is not happening after
r818969 - this is an accidental breaking change.  In
https://crbug.com/1145717 we have some evidence that some apps have
actually depended on fixing the URLs, although the evidence is not
particularily strong and doesn't include the specific URLs that were
getting fixed.

Unlike before r818969, *all* URLs are fixed, *prior* to starting the
navigation via the //content APIs - this introduces some small
differences in behavior (e.g. javascript:42 would not be rewritten
before this r818969, but would be rewritten to http://javascript:42/
after the current CL), but these differences are small and the risk
seems acceptable (e.g. javascript:42+42 would not be rewritten).

Bug: 1145717
Change-Id: I04f5ee54b3ee841cf54d0256e0eeca2e2f984e6f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2528804Reviewed-by: default avatarRichard Coles <torne@chromium.org>
Reviewed-by: default avatarPeter Kasting <pkasting@chromium.org>
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#827474}
parent c0d403ae
...@@ -536,6 +536,7 @@ android_library("browser_java") { ...@@ -536,6 +536,7 @@ android_library("browser_java") {
"//components/navigation_interception/android:navigation_interception_java", "//components/navigation_interception/android:navigation_interception_java",
"//components/policy/android:policy_java", "//components/policy/android:policy_java",
"//components/safe_browsing/android:safe_browsing_java", "//components/safe_browsing/android:safe_browsing_java",
"//components/url_formatter/android:url_formatter_java",
"//components/variations:load_seed_result_enum_java", "//components/variations:load_seed_result_enum_java",
"//components/variations/android:variations_java", "//components/variations/android:variations_java",
"//components/version_info/android:version_constants_java", "//components/version_info/android:version_constants_java",
......
...@@ -19,6 +19,7 @@ include_rules = [ ...@@ -19,6 +19,7 @@ include_rules = [
"+components/metrics", "+components/metrics",
"+components/prefs", "+components/prefs",
"+components/services/heap_profiling/public", "+components/services/heap_profiling/public",
"+components/url_formatter",
"+components/version_info", "+components/version_info",
# Only allow this header in spellchecking since allowing all of spellchecking # Only allow this header in spellchecking since allowing all of spellchecking
# would include both browser and renderer components. # would include both browser and renderer components.
......
...@@ -81,6 +81,7 @@ import org.chromium.components.content_capture.ContentCaptureConsumer; ...@@ -81,6 +81,7 @@ import org.chromium.components.content_capture.ContentCaptureConsumer;
import org.chromium.components.embedder_support.util.WebResourceResponseInfo; import org.chromium.components.embedder_support.util.WebResourceResponseInfo;
import org.chromium.components.navigation_interception.InterceptNavigationDelegate; import org.chromium.components.navigation_interception.InterceptNavigationDelegate;
import org.chromium.components.navigation_interception.NavigationParams; import org.chromium.components.navigation_interception.NavigationParams;
import org.chromium.components.url_formatter.UrlFormatter;
import org.chromium.content_public.browser.ChildProcessImportance; import org.chromium.content_public.browser.ChildProcessImportance;
import org.chromium.content_public.browser.ContentViewStatics; import org.chromium.content_public.browser.ContentViewStatics;
import org.chromium.content_public.browser.GestureListenerManager; import org.chromium.content_public.browser.GestureListenerManager;
...@@ -2069,6 +2070,12 @@ public class AwContents implements SmartClipProvider { ...@@ -2069,6 +2070,12 @@ public class AwContents implements SmartClipProvider {
params.getUrl(), params.getExtraHttpRequestHeadersString()); params.getUrl(), params.getExtraHttpRequestHeadersString());
params.setExtraHeaders(new HashMap<String, String>()); params.setExtraHeaders(new HashMap<String, String>());
// Ideally, the URL would only be "fixed" for user input (e.g. for URLs
// entered into the Omnibox), but some WebView API consumers rely on
// the legacy behavior where all navigations were subject to the
// "fixing". See also https://crbug.com/1145717.
params.setUrl(UrlFormatter.fixupUrl(params.getUrl()).getPossiblyInvalidSpec());
mNavigationController.loadUrl(params); mNavigationController.loadUrl(params);
// The behavior of WebViewClassic uses the populateVisitedLinks callback in WebKit. // The behavior of WebViewClassic uses the populateVisitedLinks callback in WebKit.
......
...@@ -809,6 +809,40 @@ public class AwContentsTest { ...@@ -809,6 +809,40 @@ public class AwContentsTest {
Assert.assertEquals(0, consoleHelper.getMessages().size()); Assert.assertEquals(0, consoleHelper.getMessages().size());
} }
/**
* Regression test for https://crbug.com/1145717. Load a URL that requires fixing and verify
* that the legacy behavior is preserved (i.e. that the URL is fixed + that no crashes happen in
* the product).
*
* The main test verification is that there are no crashes. In particular, this test tries
* to verify that the `loadUrl` call above won't trigger:
* - NOTREACHED and DwoC in content::NavigationRequest's constructor for about: scheme
* navigations that aren't about:blank nor about:srcdoc
* - CHECK in content::NavigationRequest::GetOriginForURLLoaderFactory caused by the
* mismatch between the result of this method and the "about:" process lock.
*/
@Test
@LargeTest
@Feature({"AndroidWebView"})
public void testLoadUrlAboutVersion() throws Throwable {
AwTestContainerView testView =
mActivityTestRule.createAwTestContainerViewOnMainSync(mContentsClient);
final AwContents awContents = testView.getAwContents();
mActivityTestRule.runOnUiThread(() -> {
// "about:safe-browsing" will be rewritten by
// components.url_formatter.UrlFormatter.fixupUrl into
// "chrome://safe-browsing/".
//
// Note that chrome://safe-browsing/ is one of very few chrome://... URLs that work
// in Android WebView. In particular, chrome://version/ wouldn't work.
awContents.loadUrl("about:safe-browsing");
});
mContentsClient.getOnPageFinishedHelper().waitForCallback(
0, 1, WAIT_TIMEOUT_MS, TimeUnit.MILLISECONDS);
Assert.assertEquals("chrome://safe-browsing/", awContents.getLastCommittedUrl());
}
private void doHardwareRenderingSmokeTest() throws Throwable { private void doHardwareRenderingSmokeTest() throws Throwable {
AwTestContainerView testView = AwTestContainerView testView =
mActivityTestRule.createAwTestContainerViewOnMainSync(mContentsClient); mActivityTestRule.createAwTestContainerViewOnMainSync(mContentsClient);
......
...@@ -508,7 +508,7 @@ public class LoadDataWithBaseUrlTest { ...@@ -508,7 +508,7 @@ public class LoadDataWithBaseUrlTest {
mActivityTestRule.getAwSettingsOnUiThread(mAwContents).setJavaScriptEnabled(true); mActivityTestRule.getAwSettingsOnUiThread(mAwContents).setJavaScriptEnabled(true);
mActivityTestRule.loadDataWithBaseUrlAsync( mActivityTestRule.loadDataWithBaseUrlAsync(
mAwContents, pageHtml, "text/html", false, invalidBaseUrl, null); mAwContents, pageHtml, "text/html", false, invalidBaseUrl, null);
mActivityTestRule.loadUrlAsync(mAwContents, "javascript:42"); mActivityTestRule.loadUrlAsync(mAwContents, "javascript:42+42");
onPageFinishedHelper.waitForCallback(callCount); onPageFinishedHelper.waitForCallback(callCount);
// Verify that the load succeeds. The actual base url is undefined. // Verify that the load succeeds. The actual base url is undefined.
Assert.assertEquals( Assert.assertEquals(
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment