[webkit-reviews] review denied: [Bug 202566] Implement Promise.any and AggregateError : [Attachment 396622] Patch

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <ysuzuki at apple.com> has denied Devin Rousso <drousso at apple.com>'s
request for review:
Bug 202566: Implement Promise.any and AggregateError
https://bugs.webkit.org/show_bug.cgi?id=202566

Attachment 396622: Patch

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




--- Comment #6 from Yusuke Suzuki <ysuzuki at apple.com> ---
Comment on attachment 396622
  --> https://bugs.webkit.org/attachment.cgi?id=396622
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 :)