Clear IPFS cache when clearing browsing data #7578

yrliou · 2021-01-13T00:21:59Z

This PR runs ipfs repo gc command when users clear browsing data via settings with cache type selected.
One thing to note here (brave/brave-browser#13605) is that it will only be run if ipfs component is loaded (ex: user tries to launch local node during this browser session) and IPFS is not disabled, otherwise we won't have the executable path ready to use.

There are no new automatic tests added because we need to be able to run ipfs binary to have any meaningful tests, which is unlikely right now.

Submitter Checklist:

There is a ticket for my issue.
Used Github auto-closing keywords in the commit message.
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).
Requested a security/privacy review as needed.

Reviewer Checklist:

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:

Pre-condition: IPFS resolve method is set to Local node in the settings. (If it's not, visit https://dweb.link/ipfs/QmbWqxBEKC3P8tqsKc98xmWNzrzDtRLMiMPL8wBuTGsMnR to trigger the infobar to enable local node support)

Go to brave://ipfs to launch IPFS local node.
After daemon is ready, find ipfs binary under your profile directory(For example, /Users/yrliou/Library/Application Support/BraveSoftware/Brave-Browser-Development/nljcddpbnaianmglkpkneakjaapinabi/1.0.3 on mac), then run /go-ipfs_v0.7.0_darwin-amd64 --api=/ip4/127.0.0.1/tcp/45001 repo stat (Use port 45001 for development build, port 45002 on nightly, 45003 on dev, 45004 on beta, 45005 on release), to see the initial NumObjects.
Example of command result:

yrliou@jocelyn-imacpro ~/Library/Application Support/BraveSoftware/Brave-Browser-Development/nljcddpbnaianmglkpkneakjaapinabi/1.0.3 $ ./go-ipfs_v0.7.0_darwin-amd64 --api=/ip4/127.0.0.1/tcp/45001 repo stat
NumObjects: 13
RepoSize:   179314
StorageMax: 1000000000
RepoPath:   /Users/yrliou/Library/Application Support/BraveSoftware/Brave-Browser-Development/brave_ipfs
Version:    fs-repo@10

Visit ipfs://bafybeiemxf5abjwjbikoz4mc3a3dla6ual3jsgpdr4cjr3oz3evfyavhwq/wiki/Vincent_van_Gogh.html and run the command in step 2 again, you should see NumObjects & RepoSize increased.
Go to brave://settings and search for Clear browsing data setting, open the dialog and unselect Cached images and files.
Hit clear data and run the command in step 2 again, should have no changes from the previous result.
Open the dialog again and select Cached images and files.

Repeat step 5 and this time should see NumObjects drops from the previous result.
Visit ipfs://bafybeiemxf5abjwjbikoz4mc3a3dla6ual3jsgpdr4cjr3oz3evfyavhwq/wiki/Vincent_van_Gogh.html again and observe NumObjects & RepoSize increased using the command.
Open the Clear browsing data dialog again, choose On Exit and make sure Cached images and files is selected.
Exit the browser
Start the browser and visit brave://ipfs to launch IPFS local node.
Run /go-ipfs_v0.7.0_darwin-amd64 --api=/ip4/127.0.0.1/tcp/45001 repo stat, should see the numbers are dropped from what you observed in step 8. (Might need to wait for like a minute for daemon to launch before running this command.)

yrliou · 2021-01-14T23:44:05Z

browser/browsing_data/brave_browsing_data_remover_delegate.cc

+      FROM_HERE, {base::TaskPriority::USER_VISIBLE, base::MayBlock()},
+      base::BindOnce(&BraveBrowsingDataRemoverDelegate::WaitForIPFSRepoGC,
+                     base::Unretained(this), base::Passed(&process)),
+      CreateTaskCompletionClosure(TracingDataType::kHostCache));


