845478 - JS shell should use JS::CallArgs instead of manual argc/vp+JS_SET_RVAL/JS_ARGV/etc.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

We have to change the entire tree to CallArgs, and away from argc/vp and the JS_SET_RVAL/JS_CALLEE/JS_THIS/JS_ARGV macros, so we can change JSNative to take CallArgs& rather than argc and vp both.  Much of the JS engine's switched now in incremental updates, but the shell lags considerably.  So the shell needs concentrated fixing.

This is a pretty good first bug for someone to get interested in SpiderMonkey bugfixing, I think.  Happy to review patches here -- probably start with a small patch (or with questions, if you have them), get an okay on that, then go crazy.  :-)

Comment 1

[Julian Viereck]

I would love to do this and get started hacking spider monkey but I have no idea what needs to be done here. can you point me too some docs (I know about gecko c++ hacking).

Comment 2

[Jeff Walden [:Waldo]]

Great!  Unfortunately we don't really have docs for JS::CallArgs yet.  But the implementation of the class is super-simple -- see js/src/jsapi.h -- so it shouldn't be too bad.

The gist is that every function that's a JSNative -- that is, like so:

JSBool
FunctionName(JSContext *cx, unsigned argc, Value *vp)
{
    ...
}

(or jsval *vp for the last argument, jsval is a typedef to JS::Value) should change so that internally, it doesn't use argc/vp at all except to create a CallArgs, as the very first statement of the function:

    CallArgs argc = CallArgsFromVp(argc, vp);

Then, every place that currently uses |argc| should instead use |args.length()|.  And every place that uses vp should use one of a couple things.

JS_ARGV(cx, vp)[i] should change to args[i]
JS_SET_RVAL(cx, vp, value) should change to args.rval().set(value)
JS_CALLEE(cx, vp) should change to args.callee() (maybe a toObject() call is needed in there, I can't remember)
JS_THIS(cx, vp) should change to args.thisv()
JS_THIS_OBJECT(cx, vp)...hmm.  Leave this one alone for now, I'm not sure it's quite ready for conversion.
JS_RVAL(cx, vp) should ideally go away.  If it's ever used, args.rval() should be used to copy into a local variable, then that variable can be used as desired.  Use js::RootedValue v(cx, args.rval()) to store it, with whatever variable name makes sense.

Note that whenever JS_CALLEE is used, it has to be used before args.rval().  That *should* be the case already, but if it's not, a totally mechanical change would cause assertion failures.  Be careful, and watch out just in case.

As far as building SpiderMonkey goes, it's pretty simple:

  cd js/src
  autoconf-2.13 # or however it's named on your platform
  mkdir objdir
  cd objdir
  ../configure --enable-debug --disable-optimize
  make -s -j4

That'll give you a shell in objdir/js.  From js/src, you can run tests with:

  python tests/jstests.py objdir/js
  python jit-test/jit-test.py objdir/js

You can run subsets of tests as well by putting a substring of the test file name at the end of the command line.

If you need to know anything else, feel free to ask here or on #jsapi on IRC.

Everything cleared up?

Comment 3

[Julian Viereck]

Jeff, thanks a lot for your detailed comment! Looks good enough for me to get started on this.

Comment 4

[Terrence Cole [:terrence]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #2)
> 
>     CallArgs argc = CallArgsFromVp(argc, vp);
> 

I think you mean:
     CallArgs args = CallArgsFromVp(argc, vp);

Comment 5

[Jeff Walden [:Waldo]]

Er, yes.  :-)

Comment 6

[Jeff Walden [:Waldo]]

Bug 846976 has a patch documenting CallArgs and how to use it which may be helpful in understanding what needs doing here.  I don't anticipate it changing much, if at all, between now and the time it lands.  I suspect it mostly restates what was said in comment 2, but there might be some new stuff in it.

Comment 7

[Julian Viereck]

I plan to work on this tomorrow/Sunday evening. Thanks a lot for providing such a good outline of what needs to be done here - this really makes we want to do this!

Comment 8

[Julian Viereck]

I cannot compile from the objdir using the snippets from Comment #2. See error output below. Any ideas?

  File "/Volumes/develop/moz/js/callArgs-845478/js/src/config/expandlibs.py", line 47, in splitpath
    dir, file = os.path.split(path)
  File "/Volumes/develop/moz/js/callArgs-845478/js/src/objdir/_virtualenv/bin/../lib/python2.7/posixpath.py", line 85, in split
    if head and head != '/'*len(head):
RuntimeError: maximum recursion depth exceeded in cmp
make[1]: *** [libmozjs.dylib] Error 1
make: *** [default] Error 2

Comment 9

[Jeff Walden [:Waldo]]

Try running this in js/src, as my best guess at a fix, then retry the other build steps:

find . -name *.pyc | xargs rm

Comment 10

[Julian Viereck]

I got things compiling by avoiding '//' at the beginning of the path variable in `expandlibs.py` using the following code: 

    def splitpath(path):
        if (path.startswith('//')):        << NEW CODE
          path = path[1:]                  << NEW CODE
        dir, file = os.path.split(path)
        if os.path.splitdrive(dir)[1] == os.sep:
            return [file]
        print 'relativize-splitpath: %s, %s' % (dir, file)
        return splitpath(dir) + [file]


Does this makes any sense? The failing string was of the form '//usr/bin/clang'.

With these changes I the following errors running the tests:

$ python tests/jstests.py objdir/js

REGRESSIONS
    ecma/Date/15.9.5.35-1.js
    js1_5/extensions/toLocaleFormat-01.js
    js1_5/Regress/regress-58116.js
    js1_8/genexps/regress-380237-01.js

Is this something to worry about?

> python jit-test/jit-test.py objdir/js

I guess the file "jit-test.py" should be "jit_test.py"?

  python jit-test/jit-test.py objdir/js

All tests pass here :)

Comment 11

[Till Schneidereit [:till]]

> REGRESSIONS
>     ecma/Date/15.9.5.35-1.js
>     js1_5/extensions/toLocaleFormat-01.js
>     js1_5/Regress/regress-58116.js
>     js1_8/genexps/regress-380237-01.js
> 
> Is this something to worry about?

