Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

Object comparison fails in v0.8 #3867

Closed
mattijs opened this issue Aug 14, 2012 · 9 comments
Closed

Object comparison fails in v0.8 #3867

mattijs opened this issue Aug 14, 2012 · 9 comments

Comments

@mattijs
Copy link

mattijs commented Aug 14, 2012

In some cases Object comparison fails in node v0.8. While this first occurred with a certain order of require statements using the Backbone, Underscore and Require modules (see jashkenas/backbone#1543), this does not seem to be the problem.

After some testing it seemed to occur when requiring the http module and with more prying when requiring the punycode module. The bug can be recreated with a small test script so punycode does not seem to be the problem directly.

The test script can be found at https://gist.github.com/3351832

The test script has two identical methods for comparing objects (similar to Underscore) but with two triggers fails to recognize that a String is not an Object. Removing any of the two triggers from the script results in normal behavior. After triggering on comparison method (isObjectA) the other method (isObjectB) does return the correct result while the triggered method (isObjectA) does not.

I tested the script with v0.8.6 where it fails and with v0.6.20 where it behaves normal.

@bnoordhuis
Copy link
Member

I can't reproduce it but it could be a bug in one of V8's code generators. What do node -pe process.versions and file $(which node) print?

Do you see the same error when you build V8 standalone? Here is how you do that:

$ cd deps/v8
$ ln -s ../../../tools/gyp build/gyp
$ make -j 8 native
$ out/native/d8 script.js # <- your test case

Replace the console.log statements with print statements.

@mattijs
Copy link
Author

mattijs commented Aug 15, 2012

Test done with node v0.8.6 on OSX 10.7

$ node --version
  v0.8.6

$ node -pe process.version
  v0.8.6

$ file $(which node)
  /usr/local/bin/node: Mach-O 64-bit executable x86_64

$ node test.js
  isObjectA(str): true
  isObjectB(str): false
  inline check:   false

I built the v8 dep from the master branch and from the v0.8 branch with the instruction above. Test outcome:

$ node.git/deps/v8/out/native/d8 test.v8.js
  isObjectA(str): true
  isObjectB(str): false
  inline check:   false

I had some colleagues run the script too but they are also all on a Mac. The script fails there too.

@stephank
Copy link

On EC2, 64-bit:

$ ./node -pe process.version
v0.8.6

$ file ./node
./node: ELF 64-bit LSB executable, x86-64, version 1 (GNU/Linux), dynamically linked (uses shared libs), for GNU/Linux 2.6.24, BuildID[sha1]=0x9890cd5fd4ca2011cd8586f80607f122252b3b93, not stripped

$ ./node test.js 
isObjectA(str): true
isObjectB(str): false
inline check:   false

And on EC2, 32-bit:

$ ./node -pe process.version
v0.8.6

$ file ./node
./node: ELF 32-bit LSB executable, Intel 80386, version 1 (GNU/Linux), dynamically linked (uses shared libs), for GNU/Linux 2.6.24, BuildID[sha1]=0xbe04750a6fb16b3c29612c41d2e490d837be65ee, not stripped

$ ./node test.js
isObjectA(str): true
isObjectB(str): false
inline check:   false

@medikoo
Copy link

medikoo commented Aug 15, 2012

Indeed, very strange and scary bug, I reproduced that on node v0.8.6 and OSX 10.8

It's definitely V8 bug, I've checked and unfortunately it still exists in trunk. I've simplified a test case and posted issue to V8 project: http://code.google.com/p/v8/issues/detail?id=2291

@bnoordhuis
Copy link
Member

For the record, I'm able to reproduce it and the bug still exists in V8's HEAD. Here is a minimal test script:

function a(o) {
    return o === Object(o);
}
function b(o) {
    return o === Object(o);
}

var o = {};
o == o; // EQ_STRICT does *not* trigger the bug
b(o);

if (a(0)) throw Error('a(s) === true'); // okay
if (b(0)) throw Error('b(s) === true'); // error
1;

It's interesting that V8 compiles a and b to exactly the same code. It's the non-strict comparison before the call to b that triggers it.

@bnoordhuis
Copy link
Member

Okay, looks like it'll be fixed soon: https://chromiumcodereview.appspot.com/10830334

@dougwilson
Copy link
Member

I know this issue is closed because it has been patched in some release of V8, but I was wondering when this patch would make it downstream and into node a v0.8. The current v0.8 branch HEAD still shows this bug is active (v8 3.11.10.18). Upstream there is now a v8 3.11.10.19, which includes the patch for this.

@piscisaureus
Copy link

Ya. V8 was upgraded in 2d92393 so this will be fixed in the upcoming v0.8.8 release.

@dougwilson
Copy link
Member

Awesome, thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants