Avoid leaking objects between isolated worlds via attribute event listeners

Source/core/dom/EventTarget.cpp

 /*
  * Copyright (C) 1999 Lars Knoll (knoll@kde.org)
  *           (C) 1999 Antti Koivisto (koivisto@kde.org)
  *           (C) 2001 Dirk Mueller (mueller@kde.org)
  * Copyright (C) 2004, 2005, 2006, 2007 Apple Inc. All rights reserved.
  * Copyright (C) 2006 Alexey Proskuryakov (ap@webkit.org)
  *           (C) 2007, 2008 Nikolas Zimmermann <zimmermann@kde.org>
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ skipping 14 matching lines @@
  * PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY
  * OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
  * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
  * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. 
  *
  */
 
 #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"
 #include "core/inspector/InspectorInstrumentation.h"
 #include <wtf/MainThread.h>
 #include <wtf/StdLibExtras.h>
 #include <wtf/Vector.h>
 
 using namespace WTF;
 
@@ skipping 51 matching lines @@
             continue;
 
         --firingIterator.end;
         if (indexOfRemovedListener <= firingIterator.iterator)
             --firingIterator.iterator;
     }
 
     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);
 }
 
 bool EventTarget::dispatchEvent(PassRefPtr<Event> event, ExceptionCode& ec)
 {
     if (!event || event->type().isEmpty() || event->isBeingDispatched()) {
         ec = INVALID_STATE_ERR;
         return false;
@@ skipping 143 matching lines @@
     // they have one less listener to invoke.
     if (d->firingEventIterators) {
         for (size_t i = 0; i < d->firingEventIterators->size(); ++i) {
             d->firingEventIterators->at(i).iterator = 0;
             d->firingEventIterators->at(i).end = 0;
         }
     }
 }
 
 } // namespace WebCore