kHostCache is the most reasonable existing one I could find in the list, type value is only meant for recording purpose, so we should be good here. This value is gone in chromium latest version, so we would need to update the value to kEmbedderData during future chromium upgrade.

yrliou · 2021-01-14T23:47:43Z

browser/browsing_data/brave_browsing_data_remover_delegate.cc

+#include "base/task/thread_pool.h"
+#include "base/threading/thread_restrictions.h"
+#include "base/time/time.h"
+#include "brave/browser/ipfs/ipfs_service_factory.h"


This does not have the dependency added in this PR.
We have some existing deps problems for the browsing_data target, which I would like it to be addressed in a separate PR.
For chromium, their browsing_data_remover_delegate source files are listed in the browser target itself, I think we should probably consider doing the same since we need chrome/browser and brave/browser in existing files in our browsing_data target.

bbondy · 2021-01-15T16:48:33Z

browser/browsing_data/brave_browsing_data_remover_delegate.cc

+    return;
+
+  base::FilePath path = service->GetIpfsExecutablePath();
+  if (path.empty())


We should get the last known good path when the executable path is not determined here please in a follow up.

Thanks, opened brave/brave-browser#13605.

mkarolin · 2021-01-15T16:59:28Z

chromium_src/chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.h

+class BraveBrowsingDataRemoverDelegate;
+
+#define BRAVE_CHROME_BROWSING_DATA_REMOVER_DELEGATE_H \
+  friend class BraveBrowsingDataRemoverDelegate;


Not sure I am seeing why we need the friend here.

@mkarolin For CreateTaskCompletionClosure.

mkarolin · 2021-01-15T17:03:09Z

chromium_src/base/threading/thread_restrictions.h

+
+#define BRAVE_SCOPED_ALLOW_BASE_SYNC_PRIMITIVES_H \
+  friend class ::BraveBrowsingDataRemoverDelegate;
+#include "../../../../base/threading/thread_restrictions.h"


Nit: empty line after a multi-line #define

force pushed to add an empty line after define for this file and chromium_src/chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.h

mkarolin

chromium_src/patches LGTM.

yrliou · 2021-01-15T17:33:51Z

CI already done with all green, use Skip CI label now just to fix the nit above.

kjozwiak · 2021-01-17T04:30:44Z

Verification FAILED on both Win 10 x64 & macOS 11.1 x64 using the following build:

