Skip to content

Changed behaviour for instanceof ArrayBuffer #20978

Description

@Zirro
  • Version: v10.2.1
  • Platform: Darwin MacBook-Pro.local 17.6.0 Darwin Kernel Version 17.6.0: Tue May 8 15:33:09 PDT 2018; root:xnu-4570.61.1~2/RELEASE_X86_64 x86_64
  • Subsystem: vm

Starting with the release of Node v10, we have seen a new behaviour with instanceof ArrayBuffer in jsdom. Our extensive use of the vm module is presumably involved.

I have extracted the following test case from the code in jsdom:

"use strict";

const vm = require("vm");

function Window() {
  this.console = console;

  this.ArrayBuffer = ArrayBuffer;
  this.Int8Array = Int8Array;
  this.Uint8Array = Uint8Array;
  this.Uint8ClampedArray = Uint8ClampedArray;
  this.Int16Array = Int16Array;
  this.Uint16Array = Uint16Array;
  this.Int32Array = Int32Array;
  this.Uint32Array = Uint32Array;
  this.Float32Array = Float32Array;
  this.Float64Array = Float64Array;
}

const window = new Window();

const script = new vm.Script(`
  const buffer = (new Uint8Array([0, 255, 0])).buffer;

  console.log(buffer instanceof ArrayBuffer);
`);

vm.createContext(window);

script.runInContext(window);

In Node v8.11.2, this logs true. In Node v10.2.1, it logs false.

I can't say for certain that the new behaviour is wrong - the assignments to this in Window were originally added to resolve issues with globals from different contexts, and removing them fixes this particular issue in Node v10. However, doing so reintroduces the previously mentioned issues as well.

Activity

  1. benjamingr commented on May 26, 2018

    @benjamingr
    Member

    This is because the ArrayBuffer passed from the outside is from a different "realm" than the one on the inside.

    You can reproduce this error by passing any builtin for example if you pass Error then TypeError instanceof Error will evaluate to false.

    > vm.runInNewContext('Error') instanceof Error; // false in REPL
    

    This is correct behaviour - I'm not sure why Node.js 8 behaved differently.

  2. advanceddeveloper commented on May 27, 2018

    @advanceddeveloper

    @benjamingr

    TypeError is not an instance of Error, but TypeError() should be if both Error and TypeError are passed.

    I'm not 100% sure, but it seems to me that @Zirro is right. I couldn't see why their script logs false. My guess is that in Node v8.11.2 accessing buffer of Uint8Array's instance constructs a new array buffer using global.ArrayBuffer constructor, but in Node v10.2.1 it probably calls some internal reference to ArrayBuffer which is in this case different from global.ArrayBuffer

  3. benjamingr commented on May 27, 2018

    @benjamingr
    Member

    @advanceddeveloper thank you for weighing in.

    If both Error and TypeError were passed it would still not really catch these cases, namely if an actual error was thrown it would get constructed with the "right" prototype and not the passed in one.

    In general - I'm not sure what bug we had in Node.js 8 - but the v10 behavior is definitely more correct.

    As a tangent - there are several proposals by TC39 to make this into a language (rather than Node.js) feature - https://github.com/tc39/proposal-realms

  4. targos commented on May 27, 2018

    @targos
    Member

    My expectation is that if you call new Uint8Array() with the constructor from context A, its internal ArrayBuffer should also be from context A. (kind of like when you call an async function created in context A, it returns a Promise from context A).
    I'm not certain but it looks like a V8 bug (in Node 10).

  5. targos commented on May 27, 2018

    @targos
    Member

    btw if this is a bug, it is fixed in V8 6.7.

  6. Zirro commented on May 27, 2018

    @Zirro
    Author

    Very interesting, thanks for looking into this. Going by the description, this commit seems relevant: v8/v8@c68f863

  7. Zirro commented on Jun 11, 2018

    @Zirro
    Author

    If we can confirm this as a bug per the above, could this potentially be fixed in a future 10.x release by backporting the relevant commits from V8?

  8. targos commented on Jun 11, 2018

    @targos
    Member

    Can you check if it's fixed in Node 10.4.0? We upgraded V8 in this version

  9. Zirro commented on Jun 11, 2018

    @Zirro
    Author

    Thanks, it has indeed been fixed in v10.4.0! I'll leave the issue open in case you want to add documentation about this as well, but otherwise you can close it.

  10. abonander commented on Aug 7, 2019

    @abonander

    @targos this is happening again as of Node v11.15.0; inside Jest I'm getting new Uint8Array().buffer instanceof ArrayBuffer === false. Unfortunately this check is in an indirect dependency so I can't work around it very easily.

  11. Hakerh400 commented on Aug 7, 2019

    @Hakerh400
    Contributor

    @abonander Unable to reproduce the issue on v11.15.0. Please note that this issue is related to the instanceof check only in case the ArrayBuffer constructor of the vm context is replaced with the ArrayBuffer contructor from the main context. If you hadn't explicitly passed ArrayBuffer constructor to the vm context and instanceof check failed, then it is working as intended. As I can see, authors of issues and PRs that referenced this issue are actually misunderstanding it.

    Consider the following example:

    const vm = require('vm');
    const ctx = {
      console,
      buffer: new Uint8Array().buffer,
      f(){
        ctx.Uint8Array = Uint8Array;
        ctx.ArrayBuffer = ArrayBuffer;
      },
    };
    vm.createContext(ctx);
    const script = new vm.Script(`
      console.log(new Uint8Array().buffer instanceof ArrayBuffer);
      console.log(buffer instanceof ArrayBuffer);
      f();
      console.log(new Uint8Array().buffer instanceof ArrayBuffer);
      console.log(buffer instanceof ArrayBuffer);
    `);
    script.runInContext(ctx);

    Prior to v10.4.0 it printed

    true
    false
    false
    true
    

    and after the issue is fixed it prints:

    true
    false
    true
    true
    

    Note that it still prints false in the second output line, but it is not a bug.

  12. abonander commented on Aug 24, 2019

    @abonander

    This sounds like a Jest bug then because it doesn't give the user control over the VM context as far as I know.

  13. wi-ski commented on Apr 18, 2020

    @wi-ski
  14. wi-ski commented on Apr 18, 2020

    @wi-ski
  15. odahcam commented on May 12, 2020

    @odahcam

    Happening in Node 12.14.1.

  16. silasdavis commented on Nov 19, 2020

    @silasdavis

    and v14.13.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions