1132045 - Equality algorithm does not work when ToPrimitive does return null

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[a.d.bergi]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:32.0) Gecko/20100101 Firefox/32.0
Build ID: 20140923175406

Steps to reproduce:

Try to evaluate any of the following:

0 == {valueOf: function(){ return null; }} // true
0 == {toString: function(){ return null; }} // true
"0" == {valueOf: function(){ return null; }} // true
"0" == {toString: function(){ return null; }} // true
"" == {valueOf: function(){ return null; }} // true
"" == {toString: function(){ return null; }} // true

These are the results as obtained in the console of FF 29.? and 32.0.3


Actual results:

When comparing a string or number against an object whose [[DefaultValue]] operation returned `null`, that `null` value is converted to the number `0`. The return value for those is wrong.


Expected results:

The return value should have been the same as when comparing the value directly against `null`:

0 == null // false
"0" == null // false
"" == null // false

See the spec here: http://ecma-international.org/ecma-262/5.1/#sec-11.9.3
All other tested browsers do return the expected `false` value.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Hmm.  Yeah, this is odd.  js::LooselyEqual does the ToPrimitive bits, then checks SameType, then checks for symbols, but then does ToNumber on both values.  That almost makes sense, except for the case when ToPrimitive returned null or undefined, where it falls down.

We should probably change LooselyEqual to follow the spec more closely step by step.

Note that the relevant spec is actually http://people.mozilla.org/~jorendorff/es6-draft.html#sec-abstract-equality-comparison but it's pretty similar to the ES5 version.

Comment 2

[Tom S. (please needinfo tschuster)]

Attachment: equality

Just implementing this like the spec might cause performance issues, v8 (at least the first implementation that I could find) branches based on the lhs: https://github.com/v8/v8-git-mirror/blob/c7851da4aefb644ab198ead1fa284932fd424797/src/runtime.js#L28

I am not interested in finishing this right now.

Comment 3

[Tom S. (please needinfo tschuster)]

Attachment: Correct loose equality operation

This is pretty faithful to spec, I just added some very minor optimizations. The JITs don't seem to optimize non-null/undefined == object, so they shouldn't need adjustment.

Comment 4

[Jan de Mooij [:jandem]]

Comment on attachment 8565011 [details] [diff] [review]
Correct loose equality operation

Review of attachment 8565011 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great, thanks for fixing this. Some minor suggestions below.

::: js/src/vm/Interpreter.cpp
@@ +729,4 @@
>      if (SameType(lval, rval))
>          return EqualGivenSameType(cx, lval, rval, result);
>  
> +    // Handle int32 x double

Nit: end with a period.

@@ +732,5 @@
> +    // Handle int32 x double
> +    if (lval.isNumber() && rval.isNumber()) {
> +        *result = (lval.toNumber() == rval.toNumber());
> +        return true;
> +    }

Nice, less duplication than the code in StrictlyEqual. Would you mind changing that to match this, for consistency?

@@ +750,5 @@
>          return true;
>      }
>  
>      RootedValue lvalue(cx, lval);
>      RootedValue rvalue(cx, rval);

I think this function can just take HandleValues; most (all?) callers already have handles. Would you mind changing this while you're refactoring this function?

@@ +794,5 @@
> +    }
> +
> +    // Step 9.
> +    if (rvalue.isBoolean()) {
> +        rvalue.setInt32(rvalue.toBoolean() ? 1 : 0);

There's quite some duplication here. Maybe factor this out as LooselyEqualBoolean(Other) or something?

Comment 5

[Tom S. (please needinfo tschuster)]

Attachment: Correct loose equality operation

Comment 6

[Tom S. (please needinfo tschuster)]

Attachment: Add handles to various equality operations

This turned out to be a bit more complex. I hope the fromMarkedLocation call is safe.

Comment 7

[Jan de Mooij [:jandem]]

Comment on attachment 8565193 [details] [diff] [review]
Add handles to various equality operations

Review of attachment 8565193 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for doing this!

::: js/src/builtin/MapObject.cpp
@@ +822,5 @@
>      bool b = (value.asRawBits() == other.value.asRawBits());
>  
>  #ifdef DEBUG
>      bool same;
> +    MOZ_ASSERT(SameValue(nullptr, HandleValue::fromMarkedLocation(value.unsafeGet()),

Not sure if this is safe. Since it's debug-only code, this seems safer:

PerThreadData *data = TlsPerThreadData.get();
Rooted valueRoot(data, value);
Rooted otherRoot(data, other.value);

Comment 8

[Tom S. (please needinfo tschuster)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7494b57db270
https://hg.mozilla.org/integration/mozilla-inbound/rev/e00ed1b014d0
https://hg.mozilla.org/integration/mozilla-inbound/rev/70d0d6ba5e9c

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/70d0d6ba5e9c
https://hg.mozilla.org/mozilla-central/rev/e00ed1b014d0