Rewrite DownloadsDOMHandler

chrome/browser/ui/webui/downloads_dom_handler.cc

 // Copyright (c) 2012 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 "chrome/browser/ui/webui/downloads_dom_handler.h"
 
 #include <algorithm>
 #include <functional>
 
 #include "base/basictypes.h"
@@ skipping 102 matching lines @@
       NOTREACHED();
       return "";
   }
 }
 
 // Return a JSON dictionary containing some of the attributes of |download|.
 // The JSON dictionary will also have a field "id" set to |id|, and a field
 // "otr" set to |incognito|.
 DictionaryValue* CreateDownloadItemValue(
     content::DownloadItem* download,
-    int id,
     bool incognito) {
   DictionaryValue* file_value = new DictionaryValue();
 
   file_value->SetInteger(
       "started", static_cast<int>(download->GetStartTime().ToTimeT()));
   file_value->SetString(
       "since_string", TimeFormat::RelativeDate(download->GetStartTime(), NULL));
   file_value->SetString(
       "date_string", base::TimeFormatShortDate(download->GetStartTime()));
-  file_value->SetInteger("id", id);
+  file_value->SetInteger("id", download->GetId());
 
   FilePath download_path(download->GetTargetFilePath());
   file_value->Set("file_path", base::CreateFilePathValue(download_path));
   file_value->SetString("file_url",
                         net::FilePathToFileURL(download_path).spec());
 
   // Keep file names as LTR.
   string16 file_name = download->GetFileNameToReportUser().LossyDisplayName();
   file_name = base::i18n::GetDisplayStringInLTRDirectionality(file_name);
   file_value->SetString("file_name", file_name);
@@ skipping 69 matching lines @@
   return (original_manager &&
           (manager->GetDownload(download_id) != NULL));
 }
 
 }  // namespace
 
 DownloadsDOMHandler::DownloadsDOMHandler(content::DownloadManager* dlm)
     : search_text_(),
       download_manager_(dlm),
       original_profile_download_manager_(NULL),
