Use default CSP for resource loading in webview (instead of platform app's CSP)

Use default CSP for resource loading in webview (instead of platform app's CSP)

<webview> loads page in an isolated context inside platform app and hosts drive-by web. Platform app's CSP is too restrictive for <webview>, we stop using that CSP and use the default instead in this CL.

BUG=363437
Test=Load an chrome app. Load a webview html from accessible resources. Make the webview page contain inline JS. Check that the JS loads. It didn't use to load w/o this CL.

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

[lazyboy, 2014-04-15 00:07:15]

Hi James,
Can you review the CL?
I will pass it to a process/security reviewer once you're happy with the CL.
Thanks.

[James Cook, 2014-04-15 17:21:21]

yoz, can you take a look at this as an extensions OWNER?

lazyboy, apologies for the delay in getting to this. You might want to check
first with yoz if he's OK with the method you're adding to this interface before
addressing my comments.

https://chromiumcodereview.appspot.com/237793003/diff/40001/apps/shell/browse...
File apps/shell/browser/shell_extensions_browser_client.h (right):

https://chromiumcodereview.appspot.com/237793003/diff/40001/apps/shell/browse...
apps/shell/browser/shell_extensions_browser_client.h:1: // Copyright 2013 The
Chromium Authors. All rights reserved.
About the change description - that bug is marked fixed. Is it the right bug? 
Also, maybe include another line of detail on why this change needs to happen. 
(I'm not an expert on CSP.)

https://chromiumcodereview.appspot.com/237793003/diff/40001/chrome/browser/ex...
File chrome/browser/extensions/chrome_extensions_browser_client.cc (right):

https://chromiumcodereview.appspot.com/237793003/diff/40001/chrome/browser/ex...
chrome/browser/extensions/chrome_extensions_browser_client.cc:120: const
content::ResourceRequestInfo* info =
I would implement this as a utility function in url_request_util.  Otherwise
this file is going to get increasingly bloated.  url_request_util.cc should
already #include the headers you need.

https://chromiumcodereview.appspot.com/237793003/diff/40001/extensions/browse...
File extensions/browser/extensions_browser_client.h (right):

https://chromiumcodereview.appspot.com/237793003/diff/40001/extensions/browse...
extensions/browser/extensions_browser_client.h:101: virtual bool
IsWebViewRequest(net::URLRequest* request) const = 0;
It's unfortunate that ExtensionsBrowserClient needs a method to support a
specific tag.  Likewise we hope to support <webview> in apps/shell some day. 
However, I'm not sure how else to do this and maybe webview is enough of a
special case to merit it.  yoz, opinion?

If yoz is OK with this you'll also need to override this function in
extensions/browser/test_extensions_browser_client.h

[lazyboy, 2014-04-15 17:26:31]

Thanks, let's wait for comments from Yoyo.

https://chromiumcodereview.appspot.com/237793003/diff/40001/apps/shell/browse...
File apps/shell/browser/shell_extensions_browser_client.h (right):

https://chromiumcodereview.appspot.com/237793003/diff/40001/apps/shell/browse...
apps/shell/browser/shell_extensions_browser_client.h:1: // Copyright 2013 The
Chromium Authors. All rights reserved.
On 2014/04/15 17:21:21, James Cook wrote:
> About the change description - that bug is marked fixed. Is it the right bug? 
> Also, maybe include another line of detail on why this change needs to happen.

> (I'm not an expert on CSP.)

http://crbug.com/363437 is the right one. Description updated.

[Yoyo Zhou, 2014-04-15 21:40:05]

https://chromiumcodereview.appspot.com/237793003/diff/40001/chrome/browser/ex...
File chrome/browser/extensions/chrome_extensions_browser_client.cc (right):

https://chromiumcodereview.appspot.com/237793003/diff/40001/chrome/browser/ex...
chrome/browser/extensions/chrome_extensions_browser_client.cc:120: const
content::ResourceRequestInfo* info =
On 2014/04/15 17:21:21, James Cook wrote:
> I would implement this as a utility function in url_request_util.  Otherwise
> this file is going to get increasingly bloated.  url_request_util.cc should
> already #include the headers you need.

+1

https://chromiumcodereview.appspot.com/237793003/diff/40001/extensions/browse...
File extensions/browser/extensions_browser_client.h (right):

https://chromiumcodereview.appspot.com/237793003/diff/40001/extensions/browse...
extensions/browser/extensions_browser_client.h:101: virtual bool
IsWebViewRequest(net::URLRequest* request) const = 0;
On 2014/04/15 17:21:21, James Cook wrote:
> It's unfortunate that ExtensionsBrowserClient needs a method to support a
> specific tag.  Likewise we hope to support <webview> in apps/shell some day. 
> However, I'm not sure how else to do this and maybe webview is enough of a
> special case to merit it.  yoz, opinion?
> 
> If yoz is OK with this you'll also need to override this function in
> extensions/browser/test_extensions_browser_client.h

I think it's okay to add this for now; when we make webview available to
appshell, we'll need to refactor things like ExtensionRendererState (which
tracks both tabs and webviews).

[lazyboy, 2014-04-15 22:17:02]

Comments addressed in patchset #4.

https://chromiumcodereview.appspot.com/237793003/diff/40001/chrome/browser/ex...
File chrome/browser/extensions/chrome_extensions_browser_client.cc (right):

https://chromiumcodereview.appspot.com/237793003/diff/40001/chrome/browser/ex...
chrome/browser/extensions/chrome_extensions_browser_client.cc:120: const
content::ResourceRequestInfo* info =
On 2014/04/15 21:40:06, Yoyo Zhou wrote:
> On 2014/04/15 17:21:21, James Cook wrote:
> > I would implement this as a utility function in url_request_util.  Otherwise
> > this file is going to get increasingly bloated.  url_request_util.cc should
> > already #include the headers you need.
> 
> +1

Done.

https://chromiumcodereview.appspot.com/237793003/diff/40001/extensions/browse...
File extensions/browser/extensions_browser_client.h (right):

https://chromiumcodereview.appspot.com/237793003/diff/40001/extensions/browse...
extensions/browser/extensions_browser_client.h:101: virtual bool
IsWebViewRequest(net::URLRequest* request) const = 0;
On 2014/04/15 21:40:06, Yoyo Zhou wrote:
> On 2014/04/15 17:21:21, James Cook wrote:
> > It's unfortunate that ExtensionsBrowserClient needs a method to support a
> > specific tag.  Likewise we hope to support <webview> in apps/shell some day.

> > However, I'm not sure how else to do this and maybe webview is enough of a
> > special case to merit it.  yoz, opinion?
> > 
> > If yoz is OK with this you'll also need to override this function in
> > extensions/browser/test_extensions_browser_client.h
> 
> I think it's okay to add this for now; when we make webview available to
> appshell, we'll need to refactor things like ExtensionRendererState (which
> tracks both tabs and webviews).

Updated test_extensions_browser_client.h

[Yoyo Zhou, 2014-04-15 22:20:26]

LGTM

[James Cook, 2014-04-15 22:38:38]

LGTM assuming you fix the includes

https://chromiumcodereview.appspot.com/237793003/diff/60001/chrome/browser/ex...
File chrome/browser/extensions/chrome_extensions_browser_client.cc (right):

https://chromiumcodereview.appspot.com/237793003/diff/60001/chrome/browser/ex...
chrome/browser/extensions/chrome_extensions_browser_client.cc:18: #include
"chrome/browser/extensions/extension_renderer_state.h"
Remove this include and the one below.

[lazyboy, 2014-04-15 22:41:59]

https://chromiumcodereview.appspot.com/237793003/diff/60001/chrome/browser/ex...
File chrome/browser/extensions/chrome_extensions_browser_client.cc (right):

https://chromiumcodereview.appspot.com/237793003/diff/60001/chrome/browser/ex...
chrome/browser/extensions/chrome_extensions_browser_client.cc:18: #include
"chrome/browser/extensions/extension_renderer_state.h"
On 2014/04/15 22:38:38, James Cook wrote:
> Remove this include and the one below.

Done.

[lazyboy, 2014-04-16 13:01:01]

The CQ bit was checked by lazyboy@chromium.org

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

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lazyboy@chromium.org/237793003/80001

[commit-bot: I haz the power, 2014-04-16 13:58:27]

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

[commit-bot: I haz the power, 2014-04-16 13:58:27]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_rel

[lazyboy, 2014-04-16 14:42:58]

The CQ bit was checked by lazyboy@chromium.org

[commit-bot: I haz the power, 2014-04-16 14:43:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lazyboy@chromium.org/237793003/80001

[commit-bot: I haz the power, 2014-04-16 14:48:30]

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

[commit-bot: I haz the power, 2014-04-16 14:48:31]

Try jobs failed on following builders:
  tryserver.chromium on mac_chromium_rel

[lazyboy, 2014-04-16 15:25:15]

+Fady,
Can you take a look at the
chrome/test/data/extensions/platform_apps/web_view/shim/*?

Currently guest.html has an inline script in it, which is confusing because it
is sometimes loaded as accessible resources in some tests. Inline scripts did
not work from accessible resources (that's what this CL is fixing). So the
scripts were failing silently. However this file was also loaded directly from
file system in NewWindow tests, where scripts *do* run. I've made it so that
guest.html is not loaded as accessible resource anymore and added another file
(guest_with_inline_script.html) that is loaded as accessible resource.

[Fady Samuel, 2014-04-16 15:27:42]

LGTM! Thanks for clearing up the confusion Istiaque! :-)

[lazyboy, 2014-04-16 15:28:09]

The CQ bit was checked by lazyboy@chromium.org

[commit-bot: I haz the power, 2014-04-16 15:28:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lazyboy@chromium.org/237793003/120001

[commit-bot: I haz the power, 2014-04-16 16:36:16]

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

[commit-bot: I haz the power, 2014-04-16 16:36:16]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_rel

[lazyboy, 2014-04-16 16:43:57]

The CQ bit was checked by lazyboy@chromium.org

[commit-bot: I haz the power, 2014-04-16 16:44:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lazyboy@chromium.org/237793003/120001

[commit-bot: I haz the power, 2014-04-16 18:25:27]

Change committed as 264253