diff options
-rw-r--r-- | content/child/web_database_observer_impl.cc | 22 | ||||
-rw-r--r-- | content/child/web_database_observer_impl.h | 7 | ||||
-rw-r--r-- | content/common/database_connections_unittest.cc | 36 | ||||
-rw-r--r-- | content/renderer/render_thread_impl.cc | 11 | ||||
-rw-r--r-- | storage/common/database/database_connections.cc | 48 | ||||
-rw-r--r-- | storage/common/database/database_connections.h | 14 | ||||
-rw-r--r-- | third_party/WebKit/Source/modules/webdatabase/DatabaseTracker.cpp | 76 |
7 files changed, 88 insertions, 126 deletions
diff --git a/content/child/web_database_observer_impl.cc b/content/child/web_database_observer_impl.cc index 68534a2..8209f88 100644 --- a/content/child/web_database_observer_impl.cc +++ b/content/child/web_database_observer_impl.cc @@ -4,8 +4,11 @@ #include "content/child/web_database_observer_impl.h" +#include "base/bind.h" #include "base/metrics/histogram.h" +#include "base/single_thread_task_runner.h" #include "base/strings/string16.h" +#include "base/thread_task_runner_handle.h" #include "content/common/database_messages.h" #include "third_party/WebKit/public/platform/WebCString.h" #include "third_party/WebKit/public/platform/WebString.h" @@ -54,8 +57,10 @@ int DetermineHistogramResult(int websql_error, int sqlite_error) { WebDatabaseObserverImpl::WebDatabaseObserverImpl(IPC::SyncMessageFilter* sender) : sender_(sender), - open_connections_(new storage::DatabaseConnectionsWrapper) { + open_connections_(new storage::DatabaseConnectionsWrapper), + main_thread_task_runner_(base::ThreadTaskRunnerHandle::Get()) { DCHECK(sender); + DCHECK(main_thread_task_runner_); } WebDatabaseObserverImpl::~WebDatabaseObserverImpl() { @@ -83,8 +88,13 @@ void WebDatabaseObserverImpl::databaseModified( void WebDatabaseObserverImpl::databaseClosed( const WebString& origin_identifier, const WebString& database_name) { - sender_->Send(new DatabaseHostMsg_Closed( - origin_identifier.utf8(), database_name)); + DCHECK(!main_thread_task_runner_->RunsTasksOnCurrentThread()); + main_thread_task_runner_->PostTask( + FROM_HERE, + base::Bind( + base::IgnoreResult(&IPC::SyncMessageFilter::Send), + sender_, + new DatabaseHostMsg_Closed(origin_identifier.utf8(), database_name))); open_connections_->RemoveOpenConnection(origin_identifier.utf8(), database_name); } @@ -156,8 +166,10 @@ void WebDatabaseObserverImpl::reportVacuumDatabaseResult( HandleSqliteError(origin_identifier, database_name, sqlite_error); } -void WebDatabaseObserverImpl::WaitForAllDatabasesToClose() { - open_connections_->WaitForAllDatabasesToClose(); +bool WebDatabaseObserverImpl::WaitForAllDatabasesToClose( + base::TimeDelta timeout) { + DCHECK(main_thread_task_runner_->RunsTasksOnCurrentThread()); + return open_connections_->WaitForAllDatabasesToClose(timeout); } void WebDatabaseObserverImpl::HandleSqliteError( diff --git a/content/child/web_database_observer_impl.h b/content/child/web_database_observer_impl.h index 0a05773..bfda6c4 100644 --- a/content/child/web_database_observer_impl.h +++ b/content/child/web_database_observer_impl.h @@ -10,6 +10,10 @@ #include "storage/common/database/database_connections.h" #include "third_party/WebKit/public/platform/WebDatabaseObserver.h" +namespace base { +class SingleThreadTaskRunner; +} + namespace content { class WebDatabaseObserverImpl : public blink::WebDatabaseObserver { @@ -55,7 +59,7 @@ class WebDatabaseObserverImpl : public blink::WebDatabaseObserver { const blink::WebString& database_name, int sqlite_error) override; - void WaitForAllDatabasesToClose(); + bool WaitForAllDatabasesToClose(base::TimeDelta timeout); private: void HandleSqliteError(const blink::WebString& origin_identifier, @@ -64,6 +68,7 @@ class WebDatabaseObserverImpl : public blink::WebDatabaseObserver { scoped_refptr<IPC::SyncMessageFilter> sender_; scoped_refptr<storage::DatabaseConnectionsWrapper> open_connections_; + scoped_refptr<base::SingleThreadTaskRunner> main_thread_task_runner_; DISALLOW_COPY_AND_ASSIGN(WebDatabaseObserverImpl); }; diff --git a/content/common/database_connections_unittest.cc b/content/common/database_connections_unittest.cc index 8992f0d..a329553d 100644 --- a/content/common/database_connections_unittest.cc +++ b/content/common/database_connections_unittest.cc @@ -26,16 +26,6 @@ void RemoveConnectionTask( obj->RemoveOpenConnection(origin_id, database_name); } -void ScheduleRemoveConnectionTask( - base::Thread* thread, const std::string& origin_id, - const base::string16& database_name, - scoped_refptr<DatabaseConnectionsWrapper> obj, - bool* did_task_execute) { - thread->task_runner()->PostTask( - FROM_HERE, base::Bind(&RemoveConnectionTask, origin_id, database_name, - obj, did_task_execute)); -} - } // anonymous namespace TEST(DatabaseConnectionsTest, DatabaseConnectionsTest) { @@ -115,29 +105,21 @@ TEST(DatabaseConnectionsTest, DatabaseConnectionsWrapperTest) { EXPECT_TRUE(obj->HasOpenConnections()); obj->RemoveOpenConnection(kOriginId, kName); EXPECT_FALSE(obj->HasOpenConnections()); - obj->WaitForAllDatabasesToClose(); // should return immediately - - // Test WaitForAllDatabasesToClose with the last connection - // being removed on the current thread. - obj->AddOpenConnection(kOriginId, kName); - bool did_task_execute = false; - base::ThreadTaskRunnerHandle::Get()->PostTask( - FROM_HERE, base::Bind(&RemoveConnectionTask, kOriginId, kName, obj, - &did_task_execute)); - obj->WaitForAllDatabasesToClose(); // should return after the task executes - EXPECT_TRUE(did_task_execute); - EXPECT_FALSE(obj->HasOpenConnections()); + EXPECT_TRUE(obj->WaitForAllDatabasesToClose(base::TimeDelta())); // Test WaitForAllDatabasesToClose with the last connection // being removed on another thread. obj->AddOpenConnection(kOriginId, kName); + EXPECT_FALSE(obj->WaitForAllDatabasesToClose(base::TimeDelta())); base::Thread thread("WrapperTestThread"); thread.Start(); - did_task_execute = false; - base::ThreadTaskRunnerHandle::Get()->PostTask( - FROM_HERE, base::Bind(&ScheduleRemoveConnectionTask, &thread, kOriginId, - kName, obj, &did_task_execute)); - obj->WaitForAllDatabasesToClose(); // should return after the task executes + bool did_task_execute = false; + thread.task_runner()->PostTask( + FROM_HERE, base::Bind(&RemoveConnectionTask, kOriginId, kName, obj, + &did_task_execute)); + // Use a long timeout value to avoid timeouts on test bots. + EXPECT_TRUE(obj->WaitForAllDatabasesToClose( + base::TimeDelta::FromSeconds(15))); EXPECT_TRUE(did_task_execute); EXPECT_FALSE(obj->HasOpenConnections()); } diff --git a/content/renderer/render_thread_impl.cc b/content/renderer/render_thread_impl.cc index b6d9dd6..6293989 100644 --- a/content/renderer/render_thread_impl.cc +++ b/content/renderer/render_thread_impl.cc @@ -819,13 +819,10 @@ void RenderThreadImpl::Shutdown() { // Wait for all databases to be closed. if (blink_platform_impl_) { - // WaitForAllDatabasesToClose might run a nested message loop. To avoid - // processing timer events while we're already in the process of shutting - // down blink, put a ScopePageLoadDeferrer on the stack. - WebView::willEnterModalLoop(); - blink_platform_impl_->web_database_observer_impl() - ->WaitForAllDatabasesToClose(); - WebView::didExitModalLoop(); + // Crash the process if they fail to close after a generous amount of time. + bool all_closed = blink_platform_impl_->web_database_observer_impl() + ->WaitForAllDatabasesToClose(base::TimeDelta::FromSeconds(60)); + CHECK(all_closed); } // Shutdown in reverse of the initialization order. diff --git a/storage/common/database/database_connections.cc b/storage/common/database/database_connections.cc index afa9ee3..35bc139 100644 --- a/storage/common/database/database_connections.cc +++ b/storage/common/database/database_connections.cc @@ -8,6 +8,7 @@ #include "base/bind.h" #include "base/logging.h" #include "base/message_loop/message_loop.h" +#include "base/synchronization/waitable_event.h" #include "base/thread_task_runner_handle.h" namespace storage { @@ -121,27 +122,13 @@ bool DatabaseConnections::RemoveConnectionsHelper( return true; } -DatabaseConnectionsWrapper::DatabaseConnectionsWrapper() - : waiting_for_dbs_to_close_(false), - main_thread_(base::ThreadTaskRunnerHandle::Get()) { +DatabaseConnectionsWrapper::DatabaseConnectionsWrapper() { } DatabaseConnectionsWrapper::~DatabaseConnectionsWrapper() { } -void DatabaseConnectionsWrapper::WaitForAllDatabasesToClose() { - // We assume that new databases won't be open while we're waiting. - DCHECK(main_thread_->BelongsToCurrentThread()); - if (HasOpenConnections()) { - base::AutoReset<bool> auto_reset(&waiting_for_dbs_to_close_, true); - base::MessageLoop::ScopedNestableTaskAllower allow( - base::MessageLoop::current()); - base::MessageLoop::current()->Run(); - } -} - bool DatabaseConnectionsWrapper::HasOpenConnections() { - DCHECK(main_thread_->BelongsToCurrentThread()); base::AutoLock auto_lock(open_connections_lock_); return !open_connections_.IsEmpty(); } @@ -149,7 +136,6 @@ bool DatabaseConnectionsWrapper::HasOpenConnections() { void DatabaseConnectionsWrapper::AddOpenConnection( const std::string& origin_identifier, const base::string16& database_name) { - // We add to the collection immediately on any thread. base::AutoLock auto_lock(open_connections_lock_); open_connections_.AddConnection(origin_identifier, database_name); } @@ -157,19 +143,27 @@ void DatabaseConnectionsWrapper::AddOpenConnection( void DatabaseConnectionsWrapper::RemoveOpenConnection( const std::string& origin_identifier, const base::string16& database_name) { - // But only remove from the collection on the main thread - // so we can handle the waiting_for_dbs_to_close_ case. - if (!main_thread_->BelongsToCurrentThread()) { - main_thread_->PostTask( - FROM_HERE, - base::Bind(&DatabaseConnectionsWrapper::RemoveOpenConnection, this, - origin_identifier, database_name)); - return; - } base::AutoLock auto_lock(open_connections_lock_); open_connections_.RemoveConnection(origin_identifier, database_name); - if (waiting_for_dbs_to_close_ && open_connections_.IsEmpty()) - base::MessageLoop::current()->QuitWhenIdle(); + if (waiting_to_close_event_ && open_connections_.IsEmpty()) + waiting_to_close_event_->Signal(); +} + +bool DatabaseConnectionsWrapper::WaitForAllDatabasesToClose( + base::TimeDelta timeout) { + base::WaitableEvent waitable_event(true, false); + { + base::AutoLock auto_lock(open_connections_lock_); + if (open_connections_.IsEmpty()) + return true; + waiting_to_close_event_ = &waitable_event; + } + waitable_event.TimedWait(timeout); + { + base::AutoLock auto_lock(open_connections_lock_); + waiting_to_close_event_ = nullptr; + return open_connections_.IsEmpty(); + } } } // namespace storage diff --git a/storage/common/database/database_connections.h b/storage/common/database/database_connections.h index 24bcbd0..ec36b0f 100644 --- a/storage/common/database/database_connections.h +++ b/storage/common/database/database_connections.h @@ -12,10 +12,12 @@ #include "base/memory/ref_counted.h" #include "base/strings/string16.h" #include "base/synchronization/lock.h" +#include "base/time/time.h" #include "storage/common/storage_common_export.h" namespace base { class SingleThreadTaskRunner; +class WaitableEvent; } namespace storage { @@ -74,24 +76,22 @@ class STORAGE_COMMON_EXPORT DatabaseConnectionsWrapper public: DatabaseConnectionsWrapper(); - // The Wait and Has methods should only be called on the - // main thread (the thread on which the wrapper is constructed). - void WaitForAllDatabasesToClose(); bool HasOpenConnections(); - - // Add and Remove may be called on any thread. void AddOpenConnection(const std::string& origin_identifier, const base::string16& database_name); void RemoveOpenConnection(const std::string& origin_identifier, const base::string16& database_name); + + // Returns true if all databases are closed. + bool WaitForAllDatabasesToClose(base::TimeDelta timeout); + private: ~DatabaseConnectionsWrapper(); friend class base::RefCountedThreadSafe<DatabaseConnectionsWrapper>; - bool waiting_for_dbs_to_close_; base::Lock open_connections_lock_; DatabaseConnections open_connections_; - scoped_refptr<base::SingleThreadTaskRunner> main_thread_; + base::WaitableEvent* waiting_to_close_event_ = nullptr; }; } // namespace storage diff --git a/third_party/WebKit/Source/modules/webdatabase/DatabaseTracker.cpp b/third_party/WebKit/Source/modules/webdatabase/DatabaseTracker.cpp index 8bb0199..e6833f6 100644 --- a/third_party/WebKit/Source/modules/webdatabase/DatabaseTracker.cpp +++ b/third_party/WebKit/Source/modules/webdatabase/DatabaseTracker.cpp @@ -106,60 +106,36 @@ void DatabaseTracker::addOpenDatabase(Database* database) databaseSet->add(database); } -class NotifyDatabaseObserverOnCloseTask final : public ExecutionContextTask { -public: - static PassOwnPtr<NotifyDatabaseObserverOnCloseTask> create(Database* database) - { - return adoptPtr(new NotifyDatabaseObserverOnCloseTask(database)); - } - - void performTask(ExecutionContext*) override - { - databaseClosed(m_database.get()); - } - -private: - explicit NotifyDatabaseObserverOnCloseTask(Database* database) - : m_database(database) - { - } - - CrossThreadPersistent<Database> m_database; -}; - void DatabaseTracker::removeOpenDatabase(Database* database) { - String originIdentifier = createDatabaseIdentifierFromSecurityOrigin(database->securityOrigin()); - MutexLocker openDatabaseMapLock(m_openDatabaseMapGuard); - ASSERT(m_openDatabaseMap); - DatabaseNameMap* nameMap = m_openDatabaseMap->get(originIdentifier); - if (!nameMap) - return; + { + MutexLocker openDatabaseMapLock(m_openDatabaseMapGuard); + String originIdentifier = createDatabaseIdentifierFromSecurityOrigin(database->securityOrigin()); + ASSERT(m_openDatabaseMap); + DatabaseNameMap* nameMap = m_openDatabaseMap->get(originIdentifier); + if (!nameMap) + return; - String name(database->stringIdentifier()); - DatabaseSet* databaseSet = nameMap->get(name); - if (!databaseSet) - return; + String name(database->stringIdentifier()); + DatabaseSet* databaseSet = nameMap->get(name); + if (!databaseSet) + return; - DatabaseSet::iterator found = databaseSet->find(database); - if (found == databaseSet->end()) - return; + DatabaseSet::iterator found = databaseSet->find(database); + if (found == databaseSet->end()) + return; - databaseSet->remove(found); - if (databaseSet->isEmpty()) { - nameMap->remove(name); - delete databaseSet; - if (nameMap->isEmpty()) { - m_openDatabaseMap->remove(originIdentifier); - delete nameMap; + databaseSet->remove(found); + if (databaseSet->isEmpty()) { + nameMap->remove(name); + delete databaseSet; + if (nameMap->isEmpty()) { + m_openDatabaseMap->remove(originIdentifier); + delete nameMap; + } } } - - ExecutionContext* executionContext = database->databaseContext()->executionContext(); - if (!executionContext->isContextThread()) - executionContext->postTask(BLINK_FROM_HERE, NotifyDatabaseObserverOnCloseTask::create(database)); - else - databaseClosed(database); + databaseClosed(database); } void DatabaseTracker::prepareToOpenDatabase(Database* database) @@ -176,11 +152,7 @@ void DatabaseTracker::prepareToOpenDatabase(Database* database) void DatabaseTracker::failedToOpenDatabase(Database* database) { - ExecutionContext* executionContext = database->databaseContext()->executionContext(); - if (!executionContext->isContextThread()) - executionContext->postTask(BLINK_FROM_HERE, NotifyDatabaseObserverOnCloseTask::create(database)); - else - databaseClosed(database); + databaseClosed(database); } unsigned long long DatabaseTracker::getMaxSizeForDatabase(const Database* database) |