Fix Fullscreen Flash for out-of-process iframes.

Fix Fullscreen Flash for out-of-process iframes.

This CL:
- fixes routing ID mismatch when sending ViewHostMsg_ShowFullscreenWidget
- fixes main frame URL access in CreatePepperFullscreenContainer
- allows swapped-out RVH to process OnShowFullscreenWidget
- fixes WebContentsImpl to track the fullscreen RenderWidgetHostView
using a {process_id, routing_id} pair, rather than just the widget
routing_id.

See https://crbug.com/593522#c3 for a slightly more detailed summary.

BUG=593522
Committed: https://crrev.com/c9e76ecdcb771c7fcd975afeaeb2516af6220ab9
Cr-Commit-Position: refs/heads/master@{#393965}

[alexmos, 2016-05-13 17:44:45]

Description was changed from

==========
Fix Fullscreen Flash for out-of-process iframes.

BUG=593522
==========

to

==========
Fix Fullscreen Flash for out-of-process iframes.

This CL:
- fixes routing ID mismatch when sending ViewHostMsg_ShowFullscreenWidget
- fixes main frame URL access in CreatePepperFullscreenContainer
- allows swapped-out RVH to process OnShowFullscreenWidget
- fixes WebContentsImpl to track the fullscreen RenderWidgetHostView
using a {process_id, routing_id} pair, rather than just the widget
routing_id.

See https://crbug.com/593522#c3 for a slightly more detailed summary.

BUG=593522
==========

[alexmos, 2016-05-13 17:47:45]

alexmos@chromium.org changed reviewers:
+ nick@chromium.org

[alexmos, 2016-05-13 17:47:46]

Nick, can you please take a look?

https://codereview.chromium.org/1973813002/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1973813002/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:1684:
fullscreen_widget_routing_id_));
It looks like all present consumers of DidShowFullscreenWidget and
DidDestroyFullscreenWidget just ignore the widget routing ID, so I'm going to
try to remove that parameter completely (provided miu@ or someone who knows why
the routing IDs were there to begin with is ok with this).  That's pretty
independent though, so I put that up as a separate CL:
https://codereview.chromium.org/1980633002

https://codereview.chromium.org/1973813002/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1973813002/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:1222: active_url =
render_view()->GetURLForGraphicsContext3D();
There's a comment on active_url_ that it gets passed to ggl::CreateViewContext.
I have no idea how this URL gets used by that, but it looks like we've already
dealt with the remote main frame case in GetURLForGraphicsContext3D for other
cases.

https://codereview.chromium.org/1973813002/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:1224: render_view()->routing_id(),
GetRenderWidget()->compositor_deps(),
This routing ID ends up in RenderWidget::opener_id_, which is a RenderView
routing ID. I've been wanting to move that (along with ViewHostMsg_CreateWidget
and friends) to frame routing IDs for some time now, but I think that can be
independent of this CL.

This is also what we do for popup menus:
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...

[alexmos, 2016-05-13 19:16:33]

Also, no test for this yet, but I've got a lead on that -- I'll see if I can
modify  flash_fullscreen_interactive_browsertest.cc and
ppapi/tests/test_case.html to do what I want.

[ncarter (slow), 2016-05-13 21:40:38]

nick@chromium.org changed reviewers:
+ piman@chromium.org

[ncarter (slow), 2016-05-13 21:40:39]

+piman for a second set of eyes

https://codereview.chromium.org/1973813002/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1973813002/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:1684:
fullscreen_widget_routing_id_));
On 2016/05/13 17:47:45, alexmos wrote:
> It looks like all present consumers of DidShowFullscreenWidget and
> DidDestroyFullscreenWidget just ignore the widget routing ID, so I'm going to
> try to remove that parameter completely (provided miu@ or someone who knows
why
> the routing IDs were there to begin with is ok with this).  That's pretty
> independent though, so I put that up as a separate CL:
> https://codereview.chromium.org/1980633002

