Object.getOwnPropertyDescriptor throws TypeError on process _handle

[erulabs]

• Version: v8.9.2, v8.9.3 and v9.3.0 (at least, suspect its all versions >=8.9.2)
• Platform: All (tested OSX, Linux)
• Subsystem: N/A

Hello!

Related to #16949 and #16860 - it appears that since Node 8 there have been some issues with Object.getOwnPropertyDescriptor.

The simplest test case is

Object.getOwnPropertyDescriptor(process.stdin._handle.__proto__, "bytesRead"))

Expected output: undefined (pre 8.9 behavior)
Pre 8.9.2, this crashes node with a Segmentation Fault (see linked issues)
On and after 8.9.2, this returns TypeError:

> Object.getOwnPropertyDescriptor(process.stdin._handle.__proto__, "bytesRead")
TypeError: Method bytesRead called on incompatible receiver #<TTY>
    at Function.getOwnPropertyDescriptor (<anonymous>)
    at repl:1:8
    at ContextifyScript.Script.runInThisContext (vm.js:50:33)
    at REPLServer.defaultEval (repl.js:240:29)
    at bound (domain.js:301:14)
    at REPLServer.runBound [as eval] (domain.js:314:12)
    at REPLServer.onLine (repl.js:442:10)
    at emitOne (events.js:121:20)
    at REPLServer.emit (events.js:211:7)
    at REPLServer.Interface._onLine (readline.js:282:10)

This means that calling Object.getOwnPropertyDescripton on a process handle fails.

At least according to https://tc39.github.io/ecma262/#sec-object.getownpropertydescriptor - I don't see any specification for throwing a TypeError here (particularly since the Prototype is of course an Object, it even seems to violate the ES5 spec)

Can we return to returning undefined here?

After thinking about it, I believe returning a proper descriptor with a value of 0 would actually be preferable to undefined, since bytesRead is a getter which returns a Number. This is the case with, for example length:

> Object.getOwnPropertyDescriptor([].__proto__, 'length')
{ value: 0,
  writable: true,
  enumerable: false,
  configurable: false }

Thank you for your time!

[TimothyGu]

You are seeing an interplay of a lot of different edge cases, in the ECMAScript spec, in the APIs V8 provides, and in how we use V8's APIs. In the end, I don't believe this is a "bug" per se, but there might be some ways we can improve this (admittedly surprising for ordinary users) behavior. I will first explain why this behavior is allowed by the ECMAScript spec, and then demonstrate two ways how we can fix this.

---

Primer on the ECMAScript specification

In ECMAScript, every object has a certain set of "internal methods" that the rest of the specification call on to do certain tasks. For example, you can see the definition of Object.getOwnPropertyDescriptor have the following step:

3. Let desc be ? obj.[[GetOwnProperty]](key).

which means that the [[GetOwnProperty]] internal method of the object obj is called with the parameter key. (The "?" is significant; I will get to it later.)

The ECMAScript spec divides all objects into two camps: "ordinary objects" and "exotic objects." (Further reading: 6.1.7 The Object Type.) Most of the objects you encounter are ordinary objects, which means that their internal methods are the default ones.

However, ECMAScript spec also defines a few kinds of "exotic objects," which may override the default implementations of those internal methods. There are certain minimal constraints put on what exotic objects are allowed to do, but in general the overriden internal methods can do a lot of things without going against the spec.

Array objects are one of them, because of the special semantics involved in the property length. Setting the length property on an array can trigger special behaviors, in a way similar to having setter functions on the prototype object; yet, length property is perceived to be an own property on the array. This is different from, say, new Map().size, which is in fact a getter specified on Map.prototype. You can tell this difference by comparing Object.getOwnPropertyDescriptor([], 'length') and Object.getOwnPropertyDescriptor(new Map, 'size').

The spec also allows implementations (like V8) to define their own exotic objects, and also users to do the same through Proxy objects.

Now, about that ?. Per 5.2 Algorithm Conventions, ? is really a shorthand for "if doing this results in an 'abrupt completion' (like a thrown exception), then stop what we are doing and allow that abrupt completion to propagate; otherwise carry on". That is why this interpretation is false:

