-
Notifications
You must be signed in to change notification settings - Fork 893
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Don't clear first party storage area if browser is launched in incognito mode from command line #24543
Don't clear first party storage area if browser is launched in incognito mode from command line #24543
Conversation
Thank you! Having a browser test for this would be nice. Can you try to add one please? You can use this test as a starting point, you might need to use brave-core/browser/ephemeral_storage/ephemeral_storage_forget_by_default_browsertest.cc Lines 178 to 215 in b4332ca
|
@@ -327,6 +327,13 @@ void EphemeralStorageService::ScheduleFirstPartyStorageAreasCleanupOnStartup() { | |||
|
|||
void EphemeralStorageService::CleanupFirstPartyStorageAreasOnStartup() { | |||
DCHECK(!context_->IsOffTheRecord()); | |||
|
|||
Profile* profile = Profile::FromBrowserContext(context_); | |||
if (profile->HasAnyOffTheRecordProfile()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe the better approach here is to detect if any "normal" profile windows are opened or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion, I will try to follow it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Please have a look
Yes, I will try to write a browsing test. Thanks! |
@goodov Can you help me a bit with the testing? It looks like reproducing this scenario in the test is a bit hard. |
@@ -327,6 +339,11 @@ void EphemeralStorageService::ScheduleFirstPartyStorageAreasCleanupOnStartup() { | |||
|
|||
void EphemeralStorageService::CleanupFirstPartyStorageAreasOnStartup() { | |||
DCHECK(!context_->IsOffTheRecord()); | |||
Profile* profile = Profile::FromBrowserContext(context_); | |||
if (!DoesProfileHaveAnyBrowserWindow(profile)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually, now I'm thinking: shouldn't we instead start the cleanup process timer when a regular profile window is created (or exists) for context_
?
right now if you open a normal window with an existing incognito window, the cleanup won't run.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I saw in the code, there are two situations that trigger the cleanup.
- At startup, there is a 5-second keep-alive timer after which the cleanup is executed(so if you are quick enough to enter the URL in the address bar, the cleanup is aborted)
- After closing a tab, there is a 30-second grace period (this option is enabled by default) after which all data cleanup is started.
Are you running in one of these situations? Can you please add some steps to reproduce? Thanks!
You need to create a separate test fixture with something like:
in non-PRE test you need to call |
@tech-zilla
you need to move the added function into make sure to rebase before doing that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very good job! thank you! Let's fix few things and I trigger CI.
return; | ||
} | ||
} | ||
is_window_visible = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just return a boolean from the function, I don't see why you need an optional returned as a mutable parameter here.
if you need to have a default implementation for the virtual function in .cc
in this case (clang checks may ask for it), just add a .cc
file and implement it, no big deal.
@@ -4,12 +4,12 @@ | |||
* You can obtain one at https://mozilla.org/MPL/2.0/. */ | |||
|
|||
#include "brave/browser/ephemeral_storage/ephemeral_storage_browsertest.h" | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please run npm run presubmit --fix
locally before pushing things. I think the format checks won't like the removal of this line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} | ||
} | ||
|
||
static bool IsPreTest() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I think this should be called IsPreTestToEnableIncognito
, because you have 2 PRE_
tests, but you're actually interested in one of them.
thanks for the support during the code review! |
|
@goodov Completely forgot about this PR. I will have a look next week |
…ch (brave#26531) * Add Smart-Proxy Routing Toggle * Add VPN Kill-Switch Toggle * Upgrade to GuardianConnect 2.0.1
This introduces a closer degree of parity with other browsers by supporting the de-facto standard of using Ctrl+Shift+S to create a screenshot. Resolves brave/brave-browser#42682
Create & use a frozen copy of window.origin.
fix brave/brave-browser#42693 When sidebar menu is handled, it should check previous menu state. If prev entry is separator, it should be removed before sidebar menu because sidebar menu will add it's own top & bottom separators.
Bumps [path-to-regexp](https://github.com/pillarjs/path-to-regexp) and [express](https://github.com/expressjs/express). These dependencies needed to be updated together. Updates `path-to-regexp` from 0.1.10 to 0.1.12 - [Release notes](https://github.com/pillarjs/path-to-regexp/releases) - [Changelog](https://github.com/pillarjs/path-to-regexp/blob/master/History.md) - [Commits](pillarjs/path-to-regexp@v0.1.10...v0.1.12) Updates `express` from 4.21.1 to 4.21.2 - [Release notes](https://github.com/expressjs/express/releases) - [Changelog](https://github.com/expressjs/express/blob/4.21.2/History.md) - [Commits](expressjs/express@4.21.1...4.21.2) --- updated-dependencies: - dependency-name: path-to-regexp dependency-type: indirect - dependency-name: express dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]>
* Disabled cosmetic filters on speedreader pages.
… any browser window associated
Messed up the branch after wrong rebase |
When opening the browser in incognito mode from command line, we should prevent deleting the origins marked with "Forget me when I close this site".
The reason for this is that the incognito window does not re-create the lifetime of TLD(TLDEphemeralLifetimeCreated never gets called because the default profile browser windows are not restored in incognito mode) from the main profile and as a result, they get scheduled for deletion.
Resolves brave/brave-browser#39107
Submitter Checklist:
QA/Yes
orQA/No
;release-notes/include
orrelease-notes/exclude
;OS/...
) to the associated issuenpm run test -- brave_browser_tests
,npm run test -- brave_unit_tests
wikinpm run presubmit
wiki,npm run gn_check
,npm run tslint
git rebase master
(if needed)Reviewer Checklist:
gn
After-merge Checklist:
changes has landed on
Test Plan: