Make ScriptController inherit LocalWindowProxyManager

Make ScriptController inherit LocalWindowProxyManager

This CL moves closer to storing a WindowProxyManager on
DOMWindow, so the bindings can map any DOMWindow*, even
when detached, to its WindowProxy.

Currently, WindowProxyManager exposes a problematic frame()
getter. In the past, there have been security bugs when a
navigated-away-from DOMWindow incorrectly used frame(),
assuming it was still active. This can lead to potential
UXSS attacks, so DOMWindow::frame() is now cleared on
navigation. Exposing a Frame getter from WindowProxyManager
could cause similar problems, so this proactive refactoring
aims to fix that.

Some additional benefits of this patch are the removal of
friend declarations, removal of some forwarding-only logic,
and simplifying the WindowProxyManager interface so that
windowProxy() always returns an initialized WindowProxy.

BUG=none

[dcheng, 2017-01-13 05:45:54]

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

[commit-bot: I haz the power, 2017-01-13 05:46:13]

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

[commit-bot: I haz the power, 2017-01-13 06:28:54]

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

[commit-bot: I haz the power, 2017-01-13 06:28:55]

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

[dcheng, 2017-01-13 10:08:19]

Description was changed from

==========
Make ScriptController inherit LocalWindowProxyManager

BUG=none
==========

to

==========
Make ScriptController inherit LocalWindowProxyManager

This CL moves closer to storing a WindowProxyManager on
DOMWindow, so the bindings can map any DOMWindow*, even
when detached, to its WindowProxy.

Currently, WindowProxyManager exposes a problematic frame()
getter. In the past, there have been security bugs when a
navigated-away-from DOMWindow incorrectly used frame(),
assuming it was still active. This can lead to potential
UXSS attacks, so DOMWindow::frame() is now cleared on
navigation. Exposing a Frame getter from WindowProxyManager
could cause similar problems, so this proactive refactoring
aims to fix that.

Some additional benefits of this patch are the removal of
friend declarations, removal of some forwarding-only logic,
and simplifying the WindowProxyManager interface so that
windowProxy() always returns an initialized WindowProxy.

BUG=none
==========

[dcheng, 2017-01-13 10:08:19]

dcheng@chromium.org changed reviewers:
+ haraken@chromium.org, yukishiino@chromium.org

[dcheng, 2017-01-13 10:08:22]

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

[commit-bot: I haz the power, 2017-01-13 10:08:39]

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

[Yuki, 2017-01-13 10:42:00]

LGTM.

https://codereview.chromium.org/2630693002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp (right):

https://codereview.chromium.org/2630693002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp:92:
static_cast<LocalWindowProxy*>(mainWorldProxy())
Did you make WindowProxyManagerImplHelper::mainWorldProxy() return a ProxyType,
didn't you?

Probably, you may need
  using WindowProxyManagerImplHelper<F, P>::mainWorldProxy;
in LocalWindowProxyManager.

