V8AbstractEventListener should hold a ScriptState

V8AbstractEventListener should hold a ScriptState

- Basically, this CL replaces V8AbstractEventListener::m_world with V8AbstractEventListener::m_scriptState.

- This CL uses the ScriptState in a lot of places about event listeners and cleans up the code (for example, this CL removes an empty creation context from toV8()).

- The complicated part is that V8LazyEventListener does not know the ScriptState when it's created. Thus, V8AbstractEventListener::m_scriptState can be null if the listener is a V8LazyEventListener until the V8LazyEventListener is called back from Blink (i.e., either of handleEvent(), callListenerFunction() and getEventListener() is called).

- This CL changes web-exposed behavior. In a case where the frame that registered an onload event handler (say Frame1) is different from the frame that triggers the onload event (Frame2), before this CL, the onload event handler is invoked in the context of Frame2. After this CL, the onload event handler is not triggered if Frame1 != Frame2. This behavior is not speced but aligns with Firefox. See comment #17 and #19 for more details.

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

[haraken, 2014-05-21 10:37:42]

PTAL

[dcarney, 2014-05-21 10:41:35]

lgtm

[adamk, 2014-05-21 10:51:05]

lgtm

https://codereview.chromium.org/293053007/diff/20001/Source/bindings/v8/V8Abs...
File Source/bindings/v8/V8AbstractEventListener.cpp (left):

https://codereview.chromium.org/293053007/diff/20001/Source/bindings/v8/V8Abs...
Source/bindings/v8/V8AbstractEventListener.cpp:108: if (v8Context.IsEmpty())
I take it you removed this check because you've updated the callers to do it
instead? Can you then add an ASSERT(!scriptState()->contextIsEmpty())?

[haraken, 2014-05-21 11:15:31]

https://codereview.chromium.org/293053007/diff/20001/Source/bindings/v8/V8Abs...
File Source/bindings/v8/V8AbstractEventListener.cpp (left):

https://codereview.chromium.org/293053007/diff/20001/Source/bindings/v8/V8Abs...
Source/bindings/v8/V8AbstractEventListener.cpp:108: if (v8Context.IsEmpty())
On 2014/05/21 10:51:06, adamk wrote:
> I take it you removed this check because you've updated the callers to do it
> instead? Can you then add an ASSERT(!scriptState()->contextIsEmpty())?

Yes. Added the ASSERT.

[haraken, 2014-05-21 11:15:35]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-05-21 11:16:38]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/haraken@chromium.org/293053007/60001

[haraken, 2014-05-21 11:41:09]

+tasak: FYI.

[commit-bot: I haz the power, 2014-05-21 17:54:23]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/8429)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/8526)
  linux_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)

[commit-bot: I haz the power, 2014-05-21 17:57:18]

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

[commit-bot: I haz the power, 2014-05-21 17:57:19]

Try jobs failed on following builders:
  linux_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)

[haraken, 2014-05-21 17:57:47]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-05-21 17:59:47]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/haraken@chromium.org/293053007/60001

[commit-bot: I haz the power, 2014-05-21 23:54:39]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/9144)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/8960)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/8520)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/8557)

[commit-bot: I haz the power, 2014-05-22 00:40:22]

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

[commit-bot: I haz the power, 2014-05-22 00:40:22]

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

[adamk, 2014-05-22 07:10:44]

These look like real failures too...

[haraken, 2014-05-29 05:50:39]

adamk@, dcarney@: I removed the failing two tests, because this CL is changing
behavior and I think the new behavior is correct (and then the tests become
invalid). See my comment in the CL for more details.

PTAL.

https://codereview.chromium.org/293053007/diff/80001/LayoutTests/fast/dom/res...
File
LayoutTests/fast/dom/resources/set-frame-src-while-running-script-in-frame.html
(left):

https://codereview.chromium.org/293053007/diff/80001/LayoutTests/fast/dom/res...
LayoutTests/fast/dom/resources/set-frame-src-while-running-script-in-frame.html:5:
f.src="about:blank"

This test is testing the following things.

(1) f.onload sets the event listener on this frame (say Frame1).
(2) f.src="about:blank" is loaded into this iframe. A new Frame object is
created (say Frame2).
(3) f.onload is called. The test is checking if it doesn't crash.

Before this CL, this sequence was working because when we were invoking f.onload
using the ExecutionContext of Frame2. (i.e.,
V8AbstractEventListener::invokeEventHandler)

After this CL, f.onload is not called because we try to invoke f.onload using
the ExecutionContext of Frame1. This is because we store the current ScriptState
in (1) and use the ScriptState's executionContext in (3). Because the Frame1 is
already gone, we cannot call f.onload. (i.e.,
V8AbstractEventListener::invokeEventHandler returns before invoking the event
listener.)

- I think the new behavior is more reasonable.
- I confirmed that Firefox aligns with the new behavior.
- As far as I know, the behavior is not speced (there is a W3C bug:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=24403).

