Fixed an issue in ModelImpl::Update for download service.

Currently Update call will create a new unique pointer and destroy the
existing one. This will cause crash if we get the same object, change
the states and call Update again.

This CL also removes the cache in model, since we should support pass
the same object into Model::Update call.

Bug: 731994
Change-Id: I35ea73bf8b32f7f2a08d2e212dc738cfc4d155d9
Reviewed-on: https://chromium-review.googlesource.com/530484
Commit-Queue: Xing Liu <xingliu@chromium.org>
Reviewed-by: Shakti Sahu <shaktisahu@chromium.org>
Reviewed-by: David Trainor <dtrainor@chromium.org>
Cr-Commit-Position: refs/heads/master@{#478712}

components/download/internal/model.h

@@ -78,9 +78,6 @@ class Model {
   // modifications to this model.
   virtual Entry* Get(const std::string& guid) = 0;
 
-  // Returns the total number of in-memory entries with the specific |state|.
-  virtual uint32_t StateCount(Entry::State state) = 0;
-
   // Returns a temporary list of Entry objects that this Model stores.
   // IMPORTANT NOTE: Like Get(), the result of this method should be used
   // immediately and NOT stored.  The underlying data may get updated or removed

components/download/internal/model_impl.cc

@@ -32,7 +32,6 @@ void ModelImpl::Add(const Entry& entry) {
   DCHECK(store_->IsInitialized());
   DCHECK(entries_.find(entry.guid) == entries_.end());
 
-  state_counts_[entry.state]++;
   entries_.emplace(entry.guid, base::MakeUnique<Entry>(entry));
 
   store_->Update(entry, base::BindOnce(&ModelImpl::OnAddFinished,
@@ -44,9 +43,8 @@ void ModelImpl::Update(const Entry& entry) {
   DCHECK(store_->IsInitialized());
   DCHECK(entries_.find(entry.guid) != entries_.end());
 
-  state_counts_[entries_[entry.guid]->state]--;
-  state_counts_[entry.state]++;
-  entries_[entry.guid] = base::MakeUnique<Entry>(entry);
+  *entries_[entry.guid] = entry;
+
   store_->Update(entry, base::BindOnce(&ModelImpl::OnUpdateFinished,
                                        weak_ptr_factory_.GetWeakPtr(),
                                        entry.client, entry.guid));
@@ -59,7 +57,6 @@ void ModelImpl::Remove(const std::string& guid) {
   DCHECK(it != entries_.end());
 
   DownloadClient client = it->second->client;
-  state_counts_[it->second->state]--;
   entries_.erase(it);
   store_->Remove(guid,
                  base::BindOnce(&ModelImpl::OnRemoveFinished,
@@ -71,10 +68,6 @@ Entry* ModelImpl::Get(const std::string& guid) {
   return it == entries_.end() ? nullptr : it->second.get();
 }
 
-uint32_t ModelImpl::StateCount(Entry::State state) {
-  return state_counts_[state];
-}
-
 Model::EntryList ModelImpl::PeekEntries() {
   EntryList entries;
   for (const auto& it : entries_)
@@ -94,7 +87,6 @@ void ModelImpl::OnInitializedFinished(
   }
 
   for (const auto& entry : *entries) {
-    state_counts_[entry.state]++;
     entries_.emplace(entry.guid, base::MakeUnique<Entry>(entry));
   }
 
@@ -113,7 +105,6 @@ void ModelImpl::OnAddFinished(DownloadClient client,
 
   // Remove the entry from the map if the add failed.
   if (!success) {
-    state_counts_[it->second->state]--;
     entries_.erase(it);
   }
 

components/download/internal/model_impl.h

@@ -32,7 +32,6 @@ class ModelImpl : public Model {
   void Update(const Entry& entry) override;
   void Remove(const std::string& guid) override;
   Entry* Get(const std::string& guid) override;
-  uint32_t StateCount(Entry::State state) override;
   EntryList PeekEntries() override;
 
  private:
@@ -62,9 +61,6 @@ class ModelImpl : public Model {
   // entries saved in Store.
   OwnedEntryMap entries_;
 
-  // A map of Entry::State -> the total number of entries with that state.
-  std::map<Entry::State, uint32_t> state_counts_;
-
   base::WeakPtrFactory<ModelImpl> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(ModelImpl);

components/download/internal/model_impl_unittest.cc

@@ -72,7 +72,6 @@ TEST_F(DownloadServiceModelImplTest, SuccessfulInitWithEntries) {
 
   EXPECT_TRUE(test::CompareEntry(&entry1, model_->Get(entry1.guid)));
   EXPECT_TRUE(test::CompareEntry(&entry2, model_->Get(entry2.guid)));
-  EXPECT_EQ(2u, model_->StateCount(Entry::State::NEW));
 }
 
 TEST_F(DownloadServiceModelImplTest, BadInit) {
@@ -94,18 +93,15 @@ TEST_F(DownloadServiceModelImplTest, Add) {
 
   model_->Initialize(&client_);
   store_->TriggerInit(true, base::MakeUnique<std::vector<Entry>>());
-  EXPECT_EQ(0u, model_->StateCount(Entry::State::NEW));
 
   model_->Add(entry1);
   EXPECT_TRUE(test::CompareEntry(&entry1, model_->Get(entry1.guid)));
   EXPECT_TRUE(test::CompareEntry(&entry1, store_->LastUpdatedEntry()));
   store_->TriggerUpdate(true);
-  EXPECT_EQ(1u, model_->StateCount(Entry::State::NEW));
 
   model_->Add(entry2);
   EXPECT_TRUE(test::CompareEntry(&entry2, model_->Get(entry2.guid)));
   EXPECT_TRUE(test::CompareEntry(&entry2, store_->LastUpdatedEntry()));
-  EXPECT_EQ(2u, model_->StateCount(Entry::State::NEW));
 
   store_->TriggerUpdate(false);
   EXPECT_EQ(nullptr, model_->Get(entry2.guid));
@@ -125,27 +121,31 @@ TEST_F(DownloadServiceModelImplTest, Update) {
   InSequence sequence;
   EXPECT_CALL(client_, OnModelReady(true)).Times(1);
   EXPECT_CALL(client_, OnItemUpdated(true, entry1.client, entry1.guid))
-      .Times(1);
+      .Times(2);
   EXPECT_CALL(client_, OnItemUpdated(false, entry1.client, entry1.guid))
       .Times(1);
 
   model_->Initialize(&client_);
   store_->TriggerInit(true, base::MakeUnique<std::vector<Entry>>(entries));
-  EXPECT_EQ(1u, model_->StateCount(Entry::State::NEW));
+  std::vector<Entry*> entries_pointers = model_->PeekEntries();
 
+  // Update with a different object.
   model_->Update(entry2);
   EXPECT_TRUE(test::CompareEntry(&entry2, model_->Get(entry2.guid)));
   EXPECT_TRUE(test::CompareEntry(&entry2, store_->LastUpdatedEntry()));
   store_->TriggerUpdate(true);
-  EXPECT_EQ(0u, model_->StateCount(Entry::State::NEW));
-  EXPECT_EQ(1u, model_->StateCount(Entry::State::AVAILABLE));
+
+  // Update with the same object.
+  Entry* entry = model_->Get(entry1.guid);
+  entry->state = Entry::State::NEW;
+  model_->Update(*entry);
+  store_->TriggerUpdate(true);
+  // Peek entries should return the same set of pointers.
+  EXPECT_TRUE(test::CompareEntryList(entries_pointers, model_->PeekEntries()));
 
   model_->Update(entry3);
   EXPECT_TRUE(test::CompareEntry(&entry3, model_->Get(entry3.guid)));
   EXPECT_TRUE(test::CompareEntry(&entry3, store_->LastUpdatedEntry()));
-  EXPECT_EQ(0u, model_->StateCount(Entry::State::NEW));
-  EXPECT_EQ(0u, model_->StateCount(Entry::State::AVAILABLE));
-  EXPECT_EQ(1u, model_->StateCount(Entry::State::ACTIVE));
 
   store_->TriggerUpdate(false);
   EXPECT_TRUE(test::CompareEntry(&entry3, model_->Get(entry3.guid)));
@@ -165,19 +165,16 @@ TEST_F(DownloadServiceModelImplTest, Remove) {
 
   model_->Initialize(&client_);
   store_->TriggerInit(true, base::MakeUnique<std::vector<Entry>>(entries));
-  EXPECT_EQ(2u, model_->StateCount(Entry::State::NEW));
 
   model_->Remove(entry1.guid);
   EXPECT_EQ(entry1.guid, store_->LastRemovedEntry());
   EXPECT_EQ(nullptr, model_->Get(entry1.guid));
   store_->TriggerRemove(true);
-  EXPECT_EQ(1u, model_->StateCount(Entry::State::NEW));
 
   model_->Remove(entry2.guid);
   EXPECT_EQ(entry2.guid, store_->LastRemovedEntry());
   EXPECT_EQ(nullptr, model_->Get(entry2.guid));
   store_->TriggerRemove(false);
-  EXPECT_EQ(0u, model_->StateCount(Entry::State::NEW));
 }
 
 TEST_F(DownloadServiceModelImplTest, Get) {
@@ -221,15 +218,12 @@ TEST_F(DownloadServiceModelImplTest, TestRemoveAfterAdd) {
 
   model_->Initialize(&client_);
   store_->TriggerInit(true, base::MakeUnique<std::vector<Entry>>());
-  EXPECT_EQ(0u, model_->StateCount(Entry::State::NEW));
 
   model_->Add(entry);
   EXPECT_TRUE(test::CompareEntry(&entry, model_->Get(entry.guid)));
-  EXPECT_EQ(1u, model_->StateCount(Entry::State::NEW));
 
   model_->Remove(entry.guid);
   EXPECT_EQ(nullptr, model_->Get(entry.guid));
-  EXPECT_EQ(0u, model_->StateCount(Entry::State::NEW));
 
   store_->TriggerUpdate(true);
   store_->TriggerRemove(true);
@@ -252,17 +246,12 @@ TEST_F(DownloadServiceModelImplTest, TestRemoveAfterUpdate) {
   model_->Initialize(&client_);
   store_->TriggerInit(true, base::MakeUnique<std::vector<Entry>>(entries));
   EXPECT_TRUE(test::CompareEntry(&entry1, model_->Get(entry1.guid)));
-  EXPECT_EQ(1u, model_->StateCount(Entry::State::NEW));
 
   model_->Update(entry2);
   EXPECT_TRUE(test::CompareEntry(&entry2, model_->Get(entry2.guid)));
-  EXPECT_EQ(1u, model_->StateCount(Entry::State::AVAILABLE));
-  EXPECT_EQ(0u, model_->StateCount(Entry::State::NEW));
 
   model_->Remove(entry2.guid);
   EXPECT_EQ(nullptr, model_->Get(entry2.guid));
-  EXPECT_EQ(0u, model_->StateCount(Entry::State::AVAILABLE));
-  EXPECT_EQ(0u, model_->StateCount(Entry::State::NEW));
 
   store_->TriggerUpdate(true);
   store_->TriggerRemove(true);