Issue 2506183002: Make window.open() IPCs be frame-based

ncarter (slow)

Description was changed from ========== _hoo# Enter a description of the change. Work towards moving ...

4 years, 1 month ago (2016-11-16 22:52:40 UTC) #1

ncarter (slow)

Description was changed from ========== _hoo# Enter a description of the change. Work towards moving ...

4 years, 1 month ago (2016-11-16 22:53:08 UTC) #2

ncarter (slow)

The CQ bit was checked by nick@chromium.org to run a CQ dry run

4 years ago (2016-12-08 21:17:06 UTC) #3

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2506183002/40001

4 years ago (2016-12-08 21:17:47 UTC) #4

ncarter (slow)

Description was changed from ========== Work towards moving CreateNewWindow to be frame-based. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== ...

4 years ago (2016-12-08 21:41:47 UTC) #5

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years ago (2016-12-08 21:54:01 UTC) #6

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_compile_dbg/builds/178024)

4 years ago (2016-12-08 21:54:02 UTC) #7

ncarter (slow)

The CQ bit was checked by nick@chromium.org to run a CQ dry run

4 years ago (2016-12-09 00:09:21 UTC) #8

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2506183002/60001

4 years ago (2016-12-09 00:09:49 UTC) #9

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years ago (2016-12-09 00:23:41 UTC) #10

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_rel_ng/builds/331516)

4 years ago (2016-12-09 00:23:41 UTC) #11

ncarter (slow)

The CQ bit was checked by nick@chromium.org to run a CQ dry run

4 years ago (2016-12-09 18:15:35 UTC) #12

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2506183002/60001

4 years ago (2016-12-09 18:16:21 UTC) #13

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years ago (2016-12-09 19:04:07 UTC) #14

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm64_dbg_recipe/builds/178131)

4 years ago (2016-12-09 19:04:08 UTC) #15

ncarter (slow)

Description was changed from ========== Make the CreateNewWindow and ShowCreatedWindow IPCs be routed via the ...

4 years ago (2016-12-09 19:37:09 UTC) #16

ncarter (slow)

The CQ bit was checked by nick@chromium.org to run a CQ dry run

4 years ago (2016-12-09 20:56:08 UTC) #17

ncarter (slow)

Description was changed from ========== Make the CreateNewWindow and ShowCreatedWindow IPCs be routed via the ...

4 years ago (2016-12-09 20:56:14 UTC) #18

Description was changed from

==========
Make the CreateNewWindow and ShowCreatedWindow IPCs be routed via the
opener frame, rather than the opener view.

Outline of this change:
 - CreateNewWindow params only need to identify the opener frame;
view IDs can be inferred from that. This eliminates possible trouble
from specifing unrelated RV and RF route ids here (though I don't
currently see a useful attack via this inconsistency, since
they still have to be same-process).
 - ShowCreatedWindow is likewise routed to the opener RFH.
 - The pending WebContents list is indexed by the main frame widget
(currently this is the same as the view ID, but this prepares them
to differ).
 - Cleanup-on-failure logic (ViewMsg_Close) is moved out to the 
RenderWidgetHelper::OnCreateNewWindowOnUI. This fixes some theoretical
RenderView leaks when window.open races a navigation that destroys
the opener. These races would have become more likely now that we're
routing via the frame.
 - 

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Make the CreateNewWindow and ShowCreatedWindow IPCs be routed via the
opener frame, rather than the opener view.

Outline of this change:
 - CreateNewWindow params only need to identify the opener frame;
view IDs can be inferred from that. This eliminates possible trouble
from specifing unrelated RV and RF route ids here (though I don't
currently see a useful attack via this inconsistency, since
they still have to be same-process).
 - ShowCreatedWindow is likewise routed to the opener RFH.
 - The pending WebContents list is indexed by the main frame widget
(currently this is the same as the view ID, but this prepares them
to differ).
 - The signature of ContentBrowserClient::CanCreateWindow changes to be
frame-based, which necessitates making SingleTabModeTabHelper be frame-based
also.
 - Cleanup-on-failure logic (ViewMsg_Close) is moved up to the 
RenderWidgetHelper::OnCreateNewWindowOnUI -- the outermost UI thread function
for the CreateNewWindow operation. This fixes some possible RenderView leaks
when window.open races a navigation that destroys
the opener. These races would have become more likely now that we're
routing via the frame.
 - 

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2506183002/80001

4 years ago (2016-12-09 20:57:05 UTC) #19

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years ago (2016-12-09 21:42:25 UTC) #20

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm64_dbg_recipe/builds/178254)

4 years ago (2016-12-09 21:42:26 UTC) #21

ncarter (slow)

The CQ bit was checked by nick@chromium.org to run a CQ dry run

4 years ago (2016-12-09 21:47:55 UTC) #22

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2506183002/100001

4 years ago (2016-12-09 21:49:10 UTC) #23

ncarter (slow)

Description was changed from ========== Make the CreateNewWindow and ShowCreatedWindow IPCs be routed via the ...

4 years ago (2016-12-09 21:50:28 UTC) #24

Description was changed from

==========
Make the CreateNewWindow and ShowCreatedWindow IPCs be routed via the
opener frame, rather than the opener view.

Outline of this change:
 - CreateNewWindow params only need to identify the opener frame;
view IDs can be inferred from that. This eliminates possible trouble
from specifing unrelated RV and RF route ids here (though I don't
currently see a useful attack via this inconsistency, since
they still have to be same-process).
 - ShowCreatedWindow is likewise routed to the opener RFH.
 - The pending WebContents list is indexed by the main frame widget
(currently this is the same as the view ID, but this prepares them
to differ).
 - The signature of ContentBrowserClient::CanCreateWindow changes to be
frame-based, which necessitates making SingleTabModeTabHelper be frame-based
also.
 - Cleanup-on-failure logic (ViewMsg_Close) is moved up to the 
RenderWidgetHelper::OnCreateNewWindowOnUI -- the outermost UI thread function
for the CreateNewWindow operation. This fixes some possible RenderView leaks
when window.open races a navigation that destroys
the opener. These races would have become more likely now that we're
routing via the frame.
 - 

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Make window.open() IPCs be frame-based.

 - Two IPCs, CreateNewWindow and ShowCreatedWindow, are made to be routed via
   the opener frame. Previously they were routed via the opener view.
 - The corresponding delegate methods move from RenderViewHostDelegate to
   RenderFrameHostDelegate, though the implementation stays in WebContentsImpl,
   which implements both interfaces.
 - Remove the RenderView opener_id parameter from the mojo CreateNewWindow
   params, it can be inferred from the RenderFrame id. This fixes a possible bad
   situation where an unrelated RV/RF route id pair could be supplied.
 - The pending WebContents list is made to be indexed by the main frame widget;
   this turns out to be cleaner than making it frame-based, since show() is
   an operation on the created RenderWidget.
 - The signature of ContentBrowserClient::CanCreateWindow changes to be frame-
   based, which necessitates retooling the android SingleTabModeTabHelper be
   frame-based also.
 - The ViewMsg_Close message, which cleans up after UI-thread failure of the
   CreateNewWindow operation, is hoisted out to the outermost function where
   failure is possible, RenderWidgetHelper::OnCreateNewWindowOnUI. This
   addresses some possible RenderView leaks when e.g. window.open() races a
   navigation that destroys the opener. These races would have become more
   likely now that we're routing via the frame.
 - The response to ShowCreatedWindow on the opener RenderFrame is a
   ViewMsg_Move_ACK on the openee RenderWidget. This IPC moves to
   WebContentsImpl::ShowCreatedWindow so that it can be sent via the openee
   RenderWidgetHost.
