OOPIFs: Transitioning Get/Send...SavableResourceLinks away from RenderViewHost.

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"
@@ skipping 11 matching lines @@
 #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/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/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"
@@ skipping 287 matching lines @@
   download_ = item;
   download_->AddObserver(this);
   // Confirm above didn't delete the tab out from under us.
   if (!download_created_callback.is_null())
     download_created_callback.Run(download_);
 
   // Check save type and process the save page job.
   if (save_type_ == SAVE_PAGE_TYPE_AS_COMPLETE_HTML) {
     // Get directory
     DCHECK(!saved_main_directory_path_.empty());
-    GetAllSavableResourceLinksForCurrentPage();
+    GetSavableResourceLinksForCurrentPage();
   } 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_;

[nasko, 2015/09/02 23:45:55]

A side note. Do we behave correctly when saving only HTML? I'm suspecting no,
but wanted to ask if you've checked.

[Łukasz Anforowicz, 2015/09/03 16:59:57]

I haven't tested html-only.  I need to do this (eventually - outside of the
current CL under review).  I hope that this will simply work.

Also - I haven't yet tested mhtml.  I think this will fail with OOPIFs (i.e. I
see mhtml-related messages for RV/RVH).

[nasko, 2015/09/03 18:17:01]

MHTML is almost definitely going to be broken :). HTML-only save might actually
just work, based on some test saves I just did, which is nice!

     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);
     // Add this item to waiting list.
     waiting_item_queue_.push(save_item);
@@ skipping 641 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) {
   bool handled = true;
   IPC_BEGIN_MESSAGE_MAP(SavePackage, message)
-    IPC_MESSAGE_HANDLER(ViewHostMsg_SendCurrentPageAllSavableResourceLinks,
-                        OnReceivedSavableResourceLinksForCurrentPage)
     IPC_MESSAGE_HANDLER(ViewHostMsg_SendSerializedHtmlData,
                         OnReceivedSerializedHtmlData)
     IPC_MESSAGE_UNHANDLED(handled = false)
   IPC_END_MESSAGE_MAP()
   return handled;
 }
 
+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)
+    IPC_MESSAGE_HANDLER(FrameHostMsg_SavableResourceLinksResponse,
+                        OnReceivedSavableResourceLinksForFrame)
+    IPC_MESSAGE_HANDLER(FrameHostMsg_NonSavableResponse,
+                        OnReceivedNonSavableFrameIndication)
+    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::GetSerializedHtmlDataForCurrentPageWithLocalLinks() {
   if (wait_state_ != HTML_DATA)
     return;
   std::vector<GURL> saved_links;
   std::vector<base::FilePath> saved_file_paths;
   int successful_started_items_count = 0;
@@ skipping 110 matching lines @@
                    file_manager_,
                    save_item->save_id(),
                    save_item->url(),
                    id,
                    true));
   }
 }
 
 // Ask for all savable resource links from backend, include main frame and
 // sub-frame.
