Bug 212677 – Make `errors` an own property of `AggregateError` instead of a prototype accessor

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Devin Rousso]

Issue: <https://github.com/tc39/proposal-promise-any/issues/38>
PR: <https://github.com/tc39/proposal-promise-any/pull/64>

[Devin Rousso]

Created attachment 400872 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 400872 [details]
Patch

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

r=me with comments

> Source/JavaScriptCore/runtime/AggregateError.cpp:48
> +    putDirectWithoutTransition(vm, vm.propertyNames->errors, constructArray(globalObject, static_cast<ArrayAllocationProfile*>(nullptr), errors), static_cast<unsigned>(PropertyAttribute::DontEnum));

1. constructArray can throw an error potentially (e.g. OutOfMemory error in extreme condition). So, let's perform error check.

auto scope = DECLARE_THROW_SCOPE(vm);
JSArray* errors = constructArray(globalObject, static_cast<ArrayAllocationProfile*>(nullptr), errors);
RETURN_IF_EXCEPTION(scope, void());

2. Use putDirect instead of putDirectWithoutTransition. putXXXWithoutTransition will put this name to Structure without structure transition. So, putXXXWithoutTransition is only allowed for objects which are created per-JSGlobalObject etc.
For example, we create one  AggregateError with Structure A. This structure A will have "errors" name since we are not performing structure transition. But this A will be used for another AggregateError even if it was not having "errors" name yet.

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

Let's remove subspaceFor function, and remove aggregateErrorSpace from VM. Now, the AggregateError's C++ class is identical to ErrorInstance. We do not need to have another IsoSubspace.

> Source/JavaScriptCore/runtime/AggregateError.h:78
>  

Let's put  `STATIC_ASSERT_ISO_SUBSPACE_SHARABLE(AggregateError, AggregateError::Base);` to ensure that AggregateError can share IsoSubspace with ErrorInstance.

[Devin Rousso]

Comment on attachment 400872 [details]
Patch

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

>> Source/JavaScriptCore/runtime/AggregateError.cpp:48
>> +    putDirectWithoutTransition(vm, vm.propertyNames->errors, constructArray(globalObject, static_cast<ArrayAllocationProfile*>(nullptr), errors), static_cast<unsigned>(PropertyAttribute::DontEnum));
> 
> 1. constructArray can throw an error potentially (e.g. OutOfMemory error in extreme condition). So, let's perform error check.
> 
> auto scope = DECLARE_THROW_SCOPE(vm);
> JSArray* errors = constructArray(globalObject, static_cast<ArrayAllocationProfile*>(nullptr), errors);
> RETURN_IF_EXCEPTION(scope, void());
> 
> 2. Use putDirect instead of putDirectWithoutTransition. putXXXWithoutTransition will put this name to Structure without structure transition. So, putXXXWithoutTransition is only allowed for objects which are created per-JSGlobalObject etc.
> For example, we create one  AggregateError with Structure A. This structure A will have "errors" name since we are not performing structure transition. But this A will be used for another AggregateError even if it was not having "errors" name yet.

Ah!  I hadn't thought of the "But this A will be used for another AggregateError even if it was not having "errors" name yet." situation.  Thanks for the detailed explanation :)

>> Source/JavaScriptCore/runtime/AggregateError.h:61
>>          return Structure::create(vm, globalObject, prototype, TypeInfo(ErrorInstanceType, StructureFlags), info());
> 
> Let's remove subspaceFor function, and remove aggregateErrorSpace from VM. Now, the AggregateError's C++ class is identical to ErrorInstance. We do not need to have another IsoSubspace.

Neat!

>> Source/JavaScriptCore/runtime/AggregateError.h:78
>>  
> 
> Let's put  `STATIC_ASSERT_ISO_SUBSPACE_SHARABLE(AggregateError, AggregateError::Base);` to ensure that AggregateError can share IsoSubspace with ErrorInstance.

Also Neat!

[Yusuke Suzuki]

Comment on attachment 400872 [details]
Patch

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

>>> Source/JavaScriptCore/runtime/AggregateError.h:61
>>>          return Structure::create(vm, globalObject, prototype, TypeInfo(ErrorInstanceType, StructureFlags), info());
>> 
>> Let's remove subspaceFor function, and remove aggregateErrorSpace from VM. Now, the AggregateError's C++ class is identical to ErrorInstance. We do not need to have another IsoSubspace.
> 
> Neat!

Ah, and you can also remove destroy function.

[Devin Rousso]

Created attachment 400878 [details]
Patch

i'm going to wait for test262 to update before landing this

[Yusuke Suzuki]

Can we just land it with (almost) copied test262 tests?

[Devin Rousso]

(In reply to Yusuke Suzuki from comment #6)
> Can we just land it with (almost) copied test262 tests?

AFAICT, this change would cause to fail a few existing test262 tests and I have yet to see any issues/PRs that would update them.

Here's all the related issues/PRs that I've been able to find:
 - AggregateError Updates <https://github.com/tc39/test262/issues/2646>
 - AggregateError updates. Ref gh-2646 <https://github.com/tc39/test262/pull/2647>
neither of which change the tests from checking `AggregateError.prototype.errors` to checking for an own property on any `AggregateError` instance.

I can write some super simple tests for this in the meantime.

[Devin Rousso]

Created attachment 401815 [details]
Patch

[Devin Rousso]

Created attachment 401816 [details]
Patch

oops, forgot the "Reviewed by"

[EWS]

Committed r263006: <https://trac.webkit.org/changeset/263006>

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

[Radar WebKit Bug Importer]

<rdar://problem/64332285>