Save-Page-As-Complete-Html: Each frame links to a distinct local file.

content/browser/download/save_package.cc

Index: content/browser/download/save_package.cc
diff --git a/content/browser/download/save_package.cc b/content/browser/download/save_package.cc
index fe660b469d31410ec22fc2def021493f8bcaaa48..aad6645b8c1f8197f13cfa26d605b9bfe2ec41ea 100644
--- a/content/browser/download/save_package.cc
+++ b/content/browser/download/save_package.cc
@@ -354,10 +354,8 @@ void SavePackage::InitWithDownloadItem(
     SaveFileCreateInfo::SaveFileSource save_source = page_url_.SchemeIsFile() ?
         SaveFileCreateInfo::SAVE_FILE_FROM_FILE :
         SaveFileCreateInfo::SAVE_FILE_FROM_NET;
-    SaveItem* save_item = new SaveItem(page_url_,
-                                       Referrer(),
-                                       this,
-                                       save_source);
+    SaveItem* save_item = new SaveItem(page_url_, Referrer(), this, save_source,
+                                       FrameTreeNode::kFrameTreeNodeInvalidId);
     // Add this item to waiting list.
     waiting_item_queue_.push_back(save_item);
     all_save_items_count_ = 1;
@@ -999,30 +997,54 @@ void SavePackage::GetSerializedHtmlWithLocalLinksForFrame(
     FrameTreeNode* target_tree_node) {
   DCHECK(target_tree_node);
   int target_frame_tree_node_id = target_tree_node->frame_tree_node_id();
+  RenderFrameHostImpl* target = target_tree_node->current_frame_host();
 
   // Collect all saved success items.
   // SECURITY NOTE: We don't send *all* urls / local paths, but only
   // those that the given frame had access to already (because it contained
   // the savable resources / subframes associated with save items).
   std::map<GURL, base::FilePath> url_to_local_path;
+  std::map<int, base::FilePath> routing_id_to_local_path;
   auto it = frame_tree_node_id_to_contained_save_items_.find(
       target_frame_tree_node_id);
   if (it != frame_tree_node_id_to_contained_save_items_.end()) {
     for (SaveItem* save_item : it->second) {
+      // Calculate the local link to use for this |save_item|.
       DCHECK(save_item->has_final_name());
       base::FilePath local_path(base::FilePath::kCurrentDirectory);
       if (target_tree_node->IsMainFrame()) {
         local_path = local_path.Append(saved_main_directory_path_.BaseName());
       }
       local_path = local_path.Append(save_item->file_name());
-      url_to_local_path[save_item->url()] = local_path;
+
+      // Insert the link into |url_to_local_path| or |routing_id_to_local_path|.
+      if (save_item->save_source() != SaveFileCreateInfo::SAVE_FILE_FROM_DOM) {
+        DCHECK_EQ(FrameTreeNode::kFrameTreeNodeInvalidId,
+                  save_item->frame_tree_node_id());
+        url_to_local_path[save_item->url()] = local_path;
+      } else {
+        FrameTreeNode* save_item_frame_tree_node =
+            target_tree_node->frame_tree()->FindByID(
+                save_item->frame_tree_node_id());
+        if (!save_item_frame_tree_node) {
+          // crbug.com/541354: Raciness when saving a dynamically changing page.
+          continue;
+        }
+
+        int routing_id =
+            save_item_frame_tree_node->render_manager()
+                ->GetRoutingIdForSiteInstance(target->GetSiteInstance());
+        if (routing_id == MSG_ROUTING_NONE)

[Randy Smith (Not in Mondays), 2016/01/22 15:28:39]

Is this also due to 541354?  If so, could you duplicate the comment or combine
the if ... continue tests in one place?

[Łukasz Anforowicz, 2016/01/22 17:39:40]

Good point.  Right now I think that if |save_item_frame_tree_node| exists and
|target| exists, then we should never get a MSG_ROUTING_NONE here.  So I think
I'll change this if statement into a DCHECK.  And even if I am wrong, then
having MSG_ROUTING_NONE as a key in the map shouldn't hurt - there should be no
lookups using this key.

[Łukasz Anforowicz, 2016/01/25 19:04:05]

Done (talked with Nasko and used a DCHECK [instead of an if statement +
continue] to verify that we are not getting MSG_ROUTING_NONE).

+          continue;
+
+        routing_id_to_local_path[routing_id] = local_path;
+      }
     }
   }
 
   // Ask target frame to serialize itself.
-  RenderFrameHostImpl* target = target_tree_node->current_frame_host();
   target->Send(new FrameMsg_GetSerializedHtmlWithLocalLinks(
-      target->GetRoutingID(), url_to_local_path));
+      target->GetRoutingID(), url_to_local_path, routing_id_to_local_path));
 }
 
 // Process the serialized HTML content data of a specified frame