Sounds good.

https://codereview.chromium.org/1973813002/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:2159: void
WebContentsImpl::ShowCreatedWidget(int route_id,
Does this function need more work, or is it simply not possible to get here for
anything but the main frame's process?

https://codereview.chromium.org/1973813002/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1973813002/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:1222: active_url =
render_view()->GetURLForGraphicsContext3D();
On 2016/05/13 17:47:45, alexmos wrote:
> There's a comment on active_url_ that it gets passed to
ggl::CreateViewContext.
> I have no idea how this URL gets used by that, but it looks like we've already
> dealt with the remote main frame case in GetURLForGraphicsContext3D for other
> cases.

+piman for any light that can be shed on the importance of this URL value

https://codereview.chromium.org/1973813002/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:1224: render_view()->routing_id(),
GetRenderWidget()->compositor_deps(),
On 2016/05/13 17:47:45, alexmos wrote:
> This routing ID ends up in RenderWidget::opener_id_, which is a RenderView
> routing ID. I've been wanting to move that (along with
ViewHostMsg_CreateWidget
> and friends) to frame routing IDs for some time now, but I think that can be
> independent of this CL.
> 
> This is also what we do for popup menus:
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...

Acknowledged.

[ncarter (slow), 2016-05-13 21:45:22]

oh and lgtm

Tests would be nice for these behaviors, even if it's just a matter of modifying
existing tests. Do you have a roadmap for that?

[miu, 2016-05-14 01:25:25]

miu@chromium.org changed reviewers:
+ miu@chromium.org

[miu, 2016-05-14 01:25:26]

lgtm too.

FYI--Example of some tests I wrote to test fullscreen Flash behavior in the
past:
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/tes...

These use this "simulated flash plugin":
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/tests/test_f...

(The tests invoke tab capture on the fullscreen widget to read color changes.)

So, perhaps there's some way to create an interactive_browser_test to create the
"simulated flash plugin" in an OOP iframe?

[piman, 2016-05-16 17:48:31]

lgtm

lgtm

[alexmos, 2016-05-16 19:54:14]

Thanks for reviewing!

I've poked a bit more at a test for this (thanks miu@ for the references!).  I
got a variant of 
FlashFullscreenInteractiveBrowserTest.FullscreenWithinTab_EscapeKeyExitsFullscreen
to work in a subframe with --site-per-process, but this involves several other
fixes, other than writing the actual test: 
- navigator.plugins and navigator.mimeTypes don't work for OOPIF (filed
https://crbug.com/612200)
- Simulated plugin events (PepperPluginInstanceImpl::SimulateInputEvent) need to
be fixed for OOPIFs
- adding plumbing and fixing a few assumptions in ppapi_test.cc to allow running
in a subframe.

So, to not randomize this CL, I'm planning to work on these separately and then
land the test in a followup CL.  Let me know if this is ok.

https://codereview.chromium.org/1973813002/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1973813002/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:2159: void
WebContentsImpl::ShowCreatedWidget(int route_id,
On 2016/05/13 21:40:39, ncarter wrote:
> Does this function need more work, or is it simply not possible to get here
for
> anything but the main frame's process?

Oh, this brings up a very interesting point.  |route_id| is used to look up a
pending widget (created but not shown) in pending_widget_views_ (which had been
populated by WebContentsImpl::CreateNewWidget).  This happens in the context of
the opener widget, which might be different from the main frame process.  With
OOPIFs, this can create a conflict if two subframes in different processes try
to simultaneously create two widgets, and the widgets get the same routing IDs,
clashing in pending_widget_views_, which is tracked per-WebContents.  It seems
like pending_widget_views_ should be keyed by a (process_id, route_id) pair to
avoid this race.

If it's ok, I'll work on this in a followup CL.  The risk for fullscreen widgets
in particular is low, since they require a user gesture to activate, so it's
highly improbable that we'll have two pending widgets.  But I think the race
could happen more generally with ordinary widgets as well, so it's worth fixing.

[miu, 2016-05-16 20:08:00]

On 2016/05/16 19:54:14, alexmos wrote:
> Thanks for reviewing!
> 
> I've poked a bit more at a test for this (thanks miu@ for the references!).  I
> ...
> 
> So, to not randomize this CL, I'm planning to work on these separately and
then
> land the test in a followup CL.  Let me know if this is ok.

Sounds good. Glad the tests helped reveal other issues! :)

