<webview>: navigating to WebStore should fire a loadabort instead of crashing.

<webview>: navigating to WebStore should fire a loadabort instead of crashing.

All top-level navigations now get plumbed through: BrowserPluginGuest::LoadURLWithParams.
URL validation now happens there. If a URL is determined to be inappropriate for a <webview>,
a loadabort event will fire instead of crashing the <webview> guest content.

BUG=334531
Test=WebViewTest.Shim_TestNavigateToWebStore

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=248828

[nasko, 2014-01-16 21:27:59]

LGTM content/browser/browser_plugin/browser_plugin_guest.cc

[sadrul, 2014-01-16 21:43:54]

I started looking at the test, and realized I am not really familiar with
executeScript/postMessage all that much, and I don't have JS readability either.
So I only looked for nits etc. instead, and punting off to lazyboy@

https://codereview.chromium.org/140073002/diff/30001/chrome/test/data/extensi...
File
chrome/test/data/extensions/platform_apps/web_view/shim/inject_comm_channel.js
(right):

https://codereview.chromium.org/140073002/diff/30001/chrome/test/data/extensi...
chrome/test/data/extensions/platform_apps/web_view/shim/inject_comm_channel.js:27:
if (!embedder.processMessage(data)) {
This will always be true? or does something override embedder.processMessage
function above?

.. I notice that it is overridden in a file. Perhaps leave a comment somewhere
in this file that explains this?

https://codereview.chromium.org/140073002/diff/30001/chrome/test/data/extensi...
File chrome/test/data/extensions/platform_apps/web_view/shim/main.js (right):

https://codereview.chromium.org/140073002/diff/30001/chrome/test/data/extensi...
chrome/test/data/extensions/platform_apps/web_view/shim/main.js:1291:
window.console.log('Navigation to \'' + e.url + '\' has been aborted.');
Considering the console logs show up in the test-run logs, do we really want
these?

https://codereview.chromium.org/140073002/diff/30001/chrome/test/data/extensi...
chrome/test/data/extensions/platform_apps/web_view/shim/main.js:1299:
//embedder.test.succeed();
remove?

[lazyboy, 2014-01-16 22:31:24]

https://chromiumcodereview.appspot.com/140073002/diff/30001/chrome/test/data/...
File
chrome/test/data/extensions/platform_apps/web_view/shim/inject_webstore_nav_test.js
(right):

https://chromiumcodereview.appspot.com/140073002/diff/30001/chrome/test/data/...
chrome/test/data/extensions/platform_apps/web_view/shim/inject_webstore_nav_test.js:7:
case 'navigate': {
nit: here and in other places, remove { .. } after case.

https://chromiumcodereview.appspot.com/140073002/diff/30001/chrome/test/data/...
File chrome/test/data/extensions/platform_apps/web_view/shim/main.js (right):

https://chromiumcodereview.appspot.com/140073002/diff/30001/chrome/test/data/...
chrome/test/data/extensions/platform_apps/web_view/shim/main.js:1315: var msg =
['navigate', CHROME_WEB_STORE];
The assumption here is that inject_webstore_nav_test.js is already loaded at
this point, which is not guaranteed, right? At this point inject_comm_channel.js
is only guaranteed to be loaded, wouldn't this be a race?

https://chromiumcodereview.appspot.com/140073002/diff/30001/chrome/test/data/...
chrome/test/data/extensions/platform_apps/web_view/shim/main.js:1331:
console.log('Unexpected message: \'' + data[0]  + '\'');
nit: try being consistent within the file, console.log or window.console.log.

https://chromiumcodereview.appspot.com/140073002/diff/30001/content/browser/b...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://chromiumcodereview.appspot.com/140073002/diff/30001/content/browser/b...
content/browser/browser_plugin/browser_plugin_guest.cc:614:
renderer_prefs->browser_handles_all_top_level_requests = true;
I remember creis@ raising concerns doing this before, are those concerns not an
issue anymore?

[Fady Samuel, 2014-01-16 23:40:26]

PTAL nasko. I have made some changes to fix tests. In particular, what are your
thoughts about the RenderViewHostImpl::OnOpenURL change?

https://codereview.chromium.org/140073002/diff/30001/chrome/test/data/extensi...
File
chrome/test/data/extensions/platform_apps/web_view/shim/inject_comm_channel.js
(right):

https://codereview.chromium.org/140073002/diff/30001/chrome/test/data/extensi...
chrome/test/data/extensions/platform_apps/web_view/shim/inject_comm_channel.js:27:
if (!embedder.processMessage(data)) {
On 2014/01/16 21:43:55, sadrul wrote:
> This will always be true? or does something override embedder.processMessage
> function above?
> 
> .. I notice that it is overridden in a file. Perhaps leave a comment somewhere
> in this file that explains this?

Done.

https://codereview.chromium.org/140073002/diff/30001/chrome/test/data/extensi...
File
chrome/test/data/extensions/platform_apps/web_view/shim/inject_webstore_nav_test.js
(right):

https://codereview.chromium.org/140073002/diff/30001/chrome/test/data/extensi...
chrome/test/data/extensions/platform_apps/web_view/shim/inject_webstore_nav_test.js:7:
case 'navigate': {
On 2014/01/16 22:31:24, lazyboy wrote:
> nit: here and in other places, remove { .. } after case.

Done.

https://codereview.chromium.org/140073002/diff/30001/chrome/test/data/extensi...
File chrome/test/data/extensions/platform_apps/web_view/shim/main.js (right):

https://codereview.chromium.org/140073002/diff/30001/chrome/test/data/extensi...
chrome/test/data/extensions/platform_apps/web_view/shim/main.js:1291:
window.console.log('Navigation to \'' + e.url + '\' has been aborted.');
On 2014/01/16 21:43:55, sadrul wrote:
> Considering the console logs show up in the test-run logs, do we really want
> these?

I feel they are useful. If a test hangs in the future, we'll know where it
failed. This helps us isolate the problem.

https://codereview.chromium.org/140073002/diff/30001/chrome/test/data/extensi...
chrome/test/data/extensions/platform_apps/web_view/shim/main.js:1299:
//embedder.test.succeed();
On 2014/01/16 21:43:55, sadrul wrote:
> remove?

Done.

https://codereview.chromium.org/140073002/diff/30001/chrome/test/data/extensi...
chrome/test/data/extensions/platform_apps/web_view/shim/main.js:1315: var msg =
['navigate', CHROME_WEB_STORE];
On 2014/01/16 22:31:24, lazyboy wrote:
> The assumption here is that inject_webstore_nav_test.js is already loaded at
> this point, which is not guaranteed, right? At this point
inject_comm_channel.js
> is only guaranteed to be loaded, wouldn't this be a race?

Ohh, good catch! Fixed!

https://codereview.chromium.org/140073002/diff/30001/chrome/test/data/extensi...
chrome/test/data/extensions/platform_apps/web_view/shim/main.js:1331:
console.log('Unexpected message: \'' + data[0]  + '\'');
On 2014/01/16 22:31:24, lazyboy wrote:
> nit: try being consistent within the file, console.log or window.console.log.

Done.

https://codereview.chromium.org/140073002/diff/30001/content/browser/browser_...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/140073002/diff/30001/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_guest.cc:614:
renderer_prefs->browser_handles_all_top_level_requests = true;
On 2014/01/16 22:31:24, lazyboy wrote:
> I remember creis@ raising concerns doing this before, are those concerns not
an
> issue anymore?

Nasko? Thoughts? Charlie is on paternity leave.

[nasko, 2014-01-17 00:00:17]

On 2014/01/16 23:40:26, Fady Samuel wrote:
> PTAL nasko. I have made some changes to fix tests. In particular, what are
your
> thoughts about the RenderViewHostImpl::OnOpenURL change?
> 
>
https://codereview.chromium.org/140073002/diff/30001/content/browser/browser_...
> File content/browser/browser_plugin/browser_plugin_guest.cc (right):
> 
>
https://codereview.chromium.org/140073002/diff/30001/content/browser/browser_...
> content/browser/browser_plugin/browser_plugin_guest.cc:614:
> renderer_prefs->browser_handles_all_top_level_requests = true;
> On 2014/01/16 22:31:24, lazyboy wrote:
> > I remember creis@ raising concerns doing this before, are those concerns not
> an
> > issue anymore?
> 
> Nasko? Thoughts? Charlie is on paternity leave.

I'm not seeing why FilterURL will fail for the WebStore URL. It looks to me that
the CanCommitURL check is the failing one. If this is correct, then the
OnOpenURL check is not needed.

As I mentioned in chat earlier, ideally we won't send all top level navigations
to the browser process and will make the decision on which ones in the renderer
process. The check we perform in ChromeContentBrowserClient::CanCommitURL can
also be done in ChromeContentRendererClient::ShouldFork, which will accomplish
sending only navigations to the web store URL to the browser process.
Alternatively, you could try and find a way to abort the load inside the
renderer process.

[nasko, 2014-01-17 16:32:26]

After offline discussion, this seems a reasonable solution for now. LGTM

[Fady Samuel, 2014-01-17 16:37:29]

Istiaque, could you please take another look at the tests? Thanks!

[lazyboy, 2014-01-17 20:42:53]

https://chromiumcodereview.appspot.com/140073002/diff/180001/chrome/test/data...
File chrome/test/data/extensions/platform_apps/web_view/shim/main.js (right):

https://chromiumcodereview.appspot.com/140073002/diff/180001/chrome/test/data...
chrome/test/data/extensions/platform_apps/web_view/shim/main.js:1279:
window.console.log(
Here ^^^

https://chromiumcodereview.appspot.com/140073002/diff/180001/chrome/test/data...
chrome/test/data/extensions/platform_apps/web_view/shim/main.js:1285:
webview.contentWindow.postMessage(JSON.stringify(msg), '*');
I'd move this inside the executeScript results above ^^^

[Fady Samuel, 2014-01-17 21:54:35]

PTAL Istiaque

https://chromiumcodereview.appspot.com/140073002/diff/180001/chrome/test/data...
File chrome/test/data/extensions/platform_apps/web_view/shim/main.js (right):

https://chromiumcodereview.appspot.com/140073002/diff/180001/chrome/test/data...
chrome/test/data/extensions/platform_apps/web_view/shim/main.js:1279:
window.console.log(
On 2014/01/17 20:42:54, lazyboy wrote:
> Here ^^^

Done.

https://chromiumcodereview.appspot.com/140073002/diff/180001/chrome/test/data...
chrome/test/data/extensions/platform_apps/web_view/shim/main.js:1285:
webview.contentWindow.postMessage(JSON.stringify(msg), '*');
On 2014/01/17 20:42:54, lazyboy wrote:
> I'd move this inside the executeScript results above ^^^

Done.

[lazyboy, 2014-01-17 21:56:13]

lgtm

Thanks for making the changes.

[commit-bot: I haz the power, 2014-01-17 21:59:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/140073002/320001

[commit-bot: I haz the power, 2014-01-17 22:27:16]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Fady Samuel, 2014-01-17 22:32:06]

+jam@: John, Charlie is OOO, could you please sign off on this as a content
OWNER? Thanks!

[jam, 2014-01-17 23:09:14]

lgtm

https://codereview.chromium.org/140073002/diff/320001/content/browser/browser...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/140073002/diff/320001/content/browser/browser...
content/browser/browser_plugin/browser_plugin_guest.cc:614: // navigations still
continue to function inside the app.
nit: does this need to be updated?

[Fady Samuel, 2014-01-18 22:45:57]

Addressed nits, CQ'ing now..

https://codereview.chromium.org/140073002/diff/320001/content/browser/browser...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/140073002/diff/320001/content/browser/browser...
content/browser/browser_plugin/browser_plugin_guest.cc:614: // navigations still
continue to function inside the app.
On 2014/01/17 23:09:15, jam wrote:
> nit: does this need to be updated?

Updated comment.

[commit-bot: I haz the power, 2014-01-18 22:46:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/140073002/460001

[commit-bot: I haz the power, 2014-01-18 23:36:18]

Retried try job too often on mac_rel for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2014-01-19 14:20:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/140073002/460001

[commit-bot: I haz the power, 2014-01-20 12:23:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/140073002/460001

[commit-bot: I haz the power, 2014-01-20 21:48:28]

Retried try job too often on win_rel for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2014-01-20 21:55:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/140073002/460001

[cmp, 2014-01-20 21:55:39]

On 2014/01/20 21:48:28, I haz the power (commit-bot) wrote:
> Retried try job too often on win_rel for step(s) interactive_ui_tests
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

I'm rechecking the CQ box on this CL.  It had been unfairly stuck on a hanging
compile step.

[commit-bot: I haz the power, 2014-01-20 23:44:32]

Retried try job too often on win_rel for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2014-01-21 17:32:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/140073002/460001

[cmp, 2014-01-21 17:35:14]

On 2014/01/21 17:32:13, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
> https://chromium-status.appspot.com/cq/fsamuel%40chromium.org/140073002/460001

Is the assumption for re-running that the win_rel interactive_ui_tests failure
is not related to this CL?  From the results here, it appears to show up on each
run on that bot with this patch.

[Fady Samuel, 2014-01-21 17:47:57]

On 2014/01/21 17:35:14, cmp wrote:
> On 2014/01/21 17:32:13, I haz the power (commit-bot) wrote:
> > CQ is trying da patch. Follow status at
> >
https://chromium-status.appspot.com/cq/fsamuel%2540chromium.org/140073002/460001
> 
> Is the assumption for re-running that the win_rel interactive_ui_tests failure
> is not related to this CL?  From the results here, it appears to show up on
each
> run on that bot with this patch.

Yea, it appears you're right. I was hoping it was flake. I'll have to see what's
going on on Windows. Mac is also failing but I'm unable to get a failure on my
local run of WebViewInteractiveTest.NewWindow_WebRequestRemoveElement on my
MacBook Pro...

[commit-bot: I haz the power, 2014-01-21 22:46:22]

Retried try job too often on win_rel for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2014-01-29 21:09:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/140073002/460001

[commit-bot: I haz the power, 2014-01-29 23:29:11]

Retried try job too often on win_rel for step(s) browser_tests,
interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[Fady Samuel, 2014-02-04 00:37:40]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2014-02-04 00:38:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/140073002/1000001

[commit-bot: I haz the power, 2014-02-04 02:20:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-04 02:20:22]

Retried try job too often on win_rel for step(s) browser_tests,
nacl_integration, sync_integration_tests, telemetry_perf_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[Fady Samuel, 2014-02-04 02:44:46]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2014-02-04 02:45:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/140073002/1000001

[commit-bot: I haz the power, 2014-02-04 03:13:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-04 03:13:13]

Retried try job too often on win for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[Fady Samuel, 2014-02-04 03:15:31]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2014-02-04 03:16:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/140073002/1000001

[commit-bot: I haz the power, 2014-02-04 07:24:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-04 07:24:02]

Retried try job too often on win_rel for step(s) browser_tests,
sync_integration_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[Fady Samuel, 2014-02-04 12:13:53]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2014-02-04 12:14:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/140073002/1000001

[commit-bot: I haz the power, 2014-02-04 16:42:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-04 16:42:13]

Retried try job too often on win_rel for step(s) browser_tests,
sync_integration_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[Fady Samuel, 2014-02-04 17:38:49]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2014-02-04 17:41:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/140073002/1000001

[commit-bot: I haz the power, 2014-02-04 20:36:49]

Retried try job too often on win_rel for step(s) browser_tests,
sync_integration_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2014-02-04 20:37:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/140073002/1000001

[commit-bot: I haz the power, 2014-02-05 00:51:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/140073002/1000001

[commit-bot: I haz the power, 2014-02-05 02:04:31]

Change committed as 248828