WebSQL: Avoid running nested message loops during renderer shutdown.

Instead, block the main thread while waiting for databases to close. If they don't close quickly enough, use CHECK to end the process.

BUG=472618

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

Cr-Commit-Position: refs/heads/master@{#356755}

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)