==========

ncarter (slow)

Description was changed from ========== Make window.open() IPCs be frame-based. - Two IPCs, CreateNewWindow and ...

4 years ago (2016-12-09 21:53:13 UTC) #25

Description was changed from

==========
Make window.open() IPCs be frame-based.

 - Two IPCs, CreateNewWindow and ShowCreatedWindow, are made to be routed via
   the opener frame. Previously they were routed via the opener view.
 - The corresponding delegate methods move from RenderViewHostDelegate to
   RenderFrameHostDelegate, though the implementation stays in WebContentsImpl,
   which implements both interfaces.
 - Remove the RenderView opener_id parameter from the mojo CreateNewWindow
   params, it can be inferred from the RenderFrame id. This fixes a possible bad
   situation where an unrelated RV/RF route id pair could be supplied.
 - The pending WebContents list is made to be indexed by the main frame widget;
   this turns out to be cleaner than making it frame-based, since show() is
   an operation on the created RenderWidget.
 - The signature of ContentBrowserClient::CanCreateWindow changes to be frame-
   based, which necessitates retooling the android SingleTabModeTabHelper be
   frame-based also.
 - The ViewMsg_Close message, which cleans up after UI-thread failure of the
   CreateNewWindow operation, is hoisted out to the outermost function where
   failure is possible, RenderWidgetHelper::OnCreateNewWindowOnUI. This
   addresses some possible RenderView leaks when e.g. window.open() races a
   navigation that destroys the opener. These races would have become more
   likely now that we're routing via the frame.
 - The response to ShowCreatedWindow on the opener RenderFrame is a
   ViewMsg_Move_ACK on the openee RenderWidget. This IPC moves to
   WebContentsImpl::ShowCreatedWindow so that it can be sent via the openee
   RenderWidgetHost.
==========

to

==========
Make window.open() IPCs be frame-based

 - Two IPCs, CreateNewWindow and ShowCreatedWindow, are made to be routed via
   the opener frame. Previously they were routed via the opener view.
 - The corresponding delegate methods move from RenderViewHostDelegate to
   RenderFrameHostDelegate, though the implementation stays in
   WebContentsImpl, which implements both interfaces.
 - Remove the RenderView opener_id parameter from the mojo CreateNewWindow
   params, it can be inferred from the RenderFrame id. This fixes a possible
   bad situation where an unrelated RV/RF route id pair could be supplied.
 - The pending WebContents list is made to be indexed by the main frame
   widget; this turns out to be cleaner than making it frame- based, since
   show() is an operation on the created RenderWidget.
 - The signature of ContentBrowserClient::CanCreateWindow changes to be
   frame-based, which necessitates retooling the android
   SingleTabModeTabHelper to be frame-based also.
 - The ViewMsg_Close message, which cleans up after UI-thread failure of the
   CreateNewWindow operation, is hoisted out to the outermost function where
   failure is possible, RenderWidgetHelper::OnCreateNewWindowOnUI. This
   addresses some possible RenderView leaks when e.g. window.open() races a
   navigation that destroys the opener. These races would have become more
   likely now that we're routing via the frame.
 - The response to ShowCreatedWindow on the opener RenderFrame is a
   ViewMsg_Move_ACK on the openee RenderWidget. This IPC moves to
   WebContentsImpl::ShowCreatedWindow so that it can be sent via the openee
   RenderWidgetHost.
==========

ncarter (slow)

Description was changed from ========== Make window.open() IPCs be frame-based - Two IPCs, CreateNewWindow and ...

4 years ago (2016-12-09 21:53:54 UTC) #26

Description was changed from

==========
Make window.open() IPCs be frame-based

 - Two IPCs, CreateNewWindow and ShowCreatedWindow, are made to be routed via
   the opener frame. Previously they were routed via the opener view.
 - The corresponding delegate methods move from RenderViewHostDelegate to
   RenderFrameHostDelegate, though the implementation stays in
   WebContentsImpl, which implements both interfaces.
 - Remove the RenderView opener_id parameter from the mojo CreateNewWindow
   params, it can be inferred from the RenderFrame id. This fixes a possible
   bad situation where an unrelated RV/RF route id pair could be supplied.
 - The pending WebContents list is made to be indexed by the main frame
   widget; this turns out to be cleaner than making it frame- based, since
   show() is an operation on the created RenderWidget.
 - The signature of ContentBrowserClient::CanCreateWindow changes to be
   frame-based, which necessitates retooling the android
   SingleTabModeTabHelper to be frame-based also.
 - The ViewMsg_Close message, which cleans up after UI-thread failure of the
   CreateNewWindow operation, is hoisted out to the outermost function where
   failure is possible, RenderWidgetHelper::OnCreateNewWindowOnUI. This
   addresses some possible RenderView leaks when e.g. window.open() races a
   navigation that destroys the opener. These races would have become more
   likely now that we're routing via the frame.
 - The response to ShowCreatedWindow on the opener RenderFrame is a
   ViewMsg_Move_ACK on the openee RenderWidget. This IPC moves to
   WebContentsImpl::ShowCreatedWindow so that it can be sent via the openee
   RenderWidgetHost.
==========

to

==========
Make window.open() IPCs be frame-based

 - Two IPCs, CreateNewWindow and ShowCreatedWindow, are made to be routed via
   the opener frame. Previously they were routed via the opener view.
 - The corresponding delegate methods move from RenderViewHostDelegate to
   RenderFrameHostDelegate, though the implementation stays in
   WebContentsImpl, which implements both interfaces.
 - Remove the RenderView opener_id parameter from the mojo CreateNewWindow
   params, it can be inferred from the RenderFrame id. This fixes a possible
   bad situation where an unrelated RV/RF route id pair could be supplied.
 - The pending WebContents list is made to be indexed by the main frame
   widget; this turns out to be cleaner than making it frame- based, since
   show() is an operation on the created RenderWidget.
 - The signature of ContentBrowserClient::CanCreateWindow changes to be
   frame-based, which necessitates retooling the android
   SingleTabModeTabHelper to be frame-based also.
 - The ViewMsg_Close message, which cleans up after UI-thread failure of the
   CreateNewWindow operation, is hoisted out to the outermost function where
   failure is possible, RenderWidgetHelper::OnCreateNewWindowOnUI. This
   addresses some possible RenderView leaks when e.g. window.open() races a
   navigation that destroys the opener. These races would have become more
   likely now that we're routing via the frame.
 - The response to ShowCreatedWindow on the opener RenderFrame is a
   ViewMsg_Move_ACK on the openee RenderWidget. This IPC moves to
   WebContentsImpl::ShowCreatedWindow so that it can be sent via the openee
   RenderWidgetHost's IPC::Sender, rather than via the opener RenderFrame.
==========

ncarter (slow)

Description was changed from ========== Make window.open() IPCs be frame-based - Two IPCs, CreateNewWindow and ...

4 years ago (2016-12-09 21:56:07 UTC) #27

Description was changed from

