binding: Disallows to run a function if its context is detached.

binding: Disallows to run a function if its context is detached.

There are two contexts when about to run a ScheduledAction.
|this|'s context and the context of the function to be run.
If the latter context no longer has a valid V8PerContextData,
|self| returns null.
    function() { self.testRunner; } // self will be null

This issue has been there for a long time, but
https://crrev.com/2049493005 made it easy to happen.

This CL disallows to run a function if the function's context is
detached.  This is not the right fix, but close to the behavior
before https://crrev.com/2049493005 .

The right fix is that, per spec, |self| always returns the global
proxy object that never be null.  I.e. we need to support detached
windows.

BUG=630996
Committed: https://crrev.com/981ce039703c0350086568b8c922bee8b8ca2761
Cr-Commit-Position: refs/heads/master@{#408376}

[Yuki, 2016-07-27 15:13:39]

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

[commit-bot: I haz the power, 2016-07-27 15:13:59]

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

[Yuki, 2016-07-27 15:19:44]

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

[Yuki, 2016-07-27 15:19:44]

Could you review this CL?

I'm afraid of a performance regression, but we need an extra check...

[haraken, 2016-07-27 15:28:43]

I don't think ScheduledAction is performance-sensitive (it's already doing a lot
of things), so this change would be fine from the performance perspective.

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

https://codereview.chromium.org/2191543002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/ScheduledAction.cpp:108: if
(!m_scriptState->contextIsValid()) {

I was thinking that this check was doing what you want to do with this CL.

Maybe the real problem is that we're using a wrong ScriptState here?

[commit-bot: I haz the power, 2016-07-27 16:20:11]

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

[commit-bot: I haz the power, 2016-07-27 16:20:12]

Dry run: This issue passed the CQ dry run.

[Yuki, 2016-07-28 06:12:35]

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

https://codereview.chromium.org/2191543002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/ScheduledAction.cpp:108: if
(!m_scriptState->contextIsValid()) {
On 2016/07/27 15:28:43, haraken wrote:
> 
> I was thinking that this check was doing what you want to do with this CL.
> 
> Maybe the real problem is that we're using a wrong ScriptState here?

No, m_scriptState is different from function's context.

Let's consider the following case.
    winA.setTimeout.call(winB, winC.callback, 0);

Before https://crrev.com/2049493005
    m_scriptState->context() = winA's context
    m_function.CreationContext() = winC's context
    // If winA == winC, then we don't need to check
m_function.CreationContext().

After https://crrev.com/2049493005
    m_scriptState->context() = winB's context
    m_function.CreationContext() = winC's context
    // Even if winA == winC, we need to explicitly check
m_function.CreationContext().

The problem has been that we haven't checked the ScriptState for winC.  For the
time being (i.e. while we don't support detached windows), we need to check BOTH
of winB and winC's ScriptState.

I think we didn't hit this problem just because winA == winC for most cases. 
However, we changed to use winB's context to schedule the tasks.  Then, checking
m_scriptState doesn't help about m_function.CreationContext().

[haraken, 2016-07-28 09:03:13]

On 2016/07/28 06:12:35, Yuki wrote:
>
https://codereview.chromium.org/2191543002/diff/1/third_party/WebKit/Source/b...
> File third_party/WebKit/Source/bindings/core/v8/ScheduledAction.cpp (right):
> 
>
https://codereview.chromium.org/2191543002/diff/1/third_party/WebKit/Source/b...
> third_party/WebKit/Source/bindings/core/v8/ScheduledAction.cpp:108: if
> (!m_scriptState->contextIsValid()) {
> On 2016/07/27 15:28:43, haraken wrote:
> > 
> > I was thinking that this check was doing what you want to do with this CL.
> > 
> > Maybe the real problem is that we're using a wrong ScriptState here?
> 
> No, m_scriptState is different from function's context.
> 
> Let's consider the following case.
>     winA.setTimeout.call(winB, winC.callback, 0);
> 
> Before https://crrev.com/2049493005
>     m_scriptState->context() = winA's context
>     m_function.CreationContext() = winC's context
>     // If winA == winC, then we don't need to check
> m_function.CreationContext().
> 
> After https://crrev.com/2049493005
>     m_scriptState->context() = winB's context
>     m_function.CreationContext() = winC's context
>     // Even if winA == winC, we need to explicitly check
> m_function.CreationContext().
> 
> The problem has been that we haven't checked the ScriptState for winC.  For
the
> time being (i.e. while we don't support detached windows), we need to check
BOTH
> of winB and winC's ScriptState.
> 
> I think we didn't hit this problem just because winA == winC for most cases. 
> However, we changed to use winB's context to schedule the tasks.  Then,
checking
> m_scriptState doesn't help about m_function.CreationContext().

Thanks for the clarification! Makes sense. LGTM.

[yhirano, 2016-07-28 09:09:31]

lgtm

[Yuki, 2016-07-28 12:11:21]

The CQ bit was checked by yukishiino@chromium.org

[commit-bot: I haz the power, 2016-07-28 12:11:34]

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, 2016-07-28 12:14:53]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-07-28 12:17:59]

Description was changed from

==========
binding: Disallows to run a function if its context is detached.

There are two contexts when about to run a ScheduledAction.
|this|'s context and the context of the function to be run.
If the latter context no longer has a valid V8PerContextData,
|self| returns null.
    function() { self.testRunner; } // self will be null

This issue has been there for a long time, but
https://crrev.com/2049493005 made it easy to happen.

This CL disallows to run a function if the function's context is
detached.  This is not the right fix, but close to the behavior
before https://crrev.com/2049493005 .

The right fix is that, per spec, |self| always returns the global
proxy object that never be null.  I.e. we need to support detached
windows.

BUG=630996
==========

to

==========
binding: Disallows to run a function if its context is detached.

There are two contexts when about to run a ScheduledAction.
|this|'s context and the context of the function to be run.
If the latter context no longer has a valid V8PerContextData,
|self| returns null.
    function() { self.testRunner; } // self will be null

This issue has been there for a long time, but
https://crrev.com/2049493005 made it easy to happen.

This CL disallows to run a function if the function's context is
detached.  This is not the right fix, but close to the behavior
before https://crrev.com/2049493005 .

The right fix is that, per spec, |self| always returns the global
proxy object that never be null.  I.e. we need to support detached
windows.

BUG=630996

Committed: https://crrev.com/981ce039703c0350086568b8c922bee8b8ca2761
Cr-Commit-Position: refs/heads/master@{#408376}
==========

[commit-bot: I haz the power, 2016-07-28 12:18:00]

Patchset 1 (id:??) landed as
https://crrev.com/981ce039703c0350086568b8c922bee8b8ca2761
Cr-Commit-Position: refs/heads/master@{#408376}