Bug 149027 – JSInternalPromiseDeferred should inherit JSPromiseDeferred

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

JSInternalPromiseDeferred should inherit JSPromiseDeferred

[Yusuke Suzuki]

Created attachment 260902 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 260902 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSInternalPromiseDeferred.cpp:41
> +const ClassInfo JSInternalPromiseDeferred::s_info = { "JSInternalPromiseDeferred", &Base::s_info, nullptr, CREATE_METHOD_TABLE(JSInternalPromiseDeferred) };

While JSPromiseDeferred does not inherit anything, JSInternalPromiseDeferred inherits JSPromiseDeferred.

[Yusuke Suzuki]

*** Bug 149028 has been marked as a duplicate of this bug. ***

[Yusuke Suzuki]

When visitChildren is called, since we perform jsCast<JSPromiseDeferred*>(cell), it causes the crash.

But it is difficult to reproduce this situation stably in the JSC shell because JSInternalPromiseDeferred is not held by the reachable objects.

The situation is the following,

(1): When JSInternalPromiseDeferred is created and resides on the C stack,
(2): GC starts
(3): GC calls visitChildren of JSInternalPromiseDeferred object
(4): perform jsCast<JSPromiseDeferred*>(cell) and crash.

So this patch does not contain the tests.

[Yusuke Suzuki]

These faults are figured out during the ES6 Module WebCore integration.

[Darin Adler]

Comment on attachment 260902 [details]
Patch

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

>> Source/JavaScriptCore/runtime/JSInternalPromiseDeferred.cpp:41
>> +const ClassInfo JSInternalPromiseDeferred::s_info = { "JSInternalPromiseDeferred", &Base::s_info, nullptr, CREATE_METHOD_TABLE(JSInternalPromiseDeferred) };
> 
> While JSPromiseDeferred does not inherit anything, JSInternalPromiseDeferred inherits JSPromiseDeferred.

I am confused about test coverage. Is there a reason we don’t have a test that demonstrates the problem this lack of inheritance causes?

[Yusuke Suzuki]

(In reply to comment #6)
> Comment on attachment 260902 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=260902&action=review
> 
> >> Source/JavaScriptCore/runtime/JSInternalPromiseDeferred.cpp:41
> >> +const ClassInfo JSInternalPromiseDeferred::s_info = { "JSInternalPromiseDeferred", &Base::s_info, nullptr, CREATE_METHOD_TABLE(JSInternalPromiseDeferred) };
> > 
> > While JSPromiseDeferred does not inherit anything, JSInternalPromiseDeferred inherits JSPromiseDeferred.
> 
> I am confused about test coverage. Is there a reason we don’t have a test
> that demonstrates the problem this lack of inheritance causes?

This case is a little bit edge case I think.
For the usual classes, I think we will see the assertion failure with the existing test cases.
But, JSInternalPromiseDeferred does not cause it before prototyping Module Loader in WebCore. This is because,

1. This fault causes error when executing JSInternalPromiseDeferred's visitChildren. Since it utilizes JSPromiseDeferred's one, this function attempt to perform jsCast<JSPromiseDeferred*>(cell). So, if we forgot to inherit JSPromiseDeferred::classInfo, it causes assertion failure.
2. But visitChildren is only called when the target object is live and GC occurs
3. JSInternalPromiseDeferred is only used in C++ runtime layer now. And it will be discarded relatively soon.
4. So, the problem only occurs when JSInternalPromiseDeferred is live in the C stack and GC occurs.
5. In addition to this limited situation, JSInternalPromiseDeferred is now only used for the module loader. So it only used when loading modules.

The above reasons make testing harder.
Hopefully, I this we will cover this bug.
I found this issue while prototyping the module loader in WebCore, this is because,

1. In JSC shell module loader, we fetch the module source code from the local file system. So the source code is immediately fetched. In this case, we don't need to hold JSInternalPromiseDeferred because we can resolve the promise with the fetched source immediately. So JSInternalPromiseDeferred will be discarded soon.
2. But in WebCore side, the source code will be fetched over the network. So until we fetched the source code, we need to strongly hold the JSInternalPromiseDeferred instance to resolve the promise when the source is fetched.
3. So, we keep JSInternalPromiseDeferred alive for a while. If the GC occurs during this, we can see this assertion failure.

So I think we can cover the problem solved here when adding the tests for the WebCore integrated module loader (I'm now implementing it).

[Yusuke Suzuki]

Committed r189577: <http://trac.webkit.org/changeset/189577>