[alexmos, 2016-05-16 21:56:29]

https://codereview.chromium.org/1973813002/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1973813002/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:2159: void
WebContentsImpl::ShowCreatedWidget(int route_id,
On 2016/05/16 19:54:14, alexmos wrote:
> On 2016/05/13 21:40:39, ncarter wrote:
> > Does this function need more work, or is it simply not possible to get here
> for
> > anything but the main frame's process?
> 
> Oh, this brings up a very interesting point.  |route_id| is used to look up a
> pending widget (created but not shown) in pending_widget_views_ (which had
been
> populated by WebContentsImpl::CreateNewWidget).  This happens in the context
of
> the opener widget, which might be different from the main frame process.  With
> OOPIFs, this can create a conflict if two subframes in different processes try
> to simultaneously create two widgets, and the widgets get the same routing
IDs,
> clashing in pending_widget_views_, which is tracked per-WebContents.  It seems
> like pending_widget_views_ should be keyed by a (process_id, route_id) pair to
> avoid this race.
> 
> If it's ok, I'll work on this in a followup CL.  The risk for fullscreen
widgets
> in particular is low, since they require a user gesture to activate, so it's
> highly improbable that we'll have two pending widgets.  But I think the race
> could happen more generally with ordinary widgets as well, so it's worth
fixing.

And turns out this also affects pending WebContents (for window.open). Filed
https://crbug.com/612276.

[alexmos, 2016-05-16 21:56:58]

The CQ bit was checked by alexmos@chromium.org

[alexmos, 2016-05-16 21:56:58]

The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org,
miu@chromium.org
Link to the patchset: https://codereview.chromium.org/1973813002/#ps20001
(title: "Rebase")

[commit-bot: I haz the power, 2016-05-16 21:57:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1973813002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1973813002/20001

[commit-bot: I haz the power, 2016-05-16 22:59:49]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-05-16 23:01:19]

Description was changed from

==========
Fix Fullscreen Flash for out-of-process iframes.

This CL:
- fixes routing ID mismatch when sending ViewHostMsg_ShowFullscreenWidget
- fixes main frame URL access in CreatePepperFullscreenContainer
- allows swapped-out RVH to process OnShowFullscreenWidget
- fixes WebContentsImpl to track the fullscreen RenderWidgetHostView
using a {process_id, routing_id} pair, rather than just the widget
routing_id.

See https://crbug.com/593522#c3 for a slightly more detailed summary.

BUG=593522
==========

to

==========
Fix Fullscreen Flash for out-of-process iframes.

This CL:
- fixes routing ID mismatch when sending ViewHostMsg_ShowFullscreenWidget
- fixes main frame URL access in CreatePepperFullscreenContainer
- allows swapped-out RVH to process OnShowFullscreenWidget
- fixes WebContentsImpl to track the fullscreen RenderWidgetHostView
using a {process_id, routing_id} pair, rather than just the widget
routing_id.

See https://crbug.com/593522#c3 for a slightly more detailed summary.

BUG=593522

Committed: https://crrev.com/c9e76ecdcb771c7fcd975afeaeb2516af6220ab9
Cr-Commit-Position: refs/heads/master@{#393965}
==========

[commit-bot: I haz the power, 2016-05-16 23:01:21]

Patchset 2 (id:??) landed as
https://crrev.com/c9e76ecdcb771c7fcd975afeaeb2516af6220ab9
Cr-Commit-Position: refs/heads/master@{#393965}