-void SavePackage::GetAllSavableResourceLinksForCurrentPage() {
+void SavePackage::GetSavableResourceLinksForCurrentPage() {
   if (wait_state_ != START_PROCESS)
     return;
 
+  WebContents* web_contents = WebContentsObserver::web_contents();

[nasko, 2015/09/02 23:45:55]

This object is already WebContentsObserver, so no need to prefix the method
call. Also, being an observer should guarantee this object will not outlive the
WebContents.

[Łukasz Anforowicz, 2015/09/03 16:59:57]

This is tricky.  I need the prefix, because otherwise the compiler will think
that I want to invoke the local |web_contents| variable, rather than calling the
inherited web_contents method.
Hmm... I thought that there is no such guarantee, because I saw that destructor
of WebContentsImpl has this code:

  FOR_EACH_OBSERVER(WebContentsObserver,
                    observers_,
                    ResetWebContents());

which ends up causing WebContentsObserver::web_contents() to return nullptr.

Does this make sense?  What am I missing?

[nasko, 2015/09/03 18:17:01]

Why do you need a local web_contents variable is the main question?
s/web_contents/web_contents()/ seems equivalent to me.
WebContentsImpl also destroys all the observers as part of its cleaning up the
observers_ list. This ends up deleting all objects and the destructor for
WebContentsObserver is virtual, so the inherited objects should be deleted by
default, unless they override the destructor to do something else.

[Łukasz Anforowicz, 2015/09/03 19:41:24]

Oh, right...  Done. :-)
Thanks for explaining.

+  if (!web_contents) {
+    // Tab got closed - treat it as user cancel.
+    Cancel(true);
+    return;
+  }
+
   wait_state_ = RESOURCES_LIST;
-  Send(new ViewMsg_GetAllSavableResourceLinksForCurrentPage(routing_id(),
-                                                            page_url_));
+  number_of_frames_with_pending_get_savable_resource_links_ = 0;
+  web_contents->ForEachFrame(base::Bind(
+      &SavePackage::GetSavableResourceLinksForFrame,
+      base::Unretained(this)));  // Safe, because ForEachFrame is synchronous.

[nasko, 2015/09/02 23:45:55]

Thanks for the comment!

[Łukasz Anforowicz, 2015/09/03 16:59:56]

Acknowledged.

+
+  if (0 == number_of_frames_with_pending_get_savable_resource_links_) {

[nasko, 2015/09/02 23:45:55]

Is this condition possible? If not, let's use a DCHECK instead of handling it in
code.

[Łukasz Anforowicz, 2015/09/03 16:59:56]

Done.  I wasn't sure if WebContents is guaranteed to have at least 1 frame, but
looking at FrameTree::root_, I think this can indeed be a DCHECK.  Thanks.

+    // No frames in the web contents - treat it as user cancel.
+    Cancel(true);
+    return;
+  }
 }
 
-// Give backend the lists which contain all resource links that have local
-// storage, after which, render process will serialize DOM for generating
-// HTML data.
-void SavePackage::OnReceivedSavableResourceLinksForCurrentPage(
+void SavePackage::GetSavableResourceLinksForFrame(
+    RenderFrameHost* render_frame_host) {
+  number_of_frames_with_pending_get_savable_resource_links_++;
+  Send(new FrameMsg_GetSavableResourceLinks(render_frame_host->GetRoutingID()));
+}
+
+void SavePackage::OnReceivedSavableResourceLinksForFrame(
+    RenderFrameHost* render_frame_host,
     const std::vector<GURL>& resources_list,
-    const std::vector<Referrer>& referrers_list,
-    const std::vector<GURL>& frames_list) {
+    const std::vector<Referrer>& referrers_list) {
   if (wait_state_ != RESOURCES_LIST)
     return;
 
   if (resources_list.size() != referrers_list.size())
     return;
 
-  all_save_items_count_ = static_cast<int>(resources_list.size()) +
-                           static_cast<int>(frames_list.size());
+  // 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];
+    DCHECK(u.is_valid());
+    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);
+  }
+
+  // Add the frame to wait list.
+  GURL frame_url = render_frame_host->GetLastCommittedURL();

[nasko, 2015/09/02 23:45:55]

Hmm, I wonder if this can be racy. Might be useful to include the URL of the
document in the renderer response and bail out of this method if the last
committed URL doesn't match the one coming from the renderer.

[Łukasz Anforowicz, 2015/09/03 16:59:56]

Right.  I tried to allude to this in the CL description.  We do need to discuss
this.  OTOH, maybe let's postpone discussing this until the CL is ready for
review with with asanka@ (i.e. until we flush out generic, or OOPIF-specific CR
issues).

Some discussion points for url lock (let's keep Referrer discussion in a
separate comments thread):

- Locking top-level url, vs urls of all frames (old code only did the former)

