diff options
Diffstat (limited to 'chromium/components/webdata')
9 files changed, 112 insertions, 90 deletions
diff --git a/chromium/components/webdata/common/BUILD.gn b/chromium/components/webdata/common/BUILD.gn index 7de1b1cd976..1fe078139fd 100644 --- a/chromium/components/webdata/common/BUILD.gn +++ b/chromium/components/webdata/common/BUILD.gn @@ -57,6 +57,7 @@ bundle_data("unit_tests_bundle_data") { "//components/test/data/web_database/version_68.sql", "//components/test/data/web_database/version_69.sql", "//components/test/data/web_database/version_70.sql", + "//components/test/data/web_database/version_71.sql", ] outputs = [ "{{bundle_resources_dir}}/" + diff --git a/chromium/components/webdata/common/web_data_request_manager.cc b/chromium/components/webdata/common/web_data_request_manager.cc index 3615379a758..c52cfd82c53 100644 --- a/chromium/components/webdata/common/web_data_request_manager.cc +++ b/chromium/components/webdata/common/web_data_request_manager.cc @@ -8,6 +8,7 @@ #include "base/bind.h" #include "base/location.h" +#include "base/memory/ptr_util.h" #include "base/profiler/scoped_tracker.h" #include "base/stl_util.h" #include "base/threading/thread_task_runner_handle.h" @@ -19,50 +20,47 @@ //////////////////////////////////////////////////////////////////////////////// WebDataRequest::~WebDataRequest() { - if (IsActive()) { - manager_->CancelRequest(handle_); - } + WebDataRequestManager* manager = GetManager(); + if (manager) + manager->CancelRequest(handle_); } WebDataServiceBase::Handle WebDataRequest::GetHandle() const { return handle_; } -bool WebDataRequest::IsActive() const { - return manager_ != nullptr; +bool WebDataRequest::IsActive() { + return GetManager() != nullptr; } WebDataRequest::WebDataRequest(WebDataRequestManager* manager, - WebDataServiceConsumer* consumer) + WebDataServiceConsumer* consumer, + WebDataServiceBase::Handle handle) : task_runner_(base::ThreadTaskRunnerHandle::Get()), - manager_(manager), + atomic_manager_(reinterpret_cast<base::subtle::AtomicWord>(manager)), consumer_(consumer), - handle_(0) { + handle_(handle) { + DCHECK(task_runner_); DCHECK(IsActive()); + static_assert(sizeof(atomic_manager_) == sizeof(manager), "size mismatch"); } -void WebDataRequest::AssertThreadSafe() const { - // Manipulations of the request state should only happen when the request is - // active and the manager's lock is held (i.e., during request cancellation - // or completion). - DCHECK(IsActive()); - manager_->AssertLockedByCurrentThread(); +WebDataRequestManager* WebDataRequest::GetManager() { + return reinterpret_cast<WebDataRequestManager*>( + base::subtle::Acquire_Load(&atomic_manager_)); } -WebDataServiceConsumer* WebDataRequest::GetConsumer() const { - AssertThreadSafe(); +WebDataServiceConsumer* WebDataRequest::GetConsumer() { return consumer_; } -scoped_refptr<base::SingleThreadTaskRunner> WebDataRequest::GetTaskRunner() - const { +scoped_refptr<base::SingleThreadTaskRunner> WebDataRequest::GetTaskRunner() { return task_runner_; } void WebDataRequest::MarkAsInactive() { - AssertThreadSafe(); - consumer_ = nullptr; - manager_ = nullptr; + // Set atomic_manager_ to the equivalent of nullptr; + base::subtle::Release_Store(&atomic_manager_, 0); } //////////////////////////////////////////////////////////////////////////////// @@ -77,13 +75,13 @@ WebDataRequestManager::WebDataRequestManager() std::unique_ptr<WebDataRequest> WebDataRequestManager::NewRequest( WebDataServiceConsumer* consumer) { - std::unique_ptr<WebDataRequest> request(new WebDataRequest(this, consumer)); - - // Grab the lock to get the next request handle and register the request. base::AutoLock l(pending_lock_); - request->handle_ = next_request_handle_++; - pending_requests_[request->handle_] = request.get(); - + std::unique_ptr<WebDataRequest> request = base::WrapUnique( + new WebDataRequest(this, consumer, next_request_handle_)); + bool inserted = + pending_requests_.emplace(next_request_handle_, request.get()).second; + DCHECK(inserted); + ++next_request_handle_; return request; } @@ -105,12 +103,8 @@ void WebDataRequestManager::RequestCompleted( request->GetTaskRunner(); task_runner->PostTask( FROM_HERE, - base::Bind(&WebDataRequestManager::RequestCompletedOnThread, this, - base::Passed(&request), base::Passed(&result))); -} - -void WebDataRequestManager::AssertLockedByCurrentThread() const { - pending_lock_.AssertAcquired(); + base::BindOnce(&WebDataRequestManager::RequestCompletedOnThread, this, + base::Passed(&request), base::Passed(&result))); } WebDataRequestManager::~WebDataRequestManager() { @@ -123,28 +117,15 @@ WebDataRequestManager::~WebDataRequestManager() { void WebDataRequestManager::RequestCompletedOnThread( std::unique_ptr<WebDataRequest> request, std::unique_ptr<WDTypedResult> result) { - // If the request is already inactive, early exit to avoid contending for the - // lock (aka, the double checked locking pattern). + // Check whether the request is active. It might have been cancelled in + // another thread before this completion handler was invoked. This means the + // request initiator is no longer interested in the result. if (!request->IsActive()) return; - // Used to export the consumer from the locked region below so that the - // consumer can be notified outside of the lock and after the request has - // been marked as inactive. - WebDataServiceConsumer* consumer; - - // Manipulate the pending_requests_ collection while holding the lock. + // Stop tracking the request. The request is already finished, so "stop + // tracking" is the same as post-facto cancellation. { - base::AutoLock l(pending_lock_); - - // Re-check whether the request is active. It might have been cancelled in - // another thread before the lock was acquired. - if (!request->IsActive()) - return; - - // Remember the consumer for notification below. - consumer = request->GetConsumer(); - // TODO(robliao): Remove ScopedTracker below once https://crbug.com/422460 // is fixed. tracked_objects::ScopedTracker tracking_profile( @@ -152,20 +133,11 @@ void WebDataRequestManager::RequestCompletedOnThread( "422460 " "WebDataRequestManager::RequestCompletedOnThread::UpdateMap")); - auto i = pending_requests_.find(request->GetHandle()); - DCHECK(i != pending_requests_.end()); - - // Take ownership of the request object and remove it from the map. - pending_requests_.erase(i); - - // The request is no longer active. - request->MarkAsInactive(); + CancelRequest(request->GetHandle()); } // Notify the consumer if needed. - // - // NOTE: The pending_lock_ is no longer held here. It's up to the consumer to - // be appropriately thread safe. + WebDataServiceConsumer* const consumer = request->GetConsumer(); if (consumer) { // TODO(robliao): Remove ScopedTracker below once https://crbug.com/422460 // is fixed. diff --git a/chromium/components/webdata/common/web_data_request_manager.h b/chromium/components/webdata/common/web_data_request_manager.h index 87b55091583..eb621a2cff1 100644 --- a/chromium/components/webdata/common/web_data_request_manager.h +++ b/chromium/components/webdata/common/web_data_request_manager.h @@ -12,6 +12,7 @@ #include <map> #include <memory> +#include "base/atomicops.h" #include "base/macros.h" #include "base/memory/ref_counted.h" #include "base/single_thread_task_runner.h" @@ -40,7 +41,7 @@ class WebDataRequest { // Returns |true| if the request is active and |false| if the request has been // cancelled or has already completed. - bool IsActive() const; + bool IsActive(); private: // For access to the web request mutable state under the manager's lock. @@ -48,17 +49,19 @@ class WebDataRequest { // Private constructor called for WebDataRequestManager::NewRequest. WebDataRequest(WebDataRequestManager* manager, - WebDataServiceConsumer* consumer); + WebDataServiceConsumer* consumer, + WebDataServiceBase::Handle handle); - // Internal debugging helper to assert that the request is active and that the - // manager's lock is held by the current thread. - void AssertThreadSafe() const; + // Retrieves the manager set in the constructor, if the request is still + // active, or nullptr if the request is inactive. The returned value may + // change between calls. + WebDataRequestManager* GetManager(); // Retrieves the |consumer_| set in the constructor. - WebDataServiceConsumer* GetConsumer() const; + WebDataServiceConsumer* GetConsumer(); // Retrieves the original task runner of the request. - scoped_refptr<base::SingleThreadTaskRunner> GetTaskRunner() const; + scoped_refptr<base::SingleThreadTaskRunner> GetTaskRunner(); // Marks the current request as inactive, either due to cancellation or // completion. @@ -67,16 +70,16 @@ class WebDataRequest { // Tracks task runner that the request originated on. const scoped_refptr<base::SingleThreadTaskRunner> task_runner_; - // Used to notify manager if request is cancelled. Uses a raw ptr instead of - // a ref_ptr so that it can be set to null when a request is cancelled or - // completed. - WebDataRequestManager* manager_; + // The manager associated with this request. This is stored as a raw (untyped) + // pointer value because it does double duty as the flag indicating whether or + // not this request is active (non-nullptr => active). + base::subtle::AtomicWord atomic_manager_; // The originator of the service request. - WebDataServiceConsumer* consumer_; + WebDataServiceConsumer* const consumer_; // Identifier for this request. - WebDataServiceBase::Handle handle_; + const WebDataServiceBase::Handle handle_; DISALLOW_COPY_AND_ASSIGN(WebDataRequest); }; @@ -104,10 +107,6 @@ class WebDataRequestManager void RequestCompleted(std::unique_ptr<WebDataRequest> request, std::unique_ptr<WDTypedResult> result); - // A debugging aid to assert that the pending_lock_ is held by the current - // thread. - void AssertLockedByCurrentThread() const; - private: friend class base::RefCountedThreadSafe<WebDataRequestManager>; diff --git a/chromium/components/webdata/common/web_data_results.h b/chromium/components/webdata/common/web_data_results.h index 1577866001a..71435f59fd8 100644 --- a/chromium/components/webdata/common/web_data_results.h +++ b/chromium/components/webdata/common/web_data_results.h @@ -34,6 +34,11 @@ typedef enum { AUTOFILL_CREDITCARD_RESULT, // WDResult<CreditCard> AUTOFILL_CREDITCARDS_RESULT, // WDResult<std::vector< // std::unique_ptr<CreditCard>>> +#if defined(OS_ANDROID) + PAYMENT_WEB_APP_MANIFEST, // WDResult<std::vector< + // mojom::WebAppManifestSectionPtr>> + PAYMENT_METHOD_MANIFEST, // WDResult<std::vector<std::string>> +#endif } WDResultType; // diff --git a/chromium/components/webdata/common/web_data_service_base.h b/chromium/components/webdata/common/web_data_service_base.h index ab9a4d42d22..0c442c698f3 100644 --- a/chromium/components/webdata/common/web_data_service_base.h +++ b/chromium/components/webdata/common/web_data_service_base.h @@ -17,7 +17,6 @@ class WebDatabase; class WebDatabaseService; namespace base { -// TODO(skyostil): Migrate to SingleThreadTaskRunner (crbug.com/465354). class SingleThreadTaskRunner; } diff --git a/chromium/components/webdata/common/web_database.cc b/chromium/components/webdata/common/web_database.cc index 1923176050d..072f025e4d6 100644 --- a/chromium/components/webdata/common/web_database.cc +++ b/chromium/components/webdata/common/web_database.cc @@ -13,13 +13,13 @@ // corresponding changes must happen in the unit tests, and new migration test // added. See |WebDatabaseMigrationTest::kCurrentTestedVersionNumber|. // static -const int WebDatabase::kCurrentVersionNumber = 71; +const int WebDatabase::kCurrentVersionNumber = 72; const int WebDatabase::kDeprecatedVersionNumber = 51; namespace { -const int kCompatibleVersionNumber = 71; +const int kCompatibleVersionNumber = 72; // Change the version number and possibly the compatibility version of // |meta_table_|. @@ -152,7 +152,6 @@ sql::InitStatus WebDatabase::MigrateOldVersionsAsNeeded() { for (int next_version = current_version + 1; next_version <= kCurrentVersionNumber; ++next_version) { - // Do any database-wide migrations. bool update_compatible_version = false; if (!MigrateToVersion(next_version, &update_compatible_version)) diff --git a/chromium/components/webdata/common/web_database.h b/chromium/components/webdata/common/web_database.h index 79931bedba7..36bae881f6f 100644 --- a/chromium/components/webdata/common/web_database.h +++ b/chromium/components/webdata/common/web_database.h @@ -6,6 +6,7 @@ #define COMPONENTS_WEBDATA_COMMON_WEB_DATABASE_H_ #include <map> +#include <string> #include "base/macros.h" #include "components/webdata/common/web_database_table.h" diff --git a/chromium/components/webdata/common/web_database_migration_unittest.cc b/chromium/components/webdata/common/web_database_migration_unittest.cc index 3229f86ea0e..4417725203b 100644 --- a/chromium/components/webdata/common/web_database_migration_unittest.cc +++ b/chromium/components/webdata/common/web_database_migration_unittest.cc @@ -130,7 +130,7 @@ class WebDatabaseMigrationTest : public testing::Test { DISALLOW_COPY_AND_ASSIGN(WebDatabaseMigrationTest); }; -const int WebDatabaseMigrationTest::kCurrentTestedVersionNumber = 71; +const int WebDatabaseMigrationTest::kCurrentTestedVersionNumber = 72; void WebDatabaseMigrationTest::LoadDatabase( const base::FilePath::StringType& file) { @@ -1229,16 +1229,61 @@ TEST_F(WebDatabaseMigrationTest, MigrateVersion70ToCurrent) { // Make sure that the values in masked_credit_cards are still present except // for the billing_address_id. The values are added to the table in // version_70.sql. - sql::Statement s_masked_cards(connection.GetUniqueStatement( - "SELECT id, status, name_on_card, type, last_four, exp_month, exp_year " - "FROM masked_credit_cards")); + sql::Statement s_masked_cards( + connection.GetUniqueStatement("SELECT id, status, name_on_card, " + "network, last_four, exp_month, exp_year " + "FROM masked_credit_cards")); ASSERT_TRUE(s_masked_cards.Step()); EXPECT_EQ("card_1", s_masked_cards.ColumnString(0)); EXPECT_EQ("status", s_masked_cards.ColumnString(1)); EXPECT_EQ("bob", s_masked_cards.ColumnString(2)); - EXPECT_EQ("MASKED", s_masked_cards.ColumnString(3)); + EXPECT_EQ("VISA", s_masked_cards.ColumnString(3)); EXPECT_EQ("1234", s_masked_cards.ColumnString(4)); EXPECT_EQ(12, s_masked_cards.ColumnInt(5)); EXPECT_EQ(2050, s_masked_cards.ColumnInt(6)); } } + +// Tests renaming "type" column into "network" for the "masked_credit_cards" +// table. +TEST_F(WebDatabaseMigrationTest, MigrateVersion71ToCurrent) { + ASSERT_NO_FATAL_FAILURE(LoadDatabase(FILE_PATH_LITERAL("version_71.sql"))); + + // Verify pre-conditions. + { + sql::Connection connection; + ASSERT_TRUE(connection.Open(GetDatabasePath())); + ASSERT_TRUE(sql::MetaTable::DoesTableExist(&connection)); + + sql::MetaTable meta_table; + ASSERT_TRUE(meta_table.Init(&connection, 71, 71)); + + EXPECT_TRUE(connection.DoesColumnExist("masked_credit_cards", "type")); + EXPECT_FALSE(connection.DoesColumnExist("masked_credit_cards", "network")); + + EXPECT_TRUE( + connection.Execute("INSERT INTO masked_credit_cards(id, type) " + "VALUES ('id', 'VISA')")); + } + + DoMigration(); + + // Verify post-conditions. + { + sql::Connection connection; + ASSERT_TRUE(connection.Open(GetDatabasePath())); + ASSERT_TRUE(sql::MetaTable::DoesTableExist(&connection)); + + // Check version. + EXPECT_EQ(kCurrentTestedVersionNumber, VersionFromConnection(&connection)); + + EXPECT_FALSE(connection.DoesColumnExist("masked_credit_cards", "type")); + EXPECT_TRUE(connection.DoesColumnExist("masked_credit_cards", "network")); + + sql::Statement s_cards_metadata(connection.GetUniqueStatement( + "SELECT id, network FROM masked_credit_cards")); + ASSERT_TRUE(s_cards_metadata.Step()); + EXPECT_EQ("id", s_cards_metadata.ColumnString(0)); + EXPECT_EQ("VISA", s_cards_metadata.ColumnString(1)); + } +} diff --git a/chromium/components/webdata/common/web_database_table.h b/chromium/components/webdata/common/web_database_table.h index ff75082ed46..45011710bfa 100644 --- a/chromium/components/webdata/common/web_database_table.h +++ b/chromium/components/webdata/common/web_database_table.h @@ -47,8 +47,9 @@ class WEBDATA_EXPORT WebDatabaseTable { // Migrates this table to |version|. Returns false if there was // migration work to do and it failed, true otherwise. // - // Implementations may set |*update_compatible_version| to true if - // the compatible version should be changed to |version|. + // Implementations may set |*update_compatible_version| to true if the + // compatible version should be changed to |version|, i.e., if the change will + // break previous versions when they try to use the updated database. // Implementations should otherwise not modify this parameter. virtual bool MigrateToVersion(int version, bool* update_compatible_version) = 0; |