New issue
Advanced search Search tips

Issue 800407 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jan 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 2
Type: Bug



Sign in to add a comment

extensionview cannot do cross extension navigation with OOPIF based guests

Project Member Reported by mcnee@chromium.org, Jan 9 2018

Issue description

Chrome Version: 65.0.3316.0

As seen in the currently disabled test ExtensionViewTest.LoadAPICall, the attempt to navigate an extensionview from one extension to another does not work with OOPIF based guests. Note that the test passes for non-OOPIF based guests.

Note that extensionview is an internal API and the only thing that appears to use it at the moment is the WebUI for chrome://cast which does not navigate between extensions. So this does not appear to be an issue at the moment.
 

Comment 1 by mcnee@chromium.org, Jan 9 2018

From an initial investigation, the attach is failing when navigating to the new extension.

In GuestViewInternalCustomBindings::AttachIframeGuest, we use the iframe's contentWindow to get the corresponding RenderFrame. While we do have a contentWindow, the lookup of the corresponding RenderFrame fails. Specifically, the CreationContext of the contentWindow is empty.

Comment 2 by mcnee@chromium.org, Jan 9 2018

Cc: ekaramad@chromium.org
Project Member

Comment 3 by bugdroid1@chromium.org, Jan 10 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e813105cc50038870c18ccb7e6525a6106e1928b

commit e813105cc50038870c18ccb7e6525a6106e1928b
Author: Kevin McNee <mcnee@chromium.org>
Date: Wed Jan 10 16:00:16 2018

Rewrite extension_view/load_api tests to not have large multi-step tests

These tests have been disabled since 2015. There were only two tests,
each with multiple steps. So due to a failure of one part of each test,
we lost all coverage.

We refactor these tests so that they can be run individually. The parts
of the tests that were failing before are disabled individually, namely
the tests for navigating to a new extension.

