ABANDONED: OOPIFs: Moving stitching of local paths from renderer to browser process.

content/browser/download/save_package.cc

 // Copyright (c) 2012 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 "content/browser/download/save_package.h"
 
 #include <algorithm>
 
 #include "base/bind.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/i18n/file_util_icu.h"
 #include "base/logging.h"
 #include "base/message_loop/message_loop.h"
 #include "base/stl_util.h"
 #include "base/strings/string_piece.h"
 #include "base/strings/string_split.h"
 #include "base/strings/sys_string_conversions.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/threading/thread.h"
 #include "components/url_formatter/url_formatter.h"
 #include "content/browser/download/download_item_impl.h"
 #include "content/browser/download/download_manager_impl.h"
 #include "content/browser/download/download_stats.h"
 #include "content/browser/download/save_file.h"
 #include "content/browser/download/save_file_manager.h"
 #include "content/browser/download/save_item.h"
+#include "content/browser/frame_host/frame_tree.h"
+#include "content/browser/frame_host/frame_tree_node.h"
 #include "content/browser/loader/resource_dispatcher_host_impl.h"
 #include "content/browser/renderer_host/render_process_host_impl.h"
 #include "content/browser/renderer_host/render_view_host_delegate.h"
 #include "content/browser/renderer_host/render_view_host_impl.h"
+#include "content/browser/web_contents/web_contents_impl.h"
 #include "content/common/frame_messages.h"
-#include "content/common/view_messages.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/content_browser_client.h"
 #include "content/public/browser/download_manager_delegate.h"
 #include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/notification_types.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/resource_context.h"
 #include "content/public/browser/web_contents.h"
 #include "net/base/filename_util.h"
 #include "net/base/io_buffer.h"
 #include "net/base/mime_util.h"
 #include "net/url_request/url_request_context.h"
-#include "third_party/WebKit/public/web/WebPageSerializerClient.h"
 #include "url/url_constants.h"
 
 using base::Time;
-using blink::WebPageSerializerClient;
 
 namespace content {
 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 char kDefaultSaveName[] = "saved_resource";
@@ skipping 169 matching lines @@
     Cancel(true);
   }
 
   // We should no longer be observing the DownloadItem at this point.
   CHECK(!download_);
 
   DCHECK(all_save_items_count_ == (waiting_item_queue_.size() +
                                    completed_count() +
                                    in_process_count()));
   // Free all SaveItems.
-  while (!waiting_item_queue_.empty()) {
-    // We still have some items which are waiting for start to save.
-    SaveItem* save_item = waiting_item_queue_.front();
-    waiting_item_queue_.pop();
-    delete save_item;
-  }
-
+  STLDeleteElements(&waiting_item_queue_);
+  STLDeleteValues(&in_progress_items_);
   STLDeleteValues(&saved_success_items_);
-  STLDeleteValues(&in_progress_items_);
   STLDeleteValues(&saved_failed_items_);
 
   file_manager_ = NULL;
 }
 
 GURL SavePackage::GetUrlToBeSaved() {
   // Instead of using web_contents_.GetURL here, we use url() (which is the
   // "real" url of the page) from the NavigationEntry because it reflects its
   // origin rather than the displayed one (returned by GetURL) which may be
   // different (like having "view-source:" on the front).
@@ skipping 81 matching lines @@
     GetSavableResourceLinks();
   } else if (save_type_ == SAVE_PAGE_TYPE_AS_MHTML) {
     web_contents()->GenerateMHTML(saved_main_file_path_, base::Bind(
         &SavePackage::OnMHTMLGenerated, this));
   } else {
     DCHECK_EQ(SAVE_PAGE_TYPE_AS_ONLY_HTML, save_type_) << save_type_;
     wait_state_ = NET_FILES;
     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_, this, save_source);
     // Add this item to waiting list.
