Track the script state a listener was created in

Track the script state a listener was created in

That way, we can delete the private values on the listener in the right
context

R=haraken@chromium.org
BUG=none

Committed: https://crrev.com/aaf8bd48dfd77a6945f3cfd4c5b01ab631d956cd
Cr-Commit-Position: refs/heads/master@{#360578}

[jochen (gone - plz use gerrit), 2015-11-06 23:31:56]

jochen@chromium.org changed reviewers:
+ vogelheim@chromium.org

[jochen (gone - plz use gerrit), 2015-11-06 23:31:56]

+vogelheim, ptal

https://codereview.chromium.org/1419313003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.h
(right):

https://codereview.chromium.org/1419313003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.h:151:
ScriptState* m_scriptState;
this is basically the creation contxt of m_listener, so it's not a refptr. we
know that as long as m_listener is non-empty, m_scriptState has a valid context.

[haraken, 2015-11-07 01:23:39]

https://codereview.chromium.org/1419313003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.h
(right):

https://codereview.chromium.org/1419313003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.h:151:
ScriptState* m_scriptState;
On 2015/11/06 23:31:56, jochen (slow - traveling) wrote:
> this is basically the creation contxt of m_listener, so it's not a refptr. we
> know that as long as m_listener is non-empty, m_scriptState has a valid
context.

Why can't we use a RefPtr?

https://codereview.chromium.org/1419313003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.h:151:
ScriptState* m_scriptState;
On 2015/11/06 23:31:56, jochen (slow - traveling) wrote:
> this is basically the creation contxt of m_listener, so it's not a refptr. we
> know that as long as m_listener is non-empty, m_scriptState has a valid
context.

In general, caching m_scriptState and using it later is correct, but in the
particular case of V8AbstractEventListener, it's not for some complicated
reason. That's why we currently cache m_world and use toV8Context() to retrieve
a correct context instead of m_scriptState->context().

You can find more details in https://codereview.chromium.org/823263002/.

[jochen (gone - plz use gerrit), 2015-11-07 01:40:04]

On 2015/11/07 at 01:23:39, haraken wrote:
>
https://codereview.chromium.org/1419313003/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.h
(right):
> 
>
https://codereview.chromium.org/1419313003/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.h:151:
ScriptState* m_scriptState;
> On 2015/11/06 23:31:56, jochen (slow - traveling) wrote:
> > this is basically the creation contxt of m_listener, so it's not a refptr.
we
> > know that as long as m_listener is non-empty, m_scriptState has a valid
context.
> 
> Why can't we use a RefPtr?
> 


We can

>
https://codereview.chromium.org/1419313003/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.h:151:
ScriptState* m_scriptState;
> On 2015/11/06 23:31:56, jochen (slow - traveling) wrote:
> > this is basically the creation contxt of m_listener, so it's not a refptr.
we
> > know that as long as m_listener is non-empty, m_scriptState has a valid
context.
> 
> In general, caching m_scriptState and using it later is correct, but in the
particular case of V8AbstractEventListener, it's not for some complicated
reason. That's why we currently cache m_world and use toV8Context() to retrieve
a correct context instead of m_scriptState->context().
> 
> You can find more details in https://codereview.chromium.org/823263002/.

Yeah, but we don't have an execution context or anything in the dtor :-/

[haraken, 2015-11-07 01:45:44]

On 2015/11/07 01:40:04, jochen (slow - traveling) wrote:
> On 2015/11/07 at 01:23:39, haraken wrote:
> >
>
https://codereview.chromium.org/1419313003/diff/20001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.h
> (right):
> > 
> >
>
https://codereview.chromium.org/1419313003/diff/20001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.h:151:
> ScriptState* m_scriptState;
> > On 2015/11/06 23:31:56, jochen (slow - traveling) wrote:
> > > this is basically the creation contxt of m_listener, so it's not a refptr.
> we
> > > know that as long as m_listener is non-empty, m_scriptState has a valid
> context.
> > 
> > Why can't we use a RefPtr?
> > 
> 
> 
> We can
> 
> >
>
https://codereview.chromium.org/1419313003/diff/20001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.h:151:
> ScriptState* m_scriptState;
> > On 2015/11/06 23:31:56, jochen (slow - traveling) wrote:
> > > this is basically the creation contxt of m_listener, so it's not a refptr.
> we
> > > know that as long as m_listener is non-empty, m_scriptState has a valid
> context.
> > 
> > In general, caching m_scriptState and using it later is correct, but in the
> particular case of V8AbstractEventListener, it's not for some complicated
> reason. That's why we currently cache m_world and use toV8Context() to
retrieve
> a correct context instead of m_scriptState->context().
> > 
> > You can find more details in https://codereview.chromium.org/823263002/.
> 
> Yeah, but we don't have an execution context or anything in the dtor :-/

Can you retrieve the correct context from the wrapper on which the private value
is installed? (i.e., the wrapper => creation context => ScriptState). It would
be nice if we can encapsulate the logic in V8HiddenValue (so that the call site
of V8HiddenValue doesn't need to keep track of the ScriptState).

[jochen (gone - plz use gerrit), 2015-11-07 01:49:36]

On 2015/11/07 at 01:45:44, haraken wrote:
> On 2015/11/07 01:40:04, jochen (slow - traveling) wrote:
> > On 2015/11/07 at 01:23:39, haraken wrote:
> > >
> >
https://codereview.chromium.org/1419313003/diff/20001/third_party/WebKit/Sour...
> > > File third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.h
> > (right):
> > > 
> > >
> >
https://codereview.chromium.org/1419313003/diff/20001/third_party/WebKit/Sour...
> > > third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.h:151:
> > ScriptState* m_scriptState;
> > > On 2015/11/06 23:31:56, jochen (slow - traveling) wrote:
> > > > this is basically the creation contxt of m_listener, so it's not a
refptr.
> > we
> > > > know that as long as m_listener is non-empty, m_scriptState has a valid
> > context.
> > > 
> > > Why can't we use a RefPtr?
> > > 
> > 
> > 
> > We can
> > 
> > >
> >
https://codereview.chromium.org/1419313003/diff/20001/third_party/WebKit/Sour...
> > > third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.h:151:
> > ScriptState* m_scriptState;
> > > On 2015/11/06 23:31:56, jochen (slow - traveling) wrote:
> > > > this is basically the creation contxt of m_listener, so it's not a
refptr.
> > we
> > > > know that as long as m_listener is non-empty, m_scriptState has a valid
> > context.
> > > 
> > > In general, caching m_scriptState and using it later is correct, but in
the
> > particular case of V8AbstractEventListener, it's not for some complicated
> > reason. That's why we currently cache m_world and use toV8Context() to
retrieve
> > a correct context instead of m_scriptState->context().
> > > 
> > > You can find more details in https://codereview.chromium.org/823263002/.
> > 
> > Yeah, but we don't have an execution context or anything in the dtor :-/
> 
> Can you retrieve the correct context from the wrapper on which the private
value is installed? (i.e., the wrapper => creation context => ScriptState). It
would be nice if we can encapsulate the logic in V8HiddenValue (so that the call
site of V8HiddenValue doesn't need to keep track of the ScriptState).

Creation Context is deprecated

[haraken, 2015-11-07 02:54:54]

On 2015/11/07 01:49:36, jochen (slow - traveling) wrote:
> On 2015/11/07 at 01:45:44, haraken wrote:
> > On 2015/11/07 01:40:04, jochen (slow - traveling) wrote:
> > > On 2015/11/07 at 01:23:39, haraken wrote:
> > > >
> > >
>
https://codereview.chromium.org/1419313003/diff/20001/third_party/WebKit/Sour...
> > > > File
third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.h
> > > (right):
> > > > 
> > > >
> > >
>
https://codereview.chromium.org/1419313003/diff/20001/third_party/WebKit/Sour...
> > > >
third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.h:151:
> > > ScriptState* m_scriptState;
> > > > On 2015/11/06 23:31:56, jochen (slow - traveling) wrote:
> > > > > this is basically the creation contxt of m_listener, so it's not a
> refptr.
> > > we
> > > > > know that as long as m_listener is non-empty, m_scriptState has a
valid
> > > context.
> > > > 
> > > > Why can't we use a RefPtr?
> > > > 
> > > 
> > > 
> > > We can
> > > 
> > > >
> > >
>
https://codereview.chromium.org/1419313003/diff/20001/third_party/WebKit/Sour...
> > > >
third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.h:151:
> > > ScriptState* m_scriptState;
> > > > On 2015/11/06 23:31:56, jochen (slow - traveling) wrote:
> > > > > this is basically the creation contxt of m_listener, so it's not a
> refptr.
> > > we
> > > > > know that as long as m_listener is non-empty, m_scriptState has a
valid
> > > context.
> > > > 
> > > > In general, caching m_scriptState and using it later is correct, but in
> the
> > > particular case of V8AbstractEventListener, it's not for some complicated
> > > reason. That's why we currently cache m_world and use toV8Context() to
> retrieve
> > > a correct context instead of m_scriptState->context().
> > > > 
> > > > You can find more details in https://codereview.chromium.org/823263002/.
> > > 
> > > Yeah, but we don't have an execution context or anything in the dtor :-/
> > 
> > Can you retrieve the correct context from the wrapper on which the private
> value is installed? (i.e., the wrapper => creation context => ScriptState). It
> would be nice if we can encapsulate the logic in V8HiddenValue (so that the
call
> site of V8HiddenValue doesn't need to keep track of the ScriptState).
> 
> Creation Context is deprecated

We anyway need to have a way to know a creation context of a given wrapper (for
other reasons). Until we have that way inside Blink, it would make sense to use
CreationContext in V8 API.

[jochen (gone - plz use gerrit), 2015-11-07 05:07:52]

On 2015/11/07 at 02:54:54, haraken wrote:
> On 2015/11/07 01:49:36, jochen (slow - traveling) wrote:
> > On 2015/11/07 at 01:45:44, haraken wrote:
> > > On 2015/11/07 01:40:04, jochen (slow - traveling) wrote:
> > > > On 2015/11/07 at 01:23:39, haraken wrote:
> > > > >
> > > >
> >
https://codereview.chromium.org/1419313003/diff/20001/third_party/WebKit/Sour...
> > > > > File
third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.h
> > > > (right):
> > > > > 
> > > > >
> > > >
> >
https://codereview.chromium.org/1419313003/diff/20001/third_party/WebKit/Sour...
> > > > >
third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.h:151:
> > > > ScriptState* m_scriptState;
> > > > > On 2015/11/06 23:31:56, jochen (slow - traveling) wrote:
> > > > > > this is basically the creation contxt of m_listener, so it's not a
> > refptr.
> > > > we
> > > > > > know that as long as m_listener is non-empty, m_scriptState has a
valid
> > > > context.
> > > > > 
> > > > > Why can't we use a RefPtr?
> > > > > 
> > > > 
> > > > 
> > > > We can
> > > > 
> > > > >
> > > >
> >
https://codereview.chromium.org/1419313003/diff/20001/third_party/WebKit/Sour...
> > > > >
third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.h:151:
> > > > ScriptState* m_scriptState;
> > > > > On 2015/11/06 23:31:56, jochen (slow - traveling) wrote:
> > > > > > this is basically the creation contxt of m_listener, so it's not a
> > refptr.
> > > > we
> > > > > > know that as long as m_listener is non-empty, m_scriptState has a
valid
> > > > context.
> > > > > 
> > > > > In general, caching m_scriptState and using it later is correct, but
in
> > the
> > > > particular case of V8AbstractEventListener, it's not for some
complicated
> > > > reason. That's why we currently cache m_world and use toV8Context() to
> > retrieve
> > > > a correct context instead of m_scriptState->context().
> > > > > 
> > > > > You can find more details in
https://codereview.chromium.org/823263002/.
> > > > 
> > > > Yeah, but we don't have an execution context or anything in the dtor :-/
> > > 
> > > Can you retrieve the correct context from the wrapper on which the private
> > value is installed? (i.e., the wrapper => creation context => ScriptState).
It
> > would be nice if we can encapsulate the logic in V8HiddenValue (so that the
call
> > site of V8HiddenValue doesn't need to keep track of the ScriptState).
> > 
> > Creation Context is deprecated
> 
> We anyway need to have a way to know a creation context of a given wrapper
(for other reasons). Until we have that way inside Blink, it would make sense to
use CreationContext in V8 API.

hum, but we have the creation context here in the form of the script state, so
why not use it?

[haraken, 2015-11-07 06:42:54]

On 2015/11/07 05:07:52, jochen (slow - traveling) wrote:
> On 2015/11/07 at 02:54:54, haraken wrote:
> > On 2015/11/07 01:49:36, jochen (slow - traveling) wrote:
> > > On 2015/11/07 at 01:45:44, haraken wrote:
> > > > On 2015/11/07 01:40:04, jochen (slow - traveling) wrote:
> > > > > On 2015/11/07 at 01:23:39, haraken wrote:
> > > > > >
> > > > >
> > >
>
https://codereview.chromium.org/1419313003/diff/20001/third_party/WebKit/Sour...
> > > > > > File
> third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.h
> > > > > (right):
> > > > > > 
> > > > > >
> > > > >
> > >
>
https://codereview.chromium.org/1419313003/diff/20001/third_party/WebKit/Sour...
> > > > > >
> third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.h:151:
> > > > > ScriptState* m_scriptState;
> > > > > > On 2015/11/06 23:31:56, jochen (slow - traveling) wrote:
> > > > > > > this is basically the creation contxt of m_listener, so it's not a
> > > refptr.
> > > > > we
> > > > > > > know that as long as m_listener is non-empty, m_scriptState has a
> valid
> > > > > context.
> > > > > > 
> > > > > > Why can't we use a RefPtr?
> > > > > > 
> > > > > 
> > > > > 
> > > > > We can
> > > > > 
> > > > > >
> > > > >
> > >
>
https://codereview.chromium.org/1419313003/diff/20001/third_party/WebKit/Sour...
> > > > > >
> third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.h:151:
> > > > > ScriptState* m_scriptState;
> > > > > > On 2015/11/06 23:31:56, jochen (slow - traveling) wrote:
> > > > > > > this is basically the creation contxt of m_listener, so it's not a
> > > refptr.
> > > > > we
> > > > > > > know that as long as m_listener is non-empty, m_scriptState has a
> valid
> > > > > context.
> > > > > > 
> > > > > > In general, caching m_scriptState and using it later is correct, but
> in
> > > the
> > > > > particular case of V8AbstractEventListener, it's not for some
> complicated
> > > > > reason. That's why we currently cache m_world and use toV8Context() to
> > > retrieve
> > > > > a correct context instead of m_scriptState->context().
> > > > > > 
> > > > > > You can find more details in
> https://codereview.chromium.org/823263002/.
> > > > > 
> > > > > Yeah, but we don't have an execution context or anything in the dtor
:-/
> > > > 
> > > > Can you retrieve the correct context from the wrapper on which the
private
> > > value is installed? (i.e., the wrapper => creation context =>
ScriptState).
> It
> > > would be nice if we can encapsulate the logic in V8HiddenValue (so that
the
> call
> > > site of V8HiddenValue doesn't need to keep track of the ScriptState).
> > > 
> > > Creation Context is deprecated
> > 
> > We anyway need to have a way to know a creation context of a given wrapper
> (for other reasons). Until we have that way inside Blink, it would make sense
to
> use CreationContext in V8 API.
> 
> hum, but we have the creation context here in the form of the script state, so
> why not use it?

We could. But it seems nicer to me if V8HiddenValue can encapsulate the logic
inside deleteHiddenValue etc (rather than asking all call sites of the methods
to enter a correct context).

[jochen (gone - plz use gerrit), 2015-11-07 06:46:55]

hum, I think the correct thing would be to require a ScriptState* for all
V8HiddenValue methods.

The thing is that Get/Set must be in a context anyways, and for Delete, all but
one callsite are in a context, so I don't think it's unreasonable to require
that.

[haraken, 2015-11-07 06:56:47]

Sounds reasonable to me. Mostly looks good.

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

https://codereview.chromium.org/1419313003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.cpp:62:
V8EventListenerList::clearWrapper(m_listener.newLocal(isolate()), m_isAttribute,
m_scriptState.get());

Just help me understand:

- Would you elaborate on why https://codereview.chromium.org/1422573009/ doesn't
work?

- Is it possible that the clearWrapper invokes arbitrary user JS? If that
happens, it will be problematic because we're now in a destructor...

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

https://codereview.chromium.org/1419313003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.h:151:
RefPtr<ScriptState> m_scriptState;

I'd rename this to m_scriptStateForListener, since the ScriptState has a
different meaning than m_scriptState in other DOM objects (normally
m_scriptState stores a ScriptState that instantiated the DOM object).

[jochen (gone - plz use gerrit), 2015-11-07 15:31:34]

On 2015/11/07 at 06:56:47, haraken wrote:
> Sounds reasonable to me. Mostly looks good.

Do you want me to add an explicit ScriptState* parameter to the V8HiddenValue
methods?

> 
>
https://codereview.chromium.org/1419313003/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.cpp
(right):
> 
>
https://codereview.chromium.org/1419313003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.cpp:62:
V8EventListenerList::clearWrapper(m_listener.newLocal(isolate()), m_isAttribute,
m_scriptState.get());
> 
> Just help me understand:
> 
> - Would you elaborate on why https://codereview.chromium.org/1422573009/
doesn't work?

If the event listener (on the JS side) is still reachable, dropping the
persistent handle won't make it be collected. When it then is used for another
event, we still have the hidden value on the wrapper that points to a now dead
V8AbstractEventListener class. That's what makes the layout tests fail.


> 
> - Is it possible that the clearWrapper invokes arbitrary user JS? If that
happens, it will be problematic because we're now in a destructor...

no, that's not possible.

> 
>
https://codereview.chromium.org/1419313003/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.h
(right):
> 
>
https://codereview.chromium.org/1419313003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.h:151:
RefPtr<ScriptState> m_scriptState;
> 
> I'd rename this to m_scriptStateForListener, since the ScriptState has a
different meaning than m_scriptState in other DOM objects (normally
m_scriptState stores a ScriptState that instantiated the DOM object).

[haraken, 2015-11-07 20:15:52]

On 2015/11/07 15:31:34, jochen (slow - traveling) wrote:
> On 2015/11/07 at 06:56:47, haraken wrote:
> > Sounds reasonable to me. Mostly looks good.
> 
> Do you want me to add an explicit ScriptState* parameter to the V8HiddenValue
> methods?

Sounds nicer. With that change, LG.

> > - Would you elaborate on why https://codereview.chromium.org/1422573009/
> doesn't work?
> 
> If the event listener (on the JS side) is still reachable, dropping the
> persistent handle won't make it be collected. When it then is used for another
> event, we still have the hidden value on the wrapper that points to a now dead
> V8AbstractEventListener class. That's what makes the layout tests fail.

Makes sense.

[jochen (gone - plz use gerrit), 2015-11-08 06:13:19]

done

[haraken, 2015-11-08 07:05:05]

Thanks for a lot of work! LGTM.

https://codereview.chromium.org/1419313003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8ThrowException.cpp (right):

https://codereview.chromium.org/1419313003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8ThrowException.cpp:92:
V8HiddenValue::setHiddenValue(ScriptState::current(isolate), exceptionObject,
V8HiddenValue::error(isolate), error);

I think this should be ScriptState::from(sanitizedCreationContext).

https://codereview.chromium.org/1419313003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/custom/V8CustomEventCustom.cpp
(right):

https://codereview.chromium.org/1419313003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/custom/V8CustomEventCustom.cpp:48:
static v8::Local<v8::Value> cacheState(v8::Isolate* isolate,
v8::Local<v8::Object> customEvent, v8::Local<v8::Value> detail)

You can pass in ScriptState*.

https://codereview.chromium.org/1419313003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/custom/V8ErrorEventCustom.cpp
(right):

https://codereview.chromium.org/1419313003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/custom/V8ErrorEventCustom.cpp:46:
static void setHiddenValueAndReturnValue(const
v8::FunctionCallbackInfo<v8::Value>& info, v8::Local<v8::Value> error)

You can pass in ScriptState*.

https://codereview.chromium.org/1419313003/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/bindings/core/v8/custom/V8MessageChannelCustom.cpp
(right):

https://codereview.chromium.org/1419313003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/custom/V8MessageChannelCustom.cpp:47:
MessageChannel* channel = MessageChannel::create(context);

ScriptState* scriptState = ScriptState::current(info.GetIsolate());
MessageChannel* channel =
MessageChannel::create(scriptState->executionContext());

https://codereview.chromium.org/1419313003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/custom/V8MessageChannelCustom.cpp:54:
ScriptState* scriptState = ScriptState::current(info.GetIsolate());

Remove this.

https://codereview.chromium.org/1419313003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/custom/V8PopStateEventCustom.cpp
(right):

https://codereview.chromium.org/1419313003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/custom/V8PopStateEventCustom.cpp:44:
static v8::Local<v8::Value> cacheState(v8::Local<v8::Object> popStateEvent,
v8::Local<v8::Value> state, v8::Isolate* isolate)

You can pass in ScriptState*.

https://codereview.chromium.org/1419313003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/custom/V8PopStateEventCustom.cpp:84:
ScriptState* scriptState = ScriptState::current(info.GetIsolate());

Avoid calling ScriptState::current twice in one method.

https://codereview.chromium.org/1419313003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp
(right):

https://codereview.chromium.org/1419313003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp:80: if
(context.IsEmpty())

Not related to your CL, I don't understand why this code is needed. (You don't
need to address in this CL.)

https://codereview.chromium.org/1419313003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp:101: if
(context.IsEmpty())

Ditto.

https://codereview.chromium.org/1419313003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp:279:
V8HiddenValue::setHiddenValue(ScriptState::current(isolate), info.Holder(),
V8HiddenValue::testInterfaces(isolate), interfaces);

Ditto.

[vogelheim, 2015-11-09 18:42:13]

Much of the CL is about getting the right context in V8HiddenValue::*, but then
those methods don't actually do anything with that context. Am I missing
something?

https://codereview.chromium.org/1419313003/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/bindings/core/v8/CustomElementConstructorBuilder.cpp
(right):

https://codereview.chromium.org/1419313003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/CustomElementConstructorBuilder.cpp:275:
ScriptState* scriptState = ScriptState::current(info.GetIsolate());
nitpick: ScriptState::current(isolate)

https://codereview.chromium.org/1419313003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8HiddenValue.cpp (right):

https://codereview.chromium.org/1419313003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8HiddenValue.cpp:28: return
object->GetHiddenValue(key);
Should this be:
return object->GetPrivate(scriptState->context());

(likewise below, for SetPrivste, DeletePrivate...)

https://codereview.chromium.org/1419313003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/templates/attributes.cpp (right):

https://codereview.chromium.org/1419313003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/attributes.cpp:36:
v8::Local<v8::Value> v8Value =
V8HiddenValue::getHiddenValue(ScriptState::current(info.GetIsolate()), holder,
propertyName);
Should [Property|Function]CallbackInfo know the context it's being run in?

(Not sure if this is useful feedback for this CL; but making the bindings always
guess the context from the isolate seems odd.)

[jochen (gone - plz use gerrit), 2015-11-09 18:50:51]

On 2015/11/09 at 18:42:13, vogelheim wrote:
> Much of the CL is about getting the right context in V8HiddenValue::*, but
then those methods don't actually do anything with that context. Am I missing
something?
> 

It's pre-work for https://codereview.chromium.org/1416053012 which needs a
context.

>
https://codereview.chromium.org/1419313003/diff/80001/third_party/WebKit/Sour...
> File
third_party/WebKit/Source/bindings/core/v8/CustomElementConstructorBuilder.cpp
(right):
> 
>
https://codereview.chromium.org/1419313003/diff/80001/third_party/WebKit/Sour...
>
third_party/WebKit/Source/bindings/core/v8/CustomElementConstructorBuilder.cpp:275:
ScriptState* scriptState = ScriptState::current(info.GetIsolate());
> nitpick: ScriptState::current(isolate)
> 
>
https://codereview.chromium.org/1419313003/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/bindings/core/v8/V8HiddenValue.cpp (right):
> 
>
https://codereview.chromium.org/1419313003/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/bindings/core/v8/V8HiddenValue.cpp:28: return
object->GetHiddenValue(key);
> Should this be:
> return object->GetPrivate(scriptState->context());
> 
> (likewise below, for SetPrivste, DeletePrivate...)
> 
>
https://codereview.chromium.org/1419313003/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/bindings/templates/attributes.cpp (right):
> 
>
https://codereview.chromium.org/1419313003/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/bindings/templates/attributes.cpp:36:
v8::Local<v8::Value> v8Value =
V8HiddenValue::getHiddenValue(ScriptState::current(info.GetIsolate()), holder,
propertyName);
> Should [Property|Function]CallbackInfo know the context it's being run in?
> 
> (Not sure if this is useful feedback for this CL; but making the bindings
always guess the context from the isolate seems odd.)

The main point is that it's not possible to always guess it. If you know that
you're in a context (i.e. isolate->GetCurrentContext() exists), then it's fine
to take that context. We know that this is the case in all the function
callbacks for example.

However, if we're not in a context, it's important to get the right context to
V8 (otherwise, v8 will have to do odd guesswork internally). That's why the
new-style v8 APIs all require a v8::Context as argument, so we can get rid of
the guesswork

[jochen (gone - plz use gerrit), 2015-11-09 21:33:40]

https://codereview.chromium.org/1419313003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8ThrowException.cpp (right):

https://codereview.chromium.org/1419313003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8ThrowException.cpp:92:
V8HiddenValue::setHiddenValue(ScriptState::current(isolate), exceptionObject,
V8HiddenValue::error(isolate), error);
On 2015/11/08 at 07:05:04, haraken wrote:
> I think this should be ScriptState::from(sanitizedCreationContext).

despite the name, sanitizedCreationContext is not a context. I restructured the
code a bit to address this.

https://codereview.chromium.org/1419313003/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/bindings/core/v8/custom/V8MessageChannelCustom.cpp
(right):

https://codereview.chromium.org/1419313003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/custom/V8MessageChannelCustom.cpp:54:
ScriptState* scriptState = ScriptState::current(info.GetIsolate());
On 2015/11/08 at 07:05:05, haraken wrote:
> Remove this.

hum? It's needed in the next two lines?

https://codereview.chromium.org/1419313003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp
(right):

https://codereview.chromium.org/1419313003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp:80: if
(context.IsEmpty())
On 2015/11/08 at 07:05:05, haraken wrote:
> Not related to your CL, I don't understand why this code is needed. (You don't
need to address in this CL.)

the frame could be detached

[jochen (gone - plz use gerrit), 2015-11-09 21:36:20]

the reason why the inspector tests are failing is because inspector enters the
debugger context.

Choices are: plumb ScriptState* through all of the inspector (possible, but a
lot of code), or pass a v8::Local<v8::Context> around instead of a ScriptState
(probably cleaner...)

wdyt?

[haraken, 2015-11-09 22:44:19]

On 2015/11/09 21:36:20, jochen (slow - traveling) wrote:
> the reason why the inspector tests are failing is because inspector enters the
> debugger context.
> 
> Choices are: plumb ScriptState* through all of the inspector (possible, but a
> lot of code), or pass a v8::Local<v8::Context> around instead of a ScriptState
> (probably cleaner...)
> 
> wdyt?

Would you elaborate on the issue a bit more? Is the issue that
v8::Isolate::GetCurrentContext() doesn't skip the entered debugger context?

[jochen (gone - plz use gerrit), 2015-11-09 22:49:43]

On 2015/11/09 at 22:44:19, haraken wrote:
> On 2015/11/09 21:36:20, jochen (slow - traveling) wrote:
> > the reason why the inspector tests are failing is because inspector enters
the
> > debugger context.
> > 
> > Choices are: plumb ScriptState* through all of the inspector (possible, but
a
> > lot of code), or pass a v8::Local<v8::Context> around instead of a
ScriptState
> > (probably cleaner...)
> > 
> > wdyt?
> 
> Would you elaborate on the issue a bit more? Is the issue that
v8::Isolate::GetCurrentContext() doesn't skip the entered debugger context?

this and the fact that the debugger context doesn't have an associated
scriptstate

[haraken, 2015-11-09 22:53:18]

On 2015/11/09 22:49:43, jochen (slow - traveling) wrote:
> On 2015/11/09 at 22:44:19, haraken wrote:
> > On 2015/11/09 21:36:20, jochen (slow - traveling) wrote:
> > > the reason why the inspector tests are failing is because inspector enters
> the
> > > debugger context.
> > > 
> > > Choices are: plumb ScriptState* through all of the inspector (possible,
but
> a
> > > lot of code), or pass a v8::Local<v8::Context> around instead of a
> ScriptState
> > > (probably cleaner...)
> > > 
> > > wdyt?
> > 
> > Would you elaborate on the issue a bit more? Is the issue that
> v8::Isolate::GetCurrentContext() doesn't skip the entered debugger context?
> 
> this and the fact that the debugger context doesn't have an associated
> scriptstate

Would ScriptState::hasCurrentScriptState be helpful?

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[jochen (gone - plz use gerrit), 2015-11-09 22:53:42]

https://codereview.chromium.org/1419313003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/v8/InspectorWrapper.cpp (right):

https://codereview.chromium.org/1419313003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/v8/InspectorWrapper.cpp:54:
v8::Local<v8::Value> value =
V8HiddenValue::getHiddenValue(ScriptState::current(isolate), object,
v8InternalizedString(isolate, name));
this is the place where we're sometimes in a debugger context.

[jochen (gone - plz use gerrit), 2015-11-09 22:54:58]

On 2015/11/09 at 22:53:18, haraken wrote:
> On 2015/11/09 22:49:43, jochen (slow - traveling) wrote:
> > On 2015/11/09 at 22:44:19, haraken wrote:
> > > On 2015/11/09 21:36:20, jochen (slow - traveling) wrote:
> > > > the reason why the inspector tests are failing is because inspector
enters
> > the
> > > > debugger context.
> > > > 
> > > > Choices are: plumb ScriptState* through all of the inspector (possible,
but
> > a
> > > > lot of code), or pass a v8::Local<v8::Context> around instead of a
> > ScriptState
> > > > (probably cleaner...)
> > > > 
> > > > wdyt?
> > > 
> > > Would you elaborate on the issue a bit more? Is the issue that
> > v8::Isolate::GetCurrentContext() doesn't skip the entered debugger context?
> > 
> > this and the fact that the debugger context doesn't have an associated
> > scriptstate
> 
> Would ScriptState::hasCurrentScriptState be helpful?
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

not really - I need a ScriptState. Or are you proposing to just walk up the
context stack until I find a non-debugger context?

[jochen (gone - plz use gerrit), 2015-11-09 22:55:31]

it's also that v8 doesn't really care what kind of context you pass in for the
get

[haraken, 2015-11-09 23:01:45]

https://codereview.chromium.org/1419313003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/v8/InspectorWrapper.cpp (right):

https://codereview.chromium.org/1419313003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/v8/InspectorWrapper.cpp:54:
v8::Local<v8::Value> value =
V8HiddenValue::getHiddenValue(ScriptState::current(isolate), object,
v8InternalizedString(isolate, name));
On 2015/11/09 22:53:42, jochen (slow - traveling) wrote:
> this is the place where we're sometimes in a debugger context.

Ah, I understand the issue.

When we discussed (a couple of years ago) if we should create a ScriptState for
a debugger context or not, we decided not to do that. Instead we set a rule that
we should not run anything context-dependent while we are in a debugger context.
This means that we should not run anything that requires ScriptState* while we
are in a debugger context. 

Following that rule, a right fix would be just to remove the ScriptState*
parameter from V8HiddenValue::getHiddenValue.

[jochen (gone - plz use gerrit), 2015-11-09 23:03:14]

On 2015/11/09 at 23:01:45, haraken wrote:
>
https://codereview.chromium.org/1419313003/diff/100001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/inspector/v8/InspectorWrapper.cpp (right):
> 
>
https://codereview.chromium.org/1419313003/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/inspector/v8/InspectorWrapper.cpp:54:
v8::Local<v8::Value> value =
V8HiddenValue::getHiddenValue(ScriptState::current(isolate), object,
v8InternalizedString(isolate, name));
> On 2015/11/09 22:53:42, jochen (slow - traveling) wrote:
> > this is the place where we're sometimes in a debugger context.
> 
> Ah, I understand the issue.
> 
> When we discussed (a couple of years ago) if we should create a ScriptState
for a debugger context or not, we decided not to do that. Instead we set a rule
that we should not run anything context-dependent while we are in a debugger
context. This means that we should not run anything that requires ScriptState*
while we are in a debugger context. 
> 
> Following that rule, a right fix would be just to remove the ScriptState*
parameter from V8HiddenValue::getHiddenValue.

getHiddenValue is context dependent, however

[haraken, 2015-11-09 23:09:04]

On 2015/11/09 23:03:14, jochen (slow - traveling) wrote:
> On 2015/11/09 at 23:01:45, haraken wrote:
> >
>
https://codereview.chromium.org/1419313003/diff/100001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/core/inspector/v8/InspectorWrapper.cpp
(right):
> > 
> >
>
https://codereview.chromium.org/1419313003/diff/100001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/inspector/v8/InspectorWrapper.cpp:54:
> v8::Local<v8::Value> value =
> V8HiddenValue::getHiddenValue(ScriptState::current(isolate), object,
> v8InternalizedString(isolate, name));
> > On 2015/11/09 22:53:42, jochen (slow - traveling) wrote:
> > > this is the place where we're sometimes in a debugger context.
> > 
> > Ah, I understand the issue.
> > 
> > When we discussed (a couple of years ago) if we should create a ScriptState
> for a debugger context or not, we decided not to do that. Instead we set a
rule
> that we should not run anything context-dependent while we are in a debugger
> context. This means that we should not run anything that requires ScriptState*
> while we are in a debugger context. 
> > 
> > Following that rule, a right fix would be just to remove the ScriptState*
> parameter from V8HiddenValue::getHiddenValue.
> 
> getHiddenValue is context dependent, however

What context should we use for the getHiddenValue in InspectorWrapper.cpp? The
debugger context, or the (non-debugger) context prior to the debugger context?

Assuming the answer is the latter, I think you should cache (or pass in) the
ScriptState of the non-debugger context somewhere and use it in
InspectorWrapper.cpp (instead of calling ScriptState::current() after pushing
the debugger context).

[jochen (gone - plz use gerrit), 2015-11-19 11:46:33]

rebased after fixing the inspector stuff... nothing changed expect that the
tests should be green now :)

[haraken, 2015-11-19 11:54:18]

On 2015/11/19 11:46:33, jochen wrote:
> rebased after fixing the inspector stuff... nothing changed expect that the
> tests should be green now :)

still LGTM

[vogelheim, 2015-11-19 11:58:50]

lgtm

(Sorry, had meant to lgtm earlier.)

Also, can you point me to the inspector-scriptstate-fixing CL, so I can
understand the context of this change a bit better? Thx.

[jochen (gone - plz use gerrit), 2015-11-19 11:59:27]

it's here: https://codereview.chromium.org/1455583002

[jochen (gone - plz use gerrit), 2015-11-19 12:31:54]

The CQ bit was checked by jochen@chromium.org

[commit-bot: I haz the power, 2015-11-19 12:32:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1419313003/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1419313003/120001

[commit-bot: I haz the power, 2015-11-19 14:39:56]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-11-19 14:41:01]

Patchset 7 (id:??) landed as
https://crrev.com/aaf8bd48dfd77a6945f3cfd4c5b01ab631d956cd
Cr-Commit-Position: refs/heads/master@{#360578}