Skip to content
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

Ignore cleanup cookies at startup for command line incognito mode #27111

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Ilie-Lesan
Copy link
Contributor

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:

  • I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
  • 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 wiki
    • npm run presubmit wiki, 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:

@Ilie-Lesan
Copy link
Contributor Author

@goodov I've created a new pull request for the brave/brave-browser#39107 issue. Unfortunately, I messed up the previous one(#24543) and had to close it. Sorry for that.

@@ -327,6 +327,11 @@ void EphemeralStorageService::ScheduleFirstPartyStorageAreasCleanupOnStartup() {

void EphemeralStorageService::CleanupFirstPartyStorageAreasOnStartup() {
DCHECK(!context_->IsOffTheRecord());

if (!delegate_->DoesProfileHaveAnyBrowserWindow()) {
first_party_storage_areas_to_cleanup_on_startup_.clear();
Copy link
Member

@goodov goodov Jan 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm.. this will prevent the cleanup when a normal window is opened from the incognito session (for ex. when you press Ctrl+N in the incognito window).

I think the right thing is to schedule the cleanup only after the window has appeared. I went with this approach here: #27176

I hope you don't mind that I went with a separate PR as it will speedup things a bit and we finally fix this issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@goodov Your approach better solves the problem. Thanks for looking into it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants