Remove CallWith=ScriptState from window/self/frames getters on Window.

Remove CallWith=ScriptState from window/self/frames getters on Window.

Remote contexts won't have an associated ScriptState, so this needs to
be plumbed through using a mechanism that doesn't depend on ScriptState.

BUG=527190

[dcheng, 2017-01-18 06:58:49]

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:59:09]

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-18 07:01:59]

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

[dcheng, 2017-01-18 07:02:00]

This is definitely a hack, but it's needed to unblock the remote context CL to
land for M57.

https://codereview.chromium.org/2630693002/ is the direction I want to take this
code eventually: it changes ToV8 to work with any detached DOMWindow. However,
this introduces a number of behavior changes (e.g. for things like
Document.defaultView). Thus, I've opted to try to land the behavior-changing CL
separately.

https://codereview.chromium.org/2642643002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.h (right):

https://codereview.chromium.org/2642643002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.h:38: friend
v8::Local<v8::Value> ToV8(WindowProxyManagerBase*,
Since this is a temporary hack, I've opted to just expose windowProxy() here.
Eventually, I want to take this in the direction of
https://codereview.chromium.org/2630693002/, which will remove this.

https://codereview.chromium.org/2642643002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.h:76: FrameType*
frame() const { return static_cast<FrameType*>(Base::frame()); }
To avoid DOMWindow from getting frame() from its window proxy manager.

[Yuki, 2017-01-18 07:31:43]

I'd prefer (partially) reverting my CL (https://crrev.com/2501333002) to landing
this hacky CL if it works for remote frames.  I'll come up with another way
without using ScriptState later.

What do you think?
  self() {
    if (!frame())
      return nullptr;
    return frame()->domWindow();
  }
Does this work for you?

https://codereview.chromium.org/2642643002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/ToV8.cpp (right):

https://codereview.chromium.org/2642643002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/ToV8.cpp:50: ->globalIfNotDetached();
I think globalIfNotDetached() is not equivalent to
scriptState->context()->Global() because globalIfNotDetached() returns an empty
handle for some cases.

[dcheng, 2017-01-18 07:48:14]

On 2017/01/18 07:31:43, Yuki wrote:
> I'd prefer (partially) reverting my CL (https://crrev.com/2501333002) to
landing
> this hacky CL if it works for remote frames.  I'll come up with another way
> without using ScriptState later.

I wasn't sure if this would be OK. But if this is considered an OK alternative,
I would be OK with that. By partial revert, do you mean reverting just the
non-test changes?

> 
> What do you think?
>   self() {
>     if (!frame())
>       return nullptr;
>     return frame()->domWindow();
>   }
> Does this work for you?

We could do that. But we can just return |this| directly, as ToV8 does this
check already.

> 
>
https://codereview.chromium.org/2642643002/diff/1/third_party/WebKit/Source/b...
> File third_party/WebKit/Source/bindings/core/v8/ToV8.cpp (right):
> 
>
https://codereview.chromium.org/2642643002/diff/1/third_party/WebKit/Source/b...
> third_party/WebKit/Source/bindings/core/v8/ToV8.cpp:50:
->globalIfNotDetached();
> I think globalIfNotDetached() is not equivalent to
> scriptState->context()->Global() because globalIfNotDetached() returns an
empty
> handle for some cases.

It's not completely equivalent, as it turns out, but for slightly different
reasons. Since windowProxy() always initializes, there will always be a global
proxy object--it won't return an empty handle. However, there are a few test
expectations that start failing, which is what made me nervous about landing the
other CL. But since this CL has similar issues... it's probaby easiest to
investigate and figure out the right way to address in a followup.

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

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

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

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...)

[Yuki, 2017-01-18 08:08:27]

On 2017/01/18 07:48:14, dcheng wrote:
> On 2017/01/18 07:31:43, Yuki wrote:
> > I'd prefer (partially) reverting my CL (https://crrev.com/2501333002) to
> landing
> > this hacky CL if it works for remote frames.  I'll come up with another way
> > without using ScriptState later.
> 
> I wasn't sure if this would be OK. But if this is considered an OK
alternative,
> I would be OK with that. By partial revert, do you mean reverting just the
> non-test changes?

Yes, I meant non-test changes (or least changes).  If we can keep some
improvements in testing, that's better.

My original change was a kind of silent changes, and I assume there shouldn't be
a big client or many clients on this (since it's been broken for a long time). 
So I think a partial revert wouldn't cause a big break.


>
https://codereview.chromium.org/2642643002/diff/1/third_party/WebKit/Source/b...
> > third_party/WebKit/Source/bindings/core/v8/ToV8.cpp:50:
> ->globalIfNotDetached();
> > I think globalIfNotDetached() is not equivalent to
> > scriptState->context()->Global() because globalIfNotDetached() returns an
> empty
> > handle for some cases.
> 
> It's not completely equivalent, as it turns out, but for slightly different
> reasons. Since windowProxy() always initializes, there will always be a global
> proxy object--it won't return an empty handle. However, there are a few test
> expectations that start failing, which is what made me nervous about landing
the
> other CL. But since this CL has similar issues... it's probaby easiest to
> investigate and figure out the right way to address in a followup.

If the page navigated away, then it's considered as detached and it returns the
empty handle, right?  scriptState->context()->Global() returns the global proxy
object even after the page navigated away.

[dcheng, 2017-01-18 08:26:50]

On 2017/01/18 08:08:27, Yuki wrote:
> On 2017/01/18 07:48:14, dcheng wrote:
> > On 2017/01/18 07:31:43, Yuki wrote:
> > > I'd prefer (partially) reverting my CL (https://crrev.com/2501333002) to
> > landing
> > > this hacky CL if it works for remote frames.  I'll come up with another
way
> > > without using ScriptState later.
> > 
> > I wasn't sure if this would be OK. But if this is considered an OK
> alternative,
> > I would be OK with that. By partial revert, do you mean reverting just the
> > non-test changes?
> 
> Yes, I meant non-test changes (or least changes).  If we can keep some
> improvements in testing, that's better.
> 
> My original change was a kind of silent changes, and I assume there shouldn't
be
> a big client or many clients on this (since it's been broken for a long time).

> So I think a partial revert wouldn't cause a big break.
> 
> 
> >
>
https://codereview.chromium.org/2642643002/diff/1/third_party/WebKit/Source/b...
> > > third_party/WebKit/Source/bindings/core/v8/ToV8.cpp:50:
> > ->globalIfNotDetached();
> > > I think globalIfNotDetached() is not equivalent to
> > > scriptState->context()->Global() because globalIfNotDetached() returns an
> > empty
> > > handle for some cases.
> > 
> > It's not completely equivalent, as it turns out, but for slightly different
> > reasons. Since windowProxy() always initializes, there will always be a
global
> > proxy object--it won't return an empty handle. However, there are a few test
> > expectations that start failing, which is what made me nervous about landing
> the
> > other CL. But since this CL has similar issues... it's probaby easiest to
> > investigate and figure out the right way to address in a followup.
> 
> If the page navigated away, then it's considered as detached and it returns
the
> empty handle, right?  scriptState->context()->Global() returns the global
proxy
> object even after the page navigated away.

With PS1 of this CL, I actually expected more tests to fail (like
property-access-in-closure-after-navigation-child.html, because
*LocalWindowProxyManager::windowProxy* should not initialize the WindowProxy)
but for some reason they are still passing. Very mysterious...