Given the above, this CL makes the behavior change and removes this test
(because this test is no longer testing something meaningful).

https://codereview.chromium.org/293053007/diff/80001/LayoutTests/fast/dom/set...
File LayoutTests/fast/dom/set-frame-src-while-running-script-in-frame.html
(left):

https://codereview.chromium.org/293053007/diff/80001/LayoutTests/fast/dom/set...
LayoutTests/fast/dom/set-frame-src-while-running-script-in-frame.html:8: <iframe
src="resources/set-frame-src-while-running-script-in-frame.html"></iframe>

I'm removing this test for the same reason as
set-frame-src-while-running-script-in-frame.html.

[adamk, 2014-05-29 23:33:58]

I think I agree with you that for this test, the new behavior is acceptable (and
maybe makes more sense).

But I'm not so sure that it doesn't point to a different problem (the same one
discussed in https://www.w3.org/Bugs/Public/show_bug.cgi?id=24403): what should
the "entry settings object" (the "Entered Context" in V8 language) be for event
dispatch?

Let's call the outer page Frame0 (that's the page on which the <iframe> tag
that's being manipulated lives).

Are you sure that the previous behavior was to call the onload handler with
Frame2 as the entered context? Would it not make more sense for the entered
context to be Frame0, since it's an event on an element that lives in a document
whose frame is Frame0? If _that_ was the previous behavior, I'm not sure how
safe it is to change it, since that could effect even cases where navigation
wasn't happening.

In short, I'd like to be sure that you've verified exactly why we have the
current behavior before changing it.

[haraken, 2014-05-30 04:36:45]

On 2014/05/29 23:33:58, adamk wrote:
> I think I agree with you that for this test, the new behavior is acceptable
(and
> maybe makes more sense).
> 
> But I'm not so sure that it doesn't point to a different problem (the same one
> discussed in https://www.w3.org/Bugs/Public/show_bug.cgi?id=24403): what
should
> the "entry settings object" (the "Entered Context" in V8 language) be for
event
> dispatch?
> 
> Let's call the outer page Frame0 (that's the page on which the <iframe> tag
> that's being manipulated lives).
> 
> Are you sure that the previous behavior was to call the onload handler with
> Frame2 as the entered context? Would it not make more sense for the entered
> context to be Frame0, since it's an event on an element that lives in a
document
> whose frame is Frame0? If _that_ was the previous behavior, I'm not sure how
> safe it is to change it, since that could effect even cases where navigation
> wasn't happening.
> 
> In short, I'd like to be sure that you've verified exactly why we have the
> current behavior before changing it.

Good point. I confirmed that the previous behavior is setting Frame2 (not
Frame0) as an entered context. I don't think this is right, and the new behavior
sounds much more reasonable. Firefox follows the new behavior.

// main.html
<iframe src="iframe.html"></iframe>
<script>
window.ppp = "ppp";
</script>

// iframe.html
<script>
var f = parent.document.querySelector("iframe");
f.onload = function() {  // the new behavior doesn't invoke this handler
  console.log(window.ppp);  // undefined (the previous behavior)
  console.log(parent.ppp);  // "ppp" (the previous behavior)
}
f.src = "about:blank";
</script>


> Would it not make more sense for the entered
> context to be Frame0, since it's an event on an element that lives in a
document
> whose frame is Frame0?

I don't think it makes much sense. In the above example, if you don't have
'f.src = "about:blank"', the onload handler is called on Frame1. If you have
'f.src = "about:blank"',

(a) the onload handler is called on Frame2 currently.
(b) the onload handler is called on Frame0 in your proposal.
(c) the onload handler is not called after my CL.

Since it wouldn't make sense to change the Frame on which the onload handler is
called depending on whether we have 'f.src = "about:blank"' or not, I'd buy (c).

What do you think?

[haraken, 2014-05-30 04:38:36]

Note:

- set-frame-src-while-running-script-in-frame.html is introduced in
http://src.chromium.org/viewvc/blink?revision=19094&view=revision

- fast/dom/xmlhttprequest-constructor-in-detached-document.html is introduced in
http://src.chromium.org/viewvc/blink?revision=42700&view=revision

Both CLs are just for fixing crashes, and not intending to correct contexts.

[adamk, 2014-05-30 20:03:46]

lgt, I think you've successfully convinced me that this new behavior is
reasonable. Please make a note in the CL description of the behavior you're
changing.

[adamk, 2014-05-30 20:03:53]

er, lgtm

[haraken, 2014-06-02 00:40:38]

On 2014/05/30 20:03:46, adamk wrote:
> lgt, I think you've successfully convinced me that this new behavior is
> reasonable. Please make a note in the CL description of the behavior you're
> changing.

Done.

[haraken, 2014-06-02 00:40:44]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-06-02 00:41:01]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/haraken@chromium.org/293053007/120001

[commit-bot: I haz the power, 2014-06-02 01:17:52]

Change committed as 175241