[webkit-reviews] review granted: [Bug 212677] Make `errors` an own property of `AggregateError` instead of a prototype accessor : [Attachment 400872] Patch

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <ysuzuki at apple.com> has granted Devin Rousso
<drousso at apple.com>'s request for review:
Bug 212677: Make `errors` an own property of `AggregateError` instead of a
prototype accessor
https://bugs.webkit.org/show_bug.cgi?id=212677

Attachment 400872: Patch

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




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