Avoid leaking objects between isolated worlds via attribute event listeners

Avoid leaking objects between isolated worlds via attribute event listeners

This patch adds an isolatedWorld argument to the setters and getters
for attribute event listeners (e.g., document.onload). When called
from the bindings, this is appropriately set to either the current
isolated world or null (which can be the case for either the main
world or a worker). When these methods are called from within Blink,
we always default to the "normal" (main or worker) case.

R=abarth
BUG=87520
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=152377

[adamk, 2013-06-13 00:18:08]

https://codereview.chromium.org/16904002/diff/1/Source/core/dom/EventTarget.cpp
File Source/core/dom/EventTarget.cpp (right):

https://codereview.chromium.org/16904002/diff/1/Source/core/dom/EventTarget.c...
Source/core/dom/EventTarget.cpp:122: if ((listenerWorld &&
listenerWorld->isMainWorld() && !world)
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]

LGTM

https://codereview.chromium.org/16904002/diff/1/Source/core/dom/EventTarget.cpp
File Source/core/dom/EventTarget.cpp (right):

https://codereview.chromium.org/16904002/diff/1/Source/core/dom/EventTarget.c...
Source/core/dom/EventTarget.cpp:122: if ((listenerWorld &&
listenerWorld->isMainWorld() && !world)
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();

[abarth-chromium, 2013-06-13 00:31:00]

(Please update the description with something more descriptive.)

[marja, 2013-06-13 13:56:29]

https://codereview.chromium.org/16904002/diff/1/Source/bindings/v8/V8Binding.cpp
File Source/bindings/v8/V8Binding.cpp (right):

https://codereview.chromium.org/16904002/diff/1/Source/bindings/v8/V8Binding....
Source/bindings/v8/V8Binding.cpp:436: DOMWrapperWorld*
worldForIsolate(v8::Isolate* isolate)
The name is slightly confusing, since this only returns non-NULL isolated worlds
but never the main world. isolateWorldForIsolate instead (there's also:
DOMWrapperWorld::isolatedWorld(...) so it would fit).

https://codereview.chromium.org/16904002/diff/1/Source/bindings/v8/V8Binding....
Source/bindings/v8/V8Binding.cpp:444: if (DOMWrapperWorld* isolatedWorld =
DOMWrapperWorld::isolatedWorld(v8::Context::GetEntered()))
Shorter (instead of these 3 lines):

return DOMWrapperWorld::isolatedWorld(v8::Context::GetEntered());

[adamk, 2013-06-13 18:55:06]

Thanks for the review, I've updated the change description and taken care of the
comments from the first round.

https://codereview.chromium.org/16904002/diff/1/Source/bindings/v8/V8Binding.cpp
File Source/bindings/v8/V8Binding.cpp (right):

https://codereview.chromium.org/16904002/diff/1/Source/bindings/v8/V8Binding....
Source/bindings/v8/V8Binding.cpp:436: DOMWrapperWorld*
worldForIsolate(v8::Isolate* isolate)
On 2013/06/13 13:56:29, marja wrote:
> The name is slightly confusing, since this only returns non-NULL isolated
worlds
> but never the main world. isolateWorldForIsolate instead (there's also:
> DOMWrapperWorld::isolatedWorld(...) so it would fit).

Renamed to isolatedWorldForIsolate(). Also changed the argument passed
everywhere to EventTarget to be "isolatedWorld", which I think makes all the
code clearer (if more verbose).

https://codereview.chromium.org/16904002/diff/1/Source/bindings/v8/V8Binding....
Source/bindings/v8/V8Binding.cpp:444: if (DOMWrapperWorld* isolatedWorld =
DOMWrapperWorld::isolatedWorld(v8::Context::GetEntered()))
On 2013/06/13 13:56:29, marja wrote:
> Shorter (instead of these 3 lines):
> 
> return DOMWrapperWorld::isolatedWorld(v8::Context::GetEntered());

Fixed, this was an artifact of a previous version of the patch.

https://codereview.chromium.org/16904002/diff/1/Source/core/dom/EventTarget.cpp
File Source/core/dom/EventTarget.cpp (right):

https://codereview.chromium.org/16904002/diff/1/Source/core/dom/EventTarget.c...
Source/core/dom/EventTarget.cpp:122: if ((listenerWorld &&
listenerWorld->isMainWorld() && !world)
On 2013/06/13 00:30:44, abarth wrote:
> 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();

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

[abarth-chromium, 2013-06-13 19:16:59]

LGTM

yeah, your version of that stanza is better.  :)

[commit-bot: I haz the power, 2013-06-13 19:19:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/adamk@chromium.org/16904002/25001

[commit-bot: I haz the power, 2013-06-13 20:52:52]

Change committed as 152377