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

Clear IPFS cache when clearing browsing data #7578

Merged
merged 1 commit into from
Jan 15, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions browser/browsing_data/BUILD.gn
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import("//brave/components/ipfs/buildflags/buildflags.gni")
import("//extensions/buildflags/buildflags.gni")

source_set("browsing_data") {
Expand All @@ -12,6 +13,7 @@ source_set("browsing_data") {

deps = [
"//base",
"//brave/components/ipfs/buildflags",
"//chrome/common",
"//components/browsing_data/core",
"//components/content_settings/core/browser",
Expand All @@ -26,4 +28,8 @@ source_set("browsing_data") {
"//extensions/browser",
]
}

if (ipfs_enabled) {
deps += [ "//brave/components/ipfs" ]
}
}
84 changes: 84 additions & 0 deletions browser/browsing_data/brave_browsing_data_remover_delegate.cc
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,19 @@
#include "extensions/browser/event_router.h"
#endif

#if BUILDFLAG(IPFS_ENABLED)
#include "base/command_line.h"
#include "base/files/file_path.h"
#include "base/process/launch.h"
#include "base/process/process.h"
#include "base/task/task_traits.h"
#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"
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

#include "brave/components/ipfs/ipfs_service.h"
#endif

BraveBrowsingDataRemoverDelegate::BraveBrowsingDataRemoverDelegate(
content::BrowserContext* browser_context)
: ChromeBrowsingDataRemoverDelegate(browser_context),
Expand All @@ -46,6 +59,12 @@ void BraveBrowsingDataRemoverDelegate::RemoveEmbedderData(
// shields settings with non-empty resource ids.
if (remove_mask & DATA_TYPE_CONTENT_SETTINGS)
ClearShieldsSettings(delete_begin, delete_end);

#if BUILDFLAG(IPFS_ENABLED)
if (remove_mask & content::BrowsingDataRemover::DATA_TYPE_CACHE)
ClearIPFSCache();
#endif

#if BUILDFLAG(ENABLE_EXTENSIONS)
if (remove_mask & DATA_TYPE_HISTORY) {
auto* event_router = extensions::EventRouter::Get(profile_);
Expand Down Expand Up @@ -91,3 +110,68 @@ void BraveBrowsingDataRemoverDelegate::ClearShieldsSettings(
}
}
}

#if BUILDFLAG(IPFS_ENABLED)
void BraveBrowsingDataRemoverDelegate::WaitForIPFSRepoGC(
base::Process process) {
bool exited = false;

{
base::ScopedAllowBaseSyncPrimitives scoped_allow_base_sync_primitives;

// Because we set maximum IPFS storage size as 1GB in Brave, ipfs repo gc
// command should be finished in just a few seconds and we do not expect
// this child process would hang forever. To be safe, we will wait for 30
// seconds max here.
exited = process.WaitForExitWithTimeout(base::TimeDelta::FromSeconds(30),
nullptr);
}

if (!exited)
process.Terminate(0, false /* wait */);
}

// Run ipfs repo gc command to clear IPFS cache when IPFS executable path is
// available. Because the command does not support time ranged cleanup, we will
// always clear the whole cache expect for pinned files when clearing browsing
// data.
void BraveBrowsingDataRemoverDelegate::ClearIPFSCache() {
auto* service =
ipfs::IpfsServiceFactory::GetInstance()->GetForContext(profile_);
if (!service)
return;

base::FilePath path = service->GetIpfsExecutablePath();
if (path.empty())
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, opened brave/brave-browser#13605.

return;

base::CommandLine cmdline(path);
cmdline.AppendArg("repo");
cmdline.AppendArg("gc");

base::FilePath data_path = service->GetDataPath();
base::LaunchOptions options;
#if defined(OS_WIN)
options.environment[L"IPFS_PATH"] = data_path.value();
#else
options.environment["IPFS_PATH"] = data_path.value();
#endif
#if defined(OS_LINUX)
options.kill_on_parent_death = true;
#endif
#if defined(OS_WIN)
options.start_hidden = true;
#endif

base::Process process = base::LaunchProcess(cmdline, options);
if (!process.IsValid()) {
return;
}

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

Choose a reason for hiding this comment

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

(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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

CreateTaskCompletionClosure(TracingDataType::kHostCache));
Copy link
Member Author

@yrliou yrliou Jan 14, 2021

Choose a reason for hiding this comment

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

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.

}
#endif // BUILDFLAG(IPFS_ENABLED)
9 changes: 9 additions & 0 deletions browser/browsing_data/brave_browsing_data_remover_delegate.h
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,13 @@
#define BRAVE_BROWSER_BROWSING_DATA_BRAVE_BROWSING_DATA_REMOVER_DELEGATE_H_

#include "base/time/time.h"
#include "brave/components/ipfs/buildflags/buildflags.h"
#include "chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.h"

namespace base {
class Process;
} // namespace base

namespace content_settings {
class BravePrefProvider;
} // namespace content_settings
Expand Down Expand Up @@ -41,6 +46,10 @@ class BraveBrowsingDataRemoverDelegate
override;

void ClearShieldsSettings(base::Time begin_time, base::Time end_time);
#if BUILDFLAG(IPFS_ENABLED)
void ClearIPFSCache();
void WaitForIPFSRepoGC(base::Process process);
#endif

Profile* profile_;
};
Expand Down
17 changes: 17 additions & 0 deletions chromium_src/base/threading/thread_restrictions.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
/* Copyright (c) 2021 The Brave Authors. All rights reserved.
* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this file,
* You can obtain one at http://mozilla.org/MPL/2.0/. */