At least according to tc39.github.io/ecma262/#sec-object.getownpropertydescriptor - I don't see any specification for throwing a TypeError here

Even though Object.getOwnPropertyDescriptor doesn't throw any exception on its own, it calls the [[GetOwnProperty]] internal method of obj, which may throw an exception that is subsequently propagated by Object.getOwnPropertyDescriptor. And even though the default implementation of [[GetOwnProperty]] internal method (OrdinaryGetOwnProperty) does not throw any errors, obj may be an exotic object whose [[GetOwnProperty]] does throw errors.

Why getting [].__proto__'s length property works

In the last section, I mentioned that length is actually a magical property that is an own property on an array object, not on Array.prototype (i.e., [].__proto__). Why, then, does getting [].__proto__'s length property work?

The answer lies in 22.1.3 Properties of the Array Prototype Object:

The Array prototype object is an Array exotic object and has the internal methods specified for such objects.

So the only reason that Array.prototype has a length property is because it is an array object itself!

Array.prototype.push(1);
console.log(Array.prototype[0]);
// Prints 1

That makes no sense to me. And apparently, that makes no sense to the editors of the ECMAScript standard either, and they had to explicitly spell out why this is the case in that section:

NOTE: The Array prototype object is specified to be an Array exotic object to ensure compatibility with ECMAScript code that was created prior to the ECMAScript 2015 specification.

Okay I understand everything about the spec now... but how does this relate to process.stdin._handle.__proto__?

The short answer is process.stdin._handle.__proto__ is an exotic object created by Node.js using V8's APIs, and therefore can do anything it wants to -- including throwing errors when Object.getOwnPropertyDescriptor gets called.

The long answer is the way we add properties like bytesRead onto process.stdin._handle.__proto__ is through v8::ObjectTemplate::SetAccessor():

node/src/stream_base-inl.h

Lines 55 to 61 in a893e79

	t->PrototypeTemplate()->SetAccessor(env->bytes_read_string(),
	GetBytesRead<Base>,
	nullptr,
	env->as_external(),
	v8::DEFAULT,
	attributes,
	signature);

The resulting effect of such an operation is somewhere in between [].length and Map.prototype.size: bytesRead is specified on the prototype so Object.getOwnPropertyDescriptor(process.stdin._handle, 'bytesRead') won't return anything, yet it is a magical parameter (rather than JavaScript getter/setter pair) on process.stdin._handle.__proto__ so that any attempt to get information about the property will go through the V8 mechanism (i.e., the resulting process.stdin._handle.__proto__ is made an exotic object). And because we specified the signature parameter when adding the accessor property, V8 will check whether the validity of the this before calling the C++ function.

How do we fix this

I hope I have sufficiently demonstrated why the current behavior is not a "bug". The way SetAccessor() works is well-defined, and allowed by the ECMAScript spec. But I admit this is surprising for users.

Without trying either of them out, I think there are two ways to improve this situation: making the property more like Map.prototype.size, and making the property more like [].length. Some benchmark may be needed on deciding which one is more memory- and computing-efficient though. In either method, the prototype object will become an ordinary object.

1. Making the property more like Map.prototype.size: create getter/setter pair on prototype object. V8 offers a v8::Template::SetAccessorProperty() that creates a JavaScript-observable getter/setter pair from C++ getter/setter callbacks. In contrast to v8::ObjectTemplate::SetAccessor(), SetAccessorProperty() does not make the resulting object an exotic object; and the created getter/setter pair will be visible from Object.getOwnPropertyDescriptor(process.stdin._handle.__proto__, 'bytesRead'). The instance object would still be an ordinary object.

   On the other hand, this new function requires a different C++ callback signature from SetAccessor(). Changing the signature is totally doable however.

2. Making the property more like [].length: move the accessor property from the prototype object to the actual instance. Rather than calling SetAccessor() on the t->PrototypeTemplate(), we could call it on t->InstanceTemplate(). This way, bytesRead will be observed as an own property of process.stdin._handle by virtue of process.stdin._handle becoming an exotic object, and there would be no chance for the current error to be thrown.

