From 87b8f09585b5652d58e56cf62ca0bb31d65c25a3 Mon Sep 17 00:00:00 2001 From: Jason Orendorff Date: Thu, 11 Jun 2020 14:57:00 +0000 Subject: [PATCH] Bug 1641355 - Change AggregateError.errors to a data property on instances. r=tcampbell,peterv. The changes in xpconnect are necessary because this is not being specified in the usual way, with a getter. Ordinary data properties require an explicit loophole to make them visible through X-ray wrappers. Differential Revision: https://phabricator.services.mozilla.com/D77181 --- js/src/builtin/Promise.cpp | 16 ++- js/src/tests/jstests.list | 11 ++ js/src/tests/non262/Error/AggregateError.js | 40 +++---- js/src/vm/CommonPropertyNames.h | 1 + js/src/vm/ErrorObject.cpp | 113 ++++-------------- js/src/vm/ErrorObject.h | 21 ---- js/xpconnect/src/XPCJSRuntime.cpp | 1 + js/xpconnect/src/xpcprivate.h | 1 + js/xpconnect/tests/chrome/test_xrayToJS.xhtml | 9 +- js/xpconnect/wrappers/XrayWrapper.cpp | 5 + 10 files changed, 74 insertions(+), 144 deletions(-) diff --git a/js/src/builtin/Promise.cpp b/js/src/builtin/Promise.cpp index abb05b1aa750..db27be2f09ea 100644 --- a/js/src/builtin/Promise.cpp +++ b/js/src/builtin/Promise.cpp @@ -3891,14 +3891,20 @@ static void ThrowAggregateError(JSContext* cx, return; } - // |error| isn't guaranteed to be an AggregateErrorObject in case of OOM. + // |error| isn't guaranteed to be an ErrorObject in case of OOM. RootedSavedFrame stack(cx); - if (error.isObject() && error.toObject().is()) { - auto* aggregateError = &error.toObject().as(); - aggregateError->setAggregateErrors(errors.unwrappedArray()); + if (error.isObject() && error.toObject().is()) { + Rooted errorObj(cx, &error.toObject().as()); + MOZ_ASSERT(errorObj->type() == JSEXN_AGGREGATEERR); + + RootedValue errorsVal(cx, JS::ObjectValue(*errors.unwrappedArray())); + if (!NativeDefineDataProperty(cx, errorObj, cx->names().errors, errorsVal, + 0)) { + return; + } // Adopt the existing saved frames when present. - if (JSObject* errorStack = aggregateError->stack()) { + if (JSObject* errorStack = errorObj->stack()) { stack = &errorStack->as(); } } diff --git a/js/src/tests/jstests.list b/js/src/tests/jstests.list index bd6c41e9a63e..8b36275e658a 100644 --- a/js/src/tests/jstests.list +++ b/js/src/tests/jstests.list @@ -752,6 +752,17 @@ skip script test262/built-ins/Atomics/add/bigint/nonshared-int-views.js skip script test262/built-ins/Atomics/xor/nonshared-int-views.js skip script test262/built-ins/Atomics/exchange/nonshared-int-views.js +# Not yet updated to match . +skip script test262/built-ins/NativeErrors/AggregateError/prototype/errors/invoked-as-accessor.js +skip script test262/built-ins/NativeErrors/AggregateError/prototype/errors/invoked-as-func.js +skip script test262/built-ins/NativeErrors/AggregateError/prototype/errors/length.js +skip script test262/built-ins/NativeErrors/AggregateError/prototype/errors/name.js +skip script test262/built-ins/NativeErrors/AggregateError/prototype/errors/prop-desc.js +skip script test262/built-ins/NativeErrors/AggregateError/prototype/errors/return-from-iterable-errors.js +skip script test262/built-ins/NativeErrors/AggregateError/prototype/errors/return-new-array-from-list.js +skip script test262/built-ins/NativeErrors/AggregateError/prototype/errors/this-has-no-typedarrayname-internal.js +skip script test262/built-ins/NativeErrors/AggregateError/prototype/errors/this-is-not-object.js + ############################################## # Enable Iterator Helpers tests in the shell # diff --git a/js/src/tests/non262/Error/AggregateError.js b/js/src/tests/non262/Error/AggregateError.js index 0dc5e9a23d61..81cf04e9d122 100644 --- a/js/src/tests/non262/Error/AggregateError.js +++ b/js/src/tests/non262/Error/AggregateError.js @@ -9,14 +9,11 @@ assertEq(Object.getPrototypeOf(AggregateError.prototype), Error.prototype); assertEq(AggregateError.prototype.name, "AggregateError"); assertEq(AggregateError.prototype.message, ""); -// AggregateError.prototype isn't an AggregateError instance. -assertThrowsInstanceOf(() => AggregateError.prototype.errors, TypeError); - // The |errors| argument is mandatory. assertThrowsInstanceOf(() => new AggregateError(), TypeError); assertThrowsInstanceOf(() => AggregateError(), TypeError); -// The .errors getter returns an array object. +// The .errors data property is an array object. { let err = new AggregateError([]); @@ -24,13 +21,15 @@ assertThrowsInstanceOf(() => AggregateError(), TypeError); assertEq(Array.isArray(errors), true); assertEq(errors.length, 0); - // A fresh object is returned each time calling the getter. - assertEq(errors === err.errors, false); - // The errors object is modifiable. errors.push(123); assertEq(errors.length, 1); assertEq(errors[0], 123); + assertEq(err.errors[0], 123); + + // The property is writable. + err.errors = undefined; + assertEq(err.errors, undefined); } // The errors argument can be any iterable. @@ -53,29 +52,30 @@ assertThrowsInstanceOf(() => AggregateError(), TypeError); } { - const { - get: getErrors, - set: setErrors, - } = Object.getOwnPropertyDescriptor(AggregateError.prototype, "errors"); - assertEq(typeof getErrors, "function"); - assertEq(typeof setErrors, "undefined"); + assertEq("errors" in AggregateError.prototype, false); - // The |this| argument must be an AggregateError instance. - assertThrowsInstanceOf(() => getErrors.call(null), TypeError); - assertThrowsInstanceOf(() => getErrors.call({}), TypeError); - assertThrowsInstanceOf(() => getErrors.call(new Error), TypeError); + const { + configurable, + enumerable, + value, + writable + } = Object.getOwnPropertyDescriptor(new AggregateError([]), "errors"); + assertEq(configurable, true); + assertEq(enumerable, false); + assertEq(writable, true); + assertEq(value.length, 0); const g = newGlobal(); let obj = {}; - let errors = getErrors.call(new g.AggregateError([obj])); + let errors = new g.AggregateError([obj]).errors; assertEq(errors.length, 1); assertEq(errors[0], obj); - // The prototype is (incorrectly) |g.Array.prototype| in the cross-compartment case. + // The prototype is |g.Array.prototype| in the cross-compartment case. let proto = Object.getPrototypeOf(errors); - assertEq(proto === Array.prototype || proto === g.Array.prototype, true); + assertEq(proto === g.Array.prototype, true); } if (typeof reportCompare === "function") diff --git a/js/src/vm/CommonPropertyNames.h b/js/src/vm/CommonPropertyNames.h index 701ea16d16e9..ca51ab721219 100644 --- a/js/src/vm/CommonPropertyNames.h +++ b/js/src/vm/CommonPropertyNames.h @@ -139,6 +139,7 @@ MACRO(era, era, "era") \ MACRO(ErrorToStringWithTrailingNewline, ErrorToStringWithTrailingNewline, \ "ErrorToStringWithTrailingNewline") \ + MACRO(errors, errors, "errors") \ MACRO(escape, escape, "escape") \ MACRO(eval, eval, "eval") \ MACRO(exec, exec, "exec") \ diff --git a/js/src/vm/ErrorObject.cpp b/js/src/vm/ErrorObject.cpp index b1bafdaafdd1..bb19d9acc031 100644 --- a/js/src/vm/ErrorObject.cpp +++ b/js/src/vm/ErrorObject.cpp @@ -103,16 +103,12 @@ static const JSPropertySpec error_properties[] = { JS_PSGS("stack", ErrorObject::getStack, ErrorObject::setStack, 0), JS_PS_END}; -static const JSPropertySpec AggregateError_properties[] = { - COMMON_ERROR_PROPERTIES(AggregateError), - // Only AggregateError.prototype has .errors! - JS_PSG("errors", AggregateErrorObject::getErrors, 0), JS_PS_END}; - #define IMPLEMENT_NATIVE_ERROR_PROPERTIES(name) \ static const JSPropertySpec name##_properties[] = { \ COMMON_ERROR_PROPERTIES(name), JS_PS_END}; IMPLEMENT_NATIVE_ERROR_PROPERTIES(InternalError) +IMPLEMENT_NATIVE_ERROR_PROPERTIES(AggregateError) IMPLEMENT_NATIVE_ERROR_PROPERTIES(EvalError) IMPLEMENT_NATIVE_ERROR_PROPERTIES(RangeError) IMPLEMENT_NATIVE_ERROR_PROPERTIES(ReferenceError) @@ -155,19 +151,16 @@ const ClassSpec ErrorObject::classSpecs[JSEXN_ERROR_LIMIT] = { IMPLEMENT_NONGLOBAL_ERROR_SPEC(LinkError), IMPLEMENT_NONGLOBAL_ERROR_SPEC(RuntimeError)}; -#define IMPLEMENT_ERROR_CLASS_FROM(clazz, name) \ - { \ - js_Error_str, /* yes, really */ \ - JSCLASS_HAS_CACHED_PROTO(JSProto_##name) | \ - JSCLASS_HAS_RESERVED_SLOTS(clazz::RESERVED_SLOTS) | \ - JSCLASS_BACKGROUND_FINALIZE, \ - &ErrorObjectClassOps, \ - &ErrorObject::classSpecs[JSProto_##name - JSProto_Error] \ +#define IMPLEMENT_ERROR_CLASS(name) \ + { \ + js_Error_str, /* yes, really */ \ + JSCLASS_HAS_CACHED_PROTO(JSProto_##name) | \ + JSCLASS_HAS_RESERVED_SLOTS(ErrorObject::RESERVED_SLOTS) | \ + JSCLASS_BACKGROUND_FINALIZE, \ + &ErrorObjectClassOps, \ + &ErrorObject::classSpecs[JSProto_##name - JSProto_Error] \ } -#define IMPLEMENT_ERROR_CLASS(name) \ - IMPLEMENT_ERROR_CLASS_FROM(ErrorObject, name) - static void exn_finalize(JSFreeOp* fop, JSObject* obj); static const JSClassOps ErrorObjectClassOps = { @@ -186,10 +179,10 @@ static const JSClassOps ErrorObjectClassOps = { const JSClass ErrorObject::classes[JSEXN_ERROR_LIMIT] = { IMPLEMENT_ERROR_CLASS(Error), IMPLEMENT_ERROR_CLASS(InternalError), - IMPLEMENT_ERROR_CLASS_FROM(AggregateErrorObject, AggregateError), - IMPLEMENT_ERROR_CLASS(EvalError), IMPLEMENT_ERROR_CLASS(RangeError), - IMPLEMENT_ERROR_CLASS(ReferenceError), IMPLEMENT_ERROR_CLASS(SyntaxError), - IMPLEMENT_ERROR_CLASS(TypeError), IMPLEMENT_ERROR_CLASS(URIError), + IMPLEMENT_ERROR_CLASS(AggregateError), IMPLEMENT_ERROR_CLASS(EvalError), + IMPLEMENT_ERROR_CLASS(RangeError), IMPLEMENT_ERROR_CLASS(ReferenceError), + IMPLEMENT_ERROR_CLASS(SyntaxError), IMPLEMENT_ERROR_CLASS(TypeError), + IMPLEMENT_ERROR_CLASS(URIError), // These Error subclasses are not accessible via the global object: IMPLEMENT_ERROR_CLASS(DebuggeeWouldRun), IMPLEMENT_ERROR_CLASS(CompileError), IMPLEMENT_ERROR_CLASS(LinkError), @@ -339,14 +332,18 @@ static bool AggregateError(JSContext* cx, unsigned argc, Value* vp) { } // 9.1.13 OrdinaryCreateFromConstructor, step 3. - // Step 5. - auto* obj = CreateErrorObject(cx, args, 1, JSEXN_AGGREGATEERR, proto); + // Step 4. + Rooted obj( + cx, CreateErrorObject(cx, args, 1, JSEXN_AGGREGATEERR, proto)); if (!obj) { return false; } - // Step 4. - obj->as().setAggregateErrors(errorsList); + // Step 5. + RootedValue errorsVal(cx, JS::ObjectValue(*errorsList)); + if (!NativeDefineDataProperty(cx, obj, cx->names().errors, errorsVal, 0)) { + return false; + } // Step 6. args.rval().setObject(*obj); @@ -774,71 +771,3 @@ static bool exn_toSource(JSContext* cx, unsigned argc, Value* vp) { args.rval().setString(str); return true; } - -ArrayObject* js::AggregateErrorObject::aggregateErrors() const { - const Value& val = getReservedSlot(AGGREGATE_ERRORS_SLOT); - if (val.isUndefined()) { - return nullptr; - } - return &val.toObject().as(); -} - -void js::AggregateErrorObject::setAggregateErrors(ArrayObject* errors) { - MOZ_ASSERT(!aggregateErrors(), - "aggregated errors mustn't be modified once set"); - setReservedSlot(AGGREGATE_ERRORS_SLOT, ObjectValue(*errors)); -} - -static inline bool IsAggregateError(HandleValue v) { - return v.isObject() && v.toObject().is(); -} - -// get AggregateError.prototype.errors -bool js::AggregateErrorObject::getErrors(JSContext* cx, unsigned argc, - Value* vp) { - CallArgs args = CallArgsFromVp(argc, vp); - - // Steps 1-4. - return CallNonGenericMethod(cx, args); -} - -// get AggregateError.prototype.errors -bool js::AggregateErrorObject::getErrors_impl(JSContext* cx, - const CallArgs& args) { - MOZ_ASSERT(IsAggregateError(args.thisv())); - - auto* obj = &args.thisv().toObject().as(); - - // Step 5. - // Create a copy of the [[AggregateErrors]] list. - - RootedArrayObject errorsList(cx, obj->aggregateErrors()); - - // [[AggregateErrors]] may be absent when this error was created through - // JS_ReportError. - if (!errorsList) { - ArrayObject* result = NewDenseEmptyArray(cx); - if (!result) { - return false; - } - - args.rval().setObject(*result); - return true; - } - - uint32_t length = errorsList->length(); - - ArrayObject* result = NewDenseFullyAllocatedArray(cx, length); - if (!result) { - return false; - } - - result->setLength(cx, length); - - if (length > 0) { - result->initDenseElements(errorsList, 0, length); - } - - args.rval().setObject(*result); - return true; -} diff --git a/js/src/vm/ErrorObject.h b/js/src/vm/ErrorObject.h index 0314e6edd1e4..701b09ba779b 100644 --- a/js/src/vm/ErrorObject.h +++ b/js/src/vm/ErrorObject.h @@ -118,22 +118,6 @@ class ErrorObject : public NativeObject { static bool setStack_impl(JSContext* cx, const CallArgs& args); }; -class AggregateErrorObject : public ErrorObject { - friend class ErrorObject; - - // [[AggregateErrors]] slot of AggregateErrorObjects. - static const uint32_t AGGREGATE_ERRORS_SLOT = ErrorObject::RESERVED_SLOTS; - static const uint32_t RESERVED_SLOTS = AGGREGATE_ERRORS_SLOT + 1; - - public: - ArrayObject* aggregateErrors() const; - void setAggregateErrors(ArrayObject* errors); - - // Getter for the AggregateError.prototype.errors accessor. - static bool getErrors(JSContext* cx, unsigned argc, Value* vp); - static bool getErrors_impl(JSContext* cx, const CallArgs& args); -}; - JSString* ErrorToSource(JSContext* cx, HandleObject obj); } // namespace js @@ -143,9 +127,4 @@ inline bool JSObject::is() const { return js::ErrorObject::isErrorClass(getClass()); } -template <> -inline bool JSObject::is() const { - return hasClass(js::ErrorObject::classForType(JSEXN_AGGREGATEERR)); -} - #endif // vm_ErrorObject_h_ diff --git a/js/xpconnect/src/XPCJSRuntime.cpp b/js/xpconnect/src/XPCJSRuntime.cpp index d14cee7acaf7..43a91b3d9bf4 100644 --- a/js/xpconnect/src/XPCJSRuntime.cpp +++ b/js/xpconnect/src/XPCJSRuntime.cpp @@ -117,6 +117,7 @@ const char* const XPCJSRuntime::mStrings[] = { "columnNumber", // IDX_COLUMNNUMBER "stack", // IDX_STACK "message", // IDX_MESSAGE + "errors", // IDX_ERRORS "lastIndex", // IDX_LASTINDEX "then", // IDX_THEN "isInstance", // IDX_ISINSTANCE diff --git a/js/xpconnect/src/xpcprivate.h b/js/xpconnect/src/xpcprivate.h index a7d2a5672cc3..8d0767a6f60a 100644 --- a/js/xpconnect/src/xpcprivate.h +++ b/js/xpconnect/src/xpcprivate.h @@ -394,6 +394,7 @@ class XPCJSContext final : public mozilla::CycleCollectedJSContext, IDX_COLUMNNUMBER, IDX_STACK, IDX_MESSAGE, + IDX_ERRORS, IDX_LASTINDEX, IDX_THEN, IDX_ISINSTANCE, diff --git a/js/xpconnect/tests/chrome/test_xrayToJS.xhtml b/js/xpconnect/tests/chrome/test_xrayToJS.xhtml index 88f382e1c7ef..12884a09037a 100644 --- a/js/xpconnect/tests/chrome/test_xrayToJS.xhtml +++ b/js/xpconnect/tests/chrome/test_xrayToJS.xhtml @@ -252,9 +252,6 @@ https://bugzilla.mozilla.org/show_bug.cgi?id=933681 gPrototypeProperties[c] = ["constructor", "name", "message", "stack"]; gConstructorProperties[c] = constructorProps([]); } - if (gPrototypeProperties['AggregateError']) { - gPrototypeProperties['AggregateError'].push("errors"); - } // toString and toSource only live on the parent proto (Error.prototype). gPrototypeProperties['Error'].push('toString'); gPrototypeProperties['Error'].push('toSource'); @@ -883,9 +880,9 @@ https://bugzilla.mozilla.org/show_bug.cgi?id=933681 is(errors[0], 'error 1', "errors[0] has the correct value"); is(errors[1], 'error 2', "errors[1] has the correct value"); - Object.defineProperty(e.wrappedJSObject, 'errors', {value: 42}); - is(e.wrappedJSObject.errors, 42, "Set tricky expando"); - is(e.errors.length, 2, "errors accessor works over Xrays"); + e.wrappedJSObject.errors = 42; + is(e.wrappedJSObject.errors, 42, "errors is a plain data property"); + is(e.errors, 42, "visible over Xrays"); } // Note - an Exception newed via Xrays is going to have an empty stack given the diff --git a/js/xpconnect/wrappers/XrayWrapper.cpp b/js/xpconnect/wrappers/XrayWrapper.cpp index 820ff2f9bf83..a5adfa0e3ca5 100644 --- a/js/xpconnect/wrappers/XrayWrapper.cpp +++ b/js/xpconnect/wrappers/XrayWrapper.cpp @@ -665,6 +665,11 @@ bool JSXrayTraits::resolveOwnProperty(JSContext* cx, HandleObject wrapper, } return true; } + + if (key == JSProto_AggregateError && + id == GetJSIDByIndex(cx, XPCJSContext::IDX_ERRORS)) { + return getOwnPropertyFromWrapperIfSafe(cx, wrapper, id, desc); + } } else if (key == JSProto_RegExp) { if (id == GetJSIDByIndex(cx, XPCJSContext::IDX_LASTINDEX)) { return getOwnPropertyFromWrapperIfSafe(cx, wrapper, id, desc);