==========
Make window.open() IPCs be frame-based

 - Two IPCs, CreateNewWindow and ShowCreatedWindow, are made to be routed via
   the opener frame. Previously they were routed via the opener view.
 - The corresponding delegate methods move from RenderViewHostDelegate to
   RenderFrameHostDelegate, though the implementation stays in
   WebContentsImpl, which implements both interfaces.
 - Remove the RenderView opener_id parameter from the mojo CreateNewWindow
   params, it can be inferred from the RenderFrame id. This fixes a possible
   bad situation where an unrelated RV/RF route id pair could be supplied.
 - The pending WebContents list is made to be indexed by the main frame
   widget; this turns out to be cleaner than making it frame- based, since
   show() is an operation on the created RenderWidget.
 - The signature of ContentBrowserClient::CanCreateWindow changes to be
   frame-based, which necessitates retooling the android
   SingleTabModeTabHelper to be frame-based also.
 - The ViewMsg_Close message, which cleans up after UI-thread failure of the
   CreateNewWindow operation, is hoisted out to the outermost function where
   failure is possible, RenderWidgetHelper::OnCreateNewWindowOnUI. This
   addresses some possible RenderView leaks when e.g. window.open() races a
   navigation that destroys the opener. These races would have become more
   likely now that we're routing via the frame.
 - The response to ShowCreatedWindow on the opener RenderFrame is a
   ViewMsg_Move_ACK on the openee RenderWidget. This IPC moves to
   WebContentsImpl::ShowCreatedWindow so that it can be sent via the openee
   RenderWidgetHost's IPC::Sender, rather than via the opener RenderFrame.
==========

to

==========
Make window.open() IPCs be frame-based.

 - Two IPCs, CreateNewWindow and ShowCreatedWindow, are made to be routed via
   the opener frame. Previously they were routed via the opener view.
 - The corresponding delegate methods move from RenderViewHostDelegate to
   RenderFrameHostDelegate, though the implementation stays in
   WebContentsImpl, which implements both interfaces.
 - Remove the RenderView opener_id parameter from the mojo CreateNewWindow
   params, it can be inferred from the RenderFrame id. This fixes a possible
   bad situation where an unrelated RV/RF route id pair could be supplied.
 - The pending WebContents list is made to be indexed by the main frame
   widget; this turns out to be cleaner than making it frame- based, since
   show() is an operation on the created RenderWidget.
 - The signature of ContentBrowserClient::CanCreateWindow changes to be
   frame-based, which necessitates retooling the android
   SingleTabModeTabHelper to be frame-based also.
 - The ViewMsg_Close message, which cleans up after UI-thread failure of the
   CreateNewWindow operation, is hoisted out to the outermost function where
   failure is possible, RenderWidgetHelper::OnCreateNewWindowOnUI. This
   addresses some possible RenderView leaks when e.g. window.open() races a
   navigation that destroys the opener. These races would have become more
   likely now that we're routing via the frame, since child frames can have a
   shorter lifetime than their RenderViewHosts.
 - The response to ShowCreatedWindow on the opener RenderFrame is a
   ViewMsg_Move_ACK on the openee RenderWidget. This IPC moves to
   WebContentsImpl::ShowCreatedWindow so that it can be sent via the openee
   RenderWidgetHost's IPC::Sender, rather than via the opener RenderFrame.
==========

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years ago (2016-12-09 23:36:34 UTC) #28

ncarter (slow)

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

4 years ago (2016-12-12 19:06:01 UTC) #30

ncarter (slow)

Description was changed from ========== Make window.open() IPCs be frame-based. - Two IPCs, CreateNewWindow and ...

4 years ago (2016-12-12 19:07:26 UTC) #32

Description was changed from

==========
Make window.open() IPCs be frame-based.

 - Two IPCs, CreateNewWindow and ShowCreatedWindow, are made to be routed via
   the opener frame. Previously they were routed via the opener view.
 - The corresponding delegate methods move from RenderViewHostDelegate to
   RenderFrameHostDelegate, though the implementation stays in
   WebContentsImpl, which implements both interfaces.
 - Remove the RenderView opener_id parameter from the mojo CreateNewWindow
   params, it can be inferred from the RenderFrame id. This fixes a possible
   bad situation where an unrelated RV/RF route id pair could be supplied.
 - The pending WebContents list is made to be indexed by the main frame
   widget; this turns out to be cleaner than making it frame- based, since
   show() is an operation on the created RenderWidget.
 - The signature of ContentBrowserClient::CanCreateWindow changes to be
   frame-based, which necessitates retooling the android
   SingleTabModeTabHelper to be frame-based also.
 - The ViewMsg_Close message, which cleans up after UI-thread failure of the
   CreateNewWindow operation, is hoisted out to the outermost function where
   failure is possible, RenderWidgetHelper::OnCreateNewWindowOnUI. This
   addresses some possible RenderView leaks when e.g. window.open() races a
   navigation that destroys the opener. These races would have become more
   likely now that we're routing via the frame, since child frames can have a
   shorter lifetime than their RenderViewHosts.
 - The response to ShowCreatedWindow on the opener RenderFrame is a
   ViewMsg_Move_ACK on the openee RenderWidget. This IPC moves to
   WebContentsImpl::ShowCreatedWindow so that it can be sent via the openee
   RenderWidgetHost's IPC::Sender, rather than via the opener RenderFrame.
==========

to

==========
Make window.open() IPCs be frame-based.

 - Two IPCs, CreateNewWindow and ShowCreatedWindow, are made to be routed via
   the opener frame. Previously they were routed via the opener view.
 - CreateNewWindow (which is mojofied) stays put but its parameters change
 - ViewHostMsg_ShowView becomes FrameHostMsg_ShowCreatedWindow
 - The corresponding delegate methods move from RenderViewHostDelegate to
   RenderFrameHostDelegate, though the implementation stays in
   WebContentsImpl, which implements both interfaces.
 - Remove the RenderView opener_id parameter from the mojo CreateNewWindow
   params, it can be inferred from the RenderFrame id. This fixes a possible
   bad situation where an unrelated RV/RF route id pair could be supplied.
 - The pending WebContents list is made to be indexed by the main frame
   widget; this turns out to be cleaner than making it frame- based, since
   show() is an operation on the created RenderWidget.
 - The signature of ContentBrowserClient::CanCreateWindow changes to be
   frame-based, which necessitates retooling the android
   SingleTabModeTabHelper to be frame-based also.
 - The ViewMsg_Close message, which cleans up after UI-thread failure of the
   CreateNewWindow operation, is hoisted out to the outermost function where
   failure is possible, RenderWidgetHelper::OnCreateNewWindowOnUI. This
   addresses some possible RenderView leaks when e.g. window.open() races a
   navigation that destroys the opener. These races would have become more
   likely now that we're routing via the frame, since child frames can have a
   shorter lifetime than their RenderViewHosts.
 - The response to ShowCreatedWindow on the opener RenderFrame is a
   ViewMsg_Move_ACK on the openee RenderWidget. This IPC moves to
   WebContentsImpl::ShowCreatedWindow so that it can be sent via the openee
   RenderWidgetHost's IPC::Sender, rather than via the opener RenderFrame.
==========

Avi (use Gerrit)

drive-by https://codereview.chromium.org/2506183002/diff/100001/content/browser/frame_host/interstitial_page_impl.cc File content/browser/frame_host/interstitial_page_impl.cc (right): https://codereview.chromium.org/2506183002/diff/100001/content/browser/frame_host/interstitial_page_impl.cc#newcode770 content/browser/frame_host/interstitial_page_impl.cc:770: << "InterstitialPage does not support showing full screen ...

4 years ago (2016-12-12 19:17:05 UTC) #33

alexmos

Thanks so much for working on this - this is amazing! https://codereview.chromium.org/2506183002/diff/100001/content/browser/frame_host/interstitial_page_impl.h File content/browser/frame_host/interstitial_page_impl.h (right): ...

4 years ago (2016-12-13 18:41:41 UTC) #34

alexmos

Also, it'd be nice to reference a bug number for this. Looks like we've got ...

4 years ago (2016-12-13 18:46:22 UTC) #35

ncarter (slow)

Description was changed from ========== Make window.open() IPCs be frame-based. - Two IPCs, CreateNewWindow and ...

