Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2821473002:
Service CreateNewWindow on the UI thread with a new mojo interface (Closed)
|
Created:
3 years, 8 months ago by Charlie Harrison Modified:
3 years, 6 months ago Reviewers:
Bernhard Bauer, Ken Rockot(use gerrit already), ncarter (slow), sgurun-gerrit only, Devlin, dcheng, alexmos, nasko, slan CC:
Aaron Boodman, abarth-chromium, chromium-reviews, creis+watch_chromium.org, darin (slow to review), darin-cc_chromium.org, jam, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionService CreateNewWindow on the UI thread with a new mojo interface
This patch replaces the IO-thread handler for CreateNewWindow with a
UI-thread version, using a new mojo interface called FrameHost. The
existing FrameHost interface is renamed to FrameHostInterfaceBroker.
This is an associated interface, so messages will remain ordered with
respect to the traditional IPC pipe.
There are a few changes that happened as a result of this approach:
1. ContentBrowserClient::CanCreateWindow is now on the UI thread. This
means policy makers need some UI thread context. Since the UI thread is
usually better at making policy decisions in general, most components
were simplified here.
This was the main impetus of this change, because we wanted an easy
place to put window opening policy decisions.
2. Since the mojo interface is now frame-based, all window opening logic
moved to RenderFrameHostImpl.
3. window.open() is a sync IPC. This change makes the sync wait longer
because we create the UI thread objects before replying.
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation
BUG=35980
TBR=slan@chromium.org,sgurun@chromium.org
Review-Url: https://codereview.chromium.org/2821473002
Cr-Commit-Position: refs/heads/master@{#466700}
Committed: https://chromium.googlesource.com/chromium/src/+/95f01e92e7147bee4e66ce629712fd1f50808936
Patch Set 1 #
Total comments: 3
Patch Set 2 : put CanCreateNewWindow on UI thread (no associated interface yet) #Patch Set 3 : Add mocks and set up blink trybots #Patch Set 4 : fix AW and Chromecast #Patch Set 5 : associated with IPC channel #
Total comments: 16
Patch Set 6 : fix up logic errors #
Total comments: 8
Patch Set 7 : Move over to render frame (but not WebFrameClient yet) + a bunch of cleanups #
Total comments: 2
Patch Set 8 : fix refactoring bug #Patch Set 9 : rebase on #465379 #Patch Set 10 : tweaks #
Total comments: 4
Patch Set 11 : devlin review #
Total comments: 14
Patch Set 12 : alexmos review #Patch Set 13 : rebase on #465869 #
Total comments: 2
Patch Set 14 : dcheng fixes + security exploit browsertest nerfing #
Total comments: 39
Patch Set 15 : re-do tests, dependent PS, etc. #Patch Set 16 : early return fix #Patch Set 17 : security exploit test passes a non null callback #
Total comments: 5
Patch Set 18 : s/FrameHostIPC/FrameHost and s/FrameHost/FrameInterfaceBroker/ and fix up tests #
Total comments: 1
Patch Set 19 : s/FrameInterfaceBroker/FrameHostInterfaceBroker and s/frame_interface_broker/frame_host_interface_b… #
Total comments: 11
Patch Set 20 : bauerb review #Patch Set 21 : MakeShared goodness #
Total comments: 2
Dependent Patchsets: Messages
Total messages: 158 (100 generated)
The CQ bit was checked by csharrison@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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
csharrison@chromium.org changed reviewers: + dcheng@chromium.org
dcheng: Would you PTAL high level at this WIP? I'd like some quick feedback before I continue, as I'm not super familiar with this code. Namely in this CL I completely punted on: 1. Naming. RenderMessageFilterUI is an abomination. 2. Migrating the CanCreateNewWindow code to work on the UI thread. 3. MockRenderThread implementation Feel free to bikeshed on the naming / layering (should this be a per-frame interface maybe?). https://codereview.chromium.org/2821473002/diff/1/content/renderer/render_thr... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/2821473002/diff/1/content/renderer/render_thr... content/renderer/render_thread_impl.cc:2067: blink_platform_impl_->GetInterfaceProvider()->GetInterface( This is weird and wrong. Probably should just use the connector directly?
On 2017/04/13 19:27:58, Charlie Harrison wrote: > dcheng: Would you PTAL high level at this WIP? I'd like some quick feedback > before I continue, as I'm not super familiar with this code. > > Namely in this CL I completely punted on: > 1. Naming. RenderMessageFilterUI is an abomination. > 2. Migrating the CanCreateNewWindow code to work on the UI thread. > 3. MockRenderThread implementation > > Feel free to bikeshed on the naming / layering (should this be a per-frame > interface maybe?). We could try per-frame, but that would be more involved -- we would plumb it through the opener frame. But if you're willing to try it... https://codereview.chromium.org/2821473002/diff/1/content/renderer/render_thr... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/2821473002/diff/1/content/renderer/render_thr... content/renderer/render_thread_impl.cc:2067: blink_platform_impl_->GetInterfaceProvider()->GetInterface( On 2017/04/13 19:27:58, Charlie Harrison wrote: > This is weird and wrong. Probably should just use the connector directly? Maybe this should be an associated interface to keep the same behavior?
https://codereview.chromium.org/2821473002/diff/1/content/renderer/render_thr... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/2821473002/diff/1/content/renderer/render_thr... content/renderer/render_thread_impl.cc:2067: blink_platform_impl_->GetInterfaceProvider()->GetInterface( On 2017/04/13 21:00:14, dcheng wrote: > On 2017/04/13 19:27:58, Charlie Harrison wrote: > > This is weird and wrong. Probably should just use the connector directly? > > Maybe this should be an associated interface to keep the same behavior? Hm... How would that work? Isn't it fundamentally against the spirit of an associated interface (same underlying pipe) for endpoints to be on two different threads?
On 2017/04/13 21:48:13, Charlie Harrison wrote: > https://codereview.chromium.org/2821473002/diff/1/content/renderer/render_thr... > File content/renderer/render_thread_impl.cc (right): > > https://codereview.chromium.org/2821473002/diff/1/content/renderer/render_thr... > content/renderer/render_thread_impl.cc:2067: > blink_platform_impl_->GetInterfaceProvider()->GetInterface( > On 2017/04/13 21:00:14, dcheng wrote: > > On 2017/04/13 19:27:58, Charlie Harrison wrote: > > > This is weird and wrong. Probably should just use the connector directly? > > > > Maybe this should be an associated interface to keep the same behavior? > > Hm... How would that work? Isn't it fundamentally against the spirit of an > associated interface (same underlying pipe) for endpoints to be on two different > threads? Associated interfaces don't have to live on the same thread. The endpoints don't have to be on the same thread; all the message sending/receiving happens only on one thread. But I'm thinking that maybe we want this, so that we have the same ordering semantics in Blink.
On 2017/04/14 01:17:30, dcheng wrote: > On 2017/04/13 21:48:13, Charlie Harrison wrote: > > > https://codereview.chromium.org/2821473002/diff/1/content/renderer/render_thr... > > File content/renderer/render_thread_impl.cc (right): > > > > > https://codereview.chromium.org/2821473002/diff/1/content/renderer/render_thr... > > content/renderer/render_thread_impl.cc:2067: > > blink_platform_impl_->GetInterfaceProvider()->GetInterface( > > On 2017/04/13 21:00:14, dcheng wrote: > > > On 2017/04/13 19:27:58, Charlie Harrison wrote: > > > > This is weird and wrong. Probably should just use the connector directly? > > > > > > Maybe this should be an associated interface to keep the same behavior? > > > > Hm... How would that work? Isn't it fundamentally against the spirit of an > > associated interface (same underlying pipe) for endpoints to be on two > different > > threads? > > Associated interfaces don't have to live on the same thread. The endpoints don't > have to be on the same thread; all the message sending/receiving happens only on > one thread. But I'm thinking that maybe we want this, so that we have the same > ordering semantics in Blink. By "this" do you mean an associated interface with one endpoint living on the UI thread, or what we have currently?
On 2017/04/14 03:21:11, Charlie Harrison wrote: > On 2017/04/14 01:17:30, dcheng wrote: > > On 2017/04/13 21:48:13, Charlie Harrison wrote: > > > > > > https://codereview.chromium.org/2821473002/diff/1/content/renderer/render_thr... > > > File content/renderer/render_thread_impl.cc (right): > > > > > > > > > https://codereview.chromium.org/2821473002/diff/1/content/renderer/render_thr... > > > content/renderer/render_thread_impl.cc:2067: > > > blink_platform_impl_->GetInterfaceProvider()->GetInterface( > > > On 2017/04/13 21:00:14, dcheng wrote: > > > > On 2017/04/13 19:27:58, Charlie Harrison wrote: > > > > > This is weird and wrong. Probably should just use the connector > directly? > > > > > > > > Maybe this should be an associated interface to keep the same behavior? > > > > > > Hm... How would that work? Isn't it fundamentally against the spirit of an > > > associated interface (same underlying pipe) for endpoints to be on two > > different > > > threads? > > > > Associated interfaces don't have to live on the same thread. The endpoints > don't > > have to be on the same thread; all the message sending/receiving happens only > on > > one thread. But I'm thinking that maybe we want this, so that we have the same > > ordering semantics in Blink. > > By "this" do you mean an associated interface with one endpoint living on the UI > thread, or what we have currently? An associated interface living on the UI thread. If you're feeling brave, I suppose we could try to have it non-associated and see what happens...
The CQ bit was checked by csharrison@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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
Description was changed from ========== WIP service CreateNewWindow on the UI thread with a new mojo interface BUG= ========== to ========== WIP service CreateNewWindow on the UI thread with a new mojo interface CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel BUG= ==========
The CQ bit was checked by csharrison@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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by csharrison@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.
Description was changed from ========== WIP service CreateNewWindow on the UI thread with a new mojo interface CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel BUG= ========== to ========== Service CreateNewWindow on the UI thread with a new mojo interface CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel BUG= ==========
The CQ bit was checked by csharrison@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...
csharrison@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
dcheng: trybots seemed happy in PS4 but the latest one uses an associated interface. I'm not confident I've done this correctly and mostly cribbed from [1]. LMK what you think. devlin: The purpose of this patch is to put ChromeContentBrowserClient::CanCreateNewWindow on the UI thread. This method is mostly checks made by extensions, but I think I converted them mostly correctly to use the right Profile scoped objects rather than ProfileIOData. It really needs a set of eyes on it though. Also, let me know if there's a file where this logic belongs, I couldn't find a good fit (maybe a new file extension_permission_utils?). [1]: https://codereview.chromium.org/2643183002/
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Service CreateNewWindow on the UI thread with a new mojo interface CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel BUG= ========== to ========== Service CreateNewWindow on the UI thread with a new mojo interface CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel BUG=35980 ==========
rdevlin.cronin@chromium.org changed reviewers: + alexmos@chromium.org
oooh, fun! :) I'll need to take a closer look at this on Monday. Alex has also been in some related areas lately, and I'd feel better having an extra pair of security eyes on this.
alexmos@chromium.org changed reviewers: + nick@chromium.org
Exciting! Nick has recently refactored a lot of this plumbing as part of making these IPCs frame-based, so I think it'd be good for him to take a look at this as well. I'm sheriffing today so only had time for a quick look. I'm not a mojo expert so will defer to Daniel/Nick on that, but it'd be awesome if we're able to do this. https://codereview.chromium.org/2821473002/diff/80001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2821473002/diff/80001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:2436: const GURL& opener_top_level_frame_url, Yay, doing this might allow us to fix this for OOPIFs, as we can just look up the opener's top frame URL on the UI thread. Currently, we fall back to setting this to top frame's origin with OOPIFs, which breaks path matching for file URLs. (This is from https://crbug.com/466297.) I'll plan to do this cleanup as a followup. https://codereview.chromium.org/2821473002/diff/80001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:2471: extension->permissions_data()->HasAPIPermission( This doesn't seem to quite match the old check. The cases where SecurityOriginHasAPIPermission returned true used to fall through to the AllowJSAccess check below, but now they return immediately. https://codereview.chromium.org/2821473002/diff/80001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:2485: if (!has_permission) Is this right? Or should it be outside the loop (along with a break after "has_permission = true")? https://codereview.chromium.org/2821473002/diff/80001/content/browser/rendere... File content/browser/renderer_host/render_message_filter_ui.cc (right): https://codereview.chromium.org/2821473002/diff/80001/content/browser/rendere... content/browser/renderer_host/render_message_filter_ui.cc:97: block_requests_for_route); Is this ok from race conditions point of view? I don't really know the history of this check, but is it possible for a request (I guess from a plugin, given the comment above?) to come in before block_requests_for_route gets posted to the IO thread?
https://codereview.chromium.org/2821473002/diff/80001/content/common/render_m... File content/common/render_message_filter.mojom (right): https://codereview.chromium.org/2821473002/diff/80001/content/common/render_m... content/common/render_message_filter.mojom:96: interface RenderMessageFilterUI { Did you consider just routing this to the opener RenderFrameHost?
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Thanks for the quick review, alexmos. https://codereview.chromium.org/2821473002/diff/80001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2821473002/diff/80001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:2436: const GURL& opener_top_level_frame_url, On 2017/04/17 19:41:07, alexmos wrote: > Yay, doing this might allow us to fix this for OOPIFs, as we can just look up > the opener's top frame URL on the UI thread. Currently, we fall back to setting > this to top frame's origin with OOPIFs, which breaks path matching for file > URLs. (This is from https://crbug.com/466297.) I'll plan to do this cleanup as > a followup. w00t! https://codereview.chromium.org/2821473002/diff/80001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:2471: extension->permissions_data()->HasAPIPermission( On 2017/04/17 19:41:07, alexmos wrote: > This doesn't seem to quite match the old check. The cases where > SecurityOriginHasAPIPermission returned true used to fall through to the > AllowJSAccess check below, but now they return immediately. Yep, fixed. https://codereview.chromium.org/2821473002/diff/80001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:2485: if (!has_permission) On 2017/04/17 19:41:07, alexmos wrote: > Is this right? Or should it be outside the loop (along with a break after > "has_permission = true")? Yep, fixed. https://codereview.chromium.org/2821473002/diff/80001/content/browser/rendere... File content/browser/renderer_host/render_message_filter_ui.cc (right): https://codereview.chromium.org/2821473002/diff/80001/content/browser/rendere... content/browser/renderer_host/render_message_filter_ui.cc:97: block_requests_for_route); On 2017/04/17 19:41:08, alexmos wrote: > Is this ok from race conditions point of view? I don't really know the history > of this check, but is it possible for a request (I guess from a plugin, given > the comment above?) to come in before block_requests_for_route gets posted to > the IO thread? I filed crbug.com/581037 a while ago trying to rip this logic out. AFAIK we don't need this for the plugin case since the NPAPI deprecation (plugins no longer need the HWND). When I briefly tried to remove it the only thing failing was <webview> (as far as I remember). From a pure race condition perspective I think we should be fine, given that a task posted from here will be strictly before any IPC handler task for a resource request. At least, that's how I assume IPC message handlers work but if I'm wrong we could be in more trouble. https://codereview.chromium.org/2821473002/diff/80001/content/common/render_m... File content/common/render_message_filter.mojom (right): https://codereview.chromium.org/2821473002/diff/80001/content/common/render_m... content/common/render_message_filter.mojom:96: interface RenderMessageFilterUI { On 2017/04/17 19:56:38, ncarter wrote: > Did you consider just routing this to the opener RenderFrameHost? Yep, discussed this with dcheng@ and we considered it. I'll happily do it in this CL if you prefer it.
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/2821473002/diff/80001/chrome/browser/android/... File chrome/browser/android/webapps/single_tab_mode_tab_helper.cc (right): https://codereview.chromium.org/2821473002/diff/80001/chrome/browser/android/... chrome/browser/android/webapps/single_tab_mode_tab_helper.cc:80: return itr != g_blocked_ids.Get().end(); On the UI thread, I think we we don't need g_blocked_ids/AddPair/RemovePair at all? Instead we can just do something like this: WebContents* web_contents = WebContents::FromRenderFrameHost(RenderFrameHost::FromID(process_id, frame_routing_id)); if (web_contents) { SingleTabModeTabHelper* tab_helper = SingleTabModeTabHelper::FromWebContents(web_contents); return tab_helper && tab_helper->block_all_new_windows_; } If that works, then we can delete both the globals and the WebContentsObserver methods. https://codereview.chromium.org/2821473002/diff/80001/content/common/render_m... File content/common/render_message_filter.mojom (right): https://codereview.chromium.org/2821473002/diff/80001/content/common/render_m... content/common/render_message_filter.mojom:96: interface RenderMessageFilterUI { On 2017/04/17 20:10:52, Charlie Harrison wrote: > On 2017/04/17 19:56:38, ncarter wrote: > > Did you consider just routing this to the opener RenderFrameHost? > > Yep, discussed this with dcheng@ and we considered it. I'll happily do it in > this CL if you prefer it. I think it's preferable to use the opener here, since CreateNewWindow is essentially an operation on the opener frame already. Creating a UI version (RenderMessageFilterUI) of the IO helper (RenderMessageFilter) of UI thread entity (RenderProcessHost / RenderFrameHost) is a bit like "Street Fighter: The Movie: The Game". One thing that at first glance gets a little tricky, is the cleanup-after-failure logic -- the rph->Send(new ViewMsg_Close) that used to happen in OnCreateNewWindowOnUI. However, on reflection, I think it actually gets greatly simplified. The original complexity there arose from how we would mint (and reply with) routing IDs on the IO thread, before validating any of the parameters (such as the opener_ route ID) that could only be validated on the UI thread. In that case we're left trying to cancel a half-created RenderView. But if we mint the routing IDs on the UI thread, we can actually now validate all those params too, and just reply with MSG_ROUTING_NONE if validation fails.
https://codereview.chromium.org/2821473002/diff/80001/content/common/render_m... File content/common/render_message_filter.mojom (right): https://codereview.chromium.org/2821473002/diff/80001/content/common/render_m... content/common/render_message_filter.mojom:96: interface RenderMessageFilterUI { On 2017/04/17 20:44:26, ncarter wrote: > On 2017/04/17 20:10:52, Charlie Harrison wrote: > > On 2017/04/17 19:56:38, ncarter wrote: > > > Did you consider just routing this to the opener RenderFrameHost? > > > > Yep, discussed this with dcheng@ and we considered it. I'll happily do it in > > this CL if you prefer it. > > I think it's preferable to use the opener here, since CreateNewWindow is > essentially an operation on the opener frame already. > > Creating a UI version (RenderMessageFilterUI) of the IO helper > (RenderMessageFilter) of UI thread entity (RenderProcessHost / RenderFrameHost) > is a bit like "Street Fighter: The Movie: The Game". > > One thing that at first glance gets a little tricky, is the > cleanup-after-failure logic -- the rph->Send(new ViewMsg_Close) that used to > happen in OnCreateNewWindowOnUI. However, on reflection, I think it actually > gets greatly simplified. The original complexity there arose from how we would > mint (and reply with) routing IDs on the IO thread, before validating any of the > parameters (such as the opener_ route ID) that could only be validated on the UI > thread. In that case we're left trying to cancel a half-created RenderView. But > if we mint the routing IDs on the UI thread, we can actually now validate all > those params too, and just reply with MSG_ROUTING_NONE if validation fails. I did some more thinking, and I think routing it via opener frame makes sense. Originally I was concerned about the case where an iframe removes itself from the DOM, and then tries to open a new window--but in this case already doesn't work today, so we're not breaking anything.
https://codereview.chromium.org/2821473002/diff/100001/content/renderer/rende... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/2821473002/diff/100001/content/renderer/rende... content/renderer/render_view_impl.cc:1398: WebView* RenderViewImpl::CreateView(WebLocalFrame* creator, FYI: dcheng and I had discussed moving this method from WebViewClient to WebFrameClient. I think WebFrameClient is the right place: while this is an operation that may create a WebView, it is an operation on a particular (opener) WebFrame. I mention this, in case it becomes relevant with moving to use a frame-associated mojo interface.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
A few nits, but nothing overly glaring in the latest patch set https://codereview.chromium.org/2821473002/diff/100001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2821473002/diff/100001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:2462: // TODO(csharrison): Consider moving this logic into a static method Maybe just do this now in a local anonymous namespace to make it more readable. https://codereview.chromium.org/2821473002/diff/100001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:2469: id, extensions::ExtensionRegistry::ENABLED); nit: prefer registry->enabled_extensions().GetByID(id) https://codereview.chromium.org/2821473002/diff/100001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:2479: for (const auto& extension_id : This loop should only be done if the scheme isn't kExtensionScheme, right?
Description was changed from ========== Service CreateNewWindow on the UI thread with a new mojo interface CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel BUG=35980 ========== to ========== Service CreateNewWindow on the UI thread with a new mojo interface CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation BUG=35980 ==========
The CQ bit was checked by csharrison@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 everyone, I appreciate the reviews. Current CL routes through the opener and applies some 1st order refactoring. https://codereview.chromium.org/2821473002/diff/80001/chrome/browser/android/... File chrome/browser/android/webapps/single_tab_mode_tab_helper.cc (right): https://codereview.chromium.org/2821473002/diff/80001/chrome/browser/android/... chrome/browser/android/webapps/single_tab_mode_tab_helper.cc:80: return itr != g_blocked_ids.Get().end(); On 2017/04/17 20:44:26, ncarter wrote: > On the UI thread, I think we we don't need g_blocked_ids/AddPair/RemovePair at > all? > > Instead we can just do something like this: > > WebContents* web_contents = > WebContents::FromRenderFrameHost(RenderFrameHost::FromID(process_id, > frame_routing_id)); > if (web_contents) { > SingleTabModeTabHelper* tab_helper = > SingleTabModeTabHelper::FromWebContents(web_contents); > return tab_helper && tab_helper->block_all_new_windows_; > } > > If that works, then we can delete both the globals and the WebContentsObserver > methods. Looks good, done. https://codereview.chromium.org/2821473002/diff/80001/content/common/render_m... File content/common/render_message_filter.mojom (right): https://codereview.chromium.org/2821473002/diff/80001/content/common/render_m... content/common/render_message_filter.mojom:96: interface RenderMessageFilterUI { On 2017/04/17 20:50:40, dcheng (OOO through May 2) wrote: > On 2017/04/17 20:44:26, ncarter wrote: > > On 2017/04/17 20:10:52, Charlie Harrison wrote: > > > On 2017/04/17 19:56:38, ncarter wrote: > > > > Did you consider just routing this to the opener RenderFrameHost? > > > > > > Yep, discussed this with dcheng@ and we considered it. I'll happily do it in > > > this CL if you prefer it. > > > > I think it's preferable to use the opener here, since CreateNewWindow is > > essentially an operation on the opener frame already. > > > > Creating a UI version (RenderMessageFilterUI) of the IO helper > > (RenderMessageFilter) of UI thread entity (RenderProcessHost / > RenderFrameHost) > > is a bit like "Street Fighter: The Movie: The Game". > > > > One thing that at first glance gets a little tricky, is the > > cleanup-after-failure logic -- the rph->Send(new ViewMsg_Close) that used to > > happen in OnCreateNewWindowOnUI. However, on reflection, I think it actually > > gets greatly simplified. The original complexity there arose from how we would > > mint (and reply with) routing IDs on the IO thread, before validating any of > the > > parameters (such as the opener_ route ID) that could only be validated on the > UI > > thread. In that case we're left trying to cancel a half-created RenderView. > But > > if we mint the routing IDs on the UI thread, we can actually now validate all > > those params too, and just reply with MSG_ROUTING_NONE if validation fails. > > I did some more thinking, and I think routing it via opener frame makes sense. > > Originally I was concerned about the case where an iframe removes itself from > the DOM, and then tries to open a new window--but in this case already doesn't > work today, so we're not breaking anything. Alright the current version attempts to route through the opener. https://codereview.chromium.org/2821473002/diff/100001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2821473002/diff/100001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:2462: // TODO(csharrison): Consider moving this logic into a static method On 2017/04/17 21:45:42, Devlin wrote: > Maybe just do this now in a local anonymous namespace to make it more readable. Done. https://codereview.chromium.org/2821473002/diff/100001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:2469: id, extensions::ExtensionRegistry::ENABLED); On 2017/04/17 21:45:42, Devlin wrote: > nit: prefer registry->enabled_extensions().GetByID(id) Done. https://codereview.chromium.org/2821473002/diff/100001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:2479: for (const auto& extension_id : On 2017/04/17 21:45:42, Devlin wrote: > This loop should only be done if the scheme isn't kExtensionScheme, right? Done. https://codereview.chromium.org/2821473002/diff/100001/content/renderer/rende... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/2821473002/diff/100001/content/renderer/rende... content/renderer/render_view_impl.cc:1398: WebView* RenderViewImpl::CreateView(WebLocalFrame* creator, On 2017/04/17 20:52:15, ncarter wrote: > FYI: dcheng and I had discussed moving this method from WebViewClient to > WebFrameClient. I think WebFrameClient is the right place: while this is an > operation that may create a WebView, it is an operation on a particular (opener) > WebFrame. > > I mention this, in case it becomes relevant with moving to use a > frame-associated mojo interface. Good idea. I've added a todo to do this in a followup, since this CL is getting kinda big. https://codereview.chromium.org/2821473002/diff/120001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2821473002/diff/120001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2524: if (main_frame_route_id != MSG_ROUTING_NONE && ncarter: is this what you meant about sending MSG_ROUTING_NONE if we failed to successfully create the new window?
I again had trouble with naming. This is a FrameHost interface (which already exists). Is there a canonical way to distinguish Associated interfaces with non-Associated? If I actually append "Associated" to the word we can end up with "AssociatedAssociated" in some types.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by csharrison@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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) 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_ozone_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_...)
https://codereview.chromium.org/2821473002/diff/120001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2821473002/diff/120001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2524: if (main_frame_route_id != MSG_ROUTING_NONE && On 2017/04/18 20:38:53, Charlie Harrison wrote: > ncarter: is this what you meant about sending MSG_ROUTING_NONE if we failed to > successfully create the new window? Yes, this is what I was suggesting. I'll think about it more deeply as part of the overall review but it looks correct at first glance. (One somewhat awful corner case here is the bug fixed by this CL -- https://chromium.googlesource.com/chromium/src/+/0d558487988e9047ca7b6b26e170... -- where, inside of OnCreateNewWindow, we actually created and deleted the popup; this can happens in practice on Android, but unfortunately I didn't write a test for it; IIRC it involved a window.open racing a user-initiated tab close. But from what I can see, we're only improving things here.)
The CQ bit was checked by csharrison@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_...)
The CQ bit was checked by csharrison@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.
csharrison@chromium.org changed reviewers: + nasko@chromium.org
Looks like folks are going OOO, so adding nasko@ for IPC OWNERS and for another overall check.
extensions related lgtm https://codereview.chromium.org/2821473002/diff/180001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2821473002/diff/180001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:975: // Returns true if there is exists an extension with the same origin as nit: if there is, or if there exists, not if there is exists :) https://codereview.chromium.org/2821473002/diff/180001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:983: if (source_origin.SchemeIs(extensions::kExtensionScheme)) { Sorry I didn't notice this before, but I think this can be simplified: const extensions::Extension* extension = registry->enabled_extensions().GetExtensionOrAppByURL(source_origin); return extension && extension->permissions_data()->HasAPIPermission(APIPermission::kBackground) && process_map->Contains(extension->id(), opener_render_process_id); It took me awhile to convince myself that was right, but I'm pretty sure it is, and makes this method quite a bit easier to digest. Sorry for the churn!
The CQ bit was checked by csharrison@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/2821473002/diff/180001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2821473002/diff/180001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:975: // Returns true if there is exists an extension with the same origin as On 2017/04/19 15:20:10, Devlin wrote: > nit: if there is, or if there exists, not if there is exists :) Done. https://codereview.chromium.org/2821473002/diff/180001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:983: if (source_origin.SchemeIs(extensions::kExtensionScheme)) { On 2017/04/19 15:20:10, Devlin wrote: > Sorry I didn't notice this before, but I think this can be simplified: > > const extensions::Extension* extension = > registry->enabled_extensions().GetExtensionOrAppByURL(source_origin); > return extension && > > extension->permissions_data()->HasAPIPermission(APIPermission::kBackground) && > process_map->Contains(extension->id(), opener_render_process_id); > > It took me awhile to convince myself that was right, but I'm pretty sure it is, > and makes this method quite a bit easier to digest. Sorry for the churn! Done. This is a great simplification that GetExtensionOrAppByURL includes the web extent urls.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I made another pass through this before heading out on vacation - overall LGTM with some comments below. I'll delegate to nick@ and nasko@ to follow up on those and finish reviewing. Thanks again for doing this! https://codereview.chromium.org/2821473002/diff/80001/content/browser/rendere... File content/browser/renderer_host/render_message_filter_ui.cc (right): https://codereview.chromium.org/2821473002/diff/80001/content/browser/rendere... content/browser/renderer_host/render_message_filter_ui.cc:97: block_requests_for_route); On 2017/04/17 20:10:52, Charlie Harrison wrote: > On 2017/04/17 19:41:08, alexmos wrote: > > Is this ok from race conditions point of view? I don't really know the > history > > of this check, but is it possible for a request (I guess from a plugin, given > > the comment above?) to come in before block_requests_for_route gets posted to > > the IO thread? > > I filed crbug.com/581037 a while ago trying to rip this logic out. AFAIK we > don't need this for the plugin case since the NPAPI deprecation (plugins no > longer need the HWND). When I briefly tried to remove it the only thing failing > was <webview> (as far as I remember). > > > From a pure race condition perspective I think we should be fine, given that a > task posted from here will be strictly before any IPC handler task for a > resource request. At least, that's how I assume IPC message handlers work but if > I'm wrong we could be in more trouble. Acknowledged. Thanks for adding a comment with that reference. https://codereview.chromium.org/2821473002/diff/200001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2821473002/diff/200001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:1148: // ViewMsg_Close if the above step did not adopt |main_frame_route_id|. nit: update this comment. https://codereview.chromium.org/2821473002/diff/200001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2476: BrowserContext::GetStoragePartition( nit: might be a bit more readable if you declare a separate var for results of GetStoragePartition(). https://codereview.chromium.org/2821473002/diff/200001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2484: // If the opener is supppressed or script access is disallowed, we should nit: s/supppressed/suppressed/ while you're here https://codereview.chromium.org/2821473002/diff/200001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2525: RenderWidgetHost::FromID(GetProcess()->GetID(), (probably also a question for nick@) There's a comment on RenderFrameHostDelegate::CreateNewWindow: // The caller is expected to handle cleanup if this operation fails or is // suppressed, by looking for the existence of a RenderFrameHost in // source_site_instance's process with |main_frame_route_id| after this method // returns. Should we match that and check for RFH instead of RWH? That would better match with the main_frame_route_id check, though I guess it doesn't really matter, as this used to have DCHECKs ensuring that if RWH didn't exist, RFH/RVH didn't either. Might be nice to preserve those DCHECKs? https://codereview.chromium.org/2821473002/diff/200001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/2821473002/diff/200001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:259: // assigned on the IO thread. nit: update this comment. Other random thoughts: now that there's also a RFHI::CreateNewWindow, perhaps we can rename this one to something better, so it doesn't sound like an IPC handler but rather a helper for RFHI::CreateNewWindow? Or how would we feel about just combining them? Also, if we keep this, might be nice to make it private. https://codereview.chromium.org/2821473002/diff/200001/content/common/frame.m... File content/common/frame.mojom (right): https://codereview.chromium.org/2821473002/diff/200001/content/common/frame.m... content/common/frame.mojom:49: int32 opener_render_frame_id; We don't really need to pass this anymore now that we route through the opener frame. Can we remove it? Looks like WebContentsImpl::CreateNewWindow is using it, but we might as well just pass the opener RFH as another param to it. (Also this will avoid having to look up source_render_frame_host there.)
The CQ bit was checked by csharrison@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: android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) 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 csharrison@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_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2821473002/diff/200001/chrome/browser/android... File chrome/browser/android/webapps/single_tab_mode_tab_helper.h (right): https://codereview.chromium.org/2821473002/diff/200001/chrome/browser/android... chrome/browser/android/webapps/single_tab_mode_tab_helper.h:20: bool block_all_new_windows() { return block_all_new_windows_; } Nit: const https://codereview.chromium.org/2821473002/diff/240001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2821473002/diff/240001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2523: if (succeeded) { I'm a bit confused by this comment: the original check was !succeeded, and would send back MSG_ROUTING_NONE, but this seems to send back MSG_ROUTING_NONE if succeeded is true?
The CQ bit was checked by csharrison@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 dcheng. Nick, I think your previous change to move these IPCs to be frame-based made the security exploit browsertest a bit nerfed so that it does nothing. I've noted this in a comment, as I think the test needs to be fixed to actually do something. https://codereview.chromium.org/2821473002/diff/200001/chrome/browser/android... File chrome/browser/android/webapps/single_tab_mode_tab_helper.h (right): https://codereview.chromium.org/2821473002/diff/200001/chrome/browser/android... chrome/browser/android/webapps/single_tab_mode_tab_helper.h:20: bool block_all_new_windows() { return block_all_new_windows_; } On 2017/04/20 12:04:13, dcheng (OOO through May 2) wrote: > Nit: const Done. https://codereview.chromium.org/2821473002/diff/200001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2821473002/diff/200001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:1148: // ViewMsg_Close if the above step did not adopt |main_frame_route_id|. On 2017/04/19 22:28:02, alexmos (OOO until 5-2) wrote: > nit: update this comment. Done. https://codereview.chromium.org/2821473002/diff/200001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2476: BrowserContext::GetStoragePartition( On 2017/04/19 22:28:02, alexmos (OOO until 5-2) wrote: > nit: might be a bit more readable if you declare a separate var for results of > GetStoragePartition(). Done. https://codereview.chromium.org/2821473002/diff/200001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2484: // If the opener is supppressed or script access is disallowed, we should On 2017/04/19 22:28:02, alexmos (OOO until 5-2) wrote: > nit: s/supppressed/suppressed/ while you're here Done. https://codereview.chromium.org/2821473002/diff/200001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2525: RenderWidgetHost::FromID(GetProcess()->GetID(), On 2017/04/19 22:28:02, alexmos (OOO until 5-2) wrote: > (probably also a question for nick@) There's a comment on > RenderFrameHostDelegate::CreateNewWindow: > > // The caller is expected to handle cleanup if this operation fails or is > // suppressed, by looking for the existence of a RenderFrameHost in > // source_site_instance's process with |main_frame_route_id| after this method > // returns. > > Should we match that and check for RFH instead of RWH? > > That would better match with the main_frame_route_id check, though I guess it > doesn't really matter, as this used to have DCHECKs ensuring that if RWH didn't > exist, RFH/RVH didn't either. Might be nice to preserve those DCHECKs? I re-added the DCHECKs, sorry about that! https://codereview.chromium.org/2821473002/diff/200001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/2821473002/diff/200001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:259: // assigned on the IO thread. On 2017/04/19 22:28:03, alexmos (OOO until 5-2) wrote: > nit: update this comment. > > Other random thoughts: now that there's also a RFHI::CreateNewWindow, perhaps we > can rename this one to something better, so it doesn't sound like an IPC handler > but rather a helper for RFHI::CreateNewWindow? Or how would we feel about just > combining them? Also, if we keep this, might be nice to make it private. Good point, I think I slightly just prefer inlining it. https://codereview.chromium.org/2821473002/diff/200001/content/common/frame.m... File content/common/frame.mojom (right): https://codereview.chromium.org/2821473002/diff/200001/content/common/frame.m... content/common/frame.mojom:49: int32 opener_render_frame_id; On 2017/04/19 22:28:03, alexmos (OOO until 5-2) wrote: > We don't really need to pass this anymore now that we route through the opener > frame. Can we remove it? Looks like WebContentsImpl::CreateNewWindow is using > it, but we might as well just pass the opener RFH as another param to it. (Also > this will avoid having to look up source_render_frame_host there.) Done. https://codereview.chromium.org/2821473002/diff/240001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2821473002/diff/240001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2523: if (succeeded) { On 2017/04/20 12:04:13, dcheng (OOO through May 2) wrote: > I'm a bit confused by this comment: the original check was !succeeded, and would > send back MSG_ROUTING_NONE, but this seems to send back MSG_ROUTING_NONE if > succeeded is true? Sorry, it is an error! Fixed it up.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for doing this! I'm mostly curious why we need a new interface instead of using the FrameHost existing one. Yes, it isn't associated with legacy IPC right now, but there isn't much on it anyway. https://codereview.chromium.org/2821473002/diff/260001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/2821473002/diff/260001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.h:1134: mojo::AssociatedBinding<mojom::FrameHostIPC> frame_host_associated_binding_; nit: If this interface stays around, why not include the "ipc" part in the name? https://codereview.chromium.org/2821473002/diff/260001/content/common/frame.m... File content/common/frame.mojom (right): https://codereview.chromium.org/2821473002/diff/260001/content/common/frame.m... content/common/frame.mojom:54: // The session storage namespace ID this view should use. nit: Let's move away from "view". Maybe "window", since this is what we are creating and session storage is window/tab global. https://codereview.chromium.org/2821473002/diff/260001/content/common/frame.m... content/common/frame.mojom:109: interface FrameHostIPC { Why do we need a new interface here? It seems to me that FrameHost is the main interface where RenderFrame and RenderFrameHost will be connected together, so adding one just for the IPCs seems a bit strange. Also, as we move more to mojo, I'm afraid that all legacy IPCs will move to FrameHostIPC and FrameHost will be an empty interface as it is now. https://codereview.chromium.org/2821473002/diff/260001/content/common/frame.m... content/common/frame.mojom:111: // a tab for it. If |reply.route_id| is MSG_ROUTING_NONE, the view couldn't nit: s/view/window/?
+rockot Nasko: The way I see it there are two solutions to your concern: 1. Make this IPC scoped to FrameHost, and add another interface for non-associated messages, FrameHostUnordered (strawman name). 2. Move this IPC scoped to FrameHost, and just make the one message in FrameHost (GetInterfaceProvider) also associated. Ken, I've added you for mojo-philosophy wisdom. My gut feeling is that if a message doesn't need to be ordered with traditional IPC, it shouldn't. That makes me slightly prefer (1). However, (2) is definitely simpler.
https://codereview.chromium.org/2821473002/diff/260001/chrome/browser/android... File chrome/browser/android/webapps/single_tab_mode_tab_helper.h (right): https://codereview.chromium.org/2821473002/diff/260001/chrome/browser/android... chrome/browser/android/webapps/single_tab_mode_tab_helper.h:12: // Registers and unregisters the IDs of renderers in single tab mode, which // Tracks tabs in single tab mode, which are disallowed from opening new windows // via ChromeContentBrowserClient::CanCreateWindow(). https://codereview.chromium.org/2821473002/diff/260001/chrome/browser/android... chrome/browser/android/webapps/single_tab_mode_tab_helper.h:33: content::WebContents* web_contents_; #include web_contents.h https://codereview.chromium.org/2821473002/diff/260001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2821473002/diff/260001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:789: // already been deleted if the main frame navigates away. If you add the "frame_tree_node_->current_frame_host() != this" check to the top of RenderFrameHostImpl::CreateNewWindow as I suggest in a later comment, then we can remove this conditional block. (Also: with oopifs, non-root pending delete hosts are possible, so this logic had a hole that would have needed to be addressed anyway) https://codereview.chromium.org/2821473002/diff/260001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:2495: if (extension && !extensions::BackgroundInfo::AllowJSAccess(extension)) Hah. It would appear that an extension can sidestep this restriction (and get JS access) by adding a same-origin, different-scheme iframe; via about:blank, blob:, or filesystem: ... -- really, we ought to use the same extension here as the one looked up in SecurityOriginHasExtensionBackgroundPermission. We can fix this now (which adds risk to an already huge CL), or file a bug. https://codereview.chromium.org/2821473002/diff/260001/content/browser/frame_... File content/browser/frame_host/render_frame_host_delegate.h (right): https://codereview.chromium.org/2821473002/diff/260001/content/browser/frame_... content/browser/frame_host/render_frame_host_delegate.h:255: // The passed |opener| is the RenderFrameHost initiating the window creation. This comment should clarify that |opener| is never null here, even if the opener relationship on the new window is being suppressed via |params|. https://codereview.chromium.org/2821473002/diff/260001/content/browser/frame_... content/browser/frame_host/render_frame_host_delegate.h:265: // source_site_instance's process with |main_frame_route_id| after this method In these comment paragraphs, replace |source_site_instance| with |opener|. https://codereview.chromium.org/2821473002/diff/260001/content/browser/frame_... content/browser/frame_host/render_frame_host_delegate.h:269: RenderFrameHost* opener, We can eliminate |source_site_instance| now that |opener| is being passed in. https://codereview.chromium.org/2821473002/diff/260001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2821473002/diff/260001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2432: const CreateNewWindowCallback& callback) { We should have an early-return path up top here for: if (frame_tree_node_->current_frame_host() != this || !render_frame_created_). https://codereview.chromium.org/2821473002/diff/260001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2505: // particularly egregious, since an oopif isn't expected to know its top URL. Yay, we can fix this now. I'll be happy to do it in a follow-on CL. https://codereview.chromium.org/2821473002/diff/260001/content/browser/securi... File content/browser/security_exploit_browsertest.cc (right): https://codereview.chromium.org/2821473002/diff/260001/content/browser/securi... content/browser/security_exploit_browsertest.cc:274: EXPECT_NE(opener->frame_tree_node()->current_frame_host(), opener); I remember spending some time thinking about this test with the previous refactor. I agree it's basically lame now -- the original bug had to do with the fact that we were mixing a routing id from the pending RVH, with the process id of the current RVH (I think via WebContents::GetRenderProcessHost() or something). So, lately, this test's purpose was to make sure that we rejected the OnCreateNewWindow from a non-current host. I think it can still basically do that, if we add a FRIEND_TEST to be able to call the mojo fn directly? I'm also okay deleting the test. I'm pretty sure we don't have the underlying bug anymore. https://codereview.chromium.org/2821473002/diff/260001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2821473002/diff/260001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:2062: // use; they were allocated by the RenderWidgetHelper on the IO thread. The "on the IO thread" clause is now inaccurate. https://codereview.chromium.org/2821473002/diff/260001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:2116: int opener_render_frame_id = opener->GetRoutingID(); Just inline this: there are multiple render_frame_id's in scope in this function, and I think readability is maximized if we keeping the number of aliases down. https://codereview.chromium.org/2821473002/diff/260001/content/common/frame.m... File content/common/frame.mojom (right): https://codereview.chromium.org/2821473002/diff/260001/content/common/frame.m... content/common/frame.mojom:46: // process. Mention http://crbug.com/674307 https://codereview.chromium.org/2821473002/diff/260001/content/common/frame.m... content/common/frame.mojom:109: interface FrameHostIPC { On 2017/04/20 18:58:41, nasko wrote: > Why do we need a new interface here? It seems to me that FrameHost is the main > interface where RenderFrame and RenderFrameHost will be connected together, so > adding one just for the IPCs seems a bit strange. Also, as we move more to mojo, > I'm afraid that all legacy IPCs will move to FrameHostIPC and FrameHost will be > an empty interface as it is now. I'm neutral on whether we just add methods to FrameHost, or have a FrameHostIPC for messages that are handled directly by the RenderFrameHostImpl. If we're not sure, we can ask chromium-mojo for advice. If we do add the extra layer, I think 'FrameHostImpl' might be a reasonable choice of name. https://codereview.chromium.org/2821473002/diff/260001/content/test/test_rend... File content/test/test_render_frame.cc (right): https://codereview.chromium.org/2821473002/diff/260001/content/test/test_rend... content/test/test_render_frame.cc:40: mock_render_thread->OnCreateWindow(*params, reply->get()); Could we consider having MockRenderThread provide the FrameHost/FrameHostIPC interfaces naturally, so that the GetRemoteAssociatedInterfaces()->GetInterface() call in RenderFrameImpl just works? My thinking is that it's best if the "mock browser process" behaviors are implemented in terms of the IPC conversation, rather than overrides of real code. Having said that, I'm okay with things as you've implemented them -- particularly because I'm not sure if the above suggestion is actually practical. rockot@ may also have ideas here.
The CQ bit was checked by csharrison@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...
Tried to address most comments. The current PS still has FrameHostIPC though. https://codereview.chromium.org/2821473002/diff/260001/chrome/browser/android... File chrome/browser/android/webapps/single_tab_mode_tab_helper.h (right): https://codereview.chromium.org/2821473002/diff/260001/chrome/browser/android... chrome/browser/android/webapps/single_tab_mode_tab_helper.h:12: // Registers and unregisters the IDs of renderers in single tab mode, which On 2017/04/20 21:30:36, ncarter wrote: > // Tracks tabs in single tab mode, which are disallowed from opening new windows > // via ChromeContentBrowserClient::CanCreateWindow(). Done. https://codereview.chromium.org/2821473002/diff/260001/chrome/browser/android... chrome/browser/android/webapps/single_tab_mode_tab_helper.h:33: content::WebContents* web_contents_; On 2017/04/20 21:30:35, ncarter wrote: > #include web_contents.h Only needs a fwd declaration, added one. https://codereview.chromium.org/2821473002/diff/260001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2821473002/diff/260001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:789: // already been deleted if the main frame navigates away. On 2017/04/20 21:30:36, ncarter wrote: > If you add the "frame_tree_node_->current_frame_host() != this" check to the top > of RenderFrameHostImpl::CreateNewWindow as I suggest in a later comment, then we > can remove this conditional block. > > (Also: with oopifs, non-root pending delete hosts are possible, so this logic > had a hole that would have needed to be addressed anyway) Done. https://codereview.chromium.org/2821473002/diff/260001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:2495: if (extension && !extensions::BackgroundInfo::AllowJSAccess(extension)) On 2017/04/20 21:30:36, ncarter wrote: > Hah. It would appear that an extension can sidestep this restriction (and get JS > access) by adding a same-origin, different-scheme iframe; via about:blank, > blob:, or filesystem: ... > > -- really, we ought to use the same extension here as the one looked up in > SecurityOriginHasExtensionBackgroundPermission. We can fix this now (which adds > risk to an already huge CL), or file a bug. Can you explain this a bit more? I'm happy to file a bug for this. https://codereview.chromium.org/2821473002/diff/260001/content/browser/frame_... File content/browser/frame_host/render_frame_host_delegate.h (right): https://codereview.chromium.org/2821473002/diff/260001/content/browser/frame_... content/browser/frame_host/render_frame_host_delegate.h:255: // The passed |opener| is the RenderFrameHost initiating the window creation. On 2017/04/20 21:30:36, ncarter wrote: > This comment should clarify that |opener| is never null here, even if the opener > relationship on the new window is being suppressed via |params|. Done. https://codereview.chromium.org/2821473002/diff/260001/content/browser/frame_... content/browser/frame_host/render_frame_host_delegate.h:265: // source_site_instance's process with |main_frame_route_id| after this method On 2017/04/20 21:30:36, ncarter wrote: > In these comment paragraphs, replace |source_site_instance| with |opener|. Done. https://codereview.chromium.org/2821473002/diff/260001/content/browser/frame_... content/browser/frame_host/render_frame_host_delegate.h:269: RenderFrameHost* opener, On 2017/04/20 21:30:36, ncarter wrote: > We can eliminate |source_site_instance| now that |opener| is being passed in. Done. https://codereview.chromium.org/2821473002/diff/260001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2821473002/diff/260001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2432: const CreateNewWindowCallback& callback) { On 2017/04/20 21:30:36, ncarter wrote: > We should have an early-return path up top here for: > > if (frame_tree_node_->current_frame_host() != this || > !render_frame_created_). Done. https://codereview.chromium.org/2821473002/diff/260001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2505: // particularly egregious, since an oopif isn't expected to know its top URL. On 2017/04/20 21:30:36, ncarter wrote: > Yay, we can fix this now. I'll be happy to do it in a follow-on CL. :D https://codereview.chromium.org/2821473002/diff/260001/content/browser/securi... File content/browser/security_exploit_browsertest.cc (right): https://codereview.chromium.org/2821473002/diff/260001/content/browser/securi... content/browser/security_exploit_browsertest.cc:274: EXPECT_NE(opener->frame_tree_node()->current_frame_host(), opener); On 2017/04/20 21:30:36, ncarter wrote: > I remember spending some time thinking about this test with the previous > refactor. I agree it's basically lame now -- the original bug had to do with the > fact that we were mixing a routing id from the pending RVH, with the process id > of the current RVH (I think via WebContents::GetRenderProcessHost() or > something). > > So, lately, this test's purpose was to make sure that we rejected the > OnCreateNewWindow from a non-current host. I think it can still basically do > that, if we add a FRIEND_TEST to be able to call the mojo fn directly? > > I'm also okay deleting the test. I'm pretty sure we don't have the underlying > bug anymore. FRIENDED and tested the mojo method directly. https://codereview.chromium.org/2821473002/diff/260001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2821473002/diff/260001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:2062: // use; they were allocated by the RenderWidgetHelper on the IO thread. On 2017/04/20 21:30:36, ncarter wrote: > The "on the IO thread" clause is now inaccurate. Done. https://codereview.chromium.org/2821473002/diff/260001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:2116: int opener_render_frame_id = opener->GetRoutingID(); On 2017/04/20 21:30:36, ncarter wrote: > Just inline this: there are multiple render_frame_id's in scope in this > function, and I think readability is maximized if we keeping the number of > aliases down. Done. https://codereview.chromium.org/2821473002/diff/260001/content/common/frame.m... File content/common/frame.mojom (right): https://codereview.chromium.org/2821473002/diff/260001/content/common/frame.m... content/common/frame.mojom:46: // process. On 2017/04/20 21:30:36, ncarter wrote: > Mention http://crbug.com/674307 Done. https://codereview.chromium.org/2821473002/diff/260001/content/common/frame.m... content/common/frame.mojom:54: // The session storage namespace ID this view should use. On 2017/04/20 18:58:41, nasko wrote: > nit: Let's move away from "view". Maybe "window", since this is what we are > creating and session storage is window/tab global. Done. https://codereview.chromium.org/2821473002/diff/260001/content/common/frame.m... content/common/frame.mojom:109: interface FrameHostIPC { On 2017/04/20 18:58:41, nasko wrote: > Why do we need a new interface here? It seems to me that FrameHost is the main > interface where RenderFrame and RenderFrameHost will be connected together, so > adding one just for the IPCs seems a bit strange. Also, as we move more to mojo, > I'm afraid that all legacy IPCs will move to FrameHostIPC and FrameHost will be > an empty interface as it is now. Ack, addressed earlier. https://codereview.chromium.org/2821473002/diff/260001/content/common/frame.m... content/common/frame.mojom:109: interface FrameHostIPC { On 2017/04/20 21:30:36, ncarter wrote: > On 2017/04/20 18:58:41, nasko wrote: > > Why do we need a new interface here? It seems to me that FrameHost is the main > > interface where RenderFrame and RenderFrameHost will be connected together, so > > adding one just for the IPCs seems a bit strange. Also, as we move more to > mojo, > > I'm afraid that all legacy IPCs will move to FrameHostIPC and FrameHost will > be > > an empty interface as it is now. > > I'm neutral on whether we just add methods to FrameHost, or have a FrameHostIPC > for messages that are handled directly by the RenderFrameHostImpl. > > If we're not sure, we can ask chromium-mojo for advice. > > If we do add the extra layer, I think 'FrameHostImpl' might be a reasonable > choice of name. Yeah I could go both ways, I'll post in chromium-mojo if rockot@ doesn't have a strong opinion. https://codereview.chromium.org/2821473002/diff/260001/content/common/frame.m... content/common/frame.mojom:111: // a tab for it. If |reply.route_id| is MSG_ROUTING_NONE, the view couldn't On 2017/04/20 18:58:41, nasko wrote: > nit: s/view/window/? Done. https://codereview.chromium.org/2821473002/diff/260001/content/test/test_rend... File content/test/test_render_frame.cc (right): https://codereview.chromium.org/2821473002/diff/260001/content/test/test_rend... content/test/test_render_frame.cc:40: mock_render_thread->OnCreateWindow(*params, reply->get()); On 2017/04/20 21:30:36, ncarter wrote: > Could we consider having MockRenderThread provide the FrameHost/FrameHostIPC > interfaces naturally, so that the > GetRemoteAssociatedInterfaces()->GetInterface() call in RenderFrameImpl just > works? My thinking is that it's best if the "mock browser process" behaviors are > implemented in terms of the IPC conversation, rather than overrides of real > code. > > Having said that, I'm okay with things as you've implemented them -- > particularly because I'm not sure if the above suggestion is actually practical. > > rockot@ may also have ideas here. I kept this here, but vended it via the AssociatedInterfaceProviderImpl (which required some tweaking). I saw that that was the way password autofilled did it. LMK what you think
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 csharrison@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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by csharrison@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...
csharrison@chromium.org changed reviewers: + rockot@chromium.org
Ken, I am promoting you to reviewer of this CL! The two things I want you to look at are: 1. My question about FrameHostIPC earlier in the comments (when I ccd you) 2. Changes to associated_interface_provider_impl.cc to support sync IPCs.
Re: FrameHost stuff. I agree that we should avoid using Channel-associated interfaces when not necessary. I suspect most frame-related IPC will for the foreseeable future need to retain relative ordering however, and so for the foreseeable future we'll want to use a Channel-associated interface. I do not think we should merge the existing (non-associated) FrameHost interface and whatever other thing(s) handle the more strictly ordered IPCs. Mostly because I want to make sure there is a clear path for IPCs to be migrated *without* strict relative ordering when we know it's not necessary. Adjectives like "ordererd", "unordered" or just an "IPC" suffix all seem kind of unfortunate to me. I'd be find if you wanted to name your new thing FrameHost and change the existing FrameHost to something like FrameInterfaceBroker. https://codereview.chromium.org/2821473002/diff/320001/content/common/associa... File content/common/associated_interface_provider_impl.cc (right): https://codereview.chromium.org/2821473002/diff/320001/content/common/associa... content/common/associated_interface_provider_impl.cc:85: local_provider_->WaitForBinding(name); Hmm. On one hand, I'm really not a fan of code which quietly runs nested message loops, especially when the normal non-test behavior is to be totally async. On the other hand, I can see the possibility of developers tripping over this subtle behavior when writing tests. Given that AssociatedInterfaceProvider is not something we intend to keep around for too long, and that its use (as well as sync message use in general) is relatively infrequent, I would prefer that your test code instead simply call FlushForTesting on the relevant AssociatedInterfacePtr. That does a ping-pong behind the scenes and waits for the reply in a nested loop. It should have an equivalent outcome. i.e. FooAssociatedPtr foo; provider->GetInterface(&foo); foo.FlushForTesting(); foo->PrettyCoolSyncIPC();
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)
Your points make sense to me, thanks! I'll send up a revised change with the suggestion. https://codereview.chromium.org/2821473002/diff/320001/content/common/associa... File content/common/associated_interface_provider_impl.cc (right): https://codereview.chromium.org/2821473002/diff/320001/content/common/associa... content/common/associated_interface_provider_impl.cc:85: local_provider_->WaitForBinding(name); On 2017/04/21 18:26:58, Ken Rockot wrote: > Hmm. On one hand, I'm really not a fan of code which quietly runs nested message > loops, especially when the normal non-test behavior is to be totally async. On > the other hand, I can see the possibility of developers tripping over this > subtle behavior when writing tests. > > Given that AssociatedInterfaceProvider is not something we intend to keep around > for too long, and that its use (as well as sync message use in general) is > relatively infrequent, I would prefer that your test code instead simply call > FlushForTesting on the relevant AssociatedInterfacePtr. That does a ping-pong > behind the scenes and waits for the reply in a nested loop. It should have an > equivalent outcome. i.e. > > FooAssociatedPtr foo; > provider->GetInterface(&foo); > foo.FlushForTesting(); > foo->PrettyCoolSyncIPC(); Hm, this makes sense but it's a bit tricky because the tests do not have a pointer to the AssociatedInterfacePtr. I think we'd just expose a getting RenderFrameImpl for the interface, then override it in TestRenderFrame with an extra call to FLushForTesting.
The CQ bit was checked by csharrison@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...
Ok the current PS adds a virtual RenderFrameImpl::GetFrameHost() which is overriden by TestRenderFrame whose job is just to Flush() it before forwarding it on. I've also done the rename as rockot suggests. ncarter,nasko ready for another round.
I'm happy with this; lgtm modulo the open "how to style the mojo magic" question, on which I defer to ken. https://codereview.chromium.org/2821473002/diff/260001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2821473002/diff/260001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:2495: if (extension && !extensions::BackgroundInfo::AllowJSAccess(extension)) On 2017/04/21 15:30:07, Charlie Harrison wrote: > On 2017/04/20 21:30:36, ncarter wrote: > > Hah. It would appear that an extension can sidestep this restriction (and get > JS > > access) by adding a same-origin, different-scheme iframe; via about:blank, > > blob:, or filesystem: ... > > > > -- really, we ought to use the same extension here as the one looked up in > > SecurityOriginHasExtensionBackgroundPermission. We can fix this now (which > adds > > risk to an already huge CL), or file a bug. > > Can you explain this a bit more? I'm happy to file a bug for this. I filed my thoughts as crbug.com/714251 https://codereview.chromium.org/2821473002/diff/260001/content/test/test_rend... File content/test/test_render_frame.cc (right): https://codereview.chromium.org/2821473002/diff/260001/content/test/test_rend... content/test/test_render_frame.cc:40: mock_render_thread->OnCreateWindow(*params, reply->get()); On 2017/04/21 15:30:08, Charlie Harrison wrote: > On 2017/04/20 21:30:36, ncarter wrote: > > Could we consider having MockRenderThread provide the FrameHost/FrameHostIPC > > interfaces naturally, so that the > > GetRemoteAssociatedInterfaces()->GetInterface() call in RenderFrameImpl just > > works? My thinking is that it's best if the "mock browser process" behaviors > are > > implemented in terms of the IPC conversation, rather than overrides of real > > code. > > > > Having said that, I'm okay with things as you've implemented them -- > > particularly because I'm not sure if the above suggestion is actually > practical. > > > > rockot@ may also have ideas here. > > I kept this here, but vended it via the AssociatedInterfaceProviderImpl (which > required some tweaking). I saw that that was the way password autofilled did it. > LMK what you think This looks like a better pattern. Thanks! https://codereview.chromium.org/2821473002/diff/320001/content/common/associa... File content/common/associated_interface_provider_impl.cc (right): https://codereview.chromium.org/2821473002/diff/320001/content/common/associa... content/common/associated_interface_provider_impl.cc:29: void WaitForBinding(const std::string& name) { Assuming this is test-only code, we should find a way to put it in a test-only file that's not linked against production content. https://codereview.chromium.org/2821473002/diff/320001/content/renderer/rende... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/2821473002/diff/320001/content/renderer/rende... content/renderer/render_view_impl.cc:1454: creator_frame->GetRemoteAssociatedInterfaces()->GetInterface(&frame_host_ptr); Is this one mojo IPC or two?
On 2017/04/21 at 20:42:13, nick wrote: > I'm happy with this; lgtm modulo the open "how to style the mojo magic" question, on which I defer to ken. > > https://codereview.chromium.org/2821473002/diff/260001/chrome/browser/chrome_... > File chrome/browser/chrome_content_browser_client.cc (right): > > https://codereview.chromium.org/2821473002/diff/260001/chrome/browser/chrome_... > chrome/browser/chrome_content_browser_client.cc:2495: if (extension && !extensions::BackgroundInfo::AllowJSAccess(extension)) > On 2017/04/21 15:30:07, Charlie Harrison wrote: > > On 2017/04/20 21:30:36, ncarter wrote: > > > Hah. It would appear that an extension can sidestep this restriction (and get > > JS > > > access) by adding a same-origin, different-scheme iframe; via about:blank, > > > blob:, or filesystem: ... > > > > > > -- really, we ought to use the same extension here as the one looked up in > > > SecurityOriginHasExtensionBackgroundPermission. We can fix this now (which > > adds > > > risk to an already huge CL), or file a bug. > > > > Can you explain this a bit more? I'm happy to file a bug for this. > > I filed my thoughts as crbug.com/714251 > > https://codereview.chromium.org/2821473002/diff/260001/content/test/test_rend... > File content/test/test_render_frame.cc (right): > > https://codereview.chromium.org/2821473002/diff/260001/content/test/test_rend... > content/test/test_render_frame.cc:40: mock_render_thread->OnCreateWindow(*params, reply->get()); > On 2017/04/21 15:30:08, Charlie Harrison wrote: > > On 2017/04/20 21:30:36, ncarter wrote: > > > Could we consider having MockRenderThread provide the FrameHost/FrameHostIPC > > > interfaces naturally, so that the > > > GetRemoteAssociatedInterfaces()->GetInterface() call in RenderFrameImpl just > > > works? My thinking is that it's best if the "mock browser process" behaviors > > are > > > implemented in terms of the IPC conversation, rather than overrides of real > > > code. > > > > > > Having said that, I'm okay with things as you've implemented them -- > > > particularly because I'm not sure if the above suggestion is actually > > practical. > > > > > > rockot@ may also have ideas here. > > > > I kept this here, but vended it via the AssociatedInterfaceProviderImpl (which > > required some tweaking). I saw that that was the way password autofilled did it. > > LMK what you think > > This looks like a better pattern. Thanks! > > https://codereview.chromium.org/2821473002/diff/320001/content/common/associa... > File content/common/associated_interface_provider_impl.cc (right): > > https://codereview.chromium.org/2821473002/diff/320001/content/common/associa... > content/common/associated_interface_provider_impl.cc:29: void WaitForBinding(const std::string& name) { > Assuming this is test-only code, we should find a way to put it in a test-only file that's not linked against production content. > > https://codereview.chromium.org/2821473002/diff/320001/content/renderer/rende... > File content/renderer/render_view_impl.cc (right): > > https://codereview.chromium.org/2821473002/diff/320001/content/renderer/rende... > content/renderer/render_view_impl.cc:1454: creator_frame->GetRemoteAssociatedInterfaces()->GetInterface(&frame_host_ptr); > Is this one mojo IPC or two? The first call will result in two pipelined messages since the remote interface provider pipe is itself established lazily.
https://codereview.chromium.org/2821473002/diff/320001/content/common/associa... File content/common/associated_interface_provider_impl.cc (right): https://codereview.chromium.org/2821473002/diff/320001/content/common/associa... content/common/associated_interface_provider_impl.cc:29: void WaitForBinding(const std::string& name) { On 2017/04/21 20:42:13, ncarter wrote: > Assuming this is test-only code, we should find a way to put it in a test-only > file that's not linked against production content. Sorry, this is no longer up to date since rockot@ gave me a better suggestion.
csharrison@chromium.org changed reviewers: + bauerb@chromium.org, sgurun@chromium.org, slan@chromium.org
bauerb: PTAL at single_tab_mode_tab_helper and blocked_window_params sgurun: TBR for trivial changes to aw_content_browser_client slan: TBR for trivial changes to cast_content_browser_client
Description was changed from ========== Service CreateNewWindow on the UI thread with a new mojo interface CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation BUG=35980 ========== to ========== Service CreateNewWindow on the UI thread with a new mojo interface CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation BUG=35980 TBR=slan@chromium.org,sgurun@chromium.org ==========
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2821473002/diff/340001/content/common/frame.m... File content/common/frame.mojom (right): https://codereview.chromium.org/2821473002/diff/340001/content/common/frame.m... content/common/frame.mojom:34: interface FrameInterfaceBroker { Oooh, I really like this solution. (Should this be FrameHostInterfaceBroker though? Are we going to eventually want to rename the existing "interface Frame" to "interface FrameInterfaceBroker" when we need a place to port individual browser->renderer FrameMsg_'s?)
FHIB seems reasonable to me. I was really just grasping for a naming scheme anyway :) On Fri, Apr 21, 2017 at 2:45 PM, <nick@chromium.org> wrote: > > https://codereview.chromium.org/2821473002/diff/340001/ > content/common/frame.mojom > File content/common/frame.mojom (right): > > https://codereview.chromium.org/2821473002/diff/340001/ > content/common/frame.mojom#newcode34 > content/common/frame.mojom:34: interface FrameInterfaceBroker { > Oooh, I really like this solution. > > (Should this be FrameHostInterfaceBroker though? Are we going to > eventually want to rename the existing "interface Frame" to "interface > FrameInterfaceBroker" when we need a place to port individual > browser->renderer FrameMsg_'s?) > > https://codereview.chromium.org/2821473002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
cast lgtm
The CQ bit was checked by csharrison@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...
Cool, I s/FrameInterfaceBroker/FrameHostInterfaceBroker/
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM with a suggestion: https://codereview.chromium.org/2821473002/diff/360001/chrome/browser/android... File chrome/browser/android/webapps/single_tab_mode_tab_helper.cc (right): https://codereview.chromium.org/2821473002/diff/360001/chrome/browser/android... chrome/browser/android/webapps/single_tab_mode_tab_helper.cc:22: void SingleTabModeTabHelper::HandleOpenUrl(const BlockedWindowParams& params) { It looks like that was already the case, but this method doesn't really do anything with its state. And given that there is only a single call site, you could just inline it there.
The CQ bit was checked by csharrison@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/2821473002/diff/360001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2821473002/diff/360001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2450: frame_tree_node_->current_frame_host() == this && render_frame_created_ && Out of curiosity, why didn't the original need to check render_frame_created_? Or did it check this and I'm missing it somewhere? I kind of feel like this check should be at a more generic level; if render_frame_created_ is false, it seems like we should be disallowing binding to the interface altogether. And if we gate it on interface binding, then I /think/ that individual method implementations shouldn't need to check for this? https://codereview.chromium.org/2821473002/diff/360001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2473: scoped_refptr<SessionStorageNamespaceImpl> cloned_namespace = If you're feeling adventurous, one new possibility is: auto cloned_namespace = base::MakeShared<SessionStorageNamespaceImpl>( ...); https://codereview.chromium.org/2821473002/diff/360001/content/common/frame.m... File content/common/frame.mojom (right): https://codereview.chromium.org/2821473002/diff/360001/content/common/frame.m... content/common/frame.mojom:111: // a tab for it. If |reply.route_id| is MSG_ROUTING_NONE, the window couldn't It's not necessary to change in this CL, but I kind of wonder why we don't just have an optional return value instead of having the caller check for a magic sentinel value.
Thanks! Nasko: WDYT? https://codereview.chromium.org/2821473002/diff/360001/chrome/browser/android... File chrome/browser/android/webapps/single_tab_mode_tab_helper.cc (right): https://codereview.chromium.org/2821473002/diff/360001/chrome/browser/android... chrome/browser/android/webapps/single_tab_mode_tab_helper.cc:22: void SingleTabModeTabHelper::HandleOpenUrl(const BlockedWindowParams& params) { On 2017/04/24 13:05:30, Bernhard Bauer wrote: > It looks like that was already the case, but this method doesn't really do > anything with its state. And given that there is only a single call site, you > could just inline it there. Done, mostly because this removes the web_contents_ member on this class, which is now just a WebContents-scoped bool :)
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
https://codereview.chromium.org/2821473002/diff/360001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2821473002/diff/360001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2450: frame_tree_node_->current_frame_host() == this && render_frame_created_ && On 2017/04/24 14:00:39, dcheng (OOO through May 2) wrote: > Out of curiosity, why didn't the original need to check render_frame_created_? > Or did it check this and I'm missing it somewhere? > > I kind of feel like this check should be at a more generic level; if > render_frame_created_ is false, it seems like we should be disallowing binding > to the interface altogether. And if we gate it on interface binding, then I > /think/ that individual method implementations shouldn't need to check for this? This sounds like a good solution, and it looks pretty similar to what we have in OnMessageReceived today. Do you mind if we wait to do this in a followup since this CL is mostly settled? It should be relatively simple I'm just concerned because we'd be affecting multiple IPCs rather than just this one. We used to have this check in RenderWidgetHelper::OnCreateNewWindowOnUI via IsRenderFrameLive. https://codereview.chromium.org/2821473002/diff/360001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2473: scoped_refptr<SessionStorageNamespaceImpl> cloned_namespace = On 2017/04/24 14:00:40, dcheng (OOO through May 2) wrote: > If you're feeling adventurous, one new possibility is: > > auto cloned_namespace = base::MakeShared<SessionStorageNamespaceImpl>( > ...); Hey! I didn't know we had that! Did I miss a chromium-dev PSA? Done.
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/2821473002/diff/360001/chrome/browser/android... File chrome/browser/android/webapps/single_tab_mode_tab_helper.cc (right): https://codereview.chromium.org/2821473002/diff/360001/chrome/browser/android... chrome/browser/android/webapps/single_tab_mode_tab_helper.cc:22: void SingleTabModeTabHelper::HandleOpenUrl(const BlockedWindowParams& params) { On 2017/04/24 14:04:03, Charlie Harrison wrote: > On 2017/04/24 13:05:30, Bernhard Bauer wrote: > > It looks like that was already the case, but this method doesn't really do > > anything with its state. And given that there is only a single call site, you > > could just inline it there. > > Done, mostly because this removes the web_contents_ member on this class, which > is now just a WebContents-scoped bool :) Exactly :) Sadly, it doesn't look like base::SupportsUserData lets you attach a single bool ;-)
lgtm https://codereview.chromium.org/2821473002/diff/360001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2821473002/diff/360001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2450: frame_tree_node_->current_frame_host() == this && render_frame_created_ && On 2017/04/24 14:26:37, Charlie Harrison wrote: > On 2017/04/24 14:00:39, dcheng (OOO through May 2) wrote: > > Out of curiosity, why didn't the original need to check render_frame_created_? > > Or did it check this and I'm missing it somewhere? > > > > I kind of feel like this check should be at a more generic level; if > > render_frame_created_ is false, it seems like we should be disallowing binding > > to the interface altogether. And if we gate it on interface binding, then I > > /think/ that individual method implementations shouldn't need to check for > this? > > This sounds like a good solution, and it looks pretty similar to what we have in > OnMessageReceived today. Do you mind if we wait to do this in a followup since > this CL is mostly settled? It should be relatively simple I'm just concerned > because we'd be affecting multiple IPCs rather than just this one. > > We used to have this check in RenderWidgetHelper::OnCreateNewWindowOnUI via > IsRenderFrameLive. Acknowledged. https://codereview.chromium.org/2821473002/diff/360001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2473: scoped_refptr<SessionStorageNamespaceImpl> cloned_namespace = On 2017/04/24 14:26:37, Charlie Harrison wrote: > On 2017/04/24 14:00:40, dcheng (OOO through May 2) wrote: > > If you're feeling adventurous, one new possibility is: > > > > auto cloned_namespace = base::MakeShared<SessionStorageNamespaceImpl>( > > ...); > > Hey! I didn't know we had that! Did I miss a chromium-dev PSA? > > Done. It's relatively new and there's been no PSA. I think I want to see how people like it a bit more before we publicly announce it, but that's just my opinion =)
Thanks Daniel! Nasko I know you have opinions about this CL so I'll still wait for your go ahead before landing. https://codereview.chromium.org/2821473002/diff/360001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2821473002/diff/360001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:2473: scoped_refptr<SessionStorageNamespaceImpl> cloned_namespace = On 2017/04/24 14:43:44, dcheng (OOO through May 2) wrote: > On 2017/04/24 14:26:37, Charlie Harrison wrote: > > On 2017/04/24 14:00:40, dcheng (OOO through May 2) wrote: > > > If you're feeling adventurous, one new possibility is: > > > > > > auto cloned_namespace = base::MakeShared<SessionStorageNamespaceImpl>( > > > ...); > > > > Hey! I didn't know we had that! Did I miss a chromium-dev PSA? > > > > Done. > > It's relatively new and there's been no PSA. I think I want to see how people > like it a bit more before we publicly announce it, but that's just my opinion =) base/style owners have 100% successfully conditioned me to recoil in horror at bare "new", so this will make working with ref_ptrs much much better imo.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks a bunch for doing this conversion! LGTM
Thank you everyone for the help with this CL, especially since it touches code I am not super familiar with. Let's see what the CQ has to say.
Description was changed from ========== Service CreateNewWindow on the UI thread with a new mojo interface CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation BUG=35980 TBR=slan@chromium.org,sgurun@chromium.org ========== to ========== Service CreateNewWindow on the UI thread with a new mojo interface This patch replaces the IO-thread handler for CreateNewWindow with a UI-thread version, using a new mojo interface called FrameHost. The existing FrameHost interface is renamed to FrameHostInterfaceBroker. This is an associated interface, so messages will remain ordered with respect to the traditional IPC pipe. There are two a few changes that happened as a result of this approach: 1. ContentBrowserClient::CanCreateWindow is now on the UI thread. This means policy makers need some UI thread context. Since the UI thread is usually better at making policy decisions in general, most components were simplified here. This was the main impetus of this change, because we wanted an easy place to put window opening policy decisions. 2. Since the mojo interface is now frame-based, all window opening logic moved to RenderFrameHostImpl. 3. window.open() is a sync IPC. This change makes the sync wait longer because we create the UI thread objects before replying. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation BUG=35980 TBR=slan@chromium.org,sgurun@chromium.org ==========
Description was changed from ========== Service CreateNewWindow on the UI thread with a new mojo interface This patch replaces the IO-thread handler for CreateNewWindow with a UI-thread version, using a new mojo interface called FrameHost. The existing FrameHost interface is renamed to FrameHostInterfaceBroker. This is an associated interface, so messages will remain ordered with respect to the traditional IPC pipe. There are two a few changes that happened as a result of this approach: 1. ContentBrowserClient::CanCreateWindow is now on the UI thread. This means policy makers need some UI thread context. Since the UI thread is usually better at making policy decisions in general, most components were simplified here. This was the main impetus of this change, because we wanted an easy place to put window opening policy decisions. 2. Since the mojo interface is now frame-based, all window opening logic moved to RenderFrameHostImpl. 3. window.open() is a sync IPC. This change makes the sync wait longer because we create the UI thread objects before replying. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation BUG=35980 TBR=slan@chromium.org,sgurun@chromium.org ========== to ========== Service CreateNewWindow on the UI thread with a new mojo interface This patch replaces the IO-thread handler for CreateNewWindow with a UI-thread version, using a new mojo interface called FrameHost. The existing FrameHost interface is renamed to FrameHostInterfaceBroker. This is an associated interface, so messages will remain ordered with respect to the traditional IPC pipe. There are a few changes that happened as a result of this approach: 1. ContentBrowserClient::CanCreateWindow is now on the UI thread. This means policy makers need some UI thread context. Since the UI thread is usually better at making policy decisions in general, most components were simplified here. This was the main impetus of this change, because we wanted an easy place to put window opening policy decisions. 2. Since the mojo interface is now frame-based, all window opening logic moved to RenderFrameHostImpl. 3. window.open() is a sync IPC. This change makes the sync wait longer because we create the UI thread objects before replying. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation BUG=35980 TBR=slan@chromium.org,sgurun@chromium.org ==========
The CQ bit was checked by csharrison@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdevlin.cronin@chromium.org, alexmos@chromium.org, nick@chromium.org, sgurun@chromium.org, slan@chromium.org, bauerb@chromium.org Link to the patchset: https://codereview.chromium.org/2821473002/#ps400001 (title: "MakeShared goodness")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 400001, "attempt_start_ts": 1493059408272280,
"parent_rev": "8a9eb48c6d24472e6812811ece2bf4a29d2c3747", "commit_rev":
"95f01e92e7147bee4e66ce629712fd1f50808936"}
Message was sent while issue was closed.
Description was changed from ========== Service CreateNewWindow on the UI thread with a new mojo interface This patch replaces the IO-thread handler for CreateNewWindow with a UI-thread version, using a new mojo interface called FrameHost. The existing FrameHost interface is renamed to FrameHostInterfaceBroker. This is an associated interface, so messages will remain ordered with respect to the traditional IPC pipe. There are a few changes that happened as a result of this approach: 1. ContentBrowserClient::CanCreateWindow is now on the UI thread. This means policy makers need some UI thread context. Since the UI thread is usually better at making policy decisions in general, most components were simplified here. This was the main impetus of this change, because we wanted an easy place to put window opening policy decisions. 2. Since the mojo interface is now frame-based, all window opening logic moved to RenderFrameHostImpl. 3. window.open() is a sync IPC. This change makes the sync wait longer because we create the UI thread objects before replying. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation BUG=35980 TBR=slan@chromium.org,sgurun@chromium.org ========== to ========== Service CreateNewWindow on the UI thread with a new mojo interface This patch replaces the IO-thread handler for CreateNewWindow with a UI-thread version, using a new mojo interface called FrameHost. The existing FrameHost interface is renamed to FrameHostInterfaceBroker. This is an associated interface, so messages will remain ordered with respect to the traditional IPC pipe. There are a few changes that happened as a result of this approach: 1. ContentBrowserClient::CanCreateWindow is now on the UI thread. This means policy makers need some UI thread context. Since the UI thread is usually better at making policy decisions in general, most components were simplified here. This was the main impetus of this change, because we wanted an easy place to put window opening policy decisions. 2. Since the mojo interface is now frame-based, all window opening logic moved to RenderFrameHostImpl. 3. window.open() is a sync IPC. This change makes the sync wait longer because we create the UI thread objects before replying. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation BUG=35980 TBR=slan@chromium.org,sgurun@chromium.org Review-Url: https://codereview.chromium.org/2821473002 Cr-Commit-Position: refs/heads/master@{#466700} Committed: https://chromium.googlesource.com/chromium/src/+/95f01e92e7147bee4e66ce629712... ==========
Message was sent while issue was closed.
Committed patchset #21 (id:400001) as https://chromium.googlesource.com/chromium/src/+/95f01e92e7147bee4e66ce629712...
Message was sent while issue was closed.
On 2017/04/24 18:53:01, commit-bot: I haz the power wrote: > Committed patchset #21 (id:400001) as > https://chromium.googlesource.com/chromium/src/+/95f01e92e7147bee4e66ce629712... According to an analysis by Findit https://findit-for-me.appspot.com/waterfall/flake?key=ag9zfmZpbmRpdC1mb3ItbWV..., Findit suggests this CL was responsible for introducing flakiness in SBNavigationObserverBrowserTest.DirectDownloadNoReferrerTargetBlank on browser_side_navigation_browser_tests on Mac-10.11. Can someone please help verify? Thanks, Jeff on behalf of Findit team
Message was sent while issue was closed.
On 2017/04/25 09:21:46, lijeffrey wrote: > On 2017/04/24 18:53:01, commit-bot: I haz the power wrote: > > Committed patchset #21 (id:400001) as > > > https://chromium.googlesource.com/chromium/src/+/95f01e92e7147bee4e66ce629712... > > According to an analysis by Findit > https://findit-for-me.appspot.com/waterfall/flake?key=ag9zfmZpbmRpdC1mb3ItbWV..., > Findit suggests this CL was responsible for introducing flakiness in > SBNavigationObserverBrowserTest.DirectDownloadNoReferrerTargetBlank on > browser_side_navigation_browser_tests on Mac-10.11. Can someone please help > verify? > > Thanks, > Jeff on behalf of Findit team I filed crbug.com/715076 to investigate.
Message was sent while issue was closed.
https://codereview.chromium.org/2821473002/diff/400001/content/test/test_rend... File content/test/test_render_frame.cc (right): https://codereview.chromium.org/2821473002/diff/400001/content/test/test_rend... content/test/test_render_frame.cc:129: ptr.FlushForTesting(); Can you please remind me exactly why this flush was necessary? i.e. how the sync CreateNewWindow IPC be sent at a time where it would cause the main thread to deadlock? And how wasn't this a problem before the IPC was converted to mojom, given that the ordering and threading behavior should be exactly the same as before? Any specific tests that were failing? I'm asking because this turns out to be quite problematic: we effectively can't use the FrameHost interface for all kinds of things now, because calling GetFrameHost() from various points in production code will deadlock browser tests on this FlushForTesting().
Message was sent while issue was closed.
https://codereview.chromium.org/2821473002/diff/400001/content/test/test_rend... File content/test/test_render_frame.cc (right): https://codereview.chromium.org/2821473002/diff/400001/content/test/test_rend... content/test/test_render_frame.cc:129: ptr.FlushForTesting(); On 2017/05/31 23:58:02, Ken Rockot(use gerrit already) wrote: > Can you please remind me exactly why this flush was necessary? i.e. how the sync > CreateNewWindow IPC be sent at a time where it would cause the main thread to > deadlock? And how wasn't this a problem before the IPC was converted to mojom, > given that the ordering and threading behavior should be exactly the same as > before? Any specific tests that were failing? > > I'm asking because this turns out to be quite problematic: we effectively can't > use the FrameHost interface for all kinds of things now, because calling > GetFrameHost() from various points in production code will deadlock browser > tests on this FlushForTesting(). Hey Ken, agreed this is not ideal especially if it is making testing harder in general. I've filed crbug.com/728463 to track this with some extra context for how we got here. Thanks for bringing this up
Message was sent while issue was closed.
Thanks, that all makes sense. I'm going to make the following changes in a CL I have which needs this to be fixed: - Change the signature of GetFrameHost to return a mojom::FrameHost* - Cache a FrameHostAssociatedPtr in RFI, initialized lazily and returned by GetFrameHost - Have TestRenderFrame return its mock_frame_host_ directly instead of using an associated interface + override to get to it I see no downside to avoiding the extra complexity of using Mojo when we could just return a direct pointer to the mock impl. On Wed, May 31, 2017 at 6:35 PM, <csharrison@chromium.org> wrote: > > https://codereview.chromium.org/2821473002/diff/400001/conte > nt/test/test_render_frame.cc > File content/test/test_render_frame.cc (right): > > https://codereview.chromium.org/2821473002/diff/400001/conte > nt/test/test_render_frame.cc#newcode129 > content/test/test_render_frame.cc:129: ptr.FlushForTesting(); > On 2017/05/31 23:58:02, Ken Rockot(use gerrit already) wrote: > > Can you please remind me exactly why this flush was necessary? i.e. > how the sync > > CreateNewWindow IPC be sent at a time where it would cause the main > thread to > > deadlock? And how wasn't this a problem before the IPC was converted > to mojom, > > given that the ordering and threading behavior should be exactly the > same as > > before? Any specific tests that were failing? > > > > I'm asking because this turns out to be quite problematic: we > effectively can't > > use the FrameHost interface for all kinds of things now, because > calling > > GetFrameHost() from various points in production code will deadlock > browser > > tests on this FlushForTesting(). > > Hey Ken, agreed this is not ideal especially if it is making testing > harder in general. I've filed crbug.com/728463 to track this with some > extra context for how we got here. > > Thanks for bringing this up > > https://codereview.chromium.org/2821473002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
On 2017/06/01 05:43:08, Ken Rockot(use gerrit already) wrote: > Thanks, that all makes sense. I'm going to make the following changes in a > CL I have which needs this to be fixed: > > - Change the signature of GetFrameHost to return a mojom::FrameHost* > - Cache a FrameHostAssociatedPtr in RFI, initialized lazily and returned by > GetFrameHost > - Have TestRenderFrame return its mock_frame_host_ directly instead of > using an associated interface + override to get to it > > I see no downside to avoiding the extra complexity of using Mojo when we > could just return a direct pointer to the mock impl. SGTM. The downside there is that the GetFrameHost method is still needed when ideally we could just OverrideBinderForTesting once and use GetRemoteAssociatedInterfaces()->GetInterface directly. Still, I agree it's better than what we have today, and I'm fine with your proposal. > > On Wed, May 31, 2017 at 6:35 PM, <mailto:csharrison@chromium.org> wrote: > > > > > https://codereview.chromium.org/2821473002/diff/400001/conte > > nt/test/test_render_frame.cc > > File content/test/test_render_frame.cc (right): > > > > https://codereview.chromium.org/2821473002/diff/400001/conte > > nt/test/test_render_frame.cc#newcode129 > > content/test/test_render_frame.cc:129: ptr.FlushForTesting(); > > On 2017/05/31 23:58:02, Ken Rockot(use gerrit already) wrote: > > > Can you please remind me exactly why this flush was necessary? i.e. > > how the sync > > > CreateNewWindow IPC be sent at a time where it would cause the main > > thread to > > > deadlock? And how wasn't this a problem before the IPC was converted > > to mojom, > > > given that the ordering and threading behavior should be exactly the > > same as > > > before? Any specific tests that were failing? > > > > > > I'm asking because this turns out to be quite problematic: we > > effectively can't > > > use the FrameHost interface for all kinds of things now, because > > calling > > > GetFrameHost() from various points in production code will deadlock > > browser > > > tests on this FlushForTesting(). > > > > Hey Ken, agreed this is not ideal especially if it is making testing > > harder in general. I've filed crbug.com/728463 to track this with some > > extra context for how we got here. > > > > Thanks for bringing this up > > > > https://codereview.chromium.org/2821473002/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. |
