Support webview tag when the container extension is embedded in a webUI

chrome/browser/chrome_content_browser_client.cc

Index: chrome/browser/chrome_content_browser_client.cc
diff --git a/chrome/browser/chrome_content_browser_client.cc b/chrome/browser/chrome_content_browser_client.cc
index 28382b921452de51c659ab88d84c5da821893bf2..d6848b8d412125dd15d7673418cc3ba97e5bf9ca 100644
--- a/chrome/browser/chrome_content_browser_client.cc
+++ b/chrome/browser/chrome_content_browser_client.cc
@@ -126,6 +126,7 @@
 #include "content/public/browser/web_contents_view.h"
 #include "content/public/common/child_process_host.h"
 #include "content/public/common/content_descriptors.h"
+#include "content/public/common/url_utils.h"
 #include "extensions/browser/view_type_utils.h"
 #include "extensions/common/constants.h"
 #include "extensions/common/switches.h"
@@ -803,6 +804,7 @@ void ChromeContentBrowserClient::GuestWebContentsCreated(
 void ChromeContentBrowserClient::GuestWebContentsAttached(
     WebContents* guest_web_contents,
     WebContents* embedder_web_contents,
+    const GURL& embedder_frame_url,
     const base::DictionaryValue& extra_params) {
   Profile* profile = Profile::FromBrowserContext(
       embedder_web_contents->GetBrowserContext());
@@ -812,9 +814,13 @@ void ChromeContentBrowserClient::GuestWebContentsAttached(
     NOTREACHED();
     return;
   }
-  const GURL& url = embedder_web_contents->GetSiteInstance()->GetSiteURL();
-  const Extension* extension =
-      service->extensions()->GetExtensionOrAppByURL(url);
+
+  // Only trust |embedder_frame_url| reported by a WebUI renderer.

[Charlie Reis, 2013/09/13 21:17:24]

Let's be a little more explicit here.  Something like:

We usually require BrowserPlugins to be hosted by a storage isolated extension. 
We treat WebUI pages as a special case if they host the BrowserPlugin in a
component extension iframe.  In that case, we use the iframe's URL to determine
the extension.

[guohui, 2013/09/16 22:27:02]

Done.

+  const GURL& embedder_site_url =
+      embedder_web_contents->GetSiteInstance()->GetSiteURL();
+  const Extension* extension = service->extensions()->GetExtensionOrAppByURL(
+      content::HasWebUIScheme(embedder_site_url) ?
+          embedder_frame_url : embedder_site_url);
   if (!extension) {

[Charlie Reis, 2013/09/13 21:17:24]

Fady, would it make sense to add || !extension->is_storage_isolated() to this
check?  (Or perhaps it belongs in SupportsBrowserPlugin?)

If not, we might try to create a guest partition inside the default profile
directory, which would be very bad.

[guohui, 2013/09/16 22:27:02]

A webview (browser plugin) always has an isolated storage, regardless of the
isolation setting of the embedder ext/app. The partition is put under [profile
name]/storage/ext/[ext-id]/[partition-id].
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...

Actually, storage isolation setting is only supported for apps, but not for
extensions (in the strict sense), so the check for is_stoarge_isolated does not
make sense for component extensions.

[Charlie Reis, 2013/09/18 17:42:23]

Ok.  Fady and I talked about this and skipping this check should be ok as long
as we explicitly support creating storage/ext/[ext-id] directories for
non-storage-isolated extensions when they can host guests.  This requires tests
and making sure that the directory won't be deleted on cleanup.  See
http://crbug.com/293722.

[guohui, 2013/09/19 13:48:41]

Can we address this issue in a separate CL?

[Charlie Reis, 2013/09/19 17:21:22]

Yes.  As we discussed on https://codereview.chromium.org/23653012/, the
StoragePartition management is a prerequisite to support webviews in WebUI and
component extensions in general.  As long as your component extension is the
only one on the whitelist and you're ok with the risk of bugs, we can proceed
with this independently of the fix for http://crbug.com/293722.

[guohui, 2013/09/19 17:59:11]

Done.

     // It's ok to return here, since we could be running a browser plugin
     // outside an extension, and don't need to attach a
@@ -2342,6 +2348,9 @@ bool ChromeContentBrowserClient::SupportsBrowserPlugin(
           switches::kEnableBrowserPluginForAllViewTypes))
     return true;
 
+  if (content::HasWebUIScheme(site_url))

[Charlie Reis, 2013/09/13 21:17:24]

Do we need all WebUI pages to be able to do this, or just particular ones that
we might be able to whitelist here?  There are enough requirements (e.g., must
host in an component extension iframe, extension must be storage isolated) that
I'm hesitant to open it up to all WebUI pages.

I'm also wondering if it's easy to check the embedder_frame_url here as you've
done above, or if that's difficult to pass in here.

[guohui, 2013/09/16 22:27:02]

As i replied to Fady, SupportsBrowserPlugin is called before RenderViewHost is
even created, so there is no way we could get the frame URL at this time point.

Also, this method is only called to determine whether the RenderViewHost should
monitor messages from guests, we have other checks in places that prevents
webview from creation outside of the context of a component extension with
webview permission declared.

[Charlie Reis, 2013/09/18 17:42:23]

Ok, let's skip the frame URL check.
Where are those checks?  Can we limit the support to particular WebUI URLs, or
is it necessary to support webviews in all WebUI processes?  I'd prefer to lock
it down to a whitelist if possible, since not all WebUI pages will have the
constraints you're relying on.  More general support will be possible with
out-of-process iframes.

[guohui, 2013/09/19 13:48:41]

I prefer to allows it for all WebUI URLs if possible. The reason is we want to
embed the gaia auth extension in any WebUI that needs to trigger inline signin
flow. To avoid unwanted use until general support is available,  i limited the
feature to whitelisted component extensions only in the CL 23530029.

[Charlie Reis, 2013/09/19 17:21:22]

Ok.  The whitelist in CL 23653012 should be sufficient.

[guohui, 2013/09/19 17:59:11]

Done.

+    return true;
+
   Profile* profile = Profile::FromBrowserContext(browser_context);
   ExtensionService* service =
       extensions::ExtensionSystem::Get(profile)->extension_service();