#ifndef BRAVE_CHROMIUM_SRC_BASE_THREADING_THREAD_RESTRICTIONS_H_
#define BRAVE_CHROMIUM_SRC_BASE_THREADING_THREAD_RESTRICTIONS_H_

class BraveBrowsingDataRemoverDelegate;

#define BRAVE_SCOPED_ALLOW_BASE_SYNC_PRIMITIVES_H \
friend class ::BraveBrowsingDataRemoverDelegate;

#include "../../../../base/threading/thread_restrictions.h"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: empty line after a multi-line #define

Copy link
Member Author

Choose a reason for hiding this comment

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

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

#undef BRAVE_SCOPED_ALLOW_BASE_SYNC_PRIMITIVES_H

#endif // BRAVE_CHROMIUM_SRC_BASE_THREADING_THREAD_RESTRICTIONS_H_
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
/* Copyright (c) 2021 The Brave Authors. All rights reserved.
* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this file,
* You can obtain one at http://mozilla.org/MPL/2.0/. */

#ifndef BRAVE_CHROMIUM_SRC_CHROME_BROWSER_BROWSING_DATA_CHROME_BROWSING_DATA_REMOVER_DELEGATE_H_
#define BRAVE_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;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

@mkarolin For CreateTaskCompletionClosure.


#include "../../../../../chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.h"
#undef BRAVE_CHROME_BROWSING_DATA_REMOVER_DELEGATE_H

#endif // BRAVE_CHROMIUM_SRC_CHROME_BROWSER_BROWSING_DATA_CHROME_BROWSING_DATA_REMOVER_DELEGATE_H_
2 changes: 1 addition & 1 deletion components/ipfs/ipfs_service.h
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ class IpfsService : public KeyedService,
bool IsIPFSExecutableAvailable() const;
void RegisterIpfsClientUpdater();
IPFSResolveMethodTypes GetIPFSResolveMethodType() const;
base::FilePath GetIpfsExecutablePath();
base::FilePath GetDataPath() const;
base::FilePath GetConfigFilePath() const;

Expand All @@ -88,7 +89,6 @@ class IpfsService : public KeyedService,
void RunLaunchDaemonCallbackForTest(bool result);

protected:
base::FilePath GetIpfsExecutablePath();
void OnConfigLoaded(GetConfigCallback, const std::pair<bool, std::string>&);

private:
Expand Down
12 changes: 12 additions & 0 deletions patches/base-threading-thread_restrictions.h.patch
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
diff --git a/base/threading/thread_restrictions.h b/base/threading/thread_restrictions.h
index ac19eaecc4cc36c4376052c33314bc289bab7bcf..6e79d17b49ba84fa4ccbbfefd88c3aa1888a855b 100644
--- a/base/threading/thread_restrictions.h
+++ b/base/threading/thread_restrictions.h
@@ -433,6 +433,7 @@ INLINE_IF_DCHECK_IS_OFF void DisallowBaseSyncPrimitives()
EMPTY_BODY_IF_DCHECK_IS_OFF;

class BASE_EXPORT ScopedAllowBaseSyncPrimitives {
+ BRAVE_SCOPED_ALLOW_BASE_SYNC_PRIMITIVES_H
private:
// This can only be instantiated by friends. Use
// ScopedAllowBaseSyncPrimitivesForTesting in unit tests to avoid the friend
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
diff --git a/chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.h b/chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.h
index ae0d55606244bca6ac1a1fa0037e0dc286c36174..74bc2123ced2db3fcdfc8854aac5a61f6067bd62 100644
--- a/chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.h
+++ b/chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.h
@@ -195,6 +195,7 @@ class ChromeBrowsingDataRemoverDelegate
void OverrideDomainReliabilityClearerForTesting(
DomainReliabilityClearer clearer);

+ BRAVE_CHROME_BROWSING_DATA_REMOVER_DELEGATE_H
private:
using WebRtcEventLogManager = webrtc_event_logging::WebRtcEventLogManager;