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 61 matching lines @@
   void ResumeRequest() const override {}
   void CancelRequest() const override {}
   std::string DebugString() const override {
     return "SavePackage DownloadRequestHandle";
   }
 
  private:
   base::WeakPtr<SavePackage> save_package_;
 };
 
+int GetFrameTreeNodeId(RenderFrameHost* render_frame_host) {
+  auto* rfhi = static_cast<RenderFrameHostImpl*>(render_frame_host);
+  return rfhi->frame_tree_node()->frame_tree_node_id();
+}
+
 }  // namespace
 
 const base::FilePath::CharType SavePackage::kDefaultHtmlExtension[] =
     FILE_PATH_LITERAL("html");
 
 SavePackage::SavePackage(WebContents* web_contents,
                          SavePageType save_type,
                          const base::FilePath& file_full_path,
                          const base::FilePath& directory_full_path)
     : WebContentsObserver(web_contents),
       number_of_frames_pending_response_(0),
       file_manager_(NULL),
       download_manager_(NULL),
       download_(NULL),
       page_url_(GetUrlToBeSaved()),
       saved_main_file_path_(file_full_path),
       saved_main_directory_path_(directory_full_path),
       title_(web_contents->GetTitle()),
       start_tick_(base::TimeTicks::Now()),
       finished_(false),
       mhtml_finishing_(false),
       user_canceled_(false),
       disk_error_occurred_(false),
       save_type_(save_type),
       all_save_items_count_(0),
       file_name_set_(&base::FilePath::CompareLessIgnoreCase),
       wait_state_(INITIALIZE),
       contents_id_(web_contents->GetRenderProcessHost()->GetID()),

[ncarter (slow), 2015/10/08 20:57:54]

Not introduced by your cl, but just wanted to put it on your radar: This is
yucky, since two WC's can easily share a RPH. If you've got cycles to fix it (in
a separate patch), I might suggest using the FTN id of the root FrameTreeNode of
the WebContents. It ought to be globally unique and stable over the lifetime of
a tab/WebContents.

[Łukasz Anforowicz, 2015/10/09 16:54:08]

Sure - I'll put that on my TODO list.  OTOH, I see that
SavePackage::content_id() is passed to SaveFileManager and referred to later on
as render_process_id + not used as a key in SaveFileManager's maps (so maybe
this is just a naming issue / maybe this doesn't need to be unique).

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

Looks like it's used as a key in a map here, so there's at least a theoretical
bug here:
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/do...

I'm not super worried about the implications of the bug, but it would be nice to
fix up while we're modernizing this code.

       unique_id_(g_save_package_id++),
       wrote_to_completed_file_(false),
       wrote_to_failed_file_(false) {
   DCHECK(page_url_.is_valid());
   DCHECK((save_type_ == SAVE_PAGE_TYPE_AS_ONLY_HTML) ||
          (save_type_ == SAVE_PAGE_TYPE_AS_MHTML) ||
          (save_type_ == SAVE_PAGE_TYPE_AS_COMPLETE_HTML));
   DCHECK(!saved_main_file_path_.empty() &&
          saved_main_file_path_.value().length() <= kMaxFilePathLength);
   DCHECK(!saved_main_directory_path_.empty() &&
@@ skipping 60 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 550 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 59 matching lines @@
            (save_type_ == SAVE_PAGE_TYPE_AS_MHTML));
     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;

[ncarter (slow), 2015/10/08 20:57:55]

Consider adding here:

RenderFrameHostImpl* sender =
static_cast<RenderFrameHostImpl*>(render_frame_host);

And using that as the argument to WITH_PARAM. This would allow your handler
methods to accept an RFHImpl (if needed), and might let you get rid of helpers
like GetFrameTreeNodeId.

[Łukasz Anforowicz, 2015/10/09 16:54:07]

Done.  Thanks for the suggestion.

   IPC_BEGIN_MESSAGE_MAP_WITH_PARAM(SavePackage, message, render_frame_host)
     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_SerializedLocalPathForSubFrame,
+                        OnSerializedLocalPathForSubFrame)
+    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_);

[ncarter (slow), 2015/10/08 20:57:54]

Another "fyi, no action needed, just want to put this on your radar" comment: 

This DCHECK (which this CL doesn't add) is valid because we don't skip over
crashed subframes or mainframes. You can see this in practice: navigate to
"about:crash" to get a sad tab, and hit Ctrl+S. This creates a save page
operation that'll never terminate.

That's probably not a very serious bug (I think the URL-based deduping issues
you've uncovered are way more serious, for example). But it does seem somewhat
plausible to me, that someone would want to save-page something that has a
crashed subframe.

[Łukasz Anforowicz, 2015/10/09 16:54:07]

Disclaimer: I've introduced this DCHECK in an earlier CL (to explicitly document
that the code assumes that at this point we are waiting for a response from at
least 1 renderer process).

I think handling of missing IPC responses is a problem that might need a broader
/ generic solution.  MHTML [1] tries to detect renderer crashes, but doesn't
consider timeouts [2].  SavePage doesn't account for missing IPC responses at
all.  Ideally both should be able to reuse some kind of a code pattern to
account for slow or crashing renderers.

