[webkit-dev] Question about Constructors in WebKit JS Bindings

[Drew Wilson]

OK, coming back around to this - I'm looking at the automatically generated
constructors. As an example, let's look at something simple like
EventException.
    JSEventExceptionConstructor(ExecState* exec)
        :
DOMObject(JSEventExceptionConstructor::createStructure(exec->lexicalGlobalObject()->objectPrototype()))
    {
        putDirect(exec->propertyNames().prototype,
JSEventExceptionPrototype::self(exec, *exec->lexicalGlobalObject()*), None);
    }

It looks like this uses the current lexicalGlobalObject when constructing
the event exceptions prototype chain. Furthermore,
JSDOMGlobalObject::getDOMConstructor() and JSDOMBinding.cpp both use the
lexical global object to cache constructors.

So in the case the Maciej describes below, it *looks* like for
auto-generated constructors, the window you grab the constructor from is
immaterial - it always looks up/caches the constructor object in lexical
global scope (i.e. referencing window.EventExecutor returns the same value
for a given lexical scope, no matter what "window" refers to) so his bug
won't actually happen for his case.

I guess this behavior might be a bug, given this bugzilla entry:
https://bugs.webkit.org/show_bug.cgi?id=21138, but I'm somewhat uncertain
about that conclusion as it would mean that 95% of our constructors are
buggy. Anyone want to confirm this?

The other place where problems happen are for custom constructors that are
passed a reference to a JSDOMGlobalObject - in these situations we cache the
constructor on the passed-in JSDOMGlobalObject itself rather than on the
lexicalGlobalObject. So it seems incorrect to do this:

JSMessageChannelConstructor::JSMessageChannelConstructor(ExecState* exec,
JSDOMGlobalObject* globalObject)
    : DOMObject(JSMessageChannelConstructor::createStructure(*
exec->lexicalGlobalObject()*->objectPrototype()))
    , m_globalObject(globalObject)
{
    putDirect(exec->propertyNames().prototype,
JSMessageChannelPrototype::self(exec, *exec->lexicalGlobalObject()*), None);
}

Note that the constructor prototype is being generated using
lexicalGlobalObject, but then the result is being cached in the passed-in
globalObject, leaving you with a reference in "globalObject" to a
constructor with a prototype chain that might be from a foreign context
(maciej's bug he described previously).

So it seems like we should never reference lexicalGlobalObject in our
constructor/prototype creation code at all. if I invoke "new
otherWindow.MessageChannel()", I should get a MessageChannel object whose
prototype chain == otherWindow.MessageChannel.prototype. So the constructor
should look like this instead:

JSMessageChannelConstructor::JSMessageChannelConstructor(ExecState* exec,
JSDOMGlobalObject* globalObject)
    : DOMObject(JSMessageChannelConstructor::createStructure(*globalObject*
->objectPrototype()))
    , m_globalObject(globalObject)
{
    putDirect(exec->propertyNames().prototype,
JSMessageChannelPrototype::self(exec, *globalObject*), None);
}

Sorry to belabor the point, but I want to make sure I don't propagate
invalid behavior in new code I'm writing, but I'm having trouble finding
*any* constructors that are doing the right thing currently, so I figure I
must be missing something :)

-atw

On Tue, Jun 23, 2009 at 5:14 PM, Adam Barth <abarth at webkit.org> wrote:

> [+sam]
>
> On Tue, Jun 23, 2009 at 5:11 PM, Drew Wilson<atwilson at google.com> wrote:
> > On Tue, Jun 23, 2009 at 4:53 PM, Maciej Stachowiak <mjs at apple.com>
> wrote:
> >> Also, there might be a subtle bug in the above code: what if
> window.Worker
> >> is first accessed from a different frame? Then the prototype of the
> Worker
> >> constructor itself will use the other frame's Object prototype as its
> >> prototype. I'm not sure if that is right. I think maybe
> JSWorkerConstructor
> >> should be passed the global object from which it is retrieved as a
> property,
> >> instead of using the lexical global object.
> >
> > Good catch. This bug seems to be in all our custom generated
> constructors.
>
> Yes.  This has caused us headaches (e.g., security bugs) in the past.
>
> Adam
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-dev/attachments/20090707/b0e59c20/attachment.html>