Add ephemeral storage support for localStorage and sessionStorage #6560

mrobinson · 2020-09-01T15:45:15Z

Resolves brave/brave-browser#11619

Submitter Checklist:

Test Plan:

Open browser with clean profile, pointed to rewards prod.
Enable third-party cookies
Enable "Ephemeral Storage" (#brave-ephemeral-storage) in about:flags
Navigate to https://dev-pages.brave.software/storage/frames.html
Ensure that the test on that page works properly.

Reviewer Checklist:

New files have MPL-2.0 license header.
Request a security/privacy review as needed.
Adequate test coverage exists to prevent regressions
Verify test plan is specified in PR before merging to source

After-merge Checklist:

The associated issue milestone is set to the smallest version that the
changes has landed on.
All relevant documentation has been updated.

mrobinson · 2020-09-01T15:45:51Z

Apart from review, this is still blocked on the addition of a runtime flag for this feature and running it through the CI.

mrobinson · 2020-09-04T07:16:15Z

cc @pes10k

pes10k · 2020-09-04T17:32:17Z

Thanks @mrobinson ! Let me know once the flag is in place, and im happy to start using this as a daily build. Also also, feel free to let me know if you're at the stage where additional tests on https://dev-pages.brave.software/storage/index.html would be helpful

mrobinson · 2020-09-08T16:30:54Z

I believe this change is ready for review.

mrobinson · 2020-09-10T13:50:29Z

I have pulled out the addition of a runtime flag for all of ephemeral storage to #6615. This PR now depends on that one.

browser/net/brave_network_delegate_browsertest.cc

chromium_src/components/content_settings/core/common/cookie_settings_base.cc

components/ephemeral_storage/browser/ephemeral_storage_tab_helper.cc

chromium_src/components/content_settings/core/common/features.h

chromium_src/third_party/blink/renderer/modules/storage/dom_window_storage.cc

components/ephemeral_storage/browser/ephemeral_storage_browsertest.cc

patches/third_party-blink-renderer-modules-storage-storage_namespace.h.patch

components/ephemeral_storage/browser/ephemeral_storage_browsertest.cc

chromium_src/third_party/blink/renderer/modules/storage/dom_window_storage.cc

mrobinson · 2020-10-27T16:10:26Z

Okay. I've pushed a new commit that addresses review comments and also rebased the entire branch. I think there is really only one question outstanding and it is: #6560 (comment)

It's just waiting on @bridiver for feedback on what behavior was expected with that code. Thanks for the review comments and your patience with this change.

iefremov · 2020-10-28T13:49:00Z

browser/ephemeral_storage/ephemeral_storage_tab_helper.cc

@@ -145,7 +144,10 @@ bool EphemeralStorageTabHelper::IsAnotherTabOpenWithStorageDomain(
  }
 #else
  for (Browser* browser : *BrowserList::GetInstance()) {
-    if (browser->profile() == web_contents()->GetBrowserContext()) {
+    // We need to use reinterpret_cast here, because BrowserContext is a subclass of Profile,


hmmm, it seems to me that it is actually Profile subclasses BrowserContext, so we can just write BrowserContext* context = browser->profile() without a cast

I think the issue here was that including the profile.h header introduces a circular dependency. This code should be gone in the latest revision though.

iefremov · 2020-10-28T13:53:04Z

chromium_src/third_party/blink/renderer/modules/storage/dom_window_storage.cc

+  // localStorage area, we use the sessionStorage infrastructure.
+  Document* document = window->GetFrame()->GetDocument();
+  ephemeral_local_storage_ =
+      StorageArea::Create(document->GetFrame(), std::move(storage_area),


just window->GetFrame()

iefremov · 2020-10-28T13:55:22Z

chromium_src/third_party/blink/renderer/modules/storage/dom_window_storage.cc

+  if (!base::FeatureList::IsEnabled(features::kBraveEphemeralStorage))
+    return non_ephemeral_storage;


I'm thinking that maybe we can move these checks to the upper level? Or somehow change the function name, because now it is actually not ephemeralLocalStorage, but "ephemeral local storage if things are good, otherwise normal local storage"

iefremov · 2020-10-28T14:03:09Z

chromium_src/third_party/blink/renderer/modules/storage/dom_window_storage.cc

      String::FromUTF8(webview->Client()->GetSessionStorageNamespaceId()) +
-      String("/ephemeral-session-storage");
+      "/ephemeral-session-storage");


I'd really like to move this concatenation + /ephemeral-session-storage into StringToSessionStorageId()

iefremov · 2020-10-28T14:06:53Z

browser/ephemeral_storage/ephemeral_storage_tab_helper.cc

+// hash and then use that make up our own GUID-like string. Because of the way
+// we are constructing the string we should never collide with a real GUID and
+// we only need to worry about hash collisions, which are unlikely.
+std::string StringToSessionStorageId(const std::string& string) {


this function can append "/ephemeral-local-storage" to the string

One caller needs the string "/ephemeral-local-storage" and the other needs "/ephemeral-session-storage". What do you think about adding an additional parameter which is the suffix to be added to the string?

I've gone ahead and done this in both dom_window_storage.cc and ephemeral_storage_tab_helper.cc.

iefremov · 2020-10-28T15:03:11Z

browser/ephemeral_storage/ephemeral_storage_tab_helper.cc

+  } else {
+    std::string local_partition_id = new_domain + "/ephemeral-local-storage";
+    auto local_storage_namespace = content::CreateSessionStorageNamespace(
+        partition, StringToSessionStorageId(local_partition_id));


I'm probably missing something, but if CreateSessionStorageNamespace returns same namespace for a given partition (which maps 1-1 to a browser context) and a given id, then we don't need a global map? Since storage partition already maintains one

I believe that CreateSessionStorageNamespace returns a reference. If all the references disappear then the namespace is deleted. We need the map to maintain at least one reference, keeping the namespace alive.

I'm honestly having hard times understanding this, will definitely need some help :)
How does the map help us to keep a namespace alive if (1) it only holds weak pointers (2) last per_tld_ephemeral_storage_ will erase the corresponding entry in the map?
So anyway, closing (or navigating away) all tabs with a given TLD cleans up the namespace

The maps holds weak pointers, so the map alone won't keep the PerTLDEphemeralStorage (and the namespace) alive on its own. The TabHelper does keep a real reference to the PerTLDEphemeralStorage though. As long as one TabHelper has a reference, the PerTLDEphemeralStorage will stay alive. When the last TabHelper drops the reference (either by being destroyed or by switching domains during navigation), the PerTLDEphemeralStorage will be deleted by the reference counting code and the destructor removes it from the map of weak pointers.

iefremov · 2020-10-28T15:07:33Z

browser/ephemeral_storage/ephemeral_storage_browsertest.cc

+}
+
+IN_PROC_BROWSER_TEST_F(EphemeralStorageBrowserTest,
+                       ClosingTabClearsEphemeralStorage) {


maybe also check that reload doesn't drop the storage?

and also check that storage is different in another profile (should be relatively easy to spinoff an incognito window and do same checks)

(both tests can be a follow-up of course)

Okay. This all makes sense and I'm happy to add these tests in a quick followup, as you suggest.

mrobinson · 2020-10-28T16:09:04Z

Thanks for the comments. I've pushed a new commit to this branch with fixes.

mrobinson · 2020-10-28T19:25:26Z

I've pushed a new branch completely removing PerTLDEphemeralStorage. After chatting with @iefremov we agreed that it isn't completely necessary for this change and we can rely completely on the reference-counting of the storage namespaces.

iefremov · 2020-10-28T19:49:08Z

browser/ephemeral_storage/ephemeral_storage_tab_helper.cc

+                                     const std::string& suffix) {
+  std::string hash = base::MD5String(string) + "____";
+  DCHECK_EQ(hash.size(), 36u);
+  return hash + suffix;


hmm wait, I gather we should rather do md5(hash + suffix)

Oof. You are right. I will fix this.

iefremov · 2020-10-28T19:51:13Z

browser/ephemeral_storage/ephemeral_storage_tab_helper.h

+#include <utility>
+
+#include "base/memory/ref_counted.h"
+#include "base/memory/weak_ptr.h"


not used now

iefremov · 2020-10-28T19:52:30Z

chromium_src/third_party/blink/renderer/modules/storage/dom_window_storage.cc

+    return nullptr;
+  String session_storage_id = StringToSessionStorageId(
+      String::FromUTF8(webview->Client()->GetSessionStorageNamespaceId()),
+      "/ephemeral-session-storage");


bad spacing

Do you mean vertical spacing? This particular formatting is the result of running "git cl format."

sorry, everything's fine here

iefremov · 2020-10-28T20:09:24Z

So far found one minor (probably known) trouble:

load https://dev-pages.brave.software/storage/frames.html
restart the browser, the page should be restored
do some input via the form, see all 3 values are set

4a) reload
5a) the value in the foreign iframe disappeared.

4b) open the same url in another tab
5b) there is no value in the foreign iframe

