Closed Bug 916949 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: djvj, Assigned: djvj)

References

Details

Attachments

(4 files)

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__.
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 on attachment 805522 [details] [diff] [review]
refactor-property-access.patch

Try looks good for cleanup:

https://tbpl.mozilla.org/?tree=Try&rev=f43682e6098a
Attachment #805522 - Flags: review?(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.)
Attachment #805522 - Flags: review?(jorendorff) → review+
(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.
Attachment #807458 - Flags: review?(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.
Attachment #807458 - Flags: review?(jorendorff) → review+
Indeed, it is.  Try pointed that out pretty quickly.  Already fixed.  Thanks for the quick review turnaround.
Change invocation of __noSuchMethod__ so that it only happens on undefined or nonexistant bindings, not all primitive bindings.
Attachment #819056 - Flags: review?(jorendorff)
Attachment #819056 - Flags: review?(jorendorff) → review+
Fix jsreftest to reflect new behaviour.
Attachment #819902 - Flags: review?(Ms2ger)
Comment on attachment 819902 [details] [diff] [review]
nosuchmethod-reftest.patch

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

Stealing review.
Attachment #819902 - Flags: review?(Ms2ger) → review+
Re-push after folding in jsreftest changes:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad6254a681f1
Just for refs, the try push to ensure that jsreftests are green for the latest push:
https://tbpl.mozilla.org/?tree=Try&rev=b3e9b0b528ad
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Assignee: general → kvijayan
Whiteboard: [leave open]
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.