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

chrome/browser/extensions/extension_updater.cc

 // Copyright (c) 2011 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/extensions/extension_updater.h"
 
 #include <algorithm>
 #include <set>
 
 #include "base/compiler_specific.h"
@@ skipping 360 matching lines @@
   }
   if (!fetch) {
     fetch = new ManifestFetchData(update_url);
     fetches_.insert(std::pair<GURL, ManifestFetchData*>(update_url, fetch));
     bool added = fetch->AddExtension(id, version.GetString(), ping_data,
                                      update_url_data);
     DCHECK(added);
   }
 }
 
-// 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(),
       package_hash(""),
       version("") {}
 
 ExtensionUpdater::ExtensionFetch::ExtensionFetch(const std::string& i,
                                                  const GURL& u,
                                                  const std::string& h,
                                                  const std::string& v)
@@ skipping 90 matching lines @@
 
 void ExtensionUpdater::Start() {
   DCHECK(!alive_);
   // If these are NULL, then that means we've been called after Stop()
   // has been called.
   DCHECK(service_);
   DCHECK(extension_prefs_);
   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);
   EnsureInt64PrefRegistered(prefs_, kNextExtensionsUpdateCheck);
   EnsureBlacklistVersionPrefRegistered(prefs_);
   ScheduleNextCheck(DetermineFirstCheckDelay());
 }
 
 void ExtensionUpdater::Stop() {
   weak_ptr_factory_.InvalidateWeakPtrs();
   alive_ = false;
-  file_handler_ = NULL;
   service_ = NULL;
   extension_prefs_ = NULL;
   prefs_ = NULL;
   profile_ = NULL;
   timer_.Stop();
   will_check_soon_ = false;
   method_factory_.RevokeAll();
   manifest_fetcher_.reset();
   extension_fetcher_.reset();
   STLDeleteElements(&manifests_pending_);
   manifests_pending_.clear();
   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();
   }
   NotifyIfFinished();
 }
 
 // Utility class to handle doing xml parsing in a sandboxed utility process.
 class SafeManifestParser : public UtilityProcessHost::Client {
  public:
   // Takes ownership of |fetch_data|.
@@ skipping 186 matching lines @@
 
   // Tell ExtensionService to update prefs.
   service_->UpdateExtensionBlacklist(blacklist);
 
   // Update the pref value for blacklist version
   prefs_->SetString(kExtensionBlacklistUpdateVersion,
                     current_extension_fetch_.version);
   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
     // 503 Service Unavailable / Retry-After headers, etc. here.
     // (http://crbug.com/12546).
     VLOG(1) << "Failed to fetch extension '" << url.possibly_invalid_spec()
             << "' response code:" << response_code;
   }
   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();
     FetchUpdatedExtension(next.id, next.url, next.package_hash, next.version);
   }
 }
 
 void ExtensionUpdater::OnCRXFileWritten(const std::string& id,
                                         const FilePath& path,
                                         const GURL& download_url) {
   DCHECK(alive_);
   // The ExtensionService is now responsible for cleaning up the temp file
   // 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) {
   DCHECK(alive_);
   DCHECK(!timer_.IsRunning());
   DCHECK(target_delay >= TimeDelta::FromSeconds(1));
 
   // Add +/- 10% random jitter.
   double delay_ms = target_delay.InMillisecondsF();
   double jitter_factor = (RandDouble() * .2) - 0.1;
@@ skipping 251 matching lines @@
       extensions_pending_.push_back(ExtensionFetch(id, url, hash, version));
     }
   } else {
     extension_fetcher_.reset(
         URLFetcher::Create(kExtensionFetcherId, url, URLFetcher::GET, this));
     extension_fetcher_->set_request_context(
         profile_->GetRequestContext());
     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);
   }
 }
 
 void ExtensionUpdater::NotifyStarted() {
   NotificationService::current()->Notify(
       NotificationType::EXTENSION_UPDATING_STARTED,
       Source<Profile>(profile_),
       NotificationService::NoDetails());
@@ skipping 20 matching lines @@
   std::set<std::string>::const_iterator i;
   for (i = ids.begin(); i != ids.end(); ++i)
     in_progress_ids_.insert(*i);
 }
 
 void ExtensionUpdater::RemoveFromInProgress(const std::set<std::string>& ids) {
   std::set<std::string>::const_iterator i;
   for (i = ids.begin(); i != ids.end(); ++i)
     in_progress_ids_.erase(*i);
 }