Avoid leaking objects between isolated worlds via attribute event listeners

Source/core/dom/EventTarget.cpp

Index: Source/core/dom/EventTarget.cpp
diff --git a/Source/core/dom/EventTarget.cpp b/Source/core/dom/EventTarget.cpp
index c1ae36707aff494c2b79fe8117f9e86ac912a458..17eaaa93e214ebd61da5ba29eb2062d21147fc1c 100644
--- a/Source/core/dom/EventTarget.cpp
+++ b/Source/core/dom/EventTarget.cpp
@@ -32,6 +32,7 @@
 #include "config.h"
 #include "core/dom/EventTarget.h"
 
+#include "bindings/v8/DOMWrapperWorld.h"
 #include "bindings/v8/ScriptController.h"
 #include "core/dom/Event.h"
 #include "core/dom/ExceptionCode.h"
@@ -103,27 +104,33 @@ bool EventTarget::removeEventListener(const AtomicString& eventType, EventListen
     return true;
 }
 
-bool EventTarget::setAttributeEventListener(const AtomicString& eventType, PassRefPtr<EventListener> listener)
+bool EventTarget::setAttributeEventListener(const AtomicString& eventType, PassRefPtr<EventListener> listener, DOMWrapperWorld* world)
 {
-    clearAttributeEventListener(eventType);
+    clearAttributeEventListener(eventType, world);
     if (!listener)
         return false;
+    ASSERT(listener->world() == world);
     return addEventListener(eventType, listener, false);
 }
 
-EventListener* EventTarget::getAttributeEventListener(const AtomicString& eventType)
+EventListener* EventTarget::getAttributeEventListener(const AtomicString& eventType, DOMWrapperWorld* world)
 {
     const EventListenerVector& entry = getEventListeners(eventType);
     for (size_t i = 0; i < entry.size(); ++i) {
-        if (entry[i].listener->isAttribute())
-            return entry[i].listener.get();
+        if (entry[i].listener->isAttribute()) {
+            DOMWrapperWorld* listenerWorld = entry[i].listener->world();
+            if ((listenerWorld && listenerWorld->isMainWorld() && !world)

[adamk, 2013/06/13 00:18:09]

This is the really ugly part of the current change. I'm using null as a
placeholder for the main world in some parts of the code, but
mainThreadNormalWorld() elsewhere.

This ought to be easy to untangle, but I'm not sure which way to go. It seems
cleaner to use mainThreadNormalWorld() from a "no magic constants" point of
view, but I'd rather not be calling that where I can get by with 0 instead
(e.g., in the default arguments in EventTarget.h).

Maybe I should just go back to using worldId as I was doing at first? But the
advantage of using the world itself is, again, documentation (DOMWrapperWorld*
instead of int).

[abarth-chromium, 2013/06/13 00:30:44]

We talked a bit about this in person, and we realized that we don't want to call
mainThreadNormalWorld() all the time because we might be on a worker thread and
that's not going to be threadsafe.

I think you can rework this a bit to make it look prettier:

DOMWrapperWorld* listenerWorld = entry[i].listener->world();
if (!listenerWorld)
    continue;
if (listenerWorld->isMainWorld() && !world)
    world = mainThreadNormalWorld(); // We must be on the main thread, so blah
blah
if (listenerWorld == world)
    return entry[i].listener.get();

[adamk, 2013/06/13 18:55:07]

Updated the logic somewhat, though not exactly as you requested. See what you
think, I feel it's pretty clear now.

+                || (listenerWorld == world)) {
+                return entry[i].listener.get();
+            }
+        }
     }
     return 0;
 }
 
-bool EventTarget::clearAttributeEventListener(const AtomicString& eventType)
+bool EventTarget::clearAttributeEventListener(const AtomicString& eventType, DOMWrapperWorld* world)
 {
-    EventListener* listener = getAttributeEventListener(eventType);
+    EventListener* listener = getAttributeEventListener(eventType, world);
     if (!listener)
         return false;
     return removeEventListener(eventType, listener, false);