Webview tag creation should be using storage partitions.

chrome/browser/extensions/web_view_browsertest.cc

Index: chrome/browser/extensions/web_view_browsertest.cc
diff --git a/chrome/browser/extensions/web_view_browsertest.cc b/chrome/browser/extensions/web_view_browsertest.cc
index 889af152c1fee6e95bf26e7fea1eeaa0c3d46639..4b69186017b6abfdbd5e13cfa24f88dbccc839e3 100644
--- a/chrome/browser/extensions/web_view_browsertest.cc
+++ b/chrome/browser/extensions/web_view_browsertest.cc
@@ -2,6 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "base/utf_string_conversions.h"
 #include "chrome/browser/automation/automation_util.h"
 #include "chrome/browser/extensions/platform_app_browsertest_util.h"
 #include "chrome/browser/ui/browser_tabstrip.h"
@@ -27,6 +28,102 @@ class WebViewTest : public extensions::PlatformAppBrowserTest {
 #endif
     ui::DisableTestCompositor();
   }
+
+  void NavigateAndOpenAppForIsolation(

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

FYI, if you do this kind of pattern in the future, remember that EXPECT_*
doesn't show a stack.

Thus if you had done something like:

TEST_F(Foo, Bar) {
  // Call the helper with EXPECT_* multiple times)
  NavigateAndOpenAppForIsolation();
   ...
  NavigateAndOpenAppForIsolation();
}

when the expect fails, you'd have no clue which invocation caused it.

In those cases, adding a SCOPED_TRACE() helps.

This particular instance is safe cause you only call this function once per
TEST_F, but I wanted to point it out.

[nasko, 2012/11/05 17:37:11]

Yes, you've pointed it out in the past. The main goal of this method is to do
initialization, so I don't forsee it being called multiple times.

+      GURL navigate_to_url,
+      content::WebContents** contents1,
+      content::WebContents** contents2,
+      content::WebContents** storage_contents1,
+      content::WebContents** storage_contents2) {

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

Function needs top level comment.

[nasko, 2012/11/05 17:37:11]

Done.

+    GURL::Replacements replace_host;
+    std::string host_str("localhost");  // Must stay in scope with replace_host.
+    replace_host.SetHostStr(host_str);
+
+    navigate_to_url = navigate_to_url.ReplaceComponents(replace_host);
+
+    GURL tag_url1 = test_server()->GetURL(
+        "files/extensions/platform_apps/web_view_isolation/cookie.html");
+    tag_url1 = tag_url1.ReplaceComponents(replace_host);
+    GURL tag_url2 = test_server()->GetURL(
+        "files/extensions/platform_apps/web_view_isolation/cookie2.html");
+    tag_url2 = tag_url2.ReplaceComponents(replace_host);
+    GURL tag_url3 = test_server()->GetURL(
+        "files/extensions/platform_apps/web_view_isolation/storage1.html");
+    tag_url3 = tag_url3.ReplaceComponents(replace_host);
+    GURL tag_url4 = test_server()->GetURL(
+        "files/extensions/platform_apps/web_view_isolation/storage2.html");
+    tag_url4 = tag_url4.ReplaceComponents(replace_host);
+
+    ui_test_utils::NavigateToURLWithDisposition(
+        browser(), navigate_to_url, CURRENT_TAB,
+        ui_test_utils::BROWSER_TEST_WAIT_FOR_NAVIGATION);
+
+    ui_test_utils::UrlLoadObserver observer1(
+        tag_url1, content::NotificationService::AllSources());
+    ui_test_utils::UrlLoadObserver observer2(
+        tag_url2, content::NotificationService::AllSources());
+    ui_test_utils::UrlLoadObserver observer3(
+        tag_url3, content::NotificationService::AllSources());
+    ui_test_utils::UrlLoadObserver observer4(
+        tag_url4, content::NotificationService::AllSources());
+    LoadAndLaunchPlatformApp("web_view_isolation");
+    observer1.Wait();
+    observer2.Wait();
+    observer3.Wait();
+    observer4.Wait();
+
+    content::Source<content::NavigationController> source1 = observer1.source();
+    EXPECT_TRUE(source1->GetWebContents()->GetRenderProcessHost()->IsGuest());
+    content::Source<content::NavigationController> source2 = observer2.source();
+    EXPECT_TRUE(source2->GetWebContents()->GetRenderProcessHost()->IsGuest());
+    content::Source<content::NavigationController> source3 = observer3.source();
+    EXPECT_TRUE(source3->GetWebContents()->GetRenderProcessHost()->IsGuest());
+    content::Source<content::NavigationController> source4 = observer4.source();
+    EXPECT_TRUE(source4->GetWebContents()->GetRenderProcessHost()->IsGuest());
+
+    // Tags with the same storage partition are not yet combined in the same
+    // process. Check this until http://crbug.com/138296 is fixed.
+    EXPECT_NE(source1->GetWebContents()->GetRenderProcessHost()->GetID(),
+              source2->GetWebContents()->GetRenderProcessHost()->GetID());
+
+    // Check that the storage partitions of the first two tags match and are
+    // different than the other two.
+    EXPECT_EQ(
+        source1->GetWebContents()->GetRenderProcessHost()->
+            GetStoragePartition(),
+        source2->GetWebContents()->GetRenderProcessHost()->
+            GetStoragePartition());
+    EXPECT_EQ(
+        source3->GetWebContents()->GetRenderProcessHost()->
+            GetStoragePartition(),
+        source4->GetWebContents()->GetRenderProcessHost()->
+            GetStoragePartition());
+    EXPECT_NE(
+        source1->GetWebContents()->GetRenderProcessHost()->
+            GetStoragePartition(),
+        source3->GetWebContents()->GetRenderProcessHost()->
+            GetStoragePartition());
+

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

one newline is enough

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

Done.

+
+    *contents1 = source1->GetWebContents();
+    *contents2 = source2->GetWebContents();
+    *storage_contents1 = source3->GetWebContents();
+    *storage_contents2 = source4->GetWebContents();
+  }
+
+  void ExecuteScriptWaitForTitle(content::WebContents* web_contents,
+                                 const char* script,
+                                 const char* title) {
+    std::wstring js_script(L"window.domAutomationController.send(" +
+                           ASCIIToWide(script) + L")");
+    string16 expected_title(ASCIIToUTF16(title));
+    string16 error_title(ASCIIToUTF16("error"));
+    content::TitleWatcher title_watcher(web_contents, expected_title);
+    title_watcher.AlsoWaitForTitle(error_title);
+    EXPECT_TRUE(content::ExecuteJavaScript(web_contents->GetRenderViewHost(),

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

IIRC, ExecuteJavaScript() doesn't need the domAutomationContoller.send(). 
That's only needed if you call the functions that read a value.  Try it and see
if the test hangs or not?

[nasko, 2012/11/05 17:37:11]

Done.

+                                           std::wstring(), js_script));
+    EXPECT_EQ(expected_title, title_watcher.WaitAndGetTitle());
+  }
 };
 
 IN_PROC_BROWSER_TEST_F(WebViewTest, Shim) {
@@ -38,7 +135,7 @@ IN_PROC_BROWSER_TEST_F(WebViewTest, ShimSrcAttribute) {
       << message_;
 }
 
-IN_PROC_BROWSER_TEST_F(WebViewTest, Isolation) {
+IN_PROC_BROWSER_TEST_F(WebViewTest, CookieIsolation) {
   ASSERT_TRUE(StartTestServer());
   const std::wstring kExpire =
       L"var expire = new Date(Date.now() + 24 * 60 * 60 * 1000);";
@@ -56,48 +153,24 @@ IN_PROC_BROWSER_TEST_F(WebViewTest, Isolation) {
   GURL set_cookie_url = test_server()->GetURL(
       "files/extensions/platform_apps/isolation/set_cookie.html");
   set_cookie_url = set_cookie_url.ReplaceComponents(replace_host);
-  GURL tag_url1 = test_server()->GetURL(
-      "files/extensions/platform_apps/web_view_isolation/cookie.html");
-  tag_url1 = tag_url1.ReplaceComponents(replace_host);
-  GURL tag_url2 = test_server()->GetURL(
-      "files/extensions/platform_apps/web_view_isolation/cookie2.html");
-  tag_url2 = tag_url2.ReplaceComponents(replace_host);
-
-  // Load a (non-app) page under the "localhost" origin that sets a cookie.
-  ui_test_utils::NavigateToURLWithDisposition(
-      browser(), set_cookie_url,
-      CURRENT_TAB, ui_test_utils::BROWSER_TEST_WAIT_FOR_NAVIGATION);
-  // Make sure the cookie is set.
-  int cookie_size;
-  std::string cookie_value;
-  automation_util::GetCookies(set_cookie_url,
-                              chrome::GetWebContentsAt(browser(), 0),
-                              &cookie_size, &cookie_value);
-  EXPECT_EQ("testCookie=1", cookie_value);
 
-  ui_test_utils::UrlLoadObserver observer1(
-      tag_url1, content::NotificationService::AllSources());
-  ui_test_utils::UrlLoadObserver observer2(
-      tag_url2, content::NotificationService::AllSources());
-  LoadAndLaunchPlatformApp("web_view_isolation");
-  observer1.Wait();
-  observer2.Wait();
-
-  content::Source<content::NavigationController> source1 = observer1.source();
-  EXPECT_TRUE(source1->GetWebContents()->GetRenderProcessHost()->IsGuest());
-  content::Source<content::NavigationController> source2 = observer2.source();
-  EXPECT_TRUE(source2->GetWebContents()->GetRenderProcessHost()->IsGuest());
-  EXPECT_NE(source1->GetWebContents()->GetRenderProcessHost()->GetID(),
-            source2->GetWebContents()->GetRenderProcessHost()->GetID());
+  content::WebContents* contents1;
+  content::WebContents* contents2;
+  content::WebContents* contents3;
+  content::WebContents* contents4;
+
+  NavigateAndOpenAppForIsolation(set_cookie_url, &contents1, &contents2,
+                                 &contents3, &contents4);
 
   EXPECT_TRUE(content::ExecuteJavaScript(
-      source1->GetWebContents()->GetRenderViewHost(), std::wstring(),
-      cookie_script1));
+      contents1->GetRenderViewHost(), std::wstring(), cookie_script1));
   EXPECT_TRUE(content::ExecuteJavaScript(
-      source2->GetWebContents()->GetRenderViewHost(), std::wstring(),
-      cookie_script2));
+      contents2->GetRenderViewHost(), std::wstring(), cookie_script2));
+
+  int cookie_size;
+  std::string cookie_value;
 
