Dispatching errors across iframes don't match webplatform tests

Dispatching errors across iframes don't match webplatform tests

BUG=606900

[Anton Obzhirov, 2016-10-14 21:52:27]

a.obzhirov@samsung.com changed reviewers:
+ haraken@chromium.org, jochen@chromium.org

[Anton Obzhirov, 2016-10-14 21:52:27]

First attempt to check the validity of the approach, plz have a look.

[Anton Obzhirov, 2016-10-14 21:53:27]

Description was changed from

==========
Dispatching errors across iframes don't match webplatform tests

BUG=606900
==========

to

==========
Dispatching errors across iframes don't match webplatform tests

BUG=606900
==========

[Anton Obzhirov, 2016-10-14 21:53:27]

a.obzhirov@samsung.com changed reviewers:
+ dominicc@chromium.org

[haraken, 2016-10-15 02:25:25]

This change looks good.

Maybe do you also need to fix V8ErrorHandler::callListenerFunction?

BTW, we already have a logic to prevent an error event from being dispatched to
a wrong world
(https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/...).
Do you think we can merge the logic into this CL more nicely?

My point is that it's not really nice that we have scattered the logic to handle
error events in multiple places.

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

https://codereview.chromium.org/2420223002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp:148: ScriptState*
errorEventScriptState = context->errorEventScriptState();

Add a comment about what this is doing.

[dominicc (has gone to gerrit), 2016-10-16 08:45:57]

I have some ideas about how to improve this. As-is I think it looks a bit
fragile and might be broken in case of recursion. Comments inline.

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

https://codereview.chromium.org/2420223002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/V8EventListener.cpp:107:
scriptState->getExecutionContext()->setErrorEventScriptState(scriptStateForFunction);
I think you should write a stack allocated object which sets this script state
in its constructor and clears it in its destructor. The call site will be simple
and more reliable--we won't need to track all the returns; we will be less
likely to have bugs where the error event script state isn't reset.