4 years ago (2016-12-14 22:44:12 UTC) #36

Description was changed from

==========
Make window.open() IPCs be frame-based.

 - Two IPCs, CreateNewWindow and ShowCreatedWindow, are made to be routed via
   the opener frame. Previously they were routed via the opener view.
 - CreateNewWindow (which is mojofied) stays put but its parameters change
 - ViewHostMsg_ShowView becomes FrameHostMsg_ShowCreatedWindow
 - The corresponding delegate methods move from RenderViewHostDelegate to
   RenderFrameHostDelegate, though the implementation stays in
   WebContentsImpl, which implements both interfaces.
 - Remove the RenderView opener_id parameter from the mojo CreateNewWindow
   params, it can be inferred from the RenderFrame id. This fixes a possible
   bad situation where an unrelated RV/RF route id pair could be supplied.
 - The pending WebContents list is made to be indexed by the main frame
   widget; this turns out to be cleaner than making it frame- based, since
   show() is an operation on the created RenderWidget.
 - The signature of ContentBrowserClient::CanCreateWindow changes to be
   frame-based, which necessitates retooling the android
   SingleTabModeTabHelper to be frame-based also.
 - The ViewMsg_Close message, which cleans up after UI-thread failure of the
   CreateNewWindow operation, is hoisted out to the outermost function where
   failure is possible, RenderWidgetHelper::OnCreateNewWindowOnUI. This
   addresses some possible RenderView leaks when e.g. window.open() races a
   navigation that destroys the opener. These races would have become more
   likely now that we're routing via the frame, since child frames can have a
   shorter lifetime than their RenderViewHosts.
 - The response to ShowCreatedWindow on the opener RenderFrame is a
   ViewMsg_Move_ACK on the openee RenderWidget. This IPC moves to
   WebContentsImpl::ShowCreatedWindow so that it can be sent via the openee
   RenderWidgetHost's IPC::Sender, rather than via the opener RenderFrame.
==========

to

==========
Make window.open() IPCs be frame-based.

 - Two IPCs, CreateNewWindow and ShowCreatedWindow, are made to be routed via
   the opener frame. Previously they were routed via the opener view.
 - CreateNewWindow (which is mojofied) stays put but its parameters change
 - ViewHostMsg_ShowView becomes FrameHostMsg_ShowCreatedWindow
 - The corresponding delegate methods move from RenderViewHostDelegate to
   RenderFrameHostDelegate, though the implementation stays in
   WebContentsImpl, which implements both interfaces.
 - Remove the RenderView opener_id parameter from the mojo CreateNewWindow
   params, it can be inferred from the RenderFrame id. This fixes a possible
   bad situation where an unrelated RV/RF route id pair could be supplied.
 - The pending WebContents list is made to be indexed by the main frame
   widget; this turns out to be cleaner than making it frame- based, since
   show() is an operation on the created RenderWidget.
 - The signature of ContentBrowserClient::CanCreateWindow changes to be
   frame-based, which necessitates retooling the android
   SingleTabModeTabHelper to be frame-based also.
 - The ViewMsg_Close message, which cleans up after UI-thread failure of the
   CreateNewWindow operation, is hoisted out to the outermost function where
   failure is possible, RenderWidgetHelper::OnCreateNewWindowOnUI. This
   addresses some possible RenderView leaks when e.g. window.open() races a
   navigation that destroys the opener. These races would have become more
   likely now that we're routing via the frame, since child frames can have a
   shorter lifetime than their RenderViewHosts.
 - The response to ShowCreatedWindow on the opener RenderFrame is a
   ViewMsg_Move_ACK on the openee RenderWidget. This IPC moves to
   WebContentsImpl::ShowCreatedWindow so that it can be sent via the openee
   RenderWidgetHost's IPC::Sender, rather than via the opener RenderFrame.

BUG=431769, 304341
==========

ncarter (slow)

Description was changed from ========== Make window.open() IPCs be frame-based. - Two IPCs, CreateNewWindow and ...

4 years ago (2016-12-14 23:48:56 UTC) #37

Description was changed from

==========
Make window.open() IPCs be frame-based.

 - Two IPCs, CreateNewWindow and ShowCreatedWindow, are made to be routed via
   the opener frame. Previously they were routed via the opener view.
 - CreateNewWindow (which is mojofied) stays put but its parameters change
 - ViewHostMsg_ShowView becomes FrameHostMsg_ShowCreatedWindow
 - The corresponding delegate methods move from RenderViewHostDelegate to
   RenderFrameHostDelegate, though the implementation stays in
   WebContentsImpl, which implements both interfaces.
 - Remove the RenderView opener_id parameter from the mojo CreateNewWindow
   params, it can be inferred from the RenderFrame id. This fixes a possible
   bad situation where an unrelated RV/RF route id pair could be supplied.
 - The pending WebContents list is made to be indexed by the main frame
   widget; this turns out to be cleaner than making it frame- based, since
   show() is an operation on the created RenderWidget.
 - The signature of ContentBrowserClient::CanCreateWindow changes to be
   frame-based, which necessitates retooling the android
   SingleTabModeTabHelper to be frame-based also.
 - The ViewMsg_Close message, which cleans up after UI-thread failure of the
   CreateNewWindow operation, is hoisted out to the outermost function where
   failure is possible, RenderWidgetHelper::OnCreateNewWindowOnUI. This
   addresses some possible RenderView leaks when e.g. window.open() races a
   navigation that destroys the opener. These races would have become more
   likely now that we're routing via the frame, since child frames can have a
   shorter lifetime than their RenderViewHosts.
 - The response to ShowCreatedWindow on the opener RenderFrame is a
   ViewMsg_Move_ACK on the openee RenderWidget. This IPC moves to
   WebContentsImpl::ShowCreatedWindow so that it can be sent via the openee
   RenderWidgetHost's IPC::Sender, rather than via the opener RenderFrame.

BUG=431769, 304341
==========

to

==========
Make window.open() IPCs be frame-based.

 - Two IPCs, CreateNewWindow and ShowCreatedWindow, are made to be routed via
   the opener frame. Previously they were routed via the opener view.
 - CreateNewWindow (which is mojofied) stays put but its parameters change
 - ViewHostMsg_ShowView becomes FrameHostMsg_ShowCreatedWindow
 - The corresponding delegate methods move from RenderViewHostDelegate to
   RenderFrameHostDelegate, though the implementation stays in
   WebContentsImpl, which implements both interfaces.
 - Remove the RenderView opener_id parameter from the mojo CreateNewWindow
   params, it can be inferred from the RenderFrame id. This fixes a possible
   bad situation where an unrelated RV/RF route id pair could be supplied.
 - The pending WebContents list is made to be indexed by the main frame
   widget; this turns out to be cleaner than making it frame- based, since
   show() is an operation on the created RenderWidget.
 - The signature of ContentBrowserClient::CanCreateWindow changes to be
   frame-based, which necessitates retooling the android
   SingleTabModeTabHelper to be frame-based also.
 - The ViewMsg_Close message, which cleans up after UI-thread failure of the
   CreateNewWindow operation, is hoisted out to the outermost function where
   failure is possible, RenderWidgetHelper::OnCreateNewWindowOnUI. This
   addresses some possible RenderView leaks when e.g. window.open() races a
   navigation that destroys the opener. These races would have become more
   likely now that we're routing via the frame, since child frames can have a
   shorter lifetime than their RenderViewHosts.
 - The response to ShowCreatedWindow on the opener RenderFrame is a
   ViewMsg_Move_ACK on the openee RenderWidget. This IPC moves to
   WebContentsImpl::ShowCreatedWindow so that it can be sent via the openee
   RenderWidgetHost's IPC::Sender, rather than via the opener RenderFrame.

