Move download messages from Renderer to Frame filter.

content/renderer/render_view_impl.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/renderer/render_view_impl.h"
 
 #include <algorithm>
 #include <cmath>
 
 #include "base/auto_reset.h"
@@ skipping 1400 matching lines @@
   input_handler_->set_handling_input_event(true);
   webview()->focusedFrame()->selectWordAroundCaret();
   input_handler_->set_handling_input_event(false);
 }
 
 void RenderViewImpl::OnCopyImageAt(int x, int y) {
   webview()->copyImageAt(WebPoint(x, y));
 }
 
 void RenderViewImpl::OnSaveImageAt(int x, int y) {
   webview()->saveImageAt(WebPoint(x, y));

[Charlie Reis, 2016/03/29 04:41:49]

From codesearch, it looks like this is what triggers the saveImageFromDataURL
call.  I'd be surprised if this works for OOPIFs already, unless the browser
process translates the point to the correct frame.  (Maybe this is worth moving
from WebView to WebFrame in a separate CL?)

 }
 
 void RenderViewImpl::OnUpdateTargetURLAck() {
   // Check if there is a targeturl waiting to be sent.
   if (target_url_status_ == TARGET_PENDING)
     Send(new ViewHostMsg_UpdateTargetURL(GetRoutingID(), pending_target_url_));
 
   target_url_status_ = TARGET_NONE;
 }
 
@@ skipping 251 matching lines @@
 }
 
 void RenderViewImpl::printPage(WebLocalFrame* frame) {
   FOR_EACH_OBSERVER(RenderViewObserver, observers_,
                     PrintPage(frame, input_handler().handling_input_event()));
 }
 
 void RenderViewImpl::saveImageFromDataURL(const blink::WebString& data_url) {
   // Note: We should basically send GURL but we use size-limited string instead
   // in order to send a larger data url to save a image for <canvas> or <img>.
-  if (data_url.length() < kMaxLengthOfDataURLString)
-    Send(new ViewHostMsg_SaveImageFromDataURL(
-        GetRoutingID(), GetMainRenderFrame()->GetRoutingID(), data_url.utf8()));
+  if (data_url.length() < kMaxLengthOfDataURLString) {
+    Send(new FrameHostMsg_SaveImageFromDataURL(
+         GetRoutingID(), GetMainRenderFrame()->GetRoutingID(),

[Charlie Reis, 2016/03/29 04:41:49]

This looks problematic-- we probably shouldn't be assuming that
GetMainRenderFrame() exists.  (It won't in OOPIFs.)  I would kind of expect it
to crash in that case, or if not return the wrong routing ID.

Does the CL fix Rob's repro steps as is?  Maybe we can come back to this.

[brettw, 2016/03/29 05:44:39]

It seems to fix the repro steps. I looked through the code a bit when I started
changing things to make sense of the IDs, and it looks like these are given to
the network system, I assume for prioritization and such. It doesn't look like
we do things like post back to the object (which is what would normally happen)
and the network stack may be tolerant of such errors. I'll check more tomorrow.

[Charlie Reis, 2016/03/29 18:17:13]

Ok.  CC'ing rdsmith (who added the RFH ID, I think) and csharrison (who has been
refactoring the net stack for RFHs) in case they know more.

I'm ok landing this with a TODO if it's a step in the right direction.  

Still, I'd be curious to know why we aren't crashing here, since
GetMainRenderFrame() returns null in an OOPIF process.  I guess we never get
this call in an OOPIF, due to the bug I mentioned above (line 1421)?

[asanka, 2016/03/29 20:56:05]

AFAICT the reason why it's not crashing is:

* None of the test cases in the linked issue trip this code path. In order to
get here, you'll need to create a canvas element and download it via the "Save
image as..." context menu item. For it to be sensitive to OOPIF, the canvas
element needs to be in an OOPIF.

* Invoking "Save image as" on such a canvas element causes the SaveImageAt IPC
to be sent from the main frame's RenderViewHostImpl. The corresponding
RenderView does in fact have a valid main_render_frame_.

* The RenderView that receives the SaveImageAt IPC doesn't have an image at the
location, since there's an iframe there instead. Therefore nothing happens.
Whether or not the main_render_frame_ is valid appears to be moot.

I'm guessing this code path doesn't have test coverage? It's broken ToT.

Also, the code above is trying to dispatch a FrameHostMsg to a RenderViewImpl,
which isn't quite relevant since the code runs on the wrong process.

+         data_url.utf8()));
+  }
 }
 
 bool RenderViewImpl::enumerateChosenDirectory(
     const WebString& path,
     WebFileChooserCompletion* chooser_completion) {
   int id = enumeration_completion_id_++;
   enumeration_completions_[id] = chooser_completion;
   return Send(new ViewHostMsg_EnumerateDirectory(
       GetRoutingID(), id, blink::WebStringToFilePath(path)));
 }
@@ skipping 1733 matching lines @@
   if (IsUseZoomForDSFEnabled()) {
     webview()->setZoomFactorForDeviceScaleFactor(device_scale_factor_);
   } else {
     webview()->setDeviceScaleFactor(device_scale_factor_);
   }
   webview()->settings()->setPreferCompositingToLCDTextEnabled(
       PreferCompositingToLCDText(compositor_deps_, device_scale_factor_));
 }
 
 }  // namespace content