ScriptState used by EventListener::handleEvent() is wrong

ScriptState used by EventListener::handleEvent() is wrong

This CL is a revert of r175241 (which was landed 6 months ago), essentially.

Currently an event listener is fired in a ScriptState that installed the event listener, but this is wrong. An event listener must be fired in a ScriptState that is calculated based on the ExecutionContext that fired the event listener and the world that installed the event listener. This is what we had been doing before r175241, and this CL restores the behavior. See an added test case for more details. The new behavior aligns with the spec and Firefox.

One tricky part is how to handle a beforeunload event. A return value of a beforeunload event needs to be fired in a ScriptState that installed the beforeunload event listener. To achieve the behavior, this CL records the ScriptState onto V8AbstractEventListener::m_scriptStateForBeforeUnload.

BUG=422227
TEST=event-should-be-dispatched-for-correct-frame.html,before-unload-return-bad-value.html

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=187861

[haraken, 2014-12-25 11:53:52]

haraken@chromium.org changed reviewers:
+ adamk@chromium.org, jochen@chromium.org, tasak@google.com

[haraken, 2014-12-25 11:53:58]

PTAL

[haraken, 2015-01-05 05:37:12]

Fixed the bot failures. PTAL.

[Jens Widell, 2015-01-05 10:00:58]

jl@opera.com changed reviewers:
+ jl@opera.com

[Jens Widell, 2015-01-05 10:00:59]

https://codereview.chromium.org/823263002/diff/80001/Source/bindings/core/v8/...
File Source/bindings/core/v8/V8AbstractEventListener.cpp (right):

https://codereview.chromium.org/823263002/diff/80001/Source/bindings/core/v8/...
Source/bindings/core/v8/V8AbstractEventListener.cpp:75: // the ExecutionContext
that fired the the event listener and the world
"the the"

https://codereview.chromium.org/823263002/diff/80001/Source/bindings/core/v8/...
File Source/bindings/core/v8/V8LazyEventListener.cpp (right):

https://codereview.chromium.org/823263002/diff/80001/Source/bindings/core/v8/...
Source/bindings/core/v8/V8LazyEventListener.cpp:113: // the ExecutionContext
that fired the the event listener and the world
"the the"

https://codereview.chromium.org/823263002/diff/80001/Source/core/frame/LocalD...
File Source/core/frame/LocalDOMWindow.cpp (right):

https://codereview.chromium.org/823263002/diff/80001/Source/core/frame/LocalD...
Source/core/frame/LocalDOMWindow.cpp:1595: // This is weird code to keep
compatibility.
What compatibility is this achieving? Do you have a test that fails without it?

I constructed the following test: a page with two frames, A and B. A script in
frame B sets top.onbeforeunload to a function from frame A, which returns an
object with a custom toString(), which returns the value of a global variable.

With or without this code, the global variable is accessed in frame A. (This
code triggers the toString() from frame B, I think?) This is expected; the
global scope for a function is determined when the function is compiled, not by
how it's called.

The one difference I could see is that if frame B (which installed the handler)
has been navigated when the handler is called, my regular Chrome apparently
fails to call the handler at all, a content_shell with this patch applied
crashes (due to the stored "installing script state" being invalid) and Firefox
calls the handler the same way as if frame B hadn't been navigated.

[haraken, 2015-01-05 10:04:47]

Thanks for reviewing the complicated CL...

https://codereview.chromium.org/823263002/diff/80001/Source/core/frame/LocalD...
File Source/core/frame/LocalDOMWindow.cpp (right):

https://codereview.chromium.org/823263002/diff/80001/Source/core/frame/LocalD...
Source/core/frame/LocalDOMWindow.cpp:1595: // This is weird code to keep
compatibility.
On 2015/01/05 10:00:59, Jens Widell wrote:
> What compatibility is this achieving? Do you have a test that fails without
it?
> 
> I constructed the following test: a page with two frames, A and B. A script in
> frame B sets top.onbeforeunload to a function from frame A, which returns an
> object with a custom toString(), which returns the value of a global variable.
> 
> With or without this code, the global variable is accessed in frame A. (This
> code triggers the toString() from frame B, I think?) This is expected; the
> global scope for a function is determined when the function is compiled, not
by
> how it's called.
> 
> The one difference I could see is that if frame B (which installed the
handler)
> has been navigated when the handler is called, my regular Chrome apparently
> fails to call the handler at all, a content_shell with this patch applied
> crashes (due to the stored "installing script state" being invalid) and
Firefox
> calls the handler the same way as if frame B hadn't been navigated.

before-unload-return-bad-value.html :)

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

[haraken, 2015-01-05 10:08:07]

