JSC napi: stop polluting global Object.prototype from napi_define_class

[bkaradzic-microsoft]

Closes #172.

JSObjectMakeConstructor(ctx, nullptr, ...) returns a function whose prototype property points at the global Object.prototype. Any napi caller that writes to constructor.prototype (either internally via napi_define_class for instance members, or in user code via ctor.Get("prototype").Set(...) ) was mutating Object.prototype itself, corrupting 'X' in {} / for..in across the runtime.

Previous workaround

The earlier // BEGIN TODO ... END TODO block in ConstructorInfo::Create set the constructor's [[Prototype]] internal slot to a fresh object and re-routed napi_get_prototype + CallAsConstructor through it. That kept napi_define_class's own instance-property writes out of Object.prototype, but left the user-visible .prototype property still pointing at Object.prototype. User code (#169) hit the pollution head-on.

It also conflated Object.getPrototypeOf(ctor) (per spec: Function.prototype for a normal constructor) with ctor.prototype (the per-class instance prototype). Two slots, two semantics — the previous code wrote both to the same object.

Fix

• ConstructorInfo::Create installs a fresh per-class prototype object as the constructor's .prototype property (DontEnum|DontDelete) with a non-enumerable .constructor back-reference, matching ECMAScript semantics for normal constructors. The constructor's [[Prototype]] slot is left at JSC's default (Function.prototype).
• CallAsConstructor sets the new instance's [[Prototype]] from the constructor's .prototype property, not from its [[Prototype]] slot.
• napi_define_class looks up the .prototype property directly instead of going through napi_get_prototype (which per N-API spec equals Object.getPrototypeOf, i.e. the [[Prototype]] internal slot = Function.prototype for a constructor — the old call was a semantic mismatch that only happened to work because of the kludge).
• napi_get_prototype itself is unchanged and remains spec-compliant.

Regression test

Added two tests in the existing Blob describe block (covers every engine that ships Blob, which is all of them):

• Blob.prototype is a fresh per-class object whose [[Prototype]] is Object.prototype (not Blob.prototype === Object.prototype as before).
• None of Blob's instance members (size, type, arrayBuffer, text, bytes) leak onto the global Object.prototype — i.e. 'size' in {} === false.

These tests would have caught the pollution before #169 surfaced it via the File polyfill.

Followup

Once this lands, #169's JS_PROTOTYPE_CHAIN_SHIM collapses to a single direct Object.setPrototypeOf(File.prototype, Blob.prototype) in C++; the Napi::Eval / env.RunScript dispatch goes away; and the 14-line FileReader dual-StaticValue/InstanceValue comment shrinks to one line (per #173's recommended WHATWG pattern).

[Copilot]

Pull request overview

Fixes a JavaScriptCore-specific N-API shim bug where constructors created via napi_define_class could have .prototype aliased to the global Object.prototype, causing instance-member definitions (and user code) to pollute Object.prototype and break property lookups/iteration across the runtime.

Changes:

• Install a fresh per-class object as the constructor’s .prototype property (and a non-enumerable .constructor back-reference) instead of relying on JSC’s default Object.prototype aliasing.
• Construct instances using the constructor’s .prototype property (not the constructor’s [[Prototype]] slot), and make napi_define_class attach instance members to that .prototype object.
• Add Blob-focused regression tests to ensure Blob.prototype is isolated and instance members do not leak onto Object.prototype.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
Core/Node-API/Source/js_native_api_javascriptcore.cc	Corrects JSC constructor/prototype wiring to prevent global Object.prototype pollution and align N-API behavior with JS semantics.
Tests/UnitTests/Scripts/tests.ts	Adds regression tests validating per-class prototype isolation and absence of leaked instance members on Object.prototype.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[bkaradzic-microsoft]

Closing this without merging.

I confirmed by reading the WebKit source (JSObjectRef.cpp:128) that JSObjectMakeConstructor sets the .prototype property with PropertyAttribute::DontEnum | PropertyAttribute::DontDelete | PropertyAttribute::ReadOnly. The ReadOnly bit makes the slot non-writable, so JSObjectSetProperty to replace its value calls put() and throws ([Uncaught Error] defineProperty@[native code] — exactly what CI surfaced on every JSC job).

The naive fix would be JSObjectMakeFunctionWithCallback instead, but JSCallbackFunction.cpp:46 passes nullptr for the construct callback, so the result is not constructable.

The actually-correct fix is to switch the entire ConstructorInfo::Create path from JSObjectMakeConstructor to JSObjectMake with a JSClassDefinition that provides both callAsConstructor and callAsFunction — that produces a callable+constructable JSCallbackObject whose .prototype we own from the start. That refactor also has to revisit:

• The sentinel pattern in NativeInfo::Link / Query (currently constructor and sentinel are separate JSObjects; with the new path, the constructor IS the JSCallbackObject and holds the ConstructorInfo* as private data directly).
• napi_new_instance and napi_instanceof semantics on the new object type.
• All other callers that assume constructors are JSCallbackConstructor instances.

That's bigger than what makes sense to bundle with #169, so I'm leaving #172 open and #169 will keep its JS-shim workaround for the .prototype === Object.prototype quirk. The JS shim is ugly but well-isolated to the File polyfill and survives the engine-specific behavior cleanly. Happy to take the bigger refactor in a separate PR.