Webview tag creation should be using storage partitions.

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 b765d7184df828fbcd03fe075ff277c74ff88d96..c532796ad06cc196aa2f11f6ff972de6debc17a1 100644
--- a/chrome/browser/chrome_content_browser_client.cc
+++ b/chrome/browser/chrome_content_browser_client.cc
@@ -12,6 +12,7 @@
 #include "base/command_line.h"
 #include "base/path_service.h"
 #include "base/string_tokenizer.h"
+#include "base/string_util.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/app/breakpad_mac.h"
 #include "chrome/browser/browser_about_handler.h"
@@ -100,6 +101,7 @@
 #include "content/public/common/content_descriptors.h"
 #include "grit/generated_resources.h"
 #include "grit/ui_resources.h"
+#include "net/base/escape.h"
 #include "net/base/ssl_cert_request_info.h"
 #include "net/cookies/canonical_cookie.h"
 #include "net/cookies/cookie_options.h"
@@ -468,61 +470,125 @@ content::WebContentsView*
   return NULL;
 }
 
-std::string ChromeContentBrowserClient::GetStoragePartitionIdForChildProcess(
-    content::BrowserContext* browser_context,
-    int child_process_id) {
-  const Extension* extension = NULL;
-  Profile* profile = Profile::FromBrowserContext(browser_context);
-  ExtensionService* extension_service =
-      extensions::ExtensionSystem::Get(profile)->extension_service();
-  if (extension_service) {
-    std::set<std::string> extension_ids =
-        extension_service->process_map()->
-            GetExtensionsInProcess(child_process_id);
-    if (!extension_ids.empty())
-      // Since All the apps in a process share the same storage partition,
-      // we can pick any of them to retrieve the storage partition id.
-      extension =
-          extension_service->extensions()->GetByID(*(extension_ids.begin()));
-  }
-  return GetStoragePartitionIdForExtension(browser_context, extension);
+std::string ChromeContentBrowserClient::GetStoragePartitionIdForSite(
+      content::BrowserContext* browser_context,
+      const GURL& site) {
+  std::string app_id;
+  std::string partition_name;
+  bool in_memory;

[Charlie Reis, 2012/11/05 18:15:37]

Should probably initialize this, in case a bug later crops up in
GetStoragePartitionConfigForSite (e.g., return without setting it).

[nasko, 2012/11/05 19:20:26]

Done.

+
+  // We need to get all the pieces that will go into the storage partition
+  // identifier first, before we compose it.
+  GetStoragePartitionConfigForSite(browser_context, site, &app_id,
+                                   &partition_name, &in_memory);
+
+  // If there is no app, we are in the default browser partition, so return
+  // an empty string.
+  if (app_id.empty())
+    return std::string();
+
+  // A non-empty storage partition id string is of the form
+  // "app_id:in_memory:partition_name", where each of the three parts is

[Charlie Reis, 2012/11/05 18:15:37]

I'm not entirely clear on why we need this format.  I thought we were using site
URLs because they already know how to parse what we need.  (Seems safer to avoid
adding a custom parser to the browser process if we can.)

Wouldn't the site URL have enough information?

[nasko, 2012/11/05 19:20:26]

I've had the same discussion with Albert. He feels strongly against using the
URL as identifier, as different parts of the code may start depending on the
fact that it is URL of specific format and start taking parts of it. This is the
reason for this separate way to identify them.

[awong, 2012/11/05 20:11:18]

My objection was that GURL had too much validation logic. Could be convinced
otherwise though.

+  // optional and the ':' separators are mandatory. Since "in_memory" is fixed
+  // string and the app_id cannot contain the separator, it is safe to parse

[Charlie Reis, 2012/11/05 18:15:37]

What are the restrictions on the partition_name?  Can it contain the separator? 
(I suppose you could still safely parse it, but it's not as obvious by looking
at it.)

[nasko, 2012/11/05 19:20:26]

I do have a bug here, the user supplied partition value doesn't have any
limitations at this point in time. I have to account for that in the parsing and
not expect exactly 2 separators.

+  // the string based on the two separators.
+  std::string partition_id = base::StringPrintf("%s:%s:%s",
+                                                app_id.c_str(),
+                                                in_memory ? "in-memory" : "",
+                                                partition_name.c_str());
+
+  DCHECK(IsValidStoragePartitionId(browser_context,partition_id));
+  return partition_id;
 }
 
-std::string ChromeContentBrowserClient::GetStoragePartitionIdForSite(
+bool ChromeContentBrowserClient::IsValidStoragePartitionId(
     content::BrowserContext* browser_context,
-    const GURL& site) {
-  const Extension* extension = NULL;
+    const std::string& partition_id) {
+  // The default ID is empty and is always valid.
+  if (partition_id.empty())
+    return true;
+
   Profile* profile = Profile::FromBrowserContext(browser_context);
   ExtensionService* extension_service =
-      extensions::ExtensionSystem::Get(profile)->extension_service();
-  if (extension_service) {
-    extension = extension_service->extensions()->
-        GetExtensionOrAppByURL(ExtensionURLInfo(site));
+    extensions::ExtensionSystem::Get(profile)->extension_service();
+
+  // Now, parse the three parts of the partition ID, so we can verify them.
+  // Set the tokenizer to return delimiters, otherwise we won't get the correct
+  // count of delimiters and will fail validation.
+  int token = 0;
+  StringTokenizer t(partition_id, ":");
+  t.set_options(StringTokenizer::RETURN_DELIMS);
+
+  while (t.GetNext()) {
+    if (t.token_is_delim()) {
+      token++;
+      continue;
+    }
+    switch (token) {
+      // Starting off with the app id, verify it exists.
+      case 0:
+        if (!t.token().empty())  {
+          // No extension service means no storage partitions in Chrome.
+          if (!extension_service)
+            return false;
+          if (extension_service->GetExtensionById(t.token(), false) == NULL)
+            return false;
+        }
+        break;
+      // The second token is either empty or the "in-memory" string.
+      case 1:
+        if (!t.token().empty() && t.token() != "in-memory")
+          return false;
+        break;
+      // We don't verify the partition_name, as it is user supplied and there is
+      // no format constraints to it.
+      case 2:
+        break;
+      // We only expect three parts in the partition_id string, fail otherwise.
+      default:
+        NOTREACHED();
+        return false;
+    }
   }
 
-  return GetStoragePartitionIdForExtension(browser_context, extension);
+  // If there weren't three tokens, even if empty, then it is not a valid
+  // partition id.
+  return (token == 2);
 }
 
-bool ChromeContentBrowserClient::IsValidStoragePartitionId(
+void ChromeContentBrowserClient::GetStoragePartitionConfigForSite(
     content::BrowserContext* browser_context,
-    const std::string& partition_id) {
-  // The default ID is empty which is always allowed.
-  if (partition_id.empty())
-    return true;
+    const GURL& site,
+    std::string* app_id,
+    std::string* partition_name,
+    bool* in_memory) {
+  if (site.SchemeIs(chrome::kGuestScheme)) {

[Charlie Reis, 2012/11/05 18:15:37]

Please add some comments for each of these blocks, plus an example guest URL to
show what you're pulling out of it.

[nasko, 2012/11/05 19:20:26]

Done.

+    CHECK(site.has_host());
+    *app_id = site.host();
+    *partition_name = net::UnescapeURLComponent(site.query(),
+                                                net::UnescapeRule::NORMAL);
+    *in_memory = ((site.path() == "/persist") ? false : true);
+    return;
+  }
 
-  // If it isn't empty, then it must belong to an extension of some sort. Parse
-  // out the extension ID and make sure it is still installed.
+  const Extension* extension = NULL;
   Profile* profile = Profile::FromBrowserContext(browser_context);
   ExtensionService* extension_service =
       extensions::ExtensionSystem::Get(profile)->extension_service();
-  if (!extension_service) {
-    // No extension service means no storage partitions in Chrome.
-    return false;
+  if (extension_service) {
+    extension = extension_service->extensions()->
+        GetExtensionOrAppByURL(ExtensionURLInfo(site));
+    if (extension && extension->is_storage_isolated()) {
+      *app_id = extension->id();
+      *partition_name = std::string();
+      *in_memory = false;
+      return;
+    }
   }
 
-  // See if we can find an extension. The |partition_id| is the extension ID so
-  // no parsing needed to be done.
-  return extension_service->GetExtensionById(partition_id, false) != NULL;
+  *app_id = std::string();
+  *partition_name = std::string();
+  *in_memory = false;
 }
 
 content::WebContentsViewDelegate*
@@ -1608,10 +1674,16 @@ void ChromeContentBrowserClient::OverrideWebkitPrefs(
   chrome::ViewType view_type = chrome::GetViewType(web_contents);
   ExtensionService* service = profile->GetExtensionService();
   if (service) {
-    const Extension* extension = service->extensions()->GetByID(
-        rvh->GetSiteInstance()->GetSiteURL().host());
-    extension_webkit_preferences::SetPreferences(
-        extension, view_type, web_prefs);
+    const GURL& url = rvh->GetSiteInstance()->GetSiteURL();
+    const Extension* extension = service->extensions()->GetByID(url.host());
+    // Ensure that we are only granting extension preferences to URLs with
+    // the correct scheme. Without this check, guest:// schemes used by
+    // webview tags as well as hosts that happen to match the id of an
+    // installed extension would get the wrong preferences.
+    if (url.SchemeIs(chrome::kExtensionScheme)) {
+      extension_webkit_preferences::SetPreferences(
+          extension, view_type, web_prefs);
+    }
   }
 
   if (content::IsForceCompositingModeEnabled())
@@ -1868,23 +1940,4 @@ void ChromeContentBrowserClient::SetApplicationLocaleOnIOThread(
   io_thread_application_locale_ = locale;
 }
 
-std::string ChromeContentBrowserClient::GetStoragePartitionIdForExtension(
-    content::BrowserContext* browser_context, const Extension* extension) {
-  // In chrome, we use the extension ID as the partition ID. This works well
-  // because the extension ID fits the partition ID pattern and currently only
-  // apps can designate that storage should be isolated.
-  //
-  // If |extension| is NULL, then the default, empty string, partition id is
-  // used.
-  std::string partition_id;
-  if (extension && extension->is_storage_isolated()) {
-    partition_id = extension->id();
-  }
-
-  // Enforce that IsValidStoragePartitionId() implementation stays in sync.
-  DCHECK(IsValidStoragePartitionId(browser_context, partition_id));
-  return partition_id;
-}
-
-
 }  // namespace chrome