On 2015/01/05 10:04:47, haraken wrote:
> Thanks for reviewing the complicated CL...
> 
>
https://codereview.chromium.org/823263002/diff/80001/Source/core/frame/LocalD...
> File Source/core/frame/LocalDOMWindow.cpp (right):
> 
>
https://codereview.chromium.org/823263002/diff/80001/Source/core/frame/LocalD...
> Source/core/frame/LocalDOMWindow.cpp:1595: // This is weird code to keep
> compatibility.
> On 2015/01/05 10:00:59, Jens Widell wrote:
> > What compatibility is this achieving? Do you have a test that fails without
> it?
> > 
> > I constructed the following test: a page with two frames, A and B. A script
in
> > frame B sets top.onbeforeunload to a function from frame A, which returns an
> > object with a custom toString(), which returns the value of a global
variable.
> > 
> > With or without this code, the global variable is accessed in frame A. (This
> > code triggers the toString() from frame B, I think?) This is expected; the
> > global scope for a function is determined when the function is compiled, not
> by
> > how it's called.
> > 
> > The one difference I could see is that if frame B (which installed the
> handler)
> > has been navigated when the handler is called, my regular Chrome apparently
> > fails to call the handler at all, a content_shell with this patch applied
> > crashes (due to the stored "installing script state" being invalid) and
> Firefox
> > calls the handler the same way as if frame B hadn't been navigated.
> 
> before-unload-return-bad-value.html :)
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

