<webview>: Navigate to about:blank if attempting to navigate to bad URL (issue 888563003 by fsamuel@chromium.org)

[fsa...@chromium.org]

Reviewers: lazyboy,

Description:
<webview>: Navigate to about:blank if attempting to navigate to bad URL


If <webview> attempts to navigate to a URL that is invalid or it cannot
access, it will fire a loadabort message. However, a loadstop will not fire
and <webview> may remain in an unnavigated state despite having a set src
attribute. This causes various APIs such as executeScript to fail.

This CL navigates <webview> to about:blank in the case of a bad navigation.
This puts <webview> back to a sane state.

BUG=450125

Please review this at https://codereview.chromium.org/888563003/

Base URL: https://chromium.googlesource.com/chromium/src.git@master

Affected files (+43, -24 lines):
M chrome/test/data/extensions/platform_apps/web_view/shim/main.js
M extensions/browser/guest_view/web_view/web_view_guest.cc
M extensions/test/data/web_view/apitest/main.js


Index: chrome/test/data/extensions/platform_apps/web_view/shim/main.js
diff --git
a/chrome/test/data/extensions/platform_apps/web_view/shim/main.js
b/chrome/test/data/extensions/platform_apps/web_view/shim/main.js
index
91e7db85636620f7ad061c917fabad2c72d614f3..763b63d7155d859e0a699c501589697788150571
100644
--- a/chrome/test/data/extensions/platform_apps/web_view/shim/main.js
+++ b/chrome/test/data/extensions/platform_apps/web_view/shim/main.js
@@ -1421,16 +1421,14 @@ function testLoadAbortEmptyResponse() {
// chrome URL is provided.
function testLoadAbortIllegalChromeURL() {
var webview = document.createElement('webview');
- var onFirstLoadStop = function(e) {
- webview.removeEventListener('loadstop', onFirstLoadStop);
- webview.setAttribute('src', 'chrome://newtab');
- };
- webview.addEventListener('loadstop', onFirstLoadStop);
webview.addEventListener('loadabort', function(e) {
embedder.test.assertEq('ERR_ABORTED', e.reason);
+ });
+ webview.addEventListener('loadstop', function(e) {
+ embedder.test.assertEq('about:blank', webview.src);
embedder.test.succeed();
});
- webview.setAttribute('src', 'about:blank');
+ webview.src = 'chrome://newtab';
document.body.appendChild(webview);
}

@@ -1438,9 +1436,12 @@ function testLoadAbortIllegalFileURL() {
var webview = document.createElement('webview');
webview.addEventListener('loadabort', function(e) {
embedder.test.assertEq('ERR_ABORTED', e.reason);
+ });
+ webview.addEventListener('loadstop', function(e) {
+ embedder.test.assertEq('about:blank', webview.src);
embedder.test.succeed();
});
- webview.setAttribute('src', 'file://foo');
+ webview.src = 'file://foo';
document.body.appendChild(webview);
}

@@ -1448,6 +1449,9 @@ function testLoadAbortIllegalJavaScriptURL() {
var webview = document.createElement('webview');
webview.addEventListener('loadabort', function(e) {
embedder.test.assertEq('ERR_ABORTED', e.reason);
+ });
+ webview.addEventListener('loadstop', function(e) {
+ embedder.test.assertEq('about:blank', webview.src);
embedder.test.succeed();
});

webview.setAttribute('src', 'javascript:void(document.bgColor="#0000FF")');
@@ -1457,17 +1461,19 @@ function testLoadAbortIllegalJavaScriptURL() {
// Verifies that navigating to invalid URL (e.g. 'http:') doesn't cause a
crash.
function testLoadAbortInvalidNavigation() {
var webview = document.createElement('webview');
- var validSchemeWithEmptyURL = 'http:';
webview.addEventListener('loadabort', function(e) {
embedder.test.assertEq('ERR_ABORTED', e.reason);
embedder.test.assertEq('', e.url);
+ });
+ webview.addEventListener('loadstop', function(e) {
+ embedder.test.assertEq('about:blank', webview.src);
embedder.test.succeed();
});
webview.addEventListener('exit', function(e) {
// We should not crash.
embedder.test.fail();
});
- webview.setAttribute('src', validSchemeWithEmptyURL);
+ webview.src = 'http:';
document.body.appendChild(webview);
}

@@ -1475,17 +1481,20 @@ function testLoadAbortInvalidNavigation() {
// pseudo-scheme fires loadabort and doesn't cause a crash.
function testLoadAbortNonWebSafeScheme() {
var webview = document.createElement('webview');
- var chromeGuestURL = 'chrome-guest://abc123';
+ var chromeGuestURL = 'chrome-guest://abc123/';
webview.addEventListener('loadabort', function(e) {
embedder.test.assertEq('ERR_ABORTED', e.reason);
- embedder.test.assertEq('chrome-guest://abc123/', e.url);
+ embedder.test.assertEq(chromeGuestURL, e.url);
+ });
+ webview.addEventListener('loadstop', function(e) {
+ embedder.test.assertEq('about:blank', webview.src);
embedder.test.succeed();
});
webview.addEventListener('exit', function(e) {
// We should not crash.
embedder.test.fail();
});
- webview.setAttribute('src', chromeGuestURL);
+ webview.src = chromeGuestURL;
document.body.appendChild(webview);
};

Index: extensions/browser/guest_view/web_view/web_view_guest.cc
diff --git a/extensions/browser/guest_view/web_view/web_view_guest.cc
b/extensions/browser/guest_view/web_view/web_view_guest.cc
index
e9c25ba23e1afb1c42174ed948b7be2d1647e083..8eeb22da233cc69f49e63d1fb285fa9d4a680a8b
100644
--- a/extensions/browser/guest_view/web_view/web_view_guest.cc
+++ b/extensions/browser/guest_view/web_view/web_view_guest.cc
@@ -879,6 +879,7 @@ void WebViewGuest::NavigateGuest(const std::string& src,
if (scheme_is_blocked || !url.is_valid()) {
LoadAbort(true /* is_top_level */, url,
net::ErrorToShortString(net::ERR_ABORTED));
+ NavigateGuest("about:blank", true /* force_navigation */);
return;
}
if (!force_navigation && (src_ == url))
Index: extensions/test/data/web_view/apitest/main.js
diff --git a/extensions/test/data/web_view/apitest/main.js
b/extensions/test/data/web_view/apitest/main.js
index
4495ed432e3a273422948458b5a7092619c2fb68..95fee6e4de4fbb14ebbb88a0bcddf0a72a7bedff
100644
--- a/extensions/test/data/web_view/apitest/main.js
+++ b/extensions/test/data/web_view/apitest/main.js
@@ -964,16 +964,14 @@ function testLoadAbortEmptyResponse() {
// chrome URL is provided.
function testLoadAbortIllegalChromeURL() {
var webview = document.createElement('webview');
- var onFirstLoadStop = function(e) {
- webview.removeEventListener('loadstop', onFirstLoadStop);
- webview.setAttribute('src', 'chrome://newtab');
- };
- webview.addEventListener('loadstop', onFirstLoadStop);
webview.addEventListener('loadabort', function(e) {
embedder.test.assertEq('ERR_ABORTED', e.reason);
+ });
+ webview.addEventListener('loadstop', function(e) {
+ embedder.test.assertEq('about:blank', webview.src);
embedder.test.succeed();
});
- webview.setAttribute('src', 'about:blank');
+ webview.src = 'chrome://newtab';
document.body.appendChild(webview);
}

@@ -981,9 +979,12 @@ function testLoadAbortIllegalFileURL() {
var webview = document.createElement('webview');
webview.addEventListener('loadabort', function(e) {
embedder.test.assertEq('ERR_ABORTED', e.reason);
+ });
+ webview.addEventListener('loadstop', function(e) {
+ embedder.test.assertEq('about:blank', webview.src);
embedder.test.succeed();
});
- webview.setAttribute('src', 'file://foo');
+ webview.src = 'file://foo';
document.body.appendChild(webview);
}

@@ -991,6 +992,9 @@ function testLoadAbortIllegalJavaScriptURL() {
var webview = document.createElement('webview');
webview.addEventListener('loadabort', function(e) {
embedder.test.assertEq('ERR_ABORTED', e.reason);
+ });
+ webview.addEventListener('loadstop', function(e) {
+ embedder.test.assertEq('about:blank', webview.src);
embedder.test.succeed();
});

webview.setAttribute('src', 'javascript:void(document.bgColor="#0000FF")');
@@ -1000,17 +1004,19 @@ function testLoadAbortIllegalJavaScriptURL() {
// Verifies that navigating to invalid URL (e.g. 'http:') doesn't cause a
crash.
function testLoadAbortInvalidNavigation() {
var webview = document.createElement('webview');
- var validSchemeWithEmptyURL = 'http:';
webview.addEventListener('loadabort', function(e) {
embedder.test.assertEq('ERR_ABORTED', e.reason);
embedder.test.assertEq('', e.url);
+ });
+ webview.addEventListener('loadstop', function(e) {
+ embedder.test.assertEq('about:blank', webview.src);
embedder.test.succeed();
});
webview.addEventListener('exit', function(e) {
// We should not crash.
embedder.test.fail();
});
- webview.setAttribute('src', validSchemeWithEmptyURL);
+ webview.src = 'http:';
document.body.appendChild(webview);
}

@@ -1018,17 +1024,20 @@ function testLoadAbortInvalidNavigation() {
// pseudo-scheme fires loadabort and doesn't cause a crash.
function testLoadAbortNonWebSafeScheme() {
var webview = document.createElement('webview');
- var chromeGuestURL = 'chrome-guest://abc123';
+ var chromeGuestURL = 'chrome-guest://abc123/';
webview.addEventListener('loadabort', function(e) {
embedder.test.assertEq('ERR_ABORTED', e.reason);
- embedder.test.assertEq('chrome-guest://abc123/', e.url);
+ embedder.test.assertEq(chromeGuestURL, e.url);
+ });
+ webview.addEventListener('loadstop', function(e) {
+ embedder.test.assertEq('about:blank', webview.src);
embedder.test.succeed();
});
webview.addEventListener('exit', function(e) {
// We should not crash.
embedder.test.fail();
});
- webview.setAttribute('src', chromeGuestURL);
+ webview.src = chromeGuestURL;
document.body.appendChild(webview);
};

[laz...@chromium.org]

It is unclear to me how this fixes crbug.com/450125, can you explain? Also
possibly add those comments/explanation to WebViewGuest::NavigateGuest() and
update the CL description.


https://codereview.chromium.org/888563003/diff/20001/extensions/browser/guest_view/web_view/web_view_guest.cc
File extensions/browser/guest_view/web_view/web_view_guest.cc (right):

https://codereview.chromium.org/888563003/diff/20001/extensions/browser/guest_view/web_view/web_view_guest.cc#newcode882
extensions/browser/guest_view/web_view/web_view_guest.cc:882:

NavigateGuest("about:blank", true /* force_navigation */);

s/"about:blank"/kAboutBlankURL

https://codereview.chromium.org/888563003/

[fsa...@chromium.org]

PTAL

https://codereview.chromium.org/888563003/

[laz...@chromium.org]

lgtm




https://chromiumcodereview.appspot.com/888563003/diff/40001/extensions/browser/guest_view/web_view/web_view_guest.cc
File extensions/browser/guest_view/web_view/web_view_guest.cc (right):

https://chromiumcodereview.appspot.com/888563003/diff/40001/extensions/browser/guest_view/web_view/web_view_guest.cc#newcode882
extensions/browser/guest_view/web_view/web_view_guest.cc:882:
NavigateGuest(webview::kAboutBlankURL, true /* force_navigation */);
Sorry for being vague here, I meant to use kAboutBlankURL from
url_constants.h

https://chromiumcodereview.appspot.com/888563003/diff/40001/extensions/browser/guest_view/web_view/web_view_guest.cc#newcode882
extensions/browser/guest_view/web_view/web_view_guest.cc:882:
NavigateGuest(webview::kAboutBlankURL, true /* force_navigation */);
Also add a comment here.

https://chromiumcodereview.appspot.com/888563003/

[cr...@chromium.org]

I agree that this is an improvement over the current <webview> blocked URL
behavior, so LGTM once lazyboy's comments are fixed.

However, I think this is more about improving the general window.open case
in
<webview> (blocking chrome:// navigations) than Language Settings.
Mentioning
Language Settings in the CL description is confusing to me, because that's a
feature that *should* work. (A guest should not be able to window.open to
the
"chrome://settings/languages" URL, but if there is a context menu item for
Language Settings, it should work.)


https://codereview.chromium.org/888563003/diff/40001/extensions/browser/guest_view/web_view/web_view_constants.h
File extensions/browser/guest_view/web_view/web_view_constants.h
(right):

https://codereview.chromium.org/888563003/diff/40001/extensions/browser/guest_view/web_view/web_view_constants.h#newcode113
extensions/browser/guest_view/web_view/web_view_constants.h:113: extern
const char kAboutBlankURL[];
As lazyboy@ mentioned, just use the existing url::kAboutBlankURL rather
than introducing a new one.

https://codereview.chromium.org/888563003/

[cr...@chromium.org]

(In other words, Language Settings should *not* fire a newwindow event.
It's a
browser-initiated event that should go directly to the embedder
WebContents's
OpenURL.)

https://codereview.chromium.org/888563003/

[fsa...@chromium.org]

On 2015/02/03 18:16:31, Charlie Reis wrote:
> (In other words, Language Settings should *not* fire a newwindow event.
> It's
a
> browser-initiated event that should go directly to the embedder
> WebContents's
> OpenURL.)

I see I'll update the other patch. Thnanks!

https://codereview.chromium.org/888563003/

[commi...@chromium.org]

CQ is trying da patch. Follow status at
https://chromium-cq-status.appspot.com/patch-status/888563003/60001


https://codereview.chromium.org/888563003/

[commi...@chromium.org]

Committed patchset #4 (id:60001)

https://codereview.chromium.org/888563003/

[commi...@chromium.org]

Patchset 4 (id:??) landed as
https://crrev.com/fb12824b8b2caf76dbb341ef53f3225d09f0ca86
Cr-Commit-Position: refs/heads/master@{#314517}

https://codereview.chromium.org/888563003/