diff --git a/Core/Node-API/Source/js_native_api_javascriptcore.cc b/Core/Node-API/Source/js_native_api_javascriptcore.cc index 2d0ab4ab..af8124ac 100644 --- a/Core/Node-API/Source/js_native_api_javascriptcore.cc +++ b/Core/Node-API/Source/js_native_api_javascriptcore.cc @@ -323,17 +323,24 @@ namespace { void* data, napi_value* result) { JSObjectRef constructor{JSObjectMakeConstructor(env->context, nullptr, CallAsConstructor)}; - // BEGIN TODO: This extra prototype should no longer be needed, but for some reason removing it leads to errors - // when setting properties on some prototypes. This should be investigated and removed. + + // JSObjectMakeConstructor(ctx, nullptr, ...) leaves the resulting function's + // `prototype` property pointing at the global `Object.prototype`. Writing to + // it (e.g. via napi_define_properties for instance members, or user code via + // ctor.Get("prototype").Set(...)) would pollute the global Object.prototype + // and corrupt every object in the runtime. Install a fresh per-class + // prototype object whose `[[Prototype]]` defaults to Object.prototype and + // whose `constructor` back-reference points at the new constructor. JSObjectRef prototype{JSObjectMake(env->context, nullptr, nullptr)}; - JSObjectSetPrototype(env->context, prototype, JSObjectGetPrototype(env->context, constructor)); - JSObjectSetPrototype(env->context, constructor, prototype); JSValueRef exception{}; JSObjectSetProperty(env->context, prototype, JSString("constructor"), constructor, - kJSPropertyAttributeReadOnly | kJSPropertyAttributeDontDelete, &exception); + kJSPropertyAttributeDontEnum, &exception); + CHECK_JSC(env, exception); + + JSObjectSetProperty(env->context, constructor, JSString("prototype"), prototype, + kJSPropertyAttributeDontEnum | kJSPropertyAttributeDontDelete, &exception); CHECK_JSC(env, exception); - // END TODO ConstructorInfo* info{new ConstructorInfo(env, utf8name, length, cb, data)}; if (info == nullptr) { @@ -379,8 +386,20 @@ namespace { // Make sure any errors encountered last time we were in N-API are gone. napi_clear_last_error(env); + // New instance's [[Prototype]] is the constructor's `prototype` property + // (the fresh per-class object installed by ConstructorInfo::Create), NOT + // the constructor's [[Prototype]] slot (which is Function.prototype). + JSValueRef protoValue{JSObjectGetProperty(ctx, constructor, JSString("prototype"), exception)}; + if (*exception) { + return nullptr; + } + JSObjectRef prototype{JSValueToObject(ctx, protoValue, exception)}; + if (*exception) { + return nullptr; + } + JSObjectRef instance{JSObjectMake(ctx, nullptr, nullptr)}; - JSObjectSetPrototype(ctx, instance, JSObjectGetPrototype(ctx, constructor)); + JSObjectSetPrototype(ctx, instance, prototype); napi_callback_info__ cbinfo{}; cbinfo.thisArg = ToNapi(instance); @@ -932,8 +951,16 @@ napi_status napi_define_class(napi_env env, } if (instancePropertyCount > 0) { - napi_value prototype{}; - CHECK_NAPI(napi_get_prototype(env, constructor, &prototype)); + // Look up the constructor's `prototype` property (the per-class object set up + // by ConstructorInfo::Create), not napi_get_prototype which per spec returns + // the [[Prototype]] internal slot (Function.prototype for constructors). + JSValueRef exception{}; + JSValueRef protoValue{JSObjectGetProperty(env->context, ToJSObject(env, constructor), + JSString("prototype"), &exception)}; + CHECK_JSC(env, exception); + JSObjectRef prototypeObj{JSValueToObject(env->context, protoValue, &exception)}; + CHECK_JSC(env, exception); + napi_value prototype{ToNapi(prototypeObj)}; CHECK_NAPI(napi_define_properties(env, prototype, diff --git a/Tests/UnitTests/Scripts/tests.ts b/Tests/UnitTests/Scripts/tests.ts index a08121d7..a4001664 100644 --- a/Tests/UnitTests/Scripts/tests.ts +++ b/Tests/UnitTests/Scripts/tests.ts @@ -1199,6 +1199,23 @@ describe("Blob", function () { expect(modelGltfJson.type).to.equal("model/gltf+json"); }); + // -------------------------------- Prototype isolation (regression: JsRH#172) -------------------------------- + it("Blob.prototype is a fresh per-class object, not Object.prototype", function () { + expect(Blob.prototype).to.not.equal(Object.prototype); + expect(Object.getPrototypeOf(Blob.prototype)).to.equal(Object.prototype); + expect(Object.getPrototypeOf(new Blob([]))).to.equal(Blob.prototype); + }); + + it("does not leak instance members onto the global Object.prototype", function () { + // Blob's instance accessors live on Blob.prototype. Before the JSC napi-shim + // fix in JsRH#172, napi_define_class wrote them onto the global Object.prototype, + // corrupting every object in the runtime. + for (const key of ["size", "type", "arrayBuffer", "text", "bytes"]) { + expect(key in {}).to.equal(false, `Object.prototype was polluted with '${key}'`); + expect(Object.prototype.hasOwnProperty.call(Object.prototype, key)).to.equal(false); + } + }); + // -------------------------------- Blob.text() -------------------------------- it("returns empty string for empty blobs", async function () { for (const blob of emptyBlobs) {