Handlify all unhandlified GCing interfaces in JSAPI

RESOLVED FIXED in mozilla32

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: terrence, Assigned: terrence)

Tracking

(Blocks 1 bug)

Trunk
mozilla32
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(29 attachments, 3 obsolete attachments)

10.32 KB, patch
sfink
: review+
Details | Diff | Splinter Review
23.81 KB, patch
sfink
: review+
Details | Diff | Splinter Review
91.79 KB, patch
sfink
: review+
Details | Diff | Splinter Review
9.17 KB, patch
sfink
: review+
Details | Diff | Splinter Review
38.80 KB, patch
sfink
: review+
Details | Diff | Splinter Review
1.36 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
23.04 KB, patch
jonco
: review+
Details | Diff | Splinter Review
13.61 KB, patch
jonco
: review+
Details | Diff | Splinter Review
10.50 KB, patch
terrence
: review+
bzbarsky
: review+
Details | Diff | Splinter Review
2.86 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
48.21 KB, patch
sfink
: review+
Details | Diff | Splinter Review
15.45 KB, patch
terrence
: review+
bzbarsky
: review+
Details | Diff | Splinter Review
9.09 KB, patch
sfink
: review+
bholley
: review+
Details | Diff | Splinter Review
7.25 KB, patch
sfink
: review+
Details | Diff | Splinter Review
1.34 KB, patch
terrence
: review+
Details | Diff | Splinter Review
6.92 KB, text/plain
Details
20.31 KB, patch
sfink
: review+
smaug
: review+
Details | Diff | Splinter Review
18.29 KB, patch
sfink
: review+
Details | Diff | Splinter Review
34.18 KB, patch
terrence
: review+
bzbarsky
: review+
Details | Diff | Splinter Review
101.23 KB, patch
Waldo
: review+
terrence
: checkin+
Details | Diff | Splinter Review
33.48 KB, patch
terrence
: review+
Details | Diff | Splinter Review
27.54 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
30.00 KB, patch
sfink
: review+
bzbarsky
: review+
Details | Diff | Splinter Review
26.15 KB, patch
sfink
: review+
bholley
: review+
Details | Diff | Splinter Review
29.41 KB, patch
sfink
: review+
Details | Diff | Splinter Review
26.87 KB, patch
terrence
: review+
Details | Diff | Splinter Review
55.21 KB, patch
terrence
: review+
bzbarsky
: review+
Details | Diff | Splinter Review
43.98 KB, patch
sfink
: review+
bholley
: review+
smaug
: review+
Details | Diff | Splinter Review
96.68 KB, patch
jonco
: review+
bholley
: review+
bzbarsky
: review+
Details | Diff | Splinter Review
Assignee

Description

5 years ago
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.
Attachment #8359989 - Flags: review?(sphink)
Blocks: 773686
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 on attachment 8359989 [details] [diff] [review]
handlify_1-v0.diff

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

Prettier!
Attachment #8359989 - Flags: review?(sphink) → review+
Assignee

Comment 3

5 years ago
More of the same.
Attachment #8360060 - Flags: review?(sphink)
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.
Attachment #8360060 - Flags: review?(sphink) → review+
Assignee

Comment 5

5 years ago
If it's any consolation, this was probably more tedious to implement than it will be to review. In any case, sorry.
Attachment #8360156 - Flags: review?(sphink)
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?
Attachment #8360156 - Flags: review?(sphink) → review+
Posted file unhandlifiedGcFunctions.txt (obsolete) —
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.
Here's a patch to handlify the JS_Delete*() APIs.  I guess this will need review by a browser person too.
Attachment #8360487 - Flags: review?(sphink)
Assignee

Comment 9

