Allow URLFetcher to save results in a file. Have CRX updates use this capability.

chrome/browser/extensions/extension_updater.cc

Index: chrome/browser/extensions/extension_updater.cc
diff --git a/chrome/browser/extensions/extension_updater.cc b/chrome/browser/extensions/extension_updater.cc
index f3c9e34e90f883d2aa001f25cb8ab210e5683900..b4b9634f3832dfcf28cef1f4a7c68cc24e2bba84 100644
--- a/chrome/browser/extensions/extension_updater.cc
+++ b/chrome/browser/extensions/extension_updater.cc
@@ -378,109 +378,6 @@ void ManifestFetchesBuilder::AddExtensionData(
   }
 }
 
-// A utility class to do file handling on the file I/O thread.
-class ExtensionUpdaterFileHandler
-    : public base::RefCountedThreadSafe<ExtensionUpdaterFileHandler> {
- public:
-  explicit ExtensionUpdaterFileHandler(
-      base::WeakPtr<ExtensionUpdater> updater)
-      : updater_(updater) {
-    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  }
-
-  // Writes crx file data into a tempfile, and calls back the updater.
-  void WriteTempFile(const std::string& extension_id, const std::string& data,
-                     const GURL& download_url) {
-    // Make sure we're running in the right thread.
-    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
-
-    FileWriteResult file_write_result = SUCCESS;
-    FilePath path;
-    if (!file_util::CreateTemporaryFile(&path)) {
-      LOG(WARNING) << "Failed to create temporary file path";
-      file_write_result = CANT_CREATE_TEMP_CRX;
-    } else if (file_util::WriteFile(path, data.c_str(), data.length()) !=
-        static_cast<int>(data.length())) {
-      // TODO(asargent) - It would be nice to back off updating altogether if
-      // the disk is full. (http://crbug.com/12763).
-      LOG(ERROR) << "Failed to write temporary file";
-      file_util::Delete(path, false);
-      file_write_result = CANT_WRITE_CRX_DATA;
-    } else {
-      // We are seeing a high failure rate unpacking extensions, where
-      // the crx file can not be read. See if the file we wrote is readable.
-      // See crbug.com/81687 .
-      ScopedStdioHandle file(file_util::OpenFile(path, "rb"));
-      if (!file.get()) {
-        LOG(ERROR) << "Can't read CRX file written for update at path "
-                   << path.value().c_str();
-        file_util::Delete(path, false);
-        file_write_result = CANT_READ_CRX_FILE;
-      }
-    }
-
-    UMA_HISTOGRAM_ENUMERATION("Extensions.UpdaterWriteCrx", file_write_result,
-                              NUM_FILE_WRITE_RESULTS);
-
-    if (file_write_result != SUCCESS) {
-      if (!BrowserThread::PostTask(
-              BrowserThread::UI, FROM_HERE,
-              NewRunnableMethod(
-                  this, &ExtensionUpdaterFileHandler::OnCRXFileWriteError,
-                  extension_id))) {
-        NOTREACHED();
-      }
-    } else {
-      if (!BrowserThread::PostTask(
-              BrowserThread::UI, FROM_HERE,
-              NewRunnableMethod(
-                  this, &ExtensionUpdaterFileHandler::OnCRXFileWritten,
-                  extension_id, path, download_url))) {
-        NOTREACHED();
-        // Delete |path| since we couldn't post.
-        extension_file_util::DeleteFile(path, false);
-      }
-    }
-  }
-
- private:
-  friend class base::RefCountedThreadSafe<ExtensionUpdaterFileHandler>;
-
-  ~ExtensionUpdaterFileHandler() {
-    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI) ||
-           BrowserThread::CurrentlyOn(BrowserThread::FILE));
-  }
-
-  void OnCRXFileWritten(const std::string& id,
-                        const FilePath& path,
-                        const GURL& download_url) {
-    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-    if (!updater_) {
-      // Delete |path| since we don't have an updater anymore.
-      if (!BrowserThread::PostTask(
-              BrowserThread::FILE, FROM_HERE,
-              NewRunnableFunction(
-                  extension_file_util::DeleteFile, path, false))) {
-        NOTREACHED();
-      }
-      return;
-    }
-    // The ExtensionUpdater now owns the temp file.
-    updater_->OnCRXFileWritten(id, path, download_url);
-  }
-
-  void OnCRXFileWriteError(const std::string& id) {
-    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-    if (!updater_) {
-      return;
-    }
-    updater_->OnCRXFileWriteError(id);
-  }
-
-  // Should be accessed only on UI thread.
-  base::WeakPtr<ExtensionUpdater> updater_;
-};
-
 ExtensionUpdater::ExtensionFetch::ExtensionFetch()
     : id(""),
       url(),
