Implement chrome.downloads.onDeterminingFilename()

chrome/browser/extensions/api/downloads/downloads_api.cc

Index: chrome/browser/extensions/api/downloads/downloads_api.cc
diff --git a/chrome/browser/extensions/api/downloads/downloads_api.cc b/chrome/browser/extensions/api/downloads/downloads_api.cc
index 931ee6bd489d52a03df1acd887a1590f52c374cf..9b17f1e01ca91520858dc1f11ab69d90ceb8f45f 100644
--- a/chrome/browser/extensions/api/downloads/downloads_api.cc
+++ b/chrome/browser/extensions/api/downloads/downloads_api.cc
@@ -14,6 +14,7 @@
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/callback.h"
+#include "base/file_path.h"
 #include "base/json/json_writer.h"
 #include "base/lazy_instance.h"
 #include "base/logging.h"
@@ -180,11 +181,13 @@ DownloadItem::DownloadState StateEnumFromString(const std::string& state) {
 }
 
 bool ValidateFilename(const string16& filename) {
-  // TODO(benjhayden): More robust validation of filename.
-  if ((filename.find('/') != string16::npos) ||
+  if (filename.empty() ||
+      (filename.find('/') != string16::npos) ||
       (filename.find('\\') != string16::npos))
     return false;
-  if (filename.size() >= 2u && filename[0] == L'.' && filename[1] == L'.')
+  if ((filename.size() >= 2u) &&
+      (filename[0] == L'.') &&
+      (filename[1] == L'.'))

[Randy Smith (Not in Mondays), 2013/01/14 21:02:29]

Doesn't this allow for, e.g. "phantom_subdir/../../../../etc/passwd"?

[benjhayden, 2013/01/17 02:52:05]

Yep, changed it to FilePath::ReferencesParent.

     return false;
   return true;
 }
@@ -258,8 +261,7 @@ bool DownloadFileIconExtractorImpl::ExtractIconURLForPath(
   IconManager* im = g_browser_process->icon_manager();
   // The contents of the file at |path| may have changed since a previous
   // request, in which case the associated icon may also have changed.
-  // Therefore, for the moment we always call LoadIcon instead of attempting
-  // a LookupIcon.
+  // Therefore, always call LoadIcon instead of attempting a LookupIcon.
   im->LoadIcon(path,
                icon_size,
                base::Bind(&DownloadFileIconExtractorImpl::OnIconLoadComplete,
@@ -405,7 +407,7 @@ void CompileDownloadQueryOrderBy(
   std::vector<std::string> order_by_strs;
   base::SplitString(order_by_str, ' ', &order_by_strs);
   for (std::vector<std::string>::const_iterator iter = order_by_strs.begin();
-      iter != order_by_strs.end(); ++iter) {
+       iter != order_by_strs.end(); ++iter) {
     std::string term_str = *iter;
     if (term_str.empty())
       continue;
@@ -540,7 +542,9 @@ class ExtensionDownloadsEventRouterData : public base::SupportsUserData::Data {
       scoped_ptr<base::DictionaryValue> json_item)
       : updated_(0),
         changed_fired_(0),
-        json_(json_item.Pass()) {
+        json_(json_item.Pass()),
+        determiner_index_(-1),
+        determined_overwrite_(false) {
     download_item->SetUserData(kKey, this);
   }
 
@@ -559,16 +563,88 @@ class ExtensionDownloadsEventRouterData : public base::SupportsUserData::Data {
   void OnItemUpdated() { ++updated_; }
   void OnChangedFired() { ++changed_fired_; }
 
+  void set_filename_change_callbacks(
+      const base::Closure& no_change,
+      const ExtensionDownloadsEventRouter::FilenameChangedCallback& change) {
+    filename_no_change_ = no_change;
+    filename_change_ = change;
+  }
+
+  void AddPendingDeterminer(const std::string& extension_id,
+                            const std::string& sub_event_id) {
+    determiners_.push_back(DeterminerInfo(extension_id, sub_event_id));
+  }
+
+  bool DeterminerCallback(
+      const std::string& extension_id,
+      const std::string& sub_event_id,
+      const FilePath& filename,
+      bool overwrite) {

[Randy Smith (Not in Mondays), 2013/01/14 21:02:29]

How likely is it that we'll get a response we didn't expect from a different
extension_id+sub_event_id?  I'm wondering if it's reasonable just to count down
responses here.

[benjhayden, 2013/01/17 02:52:05]

AFAIU, it should be impossible to receive an unexpected DeterminerCallback,
however, in order for there to be a deterministic precedence ordering between
determiners, the EDERD must know which determiner is calling and which
determiner previously set filename.

+    bool found_info = false;
+    for (int index = 0; index < static_cast<int>(determiners_.size());
+         ++index) {
+      if ((determiners_[index].extension_id == extension_id) &&
+          (determiners_[index].sub_event_id == sub_event_id)) {
+        DCHECK(!determiners_[index].reported);
+        determiners_[index].reported = true;
+        found_info = true;
+        if (!filename.empty() &&
+            !filename.IsAbsolute() &&
+            (index > determiner_index_)) {
+          determined_filename_ = filename;
+          determined_overwrite_ = overwrite;
+          determiner_index_ = index;
+        }
+      }
+    }
+    if (!found_info)
+      return false;
+    for (size_t index = 0; index < determiners_.size(); ++index) {
+      if (!determiners_[index].reported)
+        return true;
+    }
+    determiners_.clear();
+    if (determined_filename_.empty()) {
+      filename_no_change_.Run();
+    } else {
+      filename_change_.Run(determined_filename_, determined_overwrite_);
+    }
+    return true;
+  }
+
  private:
+  struct DeterminerInfo {
+    DeterminerInfo(const std::string& e_id, const std::string& sub_id);
+    ~DeterminerInfo();
+
+    std::string extension_id;
+    std::string sub_event_id;
+    bool reported;
+  };
+  typedef std::vector<DeterminerInfo> DeterminerInfoVector;
+
   static const char kKey[];
 
   int updated_;
   int changed_fired_;
   scoped_ptr<base::DictionaryValue> json_;
+  base::Closure filename_no_change_;
+  ExtensionDownloadsEventRouter::FilenameChangedCallback filename_change_;
+  DeterminerInfoVector determiners_;
+  int determiner_index_;
+  FilePath determined_filename_;
+  bool determined_overwrite_;
 
   DISALLOW_COPY_AND_ASSIGN(ExtensionDownloadsEventRouterData);
 };
 
+ExtensionDownloadsEventRouterData::DeterminerInfo::DeterminerInfo(
+    const std::string& e_id, const std::string& sub_id)
+    : extension_id(e_id), sub_event_id(sub_id) {
+}
+
+ExtensionDownloadsEventRouterData::DeterminerInfo::~DeterminerInfo() {}
+
 const char ExtensionDownloadsEventRouterData::kKey[] =
   "DownloadItem ExtensionDownloadsEventRouterData";
 
@@ -631,6 +707,7 @@ std::map<DownloadManager*, ManagerDestructionObserver*>*
 }  // namespace
 
 DownloadsDownloadFunction::DownloadsDownloadFunction() {}
+
 DownloadsDownloadFunction::~DownloadsDownloadFunction() {}
 
 bool DownloadsDownloadFunction::RunImpl() {
@@ -727,6 +804,7 @@ void DownloadsDownloadFunction::OnStarted(
 }
 
 DownloadsSearchFunction::DownloadsSearchFunction() {}
+
 DownloadsSearchFunction::~DownloadsSearchFunction() {}
 
 bool DownloadsSearchFunction::RunImpl() {
@@ -764,6 +842,7 @@ bool DownloadsSearchFunction::RunImpl() {
 }
 
 DownloadsPauseFunction::DownloadsPauseFunction() {}
+
 DownloadsPauseFunction::~DownloadsPauseFunction() {}
 
 bool DownloadsPauseFunction::RunImpl() {
@@ -787,6 +866,7 @@ bool DownloadsPauseFunction::RunImpl() {
 }
 
 DownloadsResumeFunction::DownloadsResumeFunction() {}
+
 DownloadsResumeFunction::~DownloadsResumeFunction() {}
 
 bool DownloadsResumeFunction::RunImpl() {
@@ -801,7 +881,7 @@ bool DownloadsResumeFunction::RunImpl() {
     error_ = download_extension_errors::kInvalidOperationError;
   } else {
     // Note that if the item isn't paused, this will be a no-op, and
-    // we will silently treat the extension call as a success.
+    // the extension call will seem successful.
     download_item->Resume();
   }
   if (error_.empty())
@@ -810,6 +890,7 @@ bool DownloadsResumeFunction::RunImpl() {
 }
 
 DownloadsCancelFunction::DownloadsCancelFunction() {}
+
 DownloadsCancelFunction::~DownloadsCancelFunction() {}
 
 bool DownloadsCancelFunction::RunImpl() {
@@ -821,13 +902,13 @@ bool DownloadsCancelFunction::RunImpl() {
   if (download_item != NULL)
     download_item->Cancel(true);
   // |download_item| can be NULL if the download ID was invalid or if the
-  // download is not currently active.  Either way, we don't consider it a
-  // failure.
+  // download is not currently active.  Either way, it's not a failure.
   RecordApiFunctions(DOWNLOADS_FUNCTION_CANCEL);
   return true;
 }
 
 DownloadsEraseFunction::DownloadsEraseFunction() {}
+
 DownloadsEraseFunction::~DownloadsEraseFunction() {}
 
 bool DownloadsEraseFunction::RunImpl() {
@@ -856,20 +937,8 @@ bool DownloadsEraseFunction::RunImpl() {
   return true;
 }
 
-DownloadsSetDestinationFunction::DownloadsSetDestinationFunction() {}
-DownloadsSetDestinationFunction::~DownloadsSetDestinationFunction() {}
-
-bool DownloadsSetDestinationFunction::RunImpl() {
-  scoped_ptr<extensions::api::downloads::SetDestination::Params> params(
-      extensions::api::downloads::SetDestination::Params::Create(*args_));
-  EXTENSION_FUNCTION_VALIDATE(params.get());
-  error_ = download_extension_errors::kNotImplementedError;
-  if (error_.empty())
-    RecordApiFunctions(DOWNLOADS_FUNCTION_SET_DESTINATION);
-  return error_.empty();
-}
-
 DownloadsAcceptDangerFunction::DownloadsAcceptDangerFunction() {}
+
 DownloadsAcceptDangerFunction::~DownloadsAcceptDangerFunction() {}
 
 bool DownloadsAcceptDangerFunction::RunImpl() {
@@ -883,6 +952,7 @@ bool DownloadsAcceptDangerFunction::RunImpl() {
 }
 
 DownloadsShowFunction::DownloadsShowFunction() {}
+
 DownloadsShowFunction::~DownloadsShowFunction() {}
 
 bool DownloadsShowFunction::RunImpl() {
@@ -901,6 +971,7 @@ bool DownloadsShowFunction::RunImpl() {
 }
 
 DownloadsOpenFunction::DownloadsOpenFunction() {}
+
 DownloadsOpenFunction::~DownloadsOpenFunction() {}
 
 bool DownloadsOpenFunction::RunImpl() {
@@ -919,6 +990,7 @@ bool DownloadsOpenFunction::RunImpl() {
 }
 
 DownloadsDragFunction::DownloadsDragFunction() {}
+
 DownloadsDragFunction::~DownloadsDragFunction() {}
 
 bool DownloadsDragFunction::RunImpl() {
@@ -973,8 +1045,8 @@ bool DownloadsGetFileIconFunction::RunImpl() {
     return false;
   }
   // In-progress downloads return the intermediate filename for GetFullPath()
-  // which doesn't have the final extension. Therefore we won't be able to
-  // derive a good file icon for it. So we use GetTargetFilePath() instead.
+  // which doesn't have the final extension. Therefore a good file icon can't be
+  // found, so use GetTargetFilePath() instead.
   DCHECK(icon_extractor_.get());
   DCHECK(icon_size == 16 || icon_size == 32);
   EXTENSION_FUNCTION_VALIDATE(icon_extractor_->ExtractIconURLForPath(
@@ -995,6 +1067,94 @@ void DownloadsGetFileIconFunction::OnIconURLExtracted(const std::string& url) {
   SendResponse(error_.empty());
 }
 
+// The method by which extensions hook into the filename determination process

[Randy Smith (Not in Mondays), 2013/01/14 21:02:29]

nit: I'd find this comment easier to read if it had paragraph breaks between the
three chunks of actions being done: The addition of the listener, the call out
to the extensions, and the return from the extensions.  (When I did my own
summary, I actually ended up doing four, splitting the fan in from the EDERD ->
CDMD callback.)

[benjhayden, 2013/01/17 02:52:05]

Done.

+// is based on the method by which the webRequest API allows extensions to hook
+// into the resource loading process.  Extensions that wish to play a part in
+// the filename determination process call
+// chrome.downloads.onDeterminingFilename.addListener, which secretly (see
+// chrome/renderer/resources/extensions/downloads_custom_bindings.js) calls
+// DownloadsInternalAddFilenameDeterminerFunction (see ../downloads_internal/),
+// which adds the extension's ID to the profile's ExtensionDownloadsEventRouter
+// (EDER).  When a download's filename is being determined,
+// ChromeDownloadManagerDelegate::CheckVisitedReferrerBeforeDone (CVRBD) passes
+// 2 callbacks to EDER::OnDownloadFilenameDetermined (ODFD), which stores the
+// callbacks in the item's ExtensionDownloadsEventRouterData (EDERD) along with
+// all of the extension IDs that are listening for onDeterminingFilename events.
+// ODFD dispatches chrome.downloads.onDeterminingFilename. When the extension's
+// event handler returns, downloads_custom_bindings.js calls
+// DownloadsInternalDetermineFilenameFunction::RunImpl, which notifies the
+// item's EDERD.  When the last extension's event handler returns, EDERD calls
+// one of the two callbacks that CVRBD passed to ODFD, allowing CDMD to complete
+// the filename determination process. If multiple extensions wish to override
+// the filename, then the last one wins. However, there's no deterministic way
+// to determine which extension will finish last, so users are advised (in the
+// developer documentation in downloads.idl) to try to use extensions that won't
+// conflict.

[Randy Smith (Not in Mondays), 2013/01/14 21:02:29]

If I read the code above, it's not the last one responding, it's the last one
registered (because of the determiner_index_ check).  Am I confused?

[benjhayden, 2013/01/17 02:52:05]

Changed the code and updated this comment.

+
+bool ExtensionDownloadsEventRouter::AddFilenameDeterminer(
+    Profile* profile,
+    bool include_incognito,
+    const std::string& ext_id,
+    const std::string& sub_event_id) {
+  if (include_incognito && profile->HasOffTheRecordProfile()) {

[Randy Smith (Not in Mondays), 2013/01/14 21:02:29]

My confusion, your opportunity to enlighten :-}: If we're an extension attached
to an incognito profile, we get visibility into the corresponding regular
profile.  The reverse is not true.  So it makes sense to me that (elsewhere) we
dispatch regular events to incognito EDERs.  But this looks like we're
installing listeners on incognito profiles in response to non-incognito
requests, which is Bad.  What am I missing?  

(I'd expect the bool to be called "include regular" and for the test to test
that and whether the profile *is* incognito, not whether it has a corresponding
incognito profile.)

[benjhayden, 2013/01/17 02:52:05]

PTAL, this changed.

+    AddFilenameDeterminer(profile->GetOffTheRecordProfile(),

[Randy Smith (Not in Mondays), 2013/01/14 21:02:29]

Ignoring return value?  That at least should have a comment (also for other such
calls).

[benjhayden, 2013/01/17 02:52:05]

Done.

+                          false,
+                          ext_id,
+                          sub_event_id);
+  }
+  ExtensionDownloadsEventRouter* router =
+      DownloadServiceFactory::GetForProfile(profile)->GetExtensionEventRouter();
+  for (DeterminerVector::const_iterator iter = router->determiners_.begin();
+       iter != router->determiners_.end(); ++iter) {
+    if (iter->first == ext_id && iter->second == sub_event_id)
+      return false;

[Randy Smith (Not in Mondays), 2013/01/14 21:02:29]

What is the larger implication about false returns from these functions? 
Doesn't it imply a bug in the internal downloads function?  (Except for
DetermineFilename, when the item could have been completed or something silly
like that.)  And if there are bugs in our code, even in Javascript, doesn't that
deserve a DCHECK?

[benjhayden, 2013/01/17 02:52:05]

Done.

+  }
+  router->determiners_.push_back(make_pair(ext_id, sub_event_id));
+  return true;
+}
+
+bool ExtensionDownloadsEventRouter::RemoveFilenameDeterminer(
+    Profile* profile,
+    bool include_incognito,
+    const std::string& ext_id,
+    const std::string& sub_event_id) {
+  if (include_incognito && profile->HasOffTheRecordProfile()) {
+    RemoveFilenameDeterminer(profile->GetOffTheRecordProfile(),
+                             false,
+                             ext_id,
+                             sub_event_id);
+  }
+  ExtensionDownloadsEventRouter* router =
+      DownloadServiceFactory::GetForProfile(profile)->GetExtensionEventRouter();
+  for (DeterminerVector::iterator iter = router->determiners_.begin();
+       iter != router->determiners_.end(); ++iter) {
+    if (iter->first == ext_id && iter->second == sub_event_id) {
+      router->determiners_.erase(iter);
+      return true;
+    }
+  }
+  return false;
+}
+
+bool ExtensionDownloadsEventRouter::DetermineFilename(
+    Profile* profile,
+    bool include_incognito,
+    const std::string& ext_id,
+    const std::string& sub_event_id,
+    int download_id,
+    const FilePath& filename,
+    bool overwrite) {
+  DownloadItem* item = GetDownloadIfInProgress(

[Randy Smith (Not in Mondays), 2013/01/14 21:02:29]

Is there any issue here with downloads resumption?  E.g. an interrupt occurs
while file determination is in progress, so we drop responses from the
extensions on the floor, and then resume the download with a EDERD still
attached to it in some intermediate state WRT responses from the extensions?

My inclination would be to reset the EDERD when we transition out of
IN_PROGRESS, but that may have gotchas I don't see.

[benjhayden, 2013/01/17 02:52:05]

I added EDERD::ClearPendingDeterminers, and call it both right before the
fan-out and right after the fan-in.
I'm not worried about leaving unusable (not exactly "leaked") determiners on the
EDERD in the rare case of interruption while extensions are responding, but if
it becomes a real problem then we can revisit this idea.
Are there any other concerns here?

[Randy Smith (Not in Mondays), 2013/01/17 19:15:42]

I think that's ok.  At least, the only problem I see is the leaking of
determiners.  (The only reason I wouldn't call it a leak is that it'll be
destroyed when the DownloadItem is.  But that's likely to be at browser
shutdown, and we don't really care about things that leak at shutdown, we care
about things that take up memory when the browser is running.)

+      profile, include_incognito, download_id);
+  if (!item)
+    return false;
+  ExtensionDownloadsEventRouterData* data =
+      ExtensionDownloadsEventRouterData::Get(item);
+  if (!data)
+    return false;
+  return data->DeterminerCallback(ext_id, sub_event_id, filename, overwrite);
+}
+
 ExtensionDownloadsEventRouter::ExtensionDownloadsEventRouter(
     Profile* profile,
     DownloadManager* manager)
@@ -1002,6 +1162,29 @@ ExtensionDownloadsEventRouter::ExtensionDownloadsEventRouter(
       ALLOW_THIS_IN_INITIALIZER_LIST(notifier_(manager, this)) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(profile_);
+  registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_UNLOADED,
+                 content::Source<Profile>(profile_));
+}
+
+void ExtensionDownloadsEventRouter::Observe(
+    int type,
+    const content::NotificationSource& source,
+    const content::NotificationDetails& details) {
+  switch (type) {
+    case chrome::NOTIFICATION_EXTENSION_UNLOADED: {
+      const extensions::Extension* extension = content::Details<
+          extensions::UnloadedExtensionInfo>(details)->extension;
+      for (DeterminerVector::iterator iter = determiners_.begin();
+           iter != determiners_.end(); ++iter) {
+        if (iter->first == extension->id())
+          iter = determiners_.erase(iter);

[Randy Smith (Not in Mondays), 2013/01/14 21:02:29]

What about download items that are waiting for responses from extensions?  Do we
need to cleanup their EDERD?

This and the comment above makes me a little tempted to suggest you not use the
UserData aspect of the download item but instead synchronize on a structure in
the EDER based on download id.  That'd allow you to not worry about what happens
to the download item at all; you can just pass the result of all this back to
the CDMD where it can decide whether to drop it on the floor all at once.  If we
don't do that, we're going to have to be thinking fairly carefully about races
as we go through this code, and I think it'll be more fragile in the future too.

[benjhayden, 2013/01/17 02:52:05]

Done.
I've had plenty of bugs due to parallel datastructures that I'd like to try to
avoid this map until we have a specific problem that can't be solved another
way.

+      }
+      break;
+    }
+    default:
+      NOTREACHED();
+      break;
+  }
 }
 
 ExtensionDownloadsEventRouter::~ExtensionDownloadsEventRouter() {
@@ -1022,6 +1205,38 @@ void ExtensionDownloadsEventRouter::OnDownloadCreated(
     new ExtensionDownloadsEventRouterData(download_item, json_item.Pass());
 }
 
+void ExtensionDownloadsEventRouter::OnDownloadFilenameDetermined(
+    DownloadItem* item,
+    const FilePath& suggested_path,
+    const base::Closure& no_change,
+    const FilenameChangedCallback& change) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  if (determiners_.empty()) {
+    no_change.Run();
+    return;
+  }
+
+  ExtensionDownloadsEventRouterData* data =
+      ExtensionDownloadsEventRouterData::Get(item);
+  data->set_filename_change_callbacks(no_change, change);
+  std::set<std::string> sub_events;
+  for (DeterminerVector::const_iterator iter = determiners_.begin();
+       iter != determiners_.end(); ++iter) {
+    data->AddPendingDeterminer(iter->first, iter->second);
+    sub_events.insert(iter->second);
+  }
+  std::string event_name(
+      extensions::event_names::kOnDownloadDeterminingFilename);
+  event_name += "/";
+  for (std::set<std::string>::const_iterator iter = sub_events.begin();
+       iter != sub_events.end(); ++iter) {
+    base::DictionaryValue* json = DownloadItemToJSON(
+        item, profile_->IsOffTheRecord()).release();
+    json->SetString(kFilenameKey, suggested_path.LossyDisplayName());
+    DispatchEvent((event_name + (*iter)).c_str(), json);
+  }
+}
+
 void ExtensionDownloadsEventRouter::OnDownloadUpdated(
     DownloadManager* manager, DownloadItem* download_item) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
@@ -1036,7 +1251,7 @@ void ExtensionDownloadsEventRouter::OnDownloadUpdated(
     return;
   }
   scoped_ptr<base::DictionaryValue> new_json(DownloadItemToJSON(
-        download_item, profile_->IsOffTheRecord()));
+      download_item, profile_->IsOffTheRecord()));
   scoped_ptr<base::DictionaryValue> delta(new base::DictionaryValue());
   delta->SetInteger(kIdKey, download_item->GetId());
   std::set<std::string> new_fields;
@@ -1101,13 +1316,13 @@ void ExtensionDownloadsEventRouter::DispatchEvent(
   // There is a one EDER for each on-record Profile, and a separate EDER for
   // each off-record Profile, so there is exactly one EDER for each
   // DownloadManager.  EDER only watches its own DM, so all the items that an
-  // EDER sees are either all on-record or all off-record. However, we want
-  // extensions in off-record contexts to see on-record items. So, if this EDER
-  // is watching an on-record DM, and there is a corresponding off-record
-  // Profile, then dispatch this event to both the on-record Profile and the
-  // off-record Profile.  There may or may not be an off-record Profile, so send
-  // a *copy* of |args| to the off-record Profile, and Pass() |args|
-  // to the Profile that we know is there.
+  // EDER sees are either all on-record or all off-record. However, extensions
+  // in off-record contexts should see on-record items. So, if this EDER is
+  // watching an on-record DM, and there is a corresponding off-record Profile,
+  // then dispatch this event to both the on-record Profile and the off-record
+  // Profile.  There may or may not be an off-record Profile, so send a *copy*
+  // of |args| to the off-record Profile, and Pass() |args| to the Profile that
+  // is certainly there.
   if (profile_->HasOffTheRecordProfile() &&
       !profile_->IsOffTheRecord()) {
     DispatchEventInternal(