No, those tests will keep failing for you. :( Partly, they rely on a specific locale, partly they only work in PST (I think). Would certainly be nice to fix them for us non-Californians (and -Oregonians).

Comment 12

[Julian Viereck]

Unassign myself - sigh, but it doesn't look promising I can get to this very soon and I don't want to keep this mentor bug reserved.

Comment 13

[Do Nhat Minh]

hi, my name is Do Nhat Minh. I'm interested in fixing this bug. Can I get assigned to this bug?

Comment 14

[Jeff Walden [:Waldo]]

Sure, go for it.  If you need anything beyond comment 2 and js/public/CallArgs.h, please comment here.  I'm a bit loaded these days, but if you ask here -- or on IRC in #jsapi, plenty of friendly people there to answer questions -- *someone* should answer relatively quickly.

Comment 15

[Do Nhat Minh]

I will. Thanks.

Comment 16

[Do Nhat Minh]

Attachment: All requirements met. Waiting for review.

Hi, I have submitted a fix. Please checkout my patch. Any suggestions are welcomed.

While testing, these regressions are reported. I trust that they are not related?

> REGRESSIONS
>     js1_5/extensions/toLocaleFormat-01.js
>     js1_5/Regress/regress-58116.js
>     ecma/Date/15.9.5.24-2.js
>     ecma/Date/15.9.5.5.js
>     ecma/Date/15.9.3.2-1.js
>     ecma/Date/15.9.5.32-1.js
>     ecma/Date/15.9.5.36-3.js
>     ecma/Date/15.9.5.25-1.js
>     ecma/Date/15.9.5.24-3.js
>     ecma/Date/15.9.5.36-7.js
>     ecma/Date/15.9.5.36-5.js
>     ecma/Date/15.9.5.22-2.js
>     ecma/Date/15.9.5.33-1.js
>     ecma/Date/15.9.5.23-1.js
>     ecma/Date/15.9.5.6.js
>     ecma/Date/15.9.5.36-6.js
>     ecma/Date/15.9.5.23-17.js
>     ecma/Date/15.9.5.24-4.js
>     ecma/Date/15.9.5.24-8.js
>     ecma/Date/15.9.5.23-11.js
>     ecma/Date/15.9.5.8.js
>     ecma/Date/15.9.3.2-3.js
>     ecma/Date/15.9.5.27-1.js
>     ecma/Date/15.9.3.8-5.js
>     ecma/Date/15.9.5.35-1.js
>     ecma/Date/15.9.5.23-10.js
>     ecma/Date/15.9.5.16.js
>     ecma/Date/15.9.5.28-1.js
>     ecma/Date/15.9.5.22-4.js
>     ecma/Date/15.9.5.23-16.js
>     ecma/Date/15.9.5.22-1.js
>     ecma/Date/15.9.5.24-7.js
>     ecma/Date/15.9.5.24-5.js
>     ecma/Date/15.9.5.36-1.js
>     ecma/Date/15.9.5.37-1.js
>     ecma/Date/15.9.5.22-3.js
>     ecma/Date/15.9.5.29-1.js
>     ecma/Date/15.9.5.23-13.js
>     ecma/Date/15.9.3.8-1.js
>     ecma/Date/15.9.3.1-1.js
>     ecma/Date/15.9.5.30-1.js
>     ecma/Date/15.9.5.37-5.js
>     ecma/Date/15.9.3.8-2.js
>     ecma/Date/15.9.5.23-15.js
>     ecma/Date/15.9.5.26-1.js
>     ecma/Date/15.9.5.24-1.js
>     ecma/Date/15.9.3.1-4.js
>     ecma/Date/15.9.5.36-4.js
>     ecma/Date/15.9.5.24-6.js
>     ecma/Date/15.9.5.36-2.js
>     ecma/Date/15.9.5.31-1.js
>     ecma_5/Date/15.9.4.2.js

Comment 17

[Do Nhat Minh]

Attachment: Patch V2

Oops, I forgot about argc.

Attached is the new patch that also replaces `argc` in the code. The remaining instances of `argc` are more subtle, so I didn't touch them.

This new patch also has the above mentioned regressions.

Comment 18

[Do Nhat Minh]

Attachment: Patch V3

This patch revert back to using JS_THIS_OBJECT, which is in the requirement.

Comment 19

[Jeff Walden [:Waldo]]

Wow, you went a lot further than I'd meant this bug to go!  I'd only meant to change js/src/shell/js.cpp, and leave the rest to other bugs.  It's great to see you do this to (apparently) all of SpiderMonkey!

I'll leave slightly more substantive responses to subsequent comments.  I just wanted to get out there that it's great to see you doing so much more than I'd expected, before I dive into preliminary thoughts on the patch.

Comment 20

[Jeff Walden [:Waldo]]

This is a biiiiiiig patch.  :-)  Might I be able to convince you to split it up into several patches?  It's much easier to get reviews on ten 20K patches, even all from the same person (although I might try to enlist a couple other people to review some of the patches here, to keep things moving), than it is to get one review on a 200K patch.  It's easier to finish and submit a small review, than it is to keep chewing off bits and pieces of a really large patch until it's all complete.  This works both ways -- if I have comments, I can give you a small set of changes to make to a small patch, versus a possibly-huge list for a huge patch.  Plus, we can land early parts even if later parts haven't been reviewed yet.

Now, assuming you're willing to do that, how to split it a little.

I happened to notice that IsConstructing is a standalone method right now.  This wasn't part of the original bug, but could I maybe convince you to move IsConstructing() to CallReceiver::isConstructing(), and to update the documentation in CallArgs.h for the change?  It seems clear on a closer look that these methods should live in CallReceiver, not on their own.

Once that's done, I think the rest of this can be split up pretty much any way you want.  If it were me, I'd probably try to do half of js/src/shell/js.cpp in one patch, the other half in another, half of js/src/ctypes/CTypes.cpp in one patch, the other half in another, and all the remaining changes in one last patch.

All these changes are relatively simple, so *in theory* they're easy reviews.  But it's hard to maintain full attention through a too-long patch.  So splitting up further is better than not splitting up.

Comment 21

[Do Nhat Minh]

Thanks for the kind comments. I will get to it when I get back to my place.

Comment 22

[Do Nhat Minh]

Attachment: Patch for various test files

Comment 23

[Do Nhat Minh]

Attachment: Patch for the rest of test files

Comment 24

[Do Nhat Minh]

Attachment: Patch for various js* files

Comment 25

[Do Nhat Minh]

Attachment: Patch for some other js* files

Comment 26

[Do Nhat Minh]

Attachment: Patch for Library.cpp

Comment 27

[Do Nhat Minh]

Attachment: Patch for Profilers.cpp

Comment 28

[Do Nhat Minh]

Attachment: Patch for TestingFunctions.cpp.patch

Comment 29

[Do Nhat Minh]

Attachment: First patch for js.cpp

Comment 30

[Do Nhat Minh]

Attachment: Second patch for js.cpp

Comment 31

[Do Nhat Minh]

Attachment: Third patch for js.cpp

Comment 32

[Do Nhat Minh]

Attachment: Forth patch for js.cpp

Comment 33

[Do Nhat Minh]

Attachment: Fifth patch for js.cpp

Comment 34

[Do Nhat Minh]

Attachment: Sixth patch for js.cpp

Comment 35

[Do Nhat Minh]

Attachment: Seventh patch for js.cpp

Comment 36

[Do Nhat Minh]

Attachment: Eighth patch for js.cpp

Comment 37

[Do Nhat Minh]

Attachment: Ninth patch for js.cpp

Comment 38

[Do Nhat Minh]

Attachment: Tenth patch for js.cpp

Comment 39

[Do Nhat Minh]

Attachment: Eleventh patch for js.cpp

Comment 40

[Do Nhat Minh]

Oops, just reread your comment. Somehow the number 200 stuck into my mind and I translated that into 200 lines per patch. Sorry for the long list of emails. I'll split CTypes.cpp's patch in two and upload them now.

Comment 41

[Do Nhat Minh]

Attachment: First patch for CTypes.cpp

Comment 42

[Do Nhat Minh]

Attachment: Second patch for CTypes.cpp

Comment 43

[Do Nhat Minh]

Regarding moving IsConstructing inside CallReceiver, according to :jorendorff, JSObject's implementation should not be made public, so I can't do it like how it's done in js/src/jsfuninlines.h. On the other hand, I can't do it like JS_IsConstructing, due to lack of a JSContext*.

Is there any other way to do this?

Comment 44

[Jason Orendorff [:jorendorff]]

I'm fine with just changing the function not to take a cx argument. Or adding a JS_FRIEND_API function that doesn't take a cx argument.

