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..a744ecc80ab4571035ef62337e870ad230530a85 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;
+
+  // We need to get all the pieces that will go into the storage partition
+  // identifier first, before we compose it.
+  GetStoragePartitionDescriptionForSite(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

[awong, 2012/11/02 21:56:13]

In partition_id, we tag "in_memory" whereas in guest site, we tag "persistent". 
Do we want to unify? Does it matter?

[nasko, 2012/11/03 00:36:24]

There is one time bit flip at the time the embedder creates the URL where we
transition from the "persist" term, which is based on the attribute to the
"in_memory", which is used all through content/chrome.
I don't see it as a big priority to use in_memory throughout the two parts.

+  // 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
+  // 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, ":");

[awong, 2012/11/02 21:56:13]

What about just doing:
 vector<std::string> parts;
 StringSplit(partition_id, ":", &parts);
 if (parts.size() != 3) {
   NOTREACHED();
   return false;
 }

  if (!parts[0].empty()) {
    // Code to test for the extension name.
  }

  if (!parts[1].empty() && parts[1] != "in-memory") {
    ...
  }


That seems simpler.

BTW, are we certain that we've disallowed embedded :s in the partition_name?

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

Is there a reason to prefer the tokenizer?

[nasko, 2012/11/05 23:06:58]

Done.

+  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::GetStoragePartitionDescriptionForSite(
     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) {

[awong, 2012/11/02 21:56:13]

We need a highlevel comment that either explains what guests are versus
extensions and why in_memory is only sane for guest, or points to overview
documentation for browser tag that can let a reader explain this.

Otherwise, random engineer is going to be confused to death by this function and
will have no way to tell if the various assignment logic is.

This also makes me wonder, is there a unittest for this class?  Should we create
one?

[nasko, 2012/11/05 23:06:58]

Done.

+  if (site.SchemeIs(chrome::kGuestScheme)) {
+    CHECK(site.has_host());
+    *app_id = site.host();
+    *partition_name = net::UnescapeURLComponent(site.query(),
+                                                net::UnescapeRule::NORMAL);

[awong, 2012/11/02 21:56:13]

Add comment that tells where to look for the authoritative location that defines
the format of this site.

[nasko, 2012/11/05 23:06:58]

I've added a verbose comment above, which describes the format.

+    *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