Commit a1504187 authored by Daniel Murphy's avatar Daniel Murphy Committed by Commit Bot

[IndexedDB] Avoid race condition in context shutdown

This change avoids a race condition in IndexedDBContextImpl's Shutdown
method. In that method, std::move is used on a variable on the UI,
where that variable is supposed to be only accesed or modified on the
IDB task runner.

Being a race condition, this is really hard to test, and I'm not sure
what I would do to test this. It is also not necessarily the fix for
the given bug.

Bug: 1018741
Change-Id: Ia071e5a053952af8c74aa30e0e5e218c4b97100f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1885498Reviewed-by: default avatarChase Phillips <cmp@chromium.org>
Reviewed-by: default avatarKen Rockot <rockot@google.com>
Commit-Queue: Daniel Murphy <dmurph@chromium.org>
Cr-Commit-Position: refs/heads/master@{#710906}
parent 8229a222
...@@ -531,9 +531,13 @@ IndexedDBContextImpl::~IndexedDBContextImpl() { ...@@ -531,9 +531,13 @@ IndexedDBContextImpl::~IndexedDBContextImpl() {
} }
void IndexedDBContextImpl::Shutdown() { void IndexedDBContextImpl::Shutdown() {
// Important: This function is NOT called on the IDB Task Runner. All variable
// access must be thread-safe.
DCHECK_CURRENTLY_ON(BrowserThread::UI);
if (is_incognito()) if (is_incognito())
return; return;
// TODO(dmurph): Make this variable atomic.
if (force_keep_session_state_) if (force_keep_session_state_)
return; return;
...@@ -544,11 +548,14 @@ void IndexedDBContextImpl::Shutdown() { ...@@ -544,11 +548,14 @@ void IndexedDBContextImpl::Shutdown() {
FROM_HERE, FROM_HERE,
base::BindOnce( base::BindOnce(
[](const base::FilePath& indexeddb_path, [](const base::FilePath& indexeddb_path,
std::unique_ptr<IndexedDBFactory> factory, scoped_refptr<IndexedDBContextImpl> context,
scoped_refptr<storage::SpecialStoragePolicy> scoped_refptr<storage::SpecialStoragePolicy>
special_storage_policy) { special_storage_policy) {
std::vector<Origin> origins; std::vector<Origin> origins;
std::vector<base::FilePath> file_paths; std::vector<base::FilePath> file_paths;
// This function only needs the factory, and not the context, but
// the context is used because passing that is thread-safe.
IndexedDBFactoryImpl* factory = context->GetIDBFactory();
GetAllOriginsAndPaths(indexeddb_path, &origins, &file_paths); GetAllOriginsAndPaths(indexeddb_path, &origins, &file_paths);
DCHECK_EQ(origins.size(), file_paths.size()); DCHECK_EQ(origins.size(), file_paths.size());
auto file_path = file_paths.cbegin(); auto file_path = file_paths.cbegin();
...@@ -559,13 +566,12 @@ void IndexedDBContextImpl::Shutdown() { ...@@ -559,13 +566,12 @@ void IndexedDBContextImpl::Shutdown() {
continue; continue;
if (special_storage_policy->IsStorageProtected(origin_url)) if (special_storage_policy->IsStorageProtected(origin_url))
continue; continue;
if (factory.get()) if (factory)
factory->ForceClose(*origin); factory->ForceClose(*origin, false);
base::DeleteFile(*file_path, true); base::DeleteFile(*file_path, true);
} }
}, },
data_path_, std::move(indexeddb_factory_), data_path_, base::WrapRefCounted(this), special_storage_policy_));
special_storage_policy_));
} }
} }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment