Last Comment Bug 761457 - Make NonGenericMethodGuard's signature more normal
: Make NonGenericMethodGuard's signature more normal
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla16
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-04 17:35 PDT by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2012-06-11 15:10 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (46.50 KB, patch)
2012-06-04 17:35 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
luke: review+
Details | Diff | Splinter Review

Description Jeff Walden [:Waldo] (remove +bmo to email) 2012-06-04 17:35:34 PDT
Created attachment 630011 [details] [diff] [review]
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] 2012-06-04 17:51:12 PDT
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] (remove +bmo to email) 2012-06-05 10:29:13 PDT
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] 2012-06-06 08:44:44 PDT
https://hg.mozilla.org/mozilla-central/rev/7d12c871a707
Comment 4 Steve Fink [:sfink] [:s:] 2012-06-11 13:19:58 PDT
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] (PTO until July 31) 2012-06-11 13:56:36 PDT
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:] 2012-06-11 15:10:24 PDT
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."

Note You need to log in before you can comment on or make changes to this bug.