enable ephemeral storage keep alive by default #8526

bridiver · 2021-04-13T19:28:54Z

Resolves brave/brave-browser#15415

Submitter Checklist:

I confirm that no security/privacy review is needed, or that I have requested one
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

bridiver · 2021-04-21T02:22:31Z

browser/ephemeral_storage/ephemeral_storage_tab_helper.cc

@@ -85,6 +92,33 @@ void EphemeralStorageTabHelper::ReadyToCommitNavigation(
  CreateEphemeralStorageAreasForDomainAndURL(new_domain, new_url);
 }

+void EphemeralStorageTabHelper::ClearEphemeralLifetimeKeepalive(
+    const content::TLDEphemeralLifetimeKey& key) {
+  ClearLocalStorageKeepAlive(


this would be simpler if TLDEphemeralLifetime owned local_storage_namespace_, but I'd rather do that refactoring in a follow-up

goodov · 2021-04-21T08:58:56Z

browser/ephemeral_storage/ephemeral_storage_tab_helper.cc

@@ -33,7 +33,8 @@ namespace {
 constexpr char kSessionStorageSuffix[] = "/ephemeral-session-storage";
 constexpr char kLocalStorageSuffix[] = "/ephemeral-local-storage";

-const base::TimeDelta kStorageKeepAliveDelay = base::TimeDelta::FromSeconds(30);
+const base::TimeDelta kStorageKeepAliveDelay = base::TimeDelta::FromSeconds(


this way kStorageKeepAliveDelay cannot be controlled via Griffin or --enable-features, because the value is set before FieldTrials initialization.

ah, right, I need to lazy initialize it

actually I don't really need this const at all

goodov · 2021-04-21T10:15:19Z

browser/ephemeral_storage/ephemeral_storage_browsertest.cc

@@ -100,6 +105,10 @@ class EphemeralStorageBrowserTest : public InProcessBrowserTest {
        https_server_.GetURL("b.com", "/ephemeral_storage.html");
    c_site_ephemeral_storage_url_ =
        https_server_.GetURL("c.com", "/ephemeral_storage.html");
+
+    ephemeral_storage::EphemeralStorageTabHelper::
+        SetKeepAliveTimeDelayForTesting(


why not ScopedFeatureList::InitAndEnableFeatureWithParameters?

is that still ok to use when the feature is enabled by default? If so I'll remove SetKeepAliveTimeDelayForTesting

I guess it must be ok because it still has InitAndEnable for kBraveEphemeralStorage

actually this seems to complicate running in both the enabled and disabled state so I'm going to leave it as-is

what about creating ScopedEphemeralStorageKeepAliveTimeDelayForTesting? it can be used as a member in test classes:

class Test : ... { ... private: // Maybe the delay can be set to 2, but can be overriden so both variants can work: ScopedEphemeralStorageKeepAliveTimeDelayForTesting scoped_ephemeral_storage_keep_alive_; ScopedEphemeralStorageKeepAliveTimeDelayForTesting scoped_ephemeral_storage_keep_alive_manual_{2}; };

Bonus: it can reset the value to the default state at test shutdown.

Also you can create a utility function for tests which will run a RunLoop with PostDelayedTask using the time delay set by the scoped object. The function can be a member of the scoped object.

that would still require a method on EphemeralStorageTabHelper to set the actual delay so not sure that's worth the extra effort since we always want the short delay in testing

going with this instead #8526 (comment)

I'm going to look at this in a follow-up so I can get this merged

goodov · 2021-04-21T10:28:46Z

browser/ephemeral_storage/ephemeral_storage_browsertest.cc

+  // after keepalive values should be cleared
+  ui_test_utils::NavigateToURL(browser(), b_site_ephemeral_storage_url_);
+  ASSERT_TRUE(WaitForLoadStop(web_contents));
+  base::PlatformThread::Sleep(base::TimeDelta::FromSeconds(kKeepAliveInterval));


this way you hang the whole process. consider this instead:

base::RunLoop run_loop; base::SequencedTaskRunnerHandle::Get()->PostDelayedTask( FROM_HERE, run_loop.QuitClosure(), base::TimeDelta::FromSeconds(kKeepAliveInterval)); run_loop.Run();

good call, I was digging around trying to remember how to wait and saw other tests using base::PlatformThread::Sleep so I thought I had misremembered that there was a better way :)

but why not ScopedMockTimeMessageLoopTaskRunner ?

I wasn't aware of its existence, but looks like a good option because it doesn't require waiting at all and also eliminates a potential race condition if the tests are very slow

mkarolin

chromium_src ++

goodov · 2021-04-22T07:35:13Z

browser/permissions/permission_lifetime_manager_browsertest.cc

@@ -273,8 +277,9 @@ class PermissionLifetimeManagerWithOriginMonitorBrowserTest
    : public PermissionLifetimeManagerBrowserTest {
 public:
  PermissionLifetimeManagerWithOriginMonitorBrowserTest() {
-    scoped_feature_list_.InitAndEnableFeature(


scoped_feature_list_ in this class can be removed.

goodov · 2021-04-22T07:36:18Z

chromium_src/content/public/browser/tld_ephemeral_lifetime.h

@@ -53,6 +53,8 @@ class CONTENT_EXPORT TLDEphemeralLifetime
  // Add a callback to a callback list to be called on destruction.
  void RegisterOnDestroyCallback(OnDestroyCallback callback);

+  const TLDEphemeralLifetimeKey& key() { return key_; }


goodov · 2021-04-22T07:49:34Z

browser/ephemeral_storage/ephemeral_storage_browsertest.cc

@@ -100,6 +105,10 @@ class EphemeralStorageBrowserTest : public InProcessBrowserTest {
        https_server_.GetURL("b.com", "/ephemeral_storage.html");
    c_site_ephemeral_storage_url_ =
        https_server_.GetURL("c.com", "/ephemeral_storage.html");
+
+    ephemeral_storage::EphemeralStorageTabHelper::
+        SetKeepAliveTimeDelayForTesting(


what about creating ScopedEphemeralStorageKeepAliveTimeDelayForTesting? it can be used as a member in test classes:

class Test : ... { ... private: // Maybe the delay can be set to 2, but can be overriden so both variants can work: ScopedEphemeralStorageKeepAliveTimeDelayForTesting scoped_ephemeral_storage_keep_alive_; ScopedEphemeralStorageKeepAliveTimeDelayForTesting scoped_ephemeral_storage_keep_alive_manual_{2}; };

Bonus: it can reset the value to the default state at test shutdown.

Also you can create a utility function for tests which will run a RunLoop with PostDelayedTask using the time delay set by the scoped object. The function can be a member of the scoped object.

browser/ephemeral_storage/ephemeral_storage_browsertest.cc

iefremov · 2021-04-22T10:34:51Z

would be nice to have a problem description somewhere just for better understanding goals of the change

iefremov · 2021-04-22T10:53:32Z

browser/ephemeral_storage/ephemeral_storage_browsertest.cc


  // Navigate away and then navigate back to the original site.
  ui_test_utils::NavigateToURL(browser(), b_site_ephemeral_storage_url_);
+  ASSERT_TRUE(WaitForLoadStop(web_contents));


you don't need these, ui_test_utils::NavigateToURL blocks until loading stops on its own

yea, I added this because I was seeing strange behavior and then forgot to remove it

iefremov · 2021-04-22T11:30:41Z

browser/ephemeral_storage/ephemeral_storage_tab_helper.cc

+  ClearLocalStorageKeepAlive(
+      StringToSessionStorageId(key.second, kLocalStorageSuffix));
+
+  for (auto it = keep_alive_tld_ephemeral_lifetime_list_.begin();


I'd rather do base::ranges::find() and then erase: this is 2 times shorter and also safer, because the current loop looks as a common pitfall (without immediate return). If return is occasionally refactored/removed, we could be in trouble

pes10k · 2021-04-22T16:05:07Z

@iefremov this issue has a fuller description of why the "keep alive" functionality is needed. brave/brave-browser#14943

… the tab is closed

bridiver · 2021-04-22T22:23:25Z

test failures are unrelated known crashes for rewards

iefremov · 2021-04-23T08:23:56Z

browser/ephemeral_storage/ephemeral_storage_tab_helper.cc

+
+void EphemeralStorageTabHelper::ClearLocalStorageKeepAlive(
+    const std::string& id) {
+  auto it = base::ranges::find_if(


in theory this is

base::ranges::find(method_data, id, &content::SessionStorageNamespace::id)

but not sure how well it works with scoped_refptrs

enable ephemeral storage keep alive by default

bridiver force-pushed the cf-crash-2 branch from 7b83ddd to cdeb399 Compare April 19, 2021 18:23

bridiver marked this pull request as ready for review April 21, 2021 01:35

bridiver requested a review from a team as a code owner April 21, 2021 01:35

bridiver requested review from goodov, pes10k and iefremov April 21, 2021 01:35

bridiver self-assigned this Apr 21, 2021

bridiver changed the title ~~clear keepalive references on tab close~~ enable ephemeral storage keep alive by default Apr 21, 2021

bridiver commented Apr 21, 2021

View reviewed changes

goodov reviewed Apr 21, 2021

View reviewed changes

mkarolin approved these changes Apr 21, 2021

View reviewed changes

bridiver force-pushed the cf-crash-2 branch 4 times, most recently from b5c2e09 to 5f7459e Compare April 21, 2021 20:10

goodov reviewed Apr 22, 2021

View reviewed changes

iefremov reviewed Apr 22, 2021

View reviewed changes

iefremov approved these changes Apr 22, 2021

View reviewed changes

goodov approved these changes Apr 22, 2021

View reviewed changes

bridiver added 8 commits April 22, 2021 15:22

make sure all keepalive ephemeral storage references are cleared when…

156dcb8

… the tab is closed

make the keep alive time configurable

c33a50c

keep local storage alive

0d4b75d

update session storage tests

53adebc

lint

85c1500

don't block the whole process waiting

f1b8ad4

correct test name

e58d8f9

don't initialize the keep alive timeout before field trials initialize

eb89166

bridiver added 7 commits April 22, 2021 15:22

ephemeral storage is enabled by default now

87c4a2b

use keepalive for permissions since they are tied to ephemeral storage

b6e845a

ScopedFeatureList is no longer needed in base class

3dbe05a

remove unnecessary WaitForLoadStop

e3c2b1f

use base::ranges instead of for

316a3e2

make key() const

8801102

lint

508c5b8

bridiver force-pushed the cf-crash-2 branch from b676a6f to 508c5b8 Compare April 22, 2021 22:23

bridiver merged commit e446573 into master Apr 22, 2021

bridiver deleted the cf-crash-2 branch April 22, 2021 22:23

bridiver added this to the 1.25.x - Nightly milestone Apr 22, 2021

iefremov reviewed Apr 23, 2021

View reviewed changes

bridiver added a commit that referenced this pull request May 20, 2021

Merge pull request #8526 from brave/cf-crash-2

53851c3

enable ephemeral storage keep alive by default

bridiver mentioned this pull request May 20, 2021

Do not show ipfs items if the ipfs daemon is not launched (uplift to 1.26.x) #8884

Merged

7 tasks

kylehickinson pushed a commit that referenced this pull request Jan 4, 2024

Fix #8526: Fix the issue of the share button disappearing (#8572)

d2f2338

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

enable ephemeral storage keep alive by default #8526

enable ephemeral storage keep alive by default #8526

bridiver commented Apr 13, 2021 •

edited

Loading

bridiver Apr 21, 2021

goodov Apr 21, 2021

bridiver Apr 21, 2021

bridiver Apr 21, 2021

goodov Apr 21, 2021

bridiver Apr 21, 2021

bridiver Apr 21, 2021

bridiver Apr 21, 2021

goodov Apr 22, 2021

bridiver Apr 22, 2021

bridiver Apr 22, 2021

bridiver Apr 22, 2021

goodov Apr 21, 2021

bridiver Apr 21, 2021

iefremov Apr 22, 2021

bridiver Apr 22, 2021

mkarolin left a comment

goodov Apr 22, 2021

goodov Apr 22, 2021

goodov Apr 22, 2021

iefremov commented Apr 22, 2021

iefremov Apr 22, 2021

bridiver Apr 22, 2021

iefremov Apr 22, 2021

pes10k commented Apr 22, 2021

bridiver commented Apr 22, 2021

iefremov Apr 23, 2021

enable ephemeral storage keep alive by default #8526

enable ephemeral storage keep alive by default #8526

Conversation

bridiver commented Apr 13, 2021 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkarolin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iefremov commented Apr 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pes10k commented Apr 22, 2021

bridiver commented Apr 22, 2021

Choose a reason for hiding this comment

bridiver commented Apr 13, 2021 •

edited

Loading