959787 - Handlify all unhandlified GCing interfaces in JSAPI

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Terrence Cole [:terrence]]

Attachment: handlify_1-v0.diff

I expect this will be a long series of patches. I'm tracking the ones I have outstanding at https://etherpad.mozilla.org/C4VCNdsk2M ; feel free to hop in with other interfaces.

Comment 1

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

Comment on attachment 8359989 [details] [diff] [review]
handlify_1-v0.diff

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

::: dom/plugins/base/nsJSNPRuntime.cpp
@@ +2002,5 @@
>    case JSTYPE_VOID:
>    case JSTYPE_STRING:
>    case JSTYPE_NUMBER:
>      vp.set(memberPrivate->fieldValue);
> +    if (!vp.isPrimitive()) {

Actually, this one would be clearer as vp.isObject(). JSVAL_IS_PRIMITIVE was only used because JSVAL_IS_OBJECT included null.

Comment 2

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

Comment on attachment 8359989 [details] [diff] [review]
handlify_1-v0.diff

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

Prettier!

Comment 3

[Terrence Cole [:terrence]]

Attachment: handlify_2-v0.diff

More of the same.

Comment 4

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

Comment on attachment 8360060 [details] [diff] [review]
handlify_2-v0.diff

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

Too bad diffstat doesn't give you a count for number of CAPITAL LETTERS added and removed.

Comment 5

[Terrence Cole [:terrence]]

Attachment: handlify_3-v0.diff

If it's any consolation, this was probably more tedious to implement than it will be to review. In any case, sorry.

Comment 6

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

Comment on attachment 8360156 [details] [diff] [review]
handlify_3-v0.diff

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

I really wish we could overload nullptr, but apparently not all of our compilers allow it yet. I don't suppose we'd want to add a JS_NewObject overload with fewer params? (Or default the last 2)

Is this going to break the b2g build?

::: dom/plugins/base/nsJSNPRuntime.cpp
@@ +2036,5 @@
>  
>  static bool
>  NPObjectMember_Call(JSContext *cx, unsigned argc, JS::Value *vp)
>  {
> +  JS::Rooted<JSObject*> memobj(cx, JSVAL_TO_OBJECT(JS_CALLEE(cx, vp)));

No CallArgs?

Comment 7

[Jon Coppeard (:jonco) (PTO until 3rd April)]

Attachment: unhandlifiedGcFunctions.txt

Here's a list of functions that need handlification.  I grepped gcFunctions.txt for identifiers starting with JS:: or JS_ whose arguments included GC pointer types.

Comment 8

[Jon Coppeard (:jonco) (PTO until 3rd April)]

Attachment: handlify-delete-apis

Here's a patch to handlify the JS_Delete*() APIs.  I guess this will need review by a browser person too.

Comment 9

[Terrence Cole [:terrence]]

(In reply to Steve Fink [:sfink] from comment #6)
> Comment on attachment 8360156 [details] [diff] [review]
> handlify_3-v0.diff
> 
> Review of attachment 8360156 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I really wish we could overload nullptr, but apparently not all of our
> compilers allow it yet. I don't suppose we'd want to add a JS_NewObject
> overload with fewer params? (Or default the last 2)
> 
> Is this going to break the b2g build?

Oh yeah, totally busted: ../../../gecko/uriloader/exthandler/nsExternalHelperAppService.cpp:629: error: conversion from 'int' to non-scalar type 'JS::Handle<JSObject*>' requested

Looks like this conversion effort might actually be effective at helping port to B2G.

> ::: dom/plugins/base/nsJSNPRuntime.cpp
> @@ +2036,5 @@
> >  
> >  static bool
> >  NPObjectMember_Call(JSContext *cx, unsigned argc, JS::Value *vp)
> >  {
> > +  JS::Rooted<JSObject*> memobj(cx, JSVAL_TO_OBJECT(JS_CALLEE(cx, vp)));
> 
> No CallArgs?

NPObjectMember_Call feeds vp straight into the xpt woodchipper. I didn't want to risk breaking it in a patch that is already almost guaranteed to break a bunch of other builds I don't normally do.

Comment 10

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

Comment on attachment 8360487 [details] [diff] [review]
handlify-delete-apis

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

Yeah, it's enough of a change to the ordering that I think I'd like a peer review to cover my a@#.

::: dom/plugins/base/nsJSNPRuntime.cpp
@@ +843,2 @@
>    if (ok && deleted) {
> +    // FIXME: See bug 425823 , we shouldn't need to do this, and once

Why the added space before the comma?

Comment 11

[Jon Coppeard (:jonco) (PTO until 3rd April)]

(In reply to Steve Fink [:sfink] from comment #10)

> Yeah, it's enough of a change to the ordering that I think I'd like a peer
> review to cover my a@#.

Great, thanks for r?ing.

> Why the added space before the comma?

Gah, didn't meant to include that.

Comment 12

[Terrence Cole [:terrence]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ff4971e84f9f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8b260c3236da
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/bf6cb0c04562

Green try run at:
https://tbpl.mozilla.org/?tree=Try&rev=325196af7916

Try bustage is pre-existing on the base here:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=7eac28351524

Comment 13

[Terrence Cole [:terrence]]

Attachment: handlify_4-v0.diff

And time for round 2.

Comment 14

[Terrence Cole [:terrence]]

Attachment: handlify_4.1-v0.diff

The prior patch required one codegen change: moving the Rooting for JS_FreezeObject up one level. This is not a new root, just moving it up a level. This is the only place that such a change was required; all other callers were double-rooting previously.

Comment 15

[Tom S [:evilpie]]

Comment on attachment 8361360 [details] [diff] [review]
handlify_4-v0.diff

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

::: js/src/jsapi.h
@@ +2311,5 @@
>  
>  } /* namespace JS */
>  
>  extern JS_PUBLIC_API(bool)
> +JS_ValueToId(JSContext *cx, jsval v, JS::MutableHandle<jsid> idp);

Why didn't you handlify v?

Comment 16

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

https://hg.mozilla.org/mozilla-central/rev/ff4971e84f9f
https://hg.mozilla.org/mozilla-central/rev/8b260c3236da
https://hg.mozilla.org/mozilla-central/rev/bf6cb0c04562

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8361361 [details] [diff] [review]
handlify_4.1-v0.diff

r=me

Comment 18

[Terrence Cole [:terrence]]

(In reply to Tom Schuster [:evilpie] from comment #15)
> Comment on attachment 8361360 [details] [diff] [review]
> handlify_4-v0.diff
> 
> Review of attachment 8361360 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jsapi.h
> @@ +2311,5 @@
> >  
> >  } /* namespace JS */
> >  
> >  extern JS_PUBLIC_API(bool)
> > +JS_ValueToId(JSContext *cx, jsval v, JS::MutableHandle<jsid> idp);
> 
> Why didn't you handlify v?

Good question! Currently, the majority of the callers are using StringValue(str). I think most probably really want something like JS_StringToId or somesuch. We don't really have anything exactly right, I think, so we probably want some new API. That should be in a different patch though.

Comment 19

[Terrence Cole [:terrence]]

Attachment: handlify_5-v0.diff

I guess I should spread some of these mechanical reviews around.

Comment 20

[Terrence Cole [:terrence]]

Attachment: handlify_6-v0.diff

The low hanging fruit from this section. JS_EvaluateScript is going to be fairly challenging because we don't have a nullable MutableHandle.

Comment 21

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

Comment on attachment 8361360 [details] [diff] [review]
handlify_4-v0.diff

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

I remember putting a lot of those in. It's good to see them finally come out.

r=me, though I suppose this patch will change a bunch if you decide to handlify the other parameter.

::: dom/base/nsDOMClassInfo.cpp
@@ +4358,5 @@
>        JS_NewUCStringCopyN(cx, key.get(), key.Length());
>      NS_ENSURE_TRUE(str, NS_ERROR_OUT_OF_MEMORY);
>  
> +    JS::Rooted<jsid> id(cx);
> +    JS_ValueToId(cx, STRING_TO_JSVAL(str), &id);

Aw, can't you spare a StringValue?

::: dom/bindings/Codegen.py
@@ +5223,5 @@
>              if self.idlNode.getExtendedAttribute("Frozen"):
>                  assert self.idlNode.type.isSequence()
>                  freezeValue = CGGeneric(
> +                    "JS::Rooted<JSObject*> rvalObj(cx, &args.rval().toObject());"
> +                    "if (!JS_FreezeObject(cx, rvalObj)) {\n"

This can't be repeated and get a duplicate definition, right?

::: js/src/jsapi.h
@@ +2311,5 @@
>  
>  } /* namespace JS */
>  
>  extern JS_PUBLIC_API(bool)
> +JS_ValueToId(JSContext *cx, jsval v, JS::MutableHandle<jsid> idp);

Yeah, v gets immediately rooted inside of JS_ValueToId.

@@ +2316,3 @@
>  
>  extern JS_PUBLIC_API(bool)
> +JS_IdToValue(JSContext *cx, jsid id, JS::MutableHandle<JS::Value> vp);

I guess id is intentionally left unrooted since it is dead before the first gc?

@@ +3168,5 @@
>   * property to visit using iterobj, or JSID_IS_VOID if there is no such property
>   * left to visit.  Return false on error.
>   */
>  extern JS_PUBLIC_API(bool)
> +JS_NextProperty(JSContext *cx, JS::Handle<JSObject*> iterobj, jsid *idp);

Are you doing these in some order? Why not MutableHandle for idp?

::: js/xpconnect/src/XPCComponents.cpp
@@ +268,3 @@
>                  if (NS_SUCCEEDED(interface->GetNameShared(&name)) && name &&
>                          nullptr != (idstr = JS_NewStringCopyZ(cx, name)) &&
> +                        JS_ValueToId(cx, STRING_TO_JSVAL(idstr), &id)) {

StringValue. Or are you avoiding touching these so we can do them en masse?

@@ +516,5 @@
>                  if (NS_SUCCEEDED(interface->GetIIDShared(&iid))) {
>                      iid->ToProvidedString(idstr);
>                      jsstr = JS_NewStringCopyZ(cx, idstr);
> +                    JS::Rooted<jsid> id(cx);
> +                    if (jsstr && JS_ValueToId(cx, STRING_TO_JSVAL(jsstr), &id)) {

StringValue

@@ +776,2 @@
>                          if (idstr &&
> +                            JS_ValueToId(cx, STRING_TO_JSVAL(idstr), &id)) {

StringValue

@@ +1018,2 @@
>                          if (idstr &&
> +                            JS_ValueToId(cx, STRING_TO_JSVAL(idstr), &id)) {

StringValue

@@ +1268,5 @@
>              iter = (const void**) JSVAL_TO_PRIVATE(*statep);
>              if (nsXPCException::IterateNSResults(nullptr, &name, nullptr, iter)) {
>                  JSString* idstr = JS_NewStringCopyZ(cx, name);
> +                JS::Rooted<jsid> id(cx);
> +                if (idstr && JS_ValueToId(cx, STRING_TO_JSVAL(idstr), &id)) {

StringValue

@@ +1269,5 @@
>              if (nsXPCException::IterateNSResults(nullptr, &name, nullptr, iter)) {
>                  JSString* idstr = JS_NewStringCopyZ(cx, name);
> +                JS::Rooted<jsid> id(cx);
> +                if (idstr && JS_ValueToId(cx, STRING_TO_JSVAL(idstr), &id)) {
> +                    *idp = id;

Ugh. Maybe idp will become a Handle too someday?

::: storage/src/mozStorageStatementParams.cpp
@@ +128,5 @@
>        NS_ENSURE_TRUE(jsname, NS_ERROR_OUT_OF_MEMORY);
>  
>        // Set our name.
> +      JS::Rooted<jsid> id(aCtx);
> +      if (!::JS_ValueToId(aCtx, STRING_TO_JSVAL(jsname), &id)) {

StringValue

Comment 22

[Jon Coppeard (:jonco) (PTO until 3rd April)]

Comment on attachment 8361360 [details] [diff] [review]
handlify_4-v0.diff

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

::: js/src/jsapi.h
@@ +2311,5 @@
>  
>  } /* namespace JS */
>  
>  extern JS_PUBLIC_API(bool)
> +JS_ValueToId(JSContext *cx, jsval v, JS::MutableHandle<jsid> idp);

Can we use the handle typedefs in the API please?  We agreed we'd do that, and I just changed all the existing ones over in bug 959683.

@@ +2747,5 @@
>   * through non-extensible objects, on the assumption that those are already
>   * deep-frozen.
>   */
>  extern JS_PUBLIC_API(bool)
> +JS_DeepFreezeObject(JSContext *cx, JS::Handle<JSObject*> obj);

Can we use the handle typedefs in the API please?  We agreed we'd do that, and I just changed all the existing ones over in bug 959683.

Comment 23

[Jon Coppeard (:jonco) (PTO until 3rd April)]

Comment on attachment 8361429 [details] [diff] [review]
handlify_5-v0.diff

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

Looks good!

::: js/src/jsapi.h
@@ +3259,5 @@
>   * Functions and scripts.
>   */
>  extern JS_PUBLIC_API(JSFunction *)
>  JS_NewFunction(JSContext *cx, JSNative call, unsigned nargs, unsigned flags,
> +               JS::Handle<JSObject*> parent, const char *name);

It looks like you're deliberately using Handle<Object> style here - I think we agreed we were going to use HandleObject style in jsapi.h in the end, or did I misremember that?

Comment 24

[Jon Coppeard (:jonco) (PTO until 3rd April)]

Comment on attachment 8361449 [details] [diff] [review]
handlify_6-v0.diff

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

It's great to get rid of all this re-rooting.

Comment 25

[Boris Zbarsky [:bzbarsky]]

> This can't be repeated and get a duplicate definition, right?

Correct.  That's handling the return value of a getter or method, and there better only be one.  ;)

Comment 26

[Jon Coppeard (:jonco) (PTO until 3rd April)]

Attachment: handlify-has-apis

Here's a patch to do various "has property" related APIs.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8361693 [details] [diff] [review]
handlify-has-apis

r=me on the two dom files; I assume that's what you wanted from me?  ;)

Comment 28

[Jon Coppeard (:jonco) (PTO until 3rd April)]

(In reply to Boris Zbarsky [:bz] from comment #27)
Thanks, yes that is what I wanted :)

Comment 29

[Terrence Cole [:terrence]]

Comment on attachment 8361693 [details] [diff] [review]
handlify-has-apis

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

r=me

Comment 30

[Terrence Cole [:terrence]]

(In reply to Steve Fink [:sfink] from comment #21)
> ::: dom/base/nsDOMClassInfo.cpp
> @@ +4358,5 @@
> >        JS_NewUCStringCopyN(cx, key.get(), key.Length());
> >      NS_ENSURE_TRUE(str, NS_ERROR_OUT_OF_MEMORY);
> >  
> > +    JS::Rooted<jsid> id(cx);
> > +    JS_ValueToId(cx, STRING_TO_JSVAL(str), &id);
> 
> Aw, can't you spare a StringValue?

Oh, all right, if you insist. :-)
 
> ::: dom/bindings/Codegen.py
> @@ +5223,5 @@
> >              if self.idlNode.getExtendedAttribute("Frozen"):
> >                  assert self.idlNode.type.isSequence()
> >                  freezeValue = CGGeneric(
> > +                    "JS::Rooted<JSObject*> rvalObj(cx, &args.rval().toObject());"
> > +                    "if (!JS_FreezeObject(cx, rvalObj)) {\n"
> 
> This can't be repeated and get a duplicate definition, right?

I guess I cut this patch before splitting that chunk out. In any case I r?'d bz on that hunk too.
 
> ::: js/src/jsapi.h
> @@ +2311,5 @@
> >  
> >  } /* namespace JS */
> >  
> >  extern JS_PUBLIC_API(bool)
> > +JS_ValueToId(JSContext *cx, jsval v, JS::MutableHandle<jsid> idp);
> 
> Yeah, v gets immediately rooted inside of JS_ValueToId.

See comment 18.
 
> @@ +2316,3 @@
> >  
> >  extern JS_PUBLIC_API(bool)
> > +JS_IdToValue(JSContext *cx, jsid id, JS::MutableHandle<JS::Value> vp);
> 
> I guess id is intentionally left unrooted since it is dead before the first
> gc?

I guess I just missed this one! I'll get it in a follow up.

> @@ +3168,5 @@
> >   * property to visit using iterobj, or JSID_IS_VOID if there is no such property
> >   * left to visit.  Return false on error.
> >   */
> >  extern JS_PUBLIC_API(bool)
> > +JS_NextProperty(JSContext *cx, JS::Handle<JSObject*> iterobj, jsid *idp);
> 
> Are you doing these in some order? Why not MutableHandle for idp?

Uhhhh. I guess I'll get that one in a follow-up too.
 
> @@ +1269,5 @@
> >              if (nsXPCException::IterateNSResults(nullptr, &name, nullptr, iter)) {
> >                  JSString* idstr = JS_NewStringCopyZ(cx, name);
> > +                JS::Rooted<jsid> id(cx);
> > +                if (idstr && JS_ValueToId(cx, STRING_TO_JSVAL(idstr), &id)) {
> > +                    *idp = id;
> 
> Ugh. Maybe idp will become a Handle too someday?

Yeah, I didn't want to go too far down the rabbit hole. I guess I know what handlify_7 is going to look like.

Comment 31

[Terrence Cole [:terrence]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/21cef8b355ce
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e40cf0c641b5

Green try run with these two patches is here:
https://tbpl.mozilla.org/?tree=Try&rev=d95d1a920334

Comment 32

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/21cef8b355ce
https://hg.mozilla.org/mozilla-central/rev/e40cf0c641b5

Comment 33

[Jon Coppeard (:jonco) (PTO until 3rd April)]

Attachment: handlify-has-apis-2

Sorry, I forgot to qref before asking for review - here are the rest of the changes.

Comment 34

[Jon Coppeard (:jonco) (PTO until 3rd April)]

Attachment: handlify-define-apis

Here's a patch to handify JS_DefineBlah apis.  The browser patch is turning out to be quite big - I'll post it later when I've finished.

Comment 35

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

Comment on attachment 8362988 [details] [diff] [review]
handlify-define-apis

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

::: js/src/builtin/TestingFunctions.cpp
@@ +1015,5 @@
>      for (NonBuiltinScriptFrameIter iter(cx); !iter.done(); ++iter) {
>          if (iter.isFunctionFrame() && iter.compartment() == cx->compartment()) {
> +            indexId = INT_TO_JSID(stackIndex);
> +            calleeValue = ObjectValue(*iter.callee());
> +            if (!JS_DefinePropertyById(cx, stack, indexId, calleeValue,

This makes me wonder if iter.calleev() should return a HandleValue. But I haven't looked at all the different iter implementations, so maybe it's not valid. Oh well, this seems good enough.

::: js/src/jsmath.cpp
@@ +1485,5 @@
>      RootedObject Math(cx, NewObjectWithGivenProto(cx, &MathClass, proto, obj, SingletonObject));
>      if (!Math)
>          return nullptr;
>  
> +    RootedValue mathValue(cx, OBJECT_TO_JSVAL(Math));

ObjectValue

::: js/src/shell/js.cpp
@@ +4740,2 @@
>          valstr = JS_NewStringCopyZ(cx, value);
> +        *value++ = '=';

Uh, shouldn't this be changed to

  valstr = JS_NewStringCopyZ(cx, value+1);

now? And you're shadowing 'value'. And the char* value is no longer pointing at the value; perhaps it could be renamed 'sep' or 'separator'?

@@ +5389,3 @@
>              return false;
> +
> +        RootedValue value(cx, StringValue(str));

Shadowing 'value' again. Just assign to the outer one.

Comment 36

[John Schoenick [:johns]]

(In reply to Steve Fink [:sfink] from comment #10)
> Comment on attachment 8360487 [details] [diff] [review]
> handlify-delete-apis
> 
> Review of attachment 8360487 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Yeah, it's enough of a change to the ordering that I think I'd like a peer
> review to cover my a@#.

Can you clarify what you mean here? Does this add a new potential to GC in the NPRuntime bits, or were you referring to the non-npruntime portions of the patch? (Which I'm not a peer on)

Comment 37

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

(In reply to John Schoenick [:johns] from comment #36)
> (In reply to Steve Fink [:sfink] from comment #10)
> > Comment on attachment 8360487 [details] [diff] [review]
> > handlify-delete-apis
> > 
> > Review of attachment 8360487 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Yeah, it's enough of a change to the ordering that I think I'd like a peer
> > review to cover my a@#.
> 
> Can you clarify what you mean here? Does this add a new potential to GC in
> the NPRuntime bits, or were you referring to the non-npruntime portions of
> the patch? (Which I'm not a peer on)

Now that I look again, this isn't changing any ordering. It *does* remove one call to NPIdentifierToJSId(). But I just looked at what that does, and it's harmless. So I'm going to just say that this is a fine mechanical change.

johns: no, there are no new places this can GC. The change is mainly to appease our automatic analysis, which saw a worrisome pattern here even though there's nothing that could cause a problem in practice. A slight reordering makes the analysis happy (and enables switching over to a safer-by-default API.)

Comment 38

[Terrence Cole [:terrence]]

Comment on attachment 8362988 [details] [diff] [review]
handlify-define-apis

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

I'm honestly not a huge fan of this patch as is. I think we should make a variant of DefineObjectProperty as for ctypes; otherwise, we're forcing a huge amount of boilerplate onto users that would be easier for us to carry. Could you hold off on holding this patch for a day and I'll see if I can flesh out what I mean?

Comment 39

[Terrence Cole [:terrence]]

Attachment: handlify_DefineProperty-v1.diff

When handlifying JS_DefineFunction in the obvious way, a huge number of the callers need a new boilerplate rooted because most of the callers pass JS::FooValue(something) to a handle. This is annoying on multiple levels as it forces extra rooting for Int32Value and DoubleValue and the extra boilerplate makes our interface demonstrably worse.

Instead, what I'd like to do is add variants of JS_DefineFunction that take the various underlying non-value types directly (as well as one for HandleValue) and root or don't root as appropriate internally. Additionally, since we have to touch the API anyway, I've given getter, setter, and attrs default values and reordered the parameters so that attrs is first. With this change the vast majority of the callsites are 1-2 lines shorter.

At this point I'm mostly just soliciting feedback to ensure that everyone agrees that this transformation is actually a good idea and that the new API is appropriate.

Comment 40

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8362443 [details] [diff] [review]
handlify-has-apis-2

r=me

Comment 41

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8363385 [details] [diff] [review]
handlify_DefineProperty-v1.diff

The benefit of not having default values is to make people think about what they're passing.  I'm pretty happy to have getter/setter default to null, esp since we're almost out of the class getter/setter weeds so we don't need to worry about the difference between null and stub ops.  But defaulting attrs to 0 means people defining non-enumerable props by accident, I suspect...  I'd almost rather it default to JSPROP_ENUMERATE.

Apart from that the changes seem fine.

Comment 42

[Jon Coppeard (:jonco) (PTO until 3rd April)]

Comment on attachment 8363385 [details] [diff] [review]
handlify_DefineProperty-v1.diff

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

Great, that is much better than what I had!

Comment 43

[Jon Coppeard (:jonco) (PTO until 3rd April)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d72341fc5b85
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f5e29e2dc77

Comment 44

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d72341fc5b85
https://hg.mozilla.org/mozilla-central/rev/6f5e29e2dc77

Comment 45

[Jon Coppeard (:jonco) (PTO until 3rd April)]

Attachment: handlify-set-apis

Here's a patch to handlify  JS_Set*() APIs.

Comment 46

[Jon Coppeard (:jonco) (PTO until 3rd April)]

Attachment: handlify-lookup

And here's one to handlify JS_Lookup*() APIs.

Comment 47

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8364387 [details] [diff] [review]
handlify-set-apis

>+JS::Rooted<JSObject*> locationObj(cx, &v.toObject());

This should be called targetObj or something.  It's only a Location in the special case of Document.location, but this code is used in various other places too.

>+            'JS::Rooted<JSObject*> callbackObj(cx, mCallback);\n'
>+            'if (!JS_SetProperty(cx, callbackObj, "${attrName}", ${argv})) {\n'

Please replace mCallback with CallbackPreserveColor() instead of adding the unnecessary root.

> CallMethodHelper::GatherAndConvertResults()

Why put the RootedObject so high up?  I'd prefer we put it down in the narrow scope, I think.

r=me with those fixed.

Comment 48

[Boris Zbarsky [:bzbarsky]]

Also, could we please fix the set APIs to not take a mutable handle?  Because that's just messed up.  ;)

Comment 49

[Jason Orendorff [:jorendorff]]

Comment on attachment 8363385 [details] [diff] [review]
handlify_DefineProperty-v1.diff

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

Nice change!

I hate Class::{get,set}Property.

Comment 50

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

Comment on attachment 8364389 [details] [diff] [review]
handlify-lookup

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

The const jsid& return type threw me for a minute, but it seems to make sense.

Comment 51

[Terrence Cole [:terrence]]

Attachment: handlify_7-v0.diff

A couple of trivial handlifications that I forgot to upload earlier.

Comment 52

[Jon Coppeard (:jonco) (PTO until 3rd April)]

(In reply to Steve Fink [:sfink] from comment #50)
> The const jsid& return type threw me for a minute, but it seems to make
> sense.

It turns out that actually causes test failures, so I took it out and re-rooted in the one case it was necessary.  I'm still not sure what's wrong with it.

Comment 53

[Jon Coppeard (:jonco) (PTO until 3rd April)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/059c71ad9637

Comment 54

[Terrence Cole [:terrence]]

Comment on attachment 8364387 [details] [diff] [review]
handlify-set-apis

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

r=me

Comment 55

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/059c71ad9637

Comment 56

[Terrence Cole [:terrence]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/64ad4b2af6ac
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/37420d9a2c76

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

Comment 57

[Jon Coppeard (:jonco) (PTO until 3rd April)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/84a0bf9566e6

Comment 58

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/64ad4b2af6ac
https://hg.mozilla.org/mozilla-central/rev/37420d9a2c76
https://hg.mozilla.org/mozilla-central/rev/84a0bf9566e6

Comment 59

[Christian Holler (:decoder)]

Comment on attachment 8364387 [details] [diff] [review]
handlify-set-apis


> JS_PUBLIC_API(bool)
>-JS_SetProperty(JSContext *cx, JSObject *objArg, const char *name, HandleValue v)
>-{
>-    RootedObject obj(cx, objArg);
>+JS_SetProperty(JSContext *cx, HandleObject obj, const char *name, HandleValue v)
>+{
>     JSAtom *atom = Atomize(cx, name, strlen(name));
>-    return atom && JS_SetPropertyById(cx, obj, AtomToId(atom), v);
>+    RootedId id(cx, AtomToId(atom));
>+    return atom && JS_SetPropertyById(cx, obj, id, v);
> }

This is broken, you're calling AtomToId on an unchecked pointer and Atomize is fallible. This crashes for me in OOM conditions.

Same for JS_SetUCProperty, as far as I can see. Please also check if this occurred somewhere else in the patch, because it's really hard to track these down sometimes :) Thanks.

Comment 60

[Jon Coppeard (:jonco) (PTO until 3rd April)]

You're right, I've broken this.  Fix is on the way.

Comment 61

[Jon Coppeard (:jonco) (PTO until 3rd April)]

Attachment: fix-set-api-oom

Fix for OOM crash added by previous patch.

Comment 62

[Jon Coppeard (:jonco) (PTO until 3rd April)]

Attachment: unhandlifiedGcFunctions.txt

Updated list of functions needing handlification, not including pending patches on this bug.  Down to 72 from 177.

Comment 63

[Jon Coppeard (:jonco) (PTO until 3rd April)]

Attachment: handlify_8

Handlify a couple of misc APIs.

Comment 64

[Jon Coppeard (:jonco) (PTO until 3rd April)]

Attachment: handlify_9

Handlify some friend APIs.

Comment 65

[Terrence Cole [:terrence]]

Comment on attachment 8365860 [details] [diff] [review]
fix-set-api-oom

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

D'oh! Good catch! r=me

Comment 66

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8366031 [details] [diff] [review]
handlify_8

># HG changeset patch
># Parent 679cadb9faea5ea73a042cb093c10a9ec38821c5
># User Jon Coppeard <[e-mail]>
>Handlify JS_InitClass() and JS::RegisterPerfMeasurement()
>
>diff --git a/dom/indexedDB/IDBObjectStore.cpp b/dom/indexedDB/IDBObjectStore.cpp
>--- a/dom/indexedDB/IDBObjectStore.cpp
>+++ b/dom/indexedDB/IDBObjectStore.cpp
>@@ -1037,19 +1037,18 @@ IDBObjectStore::AppendIndexUpdateInfo(
>     return NS_OK;
>   }
> 
>   JS::Rooted<JS::Value> val(aCx);
>   if (NS_FAILED(aKeyPath.ExtractKeyAsJSVal(aCx, aVal, val.address()))) {
>     return NS_OK;
>   }
> 
>-  if (!JSVAL_IS_PRIMITIVE(val) &&
>-      JS_IsArrayObject(aCx, JSVAL_TO_OBJECT(val))) {
>-    JS::Rooted<JSObject*> array(aCx, JSVAL_TO_OBJECT(val));
>+  if (JS_IsArrayObject(aCx, val)) {
>+    JS::Rooted<JSObject*> array(aCx, &val.toObject());
>     uint32_t arrayLength;
>     if (!JS_GetArrayLength(aCx, array, &arrayLength)) {
>       return NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR;
>     }
> 
>     for (uint32_t arrayIndex = 0; arrayIndex < arrayLength; arrayIndex++) {
>       JS::Rooted<JS::Value> arrayItem(aCx);
>       if (!JS_GetElement(aCx, array, arrayIndex, &arrayItem)) {
>diff --git a/dom/indexedDB/KeyPath.cpp b/dom/indexedDB/KeyPath.cpp
>--- a/dom/indexedDB/KeyPath.cpp
>+++ b/dom/indexedDB/KeyPath.cpp
>@@ -259,18 +259,17 @@ nsresult
> KeyPath::Parse(JSContext* aCx, const JS::Value& aValue_, KeyPath* aKeyPath)
> {
>   JS::Rooted<JS::Value> aValue(aCx, aValue_);
>   KeyPath keyPath(0);
> 
>   aKeyPath->SetType(NONEXISTENT);
> 
>   // See if this is a JS array.
>-  if (!JSVAL_IS_PRIMITIVE(aValue) &&
>-      JS_IsArrayObject(aCx, JSVAL_TO_OBJECT(aValue))) {
>+  if (JS_IsArrayObject(aCx, aValue)) {
> 
>     JS::Rooted<JSObject*> obj(aCx, JSVAL_TO_OBJECT(aValue));
> 
>     uint32_t length;
>     if (!JS_GetArrayLength(aCx, obj, &length)) {
>       return NS_ERROR_FAILURE;
>     }
> 
>diff --git a/dom/mobilemessage/src/MobileMessageManager.cpp b/dom/mobilemessage/src/MobileMessageManager.cpp
>--- a/dom/mobilemessage/src/MobileMessageManager.cpp
>+++ b/dom/mobilemessage/src/MobileMessageManager.cpp
>@@ -165,18 +165,17 @@ MobileMessageManager::Send(JSContext* aC
> NS_IMETHODIMP
> MobileMessageManager::Send(JS::Handle<JS::Value> aNumber,
>                            const nsAString& aMessage,
>                            JS::Handle<JS::Value> aSendParams,
>                            JSContext* aCx,
>                            uint8_t aArgc,
>                            JS::MutableHandle<JS::Value> aReturn)
> {
>-  if (!aNumber.isString() &&
>-      !(aNumber.isObject() && JS_IsArrayObject(aCx, &aNumber.toObject()))) {
>+  if (!aNumber.isString() && !JS_IsArrayObject(aCx, aNumber)) {
>     return NS_ERROR_INVALID_ARG;
>   }
> 
>   nsresult rv;
>   nsIScriptContext* sc = GetContextForEventHandlers(&rv);
>   NS_ENSURE_SUCCESS(rv, rv);
>   NS_ENSURE_STATE(sc);
> 
>@@ -335,17 +334,17 @@ MobileMessageManager::Delete(JS::Handle<
>   uint32_t size;
> 
>   if (aParam.isInt32()) {
>     // Single Integer Message ID
>     id = aParam.toInt32();
> 
>     size = 1;
>     idArray = &id;
>-  } else if (!JS_IsArrayObject(aCx, &aParam.toObject())) {
>+  } else if (!JS_IsArrayObject(aCx, aParam)) {
>     // Single SmsMessage/MmsMessage object
>     rv = GetMessageId(aCx, aParam, &id);
>     NS_ENSURE_SUCCESS(rv, rv);
> 
>     size = 1;
>     idArray = &id;
>   } else {
>     // Int32[], SmsMessage[], or MmsMessage[]
>diff --git a/dom/workers/File.cpp b/dom/workers/File.cpp
>--- a/dom/workers/File.cpp
>+++ b/dom/workers/File.cpp
>@@ -32,19 +32,19 @@ class Blob
>   ~Blob();
> 
>   static const JSClass sClass;
>   static const JSPropertySpec sProperties[];
>   static const JSFunctionSpec sFunctions[];
> 
> public:
>   static JSObject*
>-  InitClass(JSContext* aCx, JSObject* aObj)
>+  InitClass(JSContext* aCx, JS::Handle<JSObject*> aObj)
>   {
>-    return JS_InitClass(aCx, aObj, nullptr, &sClass, Construct, 0,
>+    return JS_InitClass(aCx, aObj, JS::NullPtr(), &sClass, Construct, 0,
>                         sProperties, sFunctions, nullptr, nullptr);
>   }
> 
>   static JSObject*
>   Create(JSContext* aCx, nsIDOMBlob* aBlob)
>   {
>     JS_ASSERT(SameCOMIdentity(static_cast<nsISupports*>(aBlob), aBlob));
> 
>@@ -232,17 +232,17 @@ class File : public Blob
>   File();
>   ~File();
> 
>   static const JSClass sClass;
>   static const JSPropertySpec sProperties[];
> 
> public:
>   static JSObject*
>-  InitClass(JSContext* aCx, JSObject* aObj, JSObject* aParentProto)
>+  InitClass(JSContext* aCx, JS::Handle<JSObject*> aObj, JS::Handle<JSObject*> aParentProto)
>   {
>     return JS_InitClass(aCx, aObj, aParentProto, &sClass, Construct, 0,
>                         sProperties, nullptr, nullptr, nullptr);
>   }
> 
>   static JSObject*
>   Create(JSContext* aCx, nsIDOMFile* aFile)
>   {
>@@ -465,17 +465,17 @@ JSObject*
> CreateBlob(JSContext* aCx, nsIDOMBlob* aBlob)
> {
>   return Blob::Create(aCx, aBlob);
> }
> 
> bool
> InitClasses(JSContext* aCx, JS::Handle<JSObject*> aGlobal)
> {
>-  JSObject* blobProto = Blob::InitClass(aCx, aGlobal);
>+  JS::Rooted<JSObject*> blobProto(aCx, Blob::InitClass(aCx, aGlobal));
>   return blobProto && File::InitClass(aCx, aGlobal, blobProto);
> }
> 
> nsIDOMBlob*
> GetDOMBlobFromJSObject(JSObject* aObj)
> {
>   return Blob::GetPrivate(aObj);
> }
>diff --git a/js/src/ctypes/CTypes.cpp b/js/src/ctypes/CTypes.cpp
>--- a/js/src/ctypes/CTypes.cpp
>+++ b/js/src/ctypes/CTypes.cpp
>@@ -1048,17 +1048,17 @@ static JSObject*
> InitInt64Class(JSContext* cx,
>                HandleObject parent,
>                const JSClass* clasp,
>                JSNative construct,
>                const JSFunctionSpec* fs,
>                const JSFunctionSpec* static_fs)
> {
>   // Init type class and constructor
>-  RootedObject prototype(cx, JS_InitClass(cx, parent, nullptr, clasp, construct,
>+  RootedObject prototype(cx, JS_InitClass(cx, parent, js::NullPtr(), clasp, construct,
>                                           0, nullptr, fs, nullptr, static_fs));
>   if (!prototype)
>     return nullptr;
> 
>   RootedObject ctor(cx, JS_GetConstructor(cx, prototype));
>   if (!ctor)
>     return nullptr;
>   if (!JS_FreezeObject(cx, ctor))
>@@ -5177,17 +5177,17 @@ StructType::FieldsArrayGetter(JSContext*
>     if (!fields)
>       return false;
>     JS_SetReservedSlot(obj, SLOT_FIELDS, OBJECT_TO_JSVAL(fields));
> 
>     args.rval().setObject(*fields);
>   }
> 
>   MOZ_ASSERT(args.rval().isObject());
>-  MOZ_ASSERT(JS_IsArrayObject(cx, &args.rval().toObject()));
>+  MOZ_ASSERT(JS_IsArrayObject(cx, args.rval()));
>   return true;
> }
> 
> bool
> StructType::FieldGetter(JSContext* cx, HandleObject obj, HandleId idval, MutableHandleValue vp)
> {
>   if (!CData::IsCData(obj)) {
>     JS_ReportError(cx, "not a CData");
>diff --git a/js/src/jsapi-tests/testClassGetter.cpp b/js/src/jsapi-tests/testClassGetter.cpp
>--- a/js/src/jsapi-tests/testClassGetter.cpp
>+++ b/js/src/jsapi-tests/testClassGetter.cpp
>@@ -51,17 +51,17 @@ static bool test_fn(JSContext *cx, unsig
> 
> static const JSFunctionSpec ptestFunctions[] = {
>     JS_FS( "test_fn", test_fn, 0, 0 ),
>     JS_FS_END
> };
> 
> BEGIN_TEST(testClassGetter_isCalled)
> {
>-    CHECK(JS_InitClass(cx, global, nullptr, &ptestClass, PTest, 0,
>+    CHECK(JS_InitClass(cx, global, js::NullPtr(), &ptestClass, PTest, 0,
>                        nullptr, ptestFunctions, nullptr, nullptr));
> 
>     EXEC("function check() { var o = new PTest(); o.test_fn(); o.test_value1; o.test_value2; o.test_value1; }");
> 
>     for (int i = 1; i < 9; i++) {
>         JS::RootedValue rval(cx);
>         CHECK(JS_CallFunctionName(cx, global, "check", 0, nullptr, rval.address()));
>         CHECK_SAME(INT_TO_JSVAL(called_test_fn), INT_TO_JSVAL(i));
>diff --git a/js/src/jsapi-tests/testCustomIterator.cpp b/js/src/jsapi-tests/testCustomIterator.cpp
>--- a/js/src/jsapi-tests/testCustomIterator.cpp
>+++ b/js/src/jsapi-tests/testCustomIterator.cpp
>@@ -57,17 +57,17 @@ IterClassConstructor(JSContext *cx, unsi
>     if (!obj)
>         return false;
>     JS_SET_RVAL(cx, vp, OBJECT_TO_JSVAL(obj));
>     return true;
> }
> 
> BEGIN_TEST(testCustomIterator_bug612523)
> {
>-    CHECK(JS_InitClass(cx, global, nullptr, Jsvalify(&HasCustomIterClass),
>+    CHECK(JS_InitClass(cx, global, js::NullPtr(), Jsvalify(&HasCustomIterClass),
>                        IterClassConstructor, 0, nullptr, nullptr, nullptr, nullptr));
> 
>     JS::RootedValue result(cx);
>     EVAL("var o = new HasCustomIter(); \n"
>          "var j = 0; \n"
>          "for (var i in o) { ++j; }; \n"
>          "j;", result.address());
> 
>diff --git a/js/src/jsapi-tests/testObjectEmulatingUndefined.cpp b/js/src/jsapi-tests/testObjectEmulatingUndefined.cpp
>--- a/js/src/jsapi-tests/testObjectEmulatingUndefined.cpp
>+++ b/js/src/jsapi-tests/testObjectEmulatingUndefined.cpp
>@@ -23,17 +23,17 @@ ObjectEmulatingUndefinedConstructor(JSCo
>     if (!obj)
>         return false;
>     JS_SET_RVAL(cx, vp, OBJECT_TO_JSVAL(obj));
>     return true;
> }
> 
> BEGIN_TEST(testObjectEmulatingUndefined_truthy)
> {
>-    CHECK(JS_InitClass(cx, global, nullptr, &ObjectEmulatingUndefinedClass,
>+    CHECK(JS_InitClass(cx, global, js::NullPtr(), &ObjectEmulatingUndefinedClass,
>                        ObjectEmulatingUndefinedConstructor, 0,
>                        nullptr, nullptr, nullptr, nullptr));
> 
>     JS::RootedValue result(cx);
> 
>     EVAL("if (new ObjectEmulatingUndefined()) true; else false;", result.address());
>     CHECK_SAME(result, JSVAL_FALSE);
> 
>@@ -49,17 +49,17 @@ BEGIN_TEST(testObjectEmulatingUndefined_
>     CHECK_SAME(result, JSVAL_TRUE);
> 
>     return true;
> }
> END_TEST(testObjectEmulatingUndefined_truthy)
> 
> BEGIN_TEST(testObjectEmulatingUndefined_equal)
> {
>-    CHECK(JS_InitClass(cx, global, nullptr, &ObjectEmulatingUndefinedClass,
>+    CHECK(JS_InitClass(cx, global, js::NullPtr(), &ObjectEmulatingUndefinedClass,
>                        ObjectEmulatingUndefinedConstructor, 0,
>                        nullptr, nullptr, nullptr, nullptr));
> 
>     JS::RootedValue result(cx);
> 
>     EVAL("if (new ObjectEmulatingUndefined() == undefined) true; else false;", result.address());
>     CHECK_SAME(result, JSVAL_TRUE);
> 
>diff --git a/js/src/jsapi-tests/testProfileStrings.cpp b/js/src/jsapi-tests/testProfileStrings.cpp
>--- a/js/src/jsapi-tests/testProfileStrings.cpp
>+++ b/js/src/jsapi-tests/testProfileStrings.cpp
>@@ -77,17 +77,17 @@ static const JSFunctionSpec ptestFunctio
>     JS_FS_END
> };
> 
> static JSObject*
> initialize(JSContext *cx)
> {
>     js::SetRuntimeProfilingStack(cx->runtime(), pstack, &psize, 10);
>     JS::RootedObject global(cx, JS::CurrentGlobalOrNull(cx));
>-    return JS_InitClass(cx, global, nullptr, &ptestClass, Prof, 0,
>+    return JS_InitClass(cx, global, js::NullPtr(), &ptestClass, Prof, 0,
>                         nullptr, ptestFunctions, nullptr, nullptr);
> }
> 
> BEGIN_TEST(testProfileStrings_isCalledWithInterpreter)
> {
>     CHECK(initialize(cx));
> 
>     EXEC("function g() { var p = new Prof(); p.test_fn(); }");
>diff --git a/js/src/jsapi.cpp b/js/src/jsapi.cpp
>--- a/js/src/jsapi.cpp
>+++ b/js/src/jsapi.cpp
>@@ -2289,23 +2289,21 @@ JS_PUBLIC_API(bool)
> JS_ConvertStub(JSContext *cx, HandleObject obj, JSType type, MutableHandleValue vp)
> {
>     JS_ASSERT(type != JSTYPE_OBJECT && type != JSTYPE_FUNCTION);
>     JS_ASSERT(obj);
>     return DefaultValue(cx, obj, type, vp);
> }
> 
> JS_PUBLIC_API(JSObject *)
>-JS_InitClass(JSContext *cx, JSObject *objArg, JSObject *parent_protoArg,
>+JS_InitClass(JSContext *cx, HandleObject obj, HandleObject parent_proto,
>              const JSClass *clasp, JSNative constructor, unsigned nargs,
>              const JSPropertySpec *ps, const JSFunctionSpec *fs,
>              const JSPropertySpec *static_ps, const JSFunctionSpec *static_fs)
> {
>-    RootedObject obj(cx, objArg);
>-    RootedObject parent_proto(cx, parent_protoArg);
>     AssertHeapIsIdle(cx);
>     CHECK_REQUEST(cx);
>     assertSameCompartment(cx, obj, parent_proto);
>     return js_InitClass(cx, obj, parent_proto, Valueify(clasp), constructor,
>                         nargs, ps, fs, static_ps, static_fs);
> }
> 
> JS_PUBLIC_API(bool)
>@@ -3814,24 +3812,32 @@ JS_NewArrayObject(JSContext *cx, int len
>     AssertHeapIsIdle(cx);
>     CHECK_REQUEST(cx);
> 
>     assertSameCompartment(cx, JSValueArray(vector, vector ? (uint32_t)length : 0));
>     return NewDenseCopiedArray(cx, (uint32_t)length, vector);
> }
> 
> JS_PUBLIC_API(bool)
>-JS_IsArrayObject(JSContext *cx, JSObject *objArg)
>-{
>-    RootedObject obj(cx, objArg);
>+JS_IsArrayObject(JSContext *cx, JS::HandleObject obj)
>+{
>     assertSameCompartment(cx, obj);
>     return ObjectClassIs(obj, ESClass_Array, cx);
> }
> 
> JS_PUBLIC_API(bool)
>+JS_IsArrayObject(JSContext *cx, JS::HandleValue value)
>+{
>+    if (!value.isObject())
>+        return false;
>+    RootedObject obj(cx, &value.toObject());
>+    return JS_IsArrayObject(cx, obj);
>+}
>+
>+JS_PUBLIC_API(bool)
> JS_GetArrayLength(JSContext *cx, HandleObject obj, uint32_t *lengthp)
> {
>     AssertHeapIsIdle(cx);
>     CHECK_REQUEST(cx);
>     assertSameCompartment(cx, obj);
>     return GetLengthProperty(cx, obj, lengthp);
> }
> 
>diff --git a/js/src/jsapi.h b/js/src/jsapi.h
>--- a/js/src/jsapi.h
>+++ b/js/src/jsapi.h
>@@ -2521,17 +2521,17 @@ struct JSFunctionSpec {
> #define JS_FNINFO(name,call,info,nargs,flags)                                 \
>     JS_FNSPEC(name, call, info, nargs, flags, nullptr)
> #define JS_SELF_HOSTED_FN(name,selfHostedName,nargs,flags)                    \
>     JS_FNSPEC(name, nullptr, nullptr, nargs, flags, selfHostedName)
> #define JS_FNSPEC(name,call,info,nargs,flags,selfHostedName)                  \
>     {name, {call, info}, nargs, flags, selfHostedName}
> 
> extern JS_PUBLIC_API(JSObject *)
>-JS_InitClass(JSContext *cx, JSObject *obj, JSObject *parent_proto,
>+JS_InitClass(JSContext *cx, JS::HandleObject obj, JS::HandleObject parent_proto,
>              const JSClass *clasp, JSNative constructor, unsigned nargs,
>              const JSPropertySpec *ps, const JSFunctionSpec *fs,
>              const JSPropertySpec *static_ps, const JSFunctionSpec *static_fs);
> 
> /*
>  * Set up ctor.prototype = proto and proto.constructor = ctor with the
>  * right property flags.
>  */
>@@ -3081,17 +3081,20 @@ JS_SetUCProperty(JSContext *cx, JS::Hand
> extern JS_PUBLIC_API(bool)
> JS_DeleteUCProperty2(JSContext *cx, JSObject *obj, const jschar *name, size_t namelen,
>                      bool *succeeded);
> 
> extern JS_PUBLIC_API(JSObject *)
> JS_NewArrayObject(JSContext *cx, int length, jsval *vector);
> 
> extern JS_PUBLIC_API(bool)
>-JS_IsArrayObject(JSContext *cx, JSObject *obj);
>+JS_IsArrayObject(JSContext *cx, JS::HandleValue value);
>+
>+extern JS_PUBLIC_API(bool)
>+JS_IsArrayObject(JSContext *cx, JS::HandleObject obj);
> 
> extern JS_PUBLIC_API(bool)
> JS_GetArrayLength(JSContext *cx, JS::Handle<JSObject*> obj, uint32_t *lengthp);
> 
> extern JS_PUBLIC_API(bool)
> JS_SetArrayLength(JSContext *cx, JS::Handle<JSObject*> obj, uint32_t length);
> 
> extern JS_PUBLIC_API(bool)
>diff --git a/js/src/perf/jsperf.cpp b/js/src/perf/jsperf.cpp
>--- a/js/src/perf/jsperf.cpp
>+++ b/js/src/perf/jsperf.cpp
>@@ -219,20 +219,21 @@ GetPM(JSContext* cx, JS::HandleValue val
>     JS_ReportErrorNumber(cx, js_GetErrorMessage, 0, JSMSG_INCOMPATIBLE_PROTO,
>                          pm_class.name, fname, JS_GetClass(obj)->name);
>     return nullptr;
> }
> 
> namespace JS {
> 
> JSObject*
>-RegisterPerfMeasurement(JSContext *cx, JSObject *global)
>+RegisterPerfMeasurement(JSContext *cx, JSObject *globalArg)
> {
>+    RootedObject global(cx, globalArg);
>     RootedObject prototype(cx);
>-    prototype = JS_InitClass(cx, global, nullptr /* parent */,
>+    prototype = JS_InitClass(cx, global, js::NullPtr() /* parent */,
>                              &pm_class, pm_construct, 1,
>                              pm_props, pm_fns, 0, 0);
>     if (!prototype)
>         return 0;
> 
>     RootedObject ctor(cx);
>     ctor = JS_GetConstructor(cx, prototype);
>     if (!ctor)
>diff --git a/js/src/shell/js.cpp b/js/src/shell/js.cpp
>--- a/js/src/shell/js.cpp
>+++ b/js/src/shell/js.cpp
>@@ -5383,18 +5383,18 @@ NewGlobalObject(JSContext *cx, JS::Compa
>             return nullptr;
> 
>         /* Initialize FakeDOMObject. */
>         static const js::DOMCallbacks DOMcallbacks = {
>             InstanceClassHasProtoAtDepth
>         };
>         SetDOMCallbacks(cx->runtime(), &DOMcallbacks);
> 
>-        RootedObject domProto(cx, JS_InitClass(cx, glob, nullptr, &dom_class, dom_constructor, 0,
>-                                               dom_props, dom_methods, nullptr, nullptr));
>+        RootedObject domProto(cx, JS_InitClass(cx, glob, js::NullPtr(), &dom_class, dom_constructor,
>+                                               0, dom_props, dom_methods, nullptr, nullptr));
>         if (!domProto)
>             return nullptr;
> 
>         /* Initialize FakeDOMObject.prototype */
>         InitDOMObject(domProto);
>     }
> 
>     JS_FireOnNewGlobalObject(cx, glob);
>diff --git a/js/xpconnect/loader/mozJSComponentLoader.cpp b/js/xpconnect/loader/mozJSComponentLoader.cpp
>--- a/js/xpconnect/loader/mozJSComponentLoader.cpp
>+++ b/js/xpconnect/loader/mozJSComponentLoader.cpp
>@@ -1266,18 +1266,17 @@ mozJSComponentLoader::ImportInto(const n
> 
>         RootedValue symbols(mContext);
>         if (!JS_GetProperty(mContext, mod->obj,
>                             "EXPORTED_SYMBOLS", &symbols)) {
>             return ReportOnCaller(cxhelper, ERROR_NOT_PRESENT,
>                                   PromiseFlatCString(aLocation).get());
>         }
> 
>-        if (!symbols.isObject() ||
>-            !JS_IsArrayObject(mContext, &symbols.toObject())) {
>+        if (!JS_IsArrayObject(mContext, symbols)) {
>             return ReportOnCaller(cxhelper, ERROR_NOT_AN_ARRAY,
>                                   PromiseFlatCString(aLocation).get());
>         }
> 
>         RootedObject symbolsObj(mContext, &symbols.toObject());
> 
>         // Iterate over symbols array, installing symbols on targetObj:
> 
>diff --git a/toolkit/components/places/History.cpp b/toolkit/components/places/History.cpp
>--- a/toolkit/components/places/History.cpp
>+++ b/toolkit/components/places/History.cpp
>@@ -408,33 +408,33 @@ GetIntFromJSObject(JSContext* aCtx,
>  * @pre aArray must be an Array object.
>  *
>  * @param aCtx
>  *        The JSContext for aArray.
>  * @param aArray
>  *        The JSObject to get the object from.
>  * @param aIndex
>  *        The index to get the object from.
>- * @param _object
>- *        The JSObject pointer on success.
>+ * @param objOut
>+ *        Set to the JSObject pointer on success.
>  */
> nsresult
> GetJSObjectFromArray(JSContext* aCtx,
>-                     JSObject* aArray,
>+                     JS::Handle<JSObject*> aArray,
>                      uint32_t aIndex,
>-                     JSObject** _rooter)
>+                     JS::MutableHandle<JSObject*> objOut)
> {
>   NS_PRECONDITION(JS_IsArrayObject(aCtx, aArray),
>                   "Must provide an object that is an array!");
> 
>   JS::Rooted<JS::Value> value(aCtx);
>   bool rc = JS_GetElement(aCtx, aArray, aIndex, &value);
>   NS_ENSURE_TRUE(rc, NS_ERROR_UNEXPECTED);
>-  NS_ENSURE_ARG(!JSVAL_IS_PRIMITIVE(value));
>-  *_rooter = JSVAL_TO_OBJECT(value);
>+  NS_ENSURE_ARG(!value.isPrimitive());
>+  objOut.set(&value.toObject());
>   return NS_OK;
> }
> 
> class VisitedQuery : public AsyncStatementCallback
> {
> public:
>   static nsresult Start(nsIURI* aURI,
>                         mozIVisitedStatusCallback* aCallback=nullptr)
>@@ -2755,17 +2755,17 @@ History::UpdatePlaces(JS::Handle<JS::Val
>   uint32_t infosLength;
>   JS::Rooted<JSObject*> infos(aCtx);
>   nsresult rv = GetJSArrayFromJSValue(aPlaceInfos, aCtx, &infos, &infosLength);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   nsTArray<VisitData> visitData;
>   for (uint32_t i = 0; i < infosLength; i++) {
>     JS::Rooted<JSObject*> info(aCtx);
>-    nsresult rv = GetJSObjectFromArray(aCtx, infos, i, info.address());
>+    nsresult rv = GetJSObjectFromArray(aCtx, infos, i, &info);
>     NS_ENSURE_SUCCESS(rv, rv);
> 
>     nsCOMPtr<nsIURI> uri = GetURIFromJSObject(aCtx, info, "uri");
>     nsCString guid;
>     {
>       nsString fatGUID;
>       GetStringFromJSObject(aCtx, info, "guid", fatGUID);
>       if (fatGUID.IsVoid()) {
>@@ -2809,17 +2809,17 @@ History::UpdatePlaces(JS::Handle<JS::Val
>       (void)JS_GetArrayLength(aCtx, visits, &visitsLength);
>     }
>     NS_ENSURE_ARG(visitsLength > 0);
> 
>     // Check each visit, and build our array of VisitData objects.
>     visitData.SetCapacity(visitData.Length() + visitsLength);
>     for (uint32_t j = 0; j < visitsLength; j++) {
>       JS::Rooted<JSObject*> visit(aCtx);
>-      rv = GetJSObjectFromArray(aCtx, visits, j, visit.address());
>+      rv = GetJSObjectFromArray(aCtx, visits, j, &visit);
>       NS_ENSURE_SUCCESS(rv, rv);
> 
>       VisitData& data = *visitData.AppendElement(VisitData(uri));
>       data.title = title;
>       data.guid = guid;
> 
>       // We must have a date and a transaction type!
>       rv = GetIntFromJSObject(aCtx, visit, "visitDate", &data.visitTime);

Comment 67

[Jon Coppeard (:jonco) (PTO until 3rd April)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/29a9828e695f

Comment 68

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/29a9828e695f

Comment 69

[Jon Coppeard (:jonco) (PTO until 3rd April)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c814dc2ff60e
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f535f3864ad

Comment 70

[Jon Coppeard (:jonco) (PTO until 3rd April)]

Attachment: handlify-get-property

Patch to handlify JS_GetPropery*() APIs plus a couple of others.

Comment 71

[Terrence Cole [:terrence]]

Comment on attachment 8367445 [details] [diff] [review]
handlify-get-property

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

Seems reasonable to me. r=me

Comment 72

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c814dc2ff60e
https://hg.mozilla.org/mozilla-central/rev/8f535f3864ad

Comment 73

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8367445 [details] [diff] [review]
handlify-get-property

>+++ b/dom/base/nsDOMClassInfo.cpp
>+  JS::Rooted<JSObject *> self(cx);

Please lose the extra space before '*' there.

>+++ b/dom/bindings/CallbackInterface.cpp
>+  JS::Rooted<JSObject*> callback(cx, mCallback);
>+  if (!JS_GetProperty(cx, callback, aPropName, aCallable)) {

Please drop the unnecessary root, and replace mCallback with CallbackPreserveColor().

>+++ b/dom/bindings/Codegen.py
>+            'JS::Rooted<JSObject *> callback(cx, mCallback);\n'
>+            'if (!JS_GetProperty(cx, callback, "${attrName}", &rval)) {\n'

Likewise.

>+        # Add include statement.

How about documenting _why_ we're including that file?  Presumably for InternedStringId?

I _think_ this is OK, but maybe make sure that this doesn't cause us to include BindingUtils.h in all binding headers or anything like that.

>+++ b/dom/media/MediaManager.cpp

I wonder whether operator[] on AutoIdArray should return a Handle... seems like it could.  Followup fine here.

r=me on the non-js-engine bits with the above nits fixed.

Comment 74

[Jon Coppeard (:jonco) (PTO until 3rd April)]

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

Comment 75

[Carsten Book [:Tomcat]]

(In reply to Jon Coppeard (:jonco) from comment #74)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/e2c75ec7f3d4

backedout for breaking b2g builds like https://tbpl.mozilla.org/php/getParsedLog.php?id=33868970&tree=Mozilla-Inbound

Comment 76

[Jon Coppeard (:jonco) (PTO until 3rd April)]

Re-landing previous patch:

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

Comment 77

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/0c194a7651a5

Comment 78

[Jeff Walden [:Waldo]]

Comment on attachment 8363385 [details] [diff] [review]
handlify_DefineProperty-v1.diff

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

::: content/html/content/src/HTMLMediaElement.cpp
@@ +1603,5 @@
>      NS_WARNING("Failed to perform string copy");
>      args->error = true;
>      return PL_DHASH_STOP;
>    }
> +  JS::Rooted<JS::Value> value(args->cx, JS::StringValue(string));

Make string into a Rooted?

::: dom/base/nsDOMClassInfo.cpp
@@ +2190,5 @@
>      switch (type) {
>        case nsXPTType::T_I8:
>        case nsXPTType::T_U8:
>        {
> +        v = JS::Int32Value(c->GetValue()->val.u8);

Maybe setInt32?  Or setNumber, if the Int32 is better latent/determined by the compiler; I could go either way on it.

@@ +2196,5 @@
>        }
>        case nsXPTType::T_I16:
>        case nsXPTType::T_U16:
>        {
> +        v = JS::Int32Value(c->GetValue()->val.u16);

Again.

::: dom/bindings/BindingUtils.cpp
@@ +396,5 @@
>    }
>  
>    // This is Enumerable: False per spec.
>    return alreadyDefined ||
> +         JS_DefineProperty(cx, global, name, constructor);

This is one of those cases, I think, where making attrs not a defaultable argument helps with readability and all.  (Better would be to see enumerable/non-enumerable and all those, instead of just a plain 0.  We'll get to that API in time here.)

::: dom/indexedDB/IDBObjectStore.cpp
@@ +921,4 @@
>      JS::Rooted<JSString*> type(aCx,
>        JS_NewUCStringCopyN(aCx, aData.type.get(), aData.type.Length()));
>      if (!type ||
> +        !JS_DefineProperty(aCx, obj, "size", size) ||

Why not just pass double(aData.size)?

::: js/src/jsapi.cpp
@@ +3162,5 @@
> +JS_DefineProperty(JSContext *cx, HandleObject obj, const char *name, double valueArg,
> +                  unsigned attrs /* = 0 */,
> +                  PropertyOp getter /* = nullptr */, JSStrictPropertyOp setter /* = nullptr */)
> +{
> +    Value value = DoubleValue(valueArg);

Not NumberValue?

::: js/src/jsapi.h
@@ +2770,5 @@
>  JS_DefineProperties(JSContext *cx, JSObject *obj, const JSPropertySpec *ps);
>  
>  extern JS_PUBLIC_API(bool)
> +JS_DefineProperty(JSContext *cx, JS::HandleObject obj, const char *name, JS::HandleValue value,
> +                  unsigned attrs = 0,

I'd prefer if attrs weren't defaulted, and people had to specify them explicitly.  The ES5/ES6 semantics for omitted attributes when defining a property, aren't the same as the semantics in play here.  Seems an opportunity for slight confusion to me.

(Of course even the semantics here aren't the same as the ES5 explicitly-specified-attributes merging semantics.  Still, slightly more explicitness about this until we fix our APIs to follow the ES5 form seems worth having.)

@@ +2784,5 @@
> +                  unsigned attrs = 0,
> +                  JSPropertyOp getter = nullptr, JSStrictPropertyOp setter = nullptr);
> +
> +extern JS_PUBLIC_API(bool)
> +JS_DefineProperty(JSContext *cx, JS::HandleObject obj, const char *name, int32_t value,

Having int32_t/uint32_t/double overloads here is probably going to be problematic at some point.  Someone's going to write code that happens to exactly match a signature wrt that one parameter, using a built-in type.  Then it's not going to compile on some other platform, because long != int32_t there, or similar.  Oh well, I don't see another option offhand.

::: js/src/jsfriendapi.cpp
@@ +281,5 @@
>      JSAtom *atom = Atomize(cx, value, strlen(value));
>      if (!atom)
>          return false;
> +    RootedValue atomVal(cx, StringValue(atom));
> +    return JS_DefineProperty(cx, obj, prop, atomVal, JSPROP_READONLY | JSPROP_PERMANENT,

Could this just make |atom| into a rooted, then pass that?  I think it could.

::: js/src/shell/js.cpp
@@ +4872,5 @@
>  static const JSPropertySpec dom_props[] = {
>      {"x", 0,
>       JSPROP_SHARED | JSPROP_ENUMERATE | JSPROP_NATIVE_ACCESSORS,
> +     { { (JSPropertyOp)dom_genericGetter, &dom_x_getterinfo } },
> +     { { (JSStrictPropertyOp)dom_genericSetter, &dom_x_setterinfo } }

I think fixed in another bug, at this point.

::: js/xpconnect/loader/mozJSComponentLoader.cpp
@@ +728,5 @@
>      JSString *exposedUri = JS_NewStringCopyN(aCx, nativePath.get(),
>                                               nativePath.Length());
> +    NS_ENSURE_TRUE(exposedUri, nullptr);
> +
> +    RootedValue exposedUriVal(aCx, StringValue(exposedUri));

RootedString up exposedUri and pass that to JS_DefineProperty.

::: tools/profiler/JSObjectBuilder.cpp
@@ +34,5 @@
>    if (!mOk)
>      return;
>  
> +  JS::RootedValue rootedValue(mCx, JS::DoubleValue(value));
> +  mOk = JS_DefineProperty(mCx, aObject, name, rootedValue, JSPROP_ENUMERATE);

Hmm, why doesn't the double overload work here?

@@ +52,5 @@
>    if (!mOk)
>      return;
>  
> +  JS::RootedValue rootedString(mCx, JS::StringValue(string));
> +  mOk = JS_DefineProperty(mCx, aObject, name, rootedString, JSPROP_ENUMERATE);

I'd make |string| a Rooted rather than having a Rooted for it as a value.

@@ +68,5 @@
>      return;
>    }
>  
> +  JS::RootedValue rootedString(mCx, JS::StringValue(string));
> +  mOk = JS_DefineProperty(mCx, aObject, name, rootedString, JSPROP_ENUMERATE); }

Same.

Comment 79

[Terrence Cole [:terrence]]

Attachment: handlify_defineproperty-v2.diff

I agree that defaulting |attrs| was a terrible idea. Fortunately, quite easy to fix.

Regarding all the places were I rooted a string in a value: rooting the strings instead of the value ends up requiring a second internal root for the Value, so saves a Rooted. That said, none of these call sites need that sort of hyper-optimization, particularly not at the cost of obviousness. Regarding the doubles, that's why I asked for your feedback: there are like a million call sites and I was sure you catch some obvious stuff I missed.

Comment 80

[Jon Coppeard (:jonco) (PTO until 3rd April)]

Attachment: handlify-call-js

Patch to handlify Call APIs

Comment 81

[Jon Coppeard (:jonco) (PTO until 3rd April)]

Attachment: handlify-call-browser

Patch to update browser uses of JS Call APIs following handlification.

Comment 82

[Terrence Cole [:terrence]]

Comment on attachment 8374894 [details] [diff] [review]
handlify-call-js

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

\o/ r=me

::: js/src/shell/js.cpp
@@ +3915,2 @@
>      // The function we should call to lazily retrieve source code.
> +    PersistentRootedFunction fun;

Nice!

Comment 83

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8374895 [details] [diff] [review]
handlify-call-browser

May want to file a followup to switch from JS_Call* to JS::Call, right?

>+++ b/dom/bindings/Codegen.py
>+    def getThisDecl(self):
>+        return ""

This should go in CallCallback, not in CallbackMethod.  That way it's near the corresponding getThisVal method; right now it's not clear at all why CallbackMethod should be defaulting to "".

>+            "getThis": self.getThisDecl(),

I'd rather call this string "declThis".  Yes, I know we have "getCallable", but it should be called "declCallable" as well.  ;)

>+++ b/js/ipc/JavaScriptChild.cpp
>-        bool success = JS::Call(cx, vals[1], vals[0], args, &rval);
>+        bool success = JS::Call(cx, vals.handleAt(1), vals.handleAt(0), args, &rval);

Followup to make operator[] return a handle here?

r=me with those fixed.

Comment 84

[Jon Coppeard (:jonco) (PTO until 3rd April)]

(In reply to Boris Zbarsky [:bz] from comment #83)
Thanks, comments addressed.

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

Comment 85

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

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

Comment 86

[Jon Coppeard (:jonco) (PTO until 3rd April)]

Attachment: handlify_10

Handlify JS_ExecuteScript, JS_EvaluateScript and some other JS APIs

Comment 87

[Jon Coppeard (:jonco) (PTO until 3rd April)]

Attachment: handlify-11

Handlify some misc friend and debug APIs

Comment 88

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8376173 [details] [diff] [review]
handlify_10

r=me

Comment 89

[Bobby Holley (:bholley)]

Comment on attachment 8376175 [details] [diff] [review]
handlify-11

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

r=bholley on the changes outside of js/src.

Comment 90

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

Comment on attachment 8376173 [details] [diff] [review]
handlify_10

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

::: ipc/testshell/XPCShellEnvironment.cpp
@@ +165,5 @@
>          JS::CompileOptions options(cx);
>          options.setUTF8(true)
>                 .setFileAndLine(filename.ptr(), 1)
>                 .setPrincipals(Environment(global)->GetPrincipal());
> +        JS::Rooted<JSScript*> script(cx, JS::Compile(cx, obj, options, file));

Heh. I had to look at this one to see that it was already rooted.

Note that there are a couple of Rooteds that ought to be hoisted out of this loop, but I don't suppose you'd pass very many scripts to Load at a time anyway, so I don't think it matters. (And it's a test shell, and execution overhead swamps Rooted construction, etc.)

::: security/manager/ssl/src/nsCrypto.cpp
@@ -2179,5 @@
>    nsNSSShutDownPreventionLock locker;
>    NS_ASSERTION(args,"Passed nullptr to nsCryptoRunnable constructor.");
>    m_args = args;
>    NS_IF_ADDREF(m_args);
> -  JS_AddNamedObjectRoot(args->m_cx, &args->m_scope,"nsCryptoRunnable::mScope");

Very nice to see this go!

Comment 91

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

Comment on attachment 8376175 [details] [diff] [review]
handlify-11

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

stealing review

Comment 92

[Frederic Bezies]

Sorry to spam this bug report, but could it lead to a bug like this one : https://bugzilla.mozilla.org/show_bug.cgi?id=973192 ?

Comment 93

[Jeff Walden [:Waldo]]

Comment on attachment 8371138 [details] [diff] [review]
handlify_defineproperty-v2.diff

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

::: content/base/src/nsFrameMessageManager.cpp
@@ +966,4 @@
>  
>        // message.principal == null
>        if (!aPrincipal) {
> +        JS_DefineProperty(cx, param, "principal", JS::UndefinedHandleValue, JSPROP_ENUMERATE);

Wow, that old code was dodgy doing it that way.

@@ +983,5 @@
>          nsCString origin;
>          rv = aPrincipal->GetOrigin(getter_Copies(origin));
>          NS_ENSURE_SUCCESS(rv, rv);
>  
> +        JS::Rooted<JSString*> originStr(cx, JS_InternString(cx, origin.get()));

Uh, JS_NewStringCopyN.  If this should be interned, it should be in a separate patch.

::: content/html/content/src/HTMLMediaElement.cpp
@@ +1597,5 @@
>  {
>    MetadataIterCx* args = static_cast<MetadataIterCx*>(aUserArg);
>  
>    nsString wideValue = NS_ConvertUTF8toUTF16(aValue);
> +  JS::Rooted<JSString*> string(args->cx, JS_NewUCStringCopyZ(args->cx, wideValue.Data()));

Change to CopyN with wideValue.Length().  As it is, I'm pretty sure this is technically a bug with videoElement.mozGetMetadata now.

::: dom/base/nsDOMClassInfo.cpp
@@ +2117,5 @@
>      NS_ENSURE_TRUE(NS_SUCCEEDED(rv) && c, rv);
>  
>      uint16_t type = c->GetType().TagPart();
>  
> +    v.setUndefined();

This seems slightly undesirable in that it could hide a bug, but whatever.

@@ +3243,5 @@
>  
>    // Create a fake interfaces object.
>    JS::Rooted<JSObject*> interfaces(cx, JS_NewObject(cx, nullptr, JS::NullPtr(), global));
>    NS_ENSURE_TRUE(interfaces, NS_ERROR_OUT_OF_MEMORY);
> +  JS::Rooted<JS::Value> interfacesValue(cx, JS::ObjectValue(*interfaces));

You don't need this, do you?  Just pass |interfaces| directly.

::: dom/base/nsJSEnvironment.cpp
@@ +1201,5 @@
>    JSObject *args = ::JS_NewArrayObject(mContext, argc, array.start());
>    if (!args) {
>      return NS_ERROR_FAILURE;
>    }
> +  JS::Rooted<JS::Value> vargs(mContext, JS::ObjectValue(*args));

As before, seems like a Rooted object for |args| would be better here.

::: dom/mobilemessage/src/MmsMessage.cpp
@@ +502,5 @@
>                                     info.receiver.Length());
>      NS_ENSURE_TRUE(tmpJsStr, NS_ERROR_OUT_OF_MEMORY);
>  
>      tmpJsVal.setString(tmpJsStr);
> +    if (!JS_DefineProperty(aCx, infoJsObj, "receiver", tmpJsVal, JSPROP_ENUMERATE)) {

How about converting tmpJsStr to a Rooted<JSString*>, then get rid of tmpJsVal?

@@ +519,5 @@
>      }
>  
>      // Get |info.deliveryTimestamp|.
>      tmpJsVal.setNumber(static_cast<double>(info.deliveryTimestamp));
> +    if (!JS_DefineProperty(aCx, infoJsObj, "deliveryTimestamp", tmpJsVal, JSPROP_ENUMERATE)) {

...and pass double(info.deliveryTimestamp) as the value here.

@@ +536,5 @@
>      }
>  
>      // Get |info.readTimestamp|.
>      tmpJsVal.setNumber(static_cast<double>(info.readTimestamp));
> +    if (!JS_DefineProperty(aCx, infoJsObj, "readTimestamp", tmpJsVal, JSPROP_ENUMERATE)) {

and double(info.readTimestamp)

@@ +628,5 @@
>                                     attachment.id.Length());
>      NS_ENSURE_TRUE(tmpJsStr, NS_ERROR_OUT_OF_MEMORY);
>  
>      tmpJsVal.setString(tmpJsStr);
> +    if (!JS_DefineProperty(aCx, attachmentObj, "id", tmpJsVal, JSPROP_ENUMERATE)) {

Again make tmpJsStr a rooted and use it directly.

@@ +652,5 @@
>                                               &NS_GET_IID(nsIDOMBlob),
>                                               &tmpJsVal);
>      NS_ENSURE_SUCCESS(rv, rv);
>  
> +    if (!JS_DefineProperty(aCx, attachmentObj, "content", tmpJsVal, JSPROP_ENUMERATE)) {

Guess you need tmpJsVal for this one thing, but you can use it only for this one thing, at least.

::: dom/mobilemessage/src/ipc/SmsParent.cpp
@@ +120,5 @@
>      }
>    }
>  
> +  JS::Rooted<JS::Value> attachmentArrayValue(aCx, JS::ObjectValue(*attachmentArray));
> +  if (!JS_DefineProperty(aCx, paramsObj, "attachments", attachmentArrayValue, 0)) {

Uh, why not pass attachmentArray directly?

::: js/src/ctypes/CTypes.cpp
@@ +886,1 @@
>      return nullptr;

Separate patch, but don't we have something like JS_LinkConstructorAndPrototype to do this?

@@ +942,1 @@
>      return nullptr;

And again.

@@ +1003,1 @@
>      return false;

And maybe again, with some fussing.

@@ +1353,5 @@
> +                         JSPROP_ENUMERATE | JSPROP_READONLY | JSPROP_PERMANENT))
> +    return false;
> +
> +  if (!JS_DefineProperty(cx, prototype, "constructor", ctor,
> +                         JSPROP_ENUMERATE | JSPROP_READONLY | JSPROP_PERMANENT))

Again.

::: js/src/jsapi.h
@@ +2764,5 @@
>  JS_DefineProperties(JSContext *cx, JSObject *obj, const JSPropertySpec *ps);
>  
>  extern JS_PUBLIC_API(bool)
> +JS_DefineProperty(JSContext *cx, JS::HandleObject obj, const char *name, JS::HandleValue value,
> +                  unsigned attrs = 0,

Erm, I thought we were *not* going to be defaulting |attrs| in this patch?  Make this non-default and add 0 to the places that require it.

@@ +2769,5 @@
> +                  JSPropertyOp getter = nullptr, JSStrictPropertyOp setter = nullptr);
> +
> +extern JS_PUBLIC_API(bool)
> +JS_DefineProperty(JSContext *cx, JS::HandleObject obj, const char *name, JS::HandleObject value,
> +                  unsigned attrs = 0,

...and all the other overloads.

Comment 94

[Boris Zbarsky [:bzbarsky]]

> Uh, JS_NewStringCopyN.  If this should be interned, it should be in a separate patch.

It shouldn't be interned.  It used to be (because someone wrote the wrong thing), but that got fixed.  Presumably this patch was then mis-merged and backed out the fix?

Comment 95

[Boris Zbarsky [:bzbarsky]]

For that matter, please file a followup bug on the JS_InternStringN bit in the profiler code; that looks pretty wrong to me too.

Comment 96

[Jon Coppeard (:jonco) (PTO until 3rd April)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9f41c59d6a59
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ae9c4eef8d6

Comment 97

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/9f41c59d6a59
https://hg.mozilla.org/mozilla-central/rev/0ae9c4eef8d6

Comment 98

[Jon Coppeard (:jonco) (PTO until 3rd April)]

Attachment: handlify-12

Handlify yet more things.  I changed a couple of functions to take an optional CallArgs pointer from a JS::Value* where I thought it made more sense.

Comment 99

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

Comment on attachment 8393626 [details] [diff] [review]
handlify-12

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

Sneaking in the CallArgs-ification. :-)

Comment 100

[Jon Coppeard (:jonco) (PTO until 3rd April)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/95722f82f9cc

Comment 101

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/95722f82f9cc

Comment 102

[Jon Coppeard (:jonco) (PTO until 3rd April)]

Attachment: handlify-13-jsapi-tests

Patch to handlify evaluate/EVAL in jit-tests.

Comment 103

[Jon Coppeard (:jonco) (PTO until 3rd April)]

Attachment: handlify-14-exec-script

Patch to handlify JS_ExecuteScript and JS::Evaluate APIs.

Since these can take an optional jsval* for the result, and the operation is affected by whether this is supplied or not, I split these functions into two versions, with and without a MutableHandleValue.

I'll ask for browser review after internal review.

Comment 104

[Jon Coppeard (:jonco) (PTO until 3rd April)]

Attachment: handlify-15

This could be the final patch!  Handlify remaining miscellaneous APIs.

Comment 105

[Terrence Cole [:terrence]]

Comment on attachment 8398581 [details] [diff] [review]
handlify-13-jsapi-tests

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

r=me

Comment 106

[Terrence Cole [:terrence]]

Comment on attachment 8398582 [details] [diff] [review]
handlify-14-exec-script

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

Nice! r=me

Comment 107

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8398582 [details] [diff] [review]
handlify-14-exec-script

I'm not a huge fan of foisting the "set the option" thing on JSUtils callers.  Why not add an overload of EvaluateString() that does not take a result argument and internally does the options management and calls the other overload?

r=me with that.

Comment 108

[Jon Coppeard (:jonco) (PTO until 3rd April)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/92f8f685d940
https://hg.mozilla.org/integration/mozilla-inbound/rev/833ff3a90b83

Comment 109

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/92f8f685d940
https://hg.mozilla.org/mozilla-central/rev/833ff3a90b83

Comment 110

[Jon Coppeard (:jonco) (PTO until 3rd April)]

Comment on attachment 8398688 [details] [diff] [review]
handlify-15

Requesting review for the XPConnect parts.

Comment 111

[Jon Coppeard (:jonco) (PTO until 3rd April)]

Comment on attachment 8398688 [details] [diff] [review]
handlify-15

Requesting review for the browser parts.

Comment 112

[Bobby Holley (:bholley)]

Comment on attachment 8398688 [details] [diff] [review]
handlify-15

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

r=bholley on the xpconnect bits.

Comment 113

[Jon Coppeard (:jonco) (PTO until 3rd April)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/695622cd84e1

Comment 114

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

https://hg.mozilla.org/mozilla-central/rev/695622cd84e1

Comment 115

[Firefox Commit Bot]

The following changesets are now in Firefox Nightly:

> 92f8f685d940 Bug 959787 - Handlify jsapi-test eval functions r=terrence
> 833ff3a90b83 Bug 959787 - Handlify JS_ExecuteScript and JS::Evaluate APIs r=terrence r=bz

Nightly Build Information:

        ID: 20140402030201
 Changeset: 4941a2ac0786109b08856738019b016a6c5a66a6
   Version: 31.0a1
      TBPL: https://tbpl.mozilla.org/?rev=4941a2ac0786
       URL: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central

Download Links:

>         Linux x86: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-i686.tar.bz2
>      Linux x86_64: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-x86_64.tar.bz2
> Linux x86_64 ASAN: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-x86_64-asan.tar.bz2
>               Mac: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.mac.dmg
>             Win32: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.win32.installer.exe
>             Win64: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.win64-x86_64.installer.exe

Previous Nightly Build Information:

        ID: 20140401030203
 Changeset: 1417d180a1d8665b1a91b897d1cc4cc31e7980d4
   Version: 31.0a1
      TBPL: https://tbpl.mozilla.org/?rev=1417d180a1d8
       URL: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-01-03-02-03-mozilla-central

Comment 116

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1b7eef53c08a

Green try run here:
https://tbpl.mozilla.org/?tree=Try&rev=91ca616c5418

Comment 117

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1b7eef53c08a

Comment 118

[Jon Coppeard (:jonco) (PTO until 3rd April)]

Attachment: handlify-16

This really should be the last patch here and then we can close this bug.

Handlify JS_DefinePropertyById, JS_DefineUCProperty, JS_DefineElement and JS_DefineObject.

I created overloads of the first three for different value types in the same way as was done for JS_DefineProperty.  I also changed js::CheckDefineProperty to match this argument order and added defaults for the getter and setter arguments.

For JS_DefineObject I added default arguments for clasp, proto and attrs since these were almost always nullptr/zero.  Maybe we should have attrs first and without a default though, like the property definition APIs.

Requesting review for /js parts first.

Comment 119

[Terrence Cole [:terrence]]

Comment on attachment 8411652 [details] [diff] [review]
handlify-16

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

\o/ Great!

::: js/src/builtin/TestingFunctions.cpp
@@ +1085,4 @@
>      for (NonBuiltinScriptFrameIter iter(cx); !iter.done(); ++iter) {
>          if (iter.isFunctionFrame() && iter.compartment() == cx->compartment()) {
> +            id = INT_TO_JSID(stackIndex);
> +            RootedObject callee(cx, iter.callee());

This looks a bit mangled. I guess you wanted the above |RootedValue callee| to be this |RootedObject callee|?

::: js/src/jsapi.h
@@ +2721,5 @@
>  
>  extern JS_PUBLIC_API(JSObject *)
> +JS_DefineObject(JSContext *cx, JS::HandleObject obj, const char *name,
> +                const JSClass *clasp = nullptr, JS::HandleObject proto = JS::NullPtr(),
> +                unsigned attrs = 0);

Yes, let's keep the attrs up front and un-defaulted, like the others.

Comment 120

[Jon Coppeard (:jonco) (PTO until 3rd April)]

Attachment: handlify-16 v2

Updated patch with review comments addressed.

Comment 121

[Jon Coppeard (:jonco) (PTO until 3rd April)]

Comment on attachment 8412497 [details] [diff] [review]
handlify-16 v2

Requesting review for XPConnect changes.

Comment 122

[Jon Coppeard (:jonco) (PTO until 3rd April)]

Comment on attachment 8412497 [details] [diff] [review]
handlify-16 v2

Requesting review for browser changes.

Comment 123

[Bobby Holley (:bholley)]

So, historically passing a nullptr for the getter would mean that the class getter would be used, whereas passing JSPropertyStub meant that no getter would be used. Is that changing now? If so, we need to document that...

Comment 124

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8412497 [details] [diff] [review]
handlify-16 v2

If we're making null a valid value for the JSClass* arg to JS_DefineObject, can you please nix all the objectClass insanity in nsSecurityNameSet::InitializeNameSet and just pass null (which I assume means using the default js::ObjectClassPtr)?  That stuff is just so broken...

>+++ b/content/media/webaudio/AudioBuffer.cpp
>+    JS::Rooted<JSObject*> object(aJSContext, aJSArrays[i]);

Maybe s/object/arrayBufferView/?

You codegen changes might need merging to the stuff jorendorff landed recently.

r=me on the bits outside js/

Comment 125

[Jon Coppeard (:jonco) (PTO until 3rd April)]

(In reply to Bobby Holley (:bholley) from comment #123)
I didn't change the semantics of this (well unless I messed up somewhere), so nullptr should mean the same as it did before, it's just defaulted now.

Comment 126

[Bobby Holley (:bholley)]

Comment on attachment 8412497 [details] [diff] [review]
handlify-16 v2

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

Sorry, I misread the path the first time. r=bholley on the XPConnect bits

Comment 127

[Jon Coppeard (:jonco) (PTO until 3rd April)]

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

Comment 128

[Ed Morley [:emorley]]

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