OOPIFs: Transitioning MHTML generation from view-oriented to frame-oriented.

content/browser/download/mhtml_generation_manager.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/mhtml_generation_manager.h"
 
+#include <map>
+#include <queue>
+
 #include "base/bind.h"
 #include "base/files/file.h"
+#include "base/guid.h"
+#include "base/rand_util.h"
 #include "base/scoped_observer.h"
 #include "base/stl_util.h"
-#include "content/browser/renderer_host/render_view_host_impl.h"
+#include "base/strings/string_number_conversions.h"
+#include "base/strings/stringprintf.h"
+#include "content/browser/frame_host/frame_tree_node.h"
+#include "content/common/frame_messages.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/render_process_host_observer.h"
 #include "content/public/browser/web_contents.h"
-#include "content/common/view_messages.h"
 
 namespace content {
 
 // The class and all of its members live on the UI thread.  Only static methods
 // are executed on other threads.
 class MHTMLGenerationManager::Job : public RenderProcessHostObserver {
  public:
   Job(int job_id, WebContents* web_contents, GenerateMHTMLCallback callback);
   ~Job() override;
 
   void set_browser_file(base::File file) { browser_file_ = file.Pass(); }
 
   GenerateMHTMLCallback callback() const { return callback_; }
 
-  // Sends IPC to the renderer, asking for MHTML generation.
+  // Sends IPC to the renderer, asking for MHTML generation of the next frame.
+  //
   // Returns true if the message was sent successfully; false otherwise.
-  bool SendToRenderView();
+  bool SendToNextRenderFrame();
+
+  // Indicates if more calls to SendToNextRenderFrame are needed.
+  bool HasMoreFramesToProcess() const {
+    return !pending_frame_tree_node_ids_.empty();
+  }
 
   // Close the file on the file thread and respond back on the UI thread with
   // file size.
   void CloseFile(base::Callback<void(int64 file_size)> callback);
 
   // RenderProcessHostObserver:
   void RenderProcessExited(RenderProcessHost* host,
                            base::TerminationStatus status,
                            int exit_code) override;
   void RenderProcessHostDestroyed(RenderProcessHost* host) override;
 
  private:
+  static std::string GenerateMHTMLBoundaryMarker();
   static int64 CloseFileOnFileThread(base::File file);
+  void AddFrame(RenderFrameHost* render_frame_host);
+
+  // Translates |frame_tree_node_to_content_id_| into
+  // a |site_instance|-specific, routing-id-based map.

[Randy Smith (Not in Mondays), 2015/12/10 23:07:55]

nit: My impression from reading the code is the primary purpose of this function
is to filter frame_tree_node_to_content_id_ down to just the frames relevant to
a site instance, and then secondarily making it a routing-id based map instead
of a FTNID based map.  If you agree, would you be willing to call out the
filtering functionality in the comment? 

[lukasza, 2015/12/11 00:29:03]

I've tried rewording the comment to make it clearer what is happening:

  // Creates a new map with values (content ids) the same as in
  // |frame_tree_node_to_content_id_| map, but with the keys translated from
  // frame_tree_node_id into a |site_instance|-specific routing_id.

Let me try to explain further below, and if you think this is important, I can
add that to the comment (I didn't add it for now, because it drowns the main
message of the comment in technical details that are borderline implementation
details):

  // frame_tree_node_id uniquely identifies a frame.  The frame might need to
  // be represented in multiple renderer processes (as either [local]
  // RenderFrame or [remote] RenderFrameProxy) and routing ids only make
  // sense only in the context of a process / belong to process-specific
  // RenderFrame/RenderFrameProxy objects.  We can get the right routing_id,
  // by looking at a frame through the lenses/perspective of a |site_instance|.

See also my reply to the next comment.

[Randy Smith (Not in Mondays), 2015/12/14 02:47:04]

Yeah, I think I was mis-understanding the functionality.  Sounds good.

+  std::map<int, std::string> CreateFrameRoutingIdToContentId(
+      SiteInstance* site_instance);
 
   // Id used to map renderer responses to jobs.
   // See also MHTMLGenerationManager::id_to_job_ map.
   int job_id_;
 
   // The handle to the file the MHTML is saved to for the browser process.
   base::File browser_file_;
 
-  // The IDs mapping to a specific contents.
-  int process_id_;
-  int routing_id_;
+  // The IDs of frames we still need to process.
+  std::queue<int> pending_frame_tree_node_ids_;
+
+  // Map from frames into content ids (see WebPageSerializer::generateMHTMLParts
+  // for more details about what "content ids" are and how they are used).
+  std::map<int, std::string> frame_tree_node_to_content_id_;
+
+  // MIME multipart boundary to use in the MHTML doc.
+  std::string mhtml_boundary_marker_;
 
   // The callback to call once generation is complete.
   GenerateMHTMLCallback callback_;
 
   // RAII helper for registering this Job as a RenderProcessHost observer.
   ScopedObserver<RenderProcessHost, MHTMLGenerationManager::Job>
       observed_renderer_process_host_;
 
   DISALLOW_COPY_AND_ASSIGN(Job);
 };
 
 MHTMLGenerationManager::Job::Job(int job_id,
                                  WebContents* web_contents,
                                  GenerateMHTMLCallback callback)
     : job_id_(job_id),
-      process_id_(web_contents->GetRenderProcessHost()->GetID()),
-      routing_id_(web_contents->GetRenderViewHost()->GetRoutingID()),
+      mhtml_boundary_marker_(GenerateMHTMLBoundaryMarker()),
       callback_(callback),
       observed_renderer_process_host_(this) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  web_contents->ForEachFrame(base::Bind(
+      &MHTMLGenerationManager::Job::AddFrame,
+      base::Unretained(this)));  // Safe because ForEachFrame is synchronous.
+
+  // Main frame needs to be processed first.
+  DCHECK(!pending_frame_tree_node_ids_.empty());
+  DCHECK(FrameTreeNode::GloballyFindByID(pending_frame_tree_node_ids_.front())
+             ->parent() == nullptr);
 }
 
 MHTMLGenerationManager::Job::~Job() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 }
 