BUG=431769, 304341
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

ncarter (slow)

The CQ bit was checked by nick@chromium.org to run a CQ dry run

4 years ago (2016-12-14 23:51:10 UTC) #38

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2506183002/120001

4 years ago (2016-12-14 23:51:41 UTC) #39

ncarter (slow)

The CQ bit was checked by nick@chromium.org to run a CQ dry run

4 years ago (2016-12-15 00:32:42 UTC) #40

ncarter (slow)

ptal https://codereview.chromium.org/2506183002/diff/100001/content/browser/frame_host/interstitial_page_impl.cc File content/browser/frame_host/interstitial_page_impl.cc (right): https://codereview.chromium.org/2506183002/diff/100001/content/browser/frame_host/interstitial_page_impl.cc#newcode770 content/browser/frame_host/interstitial_page_impl.cc:770: << "InterstitialPage does not support showing full screen ...

4 years ago (2016-12-15 00:33:17 UTC) #41

ptal

https://codereview.chromium.org/2506183002/diff/100001/content/browser/frame_...
File content/browser/frame_host/interstitial_page_impl.cc (right):

https://codereview.chromium.org/2506183002/diff/100001/content/browser/frame_...
content/browser/frame_host/interstitial_page_impl.cc:770: << "InterstitialPage
does not support showing full screen popups.";
On 2016/12/12 19:17:05, Avi wrote:
> I would argue that interstitial pages never should be doing these things, so
> this should stay as NOTREACHED but still drop the "yet" from the descriptions.
> 
> Just me tho.

Done.

https://codereview.chromium.org/2506183002/diff/100001/content/browser/frame_...
File content/browser/frame_host/interstitial_page_impl.h (right):

https://codereview.chromium.org/2506183002/diff/100001/content/browser/frame_...
content/browser/frame_host/interstitial_page_impl.h:125: int route_id,
On 2016/12/13 18:41:40, alexmos wrote:
> Update the names here as well?

Done.

https://codereview.chromium.org/2506183002/diff/100001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_delegate.h (right):

https://codereview.chromium.org/2506183002/diff/100001/content/browser/frame_...
content/browser/frame_host/render_frame_host_delegate.h:45: class
CreateNewWindowParams;
On 2016/12/13 18:41:40, alexmos wrote:
> Is this one needed?  There's one defined below in content namespace already.

Done.

https://codereview.chromium.org/2506183002/diff/100001/content/browser/frame_...
content/browser/frame_host/render_frame_host_delegate.h:229: // should be
created associated with the given |route_id| in the process of
On 2016/12/13 18:41:40, alexmos wrote:
> Update |route_id|.

Done.

https://codereview.chromium.org/2506183002/diff/100001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2506183002/diff/100001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:71: #include
"content/common/view_messages.h"
On 2016/12/13 18:41:40, alexmos wrote:
> Why is this needed?

Removed. Good catch.

https://codereview.chromium.org/2506183002/diff/100001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:1029: // particularly
egregious, since an oopif isn't expected to know its top URL.
On 2016/12/13 18:41:40, alexmos wrote:
> This cleanup seems important.  Is it worth filing a bug so we don't forget to
> deal with it, or maybe we already have one?  (There's 466297 that I used for
> opener_top_level_frame_url, but it has a different focus.)
> 
> Also, we never do FilterURL on the opener_top_level_frame_url, do we?  (IIRC,
> it's used for content settings lookups on the IO thread, so don't know how
much
> of a problem that is.  But the IO thread work that precedes this is why it's
not
> going to be easy to just switch these to last_committed_url(), etc.)

I've filed a bug. The usage on the IO thread is what makes this tricky.