BTW, I'm a bit afraid that this CL is too complicated and thus risky to merge
into previous Chrome milestones. We should probably remove the onbeforeunload
hacks from the CL. If we remove the onbeforeunload hacks, it will cause
behavioral regressions, but it will just affect super edge-case behaviors like
before-unload-return-bad-value.html. (I'm not sure which is less risky.)

[Jens Widell, 2015-01-05 10:30:10]

On 2015/01/05 10:04:47, haraken wrote:
> Thanks for reviewing the complicated CL...
> 
>
https://codereview.chromium.org/823263002/diff/80001/Source/core/frame/LocalD...
> File Source/core/frame/LocalDOMWindow.cpp (right):
> 
>
https://codereview.chromium.org/823263002/diff/80001/Source/core/frame/LocalD...
> Source/core/frame/LocalDOMWindow.cpp:1595: // This is weird code to keep
> compatibility.
> On 2015/01/05 10:00:59, Jens Widell wrote:
> > What compatibility is this achieving? Do you have a test that fails without
> it?
> > 
> > I constructed the following test: a page with two frames, A and B. A script
in
> > frame B sets top.onbeforeunload to a function from frame A, which returns an
> > object with a custom toString(), which returns the value of a global
variable.
> > 
> > With or without this code, the global variable is accessed in frame A. (This
> > code triggers the toString() from frame B, I think?) This is expected; the
> > global scope for a function is determined when the function is compiled, not
> by
> > how it's called.
> > 
> > The one difference I could see is that if frame B (which installed the
> handler)
> > has been navigated when the handler is called, my regular Chrome apparently
> > fails to call the handler at all, a content_shell with this patch applied
> > crashes (due to the stored "installing script state" being invalid) and
> Firefox
> > calls the handler the same way as if frame B hadn't been navigated.
> 
> before-unload-return-bad-value.html :)
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Thanks. In terms of compatibility with Firefox (I can't test IE) we're only
partially compatible, with the result that we appear compatible from the POV of
that test.

AFAICT, Firefox calls the onerror handler defined in the scope of the
onbeforeunload handler (or perhaps more generally the scope of the code that
threw the exception.) In my test, that's frame A's onerror.

With this compatibility code, we're calling frame B's onerror. Without it, we
end up calling top.onerror. I guess achieving full compatibility here is a bit
involved. It also feels to me more like a fundamental incompatibility with our
onerror implementation (or Firefox's) than specifically with our onbeforeunload
implementation.

I'd be inclined to suggest we should drop this workaround for now and adjust the
test accordingly.

[Jens Widell, 2015-01-05 10:30:10]

On 2015/01/05 10:04:47, haraken wrote:
> Thanks for reviewing the complicated CL...
> 
>
https://codereview.chromium.org/823263002/diff/80001/Source/core/frame/LocalD...
> File Source/core/frame/LocalDOMWindow.cpp (right):
> 
>
https://codereview.chromium.org/823263002/diff/80001/Source/core/frame/LocalD...
> Source/core/frame/LocalDOMWindow.cpp:1595: // This is weird code to keep
> compatibility.
> On 2015/01/05 10:00:59, Jens Widell wrote:
> > What compatibility is this achieving? Do you have a test that fails without
> it?
> > 
> > I constructed the following test: a page with two frames, A and B. A script
in
> > frame B sets top.onbeforeunload to a function from frame A, which returns an
> > object with a custom toString(), which returns the value of a global
variable.
> > 
> > With or without this code, the global variable is accessed in frame A. (This
> > code triggers the toString() from frame B, I think?) This is expected; the
> > global scope for a function is determined when the function is compiled, not
> by
> > how it's called.
> > 
> > The one difference I could see is that if frame B (which installed the
> handler)
> > has been navigated when the handler is called, my regular Chrome apparently
> > fails to call the handler at all, a content_shell with this patch applied
> > crashes (due to the stored "installing script state" being invalid) and
> Firefox
> > calls the handler the same way as if frame B hadn't been navigated.
> 
> before-unload-return-bad-value.html :)
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Thanks. In terms of compatibility with Firefox (I can't test IE) we're only
partially compatible, with the result that we appear compatible from the POV of
that test.

AFAICT, Firefox calls the onerror handler defined in the scope of the
onbeforeunload handler (or perhaps more generally the scope of the code that
threw the exception.) In my test, that's frame A's onerror.

With this compatibility code, we're calling frame B's onerror. Without it, we
end up calling top.onerror. I guess achieving full compatibility here is a bit
involved. It also feels to me more like a fundamental incompatibility with our
onerror implementation (or Firefox's) than specifically with our onbeforeunload
implementation.

I'd be inclined to suggest we should drop this workaround for now and adjust the
test accordingly.

[haraken, 2015-01-05 10:34:15]

On 2015/01/05 10:30:10, Jens Widell wrote:
> On 2015/01/05 10:04:47, haraken wrote:
> > Thanks for reviewing the complicated CL...
> > 
> >
>
https://codereview.chromium.org/823263002/diff/80001/Source/core/frame/LocalD...
> > File Source/core/frame/LocalDOMWindow.cpp (right):
> > 
> >
>
https://codereview.chromium.org/823263002/diff/80001/Source/core/frame/LocalD...
> > Source/core/frame/LocalDOMWindow.cpp:1595: // This is weird code to keep
> > compatibility.
> > On 2015/01/05 10:00:59, Jens Widell wrote:
> > > What compatibility is this achieving? Do you have a test that fails
without
> > it?
> > > 
> > > I constructed the following test: a page with two frames, A and B. A
script
> in
> > > frame B sets top.onbeforeunload to a function from frame A, which returns
an
> > > object with a custom toString(), which returns the value of a global
> variable.
> > > 
> > > With or without this code, the global variable is accessed in frame A.
(This
> > > code triggers the toString() from frame B, I think?) This is expected; the
> > > global scope for a function is determined when the function is compiled,
not
> > by
> > > how it's called.
> > > 
> > > The one difference I could see is that if frame B (which installed the
> > handler)
> > > has been navigated when the handler is called, my regular Chrome
apparently
> > > fails to call the handler at all, a content_shell with this patch applied
> > > crashes (due to the stored "installing script state" being invalid) and
> > Firefox
> > > calls the handler the same way as if frame B hadn't been navigated.
> > 
> > before-unload-return-bad-value.html :)
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> Thanks. In terms of compatibility with Firefox (I can't test IE) we're only
> partially compatible, with the result that we appear compatible from the POV
of
> that test.
> 
> AFAICT, Firefox calls the onerror handler defined in the scope of the
> onbeforeunload handler (or perhaps more generally the scope of the code that
> threw the exception.) In my test, that's frame A's onerror.
> 
> With this compatibility code, we're calling frame B's onerror. Without it, we
> end up calling top.onerror. I guess achieving full compatibility here is a bit
> involved. It also feels to me more like a fundamental incompatibility with our
> onerror implementation (or Firefox's) than specifically with our
onbeforeunload
> implementation.
> 
> I'd be inclined to suggest we should drop this workaround for now and adjust
the
> test accordingly.

Sounds reasonable. Let me try.

[haraken, 2015-01-05 10:59:33]

On 2015/01/05 10:34:15, haraken wrote:
> On 2015/01/05 10:30:10, Jens Widell wrote:
> > On 2015/01/05 10:04:47, haraken wrote:
> > > Thanks for reviewing the complicated CL...
> > > 
> > >
> >
>
https://codereview.chromium.org/823263002/diff/80001/Source/core/frame/LocalD...
> > > File Source/core/frame/LocalDOMWindow.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/823263002/diff/80001/Source/core/frame/LocalD...
> > > Source/core/frame/LocalDOMWindow.cpp:1595: // This is weird code to keep
> > > compatibility.
> > > On 2015/01/05 10:00:59, Jens Widell wrote:
> > > > What compatibility is this achieving? Do you have a test that fails
> without
> > > it?
> > > > 
> > > > I constructed the following test: a page with two frames, A and B. A
> script
> > in
> > > > frame B sets top.onbeforeunload to a function from frame A, which
returns
> an
> > > > object with a custom toString(), which returns the value of a global
> > variable.
> > > > 
> > > > With or without this code, the global variable is accessed in frame A.
> (This
> > > > code triggers the toString() from frame B, I think?) This is expected;
the
> > > > global scope for a function is determined when the function is compiled,
> not
> > > by
> > > > how it's called.
> > > > 
> > > > The one difference I could see is that if frame B (which installed the
> > > handler)
> > > > has been navigated when the handler is called, my regular Chrome
> apparently
> > > > fails to call the handler at all, a content_shell with this patch
applied
> > > > crashes (due to the stored "installing script state" being invalid) and
> > > Firefox
> > > > calls the handler the same way as if frame B hadn't been navigated.
> > > 
> > > before-unload-return-bad-value.html :)
> > > 
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > 
> > Thanks. In terms of compatibility with Firefox (I can't test IE) we're only
> > partially compatible, with the result that we appear compatible from the POV
> of
> > that test.
> > 
> > AFAICT, Firefox calls the onerror handler defined in the scope of the
> > onbeforeunload handler (or perhaps more generally the scope of the code that
> > threw the exception.) In my test, that's frame A's onerror.
> > 
> > With this compatibility code, we're calling frame B's onerror. Without it,
we
> > end up calling top.onerror. I guess achieving full compatibility here is a
bit
> > involved. It also feels to me more like a fundamental incompatibility with
our
> > onerror implementation (or Firefox's) than specifically with our
> onbeforeunload
> > implementation.
> > 
> > I'd be inclined to suggest we should drop this workaround for now and adjust
> the
> > test accordingly.
> 
> Sounds reasonable. Let me try.

Updated the CL; PTAL.

[Jens Widell, 2015-01-05 11:06:26]

LGTM

[haraken, 2015-01-05 11:10:41]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2015-01-05 11:11:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/823263002/100001

[commit-bot: I haz the power, 2015-01-05 13:06:06]

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

[commit-bot: I haz the power, 2015-01-05 13:06:07]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/44765)

[haraken, 2015-01-05 14:59:30]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2015-01-05 15:00:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/823263002/100001

[commit-bot: I haz the power, 2015-01-05 15:58:39]

Committed patchset #6 (id:100001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=187861

[sof, 2015-01-06 08:52:04]

sigbjornf@opera.com changed reviewers:
+ sigbjornf@opera.com

[sof, 2015-01-06 08:52:04]

Seeing some fast/workers/termination-early.html crashes developing,


https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Linux_Oil...

http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Oilpan%...

might this be involved?

[sof, 2015-01-06 08:55:25]

https://codereview.chromium.org/823263002/diff/100001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8WorkerGlobalScopeEventListener.cpp (left):

https://codereview.chromium.org/823263002/diff/100001/Source/bindings/core/v8...
Source/bindings/core/v8/V8WorkerGlobalScopeEventListener.cpp:63: if
(!scriptState()->contextIsValid())
Why is this check no longer needed?

[Jens Widell, 2015-01-06 08:59:01]

https://codereview.chromium.org/823263002/diff/100001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8WorkerGlobalScopeEventListener.cpp (left):

https://codereview.chromium.org/823263002/diff/100001/Source/bindings/core/v8...
Source/bindings/core/v8/V8WorkerGlobalScopeEventListener.cpp:63: if
(!scriptState()->contextIsValid())
On 2015/01/06 08:55:24, sof wrote:
> Why is this check no longer needed?

It's in V8AbstractEventListener::handleEvent(ExecutionContext*, ...) now, which
calls this function.

[Jens Widell, 2015-01-06 09:28:59]

https://codereview.chromium.org/823263002/diff/100001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8WorkerGlobalScopeEventListener.cpp (right):

https://codereview.chromium.org/823263002/diff/100001/Source/bindings/core/v8...
Source/bindings/core/v8/V8WorkerGlobalScopeEventListener.cpp:66:
v8::Handle<v8::Value> jsEvent = toV8(event, scriptState->context()->Global(),
isolate());
I note that V8AbstractEventListener::handleEvent(ScriptState*, ...) has an

  if (jsEvent.IsEmpty())
    return;

check before the call to invokeEventHandler(). Prior to this patch,
invokeEventHandler() had the same check, but it was removed, so this code path
now has no check at all.

[haraken, 2015-01-06 09:37:32]

Thanks for the report. Looking (though I haven't yet succeeded in reproducing
the crash...)