[erulabs]

This makes sense to me and thank you for the thoughtful reply and ECMAScript primer as well! Certainly the change was surprising - seeing undefined move to a SegFault and into a thrown error over a handful of revisions made me suspect something unintentional was happening in Node.

My naive assumption was that Object.getOwnPropertyDescriptor should work on all Objects, and being JS, that should be everything - Interesting about exotic objects! The error seems reasonable, but I wonder if it shouldn't be something more helpful than a TypeError - since "exotic object" isn't something a non-expert has in their mental list of JS data types.

I'll go ahead and handle this error properly, and we can close this issue if you'd like.

Thanks again!

[bnoordhuis]

Rather than calling SetAccessor() on the t->PrototypeTemplate(), we could call it on t->InstanceTemplate().

I deliberately moved them in the reverse direction recently, see #16482.

[jure]

The solution proposed in number 2 is indeed the easiest way out of this (and would likely be accepted widely without fuss, given that it was the previous behaviour that no one complained about), and is indeed what @bnoordhuis changed in #16482 (which caused the failed assert and thus fixing it with the throw in #16860).

I agree that accessing a property and having that throw is unexpected and the pre 8.9 behaviour of returning undefined is friendlier to the user. An example of where we've seen this issue surface is that if you use node-config to configure a logger, say:

{ 
  logger: require('bunyan').createLogger({name: 'test'})
}

The pre 8.9 behaviour is that the above works perfectly. However, the 8.9.0 behaviour is that node crashes, and 8.9.2 behaviour is that it throws, both of which are failure modes for the above configuration. It happens because node-config accesses properties in the proto (https://github.com/lorenwest/node-config/blob/master/lib/config.js#L1216 #16949 (comment)) to detect cyclical structures (also catching the thrown error here causes a loop, because now child and parent can never match). I've tried getting the same pre 8.9 behaviour with PrototypeTemplate, i.e. returning undefined, but while I found the node developing & compiling experience absolutely flawless, I'm just lacking the understanding of the relationship between InstanceTemplate, PrototypeTemplate and InternalFieldCount.

A summary of the above would be, can we get undefined back?

[bnoordhuis]

Would making _handle non-enumerable work? That is a good and simple solution, IMO.

[addaleax]

Would making _handle non-enumerable work? That is a good and simple solution, IMO.

That doesn’t sound like it removes the throw on when accessing the property? I don’t think that’s a solution then, Object.getOwnPropertyDescriptor really shouldn’t throw errors if we can avoid it.

Fwiw, I’d prefer 1., but would be fine with either of the options listed by @TimothyGu

[bnoordhuis]

For some reason I thought SetAccessorProperty() didn't exist in v4.x but it's there. That's a good solution then.

That said, fd is for debugging but _externalStream and bytesRead could just be methods. There's no reason they need to be accessors, as far as I can tell.

[TimothyGu]

Reduce the size of wrap objects by moving a couple of accessors from the instance template to the prototype template.

This is exactly the kind of potential problems I tried to point out in

Some benchmark may be needed on deciding which one is more memory- and computing-efficient though.

That said, fd is for debugging but _externalStream and bytesRead could just be methods. There's no reason they need to be accessors, as far as I can tell.

Agreed, but the elephant in the room is compatibility.

I'm fine with solution 1.

[jure]

I gave solution 1 a shot, and as discussed above, using SetAccessorProperty allows Object.getOwnPropertyDescriptor to not throw, and simply return the property descriptor. But, as also discussed above, accessing process.stdin._handle.__proto__.bytesRead still throws (as is currently the case on master).

[jure]

@bnoordhuis Any tips on where to look for making _handle non-enumerable? I'm getting lost.

[bnoordhuis]

@jure git grep -n '\.handle =' lib/ and update every call site to Object.defineProperty(...).

That said, it may not be worth it since the issue has been fixed and Object.defineProperty() is likely slower than simple assignment (and can be monkey-patched by third-party code.)