Skip to content
This repository has been archived by the owner on Apr 3, 2020. It is now read-only.

Commit

Permalink
[Merge] ServiceWorker: Directly delete database directory instead of …
Browse files Browse the repository at this point in the history 
…leveldb::DestroyDB

When a database directory contains files not related to LevelDB, DestroyDB()
does not delete the directory and leaves those files. This non-empty database
directory infinitely fails ServiceWorkerDatabase::LazyOpen during a session.
(See my comment #24 in the issue for more details).

To avoid that, this CL directly deletes the directory instead of DestroyDB.

BUG=468926
TEST=content_unittests --gtest_filter=ServiceWorkerResourceStorageDiskTest.DeleteAndStartOver*
[email protected]

Review URL: https://codereview.chromium.org/1176203007

Cr-Commit-Position: refs/heads/master@{#335893}
(cherry picked from commit 0bd5fd7)

Review URL: https://codereview.chromium.org/1214793006.

Cr-Commit-Position: refs/branch-heads/2403@{#413}
Cr-Branched-From: f54b809-refs/heads/master@{#330231}
  • Loading branch information
@nhiroki
nhiroki committed Jun 29, 2015
1 parent ac2b37f commit 9f41c10
Show file tree
Hide file tree
Showing 6 changed files with 206 additions and 101 deletions.
176 changes: 96 additions & 80 deletions content/browser/service_worker/service_worker_context_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -520,16 +520,105 @@ TEST_F(ServiceWorkerContextTest, RegisterDuplicateScript) {
EXPECT_EQ(old_registration_id, notifications_[1].registration_id);
}

TEST_F(ServiceWorkerContextTest, DeleteAndStartOver) {
TEST_F(ServiceWorkerContextTest, ProviderHostIterator) {
const int kRenderProcessId1 = 1;
const int kRenderProcessId2 = 2;
const GURL kOrigin1 = GURL("http://www.example.com/");
const GURL kOrigin2 = GURL("https://www.example.com/");
int provider_id = 1;

// Host1 (provider_id=1): process_id=1, origin1.
ServiceWorkerProviderHost* host1(new ServiceWorkerProviderHost(
kRenderProcessId1, MSG_ROUTING_NONE, provider_id++,
SERVICE_WORKER_PROVIDER_FOR_WINDOW, context()->AsWeakPtr(), nullptr));
host1->SetDocumentUrl(kOrigin1);

// Host2 (provider_id=2): process_id=2, origin2.
ServiceWorkerProviderHost* host2(new ServiceWorkerProviderHost(
kRenderProcessId2, MSG_ROUTING_NONE, provider_id++,
SERVICE_WORKER_PROVIDER_FOR_WINDOW, context()->AsWeakPtr(), nullptr));
host2->SetDocumentUrl(kOrigin2);

// Host3 (provider_id=3): process_id=2, origin1.
ServiceWorkerProviderHost* host3(new ServiceWorkerProviderHost(
kRenderProcessId2, MSG_ROUTING_NONE, provider_id++,
SERVICE_WORKER_PROVIDER_FOR_WINDOW, context()->AsWeakPtr(), nullptr));
host3->SetDocumentUrl(kOrigin1);

// Host4 (provider_id=4): process_id=2, origin2, for ServiceWorker.
ServiceWorkerProviderHost* host4(new ServiceWorkerProviderHost(
kRenderProcessId2, MSG_ROUTING_NONE, provider_id++,
SERVICE_WORKER_PROVIDER_FOR_CONTROLLER, context()->AsWeakPtr(), nullptr));
host4->SetDocumentUrl(kOrigin2);

context()->AddProviderHost(make_scoped_ptr(host1));
context()->AddProviderHost(make_scoped_ptr(host2));
context()->AddProviderHost(make_scoped_ptr(host3));
context()->AddProviderHost(make_scoped_ptr(host4));

// Iterate over all provider hosts.
std::set<ServiceWorkerProviderHost*> results;
for (auto it = context()->GetProviderHostIterator(); !it->IsAtEnd();
it->Advance()) {
results.insert(it->GetProviderHost());
}
EXPECT_EQ(4u, results.size());
EXPECT_TRUE(ContainsKey(results, host1));
EXPECT_TRUE(ContainsKey(results, host2));
EXPECT_TRUE(ContainsKey(results, host3));
EXPECT_TRUE(ContainsKey(results, host4));

// Iterate over the client provider hosts that belong to kOrigin1.
results.clear();
for (auto it = context()->GetClientProviderHostIterator(kOrigin1);
!it->IsAtEnd(); it->Advance()) {
results.insert(it->GetProviderHost());
}
EXPECT_EQ(2u, results.size());
EXPECT_TRUE(ContainsKey(results, host1));
EXPECT_TRUE(ContainsKey(results, host3));

// Iterate over the provider hosts that belong to kOrigin2.
// (This should not include host4 as it's not for controllee.)
results.clear();
for (auto it = context()->GetClientProviderHostIterator(kOrigin2);
!it->IsAtEnd(); it->Advance()) {
results.insert(it->GetProviderHost());
}
EXPECT_EQ(1u, results.size());
EXPECT_TRUE(ContainsKey(results, host2));

context()->RemoveProviderHost(kRenderProcessId1, 1);
context()->RemoveProviderHost(kRenderProcessId2, 2);
context()->RemoveProviderHost(kRenderProcessId2, 3);
context()->RemoveProviderHost(kRenderProcessId2, 4);
}

class ServiceWorkerContextRecoveryTest
: public ServiceWorkerContextTest,
public testing::WithParamInterface<bool> {
public:
ServiceWorkerContextRecoveryTest() {}
virtual ~ServiceWorkerContextRecoveryTest() {}
};

INSTANTIATE_TEST_CASE_P(ServiceWorkerContextRecoveryTest,
ServiceWorkerContextRecoveryTest,
testing::Values(true, false));

TEST_P(ServiceWorkerContextRecoveryTest, DeleteAndStartOver) {
GURL pattern("http://www.example.com/");
GURL script_url("http://www.example.com/service_worker.js");

// Reinitialize the helper to test on-disk storage.
base::ScopedTempDir user_data_directory;
ASSERT_TRUE(user_data_directory.CreateUniqueTempDir());
helper_.reset(new EmbeddedWorkerTestHelper(user_data_directory.path(),
render_process_id_));
helper_->context_wrapper()->AddObserver(this);
bool is_storage_on_disk = GetParam();
if (is_storage_on_disk) {
// Reinitialize the helper to test on-disk storage.
base::ScopedTempDir user_data_directory;
ASSERT_TRUE(user_data_directory.CreateUniqueTempDir());
helper_.reset(new EmbeddedWorkerTestHelper(user_data_directory.path(),
render_process_id_));
helper_->context_wrapper()->AddObserver(this);
}

int64 registration_id = kInvalidServiceWorkerRegistrationId;
bool called = false;
Expand Down Expand Up @@ -614,78 +703,5 @@ TEST_F(ServiceWorkerContextTest, DeleteAndStartOver) {
EXPECT_EQ(registration_id, notifications_[2].registration_id);
}

TEST_F(ServiceWorkerContextTest, ProviderHostIterator) {
const int kRenderProcessId1 = 1;
const int kRenderProcessId2 = 2;
const GURL kOrigin1 = GURL("http://www.example.com/");
const GURL kOrigin2 = GURL("https://www.example.com/");
int provider_id = 1;

// Host1 (provider_id=1): process_id=1, origin1.
ServiceWorkerProviderHost* host1(new ServiceWorkerProviderHost(
kRenderProcessId1, MSG_ROUTING_NONE, provider_id++,
SERVICE_WORKER_PROVIDER_FOR_WINDOW, context()->AsWeakPtr(), nullptr));
host1->SetDocumentUrl(kOrigin1);

// Host2 (provider_id=2): process_id=2, origin2.
ServiceWorkerProviderHost* host2(new ServiceWorkerProviderHost(
kRenderProcessId2, MSG_ROUTING_NONE, provider_id++,
SERVICE_WORKER_PROVIDER_FOR_WINDOW, context()->AsWeakPtr(), nullptr));
host2->SetDocumentUrl(kOrigin2);

// Host3 (provider_id=3): process_id=2, origin1.
ServiceWorkerProviderHost* host3(new ServiceWorkerProviderHost(
kRenderProcessId2, MSG_ROUTING_NONE, provider_id++,
SERVICE_WORKER_PROVIDER_FOR_WINDOW, context()->AsWeakPtr(), nullptr));
host3->SetDocumentUrl(kOrigin1);

// Host4 (provider_id=4): process_id=2, origin2, for ServiceWorker.
ServiceWorkerProviderHost* host4(new ServiceWorkerProviderHost(
kRenderProcessId2, MSG_ROUTING_NONE, provider_id++,
SERVICE_WORKER_PROVIDER_FOR_CONTROLLER, context()->AsWeakPtr(), nullptr));
host4->SetDocumentUrl(kOrigin2);

context()->AddProviderHost(make_scoped_ptr(host1));
context()->AddProviderHost(make_scoped_ptr(host2));
context()->AddProviderHost(make_scoped_ptr(host3));
context()->AddProviderHost(make_scoped_ptr(host4));

// Iterate over all provider hosts.
std::set<ServiceWorkerProviderHost*> results;
for (auto it = context()->GetProviderHostIterator(); !it->IsAtEnd();
it->Advance()) {
results.insert(it->GetProviderHost());
}
EXPECT_EQ(4u, results.size());
EXPECT_TRUE(ContainsKey(results, host1));
EXPECT_TRUE(ContainsKey(results, host2));
EXPECT_TRUE(ContainsKey(results, host3));
EXPECT_TRUE(ContainsKey(results, host4));

// Iterate over the client provider hosts that belong to kOrigin1.
results.clear();
for (auto it = context()->GetClientProviderHostIterator(kOrigin1);
!it->IsAtEnd(); it->Advance()) {
results.insert(it->GetProviderHost());
}
EXPECT_EQ(2u, results.size());
EXPECT_TRUE(ContainsKey(results, host1));
EXPECT_TRUE(ContainsKey(results, host3));

// Iterate over the provider hosts that belong to kOrigin2.
// (This should not include host4 as it's not for controllee.)
results.clear();
for (auto it = context()->GetClientProviderHostIterator(kOrigin2);
!it->IsAtEnd(); it->Advance()) {
results.insert(it->GetProviderHost());
}
EXPECT_EQ(1u, results.size());
EXPECT_TRUE(ContainsKey(results, host2));

context()->RemoveProviderHost(kRenderProcessId1, 1);
context()->RemoveProviderHost(kRenderProcessId2, 2);
context()->RemoveProviderHost(kRenderProcessId2, 3);
context()->RemoveProviderHost(kRenderProcessId2, 4);
}

} // namespace content
31 changes: 15 additions & 16 deletions content/browser/service_worker/service_worker_database.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -970,18 +970,17 @@ ServiceWorkerDatabase::Status ServiceWorkerDatabase::DestroyDatabase() {
DCHECK(sequence_checker_.CalledOnValidSequencedThread());
Disable(FROM_HERE, STATUS_OK);

leveldb::Options options;
if (path_.empty()) {
if (env_) {
options.env = env_.get();
} else {
// In-memory database not initialized.
return STATUS_OK;
}
if (IsDatabaseInMemory()) {
env_.reset();
return STATUS_OK;
}

Status status =
LevelDBStatusToStatus(leveldb::DestroyDB(path_.AsUTF8Unsafe(), options));
// Directly delete the database directory instead of leveldb::DestroyDB()
// because the API does not delete the directory if there are unrelated files.
// (https://code.google.com/p/chromium/issues/detail?id=468926#c24)
Status status = base::DeleteFile(path_, true /* recursive */)
? STATUS_OK
: STATUS_ERROR_FAILED;
ServiceWorkerMetrics::RecordDestroyDatabaseResult(status);
return status;
}
Expand All @@ -996,13 +995,9 @@ ServiceWorkerDatabase::Status ServiceWorkerDatabase::LazyOpen(
if (IsOpen())
return STATUS_OK;

// When |path_| is empty, open a database in-memory.
bool use_in_memory_db = path_.empty();

if (!create_if_missing) {
// Avoid opening a database if it does not exist at the |path_|.
if (use_in_memory_db ||
!base::PathExists(path_) ||
if (IsDatabaseInMemory() || !base::PathExists(path_) ||
base::IsDirectoryEmpty(path_)) {
return STATUS_ERROR_NOT_FOUND;
}
Expand All @@ -1011,7 +1006,7 @@ ServiceWorkerDatabase::Status ServiceWorkerDatabase::LazyOpen(
leveldb::Options options;
options.create_if_missing = create_if_missing;
options.reuse_logs = leveldb_env::kDefaultLogReuseOptionValue;
if (use_in_memory_db) {
if (IsDatabaseInMemory()) {
env_.reset(leveldb::NewMemEnv(leveldb::Env::Default()));
options.env = env_.get();
}
Expand Down Expand Up @@ -1463,4 +1458,8 @@ void ServiceWorkerDatabase::HandleWriteResult(
ServiceWorkerMetrics::CountWriteDatabaseResult(status);
}

bool ServiceWorkerDatabase::IsDatabaseInMemory() const {
return path_.empty();
}

} // namespace content
4 changes: 3 additions & 1 deletion content/browser/service_worker/service_worker_database.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -350,7 +350,7 @@ class CONTENT_EXPORT ServiceWorkerDatabase {
const tracked_objects::Location& from_here,
Status status);

base::FilePath path_;
const base::FilePath path_;
scoped_ptr<leveldb::Env> env_;
scoped_ptr<leveldb::DB> db_;

Expand All @@ -365,6 +365,8 @@ class CONTENT_EXPORT ServiceWorkerDatabase {
};
State state_;

bool IsDatabaseInMemory() const;

base::SequenceChecker sequence_checker_;

FRIEND_TEST_ALL_PREFIXES(ServiceWorkerDatabaseTest, OpenDatabase);
Expand Down
6 changes: 2 additions & 4 deletions content/browser/service_worker/service_worker_storage.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -825,13 +825,11 @@ void ServiceWorkerStorage::DeleteAndStartOver(const StatusCallback& callback) {

// Delete the database on the database thread.
PostTaskAndReplyWithResult(
database_task_manager_->GetTaskRunner(),
FROM_HERE,
database_task_manager_->GetTaskRunner(), FROM_HERE,
base::Bind(&ServiceWorkerDatabase::DestroyDatabase,
base::Unretained(database_.get())),
base::Bind(&ServiceWorkerStorage::DidDeleteDatabase,
weak_factory_.GetWeakPtr(),
callback));
weak_factory_.GetWeakPtr(), callback));
}

int64 ServiceWorkerStorage::NewRegistrationId() {
Expand Down
6 changes: 6 additions & 0 deletions content/browser/service_worker/service_worker_storage.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,12 @@ class CONTENT_EXPORT ServiceWorkerStorage
CleanupOnRestart);
FRIEND_TEST_ALL_PREFIXES(ServiceWorkerResourceStorageDiskTest,
ClearOnExit);
FRIEND_TEST_ALL_PREFIXES(ServiceWorkerResourceStorageDiskTest,
DeleteAndStartOver);
FRIEND_TEST_ALL_PREFIXES(ServiceWorkerResourceStorageDiskTest,
DeleteAndStartOver_UnrelatedFileExists);
FRIEND_TEST_ALL_PREFIXES(ServiceWorkerResourceStorageDiskTest,
DeleteAndStartOver_OpenedFileExists);

struct InitialData {
int64 next_registration_id;
Expand Down
Loading

0 comments on commit 9f41c10

Please sign in to comment.