- Expected save-page-as behavior for a page that has some auto-refreshing
iframes

- Does WebContentsImpl::Stop (called from WebContentsImpl::OnSave / propagated
into StopLoading for each frame in FTN) prevent *future* navigations / url
changes?

+  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);
+  }
+
+  CompleteSavableResourceLinksResponseFromFrame();
+}
+
+void SavePackage::OnReceivedNonSavableFrameIndication(
+    RenderFrameHost* render_frame_host) {
+  CompleteSavableResourceLinksResponseFromFrame();
+}
+
+void SavePackage::CompleteSavableResourceLinksResponseFromFrame() {
+  --number_of_frames_with_pending_get_savable_resource_links_;
+  if (0 < number_of_frames_with_pending_get_savable_resource_links_) {

[nasko, 2015/09/02 23:45:55]

We should never go negative, should we?

[Łukasz Anforowicz, 2015/09/03 16:59:57]

Correct - we should never go negative.  This is enforced by the DCHECK below.

Slightly related question - the code assumes that I will eventually get a
response from *each* RF/FH pair (and SavePackage will "hung" otherwise).  Not
sure if this is okay (and what to do about it if this is not ok).

[nasko, 2015/09/03 18:17:01]

If we never should go negative, then we shouldn't be adding logic to handle
this. A DCHECK indicating expectation is all we need. If we return early, we are
peppering over a bug, which is not the approach Chromium takes.

[Łukasz Anforowicz, 2015/09/03 19:41:24]

I rewrote the DCHECK and "if" to make it more obvious that they check that
number_of_frames_... is *greater* than 0 (not less than 0).

+    return;
+  }
+  DCHECK_EQ(0, number_of_frames_with_pending_get_savable_resource_links_);

[nasko, 2015/09/02 23:45:55]

It seems to me that this DCHECK can easily hit. This method is called on each
frame response, so if you have more than one frame and the first one responds,
the number will not be zero, right?

[Łukasz Anforowicz, 2015/09/03 16:59:56]

If the number is not zero, then we will return on line 1234, right?  (wow, nice
line number :-).

[nasko, 2015/09/03 18:17:01]

Since that if statement should disappear, the DCHECK will have to change. Also,
the if statement guarantees that the value is not less than zero, but why can't
it be positive? This method is called unconditionally on each response, no?

[Łukasz Anforowicz, 2015/09/03 19:41:24]

See the previous comment.

+
+  all_save_items_count_ = static_cast<int>(unique_urls_to_save_.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_) {
-    // Put all sub-resources to wait list.
-    for (int i = 0; i < static_cast<int>(resources_list.size()); ++i) {
-      const GURL& u = resources_list[i];
-      DCHECK(u.is_valid());
-      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);
-    }
-    // Put all HTML resources to wait list.
-    for (int i = 0; i < static_cast<int>(frames_list.size()); ++i) {
-      const GURL& u = frames_list[i];
-      DCHECK(u.is_valid());
-      SaveItem* save_item = new SaveItem(
-          u, Referrer(), this, SaveFileCreateInfo::SAVE_FILE_FROM_DOM);
-      waiting_item_queue_.push(save_item);
-    }
     wait_state_ = NET_FILES;
+
+    // Give backend the lists which contain all resource links that have local
+    // storage, after which, render process will serialize DOM for generating
+    // HTML data.
     DoSavingProcess();
   } else {
     // No resource files need to be saved, treat it as user cancel.
     Cancel(true);
   }
 }
 
 base::FilePath SavePackage::GetSuggestedNameForSaveAs(
     bool can_save_as_complete,
     const std::string& contents_mime_type,
@@ skipping 229 matching lines @@
 
 void SavePackage::FinalizeDownloadEntry() {
   DCHECK(download_);
   DCHECK(download_manager_);
 
   download_manager_->OnSavePackageSuccessfullyFinished(download_);
   StopObservation();
 }
 
 }  // namespace content