Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1949163002:
SendPageMessage() should not send to a WebView guest's embedder. (Closed)
|
Created:
4 years, 7 months ago by wjmaclean Modified:
4 years, 7 months ago CC:
chromium-reviews, darin-cc_chromium.org, nasko+codewatch_chromium.org, jam, creis+watch_chromium.org, lazyboy Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSendPageMessage() should not send to a WebView guest's embedder.
The test failure in the bug listed below is due to SendPageMessage(),
when initiated from the guest's WebContentsImpl(), inadvertently
sending messages to the embedder. For zoom messages this
is inappropriate, and for all messages seems to run counter to the notion
of the 'page' being the frames owned by the WebContents.
BUG=607978
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/a767092b45f7eafcc57c704f536d05bdf953c773
Cr-Commit-Position: refs/heads/master@{#392081}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Revise to use site instances. #
Total comments: 4
Patch Set 3 : Address suggestions. #Patch Set 4 : Use GetProxyToOuterDelegate() to filter page messages. #
Total comments: 1
Patch Set 5 : Revise comment. #
Messages
Total messages: 39 (16 generated)
Description was changed from ========== SendPageMessage() should not send to a WebView guest's embedder. The test failure in the bug listed below is due to SendPageMessage() inadvertently sending zoom messages to the embedder, which is inappropriate. BUG=607978 ========== to ========== SendPageMessage() should not send to a WebView guest's embedder. The test failure in the bug listed below is due to SendPageMessage() inadvertently sending zoom messages to the embedder, which is inappropriate. BUG=607978 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1949163002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1949163002/1
Description was changed from ========== SendPageMessage() should not send to a WebView guest's embedder. The test failure in the bug listed below is due to SendPageMessage() inadvertently sending zoom messages to the embedder, which is inappropriate. BUG=607978 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== SendPageMessage() should not send to a WebView guest's embedder. The test failure in the bug listed below is due to SendPageMessage(), when initiated from the guest's WebContentsImpl(), inadvertently sending zoom messages to the embedder. For zoom messages this is inappropriate. BUG=607978 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== SendPageMessage() should not send to a WebView guest's embedder. The test failure in the bug listed below is due to SendPageMessage(), when initiated from the guest's WebContentsImpl(), inadvertently sending zoom messages to the embedder. For zoom messages this is inappropriate. BUG=607978 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== SendPageMessage() should not send to a WebView guest's embedder. The test failure in the bug listed below is due to SendPageMessage(), when initiated from the guest's WebContentsImpl(), inadvertently sending zoom messages to the embedder. For zoom messages this is inappropriate, and seems to run counter to the notion of the 'page' being the frames owned by the WebContents. BUG=607978 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== SendPageMessage() should not send to a WebView guest's embedder. The test failure in the bug listed below is due to SendPageMessage(), when initiated from the guest's WebContentsImpl(), inadvertently sending zoom messages to the embedder. For zoom messages this is inappropriate, and seems to run counter to the notion of the 'page' being the frames owned by the WebContents. BUG=607978 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== SendPageMessage() should not send to a WebView guest's embedder. The test failure in the bug listed below is due to SendPageMessage(), when initiated from the guest's WebContentsImpl(), inadvertently sending messages to the embedder. For zoom messages this is inappropriate, and for all messages seems to run counter to the notion of the 'page' being the frames owned by the WebContents. BUG=607978 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
wjmaclean@chromium.org changed reviewers: + lfg@chromium.org, nasko@chromium.org
Small change, please take a look? lfg@ - I uploaded this before our discussion about whether or not WebViews can contain OOPIFs (I thought the answer was 'no', hence my exclusion of all remote proxies from SenPageMessage() if ForInnerDelegate() is true). This may have to be amended to specifically identify the proxy for the embedder, while still sending to subframes' proxies. So far though, it looks to be doing well on the bots ...
https://codereview.chromium.org/1949163002/diff/1/chrome/browser/apps/guest_v... File chrome/browser/apps/guest_view/web_view_browsertest.cc (right): https://codereview.chromium.org/1949163002/diff/1/chrome/browser/apps/guest_v... chrome/browser/apps/guest_view/web_view_browsertest.cc:2432: TestHelper("testZoomAPI", "web_view/shim", NO_TEST_SERVER); Need to fix indent after removal of the if statement. https://codereview.chromium.org/1949163002/diff/1/content/browser/frame_host/... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1949163002/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_manager.cc:2506: // When sending a PageMessage to a WebView's guest, we don't want to also nit: WebView is overloaded term, let's avoid using it, especially in content/, which doesn't even have that concept. We have the inner/outer relationship we can use here. https://codereview.chromium.org/1949163002/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_manager.cc:2509: if (!ForInnerDelegate()) { This will not be true if we allow cross-process navigations in GuestViews. Can we instead compare the SiteInstance of the proxy with the SiteInstance of the outer WebContents node?
On 2016/05/04 20:57:22, wjmaclean wrote: > Small change, please take a look? > > lfg@ - I uploaded this before our discussion about whether or not WebViews can > contain OOPIFs (I thought the answer was 'no', hence my exclusion of all remote > proxies from SenPageMessage() if ForInnerDelegate() is true). This may have to > be amended to specifically identify the proxy for the embedder, while still > sending to subframes' proxies. So far though, it looks to be doing well on the > bots ... This shouldn't be an issue, the <webview> can only be a sibling with an OOPIF today. Also, when you address nasko@'s suggestion it should make this even more generic, and should work in all possible cases (even if we allow oopifs inside <webview>).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I've revised this to use a comparison of the outer-delegate's site instance (I'm assuming it's safe to compare SiteInstance pointers for equality, is that right?) https://codereview.chromium.org/1949163002/diff/1/chrome/browser/apps/guest_v... File chrome/browser/apps/guest_view/web_view_browsertest.cc (right): https://codereview.chromium.org/1949163002/diff/1/chrome/browser/apps/guest_v... chrome/browser/apps/guest_view/web_view_browsertest.cc:2432: TestHelper("testZoomAPI", "web_view/shim", NO_TEST_SERVER); On 2016/05/04 21:58:58, nasko (slow) wrote: > Need to fix indent after removal of the if statement. Ooops! :-) Fixed. https://codereview.chromium.org/1949163002/diff/1/content/browser/frame_host/... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1949163002/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_manager.cc:2506: // When sending a PageMessage to a WebView's guest, we don't want to also On 2016/05/04 21:58:59, nasko (slow) wrote: > nit: WebView is overloaded term, let's avoid using it, especially in content/, > which doesn't even have that concept. We have the inner/outer relationship we > can use here. Done. https://codereview.chromium.org/1949163002/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_manager.cc:2509: if (!ForInnerDelegate()) { On 2016/05/04 21:58:59, nasko (slow) wrote: > This will not be true if we allow cross-process navigations in GuestViews. Can > we instead compare the SiteInstance of the proxy with the SiteInstance of the > outer WebContents node? I'm not sure I understand this. What if some sub-frame to the inner contents has the same SiteInstance as the outer WebContents? How would we differentiate them?
The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1949163002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1949163002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1949163002/diff/20001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1949163002/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:2510: ? GetOuterDelegateNode()->current_frame_host()->GetSiteInstance() I think we probably want to look at the SiteInstance of the parent of the outer delegate node, since the RFH in the outer delegate node isn't used.
PTAL https://codereview.chromium.org/1949163002/diff/20001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1949163002/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:2510: ? GetOuterDelegateNode()->current_frame_host()->GetSiteInstance() On 2016/05/05 17:27:11, lfg wrote: > I think we probably want to look at the SiteInstance of the parent of the outer > delegate node, since the RFH in the outer delegate node isn't used. Done. We're sure that parent() is always non-null, right?
The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1949163002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1949163002/40001
https://codereview.chromium.org/1949163002/diff/20001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1949163002/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:2510: ? GetOuterDelegateNode()->current_frame_host()->GetSiteInstance() On 2016/05/05 17:27:11, lfg wrote: > I think we probably want to look at the SiteInstance of the parent of the outer > delegate node, since the RFH in the outer delegate node isn't used. Just curious: as a casual reader of this code, did I miss some indication about the OuterDelegate's RFH not being used? Is it worth commenting? I do now notice other cases where parent() is used now that you mention it, though it's not obvious why. Also, I just discovered GetProxyToOuterDelegate() ... should we be using that here instead of SiteInstances, in a manner similar to RFHM::CreateProxiesForChildFrame() ?
https://codereview.chromium.org/1949163002/diff/20001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1949163002/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:2510: ? GetOuterDelegateNode()->current_frame_host()->GetSiteInstance() On 2016/05/05 18:53:49, wjmaclean wrote: > On 2016/05/05 17:27:11, lfg wrote: > > I think we probably want to look at the SiteInstance of the parent of the > outer > > delegate node, since the RFH in the outer delegate node isn't used. > > Just curious: as a casual reader of this code, did I miss some indication about > the OuterDelegate's RFH not being used? Is it worth commenting? > > I do now notice other cases where parent() is used now that you mention it, > though it's not obvious why. > > Also, I just discovered GetProxyToOuterDelegate() ... should we be using that > here instead of SiteInstances, in a manner similar to > RFHM::CreateProxiesForChildFrame() ? +lazyboy The RFH is owned by the RFHM, which comes with every FTN. When we create an inner WebContents, we swap the RenderFrame in the outer WebContents renderer with a RenderFrameProxy, and that RFPH lives in the inner WebContents FTN. Since we still want to keep the FTN in the outer WebContents tree (as a connection point between the two trees), we end up with a RFH that does not have a live RF.
The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1949163002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1949163002/60001
On 2016/05/05 19:01:00, lfg wrote: > https://codereview.chromium.org/1949163002/diff/20001/content/browser/frame_h... > File content/browser/frame_host/render_frame_host_manager.cc (right): > > https://codereview.chromium.org/1949163002/diff/20001/content/browser/frame_h... > content/browser/frame_host/render_frame_host_manager.cc:2510: ? > GetOuterDelegateNode()->current_frame_host()->GetSiteInstance() > On 2016/05/05 18:53:49, wjmaclean wrote: > > On 2016/05/05 17:27:11, lfg wrote: > > > I think we probably want to look at the SiteInstance of the parent of the > > outer > > > delegate node, since the RFH in the outer delegate node isn't used. > > > > Just curious: as a casual reader of this code, did I miss some indication > about > > the OuterDelegate's RFH not being used? Is it worth commenting? > > > > I do now notice other cases where parent() is used now that you mention it, > > though it's not obvious why. > > > > Also, I just discovered GetProxyToOuterDelegate() ... should we be using that > > here instead of SiteInstances, in a manner similar to > > RFHM::CreateProxiesForChildFrame() ? > > +lazyboy > > The RFH is owned by the RFHM, which comes with every FTN. When we create an > inner WebContents, we swap the RenderFrame in the outer WebContents renderer > with a RenderFrameProxy, and that RFPH lives in the inner WebContents FTN. Since > we still want to keep the FTN in the outer WebContents tree (as a connection > point between the two trees), we end up with a RFH that does not have a live RF. Does this explanation live in the code somewhere as a comment? Or in a link to a design doc? :-)
nasko@ lfg@ I'm ok with using either of the approaches in PS#3 or PS#4 (slight preference for #4 as it follows an existing pattern elsewhere in RenderFrameHostManager). Let me know your preferences.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
content/ LGTM in PS4. https://codereview.chromium.org/1949163002/diff/60001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1949163002/diff/60001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:2507: // send it to the outer WebContent's frames as well. nit: s/frames/frame/ as it can't send to more than the containing frame.
The CQ bit was checked by wjmaclean@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nasko@chromium.org Link to the patchset: https://codereview.chromium.org/1949163002/#ps80001 (title: "Revise comment.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1949163002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1949163002/80001
Message was sent while issue was closed.
Description was changed from ========== SendPageMessage() should not send to a WebView guest's embedder. The test failure in the bug listed below is due to SendPageMessage(), when initiated from the guest's WebContentsImpl(), inadvertently sending messages to the embedder. For zoom messages this is inappropriate, and for all messages seems to run counter to the notion of the 'page' being the frames owned by the WebContents. BUG=607978 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== SendPageMessage() should not send to a WebView guest's embedder. The test failure in the bug listed below is due to SendPageMessage(), when initiated from the guest's WebContentsImpl(), inadvertently sending messages to the embedder. For zoom messages this is inappropriate, and for all messages seems to run counter to the notion of the 'page' being the frames owned by the WebContents. BUG=607978 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== SendPageMessage() should not send to a WebView guest's embedder. The test failure in the bug listed below is due to SendPageMessage(), when initiated from the guest's WebContentsImpl(), inadvertently sending messages to the embedder. For zoom messages this is inappropriate, and for all messages seems to run counter to the notion of the 'page' being the frames owned by the WebContents. BUG=607978 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== SendPageMessage() should not send to a WebView guest's embedder. The test failure in the bug listed below is due to SendPageMessage(), when initiated from the guest's WebContentsImpl(), inadvertently sending messages to the embedder. For zoom messages this is inappropriate, and for all messages seems to run counter to the notion of the 'page' being the frames owned by the WebContents. BUG=607978 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/a767092b45f7eafcc57c704f536d05bdf953c773 Cr-Commit-Position: refs/heads/master@{#392081} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/a767092b45f7eafcc57c704f536d05bdf953c773 Cr-Commit-Position: refs/heads/master@{#392081}
Message was sent while issue was closed.
On 2016/05/05 19:03:19, wjmaclean wrote: > > The RFH is owned by the RFHM, which comes with every FTN. When we create an > > inner WebContents, we swap the RenderFrame in the outer WebContents renderer > > with a RenderFrameProxy, and that RFPH lives in the inner WebContents FTN. > Since > > we still want to keep the FTN in the outer WebContents tree (as a connection > > point between the two trees), we end up with a RFH that does not have a live > RF. > > Does this explanation live in the code somewhere as a comment? Or in a link to a > design doc? :-) I don't think this is well documented, and it would be a good idea to write a document explaining some of those differences with outer/inner WebContents and regular OOPIFs. I think it's also a great way to learn about interactions with the FrameTree on OOPIF-based webview (maybe you could look at doing it? ;)).
Message was sent while issue was closed.
On 2016/05/06 17:21:05, lfg wrote: > On 2016/05/05 19:03:19, wjmaclean wrote: > > > The RFH is owned by the RFHM, which comes with every FTN. When we create an > > > inner WebContents, we swap the RenderFrame in the outer WebContents renderer > > > with a RenderFrameProxy, and that RFPH lives in the inner WebContents FTN. > > Since > > > we still want to keep the FTN in the outer WebContents tree (as a connection > > > point between the two trees), we end up with a RFH that does not have a live > > RF. > > > > Does this explanation live in the code somewhere as a comment? Or in a link to > a > > design doc? :-) > > I don't think this is well documented, and it would be a good idea to write a > document explaining some of those differences with outer/inner WebContents and > regular OOPIFs. I think it's also a great way to learn about interactions with > the FrameTree on OOPIF-based webview (maybe you could look at doing it? ;)). Yes, I need to dig through this code and learn it anyways, so I'll generate some docs for it :-) |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
