916949 - Change __noSuchMethod__ semantics to only invoke fallback function if called property does not exist

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Kannan Vijayan [:djvj]]

Currently, __noSuchMethod__ is called if a particular method property does not exist, or if it exists but yields a primitive result.

After discussion on the js-internals and spurred by bug 912303, it's been suggested that the best route is to change the behaviour so that only the first case (property binding does not exist) causes the invocation of __noSuchMethod__.

Comment 1

[Kannan Vijayan [:djvj]]

Attachment: refactor-property-access.patch

Refactor property access to be a bit cleaner.  This patch removes some unused helper functions, changes the calling of getGeneric hooks to depend on the getGeneric hook existing (instead of the getProperty hook existing), and also removes a vestigal "getHow" parameter that is not used anywhere, threaded through several of the property access routines, and always has the value 0.

Running through try before putting it up for review.

Comment 2

[Kannan Vijayan [:djvj]]

Comment on attachment 805522 [details] [diff] [review]
refactor-property-access.patch

Try looks good for cleanup:

https://tbpl.mozilla.org/?tree=Try&rev=f43682e6098a

Comment 3

[Jason Orendorff [:jorendorff]]

Comment on attachment 805522 [details] [diff] [review]
refactor-property-access.patch

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

r=me <3 <3

::: js/src/jit/BaselineIC.cpp
@@ +5951,5 @@
>      if (!obj)
>          return false;
>  
> +    if (obj->getOps()->getGeneric) {
> +        JS_ASSERT(obj->getOps()->getProperty);

For complete sureness you'd have to assert in both branches, or assert beforehand that !!getProperty == !!getGeneric.

But this whole if-statement looks pretty much identical to JSObject::getGeneric(). I guess you probably have further changes planned for this code, but if not, you can simplify.

::: js/src/jit/IonCaches.cpp
@@ +1740,2 @@
>              return false;
>      }

Same comments here.

::: js/src/jsobj.cpp
@@ +4079,5 @@
>      return true;
>  }
>  
>  bool
>  js_NativeGet(JSContext *cx, Handle<JSObject*> obj, Handle<JSObject*> pobj, Handle<Shape*> shape,

Please do me a favor and delete this incredibly obsolete comment on js_NativeGet in jsobj.h:

/*
 * NB: js_NativeGet and js_NativeSet are called with the scope containing shape
 * (pobj's scope for Get, obj's for Set) locked, and on successful return, that
 * scope is again locked.  But on failure, both functions return false with the
 * scope containing shape unlocked.
 */

lol

::: js/src/vm/Interpreter.cpp
@@ +279,4 @@
>          if (!JSObject::getGeneric(cx, obj, obj, id, vp))
>              return false;
>      } else {
> +        if (!baseops::GetProperty(cx, obj, obj, id, vp))

And here.

(Not 100% clear on why we have to have three copies of that code, to be quite honest. Feel free to pull the loose thread a bit more if you feel like it; I'll re-review quickly.)

Comment 4

[Kannan Vijayan [:djvj]]

Attachment: refactor-property-access.patch

(In reply to Jason Orendorff [:jorendorff] from comment #3)
> (Not 100% clear on why we have to have three copies of that code, to be
> quite honest. Feel free to pull the loose thread a bit more if you feel like
> it; I'll re-review quickly.)

Thanks for bringing that up.  I did look more closely into it, and it turns out that logic was simply replicating what's happening inside JSObject::getGeneric in the first place.  So that logic has condensed.

Other comments addressed, a quick r? again and I'm ready to push.  Running try in the meantime.

Comment 5

[Jason Orendorff [:jorendorff]]

Comment on attachment 807458 [details] [diff] [review]
refactor-property-access.patch

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

r=me with one fix:

::: js/src/jsobj.h
@@ +913,2 @@
>          if (op) {
> +            js::GenericIdOp op = obj->getOps()->getGeneric;

This seems like a typo; in `if (op)` it doesn't look like `op` is in scope yet. It seems like it wouldn't compile as written.

Comment 6

[Kannan Vijayan [:djvj]]

Indeed, it is.  Try pointed that out pretty quickly.  Already fixed.  Thanks for the quick review turnaround.

Comment 7

[Kannan Vijayan [:djvj]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c5550f96b7c1

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c5550f96b7c1

Comment 9

[Kannan Vijayan [:djvj]]

Attachment: nosuchmethod-undefined.patch

Change invocation of __noSuchMethod__ so that it only happens on undefined or nonexistant bindings, not all primitive bindings.

Comment 10

[Kannan Vijayan [:djvj]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/38d2cbc11868

Comment 11

[Kannan Vijayan [:djvj]]

Attachment: nosuchmethod-reftest.patch

Fix jsreftest to reflect new behaviour.

Comment 12

[Jason Orendorff [:jorendorff]]

Comment on attachment 819902 [details] [diff] [review]
nosuchmethod-reftest.patch

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

Stealing review.

Comment 13

[Kannan Vijayan [:djvj]]

Backed out for jsreftest failure: https://hg.mozilla.org/integration/mozilla-inbound/rev/0052a204cf83

Comment 14

[Kannan Vijayan [:djvj]]

Re-push after folding in jsreftest changes:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad6254a681f1

Comment 15

[Kannan Vijayan [:djvj]]

Just for refs, the try push to ensure that jsreftests are green for the latest push:
https://tbpl.mozilla.org/?tree=Try&rev=b3e9b0b528ad

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ad6254a681f1