From e65335523f6d75123c62110dd109b6e54fc870f5 Mon Sep 17 00:00:00 2001 From: jsbell Date: Tue, 18 Aug 2015 10:20:02 -0700 Subject: [PATCH] Indexed DB: Disable transaction inactivity timeout in browser tests To work around wedged renderers stalling the transaction queue, we impose a timeout. Once a transaction has kicked off, if no new requests are posted within 60s we assume the renderer is stalled but not terminated, and abort the transaction. It appears we're hitting this when browser tests are run under MSAN, Dr. Memory, and so on. Override the default timeout in browser tests to prevent this. BUG=505609 R=cmumford@chromium.org Review URL: https://codereview.chromium.org/1285303002 Cr-Commit-Position: refs/heads/master@{#343948} --- .../browser/indexed_db/indexed_db_class_factory.cc | 12 +++++++ .../browser/indexed_db/indexed_db_class_factory.h | 17 +++++++++- content/browser/indexed_db/indexed_db_database.cc | 12 +++---- .../indexed_db/indexed_db_database_unittest.cc | 10 +++--- .../browser/indexed_db/indexed_db_transaction.cc | 10 +++--- .../browser/indexed_db/indexed_db_transaction.h | 32 +++++++++++------- .../mock_browsertest_indexed_db_class_factory.cc | 39 ++++++++++++++++++++++ .../mock_browsertest_indexed_db_class_factory.h | 11 ++++++ 8 files changed, 113 insertions(+), 30 deletions(-) diff --git a/content/browser/indexed_db/indexed_db_class_factory.cc b/content/browser/indexed_db/indexed_db_class_factory.cc index 78769db3092d..284b40cd7881 100644 --- a/content/browser/indexed_db/indexed_db_class_factory.cc +++ b/content/browser/indexed_db/indexed_db_class_factory.cc @@ -3,6 +3,7 @@ // found in the LICENSE file. #include "content/browser/indexed_db/indexed_db_class_factory.h" +#include "content/browser/indexed_db/indexed_db_transaction.h" #include "content/browser/indexed_db/leveldb/leveldb_iterator_impl.h" #include "content/browser/indexed_db/leveldb/leveldb_transaction.h" @@ -23,6 +24,17 @@ IndexedDBClassFactory* IndexedDBClassFactory::Get() { return s_factory.Pointer(); } +IndexedDBTransaction* IndexedDBClassFactory::CreateIndexedDBTransaction( + int64 id, + scoped_refptr callbacks, + const std::set& scope, + blink::WebIDBTransactionMode mode, + IndexedDBDatabase* db, + IndexedDBBackingStore::Transaction* backing_store_transaction) { + return new IndexedDBTransaction(id, callbacks, scope, mode, db, + backing_store_transaction); +} + LevelDBTransaction* IndexedDBClassFactory::CreateLevelDBTransaction( LevelDBDatabase* db) { return new LevelDBTransaction(db); diff --git a/content/browser/indexed_db/indexed_db_class_factory.h b/content/browser/indexed_db/indexed_db_class_factory.h index 03b03597f675..2fe00894f0a2 100644 --- a/content/browser/indexed_db/indexed_db_class_factory.h +++ b/content/browser/indexed_db/indexed_db_class_factory.h @@ -5,9 +5,13 @@ #ifndef CONTENT_BROWSER_INDEXED_DB_INDEXED_DB_CLASS_FACTORY_H_ #define CONTENT_BROWSER_INDEXED_DB_INDEXED_DB_CLASS_FACTORY_H_ +#include + #include "base/lazy_instance.h" #include "base/memory/scoped_ptr.h" +#include "content/browser/indexed_db/indexed_db_backing_store.h" #include "content/common/content_export.h" +#include "third_party/WebKit/public/platform/modules/indexeddb/WebIDBTypes.h" namespace leveldb { class Iterator; @@ -15,8 +19,11 @@ class Iterator; namespace content { -class LevelDBIteratorImpl; +class IndexedDBDatabase; +class IndexedDBDatabaseCallbacks; +class IndexedDBTransaction; class LevelDBDatabase; +class LevelDBIteratorImpl; class LevelDBTransaction; // Use this factory to create some IndexedDB objects. Exists solely to @@ -29,6 +36,14 @@ class CONTENT_EXPORT IndexedDBClassFactory { static void SetIndexedDBClassFactoryGetter(GetterCallback* cb); + virtual IndexedDBTransaction* CreateIndexedDBTransaction( + int64 id, + scoped_refptr callbacks, + const std::set& scope, + blink::WebIDBTransactionMode mode, + IndexedDBDatabase* db, + IndexedDBBackingStore::Transaction* backing_store_transaction); + virtual LevelDBIteratorImpl* CreateIteratorImpl( scoped_ptr iterator); virtual LevelDBTransaction* CreateLevelDBTransaction(LevelDBDatabase* db); diff --git a/content/browser/indexed_db/indexed_db_database.cc b/content/browser/indexed_db/indexed_db_database.cc index 4b50a9e32905..8628768d91f5 100644 --- a/content/browser/indexed_db/indexed_db_database.cc +++ b/content/browser/indexed_db/indexed_db_database.cc @@ -17,6 +17,7 @@ #include "base/strings/string_number_conversions.h" #include "base/strings/utf_string_conversions.h" #include "content/browser/indexed_db/indexed_db_blob_info.h" +#include "content/browser/indexed_db/indexed_db_class_factory.h" #include "content/browser/indexed_db/indexed_db_connection.h" #include "content/browser/indexed_db/indexed_db_context_impl.h" #include "content/browser/indexed_db/indexed_db_cursor.h" @@ -1664,13 +1665,10 @@ void IndexedDBDatabase::CreateTransaction( // The transaction will add itself to this database's coordinator, which // manages the lifetime of the object. - TransactionCreated(new IndexedDBTransaction( - transaction_id, - connection->callbacks(), - std::set(object_store_ids.begin(), object_store_ids.end()), - mode, - this, - new IndexedDBBackingStore::Transaction(backing_store_.get()))); + TransactionCreated(IndexedDBClassFactory::Get()->CreateIndexedDBTransaction( + transaction_id, connection->callbacks(), + std::set(object_store_ids.begin(), object_store_ids.end()), mode, + this, new IndexedDBBackingStore::Transaction(backing_store_.get()))); } void IndexedDBDatabase::TransactionCreated(IndexedDBTransaction* transaction) { diff --git a/content/browser/indexed_db/indexed_db_database_unittest.cc b/content/browser/indexed_db/indexed_db_database_unittest.cc index 865086a9b27a..eb5df982118a 100644 --- a/content/browser/indexed_db/indexed_db_database_unittest.cc +++ b/content/browser/indexed_db/indexed_db_database_unittest.cc @@ -14,6 +14,7 @@ #include "content/browser/indexed_db/indexed_db.h" #include "content/browser/indexed_db/indexed_db_backing_store.h" #include "content/browser/indexed_db/indexed_db_callbacks.h" +#include "content/browser/indexed_db/indexed_db_class_factory.h" #include "content/browser/indexed_db/indexed_db_connection.h" #include "content/browser/indexed_db/indexed_db_cursor.h" #include "content/browser/indexed_db/indexed_db_fake_backing_store.h" @@ -250,12 +251,9 @@ class IndexedDBDatabaseOperationTest : public testing::Test { EXPECT_EQ(IndexedDBDatabaseMetadata::NO_INT_VERSION, db_->metadata().int_version); - transaction_ = new IndexedDBTransaction( - transaction_id, - callbacks_, - std::set() /*scope*/, - blink::WebIDBTransactionModeVersionChange, - db_.get(), + transaction_ = IndexedDBClassFactory::Get()->CreateIndexedDBTransaction( + transaction_id, callbacks_, std::set() /*scope*/, + blink::WebIDBTransactionModeVersionChange, db_.get(), new IndexedDBFakeBackingStore::FakeTransaction(commit_success_)); db_->TransactionCreated(transaction_.get()); diff --git a/content/browser/indexed_db/indexed_db_transaction.cc b/content/browser/indexed_db/indexed_db_transaction.cc index 926799971042..7e4a9f38c7d3 100644 --- a/content/browser/indexed_db/indexed_db_transaction.cc +++ b/content/browser/indexed_db/indexed_db_transaction.cc @@ -393,13 +393,15 @@ void IndexedDBTransaction::ProcessTaskQueue() { // never requests further activity. Read-only transactions don't // block other transactions, so don't time those out. if (mode_ != blink::WebIDBTransactionModeReadOnly) { - timeout_timer_.Start( - FROM_HERE, - base::TimeDelta::FromSeconds(kInactivityTimeoutPeriodSeconds), - base::Bind(&IndexedDBTransaction::Timeout, this)); + timeout_timer_.Start(FROM_HERE, GetInactivityTimeout(), + base::Bind(&IndexedDBTransaction::Timeout, this)); } } +base::TimeDelta IndexedDBTransaction::GetInactivityTimeout() const { + return base::TimeDelta::FromSeconds(kInactivityTimeoutPeriodSeconds); +} + void IndexedDBTransaction::Timeout() { Abort(IndexedDBDatabaseError( blink::WebIDBDatabaseExceptionTimeoutError, diff --git a/content/browser/indexed_db/indexed_db_transaction.h b/content/browser/indexed_db/indexed_db_transaction.h index c9706dddefa5..5bec07b00f35 100644 --- a/content/browser/indexed_db/indexed_db_transaction.h +++ b/content/browser/indexed_db/indexed_db_transaction.h @@ -38,14 +38,6 @@ class CONTENT_EXPORT IndexedDBTransaction FINISHED, // Either aborted or committed. }; - IndexedDBTransaction( - int64 id, - scoped_refptr callbacks, - const std::set& object_store_ids, - blink::WebIDBTransactionMode, - IndexedDBDatabase* db, - IndexedDBBackingStore::Transaction* backing_store_transaction); - virtual void Abort(); leveldb::Status Commit(); void Abort(const IndexedDBDatabaseError& error); @@ -88,18 +80,34 @@ class CONTENT_EXPORT IndexedDBTransaction const Diagnostics& diagnostics() const { return diagnostics_; } + protected: + // Test classes may derive, but most creation should be done via + // IndexedDBClassFactory. + IndexedDBTransaction( + int64 id, + scoped_refptr callbacks, + const std::set& object_store_ids, + blink::WebIDBTransactionMode mode, + IndexedDBDatabase* db, + IndexedDBBackingStore::Transaction* backing_store_transaction); + virtual ~IndexedDBTransaction(); + + // May be overridden in tests. + virtual base::TimeDelta GetInactivityTimeout() const; + private: friend class BlobWriteCallbackImpl; + friend class IndexedDBClassFactory; + friend class base::RefCounted; FRIEND_TEST_ALL_PREFIXES(IndexedDBTransactionTestMode, AbortPreemptive); - FRIEND_TEST_ALL_PREFIXES(IndexedDBTransactionTest, Timeout); + FRIEND_TEST_ALL_PREFIXES(IndexedDBTransactionTestMode, AbortTasks); + FRIEND_TEST_ALL_PREFIXES(IndexedDBTransactionTest, NoTimeoutReadOnly); FRIEND_TEST_ALL_PREFIXES(IndexedDBTransactionTest, SchedulePreemptiveTask); FRIEND_TEST_ALL_PREFIXES(IndexedDBTransactionTestMode, ScheduleNormalTask); - - friend class base::RefCounted; - virtual ~IndexedDBTransaction(); + FRIEND_TEST_ALL_PREFIXES(IndexedDBTransactionTest, Timeout); void RunTasksIfStarted(); diff --git a/content/browser/indexed_db/mock_browsertest_indexed_db_class_factory.cc b/content/browser/indexed_db/mock_browsertest_indexed_db_class_factory.cc index 5cade12b5f8d..4349ee240c12 100644 --- a/content/browser/indexed_db/mock_browsertest_indexed_db_class_factory.cc +++ b/content/browser/indexed_db/mock_browsertest_indexed_db_class_factory.cc @@ -5,6 +5,7 @@ #include #include "base/logging.h" +#include "content/browser/indexed_db/indexed_db_transaction.h" #include "content/browser/indexed_db/leveldb/leveldb_iterator_impl.h" #include "content/browser/indexed_db/leveldb/leveldb_transaction.h" #include "content/browser/indexed_db/mock_browsertest_indexed_db_class_factory.h" @@ -40,6 +41,32 @@ class FunctionTracer { namespace content { +class IndexedDBTestTransaction : public IndexedDBTransaction { + public: + IndexedDBTestTransaction( + int64 id, + scoped_refptr callbacks, + const std::set& scope, + blink::WebIDBTransactionMode mode, + IndexedDBDatabase* db, + IndexedDBBackingStore::Transaction* backing_store_transaction) + : IndexedDBTransaction(id, + callbacks, + scope, + mode, + db, + backing_store_transaction) {} + + protected: + ~IndexedDBTestTransaction() override {} + + // Browser tests run under memory/address sanitizers (etc) may trip the + // default 60s timeout, so relax it during tests. + base::TimeDelta GetInactivityTimeout() const override { + return base::TimeDelta::FromSeconds(60 * 60); + } +}; + class LevelDBTestTransaction : public LevelDBTransaction { public: LevelDBTestTransaction(LevelDBDatabase* db, @@ -207,6 +234,18 @@ MockBrowserTestIndexedDBClassFactory::MockBrowserTestIndexedDBClassFactory() MockBrowserTestIndexedDBClassFactory::~MockBrowserTestIndexedDBClassFactory() { } +IndexedDBTransaction* +MockBrowserTestIndexedDBClassFactory::CreateIndexedDBTransaction( + int64 id, + scoped_refptr callbacks, + const std::set& scope, + blink::WebIDBTransactionMode mode, + IndexedDBDatabase* db, + IndexedDBBackingStore::Transaction* backing_store_transaction) { + return new IndexedDBTestTransaction(id, callbacks, scope, mode, db, + backing_store_transaction); +} + LevelDBTransaction* MockBrowserTestIndexedDBClassFactory::CreateLevelDBTransaction( LevelDBDatabase* db) { diff --git a/content/browser/indexed_db/mock_browsertest_indexed_db_class_factory.h b/content/browser/indexed_db/mock_browsertest_indexed_db_class_factory.h index 6e6f6ae0cef7..1ce5c0eb9555 100644 --- a/content/browser/indexed_db/mock_browsertest_indexed_db_class_factory.h +++ b/content/browser/indexed_db/mock_browsertest_indexed_db_class_factory.h @@ -6,8 +6,12 @@ #define CONTENT_BROWSER_INDEXED_DB_MOCK_BROWSERTEST_INDEXED_DB_CLASS_FACTORY_H_ #include +#include +#include "base/memory/scoped_ptr.h" +#include "content/browser/indexed_db/indexed_db_backing_store.h" #include "content/browser/indexed_db/indexed_db_class_factory.h" +#include "third_party/WebKit/public/platform/modules/indexeddb/WebIDBTypes.h" namespace content { @@ -32,6 +36,13 @@ class MockBrowserTestIndexedDBClassFactory : public IndexedDBClassFactory { public: MockBrowserTestIndexedDBClassFactory(); ~MockBrowserTestIndexedDBClassFactory() override; + IndexedDBTransaction* CreateIndexedDBTransaction( + int64 id, + scoped_refptr callbacks, + const std::set& scope, + blink::WebIDBTransactionMode mode, + IndexedDBDatabase* db, + IndexedDBBackingStore::Transaction* backing_store_transaction) override; LevelDBTransaction* CreateLevelDBTransaction(LevelDBDatabase* db) override; LevelDBIteratorImpl* CreateIteratorImpl( scoped_ptr iterator) override; -- 2.11.4.GIT