-bool MHTMLGenerationManager::Job::SendToRenderView() {
-  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+std::map<int, std::string>
+MHTMLGenerationManager::Job::CreateFrameRoutingIdToContentId(
+    SiteInstance* site_instance) {

[Randy Smith (Not in Mondays), 2015/12/10 23:07:55]

nit, suggestion: The only place in this file that appears to use SiteInstance is
in the interface to this function; elsewhere it's RenderFrameHost & similar at
that level.  Reasonable to make this take a RFH instead of a SiteInstance and
hide the need to access the site instance behind this interface?

[lukasza, 2015/12/11 00:29:03]

There are 2 frames: frame_A and frame_B.  We want to get routing_id of frame_B
that will work in the process that will handle messages for frame_A (process
where frame_A is a local frame).  I feel it is ok to say "give me routing id of
frame B in the context of site instance foo.com", but it feels like a layering
violation to say "give me routing id of frame B in the context of frame A",
because it really means "give me routing id of frame B in the context of site
instance where frame A is a local frame" (i.e. I don't think "routing id in the
context of frame A" alone, has a clear, specific meaning).

[Randy Smith (Not in Mondays), 2015/12/14 02:47:04]

Yeah, I think I didn't have a full appreciation of what this function was doing.
 I'm still not completely sure I do, but given what you're saying the name &
arguments make sense.

+  std::map<int, std::string> result;
+  for (const auto& it : frame_tree_node_to_content_id_) {
+    int ftn_id = it.first;
+    const std::string& content_id = it.second;
+
+    FrameTreeNode* ftn = FrameTreeNode::GloballyFindByID(ftn_id);
+    if (!ftn)
+      continue;
+
+    int routing_id =
+        ftn->render_manager()->GetRoutingIdForSiteInstance(site_instance);
+    if (routing_id == MSG_ROUTING_NONE)
+      continue;
+
+    result[routing_id] = content_id;
+  }
+  return result;
+}
+
+bool MHTMLGenerationManager::Job::SendToNextRenderFrame() {
   DCHECK(browser_file_.IsValid());
+  DCHECK_LT(0u, pending_frame_tree_node_ids_.size());
 
-  RenderViewHost* rvh = RenderViewHost::FromID(process_id_, routing_id_);
-  if (!rvh) {  // The contents went away.
+  int frame_tree_node_id = pending_frame_tree_node_ids_.front();
+  pending_frame_tree_node_ids_.pop();
+  bool is_last_frame = pending_frame_tree_node_ids_.empty();
+
+  FrameTreeNode* ftn = FrameTreeNode::GloballyFindByID(frame_tree_node_id);
+  if (!ftn)  // The contents went away.
     return false;
-  }
+  RenderFrameHost* rfh = ftn->current_frame_host();
 
-  observed_renderer_process_host_.Add(rvh->GetMainFrame()->GetProcess());
+  // Get notified if the target of the IPC message dies between responding.
+  observed_renderer_process_host_.RemoveAll();
+  observed_renderer_process_host_.Add(rfh->GetProcess());
 
   IPC::PlatformFileForTransit renderer_file = IPC::GetFileHandleForProcess(
-      browser_file_.GetPlatformFile(), rvh->GetProcess()->GetHandle(),
+      browser_file_.GetPlatformFile(), rfh->GetProcess()->GetHandle(),
       false);  // |close_source_handle|.
-  rvh->Send(
-      new ViewMsg_SavePageAsMHTML(rvh->GetRoutingID(), job_id_, renderer_file));
+  rfh->Send(new FrameMsg_SerializeAsMHTML(
+      rfh->GetRoutingID(), job_id_, renderer_file, mhtml_boundary_marker_,
+      CreateFrameRoutingIdToContentId(rfh->GetSiteInstance()), is_last_frame));
   return true;
 }
 
 void MHTMLGenerationManager::Job::RenderProcessExited(
     RenderProcessHost* host,
     base::TerminationStatus status,
     int exit_code) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   MHTMLGenerationManager::GetInstance()->RenderProcessExited(this);
 }
 