https://codereview.chromium.org/2506183002/diff/100001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:1034: if
(frame_tree_node()->current_frame_host() == this) {
On 2016/12/13 18:41:40, alexmos wrote:
> Can this be shortened to just is_active()?  (That covers pending delete, and
we
> can't ever get here from a pending RFH, can we?)

Good idea. Done.

https://codereview.chromium.org/2506183002/diff/100001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/2506183002/diff/100001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.h:234: //
RenderView/RenderFrames with the specified route ids, which were assigned
On 2016/12/13 18:41:40, alexmos wrote:
> nit: s/RenderFrames/RenderFrame/.  (just one RenderFrame, right?)

Done.

https://codereview.chromium.org/2506183002/diff/100001/content/browser/render...
File content/browser/renderer_host/render_view_host_delegate.h (right):

https://codereview.chromium.org/2506183002/diff/100001/content/browser/render...
content/browser/renderer_host/render_view_host_delegate.h:19: #include
"ui/base/window_open_disposition.h"
On 2016/12/13 18:41:40, alexmos wrote:
> Can you now remove this too?

Done.

https://codereview.chromium.org/2506183002/diff/100001/content/browser/render...
File content/browser/renderer_host/render_view_host_impl.h (right):

https://codereview.chromium.org/2506183002/diff/100001/content/browser/render...
content/browser/renderer_host/render_view_host_impl.h:35: #include
"ui/base/window_open_disposition.h"
On 2016/12/13 18:41:40, alexmos wrote:
> remove?

Done.

https://codereview.chromium.org/2506183002/diff/100001/content/browser/render...
File content/browser/renderer_host/render_widget_helper.cc (right):

https://codereview.chromium.org/2506183002/diff/100001/content/browser/render...
content/browser/renderer_host/render_widget_helper.cc:139: if (opener &&
opener->IsRenderFrameLive() &&
On 2016/12/13 18:41:40, alexmos wrote:
> I suppose it makes sense to check whether the frame died right before opening
a
> new window.  But I'm kind of curious how you arrived at putting it in?  Was
that
> something that happened in practice or in a test?

I didn't write a test for this, but I do think it's possible in practice.
OnMessageReceived ignores messages from dead RFH's, it seemed to make sense to
do that here too for the forwarded message.

Ignoring messages from dead RFH's is a general defense against the attack
scenario "start with two tabs sharing a RPH, trigger a deliberate crash, get the
user to reload one tab, then take over the renderer exploited; the exploited
process sends messages to the second tab's (still-dead) RFH, causing invariant
violations or UaFs that help you take over the browser process"

https://codereview.chromium.org/2506183002/diff/100001/content/browser/render...
content/browser/renderer_host/render_widget_helper.cc:140:
opener->frame_tree_node()->current_frame_host() == opener) {
On 2016/12/13 18:41:40, alexmos wrote:
> Similarly, can you use opener->is_active() here? This might also let you
remove
> the dependency on frame_tree_node.  Also, looks like you're also checking this
> in RFHI::OnCreateNewWindow, so do we really need both checks?

Done.

https://codereview.chromium.org/2506183002/diff/100001/content/browser/render...
content/browser/renderer_host/render_widget_helper.cc:150: if
(main_frame_route_id != MSG_ROUTING_NONE && rph != nullptr) {
On 2016/12/13 18:41:40, alexmos wrote:
> Why do we need to look up and null-check the rph, rather than using
> render_process_id_ directly below?

Looking up the rph is nececessary for the rph->Send() below.

The RPH could be null because nothing stops a MessageFilter from outliving the
RPH that nominally owns it. Again, it's an exceedingly rare case.

https://codereview.chromium.org/2506183002/diff/100001/content/browser/render...
content/browser/renderer_host/render_widget_helper.cc:164: }
On 2016/12/13 18:41:40, alexmos wrote:
> Is it worth adding an else and DCHECKing that render_view_routing_id is also
> MSG_ROUTING_NONE when main_frame_route_id is, and also mentioning how we get
> there?  IIUC, this happens when the intention is to open the new window in a
new
> BI (such as when suppressing the opener), and in that case the browser and not
> the renderer initiates creation of the corresponding RV/RF, so nothing to
clean
> up.

I added this DCHECK to the top of the WebContentsImpl function. For some reason
I thought it already existed.

https://codereview.chromium.org/2506183002/diff/100001/content/browser/site_p...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/2506183002/diff/100001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:7487: // Helper filter class to
wait for a ShowView or ShowWidget message, record the
On 2016/12/13 18:41:40, alexmos wrote:
> ShowView -> ShowCreatedWindow

Done.

https://codereview.chromium.org/2506183002/diff/100001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:7516: void
OnShowCreatedWindow(int pending_tab_main_frame_route_id,
On 2016/12/13 18:41:40, alexmos wrote:
> This is pending_widget_route_id, right?

Done.

https://codereview.chromium.org/2506183002/diff/100001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:7566: // Wait for and then drop
both ViewHostMsg_ShowView messages.  This will
On 2016/12/13 18:41:40, alexmos wrote:
> Update the IPC here

Done.

https://codereview.chromium.org/2506183002/diff/100001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:7594: // Now, simulate that both
ShowView messages arrive by showing both of the
On 2016/12/13 18:41:40, alexmos wrote:
> Ditto

Done.

https://codereview.chromium.org/2506183002/diff/100001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/2506183002/diff/100001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:2076: create_params.routing_id
= render_view_route_id;
On 2016/12/13 18:41:41, alexmos wrote:
> Something random and not for this CL, but it'd be nice if we used "route_id"
and
> "routing_id" consistently everywhere...

Acknowledged.

https://codereview.chromium.org/2506183002/diff/100001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:2320: if
(!new_contents->GetMainFrame()->GetProcess()->HasConnection() ||
On 2016/12/13 18:41:41, alexmos wrote:
> Thanks for cleaning these up!

Done.

https://codereview.chromium.org/2506183002/diff/100001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:2322: // TODO(nick): Who
deletes |new_contents|?
On 2016/12/13 18:41:41, alexmos wrote:
> Good point, I don't see who deletes it either.  So if the new window's RFH
> crashes before we get to show the new contents, we end up leaking it?

I filed a bug & left a breadcrumb here. The second case mentioned in the bug
(tab close) is probably actually more realistic.

https://codereview.chromium.org/2506183002/diff/100001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/2506183002/diff/100001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.h:496: int32_t route_id,
On 2016/12/13 18:41:41, alexmos wrote:
> render_view_route_id?

Done.

https://codereview.chromium.org/2506183002/diff/100001/content/common/view_me...
File content/common/view_messages.h (right):

https://codereview.chromium.org/2506183002/diff/100001/content/common/view_me...
content/common/view_messages.h:498: // Reply to ViewHostMsg_RequestMove, and
ViewHostMsg_ShowWidget to inform the
On 2016/12/13 18:41:41, alexmos wrote:
> Should we add FrameHostMsg_ShowCreatedWindow to this list, since we still send
> the ACK for it?

Done.

https://codereview.chromium.org/2506183002/diff/100001/content/common/view_me...
content/common/view_messages.h:630: // that was created by
CreateWidget/CreateFullscreenWidget. |routing_id| refers
On 2016/12/13 18:41:41, alexmos wrote:
> (while you're here) nit: route_id, to match the arg name in /* */ below. 

Done.

https://codereview.chromium.org/2506183002/diff/100001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2506183002/diff/100001/content/renderer/rende...
content/renderer/render_frame_impl.cc:6673:
RenderViewImpl::NavigationPolicyToDisposition(policy), initial_rect,
On 2016/12/13 18:41:41, alexmos wrote:
> Would it make sense to also move NavigationPolicyToDisposition to
> RenderFrameImpl?  (No need to do it in this CL.)  I guess there's still one
use
> left in RV vs. two in RF.

I actually looked into this before you even mentioned it (we are very much on
the same page!)

TL;DR is that, so long as there are still callers in RenderViewImpl, that's a
better place for it. The existing friend declarations only allow calling private
members in one direction.

https://codereview.chromium.org/2506183002/diff/100001/content/renderer/rende...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/2506183002/diff/100001/content/renderer/rende...
content/renderer/render_view_impl.cc:1498: // show().
On 2016/12/13 18:41:41, alexmos wrote:
> Ack.  Earlier in the CL I was thinking whether we could now get a race where
the
> main frame creates a new window and swaps out to a proxy before the new window
> is shown, but that doesn't seem possible, for this reason. 
> blink::createNewWindow will always call show() synchronously after calling
> createWindow, and it doesn't look like anything in between can destroy the
> creator frame.

SYN/ACK. I actually spent a lot of time worrying about the lifetime here before
I realized how simple it actually was. I'd thought that the message loop might
run between create and show().

I'm now confident that we could probably get rid of the show() step altogether
for all widgets; but I'm not intending on trying myself.

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2506183002/140001

4 years ago (2016-12-15 00:33:48 UTC) #42

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years ago (2016-12-15 00:42:20 UTC) #43

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/139035)

4 years ago (2016-12-15 00:42:20 UTC) #44

ncarter (slow)

Note: I see the AttemptDuplicateRenderViewHost test failure, and am looking into it. It actually looks ...

4 years ago (2016-12-15 17:47:53 UTC) #45

alexmos

LGTM with one comment about removing frame_tree_node.h from render_widget_helper. I wonder if the test failure ...

4 years ago (2016-12-15 18:36:48 UTC) #46

LGTM with one comment about removing frame_tree_node.h from
render_widget_helper.

I wonder if the test failure is due to the is_active() change - seems to be the
only behavior change in this PS, and the test was passing previously.  I'm
curious to hear what you find.

https://codereview.chromium.org/2506183002/diff/100001/content/browser/render...
File content/browser/renderer_host/render_widget_helper.cc (right):

https://codereview.chromium.org/2506183002/diff/100001/content/browser/render...
content/browser/renderer_host/render_widget_helper.cc:139: if (opener &&
opener->IsRenderFrameLive() &&
On 2016/12/15 00:33:16, ncarter wrote:
> On 2016/12/13 18:41:40, alexmos wrote:
> > I suppose it makes sense to check whether the frame died right before
opening
> a
> > new window.  But I'm kind of curious how you arrived at putting it in?  Was
> that
> > something that happened in practice or in a test?
> 
> I didn't write a test for this, but I do think it's possible in practice.
> OnMessageReceived ignores messages from dead RFH's, it seemed to make sense to
> do that here too for the forwarded message.
> 
> Ignoring messages from dead RFH's is a general defense against the attack
> scenario "start with two tabs sharing a RPH, trigger a deliberate crash, get
the
> user to reload one tab, then take over the renderer exploited; the exploited
> process sends messages to the second tab's (still-dead) RFH, causing invariant
> violations or UaFs that help you take over the browser process"

Acknowledged.

https://codereview.chromium.org/2506183002/diff/100001/content/browser/render...
content/browser/renderer_host/render_widget_helper.cc:150: if
(main_frame_route_id != MSG_ROUTING_NONE && rph != nullptr) {
On 2016/12/15 00:33:16, ncarter wrote:
> On 2016/12/13 18:41:40, alexmos wrote:
> > Why do we need to look up and null-check the rph, rather than using
> > render_process_id_ directly below?
> 
> Looking up the rph is nececessary for the rph->Send() below.
> 
> The RPH could be null because nothing stops a MessageFilter from outliving the
> RPH that nominally owns it. Again, it's an exceedingly rare case.

Acknowledged.

https://codereview.chromium.org/2506183002/diff/100001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/2506183002/diff/100001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:2322: // TODO(nick): Who
deletes |new_contents|?
On 2016/12/15 00:33:16, ncarter wrote:
> On 2016/12/13 18:41:41, alexmos wrote:
> > Good point, I don't see who deletes it either.  So if the new window's RFH
> > crashes before we get to show the new contents, we end up leaking it?
> 
> I filed a bug & left a breadcrumb here. The second case mentioned in the bug
> (tab close) is probably actually more realistic.

Acknowledged.  Good find with the close case.

https://codereview.chromium.org/2506183002/diff/100001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2506183002/diff/100001/content/renderer/rende...
content/renderer/render_frame_impl.cc:6673:
RenderViewImpl::NavigationPolicyToDisposition(policy), initial_rect,
On 2016/12/15 00:33:17, ncarter wrote:
> On 2016/12/13 18:41:41, alexmos wrote:
> > Would it make sense to also move NavigationPolicyToDisposition to
> > RenderFrameImpl?  (No need to do it in this CL.)  I guess there's still one
> use
> > left in RV vs. two in RF.
> 
> I actually looked into this before you even mentioned it (we are very much on
> the same page!)
> 
> TL;DR is that, so long as there are still callers in RenderViewImpl, that's a
> better place for it. The existing friend declarations only allow calling
private
> members in one direction.

Acknowledged.

https://codereview.chromium.org/2506183002/diff/140001/content/browser/render...
File content/browser/renderer_host/DEPS (right):

https://codereview.chromium.org/2506183002/diff/140001/content/browser/render...
content/browser/renderer_host/DEPS:42:
"+content/browser/frame_host/frame_tree_node.h",
This isn't needed anymore, right?

https://codereview.chromium.org/2506183002/diff/140001/content/browser/render...
File content/browser/renderer_host/render_widget_helper.cc (right):

https://codereview.chromium.org/2506183002/diff/140001/content/browser/render...
content/browser/renderer_host/render_widget_helper.cc:14: #include
"content/browser/frame_host/frame_tree_node.h"
Same here.

ncarter (slow)

The CQ bit was checked by nick@chromium.org to run a CQ dry run

4 years ago (2016-12-15 21:36:25 UTC) #47

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2506183002/160001

4 years ago (2016-12-15 21:37:02 UTC) #48

ncarter (slow)

nick@chromium.org changed reviewers: + alokp@chromium.org, dominickn@chromium.org, sgurun@chromium.org

4 years ago (2016-12-15 21:45:06 UTC) #51

ncarter (slow)

sgurun@chromium.org: Please review changes in android_webview/ alokp@chromium.org: Please review changes in chromecast/ dominickn@chromium.org: Please review ...

4 years ago (2016-12-15 21:45:07 UTC) #52

ncarter (slow)

https://codereview.chromium.org/2506183002/diff/140001/content/browser/renderer_host/DEPS File content/browser/renderer_host/DEPS (right): https://codereview.chromium.org/2506183002/diff/140001/content/browser/renderer_host/DEPS#newcode42 content/browser/renderer_host/DEPS:42: "+content/browser/frame_host/frame_tree_node.h", On 2016/12/15 18:36:48, alexmos wrote: > This isn't ...

4 years ago (2016-12-15 22:22:17 UTC) #53

dominickn

chrome/browser/android/webapps lgtm. +cc dfalcantara FYI (original author of single_tab_mode_tab_helper)

4 years ago (2016-12-15 23:18:33 UTC) #54

nasko

IPC LGTM https://codereview.chromium.org/2506183002/diff/180001/content/browser/web_contents/web_contents_impl.cc File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2506183002/diff/180001/content/browser/web_contents/web_contents_impl.cc#newcode2235 content/browser/web_contents/web_contents_impl.cc:2235: user_gesture, NULL); nit: If you are touching ...

4 years ago (2016-12-15 23:30:17 UTC) #55

sgurun-gerrit only

On 2016/12/15 23:30:17, nasko wrote: > IPC LGTM > > https://codereview.chromium.org/2506183002/diff/180001/content/browser/web_contents/web_contents_impl.cc > File content/browser/web_contents/web_contents_impl.cc (right): ...

4 years ago (2016-12-17 02:48:35 UTC) #56

ncarter (slow)

Description was changed from ========== Make window.open() IPCs be frame-based. - Two IPCs, CreateNewWindow and ...

4 years ago (2016-12-19 23:25:23 UTC) #57

Description was changed from

==========
Make window.open() IPCs be frame-based.

 - Two IPCs, CreateNewWindow and ShowCreatedWindow, are made to be routed via
   the opener frame. Previously they were routed via the opener view.
 - CreateNewWindow (which is mojofied) stays put but its parameters change
 - ViewHostMsg_ShowView becomes FrameHostMsg_ShowCreatedWindow
 - The corresponding delegate methods move from RenderViewHostDelegate to
   RenderFrameHostDelegate, though the implementation stays in
   WebContentsImpl, which implements both interfaces.
 - Remove the RenderView opener_id parameter from the mojo CreateNewWindow
   params, it can be inferred from the RenderFrame id. This fixes a possible
   bad situation where an unrelated RV/RF route id pair could be supplied.
 - The pending WebContents list is made to be indexed by the main frame
   widget; this turns out to be cleaner than making it frame- based, since
   show() is an operation on the created RenderWidget.
 - The signature of ContentBrowserClient::CanCreateWindow changes to be
   frame-based, which necessitates retooling the android
   SingleTabModeTabHelper to be frame-based also.
 - The ViewMsg_Close message, which cleans up after UI-thread failure of the
   CreateNewWindow operation, is hoisted out to the outermost function where
   failure is possible, RenderWidgetHelper::OnCreateNewWindowOnUI. This
   addresses some possible RenderView leaks when e.g. window.open() races a
   navigation that destroys the opener. These races would have become more
   likely now that we're routing via the frame, since child frames can have a
   shorter lifetime than their RenderViewHosts.
 - The response to ShowCreatedWindow on the opener RenderFrame is a
   ViewMsg_Move_ACK on the openee RenderWidget. This IPC moves to
   WebContentsImpl::ShowCreatedWindow so that it can be sent via the openee
   RenderWidgetHost's IPC::Sender, rather than via the opener RenderFrame.

BUG=431769, 304341
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Make window.open() IPCs be frame-based.

 - Two IPCs, CreateNewWindow and ShowCreatedWindow, are made to be routed via
   the opener frame. Previously they were routed via the opener view.
 - CreateNewWindow (which is mojofied) stays put but its parameters change
 - ViewHostMsg_ShowView becomes FrameHostMsg_ShowCreatedWindow
 - The corresponding delegate methods move from RenderViewHostDelegate to
   RenderFrameHostDelegate, though the implementation stays in
   WebContentsImpl, which implements both interfaces.
 - Remove the RenderView opener_id parameter from the mojo CreateNewWindow
   params, it can be inferred from the RenderFrame id. This fixes a possible
   bad situation where an unrelated RV/RF route id pair could be supplied.
 - The pending WebContents list is made to be indexed by the main frame
   widget; this turns out to be cleaner than making it frame- based, since
   show() is an operation on the created RenderWidget.
 - The signature of ContentBrowserClient::CanCreateWindow changes to be
   frame-based, which necessitates retooling the android
   SingleTabModeTabHelper to be frame-based also.
 - The ViewMsg_Close message, which cleans up after UI-thread failure of the
   CreateNewWindow operation, is hoisted out to the outermost function where
   failure is possible, RenderWidgetHelper::OnCreateNewWindowOnUI. This
   addresses some possible RenderView leaks when e.g. window.open() races a
   navigation that destroys the opener. These races would have become more
   likely now that we're routing via the frame, since child frames can have a
   shorter lifetime than their RenderViewHosts.
 - The response to ShowCreatedWindow on the opener RenderFrame is a
   ViewMsg_Move_ACK on the openee RenderWidget. This IPC moves to
   WebContentsImpl::ShowCreatedWindow so that it can be sent via the openee
   RenderWidgetHost's IPC::Sender, rather than via the opener RenderFrame.

BUG=431769, 304341
TBR=alokp@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

ncarter (slow)

The patchset sent to the CQ was uploaded after l-g-t-m from alexmos@chromium.org Link to the ...

4 years ago (2016-12-19 23:25:30 UTC) #59

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2506183002/180001

4 years ago (2016-12-19 23:26:15 UTC) #60

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years ago (2016-12-20 00:46:50 UTC) #61

commit-bot: I haz the power

Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_swarming_rel/builds/89178)

4 years ago (2016-12-20 00:46:52 UTC) #62

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2506183002/180001

3 years, 11 months ago (2017-01-05 18:06:25 UTC) #64

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 11 months ago (2017-01-05 18:09:11 UTC) #65

commit-bot: I haz the power

Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds/130738) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, ...

3 years, 11 months ago (2017-01-05 18:09:13 UTC) #66

ncarter (slow)

The CQ bit was checked by nick@chromium.org to run a CQ dry run

3 years, 11 months ago (2017-01-05 18:45:30 UTC) #67

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2506183002/200001

3 years, 11 months ago (2017-01-05 18:45:52 UTC) #68

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 11 months ago (2017-01-05 20:24:28 UTC) #69

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 11 months ago (2017-01-05 20:24:29 UTC) #70

ncarter (slow)

The patchset sent to the CQ was uploaded after l-g-t-m from dominickn@chromium.org, sgurun@chromium.org, alexmos@chromium.org, nasko@chromium.org ...

3 years, 11 months ago (2017-01-05 23:03:36 UTC) #72

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2506183002/200001

3 years, 11 months ago (2017-01-05 23:04:03 UTC) #73

commit-bot: I haz the power

CQ is committing da patch. Bot data: {"patchset_id": 200001, "attempt_start_ts": 1483657415763890, "parent_rev": "d1d21f7fcc326b5875fbe7d4ff95376c8fcddbc3", "commit_rev": "5ae4d2d504ad44943b99835f1eb7cba724318a97"}

3 years, 11 months ago (2017-01-06 01:18:39 UTC) #75

commit-bot: I haz the power

Description was changed from ========== Make window.open() IPCs be frame-based. - Two IPCs, CreateNewWindow and ...

3 years, 11 months ago (2017-01-06 01:19:12 UTC) #76

Message was sent while issue was closed.

Description was changed from

==========
Make window.open() IPCs be frame-based.

 - Two IPCs, CreateNewWindow and ShowCreatedWindow, are made to be routed via
   the opener frame. Previously they were routed via the opener view.
 - CreateNewWindow (which is mojofied) stays put but its parameters change
 - ViewHostMsg_ShowView becomes FrameHostMsg_ShowCreatedWindow
 - The corresponding delegate methods move from RenderViewHostDelegate to
   RenderFrameHostDelegate, though the implementation stays in
   WebContentsImpl, which implements both interfaces.
 - Remove the RenderView opener_id parameter from the mojo CreateNewWindow
   params, it can be inferred from the RenderFrame id. This fixes a possible
   bad situation where an unrelated RV/RF route id pair could be supplied.
 - The pending WebContents list is made to be indexed by the main frame
   widget; this turns out to be cleaner than making it frame- based, since
   show() is an operation on the created RenderWidget.
 - The signature of ContentBrowserClient::CanCreateWindow changes to be
   frame-based, which necessitates retooling the android
   SingleTabModeTabHelper to be frame-based also.
 - The ViewMsg_Close message, which cleans up after UI-thread failure of the
   CreateNewWindow operation, is hoisted out to the outermost function where
   failure is possible, RenderWidgetHelper::OnCreateNewWindowOnUI. This
   addresses some possible RenderView leaks when e.g. window.open() races a
   navigation that destroys the opener. These races would have become more
   likely now that we're routing via the frame, since child frames can have a
   shorter lifetime than their RenderViewHosts.
 - The response to ShowCreatedWindow on the opener RenderFrame is a
   ViewMsg_Move_ACK on the openee RenderWidget. This IPC moves to
   WebContentsImpl::ShowCreatedWindow so that it can be sent via the openee
   RenderWidgetHost's IPC::Sender, rather than via the opener RenderFrame.

BUG=431769, 304341
TBR=alokp@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Make window.open() IPCs be frame-based.

 - Two IPCs, CreateNewWindow and ShowCreatedWindow, are made to be routed via
   the opener frame. Previously they were routed via the opener view.
 - CreateNewWindow (which is mojofied) stays put but its parameters change
 - ViewHostMsg_ShowView becomes FrameHostMsg_ShowCreatedWindow
 - The corresponding delegate methods move from RenderViewHostDelegate to
   RenderFrameHostDelegate, though the implementation stays in
   WebContentsImpl, which implements both interfaces.
 - Remove the RenderView opener_id parameter from the mojo CreateNewWindow
   params, it can be inferred from the RenderFrame id. This fixes a possible
   bad situation where an unrelated RV/RF route id pair could be supplied.
 - The pending WebContents list is made to be indexed by the main frame
   widget; this turns out to be cleaner than making it frame- based, since
   show() is an operation on the created RenderWidget.
 - The signature of ContentBrowserClient::CanCreateWindow changes to be
   frame-based, which necessitates retooling the android
   SingleTabModeTabHelper to be frame-based also.
 - The ViewMsg_Close message, which cleans up after UI-thread failure of the
   CreateNewWindow operation, is hoisted out to the outermost function where
   failure is possible, RenderWidgetHelper::OnCreateNewWindowOnUI. This
   addresses some possible RenderView leaks when e.g. window.open() races a
   navigation that destroys the opener. These races would have become more
   likely now that we're routing via the frame, since child frames can have a
   shorter lifetime than their RenderViewHosts.
 - The response to ShowCreatedWindow on the opener RenderFrame is a
   ViewMsg_Move_ACK on the openee RenderWidget. This IPC moves to
   WebContentsImpl::ShowCreatedWindow so that it can be sent via the openee
   RenderWidgetHost's IPC::Sender, rather than via the opener RenderFrame.

BUG=431769, 304341
TBR=alokp@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2506183002
Cr-Commit-Position: refs/heads/master@{#441781}
Committed:
https://chromium.googlesource.com/chromium/src/+/5ae4d2d504ad44943b99835f1eb7...
==========

commit-bot: I haz the power

3 years, 11 months ago (2017-01-06 01:19:14 UTC) #77

Message was sent while issue was closed.

Committed patchset #11 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/5ae4d2d504ad44943b99835f1eb7...

Issue 2506183002: Make window.open() IPCs be frame-based (Closed)

Description

Patch Set 1 #

Patch Set 2 : y #

Patch Set 3 : Cleanup. #

Patch Set 4 : Cleanup. #

Patch Set 5 : Fixes #

Patch Set 6 : Compile fix. #

Patch Set 7 : Alex's fixes. #

Patch Set 8 : Few more fixes #

Patch Set 9 : end #

Patch Set 10 : Remove frame_tree_node include & DEPS #

Patch Set 11 : Rebase. #

Messages