Bug:  800407 ,  545656 ,  512092 ,  538114 
Change-Id: I8b30fa861bd70174fc78117b3affebaf12c623e1
Reviewed-on: https://chromium-review.googlesource.com/857594
Reviewed-by: James MacLean <wjmaclean@chromium.org>
Commit-Queue: Kevin McNee <mcnee@chromium.org>
Cr-Commit-Position: refs/heads/master@{#528327}
[modify] https://crrev.com/e813105cc50038870c18ccb7e6525a6106e1928b/chrome/browser/apps/guest_view/extension_view/extension_view_browsertest.cc
[modify] https://crrev.com/e813105cc50038870c18ccb7e6525a6106e1928b/chrome/test/data/extensions/platform_apps/extension_view/load_api/main.js

Comment 4 by mcnee@chromium.org, Jan 10 2018

Labels: -Pri-3 Pri-2
I suspect that this is not extensionview specific. I modified AppViewTest.TestAppViewMultipleConnects so that <appview>.connect calls wait until the previous connect completes and the same issue occurred where the render frame was null for the second attach.

Like the extensionview cross extension navigation case, <appview>.connect destroys and recreates the guest.

I also notice that when we access the iframe's contentWindow for the first attach, the |content_frame_| is local, whereas on the second attach, the |content_frame_| is remote. Perhaps there's some cleanup during detachment that's not happening in the OOPIF based guest case.
Interesting findings. The RenderFrame we are trying to get is the one used for guest view, right? So if the frame is remote for the second "guest" navigation, it is quite natural to get nullptr for RenderFrame given that none exists in the same process (embedder process). Perhaps we need to detach and reattach the <iframe> before navigating to the new guest?

Comment 6 by mcnee@chromium.org, Jan 10 2018

Right, in the guest_view_iframe.js implementation of |attachImpl$|, the contentWindow of the internal iframe element is used to identify the RenderFrame of the iframe element, which determines the |embedder_local_frame_routing_id| sent in |GuestViewHostMsg_AttachToEmbedderFrame|. So it certainly looks like the renderer only knows how to replace a local frame.

So when the guest is detached, we should presumably have local frame swapped back for the iframe. I'm not entirely sure how detachment works in OOPIF based guests. In |destroyImpl|, we call |DetachGuest| but that appears to only actually do something in the BrowserPlugin case.


Another observation: In the callback in |attachImpl$|, we access the contentWindow again now that it's attached and it's still local. That doesn't seem right. I played around with an OOPIF and tried getting a reference to a local frame's contentWindow, navigating cross site, and then using the reference to access the document and it was correctly denied. So I guess contentWindow references can go from local to remote, and I assume that's what's happening here. It still seems a bit strange that we acknowledge the attachment before the remote frame is actually in place.
Project Member

Comment 7 by bugdroid1@chromium.org, Jan 29 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ec31db29d7a9a2df8e93a1b115326c74b4333f19

commit ec31db29d7a9a2df8e93a1b115326c74b4333f19
Author: Kevin McNee <mcnee@chromium.org>
Date: Mon Jan 29 18:17:35 2018

OOPIF guest view: Allow a GuestViewContainer to be reattached.

In the OOPIF-based guest implementation of attachment, the contentWindow
of the internal iframe element is used to identify the local frame to
swap for a remote frame. When the guest is destroyed, we currently don't
"reset" the internal iframe element so that it can be used for another
attachment. When we try to attach, the frame is still remote, which
causes GuestViewInternalCustomBindings::AttachIframeGuest to be unable
to find the frame to swap.

We now replace our internal iframe element upon guest destruction, so
that the frame is local when we reattach.

Bug:  800407 
Change-Id: I4424793c7c0230d62a424765bc3c288080b2e0b9
Reviewed-on: https://chromium-review.googlesource.com/871292
Reviewed-by: Paul Meyer <paulmeyer@chromium.org>
Commit-Queue: Kevin McNee <mcnee@chromium.org>
Cr-Commit-Position: refs/heads/master@{#532495}
[modify] https://crrev.com/ec31db29d7a9a2df8e93a1b115326c74b4333f19/chrome/browser/apps/guest_view/app_view_browsertest.cc
[modify] https://crrev.com/ec31db29d7a9a2df8e93a1b115326c74b4333f19/chrome/browser/apps/guest_view/extension_view/extension_view_browsertest.cc
[modify] https://crrev.com/ec31db29d7a9a2df8e93a1b115326c74b4333f19/chrome/test/data/extensions/platform_apps/app_view/shim/main.js
[modify] https://crrev.com/ec31db29d7a9a2df8e93a1b115326c74b4333f19/chrome/test/data/extensions/platform_apps/extension_view/load_api/main.js
[modify] https://crrev.com/ec31db29d7a9a2df8e93a1b115326c74b4333f19/extensions/renderer/resources/guest_view/app_view/app_view.js
[modify] https://crrev.com/ec31db29d7a9a2df8e93a1b115326c74b4333f19/extensions/renderer/resources/guest_view/extension_options/extension_options.js
[modify] https://crrev.com/ec31db29d7a9a2df8e93a1b115326c74b4333f19/extensions/renderer/resources/guest_view/extension_view/extension_view.js
[modify] https://crrev.com/ec31db29d7a9a2df8e93a1b115326c74b4333f19/extensions/renderer/resources/guest_view/guest_view_container.js
[modify] https://crrev.com/ec31db29d7a9a2df8e93a1b115326c74b4333f19/extensions/renderer/resources/guest_view/guest_view_iframe.js
[modify] https://crrev.com/ec31db29d7a9a2df8e93a1b115326c74b4333f19/extensions/renderer/resources/guest_view/guest_view_iframe_container.js
[modify] https://crrev.com/ec31db29d7a9a2df8e93a1b115326c74b4333f19/extensions/renderer/resources/guest_view/web_view/web_view.js

Comment 8 by mcnee@chromium.org, Jan 29 2018

Status: Fixed (was: Assigned)
Also note that the attachment ack order issue I mentioned in c#6 is fixed in https://chromium-review.googlesource.com/c/chromium/src/+/868277

Sign in to add a comment