[Offline pages] Fixing crashes caused by access to offline pages marked for deletion

components/offline_pages/offline_page_model.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/offline_pages/offline_page_model.h"
 
 #include <algorithm>
 #include <utility>
 
 #include "base/bind.h"
@@ skipping 235 matching lines @@
     bookmark_ids.push_back(id_page_pair.first);
   DeletePagesByBookmarkId(
       bookmark_ids,
       base::Bind(&OfflinePageModel::OnRemoveAllFilesDoneForClearAll,
                  weak_ptr_factory_.GetWeakPtr(),
                  callback));
 }
 
 bool OfflinePageModel::HasOfflinePages() const {
   DCHECK(is_loaded_);
-  return !offline_pages_.empty();
+  // Check that at least one page is not marked for deletion. Because we have
+  // pages marked for deletion, we cannot simply invert result of |empty()|.
+  for (const auto& iter : offline_pages_) {

[Michael Courage, 2015/11/25 19:00:58]

optional nit: seems like there's a pretty even split between "iter" and
"id_page_pair" in this file, but the other nearby instances of "for (iterator :
offline_pages_)" go with the latter, which seems a little bit clearer to me.

[fgorski, 2015/11/25 19:27:26]

Done.

+    if (!iter.second.IsMarkedForDeletion())
+      return true;
+  }
+  return false;
 }
 
 const std::vector<OfflinePageItem> OfflinePageModel::GetAllPages() const {
   DCHECK(is_loaded_);
   std::vector<OfflinePageItem> offline_pages;
   for (const auto& id_page_pair : offline_pages_) {
     if (id_page_pair.second.IsMarkedForDeletion())
       continue;
     offline_pages.push_back(id_page_pair.second);
   }
   return offline_pages;
 }
 
 const std::vector<OfflinePageItem> OfflinePageModel::GetPagesToCleanUp() const {
   DCHECK(is_loaded_);
   std::vector<OfflinePageItem> offline_pages;
   base::Time now = base::Time::Now();
   for (const auto& id_page_pair : offline_pages_) {
     if (!id_page_pair.second.IsMarkedForDeletion() &&
         now - id_page_pair.second.last_access_time > kPageCleanUpThreshold) {
       offline_pages.push_back(id_page_pair.second);
     }
   }
   return offline_pages;
 }
 
 const OfflinePageItem* OfflinePageModel::GetPageByBookmarkId(
     int64 bookmark_id) const {
   const auto iter = offline_pages_.find(bookmark_id);
-  return iter != offline_pages_.end() ? &(iter->second) : nullptr;
+  return iter != offline_pages_.end() && !iter->second.IsMarkedForDeletion()
+             ? &(iter->second)
+             : nullptr;
 }
 
 const OfflinePageItem* OfflinePageModel::GetPageByOfflineURL(
     const GURL& offline_url) const {
   for (auto iter = offline_pages_.begin();
        iter != offline_pages_.end();
        ++iter) {
-    if (iter->second.GetOfflineURL() == offline_url)
+    if (iter->second.GetOfflineURL() == offline_url &&
+        !iter->second.IsMarkedForDeletion()) {
       return &(iter->second);
+    }
   }
   return nullptr;
 }
 
 const OfflinePageItem* OfflinePageModel::GetPageByOnlineURL(
     const GURL& online_url) const {
   for (auto iter = offline_pages_.begin(); iter != offline_pages_.end();
        ++iter) {
-    if (iter->second.url == online_url)
+    if (iter->second.url == online_url && !iter->second.IsMarkedForDeletion()) {
       return &(iter->second);
+    }
   }
   return nullptr;
 }
 
 void OfflinePageModel::CheckForExternalFileDeletion() {
   DCHECK(is_loaded_);
 
   std::vector<std::pair<int64, base::FilePath>> id_path_pairs;
   for (const auto& id_page_pair : offline_pages_) {
     id_path_pairs.push_back(
@@ skipping 373 matching lines @@
 }
 
 void OfflinePageModel::CacheLoadedData(
     const std::vector<OfflinePageItem>& offline_pages) {
   offline_pages_.clear();
   for (const auto& offline_page : offline_pages)
     offline_pages_[offline_page.bookmark_id] = offline_page;
 }
 
 }  // namespace offline_pages