-  // Test the regular browser context to ensure we still have only one cookie.
+  // Test the regular browser context to ensure we have only one cookie.
   automation_util::GetCookies(GURL("http://localhost"),
                               chrome::GetWebContentsAt(browser(), 0),
                               &cookie_size, &cookie_value);
@@ -108,12 +181,184 @@ IN_PROC_BROWSER_TEST_F(WebViewTest, Isolation) {
   // ensure we have properly set the cookies and we have both cookies in both
   // tags.
   automation_util::GetCookies(GURL("http://localhost"),
-                              source1->GetWebContents(),
+                              contents1,
                               &cookie_size, &cookie_value);
   EXPECT_EQ("guest1=true; guest2=true", cookie_value);
 
   automation_util::GetCookies(GURL("http://localhost"),
-                              source2->GetWebContents(),
+                              contents2,
                               &cookie_size, &cookie_value);
   EXPECT_EQ("guest1=true; guest2=true", cookie_value);
+
+  // The third tag should not have any cookies, as it is in separate partition.

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

s/,//

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

Done.

+  automation_util::GetCookies(GURL("http://localhost"),
+                              contents3,
+                              &cookie_size, &cookie_value);
+  EXPECT_EQ("", cookie_value);
+}
+
+IN_PROC_BROWSER_TEST_F(WebViewTest, DOMStorageIsolation) {
+  ASSERT_TRUE(StartTestServer());
+  GURL regular_url = test_server()->GetURL("files/title1.html");
+
+  std::string output;
+  std::wstring get_local_storage(L"window.domAutomationController.send("
+      L"window.localStorage.getItem('foo') || 'badval')");
+  std::wstring get_session_storage(L"window.domAutomationController.send("
+      L"window.sessionStorage.getItem('foo') || 'badval')");
+
+  content::WebContents* contents1;
+  content::WebContents* contents2;
+  content::WebContents* storage_contents1;
+  content::WebContents* storage_contents2;
+
+  NavigateAndOpenAppForIsolation(regular_url, &contents1, &contents2,
+                                 &storage_contents1, &storage_contents2);
+
+  // Initialize the storage for the first of the two tags that share a storage
+  // partition.
+  EXPECT_TRUE(content::ExecuteJavaScript(

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

Same comment about domAutomationController().

Someone really needs to go fix content::ExecuteJSAndExtractXXX to do that
wrapping for you.

[nasko, 2012/11/05 17:37:11]

This one doesn't use domAutomationController, does it? It passes a literal
string "initDomStorage('page1')".

[awong, 2012/11/05 18:00:33]

Oh right...I misread.

+      storage_contents1->GetRenderViewHost(), std::wstring(),
+      L"initDomStorage('page1')"));
+
+  // Let's test the expected values are present.

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

Useless comment. Describe why not what or don't describe at all. :)

[nasko, 2012/11/05 17:37:11]

Done.

+  EXPECT_TRUE(ExecuteJavaScriptAndExtractString(
+      storage_contents1->GetRenderViewHost(), std::wstring(),
+      get_local_storage.c_str(), &output));
+  EXPECT_STREQ("local-page1", output.c_str());
+
+  EXPECT_TRUE(ExecuteJavaScriptAndExtractString(
+      storage_contents1->GetRenderViewHost(), std::wstring(),
+      get_session_storage.c_str(), &output));
+  EXPECT_STREQ("session-page1", output.c_str());
+

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

This vertical spacing makes it really hard for me to see the various test
sections.  Maybe this is a case where 2 newlines helps.  Or possible use a
little ascii art in the comment?  Or maybe remove the newline bewteen the
related chunks above?

[nasko, 2012/11/05 17:37:11]

Done.

+  // Now, init the storage in the second tag in the same storage partition.
+  EXPECT_TRUE(content::ExecuteJavaScript(
+      storage_contents2->GetRenderViewHost(), std::wstring(),
+      L"initDomStorage('page2')"));
+
+  // The values now should reflect the second tag.
+  EXPECT_TRUE(ExecuteJavaScriptAndExtractString(
+      storage_contents1->GetRenderViewHost(), std::wstring(),
+      get_local_storage.c_str(), &output));
+  EXPECT_STREQ("local-page2", output.c_str());
+
+  EXPECT_TRUE(ExecuteJavaScriptAndExtractString(
+      storage_contents2->GetRenderViewHost(), std::wstring(),
+      get_local_storage.c_str(), &output));
+  EXPECT_STREQ("local-page2", output.c_str());
+
+  // Session storage is not shared though, as each webview tag has separate
+  // instance, even if they are in the same storage partition.
+  EXPECT_TRUE(ExecuteJavaScriptAndExtractString(
+      storage_contents1->GetRenderViewHost(), std::wstring(),
+      get_session_storage.c_str(), &output));
+  EXPECT_STREQ("session-page1", output.c_str());
+
+  EXPECT_TRUE(ExecuteJavaScriptAndExtractString(
+      storage_contents2->GetRenderViewHost(), std::wstring(),
+      get_session_storage.c_str(), &output));
+  EXPECT_STREQ("session-page2", output.c_str());
+
+  // Also, let's check that the main browser and another tag that doesn't share
+  // the same partition don't have those values stored.
+  EXPECT_TRUE(ExecuteJavaScriptAndExtractString(
+      chrome::GetWebContentsAt(browser(), 0)->GetRenderViewHost(),
+      std::wstring(), get_local_storage.c_str(), &output));
+  EXPECT_STREQ("badval", output.c_str());
+  EXPECT_TRUE(ExecuteJavaScriptAndExtractString(
+      chrome::GetWebContentsAt(browser(), 0)->GetRenderViewHost(),
+      std::wstring(), get_session_storage.c_str(), &output));
+  EXPECT_STREQ("badval", output.c_str());

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

This particular test is really hard to undersatnd for some reason. It might be
partly spacing related, but I don't know what the basic structure (which tabs
are testing what?) is and what dimensions you are playing with.  No concrete
suggestion on how to solve though...