Also note that a renderer might be killed after we send the IPC, but before it
sends back a response (although I guess one can try to dismiss this as an even
rarer occurrence than a use case of saving a page with an *already* crashed
subframe).

FWIW, I've opened crbug.com/541342 (after testing that the problem occurs before
my changes).

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/do...

[2] OTOH, maybe we don't need to worry about timeouts - there is a separate
check of whether a renderer is regularily sending IPCs and user is given an
option of killing the renderer when it is slow.

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

I agree with everything in your reply. I don't think we should bother with
timeouts or anything to be resilient against unresponsive renderers: the hang
monitor watchdog is the browser-wide solution for that.

Engineering effort around crashed and hung renderers should be limited to the
following: (a) never hang or jank the main browser UI, even if we're stuck
waiting for a reply that never comes (b) cancellation of operations should work
even in the presence of unresponsive renderers (c) we don't leak any
browser-side resources if a renderer crashes mid-operation.

 }
 
 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(RenderFrameHost* 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 = GetFrameTreeNodeId(sender);
+  SaveItem* save_item = frame_tree_node_id_to_save_item_[frame_tree_node_id];

[ncarter (slow), 2015/10/08 20:57:55]

What if it's not in the map?

[Łukasz Anforowicz, 2015/10/09 16:54:07]

Good point.  Thanks for catching this.

When thinking how to handle this, I've considered for a moment having a check in
GetSerializedHtmlWithLocalLinksForFrame to see if the frame is present in the
map.  OTOH, this wouldn't have helped in all the cases (i.e. when getting an IPC
from a misbehaving renderer), so I think we should just return here and avoid
complicating GetSerializedHtmlWithLocalLinksForFrame.

Also - I've opened crbug.com/541354 to explicitly track the raciness.

Done?  WDYT?

+  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::OnSerializedLocalPathForSubFrame(
+    RenderFrameHost* sender,
+    int render_frame_or_proxy_routing_id) {
+  if (wait_state_ != HTML_DATA)
+    return;
+
+  FrameTreeNode* frame_tree_node =
+      static_cast<WebContentsImpl*>(web_contents())
+          ->GetFrameTree()

[ncarter (slow), 2015/10/08 20:57:54]

If you passed in an RFHImpl here, you could alternatively get this via
sender->frame_tree_node()->frame_tree(). There's actually a subtle difference
between the two: when an interstitial is showing, it creates a second frame tree
for its content, so in code that needs to work with interstitial RFH's, going to
the WebContents would be a mistake. For save page, the difference doesn't
matter, since save-page seems to be disabled while an interstitial is showing.

[Łukasz Anforowicz, 2015/10/09 16:54:07]

Done (to have a safer code in case somebody wants to base his/her code on this).

+          ->FindByRoutingID(sender->GetProcess()->GetID(),
+                            render_frame_or_proxy_routing_id);
+  if (!frame_tree_node)
+    return;

[ncarter (slow), 2015/10/08 20:57:55]

Would it be worthwhile here to validate that frame_tree_node->parent() ==
sender->frame_tree_node()? 

[Łukasz Anforowicz, 2015/10/09 16:54:08]

Done.

+
+  SaveItem* save_item =
+      frame_tree_node_id_to_save_item_[frame_tree_node->frame_tree_node_id()];

[ncarter (slow), 2015/10/08 20:57:55]

What if it's not in the map?

[Łukasz Anforowicz, 2015/10/09 16:54:08]

It's ok in this case - this will be handled by SerializeLocalPathForSaveItem
(which will use the fallback url in this case).

+  SerializeLocalPathForSaveItem(sender, save_item,
+                                frame_tree_node->current_url());
+}
+
+void SavePackage::OnSerializedLocalPathForSavableResource(
+    RenderFrameHost* sender,
+    const GURL& savable_resource) {
+  if (wait_state_ != HTML_DATA)
+    return;
+  if (!savable_resource.is_valid())
+    return;
+
+  SaveItem* save_item = url_to_save_item_[savable_resource];
+  SerializeLocalPathForSaveItem(sender, save_item, savable_resource);
+}
+
+void SavePackage::OnSerializedEndOfFrame(RenderFrameHost* sender) {
+  if (wait_state_ != HTML_DATA)
+    return;
+
+  int frame_tree_node_id = GetFrameTreeNodeId(sender);
+  SaveItem* save_item = frame_tree_node_id_to_save_item_[frame_tree_node_id];
+  if (save_item->state() != SaveItem::IN_PROGRESS) {
+    LogWriteToAlreadyCompletedOrFailedSaveItem(save_item);
+    return;
+  }
+
+  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_--;

[ncarter (slow), 2015/10/08 20:57:55]

Shouldn't save_item()->state() be transitioned to something else here (as a
defense against OnSerializedEndOfFrame being sent multiple times for a frame)?

When I'm looking at browser-process code like this, I'm always asking the
question: could an attacker-controlled renderer process get me to do something I
didn't intend here? One broad defense for that is to avoid processing a response
message if we can't match it up to a request.

[Łukasz Anforowicz, 2015/10/09 16:54:08]