Comment 45

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 727634 [details] [diff] [review]
Patch for Profilers.cpp

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

::: js/src/builtin/Profilers.cpp
@@ +230,4 @@
>          return JS_TRUE;
>      }
>  
> +    RequiredStringArg profileName(cx, args.length(), vp, 0, "startProfiling");

Still using vp for all RequiredStringArg users.

Comment 46

[Jeff Walden [:Waldo]]

(In reply to Do Nhat Minh from comment #43)
> Regarding moving IsConstructing inside CallReceiver, according to
> :jorendorff, JSObject's implementation should not be made public, so I can't
> do it like how it's done in js/src/jsfuninlines.h.

The funky stuff there is only there to do a little bit of extra asserting.  I can't remember those assertions ever triggering.  I think we can just remove those problematic bits, and have isConstructing() just be |return vp.isMagic(JS_IS_CONSTRUCTING)| or its equivalent.  (Not sure why the magic-value was missing before -- we should add it, definitely, although we'll want tryservering to be sure the addition doesn't trigger any assertions.)

Comment 47

[Do Nhat Minh]

Attachment: New patch for Profilers.cpp

Changed RequiredStringArg to using CallArgs

Comment 48

[Do Nhat Minh]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #46)
> (In reply to Do Nhat Minh from comment #43)
> > Regarding moving IsConstructing inside CallReceiver, according to
> > :jorendorff, JSObject's implementation should not be made public, so I can't
> > do it like how it's done in js/src/jsfuninlines.h.
> 
> The funky stuff there is only there to do a little bit of extra asserting. 
> I can't remember those assertions ever triggering.  I think we can just
> remove those problematic bits, and have isConstructing() just be |return
> vp.isMagic(JS_IS_CONSTRUCTING)| or its equivalent.  (Not sure why the
> magic-value was missing before -- we should add it, definitely, although
> we'll want tryservering to be sure the addition doesn't trigger any
> assertions.)

I see. But what's tryservering?

Comment 49

[Do Nhat Minh]

I'll remove IsConstructing from js/src/jsfuninlines.h

Should I remove JS_IsConstructing, too?

Comment 50

[Jeff Walden [:Waldo]]

(In reply to Do Nhat Minh from comment #48)
> we should add it, definitely, although
> > we'll want tryservering to be sure the addition doesn't trigger any
> > assertions.)
> 
> I see. But what's tryservering?

Besides the main tree, Mozilla has a test tree.  If you push any change to it, it'll build the change on whatever set of platforms and with whatever set of build settings you want, and test it too.  Just something we do when we're uncertain whether a change is safe to make.  I'll do it this time around for the patches; in the longer run you can get commit access to the Try repository and test things yourself.

I'd leave JS_IsConstructing in for now.  If you read CallArgs.h you saw we have plans to change JSNative to take a const CallArgs&.  The time to remove JS_IsConstructing is probably then -- make a clean break of it.  In the meantime everything old still works, and people are free to use it (but recommended to stop doing so).

Comment 51

[Do Nhat Minh]

Okay. Can I change JS_IsConstructing to using CallReceiver::isConstructing?

When I replace `vp + 2` with `&callArgs[0]`, I get jit-test errors starting from test 107. Looking at the definition of PodCopy, I can't see why it's failing. Can you help clarify this?

Comment 52

[Do Nhat Minh]

Sorry, more info, I'm changing js/src/jsfun.cpp:1111

Comment 53

[Do Nhat Minh]

Attachment: New new patch for Profilers.cpp

Change RequiredStringArg constructor to using const CallArgs& instead of CallArgs.

Obsoleting Patch V3 as the current patch diverges from it.

Comment 54

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 728010 [details] [diff] [review]
New new patch for Profilers.cpp

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

r=me

::: js/src/builtin/Profilers.cpp
@@ +206,1 @@
>              JS_ReportError(cx, "%s: not enough arguments", caller);

You don't have to address this, but I just noticed that every caller already makes sure this doesn't happen.

@@ +293,1 @@
>          ret = JS_DumpProfile(NULL, NULL);

If you want, you could also rewrite this function a bit to assign to args.rval() directly and return early, instead of keeping the ret local; that would be closer to the other functions here. (Either way is fine with me.)

Comment 55

[Do Nhat Minh]

(In reply to :Ms2ger from comment #54)
> Comment on attachment 728010 [details] [diff] [review]
> New new patch for Profilers.cpp
> 
> Review of attachment 728010 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me
> 
> ::: js/src/builtin/Profilers.cpp
> @@ +206,1 @@
> >              JS_ReportError(cx, "%s: not enough arguments", caller);
> 
> You don't have to address this, but I just noticed that every caller already
> makes sure this doesn't happen.

A function not checking its inputs feels wrong to me. Future code might not do that check before calling. So I guess I'll leave that as is.

> @@ +293,1 @@
> >          ret = JS_DumpProfile(NULL, NULL);
> 
> If you want, you could also rewrite this function a bit to assign to
> args.rval() directly and return early, instead of keeping the ret local;
> that would be closer to the other functions here. (Either way is fine with
> me.)

I'll leave this one as is, too.

Comment 56

[Do Nhat Minh]

My bad. C++'s references, which is what CallArgs::operator[] is returning, aren't supposed to be taken address of.

Comment 57

[Do Nhat Minh]

Attachment: First part of CTypes.cpp

Comment 58

[Do Nhat Minh]

Attachment: Second part for CTypes.cpp

Comment 59

[Do Nhat Minh]

Attachment: First part for js.cpp

Comment 60

[Do Nhat Minh]

Attachment: Second part for js.cpp

Comment 61

[Do Nhat Minh]

Attachment: First patch for various files

Comment 62

[Do Nhat Minh]

Attachment: Second patch for various files

Comment 63

[Do Nhat Minh]

Sorry, I accidentally obsoleted that. It survives the merge.

Comment 64

[Do Nhat Minh]

The newly uploaded patches are the ones that are merged with origin/master at 110f758308d68...

They also move IsConstructing in js/src/jsfuninlines.cpp into CallReceiver in js/public/CallArgs.h, change JS_IsConstructing to using CallReceiver::isConstructing and fix all code to call CallReceiver::isConstructing instead of the other two.

Comment 65

[Jeff Walden [:Waldo]]

Comment on attachment 728187 [details] [diff] [review]
First part of CTypes.cpp

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

I looked at about a third or so of this, and it's mostly on the right track.  I'm a little surprised you're getting rid of *_TO_JSVAL and JSVAL_TO_* in a few places, even beyond the ones where the value fed directly into an rval().  But there's nothing wrong with making such improvements (although be careful about putting too many distinct things in one patch -- generally fixing one thing at a time in a patch is best).

However, I saw a bunch of use of val.toObject() (which returns a JSObject&) in contexts where a JSObject* was required -- you're missing & in a bunch of places.  I'm happy to review patches that compile, but I don't have enough time to help out with patches that don't.  :-(  Could you put a little work into compiling these, and fixing any errors that manifest?  Thanks!

::: js/src/ctypes/CTypes.cpp
@@ +3081,5 @@
>  
>  JSBool
>  CType::ConstructData(JSContext* cx,
>                       unsigned argc,
>                       jsval* vp)

We'd like to get rid of all uses of argc/vp, to transition to CallArgs.  This method has more uses of argc/vp below, which should be converted to pass const CallArgs& parameters.  Nothing wrong with the changes here, just that they're not quite complete changes.  :-)

I'd suggest taking a look back when you're done and finding all the places where argc/vp are still used, outside of direct calls to CallArgsFromVp.  I think we want to land this patch and get that incremental gain, then have other fixes in separate patches.  No good reason to hold up the entire thing because of incompleteness, seems to me.

@@ +3742,5 @@
>  {
>    JS_ASSERT(CType::IsCType(obj));
>  
>    jsval slot = JS_GetReservedSlot(obj, SLOT_PROTO);
> +  JSObject* prototype = slot.toObject();

Hmm, that's not going to compile -- needs a &.

@@ +3752,3 @@
>      return JS_TRUE;
>  
> +  JSObject* proto = v.toObject();

Nor is this!

@@ +3781,3 @@
>  
> +  JS_ASSERT(!valCTypes.isPrimitive());
> +  return valCTypes.toObject();

Heh, those assertions are pretty awesome.  You're missing a & here as well.

@@ +3851,2 @@
>    RootedObject obj(cx);
> +  if (arg.isPrimitive() || !CType::IsCType(obj = arg.toObject())) {

More & missing.  This looks like something you'll want to fix everywhere.  Have you managed to compile a Firefox build?  You should definitely invest some time to learn how to do that, before you try doing too much here.  #introduction on IRC has people who can help with this, if you catch it at a time when people are around to help.

@@ +3866,5 @@
>  PointerType::CreateInternal(JSContext* cx, HandleObject baseType)
>  {
>    // check if we have a cached PointerType on our base CType.
>    jsval slot = JS_GetReservedSlot(baseType, SLOT_PTR);
> +  if (!slot.isVoid())

slot.isUndefined(); we used to call |undefined| "void" in APIs, but it's much clearer making it "undefined" now, as that's the direction specs went.

@@ +3966,2 @@
>        thisObj = NULL;
> +    } else if (!args[1].isPrimitive()) {

This is probably better written as |args[1].isObject()|.  There used to be a JSVAL_IS_OBJECT, that would return true for either an object *or* null.  JSVAL_IS_PRIMITIVE was introduced to provide what most people really meant, and it propagated pretty much everywhere.  Then we ended up removing JSVAL_IS_OBJECT completely to eliminate the issue.  Now that we have methods on Value that actually behave the way you'd expect, unless you're thinking in terms of primitiveness it's best to use isObject()/toObject() directly.

Comment 66

[Do Nhat Minh]

I don't know why but on my machine, `make` does not recompile when I change js/src/ctypes/CTypes.cpp, which is why I don't see compilation errors on those. I'm using ArchLinux X64.

Comment 67

[Do Nhat Minh]

I just fixed some of the errors in js/src/ctypes/CTypes.cpp and redid a `make clean && make -sj2`. The compilation seems not to include that file at all. Can you give me any clue on this?

Comment 68

[Do Nhat Minh]

it turns out I need to enable ctypes when configure the project, I.e. /path/to/configure --enable-debug --disable-optimize --enable-ctypes

Comment 69

[Do Nhat Minh]

Attachment: First patch for CTypes.cpp

Comment 70

[Do Nhat Minh]

Attachment: Second patch for CTypes.cpp

Comment 71

[Do Nhat Minh]

Attachment: First patch for js.cpp

Comment 72

[Do Nhat Minh]

Attachment: Second patch for js.cpp

Comment 73

[Do Nhat Minh]

Attachment: First patch for various files

Comment 74

[Do Nhat Minh]

Attachment: Second patch for various files

Comment 75

[Do Nhat Minh]

The new batch of patches fixes the problems in js/src/ctypes/CTypes.cpp and revert the use of Value::isPrimitive back to JSVAL_IS_PRIMITIVE

Comment 76

[Do Nhat Minh]

@Waldo do you have any comments for these patches?

Comment 77

[Jeff Walden [:Waldo]]

Comment on attachment 728726 [details] [diff] [review]
First patch for CTypes.cpp

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

Sorry for the delay -- I have a lot of different patches to juggle and review, amidst my own code to write, so I'm not always going to get to patches within a day or so -- more likely from a few days up to a week or a little longer.  Other people besides me can review patches, too, if you want to split the load across people.  :-)  :Ms2ger, for example, can review these patches.  (Doubtless Ms2ger is now going to look for something to throw at me.  ;-) )

As a general comment, you should put a summary in your patches and associate your name/email address with them, perhaps like this:

Bug 845478 - Use CallArgs to access argc/vp in most places in half of CTypes.cpp.  r=jwalden

qrefresh and qnew have a -m argument for specifying the message on the commandline, and -e for opening up an editor to set it.  The user name can be set via -u "...".  But if it's your name, setting the username in ~/.hgrc means you can use -U to add it.  Personally, I have this in my ~/.hgrc so I never have to add it manually for my own patches:

[defaults]
qnew = -U

::: js/src/ctypes/CTypes.cpp
@@ +4083,4 @@
>                              JSHandleId idval,
>                              MutableHandleValue vp)
>  {
> +  CallReceiver receiver = CallReceiverFromVp(vp.address());

(JSContext *, HandleObject, HandleId, MutableHandleValue) methods -- that is, JSPropertyOp -- don't follow the CallArgs convention at all.  There *is* a plan to convert these to JSNative in JSPropertySpec arrays, but that's definitely a separate bug/issue.  So you shouldn't add CallArgs uses (CallReceiver here -- there's a fair chance we'll integrate CallReceiver into CallArgs at some point, to have only one class here) to methods like this right now.

Given that I only see one other use of CallReceiver in this file (which does want to be CallArgs, but not this way -- rather by making the two callers create the CallArgs and pass it down to the helper method), and I can fix that one and this one pretty easily, I'll stamp this with an r+ and queue it up to land when I have time (probably within a day, but no guarantees).

For the other patches, tho, if there's more than a couple cases like this, I'll probably throw the patch back at you to correct.  :-)

Comment 78

[Do Nhat Minh]

Thanks, I will do that the next time around.

Comment 79

[Jeff Walden [:Waldo]]

First bit of ctypes changing landed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/4c42018a4754

Comment 80

[Do Nhat Minh]

cool! Thanks for seeing it through.

about the patch for Profilers.CPP, can you land it in the main tree, too?

I've emailed :Ms2ger for help with reviewing but he/she hasn't replied.

Comment 81

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4c42018a4754

Comment 82

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

I'm going to play my "I'm not a peer" card here... I bet evilpie could do it.

Comment 83

[Tom S [:evilpie]]

Comment on attachment 728729 [details] [diff] [review]
Second patch for js.cpp

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

Good effort here. This code still has tons of C-style stuff and is kind of weird. There are a lot of smaller things, you probably didn't know about, so I am giving you a r-. 

The most obvious pattern is args.length() < 1 ? UndefinedValue() : args[0], can be much cleaner written as args.get(0).
There are some other issues, but I didn't bother you with them, because they are per-existing.

::: js/src/shell/js.cpp
@@ +2342,4 @@
>      JSPropertyDesc *pd;
>      jsval v;
>  
> +    CallArgs args = CallArgsFromVp(argc, vp);

Try moving at the top of the function.

@@ +2343,5 @@
>      jsval v;
>  
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +
> +    if (!JS_ValueToObject(cx, args.length() == 0 ? UndefinedValue() : args[0], vobj.address()))

This can be args.get(0)

@@ +2393,3 @@
>      RootedScript script(cx);
>  
> +    script = ValueToScript(cx, args.length() == 0 ? UndefinedValue() : args[0]);

.get(0) as well.

@@ +2406,3 @@
>      int32_t i;
>  
> +    if (!JS_ValueToInt32(cx, args.length() == 0 ? UndefinedValue() : args[0], &i))

get(0)

@@ +2630,2 @@
>      JS::Value v;
> +    if (args.length() < 1 || !((v = args[0]).isObject())) {

Remove the JS::Value v;
Make this if (!args.get(0).isObject())

@@ +2632,4 @@
>          JS_ReportError(cx, "shapeOf: object expected");
>          return false;
>      }
>      JSObject *obj = &v.toObject();

&args[0].toObject

@@ +2632,5 @@
>          JS_ReportError(cx, "shapeOf: object expected");
>          return false;
>      }
>      JSObject *obj = &v.toObject();
> +    args.rval().setDouble((double) ((uintptr_t)obj->lastProperty() >> 3));

C++ style cast
double(uintptr_r(obj->lastProperty()) >> 3)

@@ +2776,4 @@
>  {
>      int64_t t_ticks;
>  
> +    CallArgs args = CallArgsFromVp(argc, vp);

To the top

@@ +2781,5 @@
>          t_ticks = 0;
>      } else {
>          double t_secs;
>  
> +        if (!JS_ValueToNumber(cx, args.length() == 0 ? UndefinedValue() : args[0], &t_secs))

get()

@@ +3085,5 @@
>          JS_ReportError(cx, "Wrong number of arguments");
>          return false;
>      }
>  
> +    jsval v = args[0];

Value

@@ +3090,1 @@
>      if (JSVAL_IS_PRIMITIVE(v)) {

isPrimitive()

@@ +3112,5 @@
>          JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_MORE_ARGS_NEEDED,
>                               "compile", "0", "s");
>          return false;
>      }
> +    jsval arg0 = args[0];

Value

@@ +3390,1 @@
>      if (JSVAL_IS_PRIMITIVE(v)) {

Value v = args.get(0);
if (v.isPrimitive()) {

@@ +3432,5 @@
>  
>  static JSBool
>  Serialize(JSContext *cx, unsigned argc, jsval *vp)
>  {
> +    CallArgs args = CallArgsFromVp(argc, vp);

RootedValue v(cx, args.get(0))

@@ +3456,5 @@
>  static JSBool
>  Deserialize(JSContext *cx, unsigned argc, jsval *vp)
>  {
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +    Rooted<jsval> v(cx, args.length() > 0 ? args[0] : UndefinedValue());

RootedValue v(cx, args.get(0))

@@ +3980,4 @@
>  {
>      fprintf(gOutFile, "%s\n", JS_GetImplementationVersion());
>  
> +    CallArgs args = CallArgsFromVp(argc, vp);

To the top.

@@ +4000,5 @@
>              if (!PrintHelp(cx, obj))
>                  return false;
>          }
>      } else {
> +        CallArgs args = CallArgsFromVp(argc, vp);

You already have args at the top.

@@ +4002,5 @@
>          }
>      } else {
> +        CallArgs args = CallArgsFromVp(argc, vp);
> +        for (unsigned i = 0; i < args.length(); i++) {
> +            if (JSVAL_IS_PRIMITIVE(args[i])) {

args[0].isPrimitive()

@@ +4233,1 @@
>      JSObject *obj = JS_THIS_OBJECT(cx, vp);

if (!args.thisv().isObject()) {
    args.rval.setUndefined()
    return true;
}
JSObject *obj = &args.thisv.toObject();

@@ +4233,2 @@
>      JSObject *obj = JS_THIS_OBJECT(cx, vp);
>      if (!obj)

Can obviously go away after that.

@@ +4250,1 @@
>      JSObject *obj = JS_THIS_OBJECT(cx, vp);

Same here.

@@ +4299,5 @@
>      bool ok;
>      pid_t pid;
>      int status;
>  
> +    CallArgs args = CallArgsFromVp(argc, vp);

To the top

@@ +4306,2 @@
>  
>      fun = JS_ValueToFunction(cx, vp[0]);

I am not sure what this code is even doing here. Don't worry about it.

@@ +4667,1 @@
>      RootedObject obj(cx, JS_THIS_OBJECT(cx, vp));

Like a few above.

if (!args.thisv().isObject())
...
return

RootedObject obj(cx, &args.thisv())

@@ +4688,1 @@
>      RootedObject obj(cx, JS_THIS_OBJECT(cx, vp));

again

Comment 84

[Tom S [:evilpie]]

Comment on attachment 728730 [details] [diff] [review]
First patch for various files

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

Looks quite good as well. Changing this to feedback+. Please attach a new patch. Maybe next time don't do such massive changes at once, without previous feedback.

::: js/public/CallArgs.h
@@ +177,5 @@
> +
> +  public:
> +    /*
> +     * If isConstructing() is true, thisv() must not be used; the constructor
> +     * should construct and return a new object. Otherwise, the native is

nit: "the constructor should construct" sounds strange.

@@ +180,5 @@
> +     * If isConstructing() is true, thisv() must not be used; the constructor
> +     * should construct and return a new object. Otherwise, the native is
> +     * called as an ordinary function and thisv() may be used.
> +     */
> +    bool isConstructing() const {

Nice change!

::: js/src/builtin/TestingFunctions.cpp
@@ +190,4 @@
>       * object, we collect the object's compartment (and any other compartments
>       * scheduled for GC). Otherwise, we collect all compartments.
>       */
> +    CallArgs args = CallArgsFromVp(argc, vp);

To the top.

@@ +252,1 @@
>      }

All this can be something like:
JS::RootedString str(cx, JS_ValueToString(args.get(0)));
if (!str)
   return false;

@@ +375,5 @@
>          RootedObject callee(cx, &args.callee());
>          ReportUsageError(cx, callee, "Too many arguments");
>          return JS_FALSE;
>      }
> +    if (!JS_ValueToECMAUint32(cx, args.length() < 1 ? JSVAL_VOID : args[0], &zeal))

args.get(0)

@@ +422,3 @@
>  
> +    for (unsigned i = 0; i < args.length(); i++) {
> +        Value arg(args[i]);

Just for consistency (totally not caring about the potential rooting problems):
Value arg = args[i];

@@ +437,4 @@
>  {
>      CallArgs args = CallArgsFromVp(argc, vp);
>  
> +    if (args.length()) {

> 0

@@ +450,5 @@
>  static JSBool
>  VerifyPostBarriers(JSContext *cx, unsigned argc, jsval *vp)
>  {
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +    if (args.length()) {

> 0

@@ +666,4 @@
>      JSCountHeapNode *node;
>      size_t counter;
>  
> +    CallArgs args = CallArgsFromVp(argc, vp);

Top

@@ +818,4 @@
>  
>      fclose(dumpFile);
>  
> +    args.rval().set(JSVAL_VOID);

setUndefined()

@@ +885,4 @@
>      cx->runtime->spsProfiler.enableSlowAssertions(args[0].toBoolean());
>      cx->runtime->spsProfiler.enable(true);
>  
> +    args.rval().set(JSVAL_VOID);

setUndefined()

::: js/src/ctypes/Library.cpp
@@ +203,4 @@
>      return JS_FALSE;
>    }
>  
> +  if (args.length() != 1 || args[0].isUndefined()) {

if (!args.hasDefined(0))

@@ +223,1 @@
>    JSObject* obj = JS_THIS_OBJECT(cx, vp);

I had this in the other patch as well. Please check if you could use thisv() here.

@@ +237,4 @@
>    UnloadLibrary(obj);
>    JS_SetReservedSlot(obj, SLOT_LIBRARY, PRIVATE_TO_JSVAL(NULL));
>  
> +  args.rval().set(JSVAL_VOID);

setUndefined

@@ +248,1 @@
>    RootedObject obj(cx, JS_THIS_OBJECT(cx, vp));

thisv() maybe.

@@ +296,4 @@
>        return JS_FALSE;
>    } else {
>      // Case 2).
> +    if (JSVAL_IS_PRIMITIVE(args[1]) ||

args[1].isPrimitive()

::: js/src/jsapi-tests/testDebugger.cpp
@@ +268,5 @@
>          JS_ASSERT(script);
>  
>          if (!JS_SetSingleStepMode(cx, script, true))
>              return false;
> +        args.rval().set(JSVAL_VOID);

setUndefined

Comment 85

[Tom S [:evilpie]]

Comment on attachment 728731 [details] [diff] [review]
Second patch for various files

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

Sorry I wont get to do this until I am back around Friday.

::: js/src/jsbool.cpp
@@ +136,3 @@
>          JSObject *obj = BooleanObject::create(cx, b);
>          if (!obj)
> +            return JS_FALSE;

no.

Comment 86

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 728010 [details] [diff] [review]
New new patch for Profilers.cpp

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

Comment 87

[Ryan VanderMeulen [:RyanVM]]

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

Comment 88

[Jeff Walden [:Waldo]]

Comment on attachment 728728 [details] [diff] [review]
First patch for js.cpp

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

This is pretty much reasonable.  I've only been skimming the other comments, but I did see evilpie say we want CallArgs at the very top of functions.  He's definitely right about that.  (This makes it easier to remove them, when we get around to changing JSNative to take |const CallArgs&|.)  Also -- looking at PrintInternal specifically -- we want the callers of utility functions like that (the actual JSNatives, that is) to create the CallArgs, then pass that to the utility functions underneath.

The smattering of other jsval->Value changes are okay and all, although you were a bit inconsistent in making them, to be sure.  Forward progress beats no progress, tho, so I'll take it as-is.

Anyway, I fixed up the nits I saw and will push the adjusted patch.  Thanks!  I'm multitasking on reviews, so possibly you might be able to fix up the other patches accordingly before I can get to the others?

Comment 89

[Do Nhat Minh]

(In reply to Tom Schuster [:evilpie] from comment #83)
> @@ +4233,1 @@
> >      JSObject *obj = JS_THIS_OBJECT(cx, vp);
> 
> if (!args.thisv().isObject()) {
>     args.rval.setUndefined()
>     return true;
> }
> JSObject *obj = &args.thisv.toObject();

Could you tell me why you return true in this case while the old code returns false?

Comment 90

[Do Nhat Minh]

Sorry I made a mistake asking for a checkin again.

Comment 91

[Do Nhat Minh]

Attachment: Second patch for js.cpp

Comment 92

[Do Nhat Minh]

Attachment: First patch for various files

Comment 93

[Jeff Walden [:Waldo]]

Comment on attachment 728727 [details] [diff] [review]
Second patch for CTypes.cpp

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

I'll make the changes noted here and push this patch, too, along with the other one I have in my queue waiting for the tree to reopen.  Thanks!

::: js/src/ctypes/CTypes.cpp
@@ +7442,4 @@
>      return JS_FALSE;
>    }
>  
> +  return Int64Base::ToString(cx, obj, args.length(), vp, false);

Here's another place where you really should be passing along |args|, and not two separate arguments.

@@ +7457,4 @@
>      return JS_FALSE;
>    }
>  
> +  return Int64Base::ToSource(cx, obj, args.length(), vp, false);

And here too.

@@ +7612,4 @@
>      return JS_FALSE;
>    }
>  
> +  return Int64Base::ToString(cx, obj, args.length(), vp, true);

And here.

@@ +7627,4 @@
>      return JS_FALSE;
>    }
>  
> +  return Int64Base::ToSource(cx, obj, args.length(), vp, true);

And here.

Comment 94

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8e3d7ec35485
https://hg.mozilla.org/integration/mozilla-inbound/rev/22da2932d907

Comment 95

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8e3d7ec35485
https://hg.mozilla.org/mozilla-central/rev/22da2932d907

Comment 96

[Tom S [:evilpie]]

Would you mind to attach the patches again, so that they work with [Review]. I think the problem is the From: line at the very top.

Comment 97

[Tom S [:evilpie]]

Comment on attachment 732359 [details] [diff] [review]
First patch for various files

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

Looks very good overall. Thank you!

::: js/src/ctypes/Library.cpp
@@ +194,5 @@
>  JSBool
>  Library::Open(JSContext* cx, unsigned argc, jsval *vp)
>  {
> +  CallArgs args = CallArgsFromVp(argc, vp);
> +  JSObject* ctypesObj = &args.thisv().toObject();

This is now going to crash when .thisv() is not an object. Just revert this change, we will take care of JS_THIS_OBJECT globally.

@@ +219,5 @@
>  JSBool
>  Library::Close(JSContext* cx, unsigned argc, jsval* vp)
>  {
> +  CallArgs args = CallArgsFromVp(argc, vp);
> +  JSObject* obj = &args.thisv().toObject();

Same here. I am sorry.

Comment 98

[Tom S [:evilpie]]

Comment on attachment 732357 [details] [diff] [review]
Second patch for js.cpp

Please fix this one. I now think the missing file path is at fault. Are you manually altering patches?

Comment 99

[Tom S [:evilpie]]

Comment on attachment 728731 [details] [diff] [review]
Second patch for various files

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

Nice work!

::: js/src/jsapi-tests/tests.h
@@ +254,4 @@
>  
>          putchar('\n');
>          fflush(stdout);
> +        args.rval().set(JSVAL_VOID);

setUndefined()

::: js/src/jsapi.h
@@ -4869,5 @@
>  static JS_ALWAYS_INLINE JSBool
>  JS_IsConstructing(JSContext *cx, const jsval *vp)
>  {
> -#ifdef DEBUG
> -    JSObject *callee = JSVAL_TO_OBJECT(JS_CALLEE(cx, vp));

I don't think it's acceptable to just lose these assertions. I think I remember you saying that not every call to the new isConstructing() has a cx? I think the best solution is to just update these assertions to use args.callee() and leave them here.

::: js/src/jsbool.cpp
@@ +136,3 @@
>          JSObject *obj = BooleanObject::create(cx, b);
>          if (!obj)
> +            return JS_FALSE;

Please revert this change, internally we avoid JS_FALSE and JS_TRUE whenever we can.

::: js/src/jsfun.cpp
@@ +1111,3 @@
>          return false;
>  
> +    args.setCallee(invokedArgs.rval());

vp[0] has a slight double meaning. On function entry vp[0] contains the callee, but after leaving the function vp[0] is used to determine the return value. So this should be args.rval().set(invokedArgs.rval()).

::: js/src/jsfuninlines.h
@@ -119,5 @@
> -static JS_ALWAYS_INLINE bool
> -IsConstructing(const Value *vp)
> -{
> -#ifdef DEBUG
> -    JSObject *callee = &JS_CALLEE(cx, vp).toObject();

How did that even work?

::: js/src/jsnum.cpp
@@ +424,1 @@
>  

I think you can rewrite this like this:

double number = 0;
if (args.length() > 0) {
  if (!ToNumber(cx, args[0], &number))
    return false;
}

@@ +424,4 @@
>  
> +    if (args.length() > 0) {
> +        if (!ToNumber(cx, &args[0]))
> +            return JS_FALSE;

false

@@ +431,4 @@
>      }
>  
>      if (!isConstructing)
> +        return JS_TRUE;

true! Also fix all the other cases, please.

@@ +435,2 @@
>  
> +    JSObject *obj = NumberObject::create(cx, args.calleev().toNumber());

This could just be
JSObject *obj = NumberObject::create(cx, number);

@@ +437,3 @@
>      if (!obj)
> +        return JS_FALSE;
> +    args.setCallee(ObjectValue(*obj));

args.rval().setObject(*obj)

::: js/src/jsreflect.cpp
@@ +2964,4 @@
>  
>      RootedObject builder(cx);
>  
> +    RootedValue arg(cx, args.length() > 1 ? args[1] : UndefinedValue());

args.get(1)

::: js/src/perf/jsperf.cpp
@@ +167,5 @@
>  pm_construct(JSContext* cx, unsigned argc, jsval* vp)
>  {
>      uint32_t mask;
> +
> +    JS::CallArgs args = CallArgsFromVp(argc, vp);

Top please.

Comment 100

[Tom S [:evilpie]]

Hey Do! Are you still working on this?

Comment 101

[Biru [:poiru]]

Assigning myself as Do does not seem to be working on this any longer.

Comment 102

[Biru [:poiru]]

Attachment: Part 1: Use JS::CallArgs instead of JS_{ARGV,SET_RVAL,...} in js/src/shell/

In this and subsequent patches, I also changed JSVAL_IS_*/JSVAL_TO_* to the JS::Value equivalents in lines that were touched for other reasons. The rest will presumably be converted in bug 952650.

Comment 103

[Biru [:poiru]]

Attachment: Part 2: Use JS::CallArgs instead of JS_{ARGV,SET_RVAL,...} in js/src/

Comment 104

[Biru [:poiru]]

Attachment: Part 3: Use JS::CallArgs instead of `argc` in js/src/

Comment 105

[Biru [:poiru]]

Attachment: Part 4: Use JS::CallArgs instead of `argc` in js/src/builtin/

Comment 106

[Biru [:poiru]]

Attachment: Part 5: Use JS::CallArgs instead of `vp` in js/src/

Comment 107

[Biru [:poiru]]

Attachment: Part 6: Use JS::CallArgs instead of JS_{ARGV,SET_RVAL,...}, `argc`, `vp` in js/xpconnect/

Comment 108

[Biru [:poiru]]

Attachment: Part 7: Use JS::CallArgs instead of JS_ARGV, `argc` in ipc/{ril,nfc}

Comment 109

[Biru [:poiru]]

Attachment: Part 8: Use JS::CallArgs instead of JS_{ARGV,SET_RVAL,...} in remaining instances

Comment 110

[Biru [:poiru]]

Attachment: Part 9: Remove JS_{CALLEE,ARGV,RVAL,SET_RVAL}

Try push: https://tbpl.mozilla.org/?tree=Try&rev=33d37d7a01dc

Note: This depends on bug 984112.

Comment 111

[Tom S [:evilpie]]

Comment on attachment 8392284 [details] [diff] [review]
Part 3: Use JS::CallArgs instead of `argc` in js/src/

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

::: js/src/json.cpp
@@ +820,4 @@
>  
>      /* Steps 2-5. */
>      return ParseJSONWithReviver(cx, ConstTwoByteChars(flat->chars(), flat->length()),
>                                  flat->length(), reviver, args.rval());

Couldn't you just use args.get(1) here now?

@@ +831,1 @@
>      RootedObject replacer(cx, (argc >= 2 && vp[3].isObject())

I think you missed this one.

Comment 112

[Tom S [:evilpie]]

Comment on attachment 8392285 [details] [diff] [review]
Part 4: Use JS::CallArgs instead of `argc` in js/src/builtin/

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

::: js/src/builtin/TestingFunctions.cpp
@@ +527,5 @@
>  static bool
>  VerifyPostBarriers(JSContext *cx, unsigned argc, jsval *vp)
>  {
>      CallArgs args = CallArgsFromVp(argc, vp);
> +    if (args.length() != 0) {

let's stay with args.length()

@@ +542,5 @@
>  GCState(JSContext *cx, unsigned argc, jsval *vp)
>  {
>      CallArgs args = CallArgsFromVp(argc, vp);
>  
> +    if (args.length() != 0) {

same

Comment 113

[Tom S [:evilpie]]

Comment on attachment 8392286 [details] [diff] [review]
Part 5: Use JS::CallArgs instead of `vp` in js/src/

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

::: js/src/builtin/TestingFunctions.cpp
@@ +829,5 @@
>          JS_ReportOutOfMemory(cx);
>          return false;
>      }
>  
> +    args.rval().set(JS_NumberValue(double(counter)));

I am not sure what counter is, but you probably want something like .setNumber()

@@ +1017,5 @@
>      }
>  
>      JSFunction *fun = &args[0].toObject().as<JSFunction>();
>      JSString *str = fun->displayAtom();
> +    args.rval().setString(str == nullptr ? cx->runtime()->emptyString : str);

Can you make this str ? str : cx->runtime()->emptyStirng

Comment 114

[Tom S [:evilpie]]

Comment on attachment 8392290 [details] [diff] [review]
Part 9: Remove JS_{CALLEE,ARGV,RVAL,SET_RVAL}

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

Great work \o/

Comment 115

[Tom S [:evilpie]]

Comment on attachment 8392283 [details] [diff] [review]
Part 2: Use JS::CallArgs instead of JS_{ARGV,SET_RVAL,...} in js/src/

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

::: js/src/ctypes/Library.cpp
@@ +207,5 @@
>      JS_ReportError(cx, "not a ctypes object");
>      return false;
>    }
>  
> +  if (args.length() != 1 || args[0].isUndefined()) {

!args.hasDefined(0)

Comment 116

[Tom S [:evilpie]]

Comment on attachment 8392282 [details] [diff] [review]
Part 1: Use JS::CallArgs instead of JS_{ARGV,SET_RVAL,...} in js/src/shell/

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

::: js/src/shell/js.cpp
@@ +3383,5 @@
>          double d = 0.0;
>          JSShellContextData *data = GetContextData(cx);
>          if (data)
>              d = PRMJ_Now() - data->startTime;
> +        args.rval().set(JS_NumberValue(d));

setDouble

@@ +4110,1 @@
>      if (JSVAL_IS_PRIMITIVE(v)) {

v.isPrimitive()

Comment 117

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Tom Schuster [:evilpie] from comment #114)
> Comment on attachment 8392290 [details] [diff] [review]
> Part 9: Remove JS_{CALLEE,ARGV,RVAL,SET_RVAL}
> 
> Review of attachment 8392290 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Great work \o/

We should probably announce this to some newsgroup.

Comment 118

[Jeff Walden [:Waldo]]

Also https://developer.mozilla.org/en-US/docs/SpiderMonkey/31 will need updating for this.

Comment 119

[Jeff Walden [:Waldo]]

Comment on attachment 8392289 [details] [diff] [review]
Part 8: Use JS::CallArgs instead of JS_{ARGV,SET_RVAL,...} in remaining instances

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

Smallish nitpicks, and some slight refactoring I'd prefer you do to fix one of the problems here more thoroughly.  Otherwise in fair shape, second patch for review should be good.

::: ipc/testshell/XPCShellEnvironment.cpp
@@ +196,5 @@
>  static bool
>  BuildDate(JSContext *cx, unsigned argc, JS::Value *vp)
>  {
>      fprintf(stdout, "built on %s at %s\n", __DATE__, __TIME__);
>      return true;

THis method needs |JS::CallArgs args = JS::CallArgsFromVp(argc, vp);| at the start and a |args.rval().setUndefined();| just before |return true| at the end.  (Right now this method returns itself by happenstance, which eventually we're going to make trigger an assertion.)

::: netwerk/base/src/ProxyAutoConfig.cpp
@@ +781,5 @@
>  {
>    NetAddr remoteAddress;
>    nsAutoCString localDottedDecimal;
>    JSContext *cx = mJSRuntime->Context();
> +  JS::CallReceiver call = CallReceiverFromVp(vp);

This kind of works, but not well.  The issue here is this code is old and works in terms of |vp| at all.  Instead, it should be passed a |const JS::CallArgs&| by the caller.

So, then, PACMyIpAddress should have a |JS::CallArgs args = JS::CallArgsFromVp(argc, vp);| and should then pass |args| to ProxyAutoConfig::MyIPAddress.  That, then, should pass |args| into this code.

@@ +822,5 @@
>    if (dns && NS_SUCCEEDED(dns->GetMyHostName(hostName)) &&
>        PACResolveToString(hostName, localDottedDecimal, kTimeout)) {
>      JSString *dottedDecimalString =
>        JS_NewStringCopyZ(cx, localDottedDecimal.get());
> +    call.rval().setString(dottedDecimalString);

Hmm.  So we fail to handle a JS OOM correctly here, then.  Could you file a followup bug to fix this?  Just need to make this method return true/false to indicate success/failure, and indicate got-an-IP-address status through an outparam.  Should be easy to fix.

@@ +841,5 @@
>    // who knows? let's fallback to localhost
>    localDottedDecimal.AssignLiteral("127.0.0.1");
>    JSString *dottedDecimalString =
>      JS_NewStringCopyZ(cx, localDottedDecimal.get());
> +  call.rval().setString(dottedDecimalString);

Same failure to handle OOM here.  This one's easy to fix, tho, just needs |if (!dottedDecimalString) return false;|.

::: toolkit/components/telemetry/Telemetry.cpp
@@ +899,5 @@
>    case REFLECT_CORRUPT:
>      JS_ReportError(cx, "Histogram is corrupt");
>      return false;
>    case REFLECT_OK:
> +    args.rval().setObjectOrNull(snapshot);

If you look further up, |snapshot| was only null if an operation failed.  So it's non-null here, and you should instead use |args.rval().setObject(*snapshot);|.

Comment 120

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8392287 [details] [diff] [review]
Part 6: Use JS::CallArgs instead of JS_{ARGV,SET_RVAL,...}, `argc`, `vp` in js/xpconnect/

r=me

Comment 121

[Biru [:poiru]]

Attachment: Part 1: Use JS::CallArgs instead of JS_{ARGV,SET_RVAL,...} in js/src/shell/

Comment 122

[Biru [:poiru]]

Attachment: Part 2: Use JS::CallArgs instead of JS_{ARGV,SET_RVAL,...} in js/src/

Comment 123

[Biru [:poiru]]

Attachment: Part 3: Use JS::CallArgs instead of `argc` in js/src/

Comment 124

[Biru [:poiru]]

Attachment: Part 4: Use JS::CallArgs instead of `argc` in js/src/builtin/

Comment 125

[Biru [:poiru]]

Attachment: Part 5: Use JS::CallArgs instead of `vp` in js/src/

Comment 126

[Biru [:poiru]]

Attachment: Part 6: Use JS::CallArgs instead of JS_{ARGV,SET_RVAL,...}, `argc`, `vp` in js/xpconnect/

Comment 127

[Biru [:poiru]]

Attachment: Part 7: Use JS::CallArgs instead of JS_ARGV, `argc` in ipc/{ril,nfc}/

Comment 128

[Biru [:poiru]]

Attachment: Part 8: Use JS::CallArgs instead of JS_{ARGV,SET_RVAL,...} in remaining instances

Comment 119 addressed. Bug 986076 filed for the JS OOM issue.

Comment 129

[Biru [:poiru]]

Attachment: Part 9: Remove JS_{CALLEE,ARGV,RVAL,SET_RVAL}

Comment 130

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8395006 [details] [diff] [review]
Part 1: Use JS::CallArgs instead of JS_{ARGV,SET_RVAL,...} in js/src/shell/

https://hg.mozilla.org/integration/mozilla-inbound/rev/92d2569932a4

Comment 131

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8395007 [details] [diff] [review]
Part 2: Use JS::CallArgs instead of JS_{ARGV,SET_RVAL,...} in js/src/

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

Comment 132

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8395008 [details] [diff] [review]
Part 3: Use JS::CallArgs instead of `argc` in js/src/

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

Comment 133

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8395009 [details] [diff] [review]
Part 4: Use JS::CallArgs instead of `argc` in js/src/builtin/

https://hg.mozilla.org/integration/mozilla-inbound/rev/595c8c60d676

Comment 134

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8395011 [details] [diff] [review]
Part 5: Use JS::CallArgs instead of `vp` in js/src/

https://hg.mozilla.org/integration/mozilla-inbound/rev/0ebc8c3099ef

Comment 135

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8395012 [details] [diff] [review]
Part 6: Use JS::CallArgs instead of JS_{ARGV,SET_RVAL,...}, `argc`, `vp` in js/xpconnect/

https://hg.mozilla.org/integration/mozilla-inbound/rev/58e088726ab7

Comment 136

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8395013 [details] [diff] [review]
Part 7: Use JS::CallArgs instead of JS_ARGV, `argc` in ipc/{ril,nfc}/

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

Comment 137

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/92d2569932a4
https://hg.mozilla.org/mozilla-central/rev/e2e8f128c7a8
https://hg.mozilla.org/mozilla-central/rev/e425b681d19f
https://hg.mozilla.org/mozilla-central/rev/595c8c60d676
https://hg.mozilla.org/mozilla-central/rev/0ebc8c3099ef
https://hg.mozilla.org/mozilla-central/rev/58e088726ab7
https://hg.mozilla.org/mozilla-central/rev/a43111585107

Comment 138

[Jeff Walden [:Waldo]]

Comment on attachment 8395015 [details] [diff] [review]
Part 8: Use JS::CallArgs instead of JS_{ARGV,SET_RVAL,...} in remaining instances

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

\o/

Comment 139

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8395015 [details] [diff] [review]
Part 8: Use JS::CallArgs instead of JS_{ARGV,SET_RVAL,...} in remaining instances

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

Comment 140

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8395016 [details] [diff] [review]
Part 9: Remove JS_{CALLEE,ARGV,RVAL,SET_RVAL}

https://hg.mozilla.org/integration/mozilla-inbound/rev/726e81b3c1a9

Comment 141

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

https://hg.mozilla.org/mozilla-central/rev/e53e37d2c2bf
https://hg.mozilla.org/mozilla-central/rev/726e81b3c1a9