Closed Bug 1132045 Opened 5 years ago Closed 5 years ago

Equality algorithm does not work when ToPrimitive does return null

Categories

(Core :: JavaScript Engine, defect)

x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: a.d.bergi, Assigned: evilpie)

Details

Attachments

(2 files, 2 obsolete files)

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.
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached patch equality (obsolete) — Splinter Review
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.
QA Contact: evilpies
Assignee: nobody → evilpies
QA Contact: evilpies
Attached patch Correct loose equality operation (obsolete) — Splinter Review
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.
Attachment #8562892 - Attachment is obsolete: true
Attachment #8565011 - Flags: review?(jdemooij)
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?
Attachment #8565011 - Flags: review?(jdemooij)
Attachment #8565011 - Attachment is obsolete: true
Attachment #8565189 - Flags: review?(jdemooij)
This turned out to be a bit more complex. I hope the fromMarkedLocation call is safe.
Attachment #8565193 - Flags: review?(jdemooij)
Attachment #8565189 - Flags: review?(jdemooij) → review+
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);
Attachment #8565193 - Flags: review?(jdemooij) → review+
https://hg.mozilla.org/mozilla-central/rev/70d0d6ba5e9c
https://hg.mozilla.org/mozilla-central/rev/e00ed1b014d0
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.