Fix a crash in DownloadUIAddapter that happens when Java bridge is created/removed quickly while

components/offline_pages/downloads/download_ui_adapter.cc

 // Copyright 2016 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/downloads/download_ui_adapter.h"
 
 #include "base/bind.h"
 #include "base/guid.h"
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
@@ skipping 11 matching lines @@
 DownloadUIAdapter::ItemInfo::ItemInfo(const OfflinePageItem& page)
     : ui_item(base::MakeUnique<DownloadUIItem>(page)),
       offline_id(page.offline_id),
       offline_url(page.GetOfflineURL()) {}
 
 DownloadUIAdapter::ItemInfo::~ItemInfo() {}
 
 DownloadUIAdapter::DownloadUIAdapter(OfflinePageModel* model)
     : model_(model),
       is_loaded_(false),
+      observers_count_(0),
       weak_ptr_factory_(this) {
 }
 
 DownloadUIAdapter::~DownloadUIAdapter() { }
 
 // static
 DownloadUIAdapter* DownloadUIAdapter::FromOfflinePageModel(
     OfflinePageModel* offline_page_model) {
   DownloadUIAdapter* adapter = static_cast<DownloadUIAdapter*>(
       offline_page_model->GetUserData(kDownloadUIAdapterKey));
   if (!adapter) {
     adapter = new DownloadUIAdapter(offline_page_model);
     offline_page_model->SetUserData(kDownloadUIAdapterKey, adapter);
   }
   return adapter;
 }
 
 void DownloadUIAdapter::AddObserver(Observer* observer) {
   DCHECK(observer);
-  if (!observers_.might_have_observers())
+  if (observers_count_ == 0)
     LoadCache();
   observers_.AddObserver(observer);
+  ++observers_count_;
   // If the items are already loaded, post the notification right away.
   // Don't just invoke it from here to avoid reentrancy in the client.
   if (is_loaded_) {
     base::ThreadTaskRunnerHandle::Get()->PostTask(
         FROM_HERE, base::Bind(&DownloadUIAdapter::NotifyItemsLoaded,
                               weak_ptr_factory_.GetWeakPtr(),
                               base::Unretained(observer)));
   }
 }
 
 void DownloadUIAdapter::RemoveObserver(Observer* observer) {
   observers_.RemoveObserver(observer);
+  --observers_count_;
   // Once the last observer is gone, clear cached data.
-  if (!observers_.might_have_observers())
+  if (observers_count_ == 0)
     ClearCache();
 }
 
 void DownloadUIAdapter::OfflinePageModelLoaded(OfflinePageModel* model) {
   // This signal is not used here.
 }
 
 void DownloadUIAdapter::OfflinePageModelChanged(OfflinePageModel* model) {
   DCHECK(model == model_);
   model_->GetAllPages(
@@ skipping 47 matching lines @@
 GURL DownloadUIAdapter::GetOfflineUrlByGuid(
     const std::string& guid) const {
   // TODO(dimich): when requests are also in the cache, filter them out.
   // Requests do not yet have offline URL.
   DownloadUIItems::const_iterator it = items_.find(guid);
   if (it != items_.end())
     return it->second->offline_url;
   return GURL();
 }
 
+// Note that several LoadCache calls may be issued before the async GetAllPages
+// comes back.
 void DownloadUIAdapter::LoadCache() {
-  DCHECK(!is_loaded_);
   // TODO(dimich): Add fetching from RequestQueue as well.
   model_->AddObserver(this);

[dewittj, 2016/08/22 21:54:07]

I think you'll want to AddObserver after the GetAllPages returns.  If you
imagine a prior operation starting to add a page right before this happens, you
might get an observer method firing before OnOfflinePagesLoaded, which would
probably cause a DCHECK in OnOfflinePagesLoaded

   model_->GetAllPages(

[dewittj, 2016/08/22 21:54:07]

do you want a loading_ indicator so you don't GetAllPages multiple times?

       base::Bind(&DownloadUIAdapter::OnOfflinePagesLoaded,
                  weak_ptr_factory_.GetWeakPtr()));
 }
 
 void DownloadUIAdapter::ClearCache() {
   model_->RemoveObserver(this);
   items_.clear();
   is_loaded_ = false;
 }
 
 void DownloadUIAdapter::OnOfflinePagesLoaded(
     const MultipleOfflinePageItemResult& pages) {
+  // If multiple observers register quickly, the cache might be already loaded
+  // by the previous LoadCache call. At the same time, if all observers already
+  // left, there is no reason to populate the cache.
+  if (is_loaded_ || observers_count_ == 0)
+    return;
   for (const auto& page : pages) {
     if (IsVisibleInUI(page.client_id)) {
       std::string guid = page.client_id.id;
       DCHECK(items_.find(guid) == items_.end());
       items_[guid] = base::MakeUnique<ItemInfo>(page);
     }
   }
   is_loaded_ = true;
   FOR_EACH_OBSERVER(Observer, observers_, ItemsLoaded());
 }
@@ skipping 30 matching lines @@
   // TODO(dimich): Consider adding UMA to record user actions.
 }
 
 bool DownloadUIAdapter::IsVisibleInUI(const ClientId& client_id) {
   const std::string& name_space = client_id.name_space;
   return (name_space == kAsyncNamespace || name_space == kDownloadNamespace) &&
          base::IsValidGUID(client_id.id);
 }
 
 }  // namespace offline_pages