Fixes two crashers in saving page:

1. GetTabID was being called AFTER the process was destroyed, which
   means we could try and deref NULL. By caching the value we don't
   have to worry about whether the web contents goes away or not.
2. A PostTask was done, then we assumed the SaveItem still
   exists. That isn't the case if the user canceled the save.

BUG=2206
TEST=none, just make sure save page as still works correctly.

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

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

3 files changed, 18 insertions, 15 deletions

diff --git a/chrome/browser/download/save_file_manager.cc b/chrome/browser/download/save_file_manager.cc
index 2a909b72..13b2d1f 100644
--- a/chrome/browser/download/save_file_manager.cc
+++ b/chrome/browser/download/save_file_manager.cc
@@ -90,7 +90,7 @@ void SaveFileManager::RegisterStartingRequest(const std::wstring& save_url,
                                               SavePackage* save_package) {
   // Make sure it runs in the UI thread.
   DCHECK(MessageLoop::current() == ui_loop_);
-  int tab_id = save_package->GetTabId();
+  int tab_id = save_package->tab_id();
 
   // Register this starting request.
   StartingRequestsMap& starting_requests = tab_starting_requests_[tab_id];
@@ -201,7 +201,7 @@ void SaveFileManager::RemoveSaveFile(int save_id, const std::wstring& save_url,
   // so remove it if it exists.
   if (save_id == -1) {
     SavePackage* old_package = UnregisterStartingRequest(save_url,
-                                                         package->GetTabId());
+                                                         package->tab_id());
     DCHECK(old_package == package);
   } else {
     SavePackageMap::iterator it = packages_.find(save_id);
@@ -560,7 +560,10 @@ void SaveFileManager::OnFinishSavePageJob(int render_process_id,
   SavePackage* save_package =
       GetSavePackageFromRenderIds(render_process_id, render_view_id);
 
-  save_package->Finish();
+  if (save_package) {
+    // save_package is null if save was canceled.
+    save_package->Finish();
+  }
 }
 
 void SaveFileManager::RemoveSavedFileFromFileMap(
diff --git a/chrome/browser/download/save_package.cc b/chrome/browser/download/save_package.cc
index 5410d31..ec350ea 100644
--- a/chrome/browser/download/save_package.cc
+++ b/chrome/browser/download/save_package.cc
@@ -68,7 +68,8 @@ SavePackage::SavePackage(WebContents* web_content,
       user_canceled_(false),
       download_(NULL),
       finished_(false),
-      wait_state_(INITIALIZE) {
+      wait_state_(INITIALIZE),
+      tab_id_(web_content->process()->host_id()) {
   DCHECK(web_content);
   const GURL& current_page_url = web_contents_->GetURL();
   DCHECK(current_page_url.is_valid());
@@ -91,7 +92,8 @@ SavePackage::SavePackage(const wchar_t* file_full_path,
       finished_(true),
       download_(NULL),
       user_canceled_(false),
-      disk_error_occurred_(false) {
+      disk_error_occurred_(false),
+      tab_id_(0) {
   DCHECK(!saved_main_file_path_.empty() &&
          saved_main_file_path_.length() <= kMaxFilePathLength);
   DCHECK(!saved_main_directory_path_.empty() &&
@@ -366,7 +368,7 @@ void SavePackage::StartSave(const SaveFileCreateInfo* info) {
                           &SaveFileManager::SaveLocalFile,
                           save_item->url(),
                           save_item->save_id(),
-                          GetTabId()));
+                          tab_id()));
     return;
   }
 
@@ -709,11 +711,6 @@ void SavePackage::DoSavingProcess() {
   }
 }
 
-int SavePackage::GetTabId() {
-  DCHECK(web_contents_);
-  return web_contents_->process()->host_id();
-}
-
 // After finishing all SaveItems which need to get data from net.
 // We collect all URLs which have local storage and send the
 // map:(originalURL:currentLocalPath) to render process (backend).
@@ -776,7 +773,7 @@ void SavePackage::ProcessSerializedHtmlData(const GURL& frame_url,
   if (wait_state_ != HTML_DATA)
     return;
 
-  int tab_id = GetTabId();
+  int id = tab_id();
   // If the all frames are finished saving, we need to close the
   // remaining SaveItems.
   if (flag == webkit_glue::DomSerializerDelegate::ALL_FRAMES_ARE_FINISHED) {
@@ -787,7 +784,7 @@ void SavePackage::ProcessSerializedHtmlData(const GURL& frame_url,
                             &SaveFileManager::SaveFinished,
                             it->second->save_id(),
                             it->second->url(),
-                            tab_id,
+                            id,
                             true));
     }
     return;
@@ -821,7 +818,7 @@ void SavePackage::ProcessSerializedHtmlData(const GURL& frame_url,
                           &SaveFileManager::SaveFinished,
                           save_item->save_id(),
                           save_item->url(),
-                          tab_id,
+                          id,
                           true));
   }
 }
diff --git a/chrome/browser/download/save_package.h b/chrome/browser/download/save_package.h
index 8c12190..7b6edcd 100644
--- a/chrome/browser/download/save_package.h
+++ b/chrome/browser/download/save_package.h
@@ -127,7 +127,7 @@ class SavePackage : public base::RefCountedThreadSafe<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.
-  int GetTabId();
+  int tab_id() const { return tab_id_; }
 
   // Helper function for preparing suggested name for the SaveAs Dialog. The
   // suggested name is composed of the default save path and the web document's
@@ -303,6 +303,9 @@ class SavePackage : public base::RefCountedThreadSafe<SavePackage> {
   // from outside.
   WaitState wait_state_;
 
+  // Unique id for this SavePackage.
+  const int tab_id_;
+
   DISALLOW_EVIL_CONSTRUCTORS(SavePackage);
 };