instanceof returns incorrect result within function after prototype chain has been extended with Object.setPrototypeOf()

[nateps]

Hey there! I authored / maintain a web framework (https://derbyjs.com/). We use Object.setPrototypeOf() in a manner similar to how Node.js util.inherits() is implemented. This pattern is also demonstrated in the MDN docs for Object.setPrototypeOf(): https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/setPrototypeOf.

We discovered an issue where an instanceof check returns the incorrect result in IE 11, Edge, and Node Chakra only. In our code, this leads to an error being thrown in a case where the instanceof check is supposed to guard against the prototype chain being appended to twice. Here is a simplified repro case that demonstrates the issue:

// class Component {}
function Component() {}

function isBasePrototype(object) {
  return (object === Function.prototype) ||
    (object === Object.prototype) ||
    (object === null);
}
function getRootPrototype(object) {
  while (true) {
    var prototype = Object.getPrototypeOf(object);
    if (isBasePrototype(prototype)) return object;
    object = prototype;
  }
}
function extendComponent(klass) {
  console.log('extendComponent: klass.prototype instanceof Component', klass.prototype instanceof Component);
  console.log('extendComponent: Box.prototype instanceof Component', Box.prototype instanceof Component);
  // Don't do anything if the constructor already extends Component
  if (klass.prototype instanceof Component) return;
  // Find the end of the prototype chain
  var rootPrototype = getRootPrototype(klass.prototype);
  // Establish inheritance with the pattern that Node's util.inherits() uses
  // if Object.setPrototypeOf() is available (all modern browsers & IE11).
  // This inhertance pattern is not equivalent to class extends, but it does
  // work to make instances of the constructor inherit the desired prototype
  // https://github.com/nodejs/node/issues/4179
  Object.setPrototypeOf(rootPrototype, Component.prototype);
}

// class Shape {}
// class Box extends Shape {}
function Shape() {}
function Box() {}
Box.prototype = Object.create(Shape.prototype);
Box.prototype.constructor = Box;

console.log('before extend: Box.prototype instanceof Component', Box.prototype instanceof Component);
extendComponent(Box);
console.log('after extend 1: Box.prototype instanceof Component', Box.prototype instanceof Component);
extendComponent(Box);
console.log('after extend 2: Box.prototype instanceof Component', Box.prototype instanceof Component);

In Chrome, Firefox, and Safari, we get the correct output:

IE11, Edge, and Node Chakra fail:

Some observations:

• The same problem occurs both with ES5 class-like function constructors as well as ES6 classes.
• The instanceof outside of the function returns the correct value true and then instanceof within the function returns the incorrect value false
• This occurs when setting the prototype of the prototype of a class, but if Box doesn't inherit from any other classes, this issue does not occur.

For now, we're going to work around this issue with some extra checks in the code, but I'm pretty sure this is a bug in Chakra. Please let me know if you determine otherwise.

Best,
Nate

[zenparsing]

Here's a more minimal repro:

function Component() {}
function Shape() {}

function Box() {}
Box.prototype = Object.create(Shape.prototype);
Box.prototype.constructor = Box;

function checkInstanceOf(a, b) {
  return a instanceof b;
}

console.log(
  'Box.prototype instanceof Component:',
  Box.prototype instanceof Component);

console.log('checkInstanceOf:',
  checkInstanceOf(Box.prototype, Component));

Object.setPrototypeOf(Shape.prototype, Component.prototype);

console.log(
  'Box.prototype instanceof Component:',
  Box.prototype instanceof Component);

console.log(
  'checkInstanceOf:',
  checkInstanceOf(Box.prototype, Component));

Results

#### ch
Box.prototype instanceof Component: false
checkInstanceOf: false
Box.prototype instanceof Component: true
checkInstanceOf: false

#### v8
Box.prototype instanceof Component: false
checkInstanceOf: false
Box.prototype instanceof Component: true
checkInstanceOf: true

It appears that the second call to checkInstanceOf assumes that the prototype chain has not been modified from the first call.

[rhuanjl]

I've had a quick look setPrototypeOf does not appear to invalidate the IsInst inline cache and so this case involves an incorrect cache hit.

Unfortunately I cannot see an easy way to access the IsInst inline cache from setPrototype of.

[rhuanjl]

I'd like to tag this for v1.12 BUT I can't work out how to fix it - if anyone would like to dig into this that would be great.

[ShortDevelopment]

What about ...?

scriptContext->GetThreadContext()->InvalidateAllIsInstInlineCaches();

[MadProbe]

What about ...?

scriptContext->GetThreadContext()->InvalidateAllIsInstInlineCaches();

This will invalidate all caches, not just needed ones, so this is a pretty expensive operation to do

[rhuanjl]

Having had a look, it seems currently IsInst inline caches are stored by function, not by object.

InvalidateAllIsInstInlineCaches would dump all of them for all functions whether they relate to the object we're looking at or not.

The method to dump a single one is ThreadContext::UnregisterIsInstInlineCache(Js::IsInstInlineCache * inlineCache, Js::Var function) to use this correctly we need to get pointers to the caches we need to invalidate AND for each one, a pointer to the function it relates to.

Without creating a new structure the only way to do this would be to iterate over isInstInlineCacheByFunction (as is done in InvalidateAllIsInstInlineCaches) AND then for each one, iterate over its list (as is done in InvalidateIsInstInlineCacheList) but then put in a check to see if it's referencing the Type we're looking at and invalidate it (with UnregisterIsInstInlineCache) if it is NOTE we'd have to use UnregisterIsInstInlineCache rather than a memset because that moves up the next element rather than creating a hole in the list.

This me a bit messy and involve a lot of iteration and checks BUT no one should be using a change prototype method on a fast path so I figure it's ok. Note we'd need to do not just the object who's prototype is being changed BUT any that inherit from it.