1054906 - Implement ES6 Symbol.hasInstance

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Till Schneidereit [:till]]

Implementing this at all should be pretty simple. No idea how hard it is to make fast, though.

Comment 1

[Morgan Phillips [:mrrrgn]]

Attachment: hasinstance.diff

Comment 2

[Morgan Phillips [:mrrrgn]]

Comment on attachment 8748395 [details] [diff] [review]
hasinstance.diff

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

The jit specific portion of this in CodeGenerator.cpp needs to be dealt with before these changes can go in.

Comment 3

[Morgan Phillips [:mrrrgn]]

Attachment: hasinstance.diff

Comment 4

[Boris Zbarsky [:bzbarsky]]

So once this is done, does that mean we can work on nixing class hasInstance hooks?

Comment 5

[Morgan Phillips [:mrrrgn]]

(In reply to Boris Zbarsky [:bz] from comment #4)
> So once this is done, does that mean we can work on nixing class hasInstance
> hooks?

Yes ! I'd been discussing adding a bug for that just today. :)

Comment 6

[Tom S [:evilpie]]

Comment on attachment 8749543 [details] [diff] [review]
hasinstance.diff

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

Sorry, I don't think this is ready yet. Did this pass a complete try-run? At least this part in IonMonkey: https://mxr.mozilla.org/mozilla-central/source/js/src/jit/IonBuilder.cpp#13834 won't be a valid optimization, because it doesn't consider Symbol.hasInstance at all. I suggest you ask Jan or somebody else what actually needs to be fixed in the JITs, because it don't really understand all the details.

::: js/src/jit/BaselineIC.cpp
@@ +7754,5 @@
>          return true;
>  
> +    // The user has supplied their own @@hasInstance method, we shouldn't
> +    // bypass it.
> +    if (!fun->isNative())

This isn't a valid check. Consider Math.sin[Symbol.hasInstance] = () => false; fun->isNative would be true, but the instanceof result would still be wrong. You should split out the JIT parts in a separate patch for the next iteration. I would imagine using something like lookupPure(symbols().hasInstance) == js::fun_hasInstance could work here.

::: js/src/jsfun.cpp
@@ +662,3 @@
>   */
> +extern bool
> +js::fun_hasInstance(JSContext* cx, HandleObject obj, MutableHandleValue v, bool* bp)

We should call this InstanceofOperator and move it to Interpreter.cpp

@@ +705,2 @@
>  static bool
> +fun_ordinaryHasInstance(JSContext* cx, unsigned argc, Value* vp)

Let's call this fun_hasInstance or maybe fun_symbolHasInstance?

::: js/src/vm/Interpreter.cpp
@@ -744,2 @@
>      RootedValue local(cx, v);
> -    if (JSHasInstanceOp hasInstance = clasp->getHasInstance())

Various DOM things have special instanceof methods that work across processes (CPOWs) also across compartments IIRC? We can't just stop calling those. For example: https://mxr.mozilla.org/mozilla-central/source/dom/bindings/BindingUtils.cpp#1822

Comment 7

[Morgan Phillips [:mrrrgn]]

(In reply to Tom Schuster [:evilpie] from comment #6)
> Comment on attachment 8749543 [details] [diff] [review]
> hasinstance.diff
> 
> Review of attachment 8749543 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry, I don't think this is ready yet. Did this pass a complete try-run? At
> least this part in IonMonkey:
> https://mxr.mozilla.org/mozilla-central/source/js/src/jit/IonBuilder.
> cpp#13834 won't be a valid optimization, because it doesn't consider
> Symbol.hasInstance at all. I suggest you ask Jan or somebody else what
> actually needs to be fixed in the JITs, because it don't really understand
> all the details.
> 
> ::: js/src/jit/BaselineIC.cpp
> @@ +7754,5 @@
> >          return true;
> >  
> > +    // The user has supplied their own @@hasInstance method, we shouldn't
> > +    // bypass it.
> > +    if (!fun->isNative())
> 
> This isn't a valid check. Consider Math.sin[Symbol.hasInstance] = () =>
> false; fun->isNative would be true, but the instanceof result would still be
> wrong. You should split out the JIT parts in a separate patch for the next
> iteration. I would imagine using something like
> lookupPure(symbols().hasInstance) == js::fun_hasInstance could work here.
> 
> ::: js/src/jsfun.cpp
> @@ +662,3 @@
> >   */
> > +extern bool
> > +js::fun_hasInstance(JSContext* cx, HandleObject obj, MutableHandleValue v, bool* bp)
> 
> We should call this InstanceofOperator and move it to Interpreter.cpp
> 
> @@ +705,2 @@
> >  static bool
> > +fun_ordinaryHasInstance(JSContext* cx, unsigned argc, Value* vp)
> 
> Let's call this fun_hasInstance or maybe fun_symbolHasInstance?
> 
> ::: js/src/vm/Interpreter.cpp
> @@ -744,2 @@
> >      RootedValue local(cx, v);
> > -    if (JSHasInstanceOp hasInstance = clasp->getHasInstance())
> 
> Various DOM things have special instanceof methods that work across
> processes (CPOWs) also across compartments IIRC? We can't just stop calling
> those. For example:
> https://mxr.mozilla.org/mozilla-central/source/dom/bindings/BindingUtils.
> cpp#1822

Thank you for the review. Yeah, this patch is a little half baked. Will try to have something nicer next time. One of these days I'll learn, until then: http://images-cdn.moviepilot.com/images/c_fill,h_275,w_245/t_mp_quality_gif/xcrppnn0bq0cfnuywj5r/christina-ricci-as-morticia-addams-in-an-addams-family-reboot-yes-please-625778.gif

Comment 8

[Morgan Phillips [:mrrrgn]]

Attachment: hasinstance.diff

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2c3aaa286cbd

Comment 9

[Morgan Phillips [:mrrrgn]]

Comment on attachment 8751380 [details] [diff] [review]
hasinstance.diff

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

The requested changes were added. I left in the code for preventing ion compilation when a custom function has been supplied. Leaving that out could cause nasty bugs (see included tests). A try run looks nice and green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2c3aaa286cbd

Comment 10

[Tom S [:evilpie]]

(In reply to Morgan Phillips [:mrrrgn] from comment #9)
> Comment on attachment 8751380 [details] [diff] [review]
> hasinstance.diff
> 
> Review of attachment 8751380 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The requested changes were added. I left in the code for preventing ion
> compilation when a custom function has been supplied. Leaving that out could
> cause nasty bugs (see included tests). A try run looks nice and green:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=2c3aaa286cbd

You left in the code for Baseline. I don't see any changes to Ionmonkey, which I am sure are required. Jan can you maybe give a quick overview what needs to be done here?

Comment 11

[Morgan Phillips [:mrrrgn]]

(In reply to Tom Schuster [:evilpie] from comment #10)
> (In reply to Morgan Phillips [:mrrrgn] from comment #9)
> > Comment on attachment 8751380 [details] [diff] [review]
> > hasinstance.diff
> > 
> > Review of attachment 8751380 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > The requested changes were added. I left in the code for preventing ion
> > compilation when a custom function has been supplied. Leaving that out could
> > cause nasty bugs (see included tests). A try run looks nice and green:
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=2c3aaa286cbd
> 
> You left in the code for Baseline. I don't see any changes to Ionmonkey,
> which I am sure are required. Jan can you maybe give a quick overview what
> needs to be done here?

I apologize for any confusion. Without the changes I added in baseline user defined @@hasInstance methods will be overridden in a hot loop. My understanding has been that once they are kicked out of baseline they should never make it to ion (so additional changes wouldn't be necessary).

    function a() {};
    function b() {};
    Object.defineProperty(a.prototype, Symbol.hasInstance, {value: () => true});
    Object.defineProperty(a, Symbol.hasInstance, {value: () => true});
    assertEq(b instanceof a, true);
    for (let _ of Array(10000))
        assertEq(b instanceof a, true);

Should always return True, but without the changes in TryAttachInstanceOfSub will return false.

* As an aside, I made another derp by not setting the @@hasInstance to JSPROP_READONLY but that's a tiny change. * 

If I'm missing something about your argument against putting changes in baseline and not ion could you help me understand a bit more clearly. Perhaps a breaking test case?

Comment 12

[Tom S [:evilpie]]

Attachment: Testcase

So here an example of what could go wrong. If you run this testcase you will see that the result is correct (prints false) the first few times when running in the interpreter. But after a while various optimizations kick in and change the result to printing true. For example during the 20th iteration we end up folding away the whole instanceof operation in IonBuilder::tryFoldInstanceOf.

Comment 13

[Morgan Phillips [:mrrrgn]]

(In reply to Tom Schuster [:evilpie] from comment #12)
> Created attachment 8752599 [details]
> Testcase
> 
> So here an example of what could go wrong. If you run this testcase you will
> see that the result is correct (prints false) the first few times when
> running in the interpreter. But after a while various optimizations kick in
> and change the result to printing true. For example during the 20th
> iteration we end up folding away the whole instanceof operation in
> IonBuilder::tryFoldInstanceOf.

Oy this makes sense. Thanks for the example! I'll split the changes into two patches as you had originally requested.

Comment 14

[Jan de Mooij [:jandem]]

Clearing NI as evilpie posted a testcase.

I'd be happy to review or give feedback on patches for this.

Comment 15

[Jason Orendorff [:jorendorff]]

Comment on attachment 8751380 [details] [diff] [review]
hasinstance.diff

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

::: js/src/jit/BaselineIC.cpp
@@ +7755,5 @@
>      if (fun->isBoundFunction())
>          return true;
>  
> +    // The user has supplied their own @@hasInstance method, we shouldn't
> +    // bypass it.

Two bugs here, I think:

1. We have to check a bunch of things to make sure we really know how the property lookup is going to turn out:

    - the function object does not have its own @@hasInstance property
    - its prototype is %%FunctionPrototype%%
    - %%FunctionPrototype%%[@@hasInstance] is an own data property
    - its value is the expected function

2. We have to re-confirm this each time we have a cache hit.  The code to do this check is in ICInstanceOf_Function::Compiler::generateStubCode. The four steps above all need to be represented here. The first one already is:

    - There is already a shape check that confirms that the object is a Function
      that does not have an own @@hasInstance property.
    - need to add code to check that the object's [[Prototype]] is %%FunctionPrototype%%
    - need to add a shape check on %%FunctionPrototype%%
    - need to add a check on the value in the slot indicated by the shape

Comment 16

[Morgan Phillips [:mrrrgn]]

Attachment: hasinstance-1-of-2.diff

Comment 17

[Morgan Phillips [:mrrrgn]]

Attachment: hasinstance-2-of-2.diff

Comment 18

[Morgan Phillips [:mrrrgn]]

Comment on attachment 8757176 [details] [diff] [review]
hasinstance-2-of-2.diff

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

Interested to know if this is on the right track.

Comment 19

[Morgan Phillips [:mrrrgn]]

Comment on attachment 8757176 [details] [diff] [review]
hasinstance-2-of-2.diff

I can do better than this. Reposting in a jiffy.

Comment 20

[Jan de Mooij [:jandem]]

Comment on attachment 8757176 [details] [diff] [review]
hasinstance-2-of-2.diff

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

The approach makes sense, we just have to make sure we do the right thing when the property is overridden after we attach the stub.

Please try at least Octane with and without the patch (especially earley-boyer relies on instanceof..). If the suggestions below regress things we'll have to try something else.

::: js/src/jit/BaselineIC.cpp
@@ +7757,5 @@
>  
> +    // If the user has supplied their own @@hasInstance method we shouldn't
> +    // clobber it.
> +    if (!js::FunctionHasDefaultHasInstance(fun, cx->wellKnownSymbols()))
> +        return true;

This check is fine, because we guard on the shape of |fun| in ICInstanceOf_Function::Compiler::generateStubCode.

We currently don't shape guard the objects on its proto chain, so if we override @@hasInstance on the prototype after we attach the stub, the stub will start behaving incorrectly (we should add a test for this).

We could add shape guards for these objects, but that's a bit complicated with the current Baseline IC design. I think it's fine (and simpler) for now to check (1) the function's proto is the builtin Function.prototype, and (2) fun->hasUncacheableProto() is false. The latter is necessary to make sure mutating __proto__ will trigger a shape change.

(This works because Function.prototype[@@hasInstance] is non-configurable and non-writable.)

@@ +7763,5 @@
> +    if (!fun->hasStaticPrototype())
> +        return true;
> +
> +    size_t chainDepth = 0;
> +    WellKnownSymbols symbols = cx->wellKnownSymbols();

Nit: WellKnownSymbols& symbols (please delete WellKnownSymbols' implicit copy constructor)

::: js/src/jit/IonBuilder.cpp
@@ +13845,5 @@
>  
> +        // If the user has supplied their own @@hasInstance method we shouldn't
> +        // clobber it.
> +        JSFunction* fun = &rhsObject->as<JSFunction>();
> +        if (!js::FunctionHasDefaultHasInstance(fun, compartment->runtime()->wellKnownSymbols())) {

Here we also have to guard against @@hasInstance being overridden later. In Ion, we can use TI for this instead of a shape guard. After the FunctionHasDefaultHasInstance check I'd add something like this:

    TypeSet::ObjectKey* key = TypeSet::ObjectKey::get(fun);
    HeapTypeSetKey property = key->property(..hasInstance jsid..);
    if (property.isOwnProperty(constraints()))
        ..fail..

The isOwnProperty call will add a type constraint that invalidates our code when someone overrides @@hasInstance.

Here you can also check our proto is Function.prototype, and to make sure the proto won't be mutated behind our back:

    if (!key->hasStableClassAndProto(constraints()))
        ..fail..

@@ +13896,5 @@
>  
>      // Try to inline a fast path based on Baseline ICs.
>      do {
> +        if (skipOptimization)
> +            break;

This is no longer needed as the stuff we get from the Baseline IC stays valid - the shape guard ensures that.

::: js/src/jsfun.cpp
@@ +1161,5 @@
> +js::FunctionHasDefaultHasInstance(JSFunction* fun, const WellKnownSymbols& symbols)
> +{
> +    jsid id = SYMBOL_TO_JSID(symbols.hasInstance);
> +    Shape* shape = fun->lookupPure(id);
> +    if (shape && shape->hasSlot()) {

If !hasSlot, we should return false (can we add a test for this?):

if (shape) {
    if (!shape->hasSlot())
        return false;
}

@@ +1163,5 @@
> +    jsid id = SYMBOL_TO_JSID(symbols.hasInstance);
> +    Shape* shape = fun->lookupPure(id);
> +    if (shape && shape->hasSlot()) {
> +        const Value hasInstance = fun->as<NativeObject>().getSlot(shape->slot());
> +        if (!IsNativeFunction(hasInstance))

Nit: return IsNativeFunction(hasInstance, js::fun_symbolHasInstance);

Comment 21

[Morgan Phillips [:mrrrgn]]

Attachment: hasinstance-1-of-2.diff

Comment 22

[Morgan Phillips [:mrrrgn]]

Attachment: hasinstance-2-of-2.diff

I ran 20 rounds of Octane tests, 10 with the patch and 10 without. The results were essentially identical.

With patch:

    Median EarleyBoyer: 327
    Median Score: 1050

Without Patch:
    Median EarlyBoyer: 330
    Median Score: 1047

Comment 23

[Tom S [:evilpie]]

Comment on attachment 8758113 [details] [diff] [review]
hasinstance-1-of-2.diff

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

Nice work. I think this is good to go, with one or two additional tests.

::: js/src/jsfun.cpp
@@ +684,5 @@
> +
> +    /* Step 1. */
> +    HandleValue func = args.thisv();
> +    if (!func.isObject()) {
> +        JS_ReportErrorNumber(cx, GetErrorMessage, nullptr,

Usually we would use ReportIncompatible(cx, args); for this.

@@ +904,5 @@
>      fun_resolve,
>      fun_mayResolve,
>      nullptr,                 /* finalize    */
>      nullptr,                 /* call        */
> +    InstanceOfOperator,

Can't we just set this to nullptr now?

::: js/src/tests/ecma_6/Function/has-instance.js
@@ +3,5 @@
> +let obj = { foo: true };
> +let C = function(){};
> +
> +Object.defineProperty(C, Symbol.hasInstance, {
> +  value: function(inst) { passed = inst.foo; return false; }

I would just have done passed = inst;

@@ +34,5 @@
> +    null,
> +    "nope",
> +]
> +
> +for (let nonCallable in nonCallables) {

of

@@ +37,5 @@
> +
> +for (let nonCallable in nonCallables) {
> +    assertEq(nonCallable instanceof Function, false);
> +    assertEq(nonCallable instanceof Object, false);
> +}

I think a few more tests with a custom Symbol.hasInstance implementation would be a good idea.

For example custom hasInstance implementations can allow non-callables.

for (let nonCallble ...) {
var o = {[Symbol.hasInstance](v) { return true; }}
assertEq(nonCallable instanceof o, true);
}

@@ +73,5 @@
> +assertEq(Function.prototype[Symbol.hasInstance].call(bound, instance), true);
> +assertEq(Function.prototype[Symbol.hasInstance].call(doubleBound, instance), true);
> +assertEq(Function.prototype[Symbol.hasInstance].call(tripleBound, instance), true);
> +
> +// A null/undefined @@hasInstance falls back to default behavior.

I don't quite understand this. It would probably be more obvious to just do something like:
var desc = Object.getOwnPropertyDescriptor(Function.prototype, Symbol.hasInstance)
desc.configurable == false etc.

::: js/src/vm/Interpreter.cpp
@@ +752,5 @@
> +        return false;
> +
> +    if (!hasInstance.isNullOrUndefined()) {
> +        if (!IsCallable(hasInstance)) {
> +            ReportValueError(cx, JSMSG_NOT_CALLABLE, -1, hasInstance, nullptr);

We use  ReportIsNotFunction for this, even if the name is not accurate. We should probably change that function to ReportIsNotFunction in a new bug.

@@ +780,5 @@
>  js::HasInstance(JSContext* cx, HandleObject obj, HandleValue v, bool* bp)
>  {
>      const Class* clasp = obj->getClass();
>      RootedValue local(cx, v);
>      if (JSHasInstanceOp hasInstance = clasp->getHasInstance())

// Non-standard, bug XXXX

Comment 24

[Jason Orendorff [:jorendorff]]

(In reply to Tom Schuster [:evilpie] from comment #23)
> >      if (JSHasInstanceOp hasInstance = clasp->getHasInstance())
> 
> // Non-standard, bug XXXX

Agreed! Good comment.

Comment 25

[Jan de Mooij [:jandem]]

Comment on attachment 8758115 [details] [diff] [review]
hasinstance-2-of-2.diff

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

Looks good. Thanks for checking Octane!

::: js/src/jit/BaselineIC.cpp
@@ +7763,5 @@
> +    // Function.prototype.
> +    if (!fun->hasStaticPrototype() || fun->hasUncacheableProto())
> +        return true;
> +
> +    JSObject* funProto = cx->global()->getPrototype(JSProto_Function).toObjectOrNull();

Nit: toObject() here and in IonBuilder. It requires an extra & but asserting non-null is nice.

::: js/src/jit/IonBuilder.cpp
@@ +13864,5 @@
> +            break;
> +
> +        HeapTypeSetKey hasInstanceObject =
> +            rhsKey->property(SYMBOL_TO_JSID(symbols->hasInstance));
> +        if (hasInstanceObject.isOwnProperty(constraints()) || rhsKey->unknownProperties())

Move the rhsKey->unknownProperties() call before doing rhsKey->property(), as property() will assert if unknownProperties.

::: js/src/jsfun.cpp
@@ +1158,5 @@
>  bool
> +js::FunctionHasDefaultHasInstance(JSFunction* fun, const WellKnownSymbols& symbols)
> +{
> +    if (!fun)
> +        return false;

Nit: it's fine to assume non-null here.

@@ +1164,5 @@
> +    jsid id = SYMBOL_TO_JSID(symbols.hasInstance);
> +    Shape* shape = fun->lookupPure(id);
> +    if (shape) {
> +        if (!shape->hasSlot())
> +            return false;

I'd also check shape->hasDefaultGetter(). Most places test both and it seems we still have some getters that have a slot (bug 1153592).

Comment 26

[Morgan Phillips [:mrrrgn]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c936f8e604d5

Comment 27

[Morgan Phillips [:mrrrgn]]

Attachment: hasinstance-1-of-2.diff

asking for an extra review here because of changes to a test which reads [on failure] 'You need a security audit from an XPConnect peer - got "[\"Symbol.hasInstance\"]", expected "[]"'

Comment 28

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8759837 [details] [diff] [review]
hasinstance-1-of-2.diff

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

I would love to help but after an hour I realized that I have too many questions to be the reviewer here. I would have to discuss a few things with Bobby before I can review this, but then probably it's easier if he does the review himself. It's likely trivial, but I'm not sure how we handle symbols within wrappers and I have only vague memories about the instanceof call path for JS xrays, so I don't feel comfortable reviewing this right now. Maybe you can try Boris if Bobby is not accepting review requests.

Comment 29

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8759837 [details] [diff] [review]
hasinstance-1-of-2.diff

The change to js/xpconnect/tests/chrome/test_xrayToJS.xul looks fine on its face: the property should be there.

One though: have you done any testing for how, if at all, this affects the behavior of instanceof across Xrays?  Do we have existing tests for that?  Most simply, in testArray() in this file we should check that trickyArray instanceof iwin.Array is true....

Comment 30

[Morgan Phillips [:mrrrgn]]

(In reply to Boris Zbarsky [:bz] from comment #29)
> Comment on attachment 8759837 [details] [diff] [review]
> hasinstance-1-of-2.diff
> 
> The change to js/xpconnect/tests/chrome/test_xrayToJS.xul looks fine on its
> face: the property should be there.
> 
> One though: have you done any testing for how, if at all, this affects the
> behavior of instanceof across Xrays?  Do we have existing tests for that? 
> Most simply, in testArray() in this file we should check that trickyArray
> instanceof iwin.Array is true....

Thanks for the review! try run with |trickyArray instanceof iwin.Array| test added: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a16c5af49987&selectedJob=22057912

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/35cc4a2451cc
Implement ES6 Symbol.hasInstance 1/2; r=evilpie,bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/6394d8078dfe
Implement ES6 Symbol.hasInstance 2/2; r=jandem

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/35cc4a2451cc
https://hg.mozilla.org/mozilla-central/rev/6394d8078dfe

Comment 33

[Florian Scholz (Open Web Docs)]

Docs:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Symbol
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Symbol/hasInstance
https://developer.mozilla.org/en-US/Firefox/Releases/50#JavaScript

Additional examples / review appreciated.