-      initialized_(false),
+      update_scheduled_(false),
       ALLOW_THIS_IN_INITIALIZER_LIST(weak_ptr_factory_(this)) {
   // Create our fileicon data source.
   Profile* profile = Profile::FromBrowserContext(dlm->GetBrowserContext());
   ChromeURLDataManager::AddDataSource(profile, new FileIconSource());
 
-  // Figure out our parent DownloadManager, if any.
-  Profile* original_profile = profile->GetOriginalProfile();
-  if (original_profile != profile) {
+  // Observe the DownloadManagers.
+  download_manager_->AddObserver(this);
+  if (profile->IsOffTheRecord()) {
     original_profile_download_manager_ =
-        BrowserContext::GetDownloadManager(original_profile);
+        BrowserContext::GetDownloadManager(profile->GetOriginalProfile());
+    original_profile_download_manager_->AddObserver(this);
+  }
+
+  // Observe all the DownloadItems.
+  content::DownloadManager::DownloadVector downloads;
+  SearchDownloads(&downloads);
+  for (content::DownloadManager::DownloadVector::const_iterator
+           iter = downloads.begin();
+       iter != downloads.end(); ++iter) {
+    (*iter)->AddObserver(this);
   }
 }
 
 DownloadsDOMHandler::~DownloadsDOMHandler() {
-  ClearDownloadItems();
+  // Make SearchDownloads() return all the items.
+  search_text_ = std::wstring();
+  content::DownloadManager::DownloadVector downloads;
+  SearchDownloads(&downloads);
+  for (content::DownloadManager::DownloadVector::const_iterator
+           iter = downloads.begin();
+       iter != downloads.end(); ++iter) {
+    (*iter)->RemoveObserver(this);
+  }

[Randy Smith (Not in Mondays), 2012/08/15 19:04:16]

This is quite scary to me--you're relying on the search (which is a reasonably
high level function) always returning the same downloads, even if there's a
change in their state.  Yes, it *should* work, but if it doesn't, you've got a
use-after-free error.  The usual pattern is to track all the items observed, and
remove those items when you remove observation, and I'd rather keep that
pattern.

[benjhayden, 2012/08/16 18:52:09]

Done, but I made a note to write AddObserver(WeakPtr<Observer>).

   download_manager_->RemoveObserver(this);
-  if (original_profile_download_manager_)
+  if (original_profile_download_manager_) {
     original_profile_download_manager_->RemoveObserver(this);
+  }
 }
 
 // DownloadsDOMHandler, public: -----------------------------------------------
 
 void DownloadsDOMHandler::OnPageLoaded(const base::ListValue* args) {
-  if (initialized_)
-    return;
-  initialized_ = true;
-
-  download_manager_->AddObserver(this);
-  if (original_profile_download_manager_)
-    original_profile_download_manager_->AddObserver(this);
+  SendCurrentDownloads();
 }
 
 void DownloadsDOMHandler::RegisterMessages() {
   web_ui()->RegisterMessageCallback("onPageLoaded",
       base::Bind(&DownloadsDOMHandler::OnPageLoaded,
-                 base::Unretained(this)));
+                 weak_ptr_factory_.GetWeakPtr()));
   web_ui()->RegisterMessageCallback("getDownloads",
       base::Bind(&DownloadsDOMHandler::HandleGetDownloads,
-                 base::Unretained(this)));
+                 weak_ptr_factory_.GetWeakPtr()));
   web_ui()->RegisterMessageCallback("openFile",
       base::Bind(&DownloadsDOMHandler::HandleOpenFile,
-                 base::Unretained(this)));
+                 weak_ptr_factory_.GetWeakPtr()));
   web_ui()->RegisterMessageCallback("drag",
       base::Bind(&DownloadsDOMHandler::HandleDrag,
-                 base::Unretained(this)));
+                 weak_ptr_factory_.GetWeakPtr()));
   web_ui()->RegisterMessageCallback("saveDangerous",
       base::Bind(&DownloadsDOMHandler::HandleSaveDangerous,
-                 base::Unretained(this)));
+                 weak_ptr_factory_.GetWeakPtr()));
   web_ui()->RegisterMessageCallback("discardDangerous",
       base::Bind(&DownloadsDOMHandler::HandleDiscardDangerous,
-                 base::Unretained(this)));
+                 weak_ptr_factory_.GetWeakPtr()));
   web_ui()->RegisterMessageCallback("show",
       base::Bind(&DownloadsDOMHandler::HandleShow,
-                 base::Unretained(this)));
+                 weak_ptr_factory_.GetWeakPtr()));
   web_ui()->RegisterMessageCallback("togglepause",
       base::Bind(&DownloadsDOMHandler::HandlePause,
-                 base::Unretained(this)));
+                 weak_ptr_factory_.GetWeakPtr()));
   web_ui()->RegisterMessageCallback("resume",
       base::Bind(&DownloadsDOMHandler::HandlePause,
-                 base::Unretained(this)));
+                 weak_ptr_factory_.GetWeakPtr()));
   web_ui()->RegisterMessageCallback("remove",
       base::Bind(&DownloadsDOMHandler::HandleRemove,
-                 base::Unretained(this)));
+                 weak_ptr_factory_.GetWeakPtr()));
   web_ui()->RegisterMessageCallback("cancel",
       base::Bind(&DownloadsDOMHandler::HandleCancel,
-                 base::Unretained(this)));
+                 weak_ptr_factory_.GetWeakPtr()));
   web_ui()->RegisterMessageCallback("clearAll",
       base::Bind(&DownloadsDOMHandler::HandleClearAll,
-                 base::Unretained(this)));
+                 weak_ptr_factory_.GetWeakPtr()));
   web_ui()->RegisterMessageCallback("openDownloadsFolder",
       base::Bind(&DownloadsDOMHandler::HandleOpenDownloadsFolder,
-                 base::Unretained(this)));
+                 weak_ptr_factory_.GetWeakPtr()));
+}
+
+void DownloadsDOMHandler::OnDownloadCreated(
+    content::DownloadManager* manager, content::DownloadItem* download_item) {
+  download_item->AddObserver(this);
+  if (!update_scheduled_) {

[Randy Smith (Not in Mondays), 2012/08/15 19:04:16]

Comment as to motivation.

[benjhayden, 2012/08/16 18:52:09]

Done.

+    update_scheduled_ = true;
+    BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
+        base::Bind(&DownloadsDOMHandler::SendCurrentDownloads,
+                   weak_ptr_factory_.GetWeakPtr()));
+  }
 }
 
 void DownloadsDOMHandler::OnDownloadUpdated(content::DownloadItem* download) {
-  // Get the id for the download. Our downloads are sorted latest to first,
-  // and the id is the index into that list. We should be careful of sync
-  // errors between the UI and the download_items_ list (we may wish to use
-  // something other than 'id').
-  OrderedDownloads::iterator it = std::find(download_items_.begin(),
-                                            download_items_.end(),
-                                            download);
-  if (it == download_items_.end())
-    return;
-
-  const int id = static_cast<int>(it - download_items_.begin());
-
-  ListValue results_value;
-  results_value.Append(CreateDownloadItemValue(download, id, IsItemIncognito(
+  base::ListValue results_value;
+  results_value.Append(CreateDownloadItemValue(download, IsItemIncognito(
       download->GetId(),
       download_manager_,
       original_profile_download_manager_)));
-  web_ui()->CallJavascriptFunction("downloadUpdated", results_value);
+  CallDownloadUpdated(results_value);
 }
 
 void DownloadsDOMHandler::OnDownloadDestroyed(
     content::DownloadItem* download) {
   download->RemoveObserver(this);
-  OrderedDownloads::iterator it = std::find(download_items_.begin(),
-                                            download_items_.end(),
-                                            download);
-  if (it != download_items_.end())
-    *it = NULL;
-  // A later ModelChanged() notification will change the WebUI's
-  // view of the downloads list.
-}
-
-// A download has started or been deleted. Query our DownloadManager for the
-// current set of downloads.
-void DownloadsDOMHandler::ModelChanged(content::DownloadManager* manager) {
-  DCHECK(manager == download_manager_ ||
-         manager == original_profile_download_manager_);
-
-  ClearDownloadItems();
-  download_manager_->SearchDownloads(WideToUTF16(search_text_),
-                                     &download_items_);
-  // If we have a parent DownloadManager, let it add its downloads to the
-  // results.
-  if (original_profile_download_manager_) {
-    original_profile_download_manager_->SearchDownloads(
-        WideToUTF16(search_text_), &download_items_);
+  if (!update_scheduled_) {
+    update_scheduled_ = true;
+    BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
+        base::Bind(&DownloadsDOMHandler::SendCurrentDownloads,
+                   weak_ptr_factory_.GetWeakPtr()));
   }
-
-  sort(download_items_.begin(), download_items_.end(), DownloadItemSorter());
-
-  // Remove any extension downloads.
-  for (size_t i = 0; i < download_items_.size();) {
-    if (download_crx_util::IsExtensionDownload(*download_items_[i]))
-      download_items_.erase(download_items_.begin() + i);
-    else
-      i++;
-  }
-
-  // Add ourself to all download items as an observer.
-  for (OrderedDownloads::iterator it = download_items_.begin();
-       it != download_items_.end(); ++it) {
-    if (static_cast<int>(it - download_items_.begin()) > kMaxDownloads)
-      break;
-
-    // We should never see anything that isn't already in the history.
-    DCHECK(*it);
-    DCHECK((*it)->IsPersisted());
-
-    (*it)->AddObserver(this);
-  }
-
-  SendCurrentDownloads();
 }
 
 void DownloadsDOMHandler::ManagerGoingDown(content::DownloadManager* manager) {
   // This should never happen.  The lifetime of the DownloadsDOMHandler
   // is tied to the tab in which downloads.html is displayed, which cannot
   // outlive the Browser that contains it, which cannot outlive the Profile
   // it is associated with.  If that profile is an incognito profile,
   // it cannot outlive its original profile.  Thus this class should be
   // destroyed before a ManagerGoingDown() notification occurs.
   NOTREACHED();
 }
 
-void DownloadsDOMHandler::HandleGetDownloads(const ListValue* args) {
+void DownloadsDOMHandler::HandleGetDownloads(const base::ListValue* args) {
   CountDownloadsDOMEvents(DOWNLOADS_DOM_EVENT_GET_DOWNLOADS);
-  std::wstring new_search = UTF16ToWideHack(ExtractStringValue(args));
-  if (search_text_.compare(new_search) != 0) {
-    search_text_ = new_search;
-    ModelChanged(download_manager_);
-  } else {
-    SendCurrentDownloads();
-  }
-
+  search_text_ = UTF16ToWideHack(ExtractStringValue(args));
+  SendCurrentDownloads();
   download_manager_->CheckForHistoryFilesRemoval();
 }
 
-void DownloadsDOMHandler::HandleOpenFile(const ListValue* args) {
+void DownloadsDOMHandler::HandleOpenFile(const base::ListValue* args) {
   CountDownloadsDOMEvents(DOWNLOADS_DOM_EVENT_OPEN_FILE);
   content::DownloadItem* file = GetDownloadByValue(args);
   if (file)
     file->OpenDownload();
 }
 
-void DownloadsDOMHandler::HandleDrag(const ListValue* args) {
+void DownloadsDOMHandler::HandleDrag(const base::ListValue* args) {
   CountDownloadsDOMEvents(DOWNLOADS_DOM_EVENT_DRAG);
   content::DownloadItem* file = GetDownloadByValue(args);
-  if (file) {
-    IconManager* im = g_browser_process->icon_manager();
-    gfx::Image* icon = im->LookupIcon(file->GetUserVerifiedFilePath(),
-                                      IconLoader::NORMAL);
-    gfx::NativeView view = web_ui()->GetWebContents()->GetNativeView();
-    {
-      // Enable nested tasks during DnD, while |DragDownload()| blocks.
-      MessageLoop::ScopedNestableTaskAllower allow(MessageLoop::current());
-      download_util::DragDownload(file, icon, view);
-    }
+  if (!file)
+    return;
+  IconManager* im = g_browser_process->icon_manager();
+  gfx::Image* icon = im->LookupIcon(file->GetUserVerifiedFilePath(),
+                                    IconLoader::NORMAL);
+  gfx::NativeView view = web_ui()->GetWebContents()->GetNativeView();
+  {
+    // Enable nested tasks during DnD, while |DragDownload()| blocks.
+    MessageLoop::ScopedNestableTaskAllower allow(MessageLoop::current());
+    download_util::DragDownload(file, icon, view);
   }
 }
 
-void DownloadsDOMHandler::HandleSaveDangerous(const ListValue* args) {
+void DownloadsDOMHandler::HandleSaveDangerous(const base::ListValue* args) {
   CountDownloadsDOMEvents(DOWNLOADS_DOM_EVENT_SAVE_DANGEROUS);
   content::DownloadItem* file = GetDownloadByValue(args);
   if (file)
     ShowDangerPrompt(file);
-  // TODO(benjhayden): else ModelChanged()? Downloads might be able to disappear
-  // out from under us, so update our idea of the downloads as soon as possible.

[Randy Smith (Not in Mondays), 2012/08/15 19:04:16]

How does this end up working?  Do we tear down the danger prompt if the download
disappears out from under us?

[benjhayden, 2012/08/16 18:52:09]

The danger prompt owns itself, so we hand it a weak ptr in its callbacks. If the
tab is closed before the danger prompt is accepted/canceled, then the weak ptr
will say "nevermind", and the WebContents will make sure that the danger prompt
cleans itself up.
As for downloads disappearing from under us, we notice as soon as possible
(OnDownloadDestroyed), and update the renderer in the next UI message loop
iteration.
GetDownloadByValue now uses DownloadManager::GetDownload(), so if the download
still exists, then we'll get it, and if it doesn't, then we won't.

 }
 
-void DownloadsDOMHandler::HandleDiscardDangerous(const ListValue* args) {
+void DownloadsDOMHandler::HandleDiscardDangerous(const base::ListValue* args) {
   CountDownloadsDOMEvents(DOWNLOADS_DOM_EVENT_DISCARD_DANGEROUS);
   content::DownloadItem* file = GetDownloadByValue(args);
   if (file)
     file->Delete(content::DownloadItem::DELETE_DUE_TO_USER_DISCARD);
 }
 
-void DownloadsDOMHandler::HandleShow(const ListValue* args) {
+void DownloadsDOMHandler::HandleShow(const base::ListValue* args) {
   CountDownloadsDOMEvents(DOWNLOADS_DOM_EVENT_SHOW);
   content::DownloadItem* file = GetDownloadByValue(args);
   if (file)
     file->ShowDownloadInShell();
 }
 
-void DownloadsDOMHandler::HandlePause(const ListValue* args) {
+void DownloadsDOMHandler::HandlePause(const base::ListValue* args) {
   CountDownloadsDOMEvents(DOWNLOADS_DOM_EVENT_PAUSE);
   content::DownloadItem* file = GetDownloadByValue(args);
   if (file)
     file->TogglePause();
 }
 
-void DownloadsDOMHandler::HandleRemove(const ListValue* args) {
+void DownloadsDOMHandler::HandleRemove(const base::ListValue* args) {
   CountDownloadsDOMEvents(DOWNLOADS_DOM_EVENT_REMOVE);
   content::DownloadItem* file = GetDownloadByValue(args);
   if (file) {
     DCHECK(file->IsPersisted());
     file->Remove();
   }
 }
 
-void DownloadsDOMHandler::HandleCancel(const ListValue* args) {
+void DownloadsDOMHandler::HandleCancel(const base::ListValue* args) {
   CountDownloadsDOMEvents(DOWNLOADS_DOM_EVENT_CANCEL);
   content::DownloadItem* file = GetDownloadByValue(args);
   if (file)
     file->Cancel(true);
 }
 
-void DownloadsDOMHandler::HandleClearAll(const ListValue* args) {
+void DownloadsDOMHandler::HandleClearAll(const base::ListValue* args) {
+  // TODO(benjhayden): Should this only clear items that match search_text_?

[Randy Smith (Not in Mondays), 2012/08/15 19:04:16]

I vote yes, but obviously not in this CL.

[benjhayden, 2012/08/16 18:52:09]

Done.

   CountDownloadsDOMEvents(DOWNLOADS_DOM_EVENT_CLEAR_ALL);
   download_manager_->RemoveAllDownloads();
 
   // If this is an incognito downloader, clear All should clear main download
   // manager as well.
   if (original_profile_download_manager_)
     original_profile_download_manager_->RemoveAllDownloads();
 }
 
-void DownloadsDOMHandler::HandleOpenDownloadsFolder(const ListValue* args) {
+void DownloadsDOMHandler::HandleOpenDownloadsFolder(
+    const base::ListValue* args) {
   CountDownloadsDOMEvents(DOWNLOADS_DOM_EVENT_OPEN_FOLDER);
   platform_util::OpenItem(
       DownloadPrefs::FromDownloadManager(download_manager_)->DownloadPath());
 }
 
 // DownloadsDOMHandler, private: ----------------------------------------------
 
 void DownloadsDOMHandler::SendCurrentDownloads() {
-  ListValue results_value;
-  for (OrderedDownloads::iterator it = download_items_.begin();
-      it != download_items_.end(); ++it) {
-    if (!*it)
-      continue;
-    int index = static_cast<int>(it - download_items_.begin());
-    if (index <= kMaxDownloads)
-      results_value.Append(CreateDownloadItemValue(*it, index, IsItemIncognito(
-            (*it)->GetId(),
+  update_scheduled_ = false;
+  content::DownloadManager::DownloadVector downloads;
+  SearchDownloads(&downloads);
+  sort(downloads.begin(), downloads.end(), DownloadItemSorter());
+  base::ListValue results_value;
+  for (content::DownloadManager::DownloadVector::const_iterator
+           iter = downloads.begin();
+       iter != downloads.end(); ++iter) {
+    // Filter out extension downloads.
+    if (!download_crx_util::IsExtensionDownload(**iter)) {
+      results_value.Append(CreateDownloadItemValue(*iter, IsItemIncognito(
+            (*iter)->GetId(),
             download_manager_,
             original_profile_download_manager_)));
+    }
+    if (results_value.GetSize() == kMaxDownloads)
+      break;
   }
+  CallDownloadsList(results_value);
+}
 
-  web_ui()->CallJavascriptFunction("downloadsList", results_value);
+void DownloadsDOMHandler::SearchDownloads(
+    content::DownloadManager::DownloadVector* downloads) {
+  // TODO(benjhayden): Use DownloadQuery, simplify DownloadManager interface
+  download_manager_->SearchDownloads(WideToUTF16(search_text_), downloads);
+  if (original_profile_download_manager_) {
+    original_profile_download_manager_->SearchDownloads(
+        WideToUTF16(search_text_), downloads);
+  }
 }
 
 void DownloadsDOMHandler::ShowDangerPrompt(
     content::DownloadItem* dangerous_item) {
   DownloadDangerPrompt* danger_prompt = DownloadDangerPrompt::Create(
       dangerous_item,
-      TabContents::FromWebContents(web_ui()->GetWebContents()),
+      TabContents::FromWebContents(GetWebUIWebContents()),
       base::Bind(&DownloadsDOMHandler::DangerPromptAccepted,
                  weak_ptr_factory_.GetWeakPtr(), dangerous_item->GetId()),
       base::Closure());
   // danger_prompt will delete itself.
   DCHECK(danger_prompt);
 }
 
 void DownloadsDOMHandler::DangerPromptAccepted(int download_id) {
   content::DownloadItem* item = download_manager_->GetActiveDownloadItem(
       download_id);
   if (!item)
     return;
   CountDownloadsDOMEvents(DOWNLOADS_DOM_EVENT_SAVE_DANGEROUS);
   item->DangerousDownloadValidated();
 }
 
-void DownloadsDOMHandler::ClearDownloadItems() {
-  // Clear out old state and remove self as observer for each download.
-  for (OrderedDownloads::iterator it = download_items_.begin();
-      it != download_items_.end(); ++it) {
-    if (!*it)
-      continue;
-    (*it)->RemoveObserver(this);
+content::DownloadItem* DownloadsDOMHandler::GetDownloadByValue(
+    const base::ListValue* args) {
+  int id = -1;
+  if (!ExtractIntegerValue(args, &id))
+    return NULL;
+  content::DownloadItem* download = download_manager_->GetDownload(id);
+  if (download == NULL) {
+    download = original_profile_download_manager_->GetDownload(id);
   }
-  download_items_.clear();
+  return download;
 }
 
-content::DownloadItem* DownloadsDOMHandler::GetDownloadById(int id) {
-  for (OrderedDownloads::iterator it = download_items_.begin();
-      it != download_items_.end(); ++it) {
-    if (static_cast<int>(it - download_items_.begin() == id)) {
-      return (*it);
-    }
-  }
-
-  return NULL;
+content::WebContents* DownloadsDOMHandler::GetWebUIWebContents() {
+  return web_ui()->GetWebContents();
 }
 
-content::DownloadItem* DownloadsDOMHandler::GetDownloadByValue(
-    const ListValue* args) {
-  int id;
-  if (ExtractIntegerValue(args, &id)) {
-    return GetDownloadById(id);
-  }
-  return NULL;
+void DownloadsDOMHandler::CallDownloadsList(const base::ListValue& downloads) {
+  web_ui()->CallJavascriptFunction("downloadsList", downloads);
 }
+
+void DownloadsDOMHandler::CallDownloadUpdated(const base::ListValue& download) {
+  web_ui()->CallJavascriptFunction("downloadUpdated", download);
+}