Indexed DB: Prevent invalid transaction timeouts

A transaction watchdog timer intended to guard against hung renderers
would be scheduled even if the transaction backend still had tasks to
perform, which could be blocked on other backend work. The watchdog
timer should only be started if the backend is only waiting on the
front end. Add the extra condition.

Bug: 1045585
Change-Id: I86e978faa9735e1b7a89a10660f1c747237b3c9c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2026109
Reviewed-by: Daniel Murphy <dmurph@chromium.org>
Commit-Queue: Joshua Bell <jsbell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#736211}

content/browser/indexed_db/indexed_db_transaction.cc

@@ -514,7 +514,8 @@ IndexedDBTransaction::RunTasks() {
   // Otherwise, start a timer in case the front-end gets wedged and
   // never requests further activity. Read-only transactions don't
   // block other transactions, so don't time those out.
-  if (mode_ != blink::mojom::IDBTransactionMode::ReadOnly &&
+  if (!HasPendingTasks() &&
+      mode_ != blink::mojom::IDBTransactionMode::ReadOnly &&
       state_ == STARTED) {
     timeout_timer_.Start(FROM_HERE, GetInactivityTimeout(),
                          base::BindOnce(&IndexedDBTransaction::Timeout,

content/browser/indexed_db/indexed_db_transaction.h

@@ -46,6 +46,7 @@ FORWARD_DECLARE_TEST(IndexedDBTransactionTest, SchedulePreemptiveTask);
 FORWARD_DECLARE_TEST(IndexedDBTransactionTestMode, ScheduleNormalTask);
 FORWARD_DECLARE_TEST(IndexedDBTransactionTestMode, TaskFails);
 FORWARD_DECLARE_TEST(IndexedDBTransactionTest, Timeout);
+FORWARD_DECLARE_TEST(IndexedDBTransactionTest, TimeoutPreemptive);
 FORWARD_DECLARE_TEST(IndexedDBTransactionTest, IndexedDBObserver);
 }  // namespace indexed_db_transaction_unittest
 
@@ -196,6 +197,9 @@ class CONTENT_EXPORT IndexedDBTransaction {
   FRIEND_TEST_ALL_PREFIXES(
       indexed_db_transaction_unittest::IndexedDBTransactionTest,
       Timeout);
+  FRIEND_TEST_ALL_PREFIXES(
+      indexed_db_transaction_unittest::IndexedDBTransactionTest,
+      TimeoutPreemptive);
   FRIEND_TEST_ALL_PREFIXES(
       indexed_db_transaction_unittest::IndexedDBTransactionTest,
       IndexedDBObserver);

content/browser/indexed_db/indexed_db_transaction_unittest.cc

@@ -198,6 +198,69 @@ TEST_F(IndexedDBTransactionTest, Timeout) {
   EXPECT_EQ(1, transaction->diagnostics().tasks_completed);
 }
 
+TEST_F(IndexedDBTransactionTest, TimeoutPreemptive) {
+  const int64_t id = 0;
+  const std::set<int64_t> scope;
+  const leveldb::Status commit_success = leveldb::Status::OK();
+  std::unique_ptr<IndexedDBConnection> connection = CreateConnection();
+  IndexedDBTransaction* transaction = connection->CreateTransaction(
+      id, scope, blink::mojom::IDBTransactionMode::ReadWrite,
+      new IndexedDBFakeBackingStore::FakeTransaction(commit_success));
+  db_->RegisterAndScheduleTransaction(transaction);
+
+  // No conflicting transactions, so coordinator will start it immediately:
+  EXPECT_EQ(IndexedDBTransaction::STARTED, transaction->state());
+  EXPECT_FALSE(transaction->IsTimeoutTimerRunning());
+  EXPECT_EQ(0, transaction->diagnostics().tasks_scheduled);
+  EXPECT_EQ(0, transaction->diagnostics().tasks_completed);
+
+  // Add a preemptive task.
+  transaction->ScheduleTask(
+      blink::mojom::IDBTaskType::Preemptive,
+      base::BindOnce(&IndexedDBTransactionTest::DummyOperation,
+                     base::Unretained(this), leveldb::Status::OK()));
+  transaction->AddPreemptiveEvent();
+
+  EXPECT_TRUE(transaction->HasPendingTasks());
+  EXPECT_FALSE(transaction->IsTimeoutTimerRunning());
+  EXPECT_TRUE(transaction->task_queue_.empty());
+  EXPECT_FALSE(transaction->preemptive_task_queue_.empty());
+
+  // Pump the message loop so that the transaction completes all pending tasks,
+  // otherwise it will defer the commit.
+  RunPostedTasks();
+  EXPECT_TRUE(transaction->HasPendingTasks());
+  EXPECT_FALSE(transaction->IsTimeoutTimerRunning());
+  EXPECT_TRUE(transaction->task_queue_.empty());
+  EXPECT_TRUE(transaction->preemptive_task_queue_.empty());
+
+  // Schedule a task - timer won't be started until preemptive tasks are done.
+  transaction->ScheduleTask(
+      base::BindOnce(&IndexedDBTransactionTest::DummyOperation,
+                     base::Unretained(this), leveldb::Status::OK()));
+  EXPECT_FALSE(transaction->IsTimeoutTimerRunning());
+  EXPECT_EQ(1, transaction->diagnostics().tasks_scheduled);
+  EXPECT_EQ(0, transaction->diagnostics().tasks_completed);
+
+  // This shouldn't do anything - the preemptive task is still lurking.
+  RunPostedTasks();
+  EXPECT_TRUE(transaction->HasPendingTasks());
+  EXPECT_FALSE(transaction->IsTimeoutTimerRunning());
+  EXPECT_EQ(1, transaction->diagnostics().tasks_scheduled);
+  EXPECT_EQ(0, transaction->diagnostics().tasks_completed);
+
+  // Finish the preemptive task, which unblocks regular tasks.
+  transaction->DidCompletePreemptiveEvent();
+  // TODO(dmurph): Should this explicit call be necessary?
+  transaction->RunTasks();
+
+  // The task's completion should start the timer.
+  EXPECT_FALSE(transaction->HasPendingTasks());
+  EXPECT_TRUE(transaction->IsTimeoutTimerRunning());
+  EXPECT_EQ(1, transaction->diagnostics().tasks_scheduled);
+  EXPECT_EQ(1, transaction->diagnostics().tasks_completed);
+}
+
 TEST_F(IndexedDBTransactionTest, NoTimeoutReadOnly) {
   const int64_t id = 0;
   const std::set<int64_t> scope;