5 years ago
(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 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?
Attachment #8360487 - Flags: review?(sphink)
Attachment #8360487 - Flags: review?(jschoenick)
Attachment #8360487 - Flags: review+
(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.
Assignee

Updated

5 years ago
Whiteboard: [leave open
Assignee

Comment 13

5 years ago
And time for round 2.
Attachment #8361360 - Flags: review?(sphink)
Assignee

Comment 14

5 years ago
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.
Attachment #8361361 - Flags: review?(bzbarsky)
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 on attachment 8361361 [details] [diff] [review]
handlify_4.1-v0.diff

r=me
Attachment #8361361 - Flags: review?(bzbarsky) → review+
Assignee

Comment 18

5 years ago
(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.
Assignee

Comment 19

5 years ago
I guess I should spread some of these mechanical reviews around.
Attachment #8361429 - Flags: review?(jcoppeard)
Assignee

Comment 20

5 years ago
The low hanging fruit from this section. JS_EvaluateScript is going to be fairly challenging because we don't have a nullable MutableHandle.
Attachment #8361449 - Flags: review?(jcoppeard)
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
Attachment #8361360 - Flags: review?(sphink) → review+
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 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?
Attachment #8361429 - Flags: review?(jcoppeard) → review+
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.
Attachment #8361449 - Flags: review?(jcoppeard) → review+
> 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.  ;)
Here's a patch to do various "has property" related APIs.
Attachment #8361693 - Flags: review?(terrence)
Attachment #8361693 - Flags: review?(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?  ;)
Attachment #8361693 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] from comment #27)
Thanks, yes that is what I wanted :)
Assignee

Comment 29

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

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

r=me
Attachment #8361693 - Flags: review?(terrence) → review+
Assignee

Comment 30

5 years ago
(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.
Sorry, I forgot to qref before asking for review - here are the rest of the changes.
Attachment #8362443 - Flags: review?(bzbarsky)
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.
Attachment #8362988 - Flags: review?(sphink)
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.
Attachment #8362988 - Flags: review?(sphink) → review+
(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)
Flags: needinfo?(sphink)
(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.)
Flags: needinfo?(sphink)
Attachment #8360487 - Flags: review?(jschoenick)
Assignee

Comment 38

5 years ago
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?
Assignee

Comment 39

5 years ago
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.
Attachment #8363385 - Flags: feedback?(jwalden+bmo)
Attachment #8363385 - Flags: feedback?(jorendorff)
Attachment #8363385 - Flags: feedback?(jcoppeard)
Attachment #8363385 - Flags: feedback?(bzbarsky)
Comment on attachment 8362443 [details] [diff] [review]
handlify-has-apis-2

r=me
Attachment #8362443 - Flags: review?(bzbarsky) → review+
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.
Attachment #8363385 - Flags: feedback?(bzbarsky) → feedback+
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!
Attachment #8363385 - Flags: feedback?(jcoppeard) → feedback+
Here's a patch to handlify  JS_Set*() APIs.
Attachment #8364387 - Flags: review?(terrence)
Attachment #8364387 - Flags: review?(bzbarsky)
And here's one to handlify JS_Lookup*() APIs.
Attachment #8364389 - Flags: review?(sphink)
Attachment #8364389 - Flags: review?(bobbyholley+bmo)
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.
Attachment #8364387 - Flags: review?(bzbarsky) → review+
Also, could we please fix the set APIs to not take a mutable handle?  Because that's just messed up.  ;)
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.
Attachment #8363385 - Flags: feedback?(jorendorff) → feedback+
Attachment #8364389 - Flags: review?(bobbyholley+bmo) → review+
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.
Attachment #8364389 - Flags: review?(sphink) → review+
Assignee

Comment 51

5 years ago
A couple of trivial handlifications that I forgot to upload earlier.
Attachment #8364714 - Flags: review?(sphink)
(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.
Attachment #8364714 - Flags: review?(sphink) → review+
Assignee

Comment 54

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

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

r=me
Attachment #8364387 - Flags: review?(terrence) → review+
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.
Flags: needinfo?(jcoppeard)
You're right, I've broken this.  Fix is on the way.
Flags: needinfo?(jcoppeard)
Fix for OOM crash added by previous patch.
Attachment #8365860 - Flags: review?(terrence)
Updated list of functions needing handlification, not including pending patches on this bug.  Down to 72 from 177.
Attachment #8360477 - Attachment is obsolete: true
Posted patch handlify_8Splinter Review
Handlify a couple of misc APIs.
Attachment #8366031 - Flags: review?(sphink)
Attachment #8366031 - Flags: review?(bugs)
Posted patch handlify_9Splinter Review
Handlify some friend APIs.
Attachment #8366033 - Flags: review?(sphink)
Assignee

Comment 65

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

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

D'oh! Good catch! r=me
Attachment #8365860 - Flags: review?(terrence) → review+
Comment hidden (spam)
Attachment #8366031 - Flags: review?(sphink) → review+
Attachment #8366033 - Flags: review?(sphink) → review+
Patch to handlify JS_GetPropery*() APIs plus a couple of others.
Attachment #8367445 - Flags: review?(terrence)
Attachment #8367445 - Flags: review?(bzbarsky)
Assignee

Comment 71

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

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

Seems reasonable to me. r=me
Attachment #8367445 - Flags: review?(terrence) → review+
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.
Attachment #8367445 - Flags: review?(bzbarsky) → review+
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.
Attachment #8363385 - Flags: feedback?(jwalden+bmo) → feedback+
Assignee

Comment 79

5 years ago
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.
Attachment #8363385 - Attachment is obsolete: true
Attachment #8371138 - Flags: review?(jwalden+bmo)
Patch to handlify Call APIs
Attachment #8374894 - Flags: review?(terrence)
Patch to update browser uses of JS Call APIs following handlification.
Attachment #8374895 - Flags: review?(bzbarsky)
Assignee

Comment 82

5 years ago
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!
Attachment #8374894 - Flags: review?(terrence) → review+
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.
Attachment #8374895 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] from comment #83)
Thanks, comments addressed.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d8ceb7308dce
Posted patch handlify_10Splinter Review
Handlify JS_ExecuteScript, JS_EvaluateScript and some other JS APIs
Attachment #8376173 - Flags: review?(sphink)
Attachment #8376173 - Flags: review?(bzbarsky)
Posted patch handlify-11Splinter Review
Handlify some misc friend and debug APIs
Attachment #8376175 - Flags: review?(terrence)
Attachment #8376175 - Attachment description: handlify-misc-friendapis → handlify-11
Attachment #8376175 - Flags: review?(bobbyholley)
Comment on attachment 8376173 [details] [diff] [review]
handlify_10

r=me
Attachment #8376173 - Flags: review?(bzbarsky) → review+
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.
Attachment #8376175 - Flags: review?(bobbyholley) → review+
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!
Attachment #8376173 - Flags: review?(sphink) → review+
Comment on attachment 8376175 [details] [diff] [review]
handlify-11

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

stealing review
Attachment #8376175 - Flags: review?(terrence) → review+

Comment 92

5 years ago
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 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.
Attachment #8371138 - Flags: review?(jwalden+bmo) → review+
> 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?
For that matter, please file a followup bug on the JS_InternStringN bit in the profiler code; that looks pretty wrong to me too.
Posted patch handlify-12Splinter Review
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.
Attachment #8393626 - Flags: review?(sphink)
Comment on attachment 8393626 [details] [diff] [review]
handlify-12

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

Sneaking in the CallArgs-ification. :-)
Attachment #8393626 - Flags: review?(sphink) → review+
Patch to handlify evaluate/EVAL in jit-tests.
Attachment #8398581 - Flags: review?(terrence)
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.
Attachment #8398582 - Flags: review?(terrence)
Posted patch handlify-15Splinter Review
This could be the final patch!  Handlify remaining miscellaneous APIs.
Attachment #8398688 - Flags: review?(sphink)
Assignee

Comment 105

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

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

r=me
Attachment #8398581 - Flags: review?(terrence) → review+
Assignee

Comment 106

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

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

Nice! r=me
Attachment #8398582 - Flags: review?(terrence) → review+
Attachment #8398582 - Flags: review?(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.
Attachment #8398582 - Flags: review?(bzbarsky) → review+
Attachment #8398688 - Flags: review?(sphink) → review+
Comment on attachment 8398688 [details] [diff] [review]
handlify-15

Requesting review for the XPConnect parts.
Attachment #8398688 - Flags: review?(bobbyholley)
Comment on attachment 8398688 [details] [diff] [review]
handlify-15

Requesting review for the browser parts.
Attachment #8398688 - Flags: review?(bugs)
Attachment #8398688 - Flags: review?(bugs) → review+
Comment on attachment 8398688 [details] [diff] [review]
handlify-15

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

r=bholley on the xpconnect bits.
Attachment #8398688 - Flags: review?(bobbyholley) → review+
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
Assignee

Updated

5 years ago
Attachment #8371138 - Flags: checkin+
Posted patch handlify-16 (obsolete) — Splinter Review
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.
Attachment #8411652 - Flags: review?(terrence)
Assignee

Comment 119

5 years ago
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.
Attachment #8411652 - Flags: review?(terrence) → review+
Updated patch with review comments addressed.
Attachment #8411652 - Attachment is obsolete: true
Attachment #8412497 - Flags: review+
Comment on attachment 8412497 [details] [diff] [review]
handlify-16 v2

Requesting review for XPConnect changes.
Attachment #8412497 - Flags: review?(bobbyholley)
Comment on attachment 8412497 [details] [diff] [review]
handlify-16 v2

Requesting review for browser changes.
Attachment #8412497 - Flags: review?(bzbarsky)
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 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/
Attachment #8412497 - Flags: review?(bzbarsky) → review+
(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 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
Attachment #8412497 - Flags: review?(bobbyholley) → review+
Whiteboard: [leave open]
https://hg.mozilla.org/mozilla-central/rev/c9c1e001452b
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.