Require DOMWrapperWorld in event handler macros

Require DOMWrapperWorld in event handler macros

BUG=none

[philipj_slow, 2013-10-26 20:31:49]

haraken, this was the consequence of dropping the "= 0" where you pointed. I
have no idea what should be passed instead, so please tell me what to do :)

setAttributeEventListener and getAttributeEventListener in this file also make
DOMWrapperWorld optional, should that be fixed as well? In both cases, if it is
required and mustn't be null, should it just be a reference?

[haraken, 2013-10-27 02:11:57]

Thanks philipj.

> setAttributeEventListener and getAttributeEventListener in this file also make
> DOMWrapperWorld optional, should that be fixed as well?

Good idea.

> In both cases, if it is
> required and mustn't be null, should it just be a reference?

Since |isolatedWorld| is an integer, you cannot use a reference. Instead you can
use MainWorldId (which is defined as 0 in DOMWrapperWorld.h).

[haraken, 2013-10-27 02:12:04]

https://codereview.chromium.org/45973006/diff/1/Source/core/inspector/Inspect...
File Source/core/inspector/InspectorFileSystemAgent.cpp (right):

https://codereview.chromium.org/45973006/diff/1/Source/core/inspector/Inspect...
Source/core/inspector/InspectorFileSystemAgent.cpp:475:
m_reader->setOnload(this, 0);

You could use MainWorldId instead of 0.

[philipj_slow, 2013-10-27 08:18:53]

On 2013/10/27 02:11:57, haraken wrote:
> Thanks philipj.
> 
> > setAttributeEventListener and getAttributeEventListener in this file also
make
> > DOMWrapperWorld optional, should that be fixed as well?
> 
> Good idea.
> 
> > In both cases, if it is
> > required and mustn't be null, should it just be a reference?
> 
> Since |isolatedWorld| is an integer, you cannot use a reference. Instead you
can
> use MainWorldId (which is defined as 0 in DOMWrapperWorld.h).

[philipj_slow, 2013-10-27 08:22:21]

On 2013/10/27 02:11:57, haraken wrote:
> Thanks philipj.
> 
> > setAttributeEventListener and getAttributeEventListener in this file also
make
> > DOMWrapperWorld optional, should that be fixed as well?
> 
> Good idea.
> 
> > In both cases, if it is
> > required and mustn't be null, should it just be a reference?
> 
> Since |isolatedWorld| is an integer, you cannot use a reference. Instead you
can
> use MainWorldId (which is defined as 0 in DOMWrapperWorld.h).

|isolatedWorld| is a DOMWrapperWorld*, which has an integer |m_worldId|, but as
far as I can tell these aren't interchangeable and MainWorldId isn't used
anywhere outside of Source/bindings/.

Would using the static DOMWrapperWorld::current() work, and be at all correct?

[haraken, 2013-10-27 12:05:25]

> |isolatedWorld| is a DOMWrapperWorld*, which has an integer |m_worldId|, but
as
> far as I can tell these aren't interchangeable and MainWorldId isn't used
> anywhere outside of Source/bindings/.
> 
> Would using the static DOMWrapperWorld::current() work, and be at all correct?

Sorry, you're right. As far as I read EventTarget, |isolatedWorld| is used only
by EventTarget::getAttributeEventListener. And
EventTarget::getAttributeEventListener expects that |isolatedWorld| is set to
the isolated world of the current context. This means that we should pass
DOMWrapperWorld::current() to |isolatedWorld|. (In fact, auto-generated code is
already doing this.)

[philipj_slow, 2013-10-27 20:58:44]

