Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2417693002:
Allow MimeHandlerViewGuest be embedded inside OOPIFs (Closed)
|
Created:
4 years, 2 months ago by EhsanK Modified:
4 years, 1 month ago CC:
chromium-reviews, extensions-reviews_chromium.org, creis+watch_chromium.org, yusukes+watch_chromium.org, shuchen+watch_chromium.org, nasko+codewatch_chromium.org, jam, nona+watch_chromium.org, darin-cc_chromium.org, chromium-apps-reviews_chromium.org, James Su, site-isolation-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAllow MimeHandlerViewGuest be embedded inside OOPIFs.
Event routing for BrowserPlugin-based guests, along with some other IPCs, is through the
embedder's RenderWidgetHost. Normally the embedder is the owner WebContents. However,
when loading a PDF inside an out of process <iframe>, considering the WebContents as
the embedder is incorrect as the actual BrowserPlugin lives in the <iframe> process.
This CL will use the RWH corresponding to the embedder frame for event routing.
BUG=649856
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/51ca2097b0b7c1a6097c63690b5ca982b65924e4
Cr-Commit-Position: refs/heads/master@{#432779}
Patch Set 1 #Patch Set 2 : Rebase #Patch Set 3 : Rebased #Patch Set 4 : Added a test #Patch Set 5 : Fixed a compile error #Patch Set 6 : Added a NOTREACHED #
Total comments: 18
Patch Set 7 : Setting embedder frame id when the MimeHandlerViewGuest is created. #
Total comments: 14
Patch Set 8 : Addressing creis@, wjmaclean@, and kenrb@ comments #Patch Set 9 : Rebase and nits #
Total comments: 20
Patch Set 10 : Addressing lfg@ comments + Fixing embed.postMessage #Patch Set 11 : Rebased #
Total comments: 32
Patch Set 12 : Addressing lfg@ and nasko@ comments #
Total comments: 30
Patch Set 13 : Addressing creis@'s, alexmos@'s, and fsamuel@'s comments #
Total comments: 2
Patch Set 14 : Rebased #
Total comments: 3
Patch Set 15 : Addressing alexmos@'s comment (fixes the compile error) #Patch Set 16 : Fixed a compile error #Patch Set 17 : Rebased #Patch Set 18 : Addressing nasko@'s comments #Patch Set 19 : Fixed an input routing issue with nested browser plugings #
Total comments: 3
Patch Set 20 : Fixed a compile error and added some comments #
Total comments: 10
Patch Set 21 : Addressing creis@'s comments #Patch Set 22 : Observing life time of embedder frame #Patch Set 23 : Fixed a Compile Error #Patch Set 24 : Added a comment #
Total comments: 8
Patch Set 25 : Compile Error: Added the missing 'override' #Patch Set 26 : Addressing creis@'s comments #Patch Set 27 : Rebased #Messages
Total messages: 126 (93 generated)
Description was changed from ========== Routing BrowserPluginGuest Messages through Embedder Frame rather than OwnerWebContent's Main Frame Event routing for BrowserPlugin-based guests, along with some other IPCs, is through the embedder's RenderWidgetHost. Normally the embedder is the owner WebContents. However, when loading a PDF inside an out of process <iframe>, considering the WebContents as the embedder is incorrect as the actual BrowserPlugin lives in the <iframe> process. This CL bases the IPC routing from BrowserPlugin as well as event targeting through the embedder frame's RenderWidgetHost(View). BUG=649856 ========== to ========== Routing BrowserPluginGuest Messages through Embedder Frame rather than OwnerWebContent's Main Frame Event routing for BrowserPlugin-based guests, along with some other IPCs, is through the embedder's RenderWidgetHost. Normally the embedder is the owner WebContents. However, when loading a PDF inside an out of process <iframe>, considering the WebContents as the embedder is incorrect as the actual BrowserPlugin lives in the <iframe> process. This CL bases the IPC routing from BrowserPlugin as well as event targeting through the embedder frame's RenderWidgetHost(View). BUG=649856 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Description was changed from ========== Routing BrowserPluginGuest Messages through Embedder Frame rather than OwnerWebContent's Main Frame Event routing for BrowserPlugin-based guests, along with some other IPCs, is through the embedder's RenderWidgetHost. Normally the embedder is the owner WebContents. However, when loading a PDF inside an out of process <iframe>, considering the WebContents as the embedder is incorrect as the actual BrowserPlugin lives in the <iframe> process. This CL bases the IPC routing from BrowserPlugin as well as event targeting through the embedder frame's RenderWidgetHost(View). BUG=649856 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Routing BrowserPluginGuest Messages through Embedder Frame rather than OwnerWebContent's Main Frame Event routing for BrowserPlugin-based guests, along with some other IPCs, is through the embedder's RenderWidgetHost. Normally the embedder is the owner WebContents. However, when loading a PDF inside an out of process <iframe>, considering the WebContents as the embedder is incorrect as the actual BrowserPlugin lives in the <iframe> process. This CL bases the IPC routing from BrowserPlugin as well as event targeting through the embedder frame's RenderWidgetHost(View). BUG=649856 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Patchset #1 (id:1) has been deleted
The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Exceeded global retry quota
The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Routing BrowserPluginGuest Messages through Embedder Frame rather than OwnerWebContent's Main Frame Event routing for BrowserPlugin-based guests, along with some other IPCs, is through the embedder's RenderWidgetHost. Normally the embedder is the owner WebContents. However, when loading a PDF inside an out of process <iframe>, considering the WebContents as the embedder is incorrect as the actual BrowserPlugin lives in the <iframe> process. This CL bases the IPC routing from BrowserPlugin as well as event targeting through the embedder frame's RenderWidgetHost(View). BUG=649856 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Allow MimeHandlerViewGuest be embedded inside OOPIFs. Event routing for BrowserPlugin-based guests, along with some other IPCs, is through the embedder's RenderWidgetHost. Normally the embedder is the owner WebContents. However, when loading a PDF inside an out of process <iframe>, considering the WebContents as the embedder is incorrect as the actual BrowserPlugin lives in the <iframe> process. This CL bases the IPC routing from BrowserPlugin as well as event targeting through the embedder frame's RenderWidgetHost(View). BUG=649856 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== Routing BrowserPluginGuest Messages through Embedder Frame rather than OwnerWebContent's Main Frame Event routing for BrowserPlugin-based guests, along with some other IPCs, is through the embedder's RenderWidgetHost. Normally the embedder is the owner WebContents. However, when loading a PDF inside an out of process <iframe>, considering the WebContents as the embedder is incorrect as the actual BrowserPlugin lives in the <iframe> process. This CL bases the IPC routing from BrowserPlugin as well as event targeting through the embedder frame's RenderWidgetHost(View). BUG=649856 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Allow MimeHandlerViewGuest be embedded inside OOPIFs. Event routing for BrowserPlugin-based guests, along with some other IPCs, is through the embedder's RenderWidgetHost. Normally the embedder is the owner WebContents. However, when loading a PDF inside an out of process <iframe>, considering the WebContents as the embedder is incorrect as the actual BrowserPlugin lives in the <iframe> process. This CL bases the IPC routing from BrowserPlugin as well as event targeting through the embedder frame's RenderWidgetHost(View). BUG=649856 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== Allow MimeHandlerViewGuest be embedded inside OOPIFs. Event routing for BrowserPlugin-based guests, along with some other IPCs, is through the embedder's RenderWidgetHost. Normally the embedder is the owner WebContents. However, when loading a PDF inside an out of process <iframe>, considering the WebContents as the embedder is incorrect as the actual BrowserPlugin lives in the <iframe> process. This CL bases the IPC routing from BrowserPlugin as well as event targeting through the embedder frame's RenderWidgetHost(View). BUG=649856 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Allow MimeHandlerViewGuest be embedded inside OOPIFs. Event routing for BrowserPlugin-based guests, along with some other IPCs, is through the embedder's RenderWidgetHost. Normally the embedder is the owner WebContents. However, when loading a PDF inside an out of process <iframe>, considering the WebContents as the embedder is incorrect as the actual BrowserPlugin lives in the <iframe> process. This CL will use the RWH corresponding to the embedder frame for event routing. BUG=649856 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Could you please take a look at this CL, specifically, for test (owner LGTM): alexmos@ content/* and test: creis@ guest*: lazyboy@, lfg@, wjmclean@ input routing: wjmclean@ and kenrb@. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Thanks for taking on this fix! I'll defer to the <webview> folks for most of it, but a few concerns about how process/routing ID pairs are used below. https://codereview.chromium.org/2417693002/diff/120001/chrome/browser/chrome_... File chrome/browser/chrome_site_per_process_browsertest.cc (right): https://codereview.chromium.org/2417693002/diff/120001/chrome/browser/chrome_... chrome/browser/chrome_site_per_process_browsertest.cc:304: // Navigate subframe to a page with an embedded PDF. nit: to a cross-site page with https://codereview.chromium.org/2417693002/diff/120001/components/guest_view/... File components/guest_view/browser/guest_view_manager.cc (right): https://codereview.chromium.org/2417693002/diff/120001/components/guest_view/... components/guest_view/browser/guest_view_manager.cc:4: #include "base/debug/stack_trace.h" This belongs below. https://codereview.chromium.org/2417693002/diff/120001/components/guest_view/... components/guest_view/browser/guest_view_manager.cc:485: embedder_render_process_id, render_frame_routing_id); This doesn't seem obviously safe to me-- we're mixing a routing ID stored as state with a process ID passed in by the caller. It would be very easy to have bugs where these don't correspond to the same RenderFrameHost. https://codereview.chromium.org/2417693002/diff/120001/components/guest_view/... components/guest_view/browser/guest_view_manager.cc:489: guest_view->owner_web_contents()->GetRenderProcessHost()->GetID(); nit: No need for the GetID() calls, since you can just compare the RPHs directly. Also, it seems like it's worth including a comment on what this block (from 484 down) is trying to do. https://codereview.chromium.org/2417693002/diff/120001/components/guest_view/... File components/guest_view/browser/guest_view_manager.h (right): https://codereview.chromium.org/2417693002/diff/120001/components/guest_view/... components/guest_view/browser/guest_view_manager.h:275: int attaching_guest_embedder_routing_id_; Routing IDs can only be used in conjunction with a process ID-- they're not safe otherwise, since they can conflict with routing IDs from another process. Where is the process ID corresponding to this? https://codereview.chromium.org/2417693002/diff/120001/content/browser/browse... File content/browser/browser_plugin/browser_plugin_guest.cc (right): https://codereview.chromium.org/2417693002/diff/120001/content/browser/browse... content/browser/browser_plugin/browser_plugin_guest.cc:110: embedder_process_id_(0), ChildProcessHost::kInvalidUniqueID https://codereview.chromium.org/2417693002/diff/120001/content/browser/browse... content/browser/browser_plugin/browser_plugin_guest.cc:182: // Both |process_id| and |routing_id| are reported from the renderer side. For Renderer processes should never be told process IDs, and we shouldn't be trusting them to send up process/routing ID pairs. (They could send up any values they want.) Can we do this without getting the values from the renderer? (That said, I don't actually see where the values are being sent by the renderer, so maybe the comment is wrong?) https://codereview.chromium.org/2417693002/diff/120001/content/browser/browse... content/browser/browser_plugin/browser_plugin_guest.cc:201: // is incorrect is <webview> is at some point embedded inside an OOPIF. Typo (too many "is"'s) https://codereview.chromium.org/2417693002/diff/120001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_base.h (right): https://codereview.chromium.org/2417693002/diff/120001/content/browser/render... content/browser/renderer_host/render_widget_host_view_base.h:295: virtual RenderWidgetHostViewBase* GetEmbedderView(); Should we be using a different term here, like "outer" instead of "embedder"? Within content/, embedder tends to refer to the content/ embedder.
[+site-isolation-reviews]
ekaramad@chromium.org changed reviewers: + wjmaclean@chromium.org - wjmclean@chromium.org
The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Generally looks ok, with a few comments. I'm ok with this, but would like lfg@ to weigh in before l-g-t-m. https://codereview.chromium.org/2417693002/diff/140001/components/guest_view/... File components/guest_view/browser/guest_view_manager.cc (right): https://codereview.chromium.org/2417693002/diff/140001/components/guest_view/... components/guest_view/browser/guest_view_manager.cc:4: #include "base/debug/stack_trace.h" Remove before commit. https://codereview.chromium.org/2417693002/diff/140001/components/guest_view/... File components/guest_view/browser/guest_view_manager.h (right): https://codereview.chromium.org/2417693002/diff/140001/components/guest_view/... components/guest_view/browser/guest_view_manager.h:273: int attaching_guest_embedder_routing_id_; Where is this used? https://codereview.chromium.org/2417693002/diff/140001/content/browser/browse... File content/browser/browser_plugin/browser_plugin_guest.cc (right): https://codereview.chromium.org/2417693002/diff/140001/content/browser/browse... content/browser/browser_plugin/browser_plugin_guest.cc:251: if (attached() && rwhv && GetEmbedderFrame() && nit: Instead of the repeated calls to GetEmbedderFrame() in this block, would it be cleaner to do RenderFrameHostImpl rfhi = GetEmbedderFrame(); RenderWidgetHostImpl* rwhi = rfhi->GetRenderWidgetHost(); return (attached() && rwhv && rwhi && rwhv->OnMessageReceivedFromEmbedder( message, RenderWidgetHostImpl::From(rwhi))); ? https://codereview.chromium.org/2417693002/diff/140001/extensions/browser/gue... File extensions/browser/guest_view/extensions_guest_view_message_filter.cc (right): https://codereview.chromium.org/2417693002/diff/140001/extensions/browser/gue... extensions/browser/guest_view/extensions_guest_view_message_filter.cc:172: guest_view->SetEmbedderFrame(embedder_render_process_id, Will it ever be the case that we'll want to move a guest from the embedder in which it was originally created to another embedder? I'm guessing not, but if it can happen, do we need a mechanism to update these values on the guest_view? https://codereview.chromium.org/2417693002/diff/140001/extensions/browser/gue... File extensions/browser/guest_view/mime_handler_view/mime_handler_view_guest.cc (right): https://codereview.chromium.org/2417693002/diff/140001/extensions/browser/gue... extensions/browser/guest_view/mime_handler_view/mime_handler_view_guest.cc:103: embedder_frame_routing_id_ = routing_id; Do we need to do anything with these values when detaching the guest?
A couple of nits. https://codereview.chromium.org/2417693002/diff/140001/components/guest_view/... File components/guest_view/browser/guest_view_message_filter.cc (right): https://codereview.chromium.org/2417693002/diff/140001/components/guest_view/... components/guest_view/browser/guest_view_message_filter.cc:111: manager->AttachGuest(render_process_id_, element_instance_id, It isn't worth touching this file for what is now just a trivial format adjustment. https://codereview.chromium.org/2417693002/diff/140001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_base.h (right): https://codereview.chromium.org/2417693002/diff/140001/content/browser/render... content/browser/renderer_host/render_widget_host_view_base.h:294: // RWHVG or RWHVG is removed entirely. This should reference a bug in the tracker.
On 2016/10/20 16:12:05, kenrb wrote: > A couple of nits. . . . > > https://codereview.chromium.org/2417693002/diff/140001/content/browser/render... > content/browser/renderer_host/render_widget_host_view_base.h:294: // RWHVG or > RWHVG is removed entirely. > This should reference a bug in the tracker. Probably the bug to remove BrowserPlugin would do? https://crbug.com/533069
The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL. https://codereview.chromium.org/2417693002/diff/120001/chrome/browser/chrome_... File chrome/browser/chrome_site_per_process_browsertest.cc (right): https://codereview.chromium.org/2417693002/diff/120001/chrome/browser/chrome_... chrome/browser/chrome_site_per_process_browsertest.cc:304: // Navigate subframe to a page with an embedded PDF. On 2016/10/18 22:52:20, Charlie Reis (OOO until 10-19) wrote: > nit: to a cross-site page with Done. https://codereview.chromium.org/2417693002/diff/120001/components/guest_view/... File components/guest_view/browser/guest_view_manager.cc (right): https://codereview.chromium.org/2417693002/diff/120001/components/guest_view/... components/guest_view/browser/guest_view_manager.cc:4: #include "base/debug/stack_trace.h" On 2016/10/18 22:52:20, Charlie Reis wrote: > This belongs below. Sorry for leaving this behind. Will remove it. https://codereview.chromium.org/2417693002/diff/120001/components/guest_view/... components/guest_view/browser/guest_view_manager.cc:485: embedder_render_process_id, render_frame_routing_id); On 2016/10/18 22:52:20, Charlie Reis (OOO until 10-19) wrote: > This doesn't seem obviously safe to me-- we're mixing a routing ID stored as > state with a process ID passed in by the caller. It would be very easy to have > bugs where these don't correspond to the same RenderFrameHost. This variable and logic is removed form here. I now use MimeHandlerViewGuestCreatedCallback() to set these values as both embedder process and frame id are provided there. https://codereview.chromium.org/2417693002/diff/120001/components/guest_view/... components/guest_view/browser/guest_view_manager.cc:489: guest_view->owner_web_contents()->GetRenderProcessHost()->GetID(); On 2016/10/18 22:52:20, Charlie Reis (OOO until 10-19) wrote: > nit: No need for the GetID() calls, since you can just compare the RPHs > directly. > > Also, it seems like it's worth including a comment on what this block (from 484 > down) is trying to do. Acknowledged. https://codereview.chromium.org/2417693002/diff/120001/components/guest_view/... File components/guest_view/browser/guest_view_manager.h (right): https://codereview.chromium.org/2417693002/diff/120001/components/guest_view/... components/guest_view/browser/guest_view_manager.h:275: int attaching_guest_embedder_routing_id_; On 2016/10/18 22:52:20, Charlie Reis (OOO until 10-19) wrote: > Routing IDs can only be used in conjunction with a process ID-- they're not safe > otherwise, since they can conflict with routing IDs from another process. Where > is the process ID corresponding to this? It comes later when the attach request arrives (which also provides this routing_id). But it is removed in the newest patch. I needed this to pair up the routing ID with the process ID of the frame sending the message. I noticed that there is a better place to do this where both ids come together and we already use it to create the MimeHandlerViewGuest. I have left a comment there (in extensions_guest_view_message_filter.cc). https://codereview.chromium.org/2417693002/diff/120001/content/browser/browse... File content/browser/browser_plugin/browser_plugin_guest.cc (right): https://codereview.chromium.org/2417693002/diff/120001/content/browser/browse... content/browser/browser_plugin/browser_plugin_guest.cc:110: embedder_process_id_(0), On 2016/10/18 22:52:20, Charlie Reis (OOO until 10-19) wrote: > ChildProcessHost::kInvalidUniqueID Thanks. Used in mime_handler_view_guest.cc. https://codereview.chromium.org/2417693002/diff/120001/content/browser/browse... content/browser/browser_plugin/browser_plugin_guest.cc:182: // Both |process_id| and |routing_id| are reported from the renderer side. For On 2016/10/18 22:52:20, Charlie Reis wrote: > Renderer processes should never be told process IDs, and we shouldn't be > trusting them to send up process/routing ID pairs. (They could send up any > values they want.) Can we do this without getting the values from the renderer? > > (That said, I don't actually see where the values are being sent by the > renderer, so maybe the comment is wrong?) I see. I moved the logic to initialize the embedder process/routing ID into extensions_guest_view_message_filter.cc where we first receive the IPC to create the MimeHandlerViewGuest (ExtensionsGuestViewHostMsg_CreateMimeHandlerViewGuest). There we already have the (process id, routing id) of the embedder frame. https://codereview.chromium.org/2417693002/diff/120001/content/browser/browse... content/browser/browser_plugin/browser_plugin_guest.cc:201: // is incorrect is <webview> is at some point embedded inside an OOPIF. On 2016/10/18 22:52:20, Charlie Reis (OOO until 10-19) wrote: > Typo (too many "is"'s) Thanks! This block is removed now however. https://codereview.chromium.org/2417693002/diff/120001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_base.h (right): https://codereview.chromium.org/2417693002/diff/120001/content/browser/render... content/browser/renderer_host/render_widget_host_view_base.h:295: virtual RenderWidgetHostViewBase* GetEmbedderView(); On 2016/10/18 22:52:20, Charlie Reis (OOO until 10-19) wrote: > Should we be using a different term here, like "outer" instead of "embedder"? > Within content/, embedder tends to refer to the content/ embedder. I think outer is better in that case. But technically the name should be: GetTargetViewForMouseEvents() or something like that (given that that is the only application of the method). https://codereview.chromium.org/2417693002/diff/140001/components/guest_view/... File components/guest_view/browser/guest_view_manager.cc (right): https://codereview.chromium.org/2417693002/diff/140001/components/guest_view/... components/guest_view/browser/guest_view_manager.cc:4: #include "base/debug/stack_trace.h" On 2016/10/20 14:47:22, wjmaclean wrote: > Remove before commit. Done. https://codereview.chromium.org/2417693002/diff/140001/components/guest_view/... File components/guest_view/browser/guest_view_manager.h (right): https://codereview.chromium.org/2417693002/diff/140001/components/guest_view/... components/guest_view/browser/guest_view_manager.h:273: int attaching_guest_embedder_routing_id_; On 2016/10/20 14:47:22, wjmaclean wrote: > Where is this used? Stale. Thanks! https://codereview.chromium.org/2417693002/diff/140001/components/guest_view/... File components/guest_view/browser/guest_view_message_filter.cc (right): https://codereview.chromium.org/2417693002/diff/140001/components/guest_view/... components/guest_view/browser/guest_view_message_filter.cc:111: manager->AttachGuest(render_process_id_, element_instance_id, On 2016/10/20 16:12:05, kenrb wrote: > It isn't worth touching this file for what is now just a trivial format > adjustment. Yes. I was on my changelist in earlier patches and then git cl format kept it in the CL. https://codereview.chromium.org/2417693002/diff/140001/content/browser/browse... File content/browser/browser_plugin/browser_plugin_guest.cc (right): https://codereview.chromium.org/2417693002/diff/140001/content/browser/browse... content/browser/browser_plugin/browser_plugin_guest.cc:251: if (attached() && rwhv && GetEmbedderFrame() && On 2016/10/20 14:47:22, wjmaclean wrote: > nit: Instead of the repeated calls to GetEmbedderFrame() in this block, would it > be cleaner to do > > RenderFrameHostImpl rfhi = GetEmbedderFrame(); > RenderWidgetHostImpl* rwhi = rfhi->GetRenderWidgetHost(); > return (attached() && rwhv && rwhi && > rwhv->OnMessageReceivedFromEmbedder( > message, RenderWidgetHostImpl::From(rwhi))); > > ? Acknowledged. https://codereview.chromium.org/2417693002/diff/140001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_base.h (right): https://codereview.chromium.org/2417693002/diff/140001/content/browser/render... content/browser/renderer_host/render_widget_host_view_base.h:294: // RWHVG or RWHVG is removed entirely. On 2016/10/20 16:12:05, kenrb wrote: > This should reference a bug in the tracker. Done. https://codereview.chromium.org/2417693002/diff/140001/extensions/browser/gue... File extensions/browser/guest_view/extensions_guest_view_message_filter.cc (right): https://codereview.chromium.org/2417693002/diff/140001/extensions/browser/gue... extensions/browser/guest_view/extensions_guest_view_message_filter.cc:172: guest_view->SetEmbedderFrame(embedder_render_process_id, On 2016/10/20 14:47:22, wjmaclean wrote: > Will it ever be the case that we'll want to move a guest from the embedder in > which it was originally created to another embedder? I'm guessing not, but if it > can happen, do we need a mechanism to update these values on the guest_view? Interesting point. But 1) I am not quite sure how we can detach from on frame into another when they are cross process. 2) If we can, then I believe the BrowserPluginGuest::OnDetach is called which will check if the guest can run in detached mode (CanRunInDetachedState()). The answer is no for MimeHandlerViewGuest which is the guest type here. Having said this, I think we should be careful with referencing RFH (GetEmbedderFrame). For other guest types it should be safe since I am getting it from owner_web_contents(). For MimeHandlerView I will investigate to see if it is possible for the view to outlive its embedding frame (I guess not since it is an element after all, but I am not sure enough). https://codereview.chromium.org/2417693002/diff/140001/extensions/browser/gue... File extensions/browser/guest_view/mime_handler_view/mime_handler_view_guest.cc (right): https://codereview.chromium.org/2417693002/diff/140001/extensions/browser/gue... extensions/browser/guest_view/mime_handler_view/mime_handler_view_guest.cc:103: embedder_frame_routing_id_ = routing_id; On 2016/10/20 14:47:22, wjmaclean wrote: > Do we need to do anything with these values when detaching the guest? We could reset to 0 and MSG_ROUTING_NONE. But MimeHandlerViewGuest is destroyed on detach. Also, if guest outlives the frame, it is still fine since we check GetEmbedderFrame() against nullptr when we use it. But I will take another look to see if there are any potential hazards with these.
https://codereview.chromium.org/2417693002/diff/180001/extensions/browser/gue... File extensions/browser/guest_view/extensions_guest_view_message_filter.cc (right): https://codereview.chromium.org/2417693002/diff/180001/extensions/browser/gue... extensions/browser/guest_view/extensions_guest_view_message_filter.cc:172: guest_view->SetEmbedderFrame(embedder_render_process_id, This happens before attach which was used previously.
Mostly looks good, I'm just not sure about the proxy creation and if a postMessage would work from the embedder frame to the PDF. Can you try that? There's a test_postmessage in mimehandlerview that may be helpful. https://codereview.chromium.org/2417693002/diff/180001/components/guest_view/... File components/guest_view/browser/guest_view_manager.h (right): https://codereview.chromium.org/2417693002/diff/180001/components/guest_view/... components/guest_view/browser/guest_view_manager.h:65: // Associates the Browser Plugin with |element_instance_id| to a guest that No need to change this. https://codereview.chromium.org/2417693002/diff/180001/content/browser/browse... File content/browser/browser_plugin/browser_plugin_guest.cc (right): https://codereview.chromium.org/2417693002/diff/180001/content/browser/browse... content/browser/browser_plugin/browser_plugin_guest.cc:139: GetEmbedderFrame()->render_view_host()->GetSiteInstance(); GetEmbedderFrame()->GetSiteInstance(). https://codereview.chromium.org/2417693002/diff/180001/content/browser/browse... content/browser/browser_plugin/browser_plugin_guest.cc:142: ->root() I think the proxy has to be created in the embedder's FrameTreeNode. https://codereview.chromium.org/2417693002/diff/180001/content/browser/browse... content/browser/browser_plugin/browser_plugin_guest.cc:255: RenderWidgetHostImpl::From(rwhi))) { From is unnecessary. https://codereview.chromium.org/2417693002/diff/180001/content/browser/browse... content/browser/browser_plugin/browser_plugin_guest.cc:459: static_cast<RenderWidgetHostViewBase*>(GetEmbedderFrame()->GetView()); GetOwnerRenderWidgetHostView(). https://codereview.chromium.org/2417693002/diff/180001/content/browser/render... File content/browser/renderer_host/render_widget_host_input_event_router.cc (right): https://codereview.chromium.org/2417693002/diff/180001/content/browser/render... content/browser/renderer_host/render_widget_host_input_event_router.cc:200: RenderWidgetHostViewBase* embedder = target->GetOuterView(); This can be casted to RenderWidgetHostViewGuest, you don'tneed to add a method to RenderWidgetHostViewBase. https://codereview.chromium.org/2417693002/diff/180001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_base.h (right): https://codereview.chromium.org/2417693002/diff/180001/content/browser/render... content/browser/renderer_host/render_widget_host_view_base.h:295: virtual RenderWidgetHostViewBase* GetOuterView(); This doesn't belong here and we shouldn't need it. https://codereview.chromium.org/2417693002/diff/180001/extensions/browser/gue... File extensions/browser/guest_view/mime_handler_view/mime_handler_view_guest.cc (right): https://codereview.chromium.org/2417693002/diff/180001/extensions/browser/gue... extensions/browser/guest_view/mime_handler_view/mime_handler_view_guest.cc:93: if (embedder_frame_routing_id_ != MSG_ROUTING_NONE) This should be a DCHECK. https://codereview.chromium.org/2417693002/diff/180001/extensions/browser/gue... extensions/browser/guest_view/mime_handler_view/mime_handler_view_guest.cc:96: return GuestViewBase::GetEmbedderFrame(); And this should be NOTREACHED().
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
ekaramad@chromium.org changed reviewers: + nasko@chromium.org
Thanks for the reviews! PTAL. Adding nasko@ for some input on handling post messages in <embed> case. nasko@ Could you please take a look? This is in regards to the similar issue in https://crbug.com/485520. Also, I noticed that context menus for <embed>-ed PDF in OOPIF do not appear were they are supposed to. I will try to find a solution and add to this CL if possible. https://codereview.chromium.org/2417693002/diff/180001/components/guest_view/... File components/guest_view/browser/guest_view_manager.h (right): https://codereview.chromium.org/2417693002/diff/180001/components/guest_view/... components/guest_view/browser/guest_view_manager.h:65: // Associates the Browser Plugin with |element_instance_id| to a guest that On 2016/10/20 22:50:49, lfg wrote: > No need to change this. Acknowledged. https://codereview.chromium.org/2417693002/diff/180001/content/browser/browse... File content/browser/browser_plugin/browser_plugin_guest.cc (right): https://codereview.chromium.org/2417693002/diff/180001/content/browser/browse... content/browser/browser_plugin/browser_plugin_guest.cc:139: GetEmbedderFrame()->render_view_host()->GetSiteInstance(); On 2016/10/20 22:50:49, lfg wrote: > GetEmbedderFrame()->GetSiteInstance(). Acknowledged. https://codereview.chromium.org/2417693002/diff/180001/content/browser/browse... content/browser/browser_plugin/browser_plugin_guest.cc:142: ->root() On 2016/10/20 22:50:49, lfg wrote: > I think the proxy has to be created in the embedder's FrameTreeNode. I think the way it is and was defined is to setup the proxy inside the embedder for the incoming post messages to guest. Then with the first message, we initialize the proxy for the |message.source|. For that, I guess this part is correct. There were a couple of other places where I had to send the IPCs to the right renderer to make sure MimeHandlerViewContainer is initialized properly. With those changes in this patch, post message is half-working, i.e., we can post message to the <embed>-ed PDF (e.g., sending a message {type: selectAll} will select all text in PDF or we can send the print command). But unfortunately, we create the proxy for return trip of the message only in the WebContents root node: https://cs.chromium.org/chromium/src/content/browser/web_contents/web_content... And we actually cannot create in a child node due to this CHECK: https://cs.chromium.org/chromium/src/content/browser/frame_host/render_frame_... Because we do not add the RFPH to the (right?) node, the source proxy is null here: https://cs.chromium.org/chromium/src/content/browser/frame_host/render_frame_... And therefore, there is no |message.source|. There is some discussion in the bug listed in the comment for the last link (https//crbug.com/485520). I will add nasko@ to this CL for reviews and feedback on whether or not we would like to have the post message supported in this CL. https://codereview.chromium.org/2417693002/diff/180001/content/browser/browse... content/browser/browser_plugin/browser_plugin_guest.cc:255: RenderWidgetHostImpl::From(rwhi))) { On 2016/10/20 22:50:49, lfg wrote: > From is unnecessary. Done. https://codereview.chromium.org/2417693002/diff/180001/content/browser/browse... content/browser/browser_plugin/browser_plugin_guest.cc:459: static_cast<RenderWidgetHostViewBase*>(GetEmbedderFrame()->GetView()); On 2016/10/20 22:50:49, lfg wrote: > GetOwnerRenderWidgetHostView(). Acknowledged. https://codereview.chromium.org/2417693002/diff/180001/content/browser/render... File content/browser/renderer_host/render_widget_host_input_event_router.cc (right): https://codereview.chromium.org/2417693002/diff/180001/content/browser/render... content/browser/renderer_host/render_widget_host_input_event_router.cc:200: RenderWidgetHostViewBase* embedder = target->GetOuterView(); On 2016/10/20 22:50:49, lfg wrote: > This can be casted to RenderWidgetHostViewGuest, you don'tneed to add a method > to RenderWidgetHostViewBase. We are in renderer_host and RWHVG is in frame_host. I would have to add it to DEPS, wouldn't I? We have specific entry for RWHVCF. If we are adding RWHVG deps here too then I can simply expose the GetOwnerRenderWidgetHostView() method in RWHVG. https://codereview.chromium.org/2417693002/diff/180001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_base.h (right): https://codereview.chromium.org/2417693002/diff/180001/content/browser/render... content/browser/renderer_host/render_widget_host_view_base.h:295: virtual RenderWidgetHostViewBase* GetOuterView(); On 2016/10/20 22:50:49, lfg wrote: > This doesn't belong here and we shouldn't need it. Agreed is we can include RWHVG inside RWHIER. kenrb@, does that seem right given that you already have a comment inside the DEPS file about eventually moving RWHVCF to renderer_host and esp. given that RWHVG is hopefully temporary? https://codereview.chromium.org/2417693002/diff/180001/extensions/browser/gue... File extensions/browser/guest_view/mime_handler_view/mime_handler_view_guest.cc (right): https://codereview.chromium.org/2417693002/diff/180001/extensions/browser/gue... extensions/browser/guest_view/mime_handler_view/mime_handler_view_guest.cc:93: if (embedder_frame_routing_id_ != MSG_ROUTING_NONE) On 2016/10/20 22:50:49, lfg wrote: > This should be a DCHECK. Acknowledged. https://codereview.chromium.org/2417693002/diff/180001/extensions/browser/gue... extensions/browser/guest_view/mime_handler_view/mime_handler_view_guest.cc:96: return GuestViewBase::GetEmbedderFrame(); On 2016/10/20 22:50:50, lfg wrote: > And this should be NOTREACHED(). I guess only DCHECK would do then and I removed this line.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Sorry for the delay, the patch l-g-t-m pending the change in RWHVB and the proxy creation. https://codereview.chromium.org/2417693002/diff/180001/content/browser/browse... File content/browser/browser_plugin/browser_plugin_guest.cc (right): https://codereview.chromium.org/2417693002/diff/180001/content/browser/browse... content/browser/browser_plugin/browser_plugin_guest.cc:142: ->root() On 2016/10/25 19:30:50, EhsanK wrote: > On 2016/10/20 22:50:49, lfg wrote: > > I think the proxy has to be created in the embedder's FrameTreeNode. > > I think the way it is and was defined is to setup the proxy inside the embedder > for the incoming post messages to guest. Then with the first message, we > initialize the proxy for the |message.source|. For that, I guess this part is > correct. > > There were a couple of other places where I had to send the IPCs to the right > renderer to make sure MimeHandlerViewContainer is initialized properly. With > those changes in this patch, post message is half-working, i.e., we can post > message to the <embed>-ed PDF (e.g., sending a message {type: selectAll} will > select all text in PDF or we can send the print command). > > But unfortunately, we create the proxy for return trip of the message only in > the WebContents root node: > https://cs.chromium.org/chromium/src/content/browser/web_contents/web_content... > > And we actually cannot create in a child node due to this CHECK: > https://cs.chromium.org/chromium/src/content/browser/frame_host/render_frame_... > > Because we do not add the RFPH to the (right?) node, the source proxy is null > here: > https://cs.chromium.org/chromium/src/content/browser/frame_host/render_frame_... > And therefore, there is no |message.source|. There is some discussion in the bug > listed in the comment for the last link (https//crbug.com/485520). > > I will add nasko@ to this CL for reviews and feedback on whether or not we would > like to have the post message supported in this CL. Right, we should then add a TODO, and probably shouldn't be creating the proxy in this case.
Some initial comments to ensure I am properly understanding the CL. Also, I didn't see any changes specific to postMessage, so I'm not sure what is actually the question for me. https://codereview.chromium.org/2417693002/diff/220001/chrome/browser/chrome_... File chrome/browser/chrome_site_per_process_browsertest.cc (right): https://codereview.chromium.org/2417693002/diff/220001/chrome/browser/chrome_... chrome/browser/chrome_site_per_process_browsertest.cc:309: // TestGuestViewManager::WaitForSingleGuestCreated may and will get called nit: "may and will" - doesn't that basically mean that it "will" get called? https://codereview.chromium.org/2417693002/diff/220001/chrome/test/data/page_... File chrome/test/data/page_with_embedded_pdf.html (right): https://codereview.chromium.org/2417693002/diff/220001/chrome/test/data/page_... chrome/test/data/page_with_embedded_pdf.html:1:  nit: No need for an empty line. https://codereview.chromium.org/2417693002/diff/220001/components/guest_view/... File components/guest_view/browser/guest_view_base.cc (right): https://codereview.chromium.org/2417693002/diff/220001/components/guest_view/... components/guest_view/browser/guest_view_base.cc:420: GetEmbedderFrame()->GetView()->GetRenderWidgetHost()->Send( Before this IPC was sent through the main frame. Now it is sent through the widget. Is this intentional? Guest attaching doesn't strike me as a graphics/input event type message, but I can be wrong. https://codereview.chromium.org/2417693002/diff/220001/components/guest_view/... File components/guest_view/browser/guest_view_manager.cc (right): https://codereview.chromium.org/2417693002/diff/220001/components/guest_view/... components/guest_view/browser/guest_view_manager.cc:469: guest_view->GetEmbedderFrame()->GetProcess()->GetID(); Why shouldn't this always be the case? Why make it view type specific? https://codereview.chromium.org/2417693002/diff/220001/content/browser/browse... File content/browser/browser_plugin/browser_plugin_guest.cc (right): https://codereview.chromium.org/2417693002/diff/220001/content/browser/browse... content/browser/browser_plugin/browser_plugin_guest.cc:144: ->CreateRenderFrameProxy(owner_site_instance); This doesn't seem right, as we take the SiteInstance from the frame, which might not be the root, yet we create the proxy in the root frame. Shouldn't this be GetEmbedderFrame()->frame_tree_node()->render_manager()->CreateRenderFrameProxy()? https://codereview.chromium.org/2417693002/diff/220001/content/browser/render... File content/browser/renderer_host/render_widget_host_input_event_router.cc (right): https://codereview.chromium.org/2417693002/diff/220001/content/browser/render... content/browser/renderer_host/render_widget_host_input_event_router.cc:202: // This happens when the view is embedded inside a cross process frame. nit: cross-process https://codereview.chromium.org/2417693002/diff/220001/content/browser/render... content/browser/renderer_host/render_widget_host_input_event_router.cc:205: gfx::Point(event->x, event->y), embedder, &embedder_point)) Two line if statement should have {}. https://codereview.chromium.org/2417693002/diff/220001/content/public/browser... File content/public/browser/browser_plugin_guest_delegate.cc (right): https://codereview.chromium.org/2417693002/diff/220001/content/public/browser... content/public/browser/browser_plugin_guest_delegate.cc:41: return GetOwnerWebContents()->GetMainFrame(); Why return the main frame here? Isn't this supposed to be the frame that contains the GuestView? If you have A->B->C->GuestView, why would we return A instead of C? https://codereview.chromium.org/2417693002/diff/220001/content/public/browser... File content/public/browser/browser_plugin_guest_delegate.h (right): https://codereview.chromium.org/2417693002/diff/220001/content/public/browser... content/public/browser/browser_plugin_guest_delegate.h:102: // BrowserPlugin. This is the main frame for the owner of the guests, unless, Based on the implementation, it returns the main frame of the embedder. A GuestView can be embedded in arbitrary level of iframe nesting, right? https://codereview.chromium.org/2417693002/diff/220001/content/public/browser... content/public/browser/browser_plugin_guest_delegate.h:103: // the guest is embedded inside a cross origin frame. Can we have cross-origin frames in apps?
Thanks lfg@ and nasko@ for the reviews: lfg@: Added a TODO in browser_plugin_guest.cc. nasko@: The question is the creation of RenderFrameProxyHost in frame tree node which is not root. Currently is causes a CHECK fail in browser. I left some comments in the files. Thanks! PTAL. https://codereview.chromium.org/2417693002/diff/220001/chrome/browser/chrome_... File chrome/browser/chrome_site_per_process_browsertest.cc (right): https://codereview.chromium.org/2417693002/diff/220001/chrome/browser/chrome_... chrome/browser/chrome_site_per_process_browsertest.cc:309: // TestGuestViewManager::WaitForSingleGuestCreated may and will get called On 2016/10/27 21:03:40, nasko (slow) wrote: > nit: "may and will" - doesn't that basically mean that it "will" get called? Acknowledged. I copied and pasted this from extension_view_browsertest.cc which carried over the comment. I think "may" is a safer word here maybe?! https://codereview.chromium.org/2417693002/diff/220001/chrome/test/data/page_... File chrome/test/data/page_with_embedded_pdf.html (right): https://codereview.chromium.org/2417693002/diff/220001/chrome/test/data/page_... chrome/test/data/page_with_embedded_pdf.html:1:  On 2016/10/27 21:03:40, nasko (slow) wrote: > nit: No need for an empty line. Acknowledged. https://codereview.chromium.org/2417693002/diff/220001/components/guest_view/... File components/guest_view/browser/guest_view_base.cc (right): https://codereview.chromium.org/2417693002/diff/220001/components/guest_view/... components/guest_view/browser/guest_view_base.cc:420: GetEmbedderFrame()->GetView()->GetRenderWidgetHost()->Send( On 2016/10/27 21:03:40, nasko (slow) wrote: > Before this IPC was sent through the main frame. Now it is sent through the > widget. Is this intentional? Guest attaching doesn't strike me as a > graphics/input event type message, but I can be wrong. As I understand, WebContentsImpl::Send will send its message through RenderViewHostImpl which will send it through RenderWidgetHostImpl (of the page?). That is why I transferred it to RenderWidgetHost here. Also, this IPC specifically is handled by a GuestViewContainerDispatcher which is a RenderThreadObserver. https://codereview.chromium.org/2417693002/diff/220001/components/guest_view/... File components/guest_view/browser/guest_view_manager.cc (right): https://codereview.chromium.org/2417693002/diff/220001/components/guest_view/... components/guest_view/browser/guest_view_manager.cc:469: guest_view->GetEmbedderFrame()->GetProcess()->GetID(); On 2016/10/27 21:03:40, nasko (slow) wrote: > Why shouldn't this always be the case? Why make it view type specific? I think other guests are not supposed to be embedded inside <iframe>. I can remove this if there are no concerns about guest being embedded in <iframe>s. https://codereview.chromium.org/2417693002/diff/220001/content/browser/browse... File content/browser/browser_plugin/browser_plugin_guest.cc (right): https://codereview.chromium.org/2417693002/diff/220001/content/browser/browse... content/browser/browser_plugin/browser_plugin_guest.cc:144: ->CreateRenderFrameProxy(owner_site_instance); On 2016/10/27 21:03:40, nasko (slow) wrote: > This doesn't seem right, as we take the SiteInstance from the frame, which might > not be the root, yet we create the proxy in the root frame. This was my main question from you on this CL. Thanks for bringing it up! This is what I understand so far: 1- We do create the RFPH in the root, but it is the root of the guest frame tree. This way the RFP in the embedder frame's process can be used to post message to the guest. > Shouldn't this be > GetEmbedderFrame()->frame_tree_node()->render_manager()->CreateRenderFrameProxy()? 2- This would technically create the reverse path for post messaging, i.e., RFP in guest process to message back (right?). However, calling this on the a frame tree node which is not root and does not have a RenderViewHostImpl yet, will cause a browser crash through CHECK: https://cs.chromium.org/chromium/src/content/browser/frame_host/render_frame_... So my question is, a) Is it OK to create RFPH in the node which is not main frame given the context here: https://crbug.com/485520 and b) if yes, then how can I do it? is it OK to remove the CHECK above? https://codereview.chromium.org/2417693002/diff/220001/content/browser/render... File content/browser/renderer_host/render_widget_host_input_event_router.cc (right): https://codereview.chromium.org/2417693002/diff/220001/content/browser/render... content/browser/renderer_host/render_widget_host_input_event_router.cc:202: // This happens when the view is embedded inside a cross process frame. On 2016/10/27 21:03:40, nasko (slow) wrote: > nit: cross-process Acknowledged. https://codereview.chromium.org/2417693002/diff/220001/content/browser/render... content/browser/renderer_host/render_widget_host_input_event_router.cc:205: gfx::Point(event->x, event->y), embedder, &embedder_point)) On 2016/10/27 21:03:40, nasko (slow) wrote: > Two line if statement should have {}. Done. But is it too lines in the condition or two lines in the "then" part? Should I fix line 185 as well? https://codereview.chromium.org/2417693002/diff/220001/content/public/browser... File content/public/browser/browser_plugin_guest_delegate.cc (right): https://codereview.chromium.org/2417693002/diff/220001/content/public/browser... content/public/browser/browser_plugin_guest_delegate.cc:41: return GetOwnerWebContents()->GetMainFrame(); On 2016/10/27 21:03:40, nasko (slow) wrote: > Why return the main frame here? Isn't this supposed to be the frame that > contains the GuestView? If you have A->B->C->GuestView, why would we return A > instead of C? The only way we can have A->B->C->GuestView right now is to have <embed src="foo.pdf"> inside the inner most <iframe>. For that, the override of this method in MimeHandlerViewGuest will return C's RenderFrameHost. I don't think we can have guests in <iframe> unless the guest is a PDF. But I would like to divert this to lfg@, lazyboy@, wjmaclean@ for confirmation. https://codereview.chromium.org/2417693002/diff/220001/content/public/browser... File content/public/browser/browser_plugin_guest_delegate.h (right): https://codereview.chromium.org/2417693002/diff/220001/content/public/browser... content/public/browser/browser_plugin_guest_delegate.h:102: // BrowserPlugin. This is the main frame for the owner of the guests, unless, On 2016/10/27 21:03:40, nasko (slow) wrote: > Based on the implementation, it returns the main frame of the embedder. A > GuestView can be embedded in arbitrary level of iframe nesting, right? As far as I can tell, only MimeHandlerViewGuest can be embedded inside an <iframe>. So for all other guest views, the way it is implemented now, will return |owner_web_contents_->GetMainFrame()|. https://codereview.chromium.org/2417693002/diff/220001/content/public/browser... content/public/browser/browser_plugin_guest_delegate.h:103: // the guest is embedded inside a cross origin frame. On 2016/10/27 21:03:40, nasko (slow) wrote: > Can we have cross-origin frames in apps? I don't think we can use <iframe>'s in apps at all. For external content we are supposed to use <webview> there.
Thanks-- seems better, though some of this will be nice to clean up when we switch away from BrowserPlugin. LGTM with nits. https://codereview.chromium.org/2417693002/diff/240001/components/guest_view/... File components/guest_view/browser/guest_view_manager.cc (right): https://codereview.chromium.org/2417693002/diff/240001/components/guest_view/... components/guest_view/browser/guest_view_manager.cc:472: return embedder_render_process_id == To the clarify the comment above, let's add: // Other than MimeHandlerViewGuest, all other guests are only permitted to run in the main frame. https://codereview.chromium.org/2417693002/diff/240001/content/browser/browse... File content/browser/browser_plugin/browser_plugin_guest.cc (right): https://codereview.chromium.org/2417693002/diff/240001/content/browser/browse... content/browser/browser_plugin/browser_plugin_guest.cc:140: // TODO(ekaramad): We create the proxy for post messaging to the guest. The nit: Say "to the guest below." (I was having a hard time understanding how the rest of this comment related to the code below, until I figured out that the code below is creating the proxy that the embedder will use to talk to the guest, rather than the "reverse" one the comment is describing.) https://codereview.chromium.org/2417693002/diff/240001/content/browser/browse... content/browser/browser_plugin/browser_plugin_guest.cc:142: // the guest is inisde an <iframe> (e.g., <embed>-ed PDF) then the reverse nit: inside https://codereview.chromium.org/2417693002/diff/240001/content/browser/browse... content/browser/browser_plugin/browser_plugin_guest.cc:259: RenderWidgetHostImpl* rwhi = rfhi ? rfhi->GetRenderWidgetHost() : nullptr; nit: Move these two lines above the "// Until" comment. https://codereview.chromium.org/2417693002/diff/240001/content/browser/browse... content/browser/browser_plugin/browser_plugin_guest.cc:523: GetEmbedderFrame()->GetRenderWidgetHost()->Send(msg.release()); This change looks a little odd to me (sending via WebContents vs sending via RenderWidgetHost), but I guess under the hood they're basically equivalent. Ok. https://codereview.chromium.org/2417693002/diff/240001/content/browser/frame_... File content/browser/frame_host/render_widget_host_view_child_frame.h (right): https://codereview.chromium.org/2417693002/diff/240001/content/browser/frame_... content/browser/frame_host/render_widget_host_view_child_frame.h:195: // events is a RenderWidgetHostViewGuest which is embeded in an OOPIF (the nit: embedded https://codereview.chromium.org/2417693002/diff/240001/content/browser/frame_... content/browser/frame_host/render_widget_host_view_child_frame.h:200: virtual RenderWidgetHostViewBase* GetOwnerRenderWidgetHostView() const; Why does this need to be here, if it's implemented as a NOTREACHED and only callable on the subclass (RWHVG)? Would it be easier to cast to RWHVG at the only callsite (RenderWidgetHostInputEventRouter::RouteMouseEvent) and leave it out of this class? https://codereview.chromium.org/2417693002/diff/240001/content/browser/render... File content/browser/renderer_host/render_widget_host_input_event_router.cc (right): https://codereview.chromium.org/2417693002/diff/240001/content/browser/render... content/browser/renderer_host/render_widget_host_input_event_router.cc:201: static_cast<RenderWidgetHostViewChildFrame*>(target) Can we just cast to RenderWidgetHostViewGuest here? https://codereview.chromium.org/2417693002/diff/240001/content/public/browser... File content/public/browser/browser_plugin_guest_delegate.cc (right): https://codereview.chromium.org/2417693002/diff/240001/content/public/browser... content/public/browser/browser_plugin_guest_delegate.cc:41: return GetOwnerWebContents()->GetMainFrame(); This code should probably be in GuestViewBase or an appropriate subclass. This class only has empty implementations of methods, not any real logic. https://codereview.chromium.org/2417693002/diff/240001/content/public/browser... File content/public/browser/browser_plugin_guest_delegate.h (right): https://codereview.chromium.org/2417693002/diff/240001/content/public/browser... content/public/browser/browser_plugin_guest_delegate.h:102: // BrowserPlugin. This is the main frame for the owner of the guests, unless, nit: No comma after "unless."
I only looked at the test - just a couple of minor comments. https://codereview.chromium.org/2417693002/diff/240001/chrome/browser/chrome_... File chrome/browser/chrome_site_per_process_browsertest.cc (right): https://codereview.chromium.org/2417693002/diff/240001/chrome/browser/chrome_... chrome/browser/chrome_site_per_process_browsertest.cc:311: if (!manager) { Could we just always create the manager in SetUpOnMainThread, save it in a member, and then just return it here? Similarly to what's done in app_view_browsertest.cc? That might simplify this a bit. https://codereview.chromium.org/2417693002/diff/240001/chrome/browser/chrome_... chrome/browser/chrome_site_per_process_browsertest.cc:322: std::unique_ptr<guest_view::TestGuestViewManagerFactory> factory_; Can this just be "guest_view::TestGuestViewManagerFactory factory_"? https://codereview.chromium.org/2417693002/diff/240001/chrome/browser/chrome_... chrome/browser/chrome_site_per_process_browsertest.cc:328: // PDF, the guest is properly created. nit: include reference to https://crbug.com/649856 here. https://codereview.chromium.org/2417693002/diff/240001/chrome/browser/chrome_... chrome/browser/chrome_site_per_process_browsertest.cc:350: guest_view_manager->WaitForNumGuestsCreated(1U); nit: any reason not to use WaitForSingleGuestCreated here?
Sorry for the delay, I missed this CL. lgtm if alexmos@ is happy.
fsamuel@chromium.org changed reviewers: + fsamuel@chromium.org
https://codereview.chromium.org/2417693002/diff/240001/components/guest_view/... File components/guest_view/browser/guest_view_manager.cc (right): https://codereview.chromium.org/2417693002/diff/240001/components/guest_view/... components/guest_view/browser/guest_view_manager.cc:466: if (guest_view->IsViewType("mimehandler")) { drive-by: This is a layering violation. components/guest_view should not know about mime handler. Instead, could you make a virtual method that MimeHandlerViewGuest overrides that alters this behavior?
The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run
Thank you all for the reviews! Please, take another look. https://codereview.chromium.org/2417693002/diff/240001/chrome/browser/chrome_... File chrome/browser/chrome_site_per_process_browsertest.cc (right): https://codereview.chromium.org/2417693002/diff/240001/chrome/browser/chrome_... chrome/browser/chrome_site_per_process_browsertest.cc:311: if (!manager) { On 2016/11/02 22:22:39, alexmos wrote: > Could we just always create the manager in SetUpOnMainThread, save it in a > member, and then just return it here? Similarly to what's done in > app_view_browsertest.cc? That might simplify this a bit. Acknowledged. https://codereview.chromium.org/2417693002/diff/240001/chrome/browser/chrome_... chrome/browser/chrome_site_per_process_browsertest.cc:322: std::unique_ptr<guest_view::TestGuestViewManagerFactory> factory_; On 2016/11/02 22:22:39, alexmos wrote: > Can this just be "guest_view::TestGuestViewManagerFactory factory_"? Acknowledged. https://codereview.chromium.org/2417693002/diff/240001/chrome/browser/chrome_... chrome/browser/chrome_site_per_process_browsertest.cc:328: // PDF, the guest is properly created. On 2016/11/02 22:22:39, alexmos wrote: > nit: include reference to https://crbug.com/649856 here. Acknowledged. https://codereview.chromium.org/2417693002/diff/240001/chrome/browser/chrome_... chrome/browser/chrome_site_per_process_browsertest.cc:350: guest_view_manager->WaitForNumGuestsCreated(1U); On 2016/11/02 22:22:38, alexmos wrote: > nit: any reason not to use WaitForSingleGuestCreated here? Not really. I guess I used this because I was looking at the number of guests on line 338. I am using SingleGuest now. https://codereview.chromium.org/2417693002/diff/240001/components/guest_view/... File components/guest_view/browser/guest_view_manager.cc (right): https://codereview.chromium.org/2417693002/diff/240001/components/guest_view/... components/guest_view/browser/guest_view_manager.cc:466: if (guest_view->IsViewType("mimehandler")) { On 2016/11/08 16:14:23, Fady Samuel wrote: > drive-by: This is a layering violation. components/guest_view should not know > about mime handler. Instead, could you make a virtual method that > MimeHandlerViewGuest overrides that alters this behavior? Done. I added a new method which explicitly asks if the guest can be embedded inside a cross process frame. https://codereview.chromium.org/2417693002/diff/240001/components/guest_view/... components/guest_view/browser/guest_view_manager.cc:472: return embedder_render_process_id == On 2016/11/02 20:47:06, Charlie Reis (OOO til 11-11) wrote: > To the clarify the comment above, let's add: > // Other than MimeHandlerViewGuest, all other guests are only permitted to run > in the main frame. Thanks. I also changed "cross origin" to "cross process" in comment above since we actually pay attention to the process ID rather than the origin. https://codereview.chromium.org/2417693002/diff/240001/content/browser/browse... File content/browser/browser_plugin/browser_plugin_guest.cc (right): https://codereview.chromium.org/2417693002/diff/240001/content/browser/browse... content/browser/browser_plugin/browser_plugin_guest.cc:140: // TODO(ekaramad): We create the proxy for post messaging to the guest. The On 2016/11/02 20:47:06, Charlie Reis (OOO til 11-11) wrote: > nit: Say "to the guest below." (I was having a hard time understanding how the > rest of this comment related to the code below, until I figured out that the > code below is creating the proxy that the embedder will use to talk to the > guest, rather than the "reverse" one the comment is describing.) Sorry but I am a bit confused with the term "to the guest below" since |this| is the guest as far as I understand. I have reworded the comments; also changed the lines 129-133. Now in 129-135 we explain the process of creating RFPH to post message to guest and how the reverse one is created. The TODO now only mentions that the reverse RFPH is not created when the embedder is OOPIF. https://codereview.chromium.org/2417693002/diff/240001/content/browser/browse... content/browser/browser_plugin/browser_plugin_guest.cc:142: // the guest is inisde an <iframe> (e.g., <embed>-ed PDF) then the reverse On 2016/11/02 20:47:06, Charlie Reis (OOO til 11-11) wrote: > nit: inside Done. https://codereview.chromium.org/2417693002/diff/240001/content/browser/browse... content/browser/browser_plugin/browser_plugin_guest.cc:259: RenderWidgetHostImpl* rwhi = rfhi ? rfhi->GetRenderWidgetHost() : nullptr; On 2016/11/02 20:47:06, Charlie Reis (OOO til 11-11) wrote: > nit: Move these two lines above the "// Until" comment. Done. https://codereview.chromium.org/2417693002/diff/240001/content/browser/browse... content/browser/browser_plugin/browser_plugin_guest.cc:523: GetEmbedderFrame()->GetRenderWidgetHost()->Send(msg.release()); On 2016/11/02 20:47:06, Charlie Reis (OOO til 11-11) wrote: > This change looks a little odd to me (sending via WebContents vs sending via > RenderWidgetHost), but I guess under the hood they're basically equivalent. Ok. To respect the exact implementation, we should send it to RenderWidgetHost (since WebContents eventually does that under the hood). However, we are handling these messages in BrowserPluginManager which is a RenderThreadObserver. So, as I understand sending them to the embedder frame directly also works. I wonder if we could alternatively send them to "any other" IPC::Listener in the embedder process? (assuming that the browser side entity is alive when RWH is). https://codereview.chromium.org/2417693002/diff/240001/content/browser/frame_... File content/browser/frame_host/render_widget_host_view_child_frame.h (right): https://codereview.chromium.org/2417693002/diff/240001/content/browser/frame_... content/browser/frame_host/render_widget_host_view_child_frame.h:195: // events is a RenderWidgetHostViewGuest which is embeded in an OOPIF (the On 2016/11/02 20:47:06, Charlie Reis (OOO til 11-11) wrote: > nit: embedded Done. https://codereview.chromium.org/2417693002/diff/240001/content/browser/frame_... content/browser/frame_host/render_widget_host_view_child_frame.h:200: virtual RenderWidgetHostViewBase* GetOwnerRenderWidgetHostView() const; On 2016/11/02 20:47:06, Charlie Reis (OOO til 11-11) wrote: > Why does this need to be here, if it's implemented as a NOTREACHED and only > callable on the subclass (RWHVG)? Would it be easier to cast to RWHVG at the > only callsite (RenderWidgetHostInputEventRouter::RouteMouseEvent) and leave it > out of this class? Yes, it would definitely be easier (lfg@ also suggested this earlier). I avoided doing this originally because this is a layering violation (RWHVG lives inside frame_host/ while RWHIER is inside renderer_host). But as I understand this approached is being favored over adding the method to RWHVCF esp. given that we already violate this for RWHVCF (there is also a comment about it inside the DEPS file for renderer_host/). I made the change and modified the DEPS file. https://codereview.chromium.org/2417693002/diff/240001/content/browser/render... File content/browser/renderer_host/render_widget_host_input_event_router.cc (right): https://codereview.chromium.org/2417693002/diff/240001/content/browser/render... content/browser/renderer_host/render_widget_host_input_event_router.cc:201: static_cast<RenderWidgetHostViewChildFrame*>(target) On 2016/11/02 20:47:07, Charlie Reis (OOO til 11-11) wrote: > Can we just cast to RenderWidgetHostViewGuest here? Yes but we are in renderer_host/. Should I add exception to DEPS? https://codereview.chromium.org/2417693002/diff/240001/content/public/browser... File content/public/browser/browser_plugin_guest_delegate.cc (right): https://codereview.chromium.org/2417693002/diff/240001/content/public/browser... content/public/browser/browser_plugin_guest_delegate.cc:41: return GetOwnerWebContents()->GetMainFrame(); On 2016/11/02 20:47:07, Charlie Reis (OOO til 11-11) wrote: > This code should probably be in GuestViewBase or an appropriate subclass. This > class only has empty implementations of methods, not any real logic. We make references to this method in BrowserPluginGuest which is in content/. We do not depend on components/ as I understand so adding this to GuestViewBase might be a layering violation (since GuestViewBase is a BrowserPluginGuestDelegate I believe the right way is perhaps to add to BPGD and implement in GVB). I, however, moved the logic to GuestViewBase and return nullptr here. https://codereview.chromium.org/2417693002/diff/240001/content/public/browser... File content/public/browser/browser_plugin_guest_delegate.h (right): https://codereview.chromium.org/2417693002/diff/240001/content/public/browser... content/public/browser/browser_plugin_guest_delegate.h:102: // BrowserPlugin. This is the main frame for the owner of the guests, unless, On 2016/11/02 20:47:07, Charlie Reis (OOO til 11-11) wrote: > nit: No comma after "unless." Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks, the test LGTM with a nit. https://codereview.chromium.org/2417693002/diff/260001/chrome/browser/chrome_... File chrome/browser/chrome_site_per_process_browsertest.cc (right): https://codereview.chromium.org/2417693002/diff/260001/chrome/browser/chrome_... chrome/browser/chrome_site_per_process_browsertest.cc:341: guest_view_manager->WaitForSingleGuestCreated(1U); The 1U is not needed anymore.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2417693002/diff/220001/components/guest_view/... File components/guest_view/browser/guest_view_base.cc (right): https://codereview.chromium.org/2417693002/diff/220001/components/guest_view/... components/guest_view/browser/guest_view_base.cc:420: GetEmbedderFrame()->GetView()->GetRenderWidgetHost()->Send( On 2016/10/27 22:06:48, EhsanK wrote: > On 2016/10/27 21:03:40, nasko (slow) wrote: > > Before this IPC was sent through the main frame. Now it is sent through the > > widget. Is this intentional? Guest attaching doesn't strike me as a > > graphics/input event type message, but I can be wrong. > > As I understand, WebContentsImpl::Send will send its message through > RenderViewHostImpl which will send it through RenderWidgetHostImpl (of the > page?). That is why I transferred it to RenderWidgetHost here. They all send through the same IPC channel. What matters at the end is having the correct routing ID. > Also, this IPC specifically is handled by a GuestViewContainerDispatcher which > is a RenderThreadObserver. https://codereview.chromium.org/2417693002/diff/220001/content/browser/browse... File content/browser/browser_plugin/browser_plugin_guest.cc (right): https://codereview.chromium.org/2417693002/diff/220001/content/browser/browse... content/browser/browser_plugin/browser_plugin_guest.cc:144: ->CreateRenderFrameProxy(owner_site_instance); On 2016/10/27 22:06:48, EhsanK wrote: > On 2016/10/27 21:03:40, nasko (slow) wrote: > > This doesn't seem right, as we take the SiteInstance from the frame, which > might > > not be the root, yet we create the proxy in the root frame. > > This was my main question from you on this CL. Thanks for bringing it up! > > This is what I understand so far: > > 1- We do create the RFPH in the root, but it is the root of the guest frame > tree. This way the RFP in the embedder frame's process can be used to post > message to the guest. I didn't realize at the time that GetEmbedderFrame() and GetWebContents() come from two different frame trees. I think what you have is correct - GetWebContents() will return the guest one, which needs a proxy in the embedder process for it. So using the SiteInstance of the embedder frame is correct. > > Shouldn't this be > > > GetEmbedderFrame()->frame_tree_node()->render_manager()->CreateRenderFrameProxy()? > 2- This would technically create the reverse path for post messaging, i.e., RFP > in guest process to message back (right?). However, calling this on the a frame > tree node which is not root and does not have a RenderViewHostImpl yet, will > cause a browser crash through CHECK: > https://cs.chromium.org/chromium/src/content/browser/frame_host/render_frame_... > > So my question is, a) Is it OK to create RFPH in the node which is not main > frame given the context here: https://crbug.com/485520 and b) if yes, then how > can I do it? is it OK to remove the CHECK above? https://codereview.chromium.org/2417693002/diff/220001/content/public/browser... File content/public/browser/browser_plugin_guest_delegate.cc (right): https://codereview.chromium.org/2417693002/diff/220001/content/public/browser... content/public/browser/browser_plugin_guest_delegate.cc:41: return GetOwnerWebContents()->GetMainFrame(); On 2016/10/27 22:06:48, EhsanK wrote: > On 2016/10/27 21:03:40, nasko (slow) wrote: > > Why return the main frame here? Isn't this supposed to be the frame that > > contains the GuestView? If you have A->B->C->GuestView, why would we return A > > instead of C? > > The only way we can have A->B->C->GuestView right now is to have <embed > src="foo.pdf"> inside the inner most <iframe>. For that, the override of this > method in MimeHandlerViewGuest will return C's RenderFrameHost. > > I don't think we can have guests in <iframe> unless the guest is a PDF. But I > would like to divert this to lfg@, lazyboy@, wjmaclean@ for confirmation. Why not? I don't think we have any code restriction enforcing that <webview> needs to be always in the top-level frame. Maybe the only enforcement is other code making the same assumption and not working otherwise. I still would like to understand it, even though this code doesn't exist here anymore, but the same is moved to GuestViewBase::GetEmbedderFrame. I see the addition of BrowserPluginGuestDelegate::CanBeEmbeddedInsideCrossProcessFrames, but it doesn't have to be a cross-process frame, just a regular App v2 with <iframe> to another document in its crx, which ends up having a <webview>. https://codereview.chromium.org/2417693002/diff/220001/content/public/browser... File content/public/browser/browser_plugin_guest_delegate.h (right): https://codereview.chromium.org/2417693002/diff/220001/content/public/browser... content/public/browser/browser_plugin_guest_delegate.h:102: // BrowserPlugin. This is the main frame for the owner of the guests, unless, On 2016/10/27 22:06:48, EhsanK wrote: > On 2016/10/27 21:03:40, nasko (slow) wrote: > > Based on the implementation, it returns the main frame of the embedder. A > > GuestView can be embedded in arbitrary level of iframe nesting, right? > > As far as I can tell, only MimeHandlerViewGuest can be embedded inside an > <iframe>. So for all other guest views, the way it is implemented now, will > return |owner_web_contents_->GetMainFrame()|. I haven't had a chance to try it manually, but other than things not working properly, it should be possible to use <webview> inside an <iframe> inside an App v2. https://codereview.chromium.org/2417693002/diff/220001/content/public/browser... content/public/browser/browser_plugin_guest_delegate.h:103: // the guest is embedded inside a cross origin frame. On 2016/10/27 22:06:48, EhsanK wrote: > On 2016/10/27 21:03:40, nasko (slow) wrote: > > Can we have cross-origin frames in apps? > > I don't think we can use <iframe>'s in apps at all. For external content we are > supposed to use <webview> there. AFAIK, you can use <iframes> to local files in the crx of the app. https://codereview.chromium.org/2417693002/diff/280001/content/browser/browse... File content/browser/browser_plugin/browser_plugin_guest.cc (right): https://codereview.chromium.org/2417693002/diff/280001/content/browser/browse... content/browser/browser_plugin/browser_plugin_guest.cc:260: RenderWidgetHostImpl* rwhi = rfhi ? rfhi->GetRenderWidgetHost() : nullptr; In what cases will this result in rwhi being nullptr? Can you add a comment explaining? https://codereview.chromium.org/2417693002/diff/280001/content/browser/browse... File content/browser/browser_plugin/browser_plugin_guest.h (right): https://codereview.chromium.org/2417693002/diff/280001/content/browser/browse... content/browser/browser_plugin/browser_plugin_guest.h:284: RenderFrameHostImpl* GetEmbedderFrame() const; nit: We tried to use inner/outer instead of guest/embedder in the content/ code.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
nasko@/creis@: Please take a look. I also have one question from wjmaclean@ on RenderWidgetHostInputEventRouter. Thanks! https://codereview.chromium.org/2417693002/diff/220001/components/guest_view/... File components/guest_view/browser/guest_view_base.cc (right): https://codereview.chromium.org/2417693002/diff/220001/components/guest_view/... components/guest_view/browser/guest_view_base.cc:420: GetEmbedderFrame()->GetView()->GetRenderWidgetHost()->Send( On 2016/11/09 01:13:07, nasko (out until Nov 17) wrote: > On 2016/10/27 22:06:48, EhsanK wrote: > > On 2016/10/27 21:03:40, nasko (slow) wrote: > > > Before this IPC was sent through the main frame. Now it is sent through the > > > widget. Is this intentional? Guest attaching doesn't strike me as a > > > graphics/input event type message, but I can be wrong. > > > > As I understand, WebContentsImpl::Send will send its message through > > RenderViewHostImpl which will send it through RenderWidgetHostImpl (of the > > page?). That is why I transferred it to RenderWidgetHost here. > > They all send through the same IPC channel. What matters at the end is having > the correct routing ID. > > > Also, this IPC specifically is handled by a GuestViewContainerDispatcher which > > is a RenderThreadObserver. Acknowledged. https://codereview.chromium.org/2417693002/diff/220001/content/browser/browse... File content/browser/browser_plugin/browser_plugin_guest.cc (right): https://codereview.chromium.org/2417693002/diff/220001/content/browser/browse... content/browser/browser_plugin/browser_plugin_guest.cc:144: ->CreateRenderFrameProxy(owner_site_instance); On 2016/11/09 01:13:07, nasko wrote: > On 2016/10/27 22:06:48, EhsanK wrote: > > On 2016/10/27 21:03:40, nasko (slow) wrote: > > > This doesn't seem right, as we take the SiteInstance from the frame, which > > might > > > not be the root, yet we create the proxy in the root frame. > > > > This was my main question from you on this CL. Thanks for bringing it up! > > > > This is what I understand so far: > > > > 1- We do create the RFPH in the root, but it is the root of the guest frame > > tree. This way the RFP in the embedder frame's process can be used to post > > message to the guest. > > I didn't realize at the time that GetEmbedderFrame() and GetWebContents() come > from two different frame trees. I think what you have is correct - > GetWebContents() will return the guest one, which needs a proxy in the embedder > process for it. So using the SiteInstance of the embedder frame is correct. > > > > Shouldn't this be > > > > > > GetEmbedderFrame()->frame_tree_node()->render_manager()->CreateRenderFrameProxy()? > > 2- This would technically create the reverse path for post messaging, i.e., > RFP > > in guest process to message back (right?). However, calling this on the a > frame > > tree node which is not root and does not have a RenderViewHostImpl yet, will > > cause a browser crash through CHECK: > > > https://cs.chromium.org/chromium/src/content/browser/frame_host/render_frame_... > > > > So my question is, a) Is it OK to create RFPH in the node which is not main > > frame given the context here: https://crbug.com/485520 and b) if yes, then how > > can I do it? is it OK to remove the CHECK above? > Acknowledged. https://codereview.chromium.org/2417693002/diff/220001/content/public/browser... File content/public/browser/browser_plugin_guest_delegate.cc (right): https://codereview.chromium.org/2417693002/diff/220001/content/public/browser... content/public/browser/browser_plugin_guest_delegate.cc:41: return GetOwnerWebContents()->GetMainFrame(); On 2016/11/09 01:13:07, nasko wrote: > On 2016/10/27 22:06:48, EhsanK wrote: > > On 2016/10/27 21:03:40, nasko (slow) wrote: > > > Why return the main frame here? Isn't this supposed to be the frame that > > > contains the GuestView? If you have A->B->C->GuestView, why would we return > A > > > instead of C? > > > > The only way we can have A->B->C->GuestView right now is to have <embed > > src="foo.pdf"> inside the inner most <iframe>. For that, the override of this > > method in MimeHandlerViewGuest will return C's RenderFrameHost. > > > > I don't think we can have guests in <iframe> unless the guest is a PDF. But I > > would like to divert this to lfg@, lazyboy@, wjmaclean@ for confirmation. > > Why not? I don't think we have any code restriction enforcing that <webview> > needs to be always in the top-level frame. Maybe the only enforcement is other > code making the same assumption and not working otherwise. I still would like to > understand it, even though this code doesn't exist here anymore, but the same is > moved to GuestViewBase::GetEmbedderFrame. > > I see the addition of > BrowserPluginGuestDelegate::CanBeEmbeddedInsideCrossProcessFrames, but it > doesn't have to be a cross-process frame, just a regular App v2 with <iframe> to > another document in its crx, which ends up having a <webview>. I double checked and as you said three is ways to have <iframe>-ed <webview>, e.g., adding a html file to "web_accessible_resources" and creating <webview> inside <html>. But that <iframe> is not cross process. So the embedder frame will still be the |owner_web_contents_|'s frame. > I see the addition of > BrowserPluginGuestDelegate::CanBeEmbeddedInsideCrossProcessFrames, but it > doesn't have to be a cross-process frame, just a regular App v2 with <iframe> to > another document in its crx, which ends up having a <webview>. But I don't think that is a cross-process frame. As long as it is an in-process frame the embedder could be the main frame which is in the same process (i.e., |owner_web_contents_|). I agree however that the actual embedder frame is different from the main frame. But I don't think that matters unless the in-process frame has its own RenderWidgetHost which should not be the case when we <iframe> in the scenario you explained (there is no middle remote frame embedding the <iframe>). https://codereview.chromium.org/2417693002/diff/220001/content/public/browser... File content/public/browser/browser_plugin_guest_delegate.h (right): https://codereview.chromium.org/2417693002/diff/220001/content/public/browser... content/public/browser/browser_plugin_guest_delegate.h:102: // BrowserPlugin. This is the main frame for the owner of the guests, unless, On 2016/11/09 01:13:07, nasko wrote: > On 2016/10/27 22:06:48, EhsanK wrote: > > On 2016/10/27 21:03:40, nasko (slow) wrote: > > > Based on the implementation, it returns the main frame of the embedder. A > > > GuestView can be embedded in arbitrary level of iframe nesting, right? > > > > As far as I can tell, only MimeHandlerViewGuest can be embedded inside an > > <iframe>. So for all other guest views, the way it is implemented now, will > > return |owner_web_contents_->GetMainFrame()|. > > I haven't had a chance to try it manually, but other than things not working > properly, it should be possible to use <webview> inside an <iframe> inside an > App v2. You are right but I have not been able to do it inside an OOPIF. As long as the <iframe> is in the same process as the main frame, we could as well assume the main frame is the embedder, right? https://codereview.chromium.org/2417693002/diff/220001/content/public/browser... content/public/browser/browser_plugin_guest_delegate.h:103: // the guest is embedded inside a cross origin frame. On 2016/11/09 01:13:07, nasko wrote: > On 2016/10/27 22:06:48, EhsanK wrote: > > On 2016/10/27 21:03:40, nasko (slow) wrote: > > > Can we have cross-origin frames in apps? > > > > I don't think we can use <iframe>'s in apps at all. For external content we > are > > supposed to use <webview> there. > > AFAIK, you can use <iframes> to local files in the crx of the app. You are correct. I could do this using web_accessible_resources. But I could not find a way to make the <iframe> cross origin. I talked to lazyboy@ and he also agrees on "possibly no way to have guest inside OOPIF". So in short, I guess we might be able to add the machinery to support it, but there seems to be no apparent use case for it. In case we intent to support guest in OOPIF for all guests, I can work in a separate CL to avoid making this one too large. What do you suggest? https://codereview.chromium.org/2417693002/diff/260001/chrome/browser/chrome_... File chrome/browser/chrome_site_per_process_browsertest.cc (right): https://codereview.chromium.org/2417693002/diff/260001/chrome/browser/chrome_... chrome/browser/chrome_site_per_process_browsertest.cc:341: guest_view_manager->WaitForSingleGuestCreated(1U); On 2016/11/08 23:21:45, alexmos wrote: > The 1U is not needed anymore. Thanks! Of course, this also caused bots to go read. https://codereview.chromium.org/2417693002/diff/280001/content/browser/browse... File content/browser/browser_plugin/browser_plugin_guest.h (right): https://codereview.chromium.org/2417693002/diff/280001/content/browser/browse... content/browser/browser_plugin/browser_plugin_guest.h:284: RenderFrameHostImpl* GetEmbedderFrame() const; On 2016/11/09 01:13:07, nasko wrote: > nit: We tried to use inner/outer instead of guest/embedder in the content/ code. Acknowledged. But since we expose this method to components/ and extensions/ maybe GetOwnerFrame() is better. This is also consistent with GetOwnerWebContents() which already exists here and GetOwnerRenderWidgetHostView() which belongs to BrowserPluginGuest. WDYT? https://codereview.chromium.org/2417693002/diff/380001/content/browser/render... File content/browser/renderer_host/render_widget_host_input_event_router.cc (right): https://codereview.chromium.org/2417693002/diff/380001/content/browser/render... content/browser/renderer_host/render_widget_host_input_event_router.cc:204: while (owner_view->IsRenderWidgetHostViewGuest()) { We need to send all the input events to the embedder of the RenderWidgetHostViewGuest but apparently that cannot be a RenderWidgetHostViewGuest it self or the routing fails. wjmaclean@: Does this seem OK to you. Without adding this I could not type into a PDF inside a <webview> which was based on BrowserPlugin.
https://codereview.chromium.org/2417693002/diff/380001/content/browser/render... File content/browser/renderer_host/render_widget_host_input_event_router.cc (right): https://codereview.chromium.org/2417693002/diff/380001/content/browser/render... content/browser/renderer_host/render_widget_host_input_event_router.cc:204: while (owner_view->IsRenderWidgetHostViewGuest()) { On 2016/11/15 19:54:22, EhsanK wrote: > We need to send all the input events to the embedder of the > RenderWidgetHostViewGuest but apparently that cannot be a > RenderWidgetHostViewGuest it self or the routing fails. > > wjmaclean@: Does this seem OK to you. Without adding this I could not type into > a PDF inside a <webview> which was based on BrowserPlugin. The idea of finding the first non-RWHVG owner seems ok, given the reasons why we were going to the root_view originally ... that cod certainly never was intended to handle the case of multipally-embedded PDFs ... lgtm.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for your patience looking through these issues. It looks pretty close, but I'm concerned about the frame-based API (and would like to avoid the DEPS changes if we can, though I can live with that if needed). https://codereview.chromium.org/2417693002/diff/220001/content/public/browser... File content/public/browser/browser_plugin_guest_delegate.cc (right): https://codereview.chromium.org/2417693002/diff/220001/content/public/browser... content/public/browser/browser_plugin_guest_delegate.cc:41: return GetOwnerWebContents()->GetMainFrame(); On 2016/11/15 19:54:22, EhsanK wrote: > On 2016/11/09 01:13:07, nasko wrote: > > On 2016/10/27 22:06:48, EhsanK wrote: > > > On 2016/10/27 21:03:40, nasko (slow) wrote: > > > > Why return the main frame here? Isn't this supposed to be the frame that > > > > contains the GuestView? If you have A->B->C->GuestView, why would we > return > > A > > > > instead of C? > > > > > > The only way we can have A->B->C->GuestView right now is to have <embed > > > src="foo.pdf"> inside the inner most <iframe>. For that, the override of > this > > > method in MimeHandlerViewGuest will return C's RenderFrameHost. > > > > > > I don't think we can have guests in <iframe> unless the guest is a PDF. But > I > > > would like to divert this to lfg@, lazyboy@, wjmaclean@ for confirmation. > > > > Why not? I don't think we have any code restriction enforcing that <webview> > > needs to be always in the top-level frame. Maybe the only enforcement is other > > code making the same assumption and not working otherwise. I still would like > to > > understand it, even though this code doesn't exist here anymore, but the same > is > > moved to GuestViewBase::GetEmbedderFrame. > > > > I see the addition of > > BrowserPluginGuestDelegate::CanBeEmbeddedInsideCrossProcessFrames, but it > > doesn't have to be a cross-process frame, just a regular App v2 with <iframe> > to > > another document in its crx, which ends up having a <webview>. > > I double checked and as you said three is ways to have <iframe>-ed <webview>, > e.g., adding a html file to "web_accessible_resources" and creating <webview> > inside <html>. But that <iframe> is not cross process. So the embedder frame > will still be the |owner_web_contents_|'s frame. > > > I see the addition of > > BrowserPluginGuestDelegate::CanBeEmbeddedInsideCrossProcessFrames, but it > > doesn't have to be a cross-process frame, just a regular App v2 with <iframe> > to > > another document in its crx, which ends up having a <webview>. > But I don't think that is a cross-process frame. As long as it is an in-process > frame the embedder could be the main frame which is in the same process (i.e., > |owner_web_contents_|). I agree however that the actual embedder frame is > different from the main frame. But I don't think that matters unless the > in-process frame has > its own RenderWidgetHost which should not be the case when we <iframe> in the > scenario you explained (there is no middle remote frame embedding the <iframe>). This seems wrong to me. We shouldn't be returning the wrong frame in these cases, even if it shares a widget and process. Future use cases might care about the frame itself and get the wrong answer. As I'm noting on the GetOwnerFrame() declaration in PS20, I think we should have different APIs if what you really care about is the widget, process, and SiteInstance. We should avoid having a frame-based API if we're not returning the right frame. https://codereview.chromium.org/2417693002/diff/220001/content/public/browser... File content/public/browser/browser_plugin_guest_delegate.h (right): https://codereview.chromium.org/2417693002/diff/220001/content/public/browser... content/public/browser/browser_plugin_guest_delegate.h:103: // the guest is embedded inside a cross origin frame. On 2016/11/15 19:54:22, EhsanK wrote: > On 2016/11/09 01:13:07, nasko wrote: > > On 2016/10/27 22:06:48, EhsanK wrote: > > > On 2016/10/27 21:03:40, nasko (slow) wrote: > > > > Can we have cross-origin frames in apps? > > > > > > I don't think we can use <iframe>'s in apps at all. For external content we > > are > > > supposed to use <webview> there. > > > > AFAIK, you can use <iframes> to local files in the crx of the app. > > You are correct. I could do this using web_accessible_resources. But I could not > find a way to make the <iframe> cross origin. I talked to lazyboy@ and he also > agrees on "possibly no way to have guest inside OOPIF". > > So in short, I guess we might be able to add the machinery to support it, but > there seems to be no apparent use case for it. > > In case we intent to support guest in OOPIF for all guests, I can work in a > separate CL to avoid making this one too large. What do you suggest? We don't need to support the more general case in this CL, but let's avoid the frame-based API if we can. https://codereview.chromium.org/2417693002/diff/400001/content/browser/browse... File content/browser/browser_plugin/browser_plugin_guest.cc (right): https://codereview.chromium.org/2417693002/diff/400001/content/browser/browse... content/browser/browser_plugin/browser_plugin_guest.cc:146: // create a RenderFrameProxyHost for the reverse path, or, implement nit: No comma after "or" https://codereview.chromium.org/2417693002/diff/400001/content/browser/browse... content/browser/browser_plugin/browser_plugin_guest.cc:147: // MimeHandlerViewGuest's using OOPIF (https://crbug.com/659750). nit: No apostrophe. https://codereview.chromium.org/2417693002/diff/400001/content/browser/render... File content/browser/renderer_host/DEPS (right): https://codereview.chromium.org/2417693002/diff/400001/content/browser/render... content/browser/renderer_host/DEPS:50: "+content/browser/frame_host/render_widget_host_view_guest.h", I'd really like to avoid making changes to DEPS if possible, especially in a block that has a TODO to be removed. Why is it needed? Can we avoid it? (If it has to stay, please fix the indentation and update the comment.) https://codereview.chromium.org/2417693002/diff/400001/content/public/browser... File content/public/browser/browser_plugin_guest_delegate.h (right): https://codereview.chromium.org/2417693002/diff/400001/content/public/browser... content/public/browser/browser_plugin_guest_delegate.h:99: // the guest is embedded inside a cross origin frame. Sorry, I don't think this API makes sense. The first sentence is incorrect if the BrowserPlugin is inside a same-origin iframe rather than the main frame. It may happen to work out because the current callers only care about widget-level concepts (RWHV, process, SiteInstance), but it will break any future code that assumes it returns the actual embedding frame. Would returning the owner RenderWidgetHost or RenderWidgetHostView work instead? Maybe with a separate GetOwnerSiteInstance since RenderWidgetHost doesn't know its SiteInstance?
The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Charlie, could you please take another look. While working on redoing the API I noticed a bug in cleaning up the BrowserPluginGuest state (fixed now). The issue was that removing the <iframe> (frameDetached or process dead) would not destroy the guest since the guest used to only track the lifetime of owner WebContents. I now changed it so that MimeHandler guests track life time of the RFH embedding them. Without observing the RFH, after removing the frame the browser side state lingers around until the tab/owner dies. Testing locally, things work and state cleans up as expected. PTAL. https://codereview.chromium.org/2417693002/diff/380001/content/browser/render... File content/browser/renderer_host/render_widget_host_input_event_router.cc (right): https://codereview.chromium.org/2417693002/diff/380001/content/browser/render... content/browser/renderer_host/render_widget_host_input_event_router.cc:204: while (owner_view->IsRenderWidgetHostViewGuest()) { On 2016/11/15 20:02:39, wjmaclean wrote: > On 2016/11/15 19:54:22, EhsanK wrote: > > We need to send all the input events to the embedder of the > > RenderWidgetHostViewGuest but apparently that cannot be a > > RenderWidgetHostViewGuest it self or the routing fails. > > > > wjmaclean@: Does this seem OK to you. Without adding this I could not type > into > > a PDF inside a <webview> which was based on BrowserPlugin. > > The idea of finding the first non-RWHVG owner seems ok, given the reasons why we > were going to the root_view originally ... that cod certainly never was intended > to handle the case of multipally-embedded PDFs ... lgtm. Thanks. I added a short comment just to be more clear on this. https://codereview.chromium.org/2417693002/diff/400001/content/browser/browse... File content/browser/browser_plugin/browser_plugin_guest.cc (right): https://codereview.chromium.org/2417693002/diff/400001/content/browser/browse... content/browser/browser_plugin/browser_plugin_guest.cc:146: // create a RenderFrameProxyHost for the reverse path, or, implement On 2016/11/15 23:45:33, Charlie Reis (slow) wrote: > nit: No comma after "or" Done. https://codereview.chromium.org/2417693002/diff/400001/content/browser/browse... content/browser/browser_plugin/browser_plugin_guest.cc:147: // MimeHandlerViewGuest's using OOPIF (https://crbug.com/659750). On 2016/11/15 23:45:33, Charlie Reis (slow) wrote: > nit: No apostrophe. Done. https://codereview.chromium.org/2417693002/diff/400001/content/browser/render... File content/browser/renderer_host/DEPS (right): https://codereview.chromium.org/2417693002/diff/400001/content/browser/render... content/browser/renderer_host/DEPS:50: "+content/browser/frame_host/render_widget_host_view_guest.h", On 2016/11/15 23:45:33, Charlie Reis (slow) wrote: > I'd really like to avoid making changes to DEPS if possible, especially in a > block that has a TODO to be removed. Why is it needed? Can we avoid it? > > (If it has to stay, please fix the indentation and update the comment.) I actually explained this in patch 12 (copying over): > > On 2016/11/02 20:47:06, Charlie Reis (OOO til 11-11) wrote: > > Why does this need to be here, if it's implemented as a NOTREACHED and only > > callable on the subclass (RWHVG)? Would it be easier to cast to RWHVG at the > > only callsite (RenderWidgetHostInputEventRouter::RouteMouseEvent) and leave it > > out of this class? > Yes, it would definitely be easier (lfg@ also suggested this > earlier). I avoided > doing this originally because this is a layering violation (RWHVG > lives inside > frame_host/ while RWHIER is inside renderer_host). > But as I understand this approached is being favored over adding > the method to > RWHVCF esp. given that we already violate this for RWHVCF (there > is also a > comment about it inside the DEPS file for renderer_host/). > I made the change and modified the DEPS file. Also please note that in RenderWidgetHostInputEventRouter we need to know the target is a RWHVG so that the mouse event is sent to the embedder (not directly routed). Right now I see the two options of 1) keeping the violation here, 2) making GetOwnerRenderWidgetHostView() a virtual method of RWHVCF. For 2) it is also worth mentioning that we already have a method RenderWidgetHostViewBase::IsRenderWidgetHostViewGuest which is a bit out of place but necessary, and for 1), well there is already a violation here. WDYT? https://codereview.chromium.org/2417693002/diff/400001/content/public/browser... File content/public/browser/browser_plugin_guest_delegate.h (right): https://codereview.chromium.org/2417693002/diff/400001/content/public/browser... content/public/browser/browser_plugin_guest_delegate.h:99: // the guest is embedded inside a cross origin frame. On 2016/11/15 23:45:33, Charlie Reis (slow) wrote: > Sorry, I don't think this API makes sense. The first sentence is incorrect if > the BrowserPlugin is inside a same-origin iframe rather than the main frame. > Yes you are right. I was too focused on the OOPIF case when adding this comment. > It may happen to work out because the current callers only care about > widget-level concepts (RWHV, process, SiteInstance), but it will break any > future code that assumes it returns the actual embedding frame. Agreed. Basically, the whole guest->embedder communication is through the RenderWidgetHost. > Would returning the owner RenderWidgetHost or RenderWidgetHostView work instead? > Maybe with a separate GetOwnerSiteInstance since RenderWidgetHost doesn't know > its SiteInstance? Done.
Thanks for updating the API. LGTM with nits! On 2016/11/16 21:27:31, EhsanK wrote: > Charlie, could you please take another look. > > While working on redoing the API I noticed a bug in cleaning up the > BrowserPluginGuest state (fixed now). > > The issue was that removing the <iframe> (frameDetached or process dead) would > not destroy the guest since the guest used to only track the lifetime of owner > WebContents. I now changed it so that MimeHandler guests track life time of the > RFH embedding them. Without observing the RFH, after removing the frame the > browser side state lingers around until the tab/owner dies. Thanks for catching that bug as well. Is that something you could add a test for in a followup CL? https://codereview.chromium.org/2417693002/diff/400001/content/browser/render... File content/browser/renderer_host/DEPS (right): https://codereview.chromium.org/2417693002/diff/400001/content/browser/render... content/browser/renderer_host/DEPS:50: "+content/browser/frame_host/render_widget_host_view_guest.h", On 2016/11/16 21:27:31, EhsanK wrote: > On 2016/11/15 23:45:33, Charlie Reis (slow) wrote: > > I'd really like to avoid making changes to DEPS if possible, especially in a > > block that has a TODO to be removed. Why is it needed? Can we avoid it? > > > > (If it has to stay, please fix the indentation and update the comment.) > > I actually explained this in patch 12 (copying over): > > > On 2016/11/02 20:47:06, Charlie Reis (OOO til 11-11) wrote: > > > Why does this need to be here, if it's implemented as a NOTREACHED and only > > > callable on the subclass (RWHVG)? Would it be easier to cast to RWHVG at > the > > > only callsite (RenderWidgetHostInputEventRouter::RouteMouseEvent) and leave > it > > > out of this class? > > > Yes, it would definitely be easier (lfg@ also suggested this > earlier). I > avoided > > doing this originally because this is a layering violation (RWHVG > lives > inside > > frame_host/ while RWHIER is inside renderer_host). > > > But as I understand this approached is being favored over adding > the method > to > > RWHVCF esp. given that we already violate this for RWHVCF (there > is also a > > comment about it inside the DEPS file for renderer_host/). > > > I made the change and modified the DEPS file. > > Also please note that in RenderWidgetHostInputEventRouter we need to know the > target is a RWHVG so that the mouse event is sent to the embedder (not directly > routed). > > Right now I see the two options of 1) keeping the violation here, 2) making > GetOwnerRenderWidgetHostView() a virtual method of RWHVCF. For 2) it is also > worth mentioning that we already have a method > RenderWidgetHostViewBase::IsRenderWidgetHostViewGuest which is a bit out of > place but necessary, and for 1), well there is already a violation here. WDYT? Thanks, and sorry for missing the earlier explanation. I'd lean towards #2, but I'm ok doing that in a separate CL to avoid holding this one up further. (Out of curiosity, will RWHVGuest itself go away as we replace BrowserPlugin with OOPIFs, or will it always be necessary?) https://codereview.chromium.org/2417693002/diff/480001/components/guest_view/... File components/guest_view/browser/guest_view_base.cc (right): https://codereview.chromium.org/2417693002/diff/480001/components/guest_view/... components/guest_view/browser/guest_view_base.cc:749: return owner_contents->GetSiteInstance(); Please put comments in both of these methods about the assumption that frames can exist inside the owner but not OOPIFs (i.e., that the guest may be inside a subframe but it's guaranteed to have the same widget and SiteInstance as the main frame). That's a useful breadcrumb in case we ever change that behavior. https://codereview.chromium.org/2417693002/diff/480001/components/guest_view/... File components/guest_view/browser/guest_view_base.h (right): https://codereview.chromium.org/2417693002/diff/480001/components/guest_view/... components/guest_view/browser/guest_view_base.h:218: // methdo when BrowserPlugin is removed (https://crbug.com/535197). nit: method https://codereview.chromium.org/2417693002/diff/480001/content/browser/render... File content/browser/renderer_host/DEPS (right): https://codereview.chromium.org/2417693002/diff/480001/content/browser/render... content/browser/renderer_host/DEPS:50: # all inner WebContents are based on OOPIF structure (as opposed to nit: removed when https://codereview.chromium.org/2417693002/diff/480001/extensions/browser/gue... File extensions/browser/guest_view/mime_handler_view/mime_handler_view_guest.cc (right): https://codereview.chromium.org/2417693002/diff/480001/extensions/browser/gue... extensions/browser/guest_view/mime_handler_view/mime_handler_view_guest.cc:127: DCHECK_NE(embedder_widget_routing_id_, MSG_ROUTING_NONE); nit: Reverse order (expected, actual).
The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks Charlie. I will CQ soon. I will also submit a few follow-up CLs (at least one to add a another test + remove DEPS change). https://codereview.chromium.org/2417693002/diff/400001/content/browser/render... File content/browser/renderer_host/DEPS (right): https://codereview.chromium.org/2417693002/diff/400001/content/browser/render... content/browser/renderer_host/DEPS:50: "+content/browser/frame_host/render_widget_host_view_guest.h", On 2016/11/16 22:25:26, Charlie Reis (slow) wrote: > On 2016/11/16 21:27:31, EhsanK wrote: > > On 2016/11/15 23:45:33, Charlie Reis (slow) wrote: > > > I'd really like to avoid making changes to DEPS if possible, especially in a > > > block that has a TODO to be removed. Why is it needed? Can we avoid it? > > > > > > (If it has to stay, please fix the indentation and update the comment.) > > > > I actually explained this in patch 12 (copying over): > > > > On 2016/11/02 20:47:06, Charlie Reis (OOO til 11-11) wrote: > > > > Why does this need to be here, if it's implemented as a NOTREACHED and > only > > > > callable on the subclass (RWHVG)? Would it be easier to cast to RWHVG at > > the > > > > only callsite (RenderWidgetHostInputEventRouter::RouteMouseEvent) and > leave > > it > > > > out of this class? > > > > > Yes, it would definitely be easier (lfg@ also suggested this > earlier). I > > avoided > > > doing this originally because this is a layering violation (RWHVG > lives > > inside > > > frame_host/ while RWHIER is inside renderer_host). > > > > > But as I understand this approached is being favored over adding > the > method > > to > > > RWHVCF esp. given that we already violate this for RWHVCF (there > is also a > > > comment about it inside the DEPS file for renderer_host/). > > > > > I made the change and modified the DEPS file. > > > > Also please note that in RenderWidgetHostInputEventRouter we need to know the > > target is a RWHVG so that the mouse event is sent to the embedder (not > directly > > routed). > > > > Right now I see the two options of 1) keeping the violation here, 2) making > > GetOwnerRenderWidgetHostView() a virtual method of RWHVCF. For 2) it is also > > worth mentioning that we already have a method > > RenderWidgetHostViewBase::IsRenderWidgetHostViewGuest which is a bit out of > > place but necessary, and for 1), well there is already a violation here. WDYT? > > Thanks, and sorry for missing the earlier explanation. > > I'd lean towards #2, but I'm ok doing that in a separate CL to avoid holding > this one up further. > > (Out of curiosity, will RWHVGuest itself go away as we replace BrowserPlugin > with OOPIFs, or will it always be necessary?) Sure. I will revert this to patch 12 in a separate followup CL (with a new test added as you suggested). https://codereview.chromium.org/2417693002/diff/480001/components/guest_view/... File components/guest_view/browser/guest_view_base.cc (right): https://codereview.chromium.org/2417693002/diff/480001/components/guest_view/... components/guest_view/browser/guest_view_base.cc:749: return owner_contents->GetSiteInstance(); On 2016/11/16 22:25:27, Charlie Reis (slow) wrote: > Please put comments in both of these methods about the assumption that frames > can exist inside the owner but not OOPIFs (i.e., that the guest may be inside a > subframe but it's guaranteed to have the same widget and SiteInstance as the > main frame). > > That's a useful breadcrumb in case we ever change that behavior. Done. https://codereview.chromium.org/2417693002/diff/480001/components/guest_view/... File components/guest_view/browser/guest_view_base.h (right): https://codereview.chromium.org/2417693002/diff/480001/components/guest_view/... components/guest_view/browser/guest_view_base.h:218: // methdo when BrowserPlugin is removed (https://crbug.com/535197). On 2016/11/16 22:25:27, Charlie Reis (slow) wrote: > nit: method Done. https://codereview.chromium.org/2417693002/diff/480001/content/browser/render... File content/browser/renderer_host/DEPS (right): https://codereview.chromium.org/2417693002/diff/480001/content/browser/render... content/browser/renderer_host/DEPS:50: # all inner WebContents are based on OOPIF structure (as opposed to On 2016/11/16 22:25:27, Charlie Reis (slow) wrote: > nit: removed when Done. https://codereview.chromium.org/2417693002/diff/480001/extensions/browser/gue... File extensions/browser/guest_view/mime_handler_view/mime_handler_view_guest.cc (right): https://codereview.chromium.org/2417693002/diff/480001/extensions/browser/gue... extensions/browser/guest_view/mime_handler_view/mime_handler_view_guest.cc:127: DCHECK_NE(embedder_widget_routing_id_, MSG_ROUTING_NONE); On 2016/11/16 22:25:27, Charlie Reis (slow) wrote: > nit: Reverse order (expected, actual). Acknowledged.
The CQ bit was unchecked by ekaramad@chromium.org
The CQ bit was checked by ekaramad@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lfg@chromium.org, alexmos@chromium.org, wjmaclean@chromium.org, creis@chromium.org Link to the patchset: https://codereview.chromium.org/2417693002/#ps520001 (title: "Addressing creis@'s comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by ekaramad@chromium.org
The CQ bit was checked by ekaramad@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for
content/browser/browser_plugin/browser_plugin_guest.cc:
While running git apply --index -p1;
error: patch failed:
content/browser/browser_plugin/browser_plugin_guest.cc:506
error: content/browser/browser_plugin/browser_plugin_guest.cc: patch does not
apply
Patch: content/browser/browser_plugin/browser_plugin_guest.cc
Index: content/browser/browser_plugin/browser_plugin_guest.cc
diff --git a/content/browser/browser_plugin/browser_plugin_guest.cc
b/content/browser/browser_plugin/browser_plugin_guest.cc
index
1d6931d0a4a3d74c1085e76670112c08327f32f7..208343833cfc415d23b77862a32c18d6dfcb2984
100644
--- a/content/browser/browser_plugin/browser_plugin_guest.cc
+++ b/content/browser/browser_plugin/browser_plugin_guest.cc
@@ -126,20 +126,31 @@ int BrowserPluginGuest::GetGuestProxyRoutingID() {
if (guest_proxy_routing_id_ != MSG_ROUTING_NONE)
return guest_proxy_routing_id_;
- // Create a RenderFrameProxyHost for the guest in the embedder renderer
- // process, so that the embedder can access the guest's window object.
- // On reattachment, we can reuse the same RenderFrameProxyHost because
- // the embedder process will always be the same even if the embedder
- // WebContents changes.
+ // In order to enable the embedder to post messages to the
+ // guest, we need to create a RenderFrameProxyHost in root node of guest
+ // WebContents' frame tree (i.e., create a RenderFrameProxy in the embedder
+ // process which can be used by the embedder to post messages to the guest).
+ // The creation of RFPH for the reverse path, which enables the guest to post
+ // messages to the embedder, will be postponed to when the embedder posts its
+ // first message to the guest.
//
// TODO(fsamuel): Make sure this works for transferring guests across
// owners in different processes. We probably need to clear the
// |guest_proxy_routing_id_| and perform any necessary cleanup on Detach
// to enable this.
- SiteInstance* owner_site_instance = owner_web_contents_->GetSiteInstance();
- int proxy_routing_id =
- GetWebContents()->GetFrameTree()->root()->render_manager()->
- CreateRenderFrameProxy(owner_site_instance);
+ //
+ // TODO(ekaramad): If the guest is embedded inside a cross-process <iframe>
+ // (e.g., <embed>-ed PDF), the reverse proxy will not be created and the
+ // posted message's source attribute will be null which in turn breaks the
+ // two-way messaging between the guest and the embedder. We should either
+ // create a RenderFrameProxyHost for the reverse path, or implement
+ // MimeHandlerViewGuest using OOPIF (https://crbug.com/659750).
+ SiteInstance* owner_site_instance = delegate_->GetOwnerSiteInstance();
+ int proxy_routing_id = GetWebContents()
+ ->GetFrameTree()
+ ->root()
+ ->render_manager()
+ ->CreateRenderFrameProxy(owner_site_instance);
guest_proxy_routing_id_ = RenderFrameProxyHost::FromID(
owner_site_instance->GetProcess()->GetID(), proxy_routing_id)
->GetRenderViewHost()->GetRoutingID();
@@ -170,6 +181,11 @@ void BrowserPluginGuest::WillDestroy() {
attached_ = false;
}
+RenderWidgetHostImpl* BrowserPluginGuest::GetOwnerRenderWidgetHost() const {
+ return static_cast<RenderWidgetHostImpl*>(
+ delegate_->GetOwnerRenderWidgetHost());
+}
+
void BrowserPluginGuest::Init() {
if (initialized_)
return;
@@ -241,12 +257,11 @@ bool BrowserPluginGuest::OnMessageReceivedFromEmbedder(
const IPC::Message& message) {
RenderWidgetHostViewGuest* rwhv = static_cast<RenderWidgetHostViewGuest*>(
web_contents()->GetRenderWidgetHostView());
+
// Until the guest is attached, it should not be handling input events.
if (attached() && rwhv &&
- rwhv->OnMessageReceivedFromEmbedder(
- message,
- RenderWidgetHostImpl::From(
- embedder_web_contents()->GetRenderViewHost()->GetWidget()))) {
+ rwhv->OnMessageReceivedFromEmbedder(message,
+ GetOwnerRenderWidgetHost())) {
return true;
}
@@ -374,9 +389,9 @@ bool BrowserPluginGuest::IsGuest(RenderViewHostImpl*
render_view_host) {
}
RenderWidgetHostView* BrowserPluginGuest::GetOwnerRenderWidgetHostView() {
- if (!owner_web_contents_)
- return nullptr;
- return owner_web_contents_->GetRenderWidgetHostView();
+ if (RenderWidgetHostImpl* owner = GetOwnerRenderWidgetHost())
+ return owner->GetView();
+ return nullptr;
}
void BrowserPluginGuest::UpdateVisibility() {
@@ -448,8 +463,8 @@ void BrowserPluginGuest::ResendEventToEmbedder(
return;
DCHECK(browser_plugin_instance_id_);
- RenderWidgetHostViewBase* view = static_cast<RenderWidgetHostViewBase*>(
- embedder_web_contents()->GetMainFrame()->GetView());
+ RenderWidgetHostViewBase* view =
+ static_cast<RenderWidgetHostViewBase*>(GetOwnerRenderWidgetHostView());
gfx::Vector2d offset_from_embedder = guest_window_rect_.OffsetFromOrigin();
if (event.type == blink::WebInputEvent::GestureScrollUpdate) {
@@ -506,7 +521,12 @@ void BrowserPluginGuest::SendMessageToEmbedder(
pending_messages_.push_back(std::move(msg));
return;
}
- owner_web_contents_->Send(msg.release());
+
+ // If the guest is inside a cross-process frame, it is possible to get here
+ // after the owner frame is detached. Then, the owner RenderWidgetHost will
+ // be null and the message is dropped.
+ if (auto* rwh = GetOwnerRenderWidgetHost())
+ rwh->Send(msg.release());
}
void BrowserPluginGuest::DragSourceEndedAt(int client_x, int client_y,
The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by ekaramad@chromium.org
The CQ bit was checked by ekaramad@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wjmaclean@chromium.org, alexmos@chromium.org, creis@chromium.org, lfg@chromium.org Link to the patchset: https://codereview.chromium.org/2417693002/#ps540001 (title: "Rebased")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_TIMED_OUT, no build URL) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_TIMED_OUT, no build URL) android_compile_dbg on master.tryserver.chromium.android (JOB_TIMED_OUT, no build URL) android_cronet on master.tryserver.chromium.android (JOB_TIMED_OUT, no build URL) cast_shell_android on master.tryserver.chromium.android (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by creis@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Allow MimeHandlerViewGuest be embedded inside OOPIFs. Event routing for BrowserPlugin-based guests, along with some other IPCs, is through the embedder's RenderWidgetHost. Normally the embedder is the owner WebContents. However, when loading a PDF inside an out of process <iframe>, considering the WebContents as the embedder is incorrect as the actual BrowserPlugin lives in the <iframe> process. This CL will use the RWH corresponding to the embedder frame for event routing. BUG=649856 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Allow MimeHandlerViewGuest be embedded inside OOPIFs. Event routing for BrowserPlugin-based guests, along with some other IPCs, is through the embedder's RenderWidgetHost. Normally the embedder is the owner WebContents. However, when loading a PDF inside an out of process <iframe>, considering the WebContents as the embedder is incorrect as the actual BrowserPlugin lives in the <iframe> process. This CL will use the RWH corresponding to the embedder frame for event routing. BUG=649856 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Message was sent while issue was closed.
Committed patchset #27 (id:540001)
Message was sent while issue was closed.
Description was changed from ========== Allow MimeHandlerViewGuest be embedded inside OOPIFs. Event routing for BrowserPlugin-based guests, along with some other IPCs, is through the embedder's RenderWidgetHost. Normally the embedder is the owner WebContents. However, when loading a PDF inside an out of process <iframe>, considering the WebContents as the embedder is incorrect as the actual BrowserPlugin lives in the <iframe> process. This CL will use the RWH corresponding to the embedder frame for event routing. BUG=649856 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Allow MimeHandlerViewGuest be embedded inside OOPIFs. Event routing for BrowserPlugin-based guests, along with some other IPCs, is through the embedder's RenderWidgetHost. Normally the embedder is the owner WebContents. However, when loading a PDF inside an out of process <iframe>, considering the WebContents as the embedder is incorrect as the actual BrowserPlugin lives in the <iframe> process. This CL will use the RWH corresponding to the embedder frame for event routing. BUG=649856 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/51ca2097b0b7c1a6097c63690b5ca982b65924e4 Cr-Commit-Position: refs/heads/master@{#432779} ==========
Message was sent while issue was closed.
Patchset 27 (id:??) landed as https://crrev.com/51ca2097b0b7c1a6097c63690b5ca982b65924e4 Cr-Commit-Position: refs/heads/master@{#432779} |
