Bug 202566 – Implement Promise.any and AggregateError

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mathias Bynens]

Stage 3 proposal: https://github.com/tc39/proposal-promise-any

Note that this includes AggregateError as well.

[Sergey Rubanov]

May I take this task?

[Saam Barati]

(In reply to Sergey Rubanov from comment #1)
> May I take this task?

Sure

[Devin Rousso]

@Sergey Rubanov, are you still working on this, or can I give it a shot? =D

[Devin Rousso]

Created attachment 396617 [details]
Patch

[Devin Rousso]

Created attachment 396622 [details]
Patch

add forgotten semicolon 🤦‍♂️

[Yusuke Suzuki]

Comment on attachment 396622 [details]
Patch

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

Nice. I've commented about how to avoid GC bugs :)

> Source/JavaScriptCore/runtime/AggregateError.cpp:52
> +        for (auto& error : thisObject->m_errors)
> +            visitor.appendHidden(error);

Let's use append instead of appendHidden in this case. And let's use `appendValues` to mark all of them at once.

> Source/JavaScriptCore/runtime/AggregateError.cpp:66
> +    Vector<JSValue> errorsVector;

JSValue in Vector is not marked by conservative GC and causes GC crash.
I recommend putting all of these logics into AggregateError::finishCreation, and append to AggregateError::m_errors directly.

Or, another way. Let's collect all of these information before allocating AggregateError here, and allocating Vector with that size in AggregateError constructor (In this case, we should make sure that Vector<> is cleared) instead of AggregateError::finishCreation.
Because while executing AggregateError constructor (not finishCreation), GC will never see it.

In that case, we can remove cellLock() use completely.

> Source/JavaScriptCore/runtime/AggregateError.cpp:79
> +    for (auto& error : errors)
> +        m_errors.append({vm, this, error});
> +

While modifying m_error, we should take `cellLock()`.

> Source/JavaScriptCore/runtime/AggregateErrorConstructor.cpp:45
> +    : InternalFunction(vm, structure, callAggregateErrorConstructor, constructAggregateErrorConstructor)

Use `: Base(` instead.

> Source/JavaScriptCore/runtime/AggregateErrorPrototype.cpp:69
> +            result->putDirectIndex(globalObject, index++, error.get());

This can potentially cause GC and in that case, GC will attempt to take `aggregateError->cellLock()` => deadlock.
If we know that `m_error` will not be modified by the main thread after creation, we can just access to this vector here without the lock since GC side never modifies it. (But let's comment about it in AggregateError::m_error member).
So, deadlock does not happen.

> Source/JavaScriptCore/runtime/Error.cpp:109
> +

Remove this line.

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1072
> +    m_linkTimeConstants[static_cast<unsigned>(LinkTimeConstant::makeAggregateError)].initLater([] (const Initializer<JSCell>& init) {
> +            init.set(JSFunction::create(init.vm, jsCast<JSGlobalObject*>(init.owner), 0, String(), globalFuncMakeAggregateError));
> +        });

Instead of exposing makeAggregateError, let's just expose @AggregateError, and call this in JS via `new @AggregateError`.
See the `LinkTimeConstant::Set` example to do that :)

[Yusuke Suzuki]

Comment on attachment 396622 [details]
Patch

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

>> Source/JavaScriptCore/runtime/AggregateError.cpp:66
>> +    Vector<JSValue> errorsVector;
> 
> JSValue in Vector is not marked by conservative GC and causes GC crash.
> I recommend putting all of these logics into AggregateError::finishCreation, and append to AggregateError::m_errors directly.
> 
> Or, another way. Let's collect all of these information before allocating AggregateError here, and allocating Vector with that size in AggregateError constructor (In this case, we should make sure that Vector<> is cleared) instead of AggregateError::finishCreation.
> Because while executing AggregateError constructor (not finishCreation), GC will never see it.
> 
> In that case, we can remove cellLock() use completely.

If we take the second way, what we should do is,

1. Use MarkedArgumentBuffer to collect JSValue here.
2. Pass MarkedArgumentBuffer to AggregatedError constructor (do not pass it to finishCreation, this is wrong).
3. In the constructor (this is important to avoid GC bug), initialize `m_error` with MarkedArgumentBuffer's size, and set values with `setWithoutWriteBarrier(JSValue)`.
4. Remove cellLock() use

[Sergey Rubanov]

(In reply to Devin Rousso from comment #3)
> @Sergey Rubanov, are you still working on this, or can I give it a shot? =D

I've started to work on this in December, but had no time to finish and probably won't have time for that in near future. It's great to see that you picked this up!

Sorry for the late response!😅

[Devin Rousso]

Created attachment 396678 [details]
Patch

Thanks for the review/feedback/suggestions Yusuke! :)

[Devin Rousso]

Created attachment 396680 [details]
Patch

rebase

[Yusuke Suzuki]

Comment on attachment 396680 [details]
Patch

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

r=me with some comments.

> Source/JavaScriptCore/runtime/AggregateError.cpp:38
> +AggregateError::AggregateError(VM& vm, Structure* structure, MarkedArgumentBuffer&& errors)

I recommend using `const MarkedArgumentBuffer&` here since I think this should be non-movable too.
Can you annotate MarkedArgumentBuffer class with `WTF_MAKE_NONMOVABLE(...)`?

> Source/JavaScriptCore/runtime/AggregateError.cpp:55
> +AggregateError* AggregateError::create(VM& vm, JSGlobalObject* globalObject, Structure* structure, JSValue errors, JSValue message, SourceAppender appender, RuntimeType type, bool useCurrentFrame)

If the function fails with error, typically, we put JSGlobalObject* as a first argument.

> Source/JavaScriptCore/runtime/AggregateError.cpp:73
> +    return create(vm, globalObject, structure, WTFMove(errorsList), messageString, appender, type, useCurrentFrame);

Let's release scope by using,

RELEASE_AND_RETURN(scope, create(vm, globalObject, structure, WTFMove(errorsList), messageString, appender, type, useCurrentFrame));

> Source/JavaScriptCore/runtime/AggregateError.h:43
> +class AggregateError : public ErrorInstance {

Let's annotate it with `final`. Using `final` is encouraged for JSC JSObject classes because (1) it makes inheritance explicit and (2) jsDynamicCast has an optimization if the class is annotated with `final`.

> Source/JavaScriptCore/runtime/AggregateError.h:69
> +    static AggregateError* create(VM& vm, JSGlobalObject* globalObject, Structure* structure, MarkedArgumentBuffer&& errors, const String& message, SourceAppender appender = nullptr, RuntimeType type = TypeNothing, bool useCurrentFrame = true)

Ditto, let's use `const MarkedArgumentBuffer&` instead.

> Source/JavaScriptCore/runtime/AggregateErrorPrototype.h:55
> +        return Structure::create(vm, globalObject, prototype, TypeInfo(ErrorInstanceType, StructureFlags), info());

I guess this is not correct even in ErrorPrototypeBase side & NativeErrorPrototype side (looks like harmless but still wrong). Can you change ErrorInstanceType to ObjectType?
ErrorInstanceType should be only used for the derived classes of ErrorInstance.

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:880
> +        });