@@ -591,8 +488,6 @@ void ExtensionUpdater::Start() {
   DCHECK(prefs_);
   DCHECK(profile_);
   DCHECK(!weak_ptr_factory_.HasWeakPtrs());
-  file_handler_ =
-      new ExtensionUpdaterFileHandler(weak_ptr_factory_.GetWeakPtr());
   alive_ = true;
   // Make sure our prefs are registered, then schedule the first check.
   EnsureInt64PrefRegistered(prefs_, kLastExtensionsUpdateCheck);
@@ -604,7 +499,6 @@ void ExtensionUpdater::Start() {
 void ExtensionUpdater::Stop() {
   weak_ptr_factory_.InvalidateWeakPtrs();
   alive_ = false;
-  file_handler_ = NULL;
   service_ = NULL;
   extension_prefs_ = NULL;
   prefs_ = NULL;
@@ -619,21 +513,23 @@ void ExtensionUpdater::Stop() {
   extensions_pending_.clear();
 }
 
-void ExtensionUpdater::OnURLFetchComplete(
-    const URLFetcher* source,
-    const GURL& url,
-    const net::URLRequestStatus& status,
-    int response_code,
-    const net::ResponseCookies& cookies,
-    const std::string& data) {
+void ExtensionUpdater::OnURLFetchComplete(const URLFetcher* source) {
   // Stop() destroys all our URLFetchers, which means we shouldn't be
   // called after Stop() is called.
   DCHECK(alive_);
 
   if (source == manifest_fetcher_.get()) {
-    OnManifestFetchComplete(url, status, response_code, data);
+    std::string data;
+    CHECK(source->GetResponseAsString(&data));
+    OnManifestFetchComplete(source->url(),
+                            source->status(),
+                            source->response_code(),
+                            data);
   } else if (source == extension_fetcher_.get()) {
-    OnCRXFetchComplete(url, status, response_code, data);
+    OnCRXFetchComplete(source,
+                       source->url(),
+                       source->status(),
+                       source->response_code());
   } else {
     NOTREACHED();
   }
@@ -840,26 +736,31 @@ void ExtensionUpdater::ProcessBlacklist(const std::string& data) {
   prefs_->ScheduleSavePersistentPrefs();
 }
 
-void ExtensionUpdater::OnCRXFetchComplete(const GURL& url,
-                                          const net::URLRequestStatus& status,
-                                          int response_code,
-                                          const std::string& data) {
-  if (status.status() == net::URLRequestStatus::SUCCESS &&
-      (response_code == 200 || (url.SchemeIsFile() && data.length() > 0))) {
+void ExtensionUpdater::OnCRXFetchComplete(
+    const URLFetcher* source,
+    const GURL& url,
+    const net::URLRequestStatus& status,
+    int response_code) {
+
+  base::PlatformFileError error_code = base::PLATFORM_FILE_OK;
+  if (source->FileErrorOccurred(&error_code)) {
+    LOG(ERROR) << "Failed to write update CRX with id "
+               << current_extension_fetch_.id << ". "
+               << "Error code is "<< error_code;
+    OnCRXFileWriteError(current_extension_fetch_.id);

[asargent_no_longer_on_chrome, 2011/05/18 19:00:49]

It looks like we've lost the histogramming of file write success/failure from 
ExtensionUpdaterFileHandler::WriteTempFile. Should you add something back here?
Or would it make sense to add an option to URLFetcher to have it do histograms
of such failures (perhaps with a tag the consumer passes in for the histogram
name)? This could be a future enhancement. 

[Sam Kerner (Chrome), 2011/05/18 23:45:57]

Added a histogram to this file.  A histogram option is hard because histograms
with non-constant names are not allowed.

+
+  } else if (status.status() == net::URLRequestStatus::SUCCESS &&
+      (response_code == 200 || url.SchemeIsFile())) {

[asargent_no_longer_on_chrome, 2011/05/18 19:00:49]

Slightly off-topic: I vaguely recall a bug where we didn't handle redirects
properly, and I wonder if this is because this just handles status 200. Do you
know off the top of your head if URLFetcher automatically follows redirects and
only calls your Delegate with the final result of any redirect chain?

[Sam Kerner (Chrome), 2011/05/18 23:45:57]

I am not sure about this.  URLRequest::Delegate may implement
OnReceivedRedirect(), but URLFetcher does not implement it.

There are comments in this file about dealing with 5xx responses.  URLFetcher
can do this.  The default number of retries is 0, so we are not doing it today.

     if (current_extension_fetch_.id == kBlacklistAppID) {
+      std::string data;
+      CHECK(source->GetResponseAsString(&data));
       ProcessBlacklist(data);
       in_progress_ids_.erase(current_extension_fetch_.id);
     } else {
-      // Successfully fetched - now write crx to a file so we can have the
-      // ExtensionService install it.
-      if (!BrowserThread::PostTask(
-              BrowserThread::FILE, FROM_HERE,
-              NewRunnableMethod(
-                  file_handler_.get(),
-                  &ExtensionUpdaterFileHandler::WriteTempFile,
-                  current_extension_fetch_.id, data, url))) {
-        NOTREACHED();
-      }
+      FilePath crx_path;
+      // Take ownership of the file at |crx_path|.
+      CHECK(source->GetResponseAsFilePath(true, &crx_path));
+      OnCRXFileWritten(current_extension_fetch_.id, crx_path, url);
     }
   } else {
     // TODO(asargent) do things like exponential backoff, handling
@@ -871,7 +772,7 @@ void ExtensionUpdater::OnCRXFetchComplete(const GURL& url,
   extension_fetcher_.reset();
   current_extension_fetch_ = ExtensionFetch();
 
-  // If there are any pending downloads left, start one.
+  // If there are any pending downloads left, start the next one.
   if (!extensions_pending_.empty()) {
     ExtensionFetch next = extensions_pending_.front();
     extensions_pending_.pop_front();
@@ -887,13 +788,11 @@ void ExtensionUpdater::OnCRXFileWritten(const std::string& id,
   // at |path|.
   service_->UpdateExtension(id, path, download_url);
   in_progress_ids_.erase(id);
-  NotifyIfFinished();

[asargent_no_longer_on_chrome, 2011/05/18 19:00:49]

whoa, are you sure this isn't needed anymore?

[Sam Kerner (Chrome), 2011/05/18 23:45:57]

OnCRXFileWritten() and OnCRXFileWriteError() are now called from
OnURLFetchComplete().  The call to NotifyIfFinished() at the end of 
ExtensionUpdater::OnURLFetchComplete() will be called after they run.  I
initially left them in, and saw test failures because the notification fired
twice.

 }
 
 void ExtensionUpdater::OnCRXFileWriteError(const std::string& id) {
   DCHECK(alive_);
   in_progress_ids_.erase(id);
-  NotifyIfFinished();

[asargent_no_longer_on_chrome, 2011/05/18 19:00:49]

same question here

 }
 
 void ExtensionUpdater::ScheduleNextCheck(const TimeDelta& target_delay) {
@@ -1165,6 +1064,14 @@ void ExtensionUpdater::FetchUpdatedExtension(const std::string& id,
     extension_fetcher_->set_load_flags(net::LOAD_DO_NOT_SEND_COOKIES |
                                        net::LOAD_DO_NOT_SAVE_COOKIES |
                                        net::LOAD_DISABLE_CACHE);
+    // Download CRX files to a temp file. The blacklist is small and will be
+    // processed in memory, so it is fetched into a string.
+    if (id != ExtensionUpdater::kBlacklistAppID) {
+      extension_fetcher_->set_file_message_loop_proxy(
+          BrowserThread::GetMessageLoopProxyForThread(BrowserThread::FILE));
+      extension_fetcher_->SaveResponseToTemporaryFile();
+    }
+
     extension_fetcher_->Start();
     current_extension_fetch_ = ExtensionFetch(id, url, hash, version);
   }