Fix save complete web page crasher

Repeatedly saving a page may cause a crash. See the bug for a full discussion. This fixes that by making sure the curretn SavePackage is the same one that called into SaveFileManager::RenameAllFiles.

One more hack on the giant, teetering pile of hacks that is the SavePackage system. Can't hurt, right?

BUG=42454
TEST=see bug

Review URL: http://codereview.chromium.org/1729016

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@45599 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 28 insertions, 18 deletions

diff --git a/chrome/browser/download/save_file_manager.cc b/chrome/browser/download/save_file_manager.cc
index a985071..5d9ad43 100644
--- a/chrome/browser/download/save_file_manager.cc
+++ b/chrome/browser/download/save_file_manager.cc
@@ -473,7 +473,8 @@ void SaveFileManager::RenameAllFiles(
     const FinalNameList& final_names,
     const FilePath& resource_dir,
     int render_process_id,
-    int render_view_id) {
+    int render_view_id,
+    int save_package_id) {
   DCHECK(ChromeThread::CurrentlyOn(ChromeThread::FILE));
 
   if (!resource_dir.empty() && !file_util::PathExists(resource_dir))
@@ -495,20 +496,19 @@ void SaveFileManager::RenameAllFiles(
       ChromeThread::UI, FROM_HERE,
       NewRunnableMethod(
           this, &SaveFileManager::OnFinishSavePageJob, render_process_id,
-          render_view_id));
+          render_view_id, save_package_id));
 }
 
 void SaveFileManager::OnFinishSavePageJob(int render_process_id,
-                                          int render_view_id) {
+                                          int render_view_id,
+                                          int save_package_id) {
   DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
 
   SavePackage* save_package =
       GetSavePackageFromRenderIds(render_process_id, render_view_id);
 
-  if (save_package) {
-    // save_package is null if save was canceled.
+  if (save_package && save_package->id() == save_package_id)
     save_package->Finish();
-  }
 }
 
 void SaveFileManager::RemoveSavedFileFromFileMap(
diff --git a/chrome/browser/download/save_file_manager.h b/chrome/browser/download/save_file_manager.h
index 24684ff..44597d5 100644
--- a/chrome/browser/download/save_file_manager.h
+++ b/chrome/browser/download/save_file_manager.h
@@ -136,7 +136,8 @@ class SaveFileManager
       const FinalNameList& final_names,
       const FilePath& resource_dir,
       int render_process_id,
-      int render_view_id);
+      int render_view_id,
+      int save_package_id);
 
   // When the user cancels the saving, we need to remove all remaining saved
   // files of this page saving job from save_file_map_.
@@ -190,7 +191,9 @@ class SaveFileManager
   // map:(url, SavePackage) to find the request and remove it.
   void OnErrorFinished(GURL save_url, int tab_id);
   // Notifies SavePackage that the whole page saving job is finished.
-  void OnFinishSavePageJob(int render_process_id, int render_view_id);
+  void OnFinishSavePageJob(int render_process_id,
+                           int render_view_id,
+                           int save_package_id);
 
   // Notifications sent from the UI thread and run on the file thread.
 
diff --git a/chrome/browser/download/save_package.cc b/chrome/browser/download/save_package.cc
index d58cc48..c276517 100644
--- a/chrome/browser/download/save_package.cc
+++ b/chrome/browser/download/save_package.cc
@@ -68,6 +68,9 @@ struct SavePackageParam {
 
 namespace {
 
+// A counter for uniquely identifying each save package.
+int g_save_package_id = 0;
+
 // Default name which will be used when we can not get proper name from
 // resource URL.
 const FilePath::CharType kDefaultSaveName[] =
@@ -165,6 +168,7 @@ SavePackage::SavePackage(TabContents* web_content,
       all_save_items_count_(0),
       wait_state_(INITIALIZE),
       tab_id_(web_content->GetRenderProcessHost()->id()),
+      unique_id_(g_save_package_id++),
       ALLOW_THIS_IN_INITIALIZER_LIST(method_factory_(this)) {
   DCHECK(web_content);
   const GURL& current_page_url = GetUrlToBeSaved();
@@ -190,6 +194,7 @@ SavePackage::SavePackage(TabContents* tab_contents)
       all_save_items_count_(0),
       wait_state_(INITIALIZE),
       tab_id_(tab_contents->GetRenderProcessHost()->id()),
+      unique_id_(g_save_package_id++),
       ALLOW_THIS_IN_INITIALIZER_LIST(method_factory_(this)) {
 
   const GURL& current_page_url = GetUrlToBeSaved();
@@ -216,6 +221,7 @@ SavePackage::SavePackage(TabContents* tab_contents,
       all_save_items_count_(0),
       wait_state_(INITIALIZE),
       tab_id_(0),
+      unique_id_(g_save_package_id++),
       ALLOW_THIS_IN_INITIALIZER_LIST(method_factory_(this)) {
   DCHECK(!saved_main_file_path_.empty() &&
          saved_main_file_path_.value().length() <= kMaxFilePathLength);
@@ -642,7 +648,8 @@ void SavePackage::CheckFinish() {
                         final_names,
                         dir,
                         tab_contents_->GetRenderProcessHost()->id(),
-                        tab_contents_->render_view_host()->routing_id()));
+                        tab_contents_->render_view_host()->routing_id(),
+                        id()));
 }
 
 // Successfully finished all items of this SavePackage.
diff --git a/chrome/browser/download/save_package.h b/chrome/browser/download/save_package.h
index 67db2bd..8d842d7 100644
--- a/chrome/browser/download/save_package.h
+++ b/chrome/browser/download/save_package.h
@@ -118,15 +118,11 @@ class SavePackage : public base::RefCountedThreadSafe<SavePackage>,
   // Show or Open a saved page via the Windows shell.
   void ShowDownloadInShell();
 
-  bool canceled() { return user_canceled_ || disk_error_occurred_; }
-
-  // Accessor
-  bool finished() { return finished_; }
-  SavePackageType save_type() { return save_type_; }
-
-  // Since for one tab, it can only have one SavePackage in same time.
-  // Now we actually use render_process_id as tab's unique id.
+  bool canceled() const { return user_canceled_ || disk_error_occurred_; }
+  bool finished() const { return finished_; }
+  SavePackageType save_type() const { return save_type_; }
   int tab_id() const { return tab_id_; }
+  int id() const { return unique_id_; }
 
   void GetSaveInfo();
   void ContinueGetSaveInfo(FilePath save_dir);
@@ -326,9 +322,13 @@ class SavePackage : public base::RefCountedThreadSafe<SavePackage>,
   // from outside.
   WaitState wait_state_;
 
-  // Unique id for this SavePackage.
+  // Since for one tab, it can only have one SavePackage in same time.
+  // Now we actually use render_process_id as tab's unique id.
   const int tab_id_;
 
+  // Unique ID for this SavePackage.
+  const int unique_id_;
+
   // For managing select file dialogs.
   scoped_refptr<SelectFileDialog> select_file_dialog_;