I've spent 10-15 minutes thinking that SaveItem::state_ might be getting
accessed outside of the browser UI thread, but couldn't find any such case. 
I've added DCHECKs to this point.

I guess it makes sense to add a COMPLETING state.

Done (I think).

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

Gosh, I hope not. I'm glad you added the UI thread DCHECKs in addition to the
COMPLETING state.

+  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(RenderFrameHost* 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 {
+    attr_value = fallback_url.spec();

[ncarter (slow), 2015/10/08 21:04:14]

When does this happen? Could you add a comment explaining why we need it?

[Łukasz Anforowicz, 2015/10/09 16:54:08]

Done.

+  }
+
+  // TODO(lukasza): Escape attr_value (" + &).

[ncarter (slow), 2015/10/08 20:57:55]

My understanding is that there are no security considerations here, really,
since the renderer can by design write arbitrary content to the file.

But escaping could matter for correctness, if URL-important characters (like #
or ?) or context-important characters (like ") were to find their way into
saved_main_directory_path_ or save_item->file_name(). So, what kind of escaping
needs to happen here, exactly? Should we pass it through GURL for
canonicalization?

I'm OK with leaving this as a TODO, provided a CL to address it is right around
the corner. Doubly okay if that CL includes unittests that exercise the weird
cases (filename-reserved chars, url chars, PRN, COM9, etc). Over-escaping is
just as likely to be a cause of trouble as under-escaping, and it seems like
there's already some canonicalization code in place.

[Łukasz Anforowicz, 2015/10/09 16:54:08]

Hmmm... thinking about it some more, I've realized that this code is not
landable for 2 reasons:

1. It also handles fallback url (which doesn't go through filename sanitization)

2. This doesn't do encoding equivalent to
WebPageSerializerImpl::encodeAndFlushBuffer:

2a. I am not quite sure what will happen to characters outside of ASCII /
whether they will match file encoding.

2b. I see that encoding from #2 somehow handles quotes (not sure yet how, but
they get escaped via %).  Interestingly #2 still doesn't handle amperstands.

Let's get the other code review feedback out of the way and I'll work on this
issue next...

More notes:

- Not sure if the browser has any html escaping utilities - last time I checked
most utilities of this kind were in the blink layer...

- We probably need to keep in mind that if the escaping ends up being more
complicated then it will be an additional attack surface the renderer can
exploit...

+  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(
@@ skipping 16 matching lines @@
     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) {

[ncarter (slow), 2015/10/08 20:57:55]

You seem to be preferring operator[] throughout instead of map::find(), which
means that a failed lookup can wind up with a nullptr getting into the map.  Why
is that? With the auto keyword, you can do it pretty concisely, and for
readability, you can store it->second into a SaveItem* variable once you've
verified it != map.end().

Here, even though the null value is replaced a couple lines later, I did a
double-take before realizing that's what was going on -- it's not a pattern I'm
used to seeing.

[Łukasz Anforowicz, 2015/10/09 16:54:07]

I've changed to find in some places, but when I tried this in this case , it led
to more complicated code (because we want to call set_referrer when the item is
already present *and* when it is missing).  So I think I'll just leave the code
as-is (since it won't leave weird nullptr entries in the map [unlike the other
places in this cc file]).

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

I'm OK with operator[] in these cases, since it's safe. But FWIW, if you wanted
to use iterators, the pattern I'd use is:

auto it = url_to_save_item_.find(u);
if (it == url_to_save_item_.end()) {
  it = map.insert(it, std::make_pair(u, new SaveItem(..)));
}
SaveItem* save_item = it->second;
save_item->set_referrer(...);

+      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);
+    }
+    frame_tree_node_id_to_save_item_[GetFrameTreeNodeId(sender)] = save_item;
+  }
 
   CompleteSavableResourceLinksResponse();
 }
 
 void SavePackage::OnSavableResourceLinksError(RenderFrameHost* 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.
+  std::sort(

[ncarter (slow), 2015/10/08 20:57:55]

I wonder if we ought to use stable_sort here to avoid a possible source of
nondeterminism / cross-platform differences? (this assumes, of course, that
waiting_item_queue_ is already in some kind of relative tree-order, which may
not be the case at all? If the order is random going in, sort() won't make it
worse)

[Łukasz Anforowicz, 2015/10/09 16:54:07]

There is a partial order here (things from the same frame are next to each other
+ things from the same order are in the order they were reported by the frame). 
So it probably makes sense to preserve some if it via stable_sort.

Done.

+      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);

[ncarter (slow), 2015/10/08 20:57:55]

Would this be easier to comprehend if we wrote it as:

x != y && y == SAVE_FILE_FROM_DOM ?

(I'm honestly not sure. I was confused at first by your version, but once I
figured it out, it looks obvious. Leaving it up to you.)

[Łukasz Anforowicz, 2015/10/09 16:54:08]

I think your comparison would be broken as it would say "false" for both x<y and
y<x if both x and y were SAVE_FILE_FROM_DOM.

At the same time, you have a point.  I am not sure if it helps, but I've added a
comment clarifying what it means that frames go last.

+      });
   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