[nasko, 2012/11/05 17:37:11]

Tried to bundle things together with comments a bit improved. Hopefully that
helps.

+
+  EXPECT_TRUE(ExecuteJavaScriptAndExtractString(
+      contents1->GetRenderViewHost(), std::wstring(),
+      get_local_storage.c_str(), &output));
+  EXPECT_STREQ("badval", output.c_str());
+  EXPECT_TRUE(ExecuteJavaScriptAndExtractString(
+      contents1->GetRenderViewHost(), std::wstring(),
+      get_session_storage.c_str(), &output));
+  EXPECT_STREQ("badval", output.c_str());
+}
+
+IN_PROC_BROWSER_TEST_F(WebViewTest, IndexedDBIsolation) {
+  ASSERT_TRUE(StartTestServer());
+  GURL regular_url = test_server()->GetURL("files/title1.html");
+
+  content::WebContents* contents1;
+  content::WebContents* contents2;
+  content::WebContents* storage_contents1;
+  content::WebContents* storage_contents2;
+
+  NavigateAndOpenAppForIsolation(regular_url, &contents1, &contents2,
+                                 &storage_contents1, &storage_contents2);
+
+  // Initialize the storage for the first of the two tags that share a storage
+  // partition.
+  ExecuteScriptWaitForTitle(storage_contents1, "initIDB()", "idb created");
+  ExecuteScriptWaitForTitle(storage_contents1, "addItemIDB(7, 'page1')",
+                            "addItemIDB complete");
+  ExecuteScriptWaitForTitle(storage_contents1, "readItemIDB(7)",
+                            "readItemIDB complete");
+
+  std::string output;
+  std::wstring get_value(
+      L"window.domAutomationController.send(getValueIDB() || 'badval')");
+
+  EXPECT_TRUE(ExecuteJavaScriptAndExtractString(
+      storage_contents1->GetRenderViewHost(), std::wstring(),
+      get_value.c_str(), &output));
+  EXPECT_STREQ("page1", output.c_str());
+
+  // Initialize the db in the second tag.
+  ExecuteScriptWaitForTitle(storage_contents2, "initIDB()", "idb open");
+
+  // Since we share a partition, reading the value should return the existing
+  // one.
+  ExecuteScriptWaitForTitle(storage_contents2, "readItemIDB(7)",
+                            "readItemIDB complete");
+  EXPECT_TRUE(ExecuteJavaScriptAndExtractString(
+      storage_contents2->GetRenderViewHost(), std::wstring(),
+      get_value.c_str(), &output));
+  EXPECT_STREQ("page1", output.c_str());
+
+  // Now write through the second tag and read it back.
+  ExecuteScriptWaitForTitle(storage_contents2, "addItemIDB(7, 'page2')",
+                            "addItemIDB complete");
+  ExecuteScriptWaitForTitle(storage_contents2, "readItemIDB(7)",
+                            "readItemIDB complete");
+  EXPECT_TRUE(ExecuteJavaScriptAndExtractString(
+      storage_contents2->GetRenderViewHost(), std::wstring(),
+      get_value.c_str(), &output));
+  EXPECT_STREQ("page2", output.c_str());
+
+  // Reset the document title, otherwise the next call will not see a change and
+  // will hang waiting for it.
+  EXPECT_TRUE(content::ExecuteJavaScript(
+      storage_contents1->GetRenderViewHost(), std::wstring(),
+      L"document.title = 'foo'"));
+
+  // Read through the first tag to ensure we have the second value.
+  ExecuteScriptWaitForTitle(storage_contents1, "readItemIDB(7)",
+                            "readItemIDB complete");
+  EXPECT_TRUE(ExecuteJavaScriptAndExtractString(
+      storage_contents1->GetRenderViewHost(), std::wstring(),
+      get_value.c_str(), &output));
+  EXPECT_STREQ("page2", output.c_str());
+
+  // Now, let's confirm there is no database in the main browser and another
+  // tag that doesn't share the same partition. Due to the IndexedDB API design,
+  // open will succeed, but the version will be 1, since it creates the database
+  // if it is not found. The two tags use database version 3, so we avoid
+  // ambiguity.
+  const char* script =
+    "indexedDB.open('isolation').onsuccess = function(e) {"
+    "  if (e.target.result.version == 1)"
+    "    document.title = 'db not found';"
+    "  else "
+    "    document.title = 'error';"
+    "}";
+  ExecuteScriptWaitForTitle(chrome::GetWebContentsAt(browser(), 0),
+                            script, "db not found");
+  ExecuteScriptWaitForTitle(contents1, script, "db not found");
 }