Function[Symbol.hasInstance] seems to have incorrect behavior when called with non-object "this"

RESOLVED FIXED in Firefox 50

Status

()

Core
JavaScript Engine
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: bz, Unassigned)

Tracking

Trunk
mozilla50
Points:
---

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(1 attachment)

This is new code in bug 1054906.

Spec says to just call OrdinaryHasInstance(F, V), where F is the "this" value and "V" is the first argument.  Then OrdinaryHasInstance step 1 says:

  If IsCallable(C) is false, return false.

So something like this:

  Function[Symbol.hasInstance].call(5, 6)

should return false per spec (and does in Chrome and Safari).  But in our implementation, we have:

  js::fun_symbolHasInstance(JSContext* cx, unsigned argc, Value* vp)
...
    /* Step 1. */
    HandleValue func = args.thisv();
    if (!func.isObject()) {
        ReportIncompatible(cx, args);
        return false;
    }

which throws an exception.  That's not what the spec's step 1 says, unless I'm missing something.
Flags: needinfo?(winter2718)
Created attachment 8760840 [details] [diff] [review]
hasinstance-this.diff
Flags: needinfo?(winter2718)
Attachment #8760840 - Flags: review?(evilpies)
(In reply to Boris Zbarsky [:bz] from comment #0)
> This is new code in bug 1054906.
> 
> Spec says to just call OrdinaryHasInstance(F, V), where F is the "this"
> value and "V" is the first argument.  Then OrdinaryHasInstance step 1 says:
> 
>   If IsCallable(C) is false, return false.
> 
> So something like this:
> 
>   Function[Symbol.hasInstance].call(5, 6)
> 
> should return false per spec (and does in Chrome and Safari).  But in our
> implementation, we have:
> 
>   js::fun_symbolHasInstance(JSContext* cx, unsigned argc, Value* vp)
> ...
>     /* Step 1. */
>     HandleValue func = args.thisv();
>     if (!func.isObject()) {
>         ReportIncompatible(cx, args);
>         return false;
>     }
> 
> which throws an exception.  That's not what the spec's step 1 says, unless
> I'm missing something.

You're right, I'd conflated the first step of "OrdinaryHasInstance" with the first step of "InstanceofOperator" a patch with the fix and some new test cases is attached.
Comment on attachment 8760840 [details] [diff] [review]
hasinstance-this.diff

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

::: js/src/jsfun.cpp
@@ +682,5 @@
>      }
>      /* Step 1. */
>      HandleValue func = args.thisv();
> +
> +    // Non-callables will always return false from OrdinaryHasInstance.

I would add "Primitives are non-callable and will ..
Attachment #8760840 - Flags: review?(evilpies) → review+

Comment 5

2 years ago
Pushed by mphillips@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8593fe84d84e
Function[Symbol.hasInstance] should return false when called with a non-callable 'this'; r=evilpie

Comment 6

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8593fe84d84e
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox50: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.