Bug 70015 – Constructor should not be called if the object is being constructed inside WebCore

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Kentaro Hara]

Summary:

A DOM object can be created from the JS context and from the WebCore context. Constructor should be called if the object is created from the JS context, but should not be called if the object is created from the WebCore context. 


Details:

- Expected behavior when the object is created from the JS context (e.g. "new Event()"):
(1) V8XXXX::constructorCallback() is called.
(2) V8XXXX::constructorCallback() calls XXXX::create().
(3) XXXX::create() creates a C++ object.
(4) V8XXXX::constructorCallback() calls toV8() for the C++ object.
(5) toV8() wraps the C++ object and returns the wrapped JS object.

- Actual behavior when the object is created from the JS context (e.g. "new Event()"):
As described above (1) - (5). That's fine!!

- Expected behavior when the object is created from the WebCore context. (e.g. "window.addEventListener("load", function (event) { ... });". In this case, the Event object is created inside the WebCore context):
(1) WebCore calls XXXX::create().
(2) XXXX::create() creates a C++ object.
(3) WebCore calls toV8() for the C++ object.
(4) toV8() wraps the C++ object and returns the wrapped JS object.

- Actual behavior when the object is created from the WebCore context. (e.g. "window.addEventListener("load", function (event) { ... });"):
(1) WebCore calls XXXX::create().
(2) XXXX::create() creates a C++ object.
(3) WebCore calls toV8() for the C++ object.
(4) toV8() can call XXXX::constructorCallback(). (Whether or not toV8() calls XXXX::constructorCallback() depends on the implementation of toV8().)
(5) V8XXXX::constructorCallback() calls XXXX::create().
(6) XXXX::create() creates __another__ C++ object.
(7) V8XXXX::constructorCallback() calls toV8() for the C++ object.
(8) toV8() wraps the C++ object and returns the wrapped JS object.

This actual behavior definitely causes the following problems:

- Problem1: The object returned to JS is not the object created in (2) but the object created in (6). However, I do not yet know a test case that causes some visible bug because of this problem. 

- Problem2: In (4), XXXX::constructorCallback() can be called with no argument. If XXXX::constructorCallback() expects at least one argument, XXXX::constructorCallback() throws TypeError, resulting in crash. For example, Event caused this problem when I implemented constructor for Event. Based on the discussion with Dominicc, we solved this problem by adding the following two lines of code to Event::constructorCallback() (See here: http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/bindings/v8/custom/V8EventConstructors.cpp&exact_package=chromium&q=allowallocation&type=cs):

XXXX::constructorCallback(...) {
    ...;
    if (AllowAllocation::current())
        return args.Holder();
    ...;
}

This if check means "XXXX::constructorCallback() returns immediately if it is called from the WebCore context".



With these observations, we think that all constructorCallback() should have the above if check. This patch adds the if check to CodeGeneratorV8.pm. After this patch is landed, I would like to add the if check to all existing custom V8 constructors.

[Kentaro Hara]

Created attachment 110823 [details]
Patch

[Adam Barth]

Comment on attachment 110823 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=110823&action=review

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1536
> +    if (AllowAllocation::current())
> +        return args.Holder();

Do we need this check in any of our custom constructors as well?  I don't fully understand in which cases AllowAllocation::current() return true.

[WebKit Review Bot]

Comment on attachment 110823 [details]
Patch

Clearing flags on attachment: 110823

Committed r97423: <http://trac.webkit.org/changeset/97423>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kentaro Hara]

(In reply to comment #2)
> (From update of attachment 110823 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=110823&action=review
> 
> > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1536
> > +    if (AllowAllocation::current())
> > +        return args.Holder();
> 
> Do we need this check in any of our custom constructors as well?  I don't fully understand in which cases AllowAllocation::current() return true.

Adam: AllowAllocation::current() just calls Isolate::GetCurrent(), and Isolate::GetCurrent() returns the entered isolate for the current thread or NULL in case there is no current isolate (See the comment of Isolate::GetCurrent(): http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/v8/include/v8.h&type=cs&l=2727). In my understanding, this means that AllowAllocation::current() returns true if it is called inside toV8() which is invoked from the WebCore context, which is the case where constructorCallback() should not be executed.

This patch added the AllowAllocation check to all non-custom constructors of V8. I think that the AllowAllocation check should be also in all custom constructors of V8. Indeed, as for existing custom constructors, no bugs have been occurring without the AllowAllocation check. However, (1) the Problem1 and Problem2 that I described above can happen in the future if someone changes code, and (2) if the AllowAllocation check does not exist in the existing custom constructors, people will add a new custom constructor without the AllowAllocation check without considering the possibility of Problem1 and Problem2, which may result in ugly bugs. 

WDYT?

[Adam Barth]

Checking that static information isn't correct in all cases.  For example, sometimes we run WebCore code with JavaScript on the static and sometimes we run WebCore code without JavaScript on the stack.  Have the constructors work differently in those cases isn't really correct.

I'd feel a lot better about all of this if we could write tests to exercise the differences.

[Kentaro Hara]

(In reply to comment #6)
> Checking that static information isn't correct in all cases.  For example, sometimes we run WebCore code with JavaScript on the static and sometimes we run WebCore code without JavaScript on the stack.  Have the constructors work differently in those cases isn't really correct.
> 

Adam: Thank you very much. I will investigate more to find a correct way to make constructorCallback() to be called only when constructorCallback() should be called.

[Adam Barth]

Thanks.  I wouldn't get overly fixated on this problem, but it would be good to sort out.

[Dominic Cooney]

(in reply to comment #5):

> Adam: AllowAllocation::current() just calls Isolate::GetCurrent(), and Isolate::GetCurrent() returns the
> entered isolate for the current thread or NULL in case there is no current isolate (See the comment of
> Isolate::GetCurrent(): http://codesearch.google.com/codesearch#OAMlx_jo-
> ck/src/v8/include/v8.h&type=cs&l=2727). In my understanding, this means that
> AllowAllocation::current() returns true if it is called inside toV8() which is invoked from the WebCore
> context, which is the case where constructorCallback() should not be executed.

This is not quite right. One thing that makes this confusing is that the name ”AllowAllocation” is not a good name any more. Let me try to explain:

As described in the description, V8 calls C++ constructor callbacks in two situations:

1. The author allocates an instance via "new X"; X is a function with a C++ construct callback.

2. C++ allocates an instance via the function template.

Typical DOM constructor functions disallow the first way—they throw.

AllowAllocation is an RAII guard. When the bindings are doing number two (ugh… that sounds bad…) it puts an AllowAllocation on the stack, the callback runs and checks the guard (this is _all_ the typical DOM constructor callback does—it is V8Proxy::checkNewLegal.)

The guard is not just "is an isolate/V8 context present." It is, _are "allocations allowed"_ in the current isolate. A boolean that is set and restored by RAII ctor and dtor.

So you can see why AllowAllocations is no longer an appropriate name. Because method 1—the author allocates via "new X"—works, the allocation should be "allowed." But the C++ callback needs to discern when it is invoked by the author, and should allocate the C++ object to wrap, or when it is invoked by the wrapper table which will hand it a pointer to wrap.

[Adam Barth]

Ah, that makes sense.  Can we re-name AllowAllocations to make this more obvious.

[Kentaro Hara]

(In reply to comment #10)
> Ah, that makes sense.  Can we re-name AllowAllocations to make this more obvious.

OK, I will fix it in the bug 70397.