Bug 202566

Summary: Implement Promise.any and AggregateError
Product: WebKit Reporter: Mathias Bynens <mathias>
Component: JavaScriptCoreAssignee: Devin Rousso <drousso>
Status: RESOLVED FIXED    
Severity: Normal CC: chi187, daytonlowell, drousso, ews-watchlist, fpizlo, joepeck, keith_miller, mark.lam, mathias, msaboff, ross.kirsling, sbarati, shvaikalesh, tzagallo, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
URL: https://github.com/tc39/proposal-promise-any
Bug Depends on:    
Bug Blocks: 212677    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Mathias Bynens 2019-10-03 23:37:58 PDT
Stage 3 proposal: https://github.com/tc39/proposal-promise-any

Note that this includes AggregateError as well.
Comment 1 Sergey Rubanov 2019-11-11 03:15:24 PST
May I take this task?
Comment 2 Saam Barati 2019-11-30 16:50:20 PST
(In reply to Sergey Rubanov from comment #1)
> May I take this task?

Sure
Comment 3 Devin Rousso 2020-04-15 11:50:36 PDT
@Sergey Rubanov, are you still working on this, or can I give it a shot? =D
Comment 4 Devin Rousso 2020-04-15 22:07:46 PDT
Created attachment 396617 [details]
Patch
Comment 5 Devin Rousso 2020-04-15 22:20:30 PDT
Created attachment 396622 [details]
Patch

add forgotten semicolon 🤦‍♂️
Comment 6 Yusuke Suzuki 2020-04-16 04:48:03 PDT
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 :)
Comment 7 Yusuke Suzuki 2020-04-16 04:55:47 PDT
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
Comment 8 Sergey Rubanov 2020-04-16 05:31:43 PDT
(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!😅
Comment 9 Devin Rousso 2020-04-16 11:36:05 PDT
Created attachment 396678 [details]
Patch

Thanks for the review/feedback/suggestions Yusuke! :)
Comment 10 Devin Rousso 2020-04-16 11:48:00 PDT
Created attachment 396680 [details]
Patch

rebase
Comment 11 Yusuke Suzuki 2020-04-16 12:11:32 PDT
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.
Comment 12 Devin Rousso 2020-04-16 12:27:55 PDT
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!
Comment 13 Alexey Shvayka 2020-04-16 12:51:59 PDT
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.
Comment 14 Devin Rousso 2020-04-16 13:57:41 PDT
Created attachment 396695 [details]
Patch

@Alexey Shvayka thanks for the info/suggestions\!
Comment 15 EWS 2020-04-17 12:31:12 PDT
Committed r260273: <https://trac.webkit.org/changeset/260273>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 396695 [details].
Comment 16 Radar WebKit Bug Importer 2020-04-17 12:32:14 PDT
<rdar://problem/61946036>