You need to add `thisObject->m_aggregateErrorStructure.visit(this)` in visitChildren of JSGlobalObject as the same to m_URIErrorStructure etc.

[Devin Rousso]

Comment on attachment 396680 [details]
Patch

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

>> Source/JavaScriptCore/runtime/AggregateError.cpp:38
>> +AggregateError::AggregateError(VM& vm, Structure* structure, MarkedArgumentBuffer&& errors)
> 
> I recommend using `const MarkedArgumentBuffer&` here since I think this should be non-movable too.
> Can you annotate MarkedArgumentBuffer class with `WTF_MAKE_NONMOVABLE(...)`?

Sure!

>> Source/JavaScriptCore/runtime/AggregateError.cpp:55
>> +AggregateError* AggregateError::create(VM& vm, JSGlobalObject* globalObject, Structure* structure, JSValue errors, JSValue message, SourceAppender appender, RuntimeType type, bool useCurrentFrame)
> 
> If the function fails with error, typically, we put JSGlobalObject* as a first argument.

Interesting!  I was wondering whether there was a pattern to it.

>> Source/JavaScriptCore/runtime/AggregateError.h:43
>> +class AggregateError : public ErrorInstance {
> 
> Let's annotate it with `final`. Using `final` is encouraged for JSC JSObject classes because (1) it makes inheritance explicit and (2) jsDynamicCast has an optimization if the class is annotated with `final`.

Argh, I forgot one.  I'll add it.

>> Source/JavaScriptCore/runtime/AggregateErrorPrototype.h:55
>> +        return Structure::create(vm, globalObject, prototype, TypeInfo(ErrorInstanceType, StructureFlags), info());
> 
> I guess this is not correct even in ErrorPrototypeBase side & NativeErrorPrototype side (looks like harmless but still wrong). Can you change ErrorInstanceType to ObjectType?
> ErrorInstanceType should be only used for the derived classes of ErrorInstance.

`AggregateError` does inherit from `ErrorInstance` though.

>> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:880
>> +        });
> 
> You need to add `thisObject->m_aggregateErrorStructure.visit(this)` in visitChildren of JSGlobalObject as the same to m_URIErrorStructure etc.

Wow how did I miss that?  I for sure I thought I'd searched everywhere for "URIError" 😅  Thanks!

[Alexey Shvayka]

Comment on attachment 396680 [details]
Patch

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

Nicely done! Commented on minor spec incompatibilities.

> Source/JavaScriptCore/builtins/PromiseConstructor.js:180
> +        return function (reason)

If you use an arrow function here, `test/built-ins/Promise/any/reject-element-function-nonconstructor.js` will pass.
Other Promise methods would also benefit from similar tweaks, hence the failing tests.

> Source/JavaScriptCore/runtime/AggregateErrorConstructor.cpp:73
> +    Structure* errorStructure = InternalFunction::createSubclassStructure(globalObject, callFrame->jsCallee(), callFrame->newTarget(), jsCast<AggregateErrorConstructor*>(callFrame->jsCallee())->errorStructure(vm));

Per step 2 of https://tc39.es/proposal-promise-any/#sec-aggregate-error, errorStructure() should be acquired from newTarget's global object.
Please see https://bugs.webkit.org/show_bug.cgi?id=202599 for similar tweaks; test262 coverage for AggregateError is missing though.

[Devin Rousso]

Created attachment 396695 [details]
Patch

@Alexey Shvayka thanks for the info/suggestions\!

[EWS]

Committed r260273: <https://trac.webkit.org/changeset/260273>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 396695 [details].

[Radar WebKit Bug Importer]

<rdar://problem/61946036>