I think you should simply *always* set and reset the script state (or maybe it
should be the context instead, since that's lighter?)

In addition, you need to CHECK that the error event script state isn't set/is
set to the same context when setting this. Because you set and clear it, if
there's a nested event listener call it will be reset too early. Could you add a
test for something like this?

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

https://codereview.chromium.org/2420223002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp:148: ScriptState*
errorEventScriptState = context->errorEventScriptState();
On 2016/10/15 at 02:25:25, haraken wrote:
> Add a comment about what this is doing.

This could be simpler if the errorEventScriptState always returns a script
state--one for the context if one was not explicitly set.

It may make sense to use context and not script state here since that's lighter.
haraken WDYT?

[haraken, 2016-10-17 01:13:27]

On 2016/10/16 08:45:57, dominicc wrote:
> I have some ideas about how to improve this. As-is I think it looks a bit
> fragile and might be broken in case of recursion. Comments inline.
> 
>
https://codereview.chromium.org/2420223002/diff/1/third_party/WebKit/Source/b...
> File third_party/WebKit/Source/bindings/core/v8/V8EventListener.cpp (right):
> 
>
https://codereview.chromium.org/2420223002/diff/1/third_party/WebKit/Source/b...
> third_party/WebKit/Source/bindings/core/v8/V8EventListener.cpp:107:
>
scriptState->getExecutionContext()->setErrorEventScriptState(scriptStateForFunction);
> I think you should write a stack allocated object which sets this script state
> in its constructor and clears it in its destructor. The call site will be
simple
> and more reliable--we won't need to track all the returns; we will be less
> likely to have bugs where the error event script state isn't reset.
> 
> I think you should simply *always* set and reset the script state (or maybe it
> should be the context instead, since that's lighter?)
> 
> In addition, you need to CHECK that the error event script state isn't set/is
> set to the same context when setting this. Because you set and clear it, if
> there's a nested event listener call it will be reset too early. Could you add
a
> test for something like this?
> 
>
https://codereview.chromium.org/2420223002/diff/1/third_party/WebKit/Source/b...
> File third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp (right):
> 
>
https://codereview.chromium.org/2420223002/diff/1/third_party/WebKit/Source/b...
> third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp:148: ScriptState*
> errorEventScriptState = context->errorEventScriptState();
> On 2016/10/15 at 02:25:25, haraken wrote:
> > Add a comment about what this is doing.
> 
> This could be simpler if the errorEventScriptState always returns a script
> state--one for the context if one was not explicitly set.
> 
> It may make sense to use context and not script state here since that's
lighter.
> haraken WDYT?

This sounds much nicer to me :)

You can add ScriptStateForErrorEvent::Scope,
ScriptStateForErrorEvent::getCurrent etc to ScriptState.h.

Bonus point if you can merge the world check
(https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/...)
into ScriptStateForErrorEvent.

[Anton Obzhirov, 2016-10-17 09:49:34]

Hi guys,

Thanks for review, will try to address all comments
including ScriptStateForErrorEvent and new test.

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

https://codereview.chromium.org/2420223002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/V8EventListener.cpp:107:
scriptState->getExecutionContext()->setErrorEventScriptState(scriptStateForFunction);
On 2016/10/16 08:45:57, dominicc wrote:
> I think you should write a stack allocated object which sets this script state
> in its constructor and clears it in its destructor. The call site will be
simple
> and more reliable--we won't need to track all the returns; we will be less
> likely to have bugs where the error event script state isn't reset.
> 
> I think you should simply *always* set and reset the script state (or maybe it
> should be the context instead, since that's lighter?)
> 
> In addition, you need to CHECK that the error event script state isn't set/is
> set to the same context when setting this. Because you set and clear it, if
> there's a nested event listener call it will be reset too early. Could you add
a
> test for something like this?
Yes, I was thinking about setting context as well. Good point about nested call,
will develop a test for that.

[Anton Obzhirov, 2016-10-27 14:21:18]

Hi guys, 

I've added new test and Scope, plz have a look. To handle nested events I have
now a vector of ScriptStateForErrorEvent in ExectionContext. I wonder if there
are other ways of dealing with the recursion. I haven't incorporated yet the
world check, will do it for the next iteration.

[haraken, 2016-10-27 14:56:27]

https://codereview.chromium.org/2420223002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp (right):

https://codereview.chromium.org/2420223002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp:148: ScriptState*
errorEventScriptState = context->errorEventScriptState();

Why do you need to associate the ScriptStateForErrorEvent with ExecutionContext?

Can we simply implement ScriptStateForErrorEvent::current() to get the top-most
ScriptStateForErrorEvent (without involving ExecutionContext)?

[Anton Obzhirov, 2016-10-28 09:45:36]

On 2016/10/27 14:56:27, haraken wrote:
>
https://codereview.chromium.org/2420223002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp (right):
> 
>
https://codereview.chromium.org/2420223002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp:148: ScriptState*
> errorEventScriptState = context->errorEventScriptState();
> 
> Why do you need to associate the ScriptStateForErrorEvent with
ExecutionContext?
> 
> Can we simply implement ScriptStateForErrorEvent::current() to get the
top-most
> ScriptStateForErrorEvent (without involving ExecutionContext)?

Yes, makes sense, will do.

[jochen (gone - plz use gerrit), 2016-10-28 13:40:54]

I'm deferring to haraken@ here

[dominicc (has gone to gerrit), 2016-10-31 00:50:26]

Could you add a link to the relevant part of the HTML spec, and maybe copy and
paste the text of the relevant step in a comment in the code, to make it clear
where this comes from?

[Anton Obzhirov, 2016-11-01 17:45:43]

On 2016/10/31 00:50:26, dominicc wrote:
> Could you add a link to the relevant part of the HTML spec, and maybe copy and
> paste the text of the relevant step in a comment in the code, to make it clear
> where this comes from?

Hi, I actually couldn't find the part of the spec so far. I've checked
https://html.spec.whatwg.org/. 
I wonder how the tests were originally added. I found
https://bugzilla.mozilla.org/show_bug.cgi?id=1259784
but there is no ref to the spec.

[Anton Obzhirov, 2016-11-01 17:47:56]

Hi guys, plz have a look.

[haraken, 2016-11-02 01:40:17]

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

[haraken, 2016-11-02 01:40:18]

Please add more explanation to the CL description.

I don't know where this behavior is speced. yukishiino@ might have thoughts.

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

https://codereview.chromium.org/2420223002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptState.h:193:
ScriptStateForErrorEvent(v8::Local<v8::Context> context)

Add explicit.

https://codereview.chromium.org/2420223002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptState.h:217:
ScopedPersistent<v8::Context> m_context;

You can use v8::Local<v8::Context> because ScriptStateForErrorEvent is used only
on the stack.

However, I'd prefer just storing RefPtr<ScriptState>. Then you don't need to
store v8::Isolate* since you can get it via m_scriptState->isolate().

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

https://codereview.chromium.org/2420223002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8ErrorHandler.cpp:64:
ScriptStateForErrorEvent scope(callFunction->CreationContext());

Add a comment about what this is doing.

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

https://codereview.chromium.org/2420223002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8EventListener.cpp:109:
ScriptStateForErrorEvent scope(handlerFunction->CreationContext());

Add a comment about what this is doing.

By the way, are you sure that we need to enter the scope only in
V8EventListener::callListenerFunction? I'm wondering if we should move the scope
into V8ScriptRunner::callFunction, because I guess this would be a generic
problem for all function calls from Blink to V8.

[Yuki, 2016-11-02 07:32:43]

No offence, but this CL seems doing a wrong thing, IIUC.

DOM spec: 3.9 Firing events
https://dom.spec.whatwg.org/#firing-events
step 2. Let event be the result of creating an event given eventConstructor, in
the relevant Realm of target.

I think you want to implement this behavior correctly.  Then, you should use the
relevant Realm for the event target object.  In general, there are multiple
event listeners for a single event, so it's not reasonable to create a new event
for each event listener.

By the way, if we're dispatching a single event to multiple event listeners that
came from different worlds, we have trouble here.  We may need to create an
event per world.

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

https://codereview.chromium.org/2420223002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8ErrorHandler.cpp:64:
ScriptStateForErrorEvent scope(callFunction->CreationContext());
nit: Avoid a general name "scope", which could be used HandleScope, etc.

[Anton Obzhirov, 2016-11-03 15:28:02]

Hi guys,

About the spec - I contacted Boris Zbarsky who was originally the author of the
tests. I will forward his reply to you.