Brave | 1.21.7 Chromium: 88.0.4324.87 (Official Build) nightly (64-bit)
-- | --
Revision | dd01ff8f58c65af81127ad5c105c79d5b571d8f3-refs/branch-heads/4324@{#1702}

Found a crash when clearing the data as per brave/brave-browser#13625.

iefremov · 2021-01-18T11:30:18Z

browser/browsing_data/brave_browsing_data_remover_delegate.cc

+  base::ThreadPool::PostTaskAndReply(
+      FROM_HERE, {base::TaskPriority::USER_VISIBLE, base::MayBlock()},
+      base::BindOnce(&BraveBrowsingDataRemoverDelegate::WaitForIPFSRepoGC,
+                     base::Unretained(this), base::Passed(&process)),


(sorry, just randomly came across this)
we really shouldn't use base::Unretained like this, because this can easily die before the task is started (especially given that most tasks posted around can be time consuming). Note, that CreateTaskCompletionClosure binds a weak pointer.

Fortunately, in our case WaitForIPFSRepoGC doesn't use that pointer at all, so we shouldn't crash. The most easy fix would be to just post a free function (i.e. not a member of BraveBrowsingDataRemoverDelegate) - there are examples of WaitForExitWithTimeout across the codebase

also, probably we'd better explicitly mention TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN

Thank you @iefremov, I'll open a follow-up PR for this.

yrliou self-assigned this Jan 13, 2021

yrliou force-pushed the ipfs_clear_cache branch 3 times, most recently from 6cfb437 to f8647b2 Compare January 14, 2021 22:22

yrliou added this to the 1.21.x - Nightly milestone Jan 14, 2021

yrliou force-pushed the ipfs_clear_cache branch from f8647b2 to 030dca1 Compare January 14, 2021 22:59

yrliou mentioned this pull request Jan 14, 2021

Empty IPFS cache missing from Delete browsing data dialog brave/brave-browser#13302

Closed

yrliou commented Jan 14, 2021

View reviewed changes

yrliou marked this pull request as ready for review January 14, 2021 23:53

yrliou requested a review from a team as a code owner January 14, 2021 23:53

yrliou requested review from bbondy and mkarolin January 14, 2021 23:53

yrliou force-pushed the ipfs_clear_cache branch from 030dca1 to 50c6f40 Compare January 14, 2021 23:54

bbondy reviewed Jan 15, 2021

View reviewed changes

bbondy approved these changes Jan 15, 2021

View reviewed changes

yrliou mentioned this pull request Jan 15, 2021

Don't rely on ipfs_client_updater component being loaded for clearing IPFS cache brave/brave-browser#13605

Closed

mkarolin reviewed Jan 15, 2021

View reviewed changes

mkarolin approved these changes Jan 15, 2021

View reviewed changes

yrliou added the CI/skip Do not run CI builds (except noplatform) label Jan 15, 2021

Clear IPFS cache when clearing browsing data

0d78434

yrliou force-pushed the ipfs_clear_cache branch from 50c6f40 to 0d78434 Compare January 15, 2021 17:35

yrliou merged commit 33ad85d into master Jan 15, 2021

yrliou deleted the ipfs_clear_cache branch January 15, 2021 17:39

yrliou removed the CI/skip Do not run CI builds (except noplatform) label Jan 15, 2021

brave-builds pushed a commit that referenced this pull request Jan 15, 2021

Uplift of #7578 (squashed) to beta

90b655e

brave-builds pushed a commit that referenced this pull request Jan 15, 2021

Uplift of #7578 (squashed) to release

54ecffd

brave-builds mentioned this pull request Jan 15, 2021

Clear IPFS cache when clearing browsing data (uplift to 1.20.x) #7616

Merged

4 tasks

brave-builds mentioned this pull request Jan 15, 2021

Clear IPFS cache when clearing browsing data (uplift to 1.19.x) #7617

Merged

4 tasks

kjozwiak mentioned this pull request Jan 17, 2021

crash when clearing "Cached images and files" while IPFS enabled/running brave/brave-browser#13625

Closed

iefremov reviewed Jan 18, 2021

View reviewed changes

yrliou mentioned this pull request Feb 11, 2021

Remove base::Unretained usage in BraveBrowsingDataRemoverDelegate::ClearIPFSCache brave/brave-browser#14102

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clear IPFS cache when clearing browsing data #7578

Clear IPFS cache when clearing browsing data #7578

yrliou commented Jan 13, 2021 •

edited

Loading

yrliou Jan 14, 2021 •

edited

Loading

yrliou Jan 14, 2021

bbondy Jan 15, 2021

yrliou Jan 15, 2021

mkarolin Jan 15, 2021

yrliou Jan 15, 2021

mkarolin Jan 15, 2021

yrliou Jan 15, 2021

mkarolin left a comment

yrliou commented Jan 15, 2021 •

edited

Loading

kjozwiak commented Jan 17, 2021

iefremov Jan 18, 2021

iefremov Jan 18, 2021

yrliou Jan 28, 2021

Clear IPFS cache when clearing browsing data #7578

Clear IPFS cache when clearing browsing data #7578

Conversation

yrliou commented Jan 13, 2021 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

yrliou Jan 14, 2021 • edited Loading

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

yrliou commented Jan 15, 2021 • edited Loading

kjozwiak commented Jan 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yrliou commented Jan 13, 2021 •

edited

Loading

yrliou Jan 14, 2021 •

edited

Loading

yrliou commented Jan 15, 2021 •

edited

Loading