@@ -1156,6 +1178,7 @@ void SavePackage::OnSavableResourceLinksResponse(
 
 SaveItem* SavePackage::CreatePendingSaveItem(
     int container_frame_tree_node_id,
+    int save_item_frame_tree_node_id,
     const GURL& url,
     const Referrer& referrer,
     SaveFileCreateInfo::SaveFileSource save_source) {
@@ -1163,7 +1186,8 @@ SaveItem* SavePackage::CreatePendingSaveItem(
 
   SaveItem* save_item;
   Referrer sanitized_referrer = Referrer::SanitizeForRequest(url, referrer);
-  save_item = new SaveItem(url, sanitized_referrer, this, save_source);
+  save_item = new SaveItem(url, sanitized_referrer, this, save_source,
+                           save_item_frame_tree_node_id);
   waiting_item_queue_.push_back(save_item);
 
   frame_tree_node_id_to_contained_save_items_[container_frame_tree_node_id]
@@ -1173,6 +1197,7 @@ SaveItem* SavePackage::CreatePendingSaveItem(
 
 SaveItem* SavePackage::CreatePendingSaveItemDeduplicatingByUrl(
     int container_frame_tree_node_id,
+    int save_item_frame_tree_node_id,
     const GURL& url,
     const Referrer& referrer,
     SaveFileCreateInfo::SaveFileSource save_source) {
@@ -1188,7 +1213,8 @@ SaveItem* SavePackage::CreatePendingSaveItemDeduplicatingByUrl(
     frame_tree_node_id_to_contained_save_items_[container_frame_tree_node_id]
         .push_back(save_item);
   } else {
-    save_item = CreatePendingSaveItem(container_frame_tree_node_id, url,
+    save_item = CreatePendingSaveItem(container_frame_tree_node_id,
+                                      save_item_frame_tree_node_id, url,
                                       referrer, save_source);
     url_to_save_item_[url] = save_item;
   }
@@ -1205,8 +1231,9 @@ void SavePackage::EnqueueSavableResource(int container_frame_tree_node_id,
   SaveFileCreateInfo::SaveFileSource save_source =
       url.SchemeIsFile() ? SaveFileCreateInfo::SAVE_FILE_FROM_FILE
                          : SaveFileCreateInfo::SAVE_FILE_FROM_NET;
-  CreatePendingSaveItemDeduplicatingByUrl(container_frame_tree_node_id, url,
-                                          referrer, save_source);
+  CreatePendingSaveItemDeduplicatingByUrl(
+      container_frame_tree_node_id, FrameTreeNode::kFrameTreeNodeInvalidId, url,
+      referrer, save_source);
 }
 
 void SavePackage::EnqueueFrame(int container_frame_tree_node_id,
@@ -1215,9 +1242,9 @@ void SavePackage::EnqueueFrame(int container_frame_tree_node_id,
   if (!frame_original_url.is_valid())
     return;
 
-  SaveItem* save_item =
-      CreatePendingSaveItem(container_frame_tree_node_id, frame_original_url,
-                            Referrer(), SaveFileCreateInfo::SAVE_FILE_FROM_DOM);
+  SaveItem* save_item = CreatePendingSaveItem(
+      container_frame_tree_node_id, frame_tree_node_id, frame_original_url,
+      Referrer(), SaveFileCreateInfo::SAVE_FILE_FROM_DOM);
   DCHECK(save_item);
   frame_tree_node_id_to_save_item_[frame_tree_node_id] = save_item;
 }