On 2013/10/27 12:05:25, haraken wrote:
> > |isolatedWorld| is a DOMWrapperWorld*, which has an integer |m_worldId|, but
> as
> > far as I can tell these aren't interchangeable and MainWorldId isn't used
> > anywhere outside of Source/bindings/.
> > 
> > Would using the static DOMWrapperWorld::current() work, and be at all
correct?
> 
> Sorry, you're right. As far as I read EventTarget, |isolatedWorld| is used
only
> by EventTarget::getAttributeEventListener. And
> EventTarget::getAttributeEventListener expects that |isolatedWorld| is set to
> the isolated world of the current context. This means that we should pass
> DOMWrapperWorld::current() to |isolatedWorld|. (In fact, auto-generated code
is
> already doing this.)

I've switched to using DOMWrapperWorld::current() now.

For set/getAttributeEventListener, there are a lot more places where
DOMWrapperWorld::current() would have to be added... What (potential) bug would
this fix, and is the difference testable in LayoutTests?

[haraken, 2013-10-28 00:27:47]

> What (potential) bug would this fix, and is the difference testable in
LayoutTests?

In case of IDBObjectStore, I guess that the following potential bug exists:

(1) IDBObjectStore::createIndex is called from an isolated world (e.g., content
script of Chrome extension).
(2) IDBObjectStore::createIndex calls indexRequest->setOnsuccess(indexPopulator,
0). "0" means the main world.
(3) setOnsuccess calls EventTarget::clearAttributeEventListener.
(4) EventTarget::clearAttributeEventListener will clear an event listener that
was registered in the main world.
(5) The main world will be surprised because the event listener of the main
world has disappeared.

I think you can test it in LayoutTests using evaluateScriptInIsolatedWorld().
(Please grep evaluateScriptInIsolatedWorld() in existing LayoutTests.)

[alecflett, 2013-10-28 20:29:54]

On 2013/10/28 00:27:47, haraken wrote:
> > What (potential) bug would this fix, and is the difference testable in
> LayoutTests?
> 
> In case of IDBObjectStore, I guess that the following potential bug exists:
> 
> (1) IDBObjectStore::createIndex is called from an isolated world (e.g.,
content
> script of Chrome extension).
> (2) IDBObjectStore::createIndex calls
indexRequest->setOnsuccess(indexPopulator,
> 0). "0" means the main world.
> (3) setOnsuccess calls EventTarget::clearAttributeEventListener.
> (4) EventTarget::clearAttributeEventListener will clear an event listener that
> was registered in the main world.
> (5) The main world will be surprised because the event listener of the main
> world has disappeared.
> 
> I think you can test it in LayoutTests using evaluateScriptInIsolatedWorld().
> (Please grep evaluateScriptInIsolatedWorld() in existing LayoutTests.)


This patch looks fine from IDB's perspective but I offer this explanation in
case there is a better/simpler/more consistent approach:

The use of createIndex() is fairly unique here - as indexePopulator is a
C++-based event listener, not a JS event listener, not exposed to script.

The onsuccess handler will be called multiple times by IPC messages that
originate in the browser process, and the renderer will respond only by sending
data back to the browser. These messages from the browser process are a payload
that includes a SerializedScript value. The onsuccess handler needs to
deserialize the script value, extract a value using the V8 APIs
(IDBBindings.cpp) and reserialize the extracted value back into a
SerializedScriptValue. This value is in turn sent back to the browser process.

The deserialization/extraction/serialization process is not observable to the
createIndex() caller, (in fact it happens after createIndex() has returned, and
if it was observable, this would be a bug) Because of this, this process *could*
be run in any world, and potentially any context (there was a time we even ran
it in a separate process!)

I hope that is helpful.

[philipj_slow, 2013-11-04 08:00:54]

Sorry for letting this sit for so long, but I don't really know what to do with
it. The internal use of event handlers and macros that are otherwise only used
for bindings looks really weird. If these events are fired internally and
handled internally, isn't the proper thing to just use an internal API, instead
of masquerading as an attribute event handler?

[philipj_slow, 2013-11-13 14:44:04]

Since this has been sitting with no progress for over a week I am going to close
and abandon it now. Maybe someone with a better understanding of the root
problem can pick this up, if it seems important.