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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

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.

Comment 1

[Morgan Phillips [:mrrrgn]]

Attachment: hasinstance-this.diff

Comment 2

[Morgan Phillips [:mrrrgn]]

(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 3

[Tom S [:evilpie]]

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 ..

Comment 4

[Morgan Phillips [:mrrrgn]]

Try push before landing: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e9f9004ee28

Comment 5

[Pulsebot]

Pushed by [e-mail]:
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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8593fe84d84e