ScriptState used by EventListener::handleEvent() is wrong

Source/core/frame/LocalDOMWindow.cpp

Index: Source/core/frame/LocalDOMWindow.cpp
diff --git a/Source/core/frame/LocalDOMWindow.cpp b/Source/core/frame/LocalDOMWindow.cpp
index 0eb295eb957c3f684cf3e37a0385ced090dc1e5f..6497b0e0be294306c55a9fb2215736cd9cc33a12 100644
--- a/Source/core/frame/LocalDOMWindow.cpp
+++ b/Source/core/frame/LocalDOMWindow.cpp
@@ -34,6 +34,7 @@
 #include "bindings/core/v8/ScriptCallStackFactory.h"
 #include "bindings/core/v8/ScriptController.h"
 #include "bindings/core/v8/SerializedScriptValue.h"
+#include "bindings/core/v8/V8AbstractEventListener.h"
 #include "bindings/core/v8/V8DOMActivityLogger.h"
 #include "core/css/CSSComputedStyleDeclaration.h"
 #include "core/css/CSSRuleList.h"
@@ -1570,8 +1571,9 @@ DOMWindowCSS* LocalDOMWindow::css() const
     return m_css.get();
 }
 
-bool LocalDOMWindow::addEventListener(const AtomicString& eventType, PassRefPtr<EventListener> listener, bool useCapture)
+bool LocalDOMWindow::addEventListener(const AtomicString& eventType, PassRefPtr<EventListener> prpListener, bool useCapture)
 {
+    RefPtr<EventListener> listener = prpListener;
     if (!EventTarget::addEventListener(eventType, listener, useCapture))
         return false;
 
@@ -1588,6 +1590,18 @@ bool LocalDOMWindow::addEventListener(const AtomicString& eventType, PassRefPtr<
         UseCounter::count(document(), UseCounter::DocumentUnloadRegistered);
         addUnloadEventListener(this);
     } else if (eventType == EventTypeNames::beforeunload) {
+        v8::Isolate* isolate = v8::Isolate::GetCurrent();
+        if (isolate->InContext()) {
+            // This is weird code to keep compatibility.

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

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:46]

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

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

+            // When a beforeunload event is fired, a return value of the
+            // beforeunload event must be evaluated in a context that installed
+            // a beforeunload event listener. Thus we record the current
+            // ScriptState onto the event listener.
+            ScriptState* scriptState = ScriptState::current(v8::Isolate::GetCurrent());
+            ASSERT(scriptState);
+            static_cast<V8AbstractEventListener*>(listener.get())->setScriptStateForBeforeUnload(scriptState);
+        }
+
         UseCounter::count(document(), UseCounter::DocumentBeforeUnloadRegistered);
         if (allowsBeforeUnloadListeners(this)) {
             // This is confusingly named. It doesn't actually add the listener. It just increments a count