+void MHTMLGenerationManager::Job::AddFrame(RenderFrameHost* render_frame_host) {
+  auto* rfhi = static_cast<RenderFrameHostImpl*>(render_frame_host);

[Randy Smith (Not in Mondays), 2015/12/10 23:07:55]

I just want to confirm that someone familiar with the RenderFrameHost class
structure in content signed off on this code?  This makes me nervous, but it may
well be perfectly fine given content/ idioms.

[lukasza, 2015/12/11 00:29:02]

Yup - that would be nick@.  AFAIK the cast is ok within //content/browser

+  int frame_tree_node_id = rfhi->frame_tree_node()->frame_tree_node_id();
+  pending_frame_tree_node_ids_.push(frame_tree_node_id);
+
+  std::string guid = base::GenerateGUID();
+  std::string content_id = base::StringPrintf("<frame-%d-%s@mhtml.blink>",
+                                              frame_tree_node_id, guid.c_str());
+  frame_tree_node_to_content_id_[frame_tree_node_id] = content_id;
+}
+
 void MHTMLGenerationManager::Job::RenderProcessHostDestroyed(
     RenderProcessHost* host) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   observed_renderer_process_host_.Remove(host);
 }
 
 void MHTMLGenerationManager::Job::CloseFile(
     base::Callback<void(int64)> callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
@@ skipping 11 matching lines @@
 
 // static
 int64 MHTMLGenerationManager::Job::CloseFileOnFileThread(base::File file) {
   DCHECK_CURRENTLY_ON(BrowserThread::FILE);
   DCHECK(file.IsValid());
   int64 file_size = file.GetLength();
   file.Close();
   return file_size;
 }
 
+// static
+std::string MHTMLGenerationManager::Job::GenerateMHTMLBoundaryMarker() {

[Randy Smith (Not in Mondays), 2015/12/10 23:07:55]

Do you need me to learn enough about MIME/MHTML to properly review this
function, or can I assume one of the other reviewers did so?

[lukasza, 2015/12/11 00:29:03]

I think this is fine as-is:
1. I copied it from third_party/WebKit/Source/platform/mhtml/MHTMLArchive.cpp
2. It seems that the MIME specs/rfcs don't really put any restrictions on the
string used as a mime multipart boundary (other than having 2 dashes in the
front + restricting the characterset somewhat).

+  // TODO(lukasza): Introduce and use a shared helper function in
+  // net/base/mime_util.h instead of having the ad-hoc code below.
+
+  // Trying to generate random boundaries similar to IE/UnMHT
+  // (ex: ----=_NextPart_000_001B_01CC157B.96F808A0).
+  uint8_t random_values[10];
+  base::RandBytes(&random_values, sizeof(random_values));
+
+  std::string result("----=_NextPart_000_");
+  result += base::HexEncode(random_values + 0, 2);
+  result += '_';
+  result += base::HexEncode(random_values + 2, 4);
+  result += '.';
+  result += base::HexEncode(random_values + 6, 4);
+  return result;
+}
+
 MHTMLGenerationManager* MHTMLGenerationManager::GetInstance() {
   return base::Singleton<MHTMLGenerationManager>::get();
 }
 
 MHTMLGenerationManager::MHTMLGenerationManager() : next_job_id_(0) {}
 
 MHTMLGenerationManager::~MHTMLGenerationManager() {
   STLDeleteValues(&id_to_job_);
 }
 
 void MHTMLGenerationManager::SaveMHTML(WebContents* web_contents,
                                        const base::FilePath& file_path,
                                        const GenerateMHTMLCallback& callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   int job_id = NewJob(web_contents, callback);
 
   BrowserThread::PostTaskAndReplyWithResult(
       BrowserThread::FILE, FROM_HERE,
       base::Bind(&MHTMLGenerationManager::CreateFile, file_path),
       base::Bind(&MHTMLGenerationManager::OnFileAvailable,
                  base::Unretained(this),  // Safe b/c |this| is a singleton.
                  job_id));
 }
 
 void MHTMLGenerationManager::OnSavedPageAsMHTML(

[Randy Smith (Not in Mondays), 2015/12/10 23:07:55]

Hmmm.  Given this change, I'm uncomfortable with this function name.  It's
really "OnSavedFrameAsMHTML" now; reasonable to change the name?

[lukasza, 2015/12/11 00:29:02]

Good catch.  Yes - it should be "frame", not "page" (and I also have a todo to
rename WebPageSerializer and PageSerializer to "frame"-named-equivalents).

Done.  (there was also a stale comment with page-related name of an IPC message)

     int job_id,
     bool mhtml_generation_in_renderer_succeeded) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  JobStatus job_status = mhtml_generation_in_renderer_succeeded
-                             ? JobStatus::SUCCESS
-                             : JobStatus::FAILURE;
-  JobFinished(job_id, job_status);
+
+  if (!mhtml_generation_in_renderer_succeeded) {
+    JobFinished(job_id, JobStatus::FAILURE);
+    return;
+  }
+
+  Job* job = FindJob(job_id);
+  if (!job)
+    return;
+
+  if (job->HasMoreFramesToProcess()) {
+    if (!job->SendToNextRenderFrame()) {
+      JobFinished(job_id, JobStatus::FAILURE);
+    }
+    return;
+  }
+
+  JobFinished(job_id, JobStatus::SUCCESS);
 }
 
 // static
 base::File MHTMLGenerationManager::CreateFile(const base::FilePath& file_path) {
   DCHECK_CURRENTLY_ON(BrowserThread::FILE);
-  base::File browser_file(
-      file_path, base::File::FLAG_CREATE_ALWAYS | base::File::FLAG_WRITE);
+
+  // SECURITY NOTE: A file descriptor to the file created below will be passed
+  // to multiple renderer processes which (in out-of-process iframes mode) can
+  // act on behalf of separate web principals.  Therefore it is important to
+  // only allow writing to the file and forbid reading from the file (as this
+  // would allow reading content generated by other renderers / other web
+  // principals).
+  uint32 file_flags = base::File::FLAG_CREATE_ALWAYS | base::File::FLAG_WRITE;

[Randy Smith (Not in Mondays), 2015/12/10 23:07:55]

Can the renderer seek in the file?  If it can, that means it could overwrite
things written by other frames.  This is probably ok--I don't think revisiting
the MHTML would re-give those frames privileges to that origin--but I wanted to
raise the issue in case I was wrong.

[lukasza, 2015/12/11 00:29:02]

Thank you for catching this.  It does seem that base::File::FLAG_APPEND is
better because on POSIX it gets translated to O_APPEND | O_WRONLY and for
O_APPEND docs say "if set, the file offset shall be set to the end of the file
prior to each write".  Note that FLAG_WRITE and FLAG_APPEND have to be used
together (otherwise we hit a DCHECK in file_posix.cc(438) - check failed: flags
& FLAG_WRITE.).

Done.  OTOH, note that I still need to retest on Windows... and I don't really
understand what file_posix.cc and file_win.cc try to do wrt requiring (posix) or
disallowing (win?) write+append together.  Maybe I'll have to revert this
particular change if it blows up on debug flavor of a windows build.

[Randy Smith (Not in Mondays), 2015/12/14 02:47:04]

Sounds good; I'm happy to leave that to you.  I don't think this is a security
issue as there are almost certainly more interesting things that a malicious
renderer could do with an MHTML output request, but if O_APPEND | O_WRONLY can
be made to work, that's probably better.

[Łukasz Anforowicz, 2015/12/14 18:08:42]

I've removed FLAG_APPEND in the latest patchset.  AFAICT using this flag (with
FLAG_CREATE_ALWAYS) in a way that doesn't DCHECK on either Posix or Windows
would requires changes under base/files/... (i.e. I've run tests with FLAG_WRITE
| FLAG_APPEND on Windows and they did hit the DCHECK I saw earlier in the code).
 I've discussed this with Nasko and he is also ok with landing as-is.

+
+  base::File browser_file(file_path, file_flags);
   if (!browser_file.IsValid()) {
     LOG(ERROR) << "Failed to create file to save MHTML at: " <<
         file_path.value();
   }
   return browser_file.Pass();
 }
 
 void MHTMLGenerationManager::OnFileAvailable(int job_id,
                                              base::File browser_file) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   if (!browser_file.IsValid()) {
     LOG(ERROR) << "Failed to create file";
     JobFinished(job_id, JobStatus::FAILURE);
     return;
   }
 
   Job* job = FindJob(job_id);
   if (!job)
     return;
 
   job->set_browser_file(browser_file.Pass());
 
-  if (!job->SendToRenderView()) {
+  if (!job->SendToNextRenderFrame()) {
     JobFinished(job_id, JobStatus::FAILURE);
   }
 }
 
 void MHTMLGenerationManager::JobFinished(int job_id, JobStatus job_status) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   Job* job = FindJob(job_id);
   if (!job)
     return;
@@ skipping 46 matching lines @@
        ++it) {
     if (it->second == job) {
       JobFinished(it->first, JobStatus::FAILURE);
       return;
     }
   }
   NOTREACHED();
 }
 
 }  // namespace content