iefremov · 2020-10-28T20:14:15Z

Also, would be super helpful to add some top-level comments for key classes (e.g. the TabHelper, EphemeralStorageNamespaces) saying what are they responsible for and how do we connect a namespace in the browser process with renderers.

mrobinson · 2020-10-28T20:16:11Z

So far found one minor (probably known) trouble:

It seems this is a regression from removing PerTLDEphemeralStorage.

iefremov

LGTM (only needs fix for #6560 (comment))

iefremov · 2020-10-28T20:25:00Z

It seems this is a regression from removing PerTLDEphemeralStorage.

nope, actually reproduced with PerTLDEphemeralStorage as well.
Edit: I think it could be related to not having a "previous" URL after session restore

Ephemeral storage persists DOM storage in third-party frames in a partitioned storage area that is unique to each top-level frame eTLD. The storage area is persisted until no tabs from the top-level eTLD exist. This change also adds a runtime flag for ephemeral storage, which is only useful if third-party storage is activated. A future change will selectively enable third-party storage when the flag is active.

mrobinson · 2020-11-02T16:54:00Z

It seems this is a regression from removing PerTLDEphemeralStorage.

nope, actually reproduced with PerTLDEphemeralStorage as well.
Edit: I think it could be related to not having a "previous" URL after session restore

The issue here was that there was a typo in the code generating the hash. I have fixed this now and all the CI are passing. I think this can be landed now?

iefremov · 2020-11-02T17:06:22Z

The issue here was that there was a typo in the code generating the hash. I have fixed this now and all the CI are passing. I think this can be landed now?

Sure thing, please go ahead!

mrobinson requested a review from bridiver September 1, 2020 15:45

mrobinson assigned mrobinson and cathiechen Sep 1, 2020

mrobinson force-pushed the ephemeral-dom-storage branch 4 times, most recently from 6af3bf6 to 8c73cfa Compare September 4, 2020 05:57

mrobinson marked this pull request as ready for review September 8, 2020 16:29

mrobinson requested a review from iefremov as a code owner September 8, 2020 16:29

mrobinson force-pushed the ephemeral-dom-storage branch 2 times, most recently from fb0592c to 22697c2 Compare September 10, 2020 13:49

mrobinson changed the title ~~Add Ephemeral storage support for localStorage and sessionStorage~~ Add ephemeral storage support for localStorage and sessionStorage Sep 10, 2020

mrobinson force-pushed the ephemeral-dom-storage branch from 22697c2 to 92c6af2 Compare September 10, 2020 16:10