ScriptState used by EventListener::handleEvent() is wrong

Source/bindings/core/v8/V8WorkerGlobalScopeEventListener.cpp

Index: Source/bindings/core/v8/V8WorkerGlobalScopeEventListener.cpp
diff --git a/Source/bindings/core/v8/V8WorkerGlobalScopeEventListener.cpp b/Source/bindings/core/v8/V8WorkerGlobalScopeEventListener.cpp
index f8a9e32ae83357d021ea31268d0c0b78313c4233..35b035bb9cf5ba8f33d9d282692b875a28ce03e8 100644
--- a/Source/bindings/core/v8/V8WorkerGlobalScopeEventListener.cpp
+++ b/Source/bindings/core/v8/V8WorkerGlobalScopeEventListener.cpp
@@ -50,34 +50,32 @@ V8WorkerGlobalScopeEventListener::V8WorkerGlobalScopeEventListener(v8::Local<v8:
 {
 }
 
-void V8WorkerGlobalScopeEventListener::handleEvent(ExecutionContext*, Event* event)
+void V8WorkerGlobalScopeEventListener::handleEvent(ScriptState* scriptState, Event* event)
 {
     // The callback function on XMLHttpRequest can clear the event listener and destroys 'this' object. Keep a local reference to it.
     // See issue 889829.
     RefPtr<V8AbstractEventListener> protect(this);
 
-    WorkerScriptController* script = toWorkerGlobalScope(scriptState()->executionContext())->script();
+    WorkerScriptController* script = toWorkerGlobalScope(scriptState->executionContext())->script();
     if (!script)
         return;
 
-    if (!scriptState()->contextIsValid())

[sof, 2015/01/06 08:55:24]

Why is this check no longer needed?

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

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

-        return;
-    ScriptState::Scope scope(scriptState());
+    ScriptState::Scope scope(scriptState);
 
     // Get the V8 wrapper for the event object.
-    v8::Handle<v8::Value> jsEvent = toV8(event, scriptState()->context()->Global(), isolate());
+    v8::Handle<v8::Value> jsEvent = toV8(event, scriptState->context()->Global(), isolate());

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

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.

 
-    invokeEventHandler(event, v8::Local<v8::Value>::New(isolate(), jsEvent));
+    invokeEventHandler(scriptState, event, v8::Local<v8::Value>::New(isolate(), jsEvent));
 }
 
-v8::Local<v8::Value> V8WorkerGlobalScopeEventListener::callListenerFunction(v8::Handle<v8::Value> jsEvent, Event* event)
+v8::Local<v8::Value> V8WorkerGlobalScopeEventListener::callListenerFunction(ScriptState* scriptState, v8::Handle<v8::Value> jsEvent, Event* event)
 {
-    v8::Local<v8::Function> handlerFunction = getListenerFunction(scriptState()->executionContext());
-    v8::Local<v8::Object> receiver = getReceiverObject(event);
+    v8::Local<v8::Function> handlerFunction = getListenerFunction(scriptState);
+    v8::Local<v8::Object> receiver = getReceiverObject(scriptState, event);
     if (handlerFunction.IsEmpty() || receiver.IsEmpty())
         return v8::Local<v8::Value>();
 
-    TRACE_EVENT1(TRACE_DISABLED_BY_DEFAULT("devtools.timeline"), "FunctionCall", "data", devToolsTraceEventData(isolate(), scriptState()->executionContext(), handlerFunction));
+    TRACE_EVENT1(TRACE_DISABLED_BY_DEFAULT("devtools.timeline"), "FunctionCall", "data", devToolsTraceEventData(isolate(), scriptState->executionContext(), handlerFunction));
     // FIXME(361045): remove InspectorInstrumentation calls once DevTools Timeline migrates to tracing.
     InspectorInstrumentationCookie cookie;
     if (InspectorInstrumentation::hasFrontends()) {
@@ -85,11 +83,11 @@ v8::Local<v8::Value> V8WorkerGlobalScopeEventListener::callListenerFunction(v8::
         String resourceName;
         int lineNumber = 1;
         GetDevToolsFunctionInfo(handlerFunction, isolate(), scriptId, resourceName, lineNumber);
-        cookie = InspectorInstrumentation::willCallFunction(scriptState()->executionContext(), scriptId, resourceName, lineNumber);
+        cookie = InspectorInstrumentation::willCallFunction(scriptState->executionContext(), scriptId, resourceName, lineNumber);
     }
 
     v8::Handle<v8::Value> parameters[1] = { jsEvent };
-    v8::Local<v8::Value> result = V8ScriptRunner::callFunction(handlerFunction, scriptState()->executionContext(), receiver, WTF_ARRAY_LENGTH(parameters), parameters, isolate());
+    v8::Local<v8::Value> result = V8ScriptRunner::callFunction(handlerFunction, scriptState->executionContext(), receiver, WTF_ARRAY_LENGTH(parameters), parameters, isolate());
 
     InspectorInstrumentation::didCallFunction(cookie);
     TRACE_EVENT_INSTANT1(TRACE_DISABLED_BY_DEFAULT("devtools.timeline"), "UpdateCounters", "data", InspectorUpdateCountersEvent::data());
@@ -97,15 +95,17 @@ v8::Local<v8::Value> V8WorkerGlobalScopeEventListener::callListenerFunction(v8::
     return result;
 }
 
-v8::Local<v8::Object> V8WorkerGlobalScopeEventListener::getReceiverObject(Event* event)
+// FIXME: Remove getReceiverObject().
+// This is almost identical to V8AbstractEventListener::getReceiverObject().
+v8::Local<v8::Object> V8WorkerGlobalScopeEventListener::getReceiverObject(ScriptState* scriptState, Event* event)
 {
-    v8::Local<v8::Object> listener = getListenerObject(scriptState()->executionContext());
+    v8::Local<v8::Object> listener = getListenerObject(scriptState->executionContext());
 
     if (!listener.IsEmpty() && !listener->IsFunction())
         return listener;
 
     EventTarget* target = event->currentTarget();
-    v8::Handle<v8::Value> value = toV8(target, scriptState()->context()->Global(), isolate());
+    v8::Handle<v8::Value> value = toV8(target, scriptState->context()->Global(), isolate());
     if (value.IsEmpty())
         return v8::Local<v8::Object>();
     return v8::Local<v8::Object>::New(isolate(), v8::Handle<v8::Object>::Cast(value));