https://codereview.chromium.org/2630693002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp:286: void
ScriptController::clearForNavigation() {
nit: Could you put clearForClose and clearForNavigation close to each other?

https://codereview.chromium.org/2630693002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp:289:
WindowProxyManagerBase::clearForNavigation();
Given LocalWindowProxyManager is a direct base class, this should be
LocalWindowProxyManager::clearForNavigation(), I think.

https://codereview.chromium.org/2630693002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ScriptController.h (right):

https://codereview.chromium.org/2630693002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptController.h:63: class
CORE_EXPORT ScriptController final : public LocalWindowProxyManager {
If possible, could you elaborate what ScriptController's responsibilities are
and what (Local)WindowProxyManager's responsibilities are?

https://codereview.chromium.org/2630693002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.h (right):

https://codereview.chromium.org/2630693002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.h:60: // Note that
this may return an uninitialized WindowProxy.
This is a very good comment, but could you write this in
WindowProxyManagerBase(, too)?

https://codereview.chromium.org/2630693002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.h:65: // is
guaranteed to be initialized.
Ditto.

[commit-bot: I haz the power, 2017-01-13 10:57:24]

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

[commit-bot: I haz the power, 2017-01-13 10:57:24]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[haraken, 2017-01-13 11:35:35]

> This CL moves closer to storing a WindowProxyManager on DOMWindow

What are you planning in the end?

Conceptually it seems a bit strange to me to make ScriptController inherit from
WindowProxyManager. ScriptController is a grandfarther to control all
script-related things, including WindowProxy. In that sense, it would make sense
to make ScriptController own WindowProxyManager.

[dcheng, 2017-01-13 18:34:05]

On 2017/01/13 11:35:35, haraken wrote:
> > This CL moves closer to storing a WindowProxyManager on DOMWindow
> 
> What are you planning in the end?
> 
> Conceptually it seems a bit strange to me to make ScriptController inherit
from
> WindowProxyManager. ScriptController is a grandfarther to control all
> script-related things, including WindowProxy. In that sense, it would make
sense
> to make ScriptController own WindowProxyManager.

Originally, this logic was embedded in ScriptController directly. I pulled it
out into a separate class when I originally refactored WindowProxy to handle
both local and remote frames.
However, after refactoring this code, LocalWindowProxyManager basically has
nothing left. Many functions in ScriptController just forward to
LocalWindowProxyManager. We also have an awkward split of functionality:
LocalWindowProxyManager::updateSecurityOrigin() is basically a ScriptController
function, but needs to be on LocalWindowProxyManager to access protected
members.

Finally, for the followup CL, I plan to have a WindowProxyManagerBase directly
on DOMWindow. For that, it is problematic to expose the frame() getter for the
reason mentioned in the CL description. In order to expose only to
ScriptController, I would have to put using Base::frame in the protected/private
section of LocalWindowProxyManager to forward the frame() getter to
ScriptController.

For these reasons, it seems best to just inheritance here: all the code related
to ScriptController is in one place and there's fewer friend declarations that
expose class internals. WDYT?

[dcheng, 2017-01-13 18:57:29]

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

[dcheng, 2017-01-13 18:57:31]

https://codereview.chromium.org/2630693002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp (right):

https://codereview.chromium.org/2630693002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp:92:
static_cast<LocalWindowProxy*>(mainWorldProxy())
On 2017/01/13 10:41:59, Yuki wrote:
> Did you make WindowProxyManagerImplHelper::mainWorldProxy() return a
ProxyType,
> didn't you?
> 
> Probably, you may need
>   using WindowProxyManagerImplHelper<F, P>::mainWorldProxy;
> in LocalWindowProxyManager.

Originally, I wrote it this way for consistency originally (since the loop still
needs to downcast), but I guess it's fine to use mainWorldProxy() here.

https://codereview.chromium.org/2630693002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp:286: void
ScriptController::clearForNavigation() {
On 2017/01/13 10:42:00, Yuki wrote:
> nit: Could you put clearForClose and clearForNavigation close to each other?

Done.

https://codereview.chromium.org/2630693002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp:289:
WindowProxyManagerBase::clearForNavigation();
On 2017/01/13 10:42:00, Yuki wrote:
> Given LocalWindowProxyManager is a direct base class, this should be
> LocalWindowProxyManager::clearForNavigation(), I think.

Done.

https://codereview.chromium.org/2630693002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ScriptController.h (right):

https://codereview.chromium.org/2630693002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptController.h:63: class
CORE_EXPORT ScriptController final : public LocalWindowProxyManager {
On 2017/01/13 10:42:00, Yuki wrote:
> If possible, could you elaborate what ScriptController's responsibilities are
> and what (Local)WindowProxyManager's responsibilities are?

Done.

https://codereview.chromium.org/2630693002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.cpp (right):

https://codereview.chromium.org/2630693002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.cpp:49:
windowProxy(entry.value->world())->releaseGlobal());
I missed this in my original patch, but a few tests broke. I simplified this to
just use the map entries directly.

https://codereview.chromium.org/2630693002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.h (right):

https://codereview.chromium.org/2630693002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.h:60: // Note that
this may return an uninitialized WindowProxy.
On 2017/01/13 10:42:00, Yuki wrote:
> This is a very good comment, but could you write this in
> WindowProxyManagerBase(, too)?

Done.

https://codereview.chromium.org/2630693002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.h:65: // is
guaranteed to be initialized.
On 2017/01/13 10:42:00, Yuki wrote:
> Ditto.

Done.

[commit-bot: I haz the power, 2017-01-13 18:58:07]

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

[dcheng, 2017-01-13 19:05:47]

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

[commit-bot: I haz the power, 2017-01-13 19:06:38]

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

[commit-bot: I haz the power, 2017-01-13 20:21:54]

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

[commit-bot: I haz the power, 2017-01-13 20:21:55]

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

[dcheng, 2017-01-13 22:41:29]

Actually, looking over things again, I think there's a couple problematic things
about this CL. It was confusing that windowProxy() would sometimes initialize
before and sometimes not, but it is not right to always initialize either.

I am going to restructure this code a little bit to make it easier to understand
/ more predictable and hide more details of WindowProxy initialization.

[dcheng, 2017-01-18 06:08:52]

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

[commit-bot: I haz the power, 2017-01-18 06:09:15]

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

[commit-bot: I haz the power, 2017-01-18 06:11:17]

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

[commit-bot: I haz the power, 2017-01-18 06:11:18]

Dry run: 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...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)