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 10 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),
+      state_(State::NOT_LOADED),
+      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_;

[fgorski, 2016/08/22 22:52:22]

It may make sense to use HasObserver as a guard when adding and removing the
observers, so that your observer count does not get out of sync with reality.

E.g. some code adds/removes itself twice.

[Dmitry Titov, 2016/08/23 01:21:34]

Done. Indeed. Crazy. 

   // 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_) {
+  if (state_ == State::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);
+  state_ = State::LOADING;
   model_->GetAllPages(
       base::Bind(&DownloadUIAdapter::OnOfflinePagesLoaded,
                  weak_ptr_factory_.GetWeakPtr()));
 }
 
 void DownloadUIAdapter::ClearCache() {
   model_->RemoveObserver(this);
   items_.clear();
-  is_loaded_ = false;
+  state_ = State::NOT_LOADED;

[fgorski, 2016/08/22 22:52:22]

perhaps invalidate weak pointers here to prevent any callbacks? (Which would be
a more canonical way of handling the all observers unsubscribed scenario...
But I think your current approach works too.

[Dmitry Titov, 2016/08/23 01:21:34]

Ack. But I think that also will stop other operations, like DeleteItem, which
would be unintended. 

 }
 
 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 (state_ != State::LOADING)
+    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;
+  model_->AddObserver(this);
+  state_ = State::LOADED;
   FOR_EACH_OBSERVER(Observer, observers_, ItemsLoaded());
 }
 
 void DownloadUIAdapter::NotifyItemsLoaded(Observer* observer) {
   if (observer && observers_.HasObserver(observer))
     observer->ItemsLoaded();
 }
 
 // This method is only called by OPM when a single item added.
 // TODO(dimich): change OPM to have real OnPageAdded/OnPageUpdated and
@@ skipping 22 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