-    waiting_item_queue_.push(save_item);
+    waiting_item_queue_.push_back(save_item);
     all_save_items_count_ = 1;
     download_->SetTotalBytes(1);
 
     DoSavingProcess();
   }
 }
 
 void SavePackage::OnMHTMLGenerated(int64 size) {
   if (size <= 0) {
     Cancel(false);
@@ skipping 260 matching lines @@
     // HTML data back.
     GetSerializedHtmlWithLocalLinks();
   }
 }
 
 SaveItem* SavePackage::LookupItemInProcessBySaveId(int32 save_id) {
   if (in_process_count()) {
     for (SaveUrlItemMap::iterator it = in_progress_items_.begin();
         it != in_progress_items_.end(); ++it) {
       SaveItem* save_item = it->second;
-      DCHECK(save_item->state() == SaveItem::IN_PROGRESS);
+      DCHECK(save_item->state() == SaveItem::IN_PROGRESS ||
+             save_item->state() == SaveItem::COMPLETING);
       if (save_item->save_id() == save_id)
         return save_item;
     }
   }
   return NULL;
 }
 
 void SavePackage::PutInProgressItemToSavedMap(SaveItem* save_item) {
   SaveUrlItemMap::iterator it = in_progress_items_.find(
       save_item->url().spec());
@@ skipping 41 matching lines @@
   // there won't be valid pointers for file_manager_ or download_.
   if (wait_state_ == INITIALIZE)
     return;
 
   // When stopping, if it still has some items in in_progress, cancel them.
   DCHECK(canceled());
   if (in_process_count()) {
     SaveUrlItemMap::iterator it = in_progress_items_.begin();
     for (; it != in_progress_items_.end(); ++it) {
       SaveItem* save_item = it->second;
-      DCHECK(save_item->state() == SaveItem::IN_PROGRESS);
+      DCHECK(save_item->state() == SaveItem::IN_PROGRESS ||
+             save_item->state() == SaveItem::COMPLETING);
       save_item->Cancel();
     }
     // Remove all in progress item to saved map. For failed items, they will
     // be put into saved_failed_items_, for successful item, they will be put
     // into saved_success_items_.
     while (in_process_count())
       PutInProgressItemToSavedMap(in_progress_items_.begin()->second);
   }
 
   // This vector contains the save ids of the save files which SaveFileManager
@@ skipping 207 matching lines @@
 // SaveFileManager, which will dispatch it to different approach according to
 // the save source. Parameter process_all_remaining_items indicates whether
 // we need to save all remaining items.
 void SavePackage::SaveNextFile(bool process_all_remaining_items) {
   DCHECK(web_contents());
   DCHECK(waiting_item_queue_.size());
 
   do {
     // Pop SaveItem from waiting list.
     SaveItem* save_item = waiting_item_queue_.front();
-    waiting_item_queue_.pop();
+    waiting_item_queue_.pop_front();
 
     // Add the item to in_progress_items_.
     SaveUrlItemMap::iterator it = in_progress_items_.find(
         save_item->url().spec());
     DCHECK(it == in_progress_items_.end());
     in_progress_items_[save_item->url().spec()] = save_item;
     save_item->Start();
     file_manager_->SaveURL(save_item->url(),
                            save_item->referrer(),
                            web_contents()->GetRenderProcessHost()->GetID(),
@@ skipping 60 matching lines @@
     if (waiting_item_queue_.size()) {
       DCHECK(all_save_items_count_ == waiting_item_queue_.size());
       SaveNextFile(false);
     }
   }
 }
 
 bool SavePackage::OnMessageReceived(const IPC::Message& message,
                                     RenderFrameHost* render_frame_host) {
   bool handled = true;
-  IPC_BEGIN_MESSAGE_MAP_WITH_PARAM(SavePackage, message, render_frame_host)
+  auto* sender = static_cast<RenderFrameHostImpl*>(render_frame_host);
+  IPC_BEGIN_MESSAGE_MAP_WITH_PARAM(SavePackage, message, sender)
     IPC_MESSAGE_HANDLER(FrameHostMsg_SavableResourceLinksResponse,
                         OnSavableResourceLinksResponse)
     IPC_MESSAGE_HANDLER(FrameHostMsg_SavableResourceLinksError,
                         OnSavableResourceLinksError)
-    IPC_MESSAGE_HANDLER(FrameHostMsg_SerializedHtmlWithLocalLinksResponse,
-                        OnSerializedHtmlWithLocalLinksResponse)
+    IPC_MESSAGE_HANDLER(FrameHostMsg_SerializedHtmlFragment,
+                        OnSerializedHtmlFragment)
+    IPC_MESSAGE_HANDLER(FrameHostMsg_SerializedLocalPathForChildFrame,
+                        OnSerializedLocalPathForChildFrame)
+    IPC_MESSAGE_HANDLER(FrameHostMsg_SerializedLocalPathForSavableResource,
+                        OnSerializedLocalPathForSavableResource)
+    IPC_MESSAGE_HANDLER(FrameHostMsg_SerializedEndOfFrame,
+                        OnSerializedEndOfFrame)
     IPC_MESSAGE_UNHANDLED(handled = false)
   IPC_END_MESSAGE_MAP()
   return handled;
 }
 
 // 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).
 // Then render process will serialize DOM and send data to us.
 void SavePackage::GetSerializedHtmlWithLocalLinks() {
   if (wait_state_ != HTML_DATA)
     return;
-  std::vector<GURL> saved_links;
-  std::vector<base::FilePath> saved_file_paths;
   int successful_started_items_count = 0;
 
-  // Collect all saved items which have local storage.
-  // First collect the status of all the resource files and check whether they
-  // have created local files although they have not been completely saved.
-  // If yes, the file can be saved. Otherwise, there is a disk error, so we
-  // need to cancel the page saving job.
-  for (SaveUrlItemMap::iterator it = in_progress_items_.begin();
-       it != in_progress_items_.end(); ++it) {
-    DCHECK(it->second->save_source() ==
-           SaveFileCreateInfo::SAVE_FILE_FROM_DOM);
-    if (it->second->has_final_name())
+  // Count all saved items which have local storage.
+  for (const auto& it : in_progress_items_) {
+    DCHECK_EQ(SaveFileCreateInfo::SAVE_FILE_FROM_DOM, it.second->save_source());
+    if (it.second->has_final_name())
       successful_started_items_count++;
-    saved_links.push_back(it->second->url());
-    saved_file_paths.push_back(it->second->file_name());
   }
 
   // If not all file of HTML resource have been started, then wait.
   if (successful_started_items_count != in_process_count())
     return;
 
-  // Collect all saved success items.
-  for (SavedItemMap::iterator it = saved_success_items_.begin();
-       it != saved_success_items_.end(); ++it) {
-    DCHECK(it->second->has_final_name());
-    saved_links.push_back(it->second->url());
-    saved_file_paths.push_back(it->second->file_name());
-  }
-
-  // Get the relative directory name.
-  base::FilePath relative_dir_name = saved_main_directory_path_.BaseName();
-
   // Ask all frames for their serialized data.
   DCHECK_EQ(0, number_of_frames_pending_response_);
   web_contents()->ForEachFrame(base::Bind(
       &SavePackage::GetSerializedHtmlWithLocalLinksForFrame,
-      base::Unretained(this),  // Safe, because ForEachFrame is synchronous.
-      saved_links, saved_file_paths, relative_dir_name));
+      base::Unretained(this)));  // Safe, because ForEachFrame is synchronous.
   DCHECK_LT(0, number_of_frames_pending_response_);
 }
 
 void SavePackage::GetSerializedHtmlWithLocalLinksForFrame(
-    const std::vector<GURL>& saved_links,
-    const std::vector<base::FilePath>& saved_file_paths,
-    const base::FilePath& relative_dir_name,
     RenderFrameHost* target) {
   number_of_frames_pending_response_++;
-  target->Send(new FrameMsg_GetSerializedHtmlWithLocalLinks(
-      target->GetRoutingID(), saved_links, saved_file_paths,
-      relative_dir_name));
+  target->Send(
+      new FrameMsg_GetSerializedHtmlWithLocalLinks(target->GetRoutingID()));
 }
 
-// Process the serialized HTML content data of a specified frame
-// retrieved from the renderer process.
-void SavePackage::OnSerializedHtmlWithLocalLinksResponse(
-    RenderFrameHost* sender,
-    const GURL& frame_url,
-    const std::string& data,
-    int32 status) {
-  WebPageSerializerClient::PageSerializationStatus flag =
-      static_cast<WebPageSerializerClient::PageSerializationStatus>(status);
-
-  // When calling WebPageSerializer::serialize in non-recursive mode, the
-  // AllFramesAreFinished is redundant - it is sent by each frame right after
-  // CurrentFrameIsFinished.  Therefore we ignore AllFramesAreFinished and
-  // instead track pending frames in |number_of_frames_pending_response_|.
-  if (flag == WebPageSerializerClient::AllFramesAreFinished)
-    return;
-
-  // Check current state.
+void SavePackage::OnSerializedHtmlFragment(RenderFrameHostImpl* sender,
+                                           const std::string& data) {
   if (wait_state_ != HTML_DATA)
     return;
 
-  int id = contents_id();
-
-  SaveUrlItemMap::iterator it = in_progress_items_.find(frame_url.spec());
-  if (it == in_progress_items_.end()) {
-    for (SavedItemMap::iterator saved_it = saved_success_items_.begin();
-      saved_it != saved_success_items_.end(); ++saved_it) {
-      if (saved_it->second->url() == frame_url) {
-        wrote_to_completed_file_ = true;
-        break;
-      }
-    }
-
-    it = saved_failed_items_.find(frame_url.spec());
-    if (it != saved_failed_items_.end())
-      wrote_to_failed_file_ = true;
-
+  int frame_tree_node_id = sender->frame_tree_node()->frame_tree_node_id();
+  auto it = frame_tree_node_id_to_save_item_.find(frame_tree_node_id);
+  if (it == frame_tree_node_id_to_save_item_.end()) {
+    // Sanitization of renderer IPC - we will have no save item if
+    // 1) the renderer misbehaves and sends OnSerializedHtmlFragment IPC without
+    //    being asked to.
+    // 2) if a new frame gets created between OnSavableResourceLinksResponse
+    //    and GetSerializedHtmlWithLocalLinksForFrame (crbug.com/541354).
+    return;
+  }
+  SaveItem* save_item = it->second;
+  if (save_item->state() != SaveItem::IN_PROGRESS) {
+    LogWriteToAlreadyCompletedOrFailedSaveItem(save_item);
     return;
   }
 
-  SaveItem* save_item = it->second;
   DCHECK(save_item->save_source() == SaveFileCreateInfo::SAVE_FILE_FROM_DOM);
-
   if (!data.empty()) {
     // Prepare buffer for saving HTML data.
     scoped_refptr<net::IOBuffer> new_data(new net::IOBuffer(data.size()));
     memcpy(new_data->data(), data.data(), data.size());
 
     // Call write file functionality in file thread.
     BrowserThread::PostTask(
         BrowserThread::FILE, FROM_HERE,
-        base::Bind(&SaveFileManager::UpdateSaveProgress,
-                   file_manager_,
-                   save_item->save_id(),
-                   new_data,
+        base::Bind(&SaveFileManager::UpdateSaveProgress, file_manager_,
+                   save_item->save_id(), new_data,
                    static_cast<int>(data.size())));
   }
+}
+
+void SavePackage::OnSerializedLocalPathForChildFrame(
+    RenderFrameHostImpl* sender,
+    int render_frame_or_proxy_routing_id) {
+  if (wait_state_ != HTML_DATA)
+    return;
+
+  FrameTreeNode* frame_tree_node =
+      sender->frame_tree_node()->frame_tree()->FindByRoutingID(
+          sender->GetProcess()->GetID(), render_frame_or_proxy_routing_id);
+  if (!frame_tree_node)
+    return;
+  if (frame_tree_node->parent() != sender->frame_tree_node())
+    return;
+
+  auto it = frame_tree_node_id_to_save_item_.find(
+      frame_tree_node->frame_tree_node_id());
+  SaveItem* save_item =
+      it != frame_tree_node_id_to_save_item_.end() ? it->second : nullptr;
+  SerializeLocalPathForSaveItem(sender, save_item,
+                                frame_tree_node->current_url());
+}
+
+void SavePackage::OnSerializedLocalPathForSavableResource(
+    RenderFrameHostImpl* sender,
+    const GURL& savable_resource) {
+  if (wait_state_ != HTML_DATA)
+    return;
+  if (!savable_resource.is_valid())
+    return;
+
+  auto it = url_to_save_item_.find(savable_resource);
+  SaveItem* save_item = it != url_to_save_item_.end() ? it->second : nullptr;
+  SerializeLocalPathForSaveItem(sender, save_item, savable_resource);
+}
+
+void SavePackage::OnSerializedEndOfFrame(RenderFrameHostImpl* sender) {
+  if (wait_state_ != HTML_DATA)
+    return;
+
+  int frame_tree_node_id = sender->frame_tree_node()->frame_tree_node_id();
+  auto it = frame_tree_node_id_to_save_item_.find(frame_tree_node_id);
+  if (it == frame_tree_node_id_to_save_item_.end())
+    return;
+  SaveItem* save_item = it->second;
+  if (save_item->state() != SaveItem::IN_PROGRESS) {
+    LogWriteToAlreadyCompletedOrFailedSaveItem(save_item);
+    return;
+  }
+  save_item->MarkAsCompleting();
+
+  int id = contents_id();
 
   // Current frame is completed saving, call finish in file thread.
-  if (flag == WebPageSerializerClient::CurrentFrameIsFinished) {
-    DVLOG(20) << " " << __FUNCTION__ << "()"
-              << " save_id = " << save_item->save_id()
-              << " url = \"" << save_item->url().spec() << "\"";
-    BrowserThread::PostTask(
-        BrowserThread::FILE, FROM_HERE,
-        base::Bind(&SaveFileManager::SaveFinished,
-                   file_manager_,
-                   save_item->save_id(),
-                   save_item->url(),
-                   id,
-                   true));
-    number_of_frames_pending_response_--;
-    DCHECK_LE(0, number_of_frames_pending_response_);
-  }
+  DVLOG(20) << " " << __FUNCTION__ << "()"
+            << " save_id = " << save_item->save_id() << " url = \""
+            << save_item->url().spec() << "\"";
+  BrowserThread::PostTask(
+      BrowserThread::FILE, FROM_HERE,
+      base::Bind(&SaveFileManager::SaveFinished, file_manager_,
+                 save_item->save_id(), save_item->url(), id, true));
+  number_of_frames_pending_response_--;
+  DCHECK_LE(0, number_of_frames_pending_response_);
 
-  // If all frames are finished saving, we need to close the remaining
-  // SaveItems.
+  // If the all frames are finished saving, we need to close the
+  // remaining SaveItems.
   if (number_of_frames_pending_response_ == 0) {
-    for (SaveUrlItemMap::iterator it = in_progress_items_.begin();
-         it != in_progress_items_.end(); ++it) {
+    for (const auto& pair : in_progress_items_) {
       DVLOG(20) << " " << __FUNCTION__ << "()"
-                << " save_id = " << it->second->save_id() << " url = \""
-                << it->second->url().spec() << "\"";
+                << " save_id = " << pair.second->save_id() << " url = \""
+                << pair.second->url().spec() << "\"";
       BrowserThread::PostTask(
           BrowserThread::FILE, FROM_HERE,
           base::Bind(&SaveFileManager::SaveFinished, file_manager_,
-                     it->second->save_id(), it->second->url(), id, true));
+                     pair.second->save_id(), pair.second->url(), id, true));
     }
   }
 }
 
+void SavePackage::SerializeLocalPathForSaveItem(RenderFrameHostImpl* sender,
+                                                SaveItem* save_item,
+                                                const GURL& fallback_url) {
+  std::string attr_value;
+  if (save_item) {
+    base::FilePath relative_path =
+        base::FilePath(FILE_PATH_LITERAL("."))
+            .Append(saved_main_directory_path_.BaseName())
+            .Append(save_item->file_name())
+            .NormalizePathSeparatorsTo(FILE_PATH_LITERAL('/'));
+    attr_value = relative_path.AsUTF8Unsafe();
+  } else {
+    // Use |fallback_url| if we didn't see the referenced item when gathering
+    // savable resources in an earlier step (i.e. because the page being saved
+    // changes during the save - crbug.com/541354).
+    attr_value = fallback_url.spec();
+  }
+
+  // TODO(lukasza): Escape attr_value (&quot; + &amp;).

[ncarter (slow), 2015/10/12 23:31:48]

Responding to your comments in the earlier thread, three thoughts:

==

I don't see this as being worrisome attack surface for a sandbox escape (since
the attacker can write whatever they want to the file already), but it might be
an interesting target somehow from the web, to the extent that a saved-page can
insert """>;}"'/>"""LPT1 or something into what becomes the saved the filename.
We definitely need to plug the latter kind of hole as part of this CL, since
we're losing the protection offered by blink.

==

I don't see why we need any HTML escaping at all here?

The renderer has written everything up to and including the quotation mark, say:

<img src="

And we have to write something after that that's a legal URL, that maps to the
filename, and that doesn't break out of the context?, right? I think URL
canonicalization (via GURL, or maybe there's another util) will do that.

In other words, quotation marks should become %22, not &quot; -- right?

==

It looks like there might be some utils in net for going between file-scheme
URLs and FilePaths, but I'm not sure if they help here or not.

[Łukasz Anforowicz, 2015/10/21 22:50:35]

Daniel pointed out net::EscapeForHTML.  I am not sure how I could have missed it
earlier.  Works great.

+  OnSerializedHtmlFragment(sender, attr_value);
+}
+
+void SavePackage::LogWriteToAlreadyCompletedOrFailedSaveItem(
+    SaveItem* save_item) {
+  auto already_completed_it =
+      std::find_if(saved_success_items_.begin(), saved_success_items_.end(),
+                   [save_item](const SavedItemMap::value_type& v) {
+                     return v.second->url() == save_item->url();
+                   });
+  if (already_completed_it != saved_success_items_.end())
+    wrote_to_completed_file_ = true;
+
+  auto failed_it =
+      std::find_if(saved_failed_items_.begin(), saved_failed_items_.end(),
+                   [save_item](const SaveUrlItemMap::value_type& v) {
+                     DCHECK(v.second->has_final_name());
+                     return v.second->url() == save_item->url();
+                   });
+  if (failed_it != saved_failed_items_.end())
+    wrote_to_failed_file_ = true;
+}
+
 // Ask for all savable resource links from backend, include main frame and
 // sub-frame.
 void SavePackage::GetSavableResourceLinks() {
   if (wait_state_ != START_PROCESS)
     return;
 
   wait_state_ = RESOURCES_LIST;
 
   DCHECK_EQ(0, number_of_frames_pending_response_);
   web_contents()->ForEachFrame(base::Bind(
       &SavePackage::GetSavableResourceLinksForFrame,
       base::Unretained(this)));  // Safe, because ForEachFrame is synchronous.
   DCHECK_LT(0, number_of_frames_pending_response_);
 }
 
 void SavePackage::GetSavableResourceLinksForFrame(RenderFrameHost* target) {
   number_of_frames_pending_response_++;
   target->Send(new FrameMsg_GetSavableResourceLinks(target->GetRoutingID()));
 }
 
 void SavePackage::OnSavableResourceLinksResponse(
-    RenderFrameHost* sender,
+    RenderFrameHostImpl* sender,
     const GURL& frame_url,
     const std::vector<GURL>& resources_list,
     const std::vector<Referrer>& referrers_list) {
   if (wait_state_ != RESOURCES_LIST)
     return;
 
   if (resources_list.size() != referrers_list.size())
     return;
 
   // Add all sub-resources to wait list.
   for (int i = 0; i < static_cast<int>(resources_list.size()); ++i) {
     const GURL& u = resources_list[i];
     if (!u.is_valid())
       continue;
-    if (unique_urls_to_save_.count(u))
-      continue;
-    unique_urls_to_save_.insert(u);
 
-    SaveFileCreateInfo::SaveFileSource save_source =
-        u.SchemeIsFile() ? SaveFileCreateInfo::SAVE_FILE_FROM_FILE
-                         : SaveFileCreateInfo::SAVE_FILE_FROM_NET;
-    SaveItem* save_item = new SaveItem(u, referrers_list[i], this, save_source);
-    waiting_item_queue_.push(save_item);
+    SaveItem* save_item = url_to_save_item_[u];
+    if (save_item == nullptr) {
+      SaveFileCreateInfo::SaveFileSource save_source =
+          u.SchemeIsFile() ? SaveFileCreateInfo::SAVE_FILE_FROM_FILE
+                           : SaveFileCreateInfo::SAVE_FILE_FROM_NET;
+      url_to_save_item_[u] = save_item = new SaveItem(u, this, save_source);
+      waiting_item_queue_.push_back(save_item);
+    }
+    save_item->set_referrer(referrers_list[i]);
   }
 
-  // Store savable frame_url for later processing.
-  if (frame_url.is_valid())
-    frame_urls_to_save_.push_back(frame_url);
+  // Add the frame to wait list.
+  // TODO(lukasza): Do not dedupe subframes based on url (see crbug.com/538188).
+  if (frame_url.is_valid()) {
+    SaveItem* save_item = url_to_save_item_[frame_url];
+    if (save_item == nullptr) {
+      url_to_save_item_[frame_url] = save_item =
+          new SaveItem(frame_url, this, SaveFileCreateInfo::SAVE_FILE_FROM_DOM);
+      waiting_item_queue_.push_back(save_item);
+    }
+    int frame_tree_node_id = sender->frame_tree_node()->frame_tree_node_id();
+    frame_tree_node_id_to_save_item_[frame_tree_node_id] = save_item;
+  }
 
   CompleteSavableResourceLinksResponse();
 }
 
-void SavePackage::OnSavableResourceLinksError(RenderFrameHost* sender) {
+void SavePackage::OnSavableResourceLinksError(RenderFrameHostImpl* sender) {
   CompleteSavableResourceLinksResponse();
 }
 
 void SavePackage::CompleteSavableResourceLinksResponse() {
   --number_of_frames_pending_response_;
   DCHECK_LE(0, number_of_frames_pending_response_);
   if (number_of_frames_pending_response_ != 0)
     return;  // Need to wait for more responses from RenderFrames.
 
-  // Add frame urls to the waiting_item_queue_.  This is done *after* processing
-  // all savable resource links (i.e. in OnSavableResourceLinksResponse), to
-  // prefer their referrers in cases where the frame url has already been
-  // covered by savable resource links.
-  for (auto& frame_url : frame_urls_to_save_) {
-    DCHECK(frame_url.is_valid());
-    if (0 == unique_urls_to_save_.count(frame_url)) {
-      unique_urls_to_save_.insert(frame_url);
-      SaveItem* save_item = new SaveItem(
-          frame_url, Referrer(), this, SaveFileCreateInfo::SAVE_FILE_FROM_DOM);
-      waiting_item_queue_.push(save_item);
-    }
-  }
-
+  // Sort |waiting_item_queue_| so that frames go last (frames are identified by
+  // SAVE_FILE_FROM_DOM in the comparison function below).
+  std::stable_sort(
+      waiting_item_queue_.begin(), waiting_item_queue_.end(),
+      [](SaveItem* x, SaveItem* y) {
+        DCHECK(x);
+        DCHECK(y);
+        return (x->save_source() != SaveFileCreateInfo::SAVE_FILE_FROM_DOM) &&
+               (y->save_source() == SaveFileCreateInfo::SAVE_FILE_FROM_DOM);
+      });
   all_save_items_count_ = static_cast<int>(waiting_item_queue_.size());
 
   // We use total bytes as the total number of files we want to save.
   // Hack to avoid touching download_ after user cancel.
   // TODO(rdsmith/benjhayden): Integrate canceling on DownloadItem
   // with SavePackage flow.
   if (download_ && (download_->GetState() == DownloadItem::IN_PROGRESS))
     download_->SetTotalBytes(all_save_items_count_);
 
   if (all_save_items_count_) {
@@ skipping 244 matching lines @@
 
 void SavePackage::FinalizeDownloadEntry() {
   DCHECK(download_);
   DCHECK(download_manager_);
 
   download_manager_->OnSavePackageSuccessfullyFinished(download_);
   StopObservation();
 }
 
 }  // namespace content