761457 - Make NonGenericMethodGuard's signature more normal

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

Attachment: Patch

Right now NonGenericMethodGuard's signature is like this:

  JSObject* NonGenericMethodGuard(..., Class* clasp, bool* ok);

And the dance you do to properly call it looks basically like this:

  bool ok;
  JSObject* thisObj = NonGenericMethodGuard(..., clasp, &ok);
  if (!thisObj)
    return ok;
  // thisObj has clasp, carry on

The normal case is that a method returns NULL to indicate failure, outside the rare infallible method.  But this method's fallible!  So this pattern violates that idiom.

There's never going to be anything very nicely natural here, I think.  Maybe an object with class methods on it might be truly nice, but I couldn't think of a good, clean way to do it.  In the meantime, we should at least get rid of the failure-looking return that isn't.  Let's make the signature this instead:

  bool NonGenericMethodGuard(..., Class* clasp, JSObject** thisObj);

and the normal dance this:

  JSObject* thisObj;
  if (!NonGenericMethodGuard(..., clasp, &thisObj))
    return false;
  if (!thisObj)
    return true;
  // thisObj has clasp, carry on

The last condition's still a bit weird, to be sure.  But it does communicate failure the normal way, which is a big help in using it.

Comment 1

[Luke Wagner [:luke]]

Comment on attachment 630011 [details] [diff] [review]
Patch

Thanks for fixing this up; it does seem more regular this way.

>diff --git a/js/src/vm/MethodGuard.h b/js/src/vm/MethodGuard.h
>+ *   1. NonGenericMethodGuard returned false because it encountered an error:
>+ *      args.thisv wasn't an object, was an object of the wrong class, was a
>+ *      proxy to an object of the wrong class, or was a proxy to an object of
>+ *      the right class but the recursive call to args.callee failed.  This
...

can you take out the double spaces after .?

Comment 2

[Jeff Walden [:Waldo]]

Haters gonna hate.  I also had to make some minor adjustments to BoxedPrimitiveMethodGuard, sniffed out by a try run.

https://hg.mozilla.org/integration/mozilla-inbound/rev/7d12c871a707

Comment 3

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7d12c871a707

Comment 4

[Steve Fink [:sfink] [:s:]]

The new dance is even more tangled with rooting, btw. This goes from

  bool ok;
  RootedObject thisObj(NonGenericMethodGuard(..., clasp, &ok));
  if (!thisObj)
    return ok;
  ...

to

  JSObject* thisObjUnrooted;
  if (!NonGenericMethodGuard(..., clasp, &thisObj))
    return false;
  if (!thisObj)
    return true;
  RootedObject thisObj(thisObjUnrooted);
  ...

which makes me want something more like

  NonGenericMethodGuard guard(cx, args, native, clasp);
  if (guard.handled())
    return guard.status();
  RootedObject thisObj(guard.thisObj());
  ...

Comment 5

[Jan de Mooij [:jandem]]

When I merged this to IM I had to fix the ParallelArray uses and I noticed you can also write it like this (just one line shorter):

  RootedObject thisObjUnrooted(cx);
  if (!NonGenericMethodGuard(..., clasp, thisObj.address()))
    return false;
  if (!thisObj)
    return true;

Comment 6

[Steve Fink [:sfink] [:s:]]

Oh, you're right. That's far more tolerable. I can live with that.

Though I still prefer a different interface, mostly because I don't like the double-check. The previous version felt totally bizarre because the interface was returning two values with unfamiliar semantics, but once I figured it out it actually felt pretty good. Probably because my natural desire was to think of it as "do the wacky proxy stuff. Ok, did that handle it for me? Then return. Otherwise, go do the stuff." So I kind of want an API along those lines instead of "did the proxy gunk handle this unsuccessfully? How about successfully? No to both? Ok, go do the stuff."