[Offline pages] Fixing crashes caused by access to offline pages mark…

…ed for deletion

This patch fixes the problems caused by:
* HasOfflinePages
* GetPageByBookmarkId
* GetPageByOfflineURL
* GetPageByOnlineURL
All of which allow for access to the offline page that is marked for
deletion, which means there is no valid bookmark corresponding to them.
This leads to crashes in the bookmark UI, as well as exposing
functionality that should not work.

BUG=560424

Review URL: https://codereview.chromium.org/1475883003

Cr-Commit-Position: refs/heads/master@{#361746}
(cherry picked from commit 1a6c2e1)

Review URL: https://codereview.chromium.org/1487913002 .

Cr-Commit-Position: refs/branch-heads/2564@{#166}
Cr-Branched-From: 1283eca-refs/heads/master@{#359700}

components/offline_pages/offline_page_model.cc

@@ -253,7 +253,13 @@ void OfflinePageModel::ClearAll(const base::Closure& 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& id_page_pair : offline_pages_) {
+    if (!id_page_pair.second.IsMarkedForDeletion())
+      return true;
+  }
+  return false;
 }
 
 const std::vector<OfflinePageItem> OfflinePageModel::GetAllPages() const {
@@ -283,16 +289,20 @@ const std::vector<OfflinePageItem> OfflinePageModel::GetPagesToCleanUp() const {
 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;
 }
@@ -301,7 +311,7 @@ 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;

components/offline_pages/offline_page_model_unittest.cc

@@ -600,6 +600,8 @@ TEST_F(OfflinePageModelTest, MarkPageForDeletion) {
       base::Bind(&OfflinePageModelTest::OnSavePageDone, AsWeakPtr()));
   PumpLoop();
 
+  GURL offline_url = model()->GetAllPages().begin()->GetOfflineURL();
+
   // Delete the page with undo tiggerred.
   model()->MarkPageForDeletion(
       kTestPageBookmarkId1,
@@ -610,6 +612,11 @@ TEST_F(OfflinePageModelTest, MarkPageForDeletion) {
   const std::vector<OfflinePageItem>& offline_pages = model()->GetAllPages();
   EXPECT_EQ(0UL, offline_pages.size());
 
+  EXPECT_FALSE(model()->HasOfflinePages());
+  EXPECT_EQ(nullptr, model()->GetPageByOnlineURL(kTestUrl));
+  EXPECT_EQ(nullptr, model()->GetPageByBookmarkId(kTestPageBookmarkId1));
+  EXPECT_EQ(nullptr, model()->GetPageByOfflineURL(offline_url));
+
   // Undo the deletion.
   model()->UndoPageDeletion(kTestPageBookmarkId1);
   base::RunLoop().RunUntilIdle();