943357 - Make name argument on exportFunction optional

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Gabor Krizsanits (INACTIVE)]

Actually the name parameter here should be optional. Irakli seem to prefer to do that bit in JS, and with the Cu.isProxy check and a defineProperty call that should be safe.

Comment 1

[Gabor Krizsanits (INACTIVE)]

The difficulty I run into here is that we cannot create a function object without a string id. So if one is not provided we can use the name of the original (to be exported) function, but if that is an anonymous function for example Cu.exportFunction(function(){...}, contentWindow), then we have to make up something... What do you think?

Comment 2

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #1)
> The difficulty I run into here is that we cannot create a function object
> without a string id. So if one is not provided we can use the name of the
> original (to be exported) function, but if that is an anonymous function for
> example Cu.exportFunction(function(){...}, contentWindow), then we have to
> make up something... What do you think?

Can't we just borrow the old name unconditionally? In the anonymous function case that'll be "", but that should be fine, I'd think.

Comment 3

[Gabor Krizsanits (INACTIVE)]

Attachment: name argument of exportFunction should be optional

(In reply to Bobby Holley (:bholley) from comment #2)
> Can't we just borrow the old name unconditionally? In the anonymous function
> case that'll be "", but that should be fine, I'd think.

Almost... It seems that JS API is not very great here and this simple task gets a bit more complex/messy than it should be really... Please tell me that I'm missing something obvious and this can be done a lot nicer/simpler than this.

Comment 4

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8339323 [details] [diff] [review]
name argument of exportFunction should be optional

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

Eh sorry wrong patch...

Comment 5

[Gabor Krizsanits (INACTIVE)]

Attachment: name argument of exportFunction should be optional. v2

Not that this version is a million time better... so my problem is that I get back the id as a JSString* and it's null for anonym functions. And ofc I need a jsvalue to cast a jsobject to a jsfunction to start with sice there is no direct API. So it's a lot of dancing I wish I didn't have to do for something this simple.

Comment 6

[Bobby Holley (:bholley)]

Comment on attachment 8339350 [details] [diff] [review]
name argument of exportFunction should be optional. v2

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

::: js/xpconnect/idl/xpccomponents.idl
@@ +342,5 @@
>       * be set on the target scope, with the forwarder function as
>       * the value.
>       */
>      [implicit_jscontext]
> +    jsval exportFunction(in jsval vfunction, in jsval vscope, [optional] in jsval vname);

I think this API is likely enough to grow features that we should use an options object for this.

::: js/xpconnect/src/Sandbox.cpp
@@ +287,5 @@
>              JS_ReportError(cx, "First argument must be a function");
>              return false;
>          }
>  
> +        if (!define) {

Can we do all this work above with the declaration of funName?

Something like:

RootedString funName(cx);
if (define) {
  funName.set(vname.toString());
} else {
  // this stuff
}

This has the added advantage of not needing to enter the compartment.

@@ +292,5 @@
> +            // If there weren't any function name specified, let's just use the
> +            // id of the one to be imported for the new one.
> +            JSAutoCompartment ac(cx, funObj);
> +            RootedValue funVal(cx, ObjectValue(*funObj));
> +            JSFunction *fun = JS_ValueToFunction(cx, funVal);

You can just call JS_GetObjectFunction.

@@ +296,5 @@
> +            JSFunction *fun = JS_ValueToFunction(cx, funVal);
> +            funName = JS_GetFunctionId(fun);
> +            if (!funName) {
> +                funName = JS_InternString(cx, "");
> +            }

Add a comment here indicating that both JS_GetFunctionId and JS_InternString return atoms, so we don't need to wrap the resulting string when changing compartments.

Comment 7

[Gabor Krizsanits (INACTIVE)]

(In reply to Bobby Holley (:bholley) from comment #6)
> I think this API is likely enough to grow features that we should use an
> options object for this.

Yes. Actually for evalInWindow we will need some options too, like evalInSandbox has. So I think I will just do that. Better do it now than later.

Comment 8

[Gabor Krizsanits (INACTIVE)]

Attachment: name argument of exportFunction should be optional. v3

Comment 9

[Bobby Holley (:bholley)]

Comment on attachment 8341103 [details] [diff] [review]
name argument of exportFunction should be optional. v3

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

::: js/xpconnect/idl/xpccomponents.idl
@@ +338,5 @@
>       * algorithm.
>       * The return value is the new forwarder function, wrapped into
>       * the caller's compartment.
> +     * The 3rd argument is an optional options object:
> +     * - "defeineAs:" the name of the property that will

This should be |defineAs|, and kill the quotes.

@@ +339,5 @@
>       * The return value is the new forwarder function, wrapped into
>       * the caller's compartment.
> +     * The 3rd argument is an optional options object:
> +     * - "defeineAs:" the name of the property that will
> +     *                be set on the target scope, with 

whitespace error.

::: js/xpconnect/src/Sandbox.cpp
@@ +285,5 @@
>  
> +        RootedId id(cx, options.defineAs);
> +        if (JSID_IS_VOID(id)) {
> +            // If there weren't any function name specified, let's just use the
> +            // id of the one to be imported for the new one.

// If there was no function name specified, copy the name from the function being imported.

@@ +289,5 @@
> +            // id of the one to be imported for the new one.
> +            JSFunction *fun = JS_GetObjectFunction(funObj);
> +            RootedString funName(cx, JS_GetFunctionId(fun));
> +            if (!funName) {
> +                // In case it is null, we want to use empty string for the id.

I think we can kill the comment, which will let us get rid of the braces.

@@ +298,5 @@
> +            if (!JS_ValueToId(cx, vname, id.address()))
> +                return false;
> +        }
> +        MOZ_ASSERT(JSID_IS_STRING(id),
> +                   "String id is needed for creating a new function object");

I don't think the explanation adds much here. Let's just do MOZ_ASSERT(JSID_IS_STRING(id));

@@ +315,5 @@
>          }
>  
>          // We have the forwarder function in the target compartment, now
> +        // we have to add it to the target scope as a property if defineAs
> +        // was set.

// We have the forwarder function in the target compartment. If
// defineAs was set, we also need to define it as a property on
// the target.

@@ +317,5 @@
>          // We have the forwarder function in the target compartment, now
> +        // we have to add it to the target scope as a property if defineAs
> +        // was set.
> +        if (!JSID_IS_VOID(options.defineAs) &&
> +            !JS_DefinePropertyById(cx, targetScope, id, rval,

I tend to think that mixing first-class conditional logic with error-checking is confusing. Also, multi-line conditionals require braces, even if the consequent is a single line.

Please do:

if (!JSID_IS_VOID(options.defineAs)) {
    if (!JS_DefinePropertyById(cx, targetScope, id, rval,
                               JS_PropertyStub, JS_StrictPropertyStub,
                               JSPROP_ENUMERATE)
    {
        return false;
    }
}

@@ +334,5 @@
>  /*
>   * Expected type of the arguments and the return value:
>   * function exportFunction(function funToExport,
>   *                         object targetScope,
> + *                         [optional] string name)

Er, I think we should expose the options object to sandboxes as well. It seems dangerous to let the APIs diverge.

Comment 10

[Gabor Krizsanits (INACTIVE)]

Attachment: name argument of exportFunction should be an optional options object instead. v4

(In reply to Bobby Holley (:bholley) from comment #9)
> @@ +334,5 @@
> >  /*
> >   * Expected type of the arguments and the return value:
> >   * function exportFunction(function funToExport,
> >   *                         object targetScope,
> > + *                         [optional] string name)
> 
> Er, I think we should expose the options object to sandboxes as well. It
> seems dangerous to let the APIs diverge.

The API is not diverged, just forgot to update this comment. Was this the reason for the r-? Because I see only nits... Anyway flagging you for r? again, let me know if I was missing something.

Comment 11

[Bobby Holley (:bholley)]

Comment on attachment 8341636 [details] [diff] [review]
name argument of exportFunction should be an optional options object instead. v4

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

r=bholley, but I'm kind of unhappy about the level of nits here, especially the two pieces that were already mentioned in the previous review. This is something that I think is very important, and it makes me uncomfortable about doing "r=bholley with nits", because I have less confidence that the final patch is going to be something I'd approve of.

I totally empathize with the difficulties of being a non-native speaker in a given language. But I think it's something that you can compensate for (to some degree) with more attention (especially punctuation and spelling). And most of the stylistic issues are not grounded in natural language.

XPConnect is a hard module to work in stylistically, because of the mix between SpiderMonkey and Gecko. There are a lot of situations where we need to crib from the style of nearby code. It's a tough line to walk, but I'm not willing to just throw up my hands and say "style doesn't matter". It just means we have to think even harder about all the various choices we make.

In general, we should use SpiderMonkey style unless we have a clear reason not to. I don't want you to feel like I'm enforcing arbitrary and whimsical stylistic choices. If there are ever things you have questions about, we should talk about them and write the results up somewhere. But I'd like you to be more critical and deliberate about style, so that we can make this module as pretty and clean as possible in the face of all the ugly work that it's doing. ;-)

::: js/xpconnect/idl/xpccomponents.idl
@@ +338,5 @@
>       * algorithm.
>       * The return value is the new forwarder function, wrapped into
>       * the caller's compartment.
> +     * The 3rd argument is an optional options object:
> +     * - |defeineAs|: the name of the property that will

I didn't mean to put it in bars. I meant that it was misspelled. It should look like:

* - defineAs: the name of the property that...

::: js/xpconnect/src/Sandbox.cpp
@@ +285,5 @@
>  
> +        RootedId id(cx, options.defineAs);
> +        if (JSID_IS_VOID(id)) {
> +            // If there wasn't any function name specified,
> +            // copy the name from the function being imported..

Extra trailing period.

@@ +301,1 @@
>          // The function forwarder will live in the target compartment. Since

There should be a newline between the MOZ_ASSERT and the comment block.

@@ +317,5 @@
> +        if (!JSID_IS_VOID(options.defineAs)) {
> +            if (!JS_DefinePropertyById(cx, targetScope, id, rval,
> +                                       JS_PropertyStub, JS_StrictPropertyStub,
> +                                       JSPROP_ENUMERATE))
> +                return false;

As noted in comment 9, this requires braces.

::: js/xpconnect/tests/unit/test_exportFunction.js
@@ +31,5 @@
>      this.checkIfCalled = function() {
>        do_check_true(wasCalled);
>        wasCalled = false;
>      }
> +    exportFunction(funToExport, subsb, {defineAs: "imported"});

There should be internal space padding for object literals:

exportFunction(funToExport, subsb, { defineAs: "imported" });

This also applies to all the rest of the added lines below.

@@ +63,5 @@
>    // Exporting should throw if princpal of the source sandbox does
>    // not subsume the principal of the target.
>    Cu.evalInSandbox("(" + function() {
>      try{
> +      exportFunction(function(){}, this.xorigsb, {defeineAs: "denied"});

defineAs is misspelled again, and here it isn't just a comment :-(

Also, there should be a space between the function arguments and the braces:

function() {}

@@ +88,5 @@
>  
>    // exportFunction and createObjectIn should be available from Cu too.
>    var newContentObject = Cu.createObjectIn(subsb, {defineAs:"importedObject2"});
>    var wasCalled = false;
> +  Cu.exportFunction(function(arg){wasCalled = arg.wasCalled;}, newContentObject, {defineAs:"privMethod"});

function(arg) { wasCalled = arg.wasCalled; }

Comment 12

[Gabor Krizsanits (INACTIVE)]

Sorry about the delay here, but for some reason I took this comment too personally, and needed some time to think and re-read it with clear head. You are touching some topics that I think does not really belong to this bug but very important for me, I think in general those should be discussed in a different channel next time. Just to make it clear, I do understand that as a module owner you are taking these nits seriously. I kind of felt like you quit the review because you didn't feel the patch "review ready". With a valid reason. (Which by the way made me think a lot about my work flow...) So with that question I simply wanted to know if there is any high level changes as well I should take care of while polishing it.

(In reply to Bobby Holley (:bholley) from comment #11)
> 
> I totally empathize with the difficulties of being a non-native speaker in a
> given language. But I think it's something that you can compensate for (to
> some degree) with more attention (especially punctuation and spelling). And
> most of the stylistic issues are not grounded in natural language.

As someone who is making 2 typos per sentence as an average, I don't think that
this is the best way I can compensate. I wish it were, but it really does not
seem to work out. In general I spend more time on checking my patches in the hope
of catching these type of mistakes than I'm willing to admit. I'm kind of shamed
to admit but I'm really bad at this. What I usually do that after an r+ I do two
rounds of checking, the second one happens always in the morning, when my eyes and
mind is fresh. I'm thinking how could I script this process, but I'm afraid if I
go down that route it only will give me a false positive comfort feeling (since
I doubt a script would be super efficient in catching some of these things)
So what I do instead, that I try picking up more refactoring patches, which
a., gives me a good practice ground to get better in this
b., an attempt to compensate you for all the fixes you have to make on my patches

As it seems to generate quite some frustration on your side, I guess I should make
some more changes and put the focus on catching these things earlier than the final
stage. Which is a lot more time consuming on my side (had to do it at each
stage) but will hopefully generate less frustration.

> 
> In general, we should use SpiderMonkey style unless we have a clear reason
> not to. I don't want you to feel like I'm enforcing arbitrary and whimsical
> stylistic choices. If there are ever things you have questions about, we
> should talk about them and write the results up somewhere. But I'd like you
> to be more critical and deliberate about style, so that we can make this
> module as pretty and clean as possible in the face of all the ugly work that
> it's doing. ;-)

Only thing I would like to make clear, that this is my biggest weak spot, and
although result my suggest otherwise in some cases, I'm far from not caring
about leaving behind a clean code. Since I have in many cases different opinion
about these things, I think the best way to keep the code consistent is to instead
of fighting my way through, accepting your approach. This might give you the false
impression sometimes that I don't care. But that is not the case.
Many cases for the same reason it's tempting to throw up my hand and file an unpolished
patch for feedback, instead of try and read your mind about many things. And often I don't
put in enough effort to clean up those patches, since I know that I will have to redo much
part of it. But I think this approach as a side effect adds up on your side, and generates
frustration on your side, and also it helps false assumptions to be made about me.

I have a lot more thought about this topic but I will stop now flaming this bug let's
get back to the patch.

> > +     * - |defeineAs|: the name of the property that will
> 
> I didn't mean to put it in bars. I meant that it was misspelled.

Speaking of how good I'm at spotting typos...

> @@ +317,5 @@
> > +        if (!JSID_IS_VOID(options.defineAs)) {
> > +            if (!JS_DefinePropertyById(cx, targetScope, id, rval,
> > +                                       JS_PropertyStub, JS_StrictPropertyStub,
> > +                                       JSPROP_ENUMERATE))
> > +                return false;
> 
> As noted in comment 9, this requires braces.

Actually what it says is: "I tend to think that mixing first-class conditional logic with error-checking is confusing. Also, multi-line conditionals require braces, even if the consequent is a single line."
I interpreted it as I should break the original one if into two if's for better separation. Since I did that it is no longer a multi line condition hence I don't need the braces there. What do I get wrong?

Comment 13

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #12)
> Sorry about the delay here, but for some reason I took this comment too
> personally

I understand entirely. I felt really bad about comment 11 after I posted it. You're one the best and most dedicated XPConnect hackers, and I think I came off as pretty ungrateful. I'm really sorry about that, and hope you'll forgive me. :-(

> You are touching some topics that I think does not really belong to this bug
> but very important for me, I think in general those should be discussed in a
> different channel next time.

Yeah, agreed. Very poor forum choice on my part.

> As someone who is making 2 typos per sentence as an average, I don't think
> that
> this is the best way I can compensate. I wish it were, but it really does not
> seem to work out. In general I spend more time on checking my patches in the
> hope
> of catching these type of mistakes than I'm willing to admit.

Yeah, it was pretty presumptuous of me to assume that what I was seeing was the result of inattentiveness.

> I'm thinking how could I script this process, but I'm afraid
> if I
> go down that route it only will give me a false positive comfort feeling
> (since
> I doubt a script would be super efficient in catching some of these things)

I don't think a scripting approach is very feasible TBH. Though it's been done: http://www.johnkeiser.com/cgi-bin/jst-review-cgi.pl ;-)

> So what I do instead, that I try picking up more refactoring patches, which
> a., gives me a good practice ground to get better in this
> b., an attempt to compensate you for all the fixes you have to make on my
> patches

I really, _really_ appreciate your work on refactoring patches.

> As it seems to generate quite some frustration on your side, I guess I
> should make
> some more changes and put the focus on catching these things earlier than
> the final
> stage. Which is a lot more time consuming on my side (had to do it at each
> stage) but will hopefully generate less frustration.

Which stages are you referring to? The various stages of development, or the various iterations of patches that go up for review? I can only speak for my workflow, but here are my thoughts:

During development, I put myself in the mindset that everything I type is going to end up in the tree. So I write (and read over!) comments as I go, and think very carefully about each style decision I make. I find that I never really go back and fix these sorts of things, and so I try to just assume that I never will.

When addressing review feedback, I methodically go through each comment and check it off. I recognize that you probably do this too, and the repeat issues were mostly an issue of miscommunication in the feedback. The best I can offer for that is an attempt to understand _why_ the reviewer is asking for what they are. If it feels like you need to read the reviewer's mind, then it's really not your fault, and you should bring that up. Ideally, we should have crystal-clear guidelines for anything.

> Only thing I would like to make clear, that this is my biggest weak spot, and
> although result my suggest otherwise in some cases, I'm far from not caring
> about leaving behind a clean code. Since I have in many cases different
> opinion
> about these things, I think the best way to keep the code consistent is to
> instead
> of fighting my way through, accepting your approach.

I think we should spend more time discussing these issues. Since you're a peer, it's pretty important to me that we're very much on the same page stylistically, so that you can apply the exact same standard when you review patches (from me and from others). Some issues are pretty hard to codify, but a lot of the mechanical ones can actually be expressed in a style guide. Most of our style influence comes from SpiderMonkey, but we should probably make a separate XPConnect style guide for any of the exceptions.

Basically for each item of my review feedback, I would like you to ask "would I say the same thing if I were reviewing this?". If you can't think of a reason why you would, then we should talk about it, and write down the result of our discussion somewhere.

Spelling and grammar in comments is kind of a harder thing. I have the luxury of being a native English speaker, and it's not at all fair for me to expect you to write like one. I'd just suggest being as careful as you can about simple mechanics (capitalization, punctuation, and spelling), and do your best on sentence construction. Reading things out loud helps (I do this when writing love letters in French, and it helps a lot, heh).

> Many cases for the same reason it's tempting to throw up my hand and file an
> unpolished
> patch for feedback, instead of try and read your mind about many things. And
> often I don't
> put in enough effort to clean up those patches, since I know that I will
> have to redo much
> part of it.

As noted earlier, I think this isn't the right approach - it's ten times harder to go back and clean up a patch than it is to write it cleanly the first time. I think we should always be writing patches that we _hope_ could land as-is (the exception would be XXX comments meant for the reviewer). What do you think?

> and also it helps false assumptions to be made
> about me.

Again, I really apologize for making those. And there were a lot of things that were implied in comment 11 that I didn't actually mean. For example, the part about "throwing up your hands" wasn't directed to you specifically, but rather at the dominant attitude when I came to the module that proper style in XPConnect was hopeless. I refused to accept that, and even wrote a custom python tool to restyle XPConnect to something more sane (bug 688012). So this is something I'm pretty invested in. ;-)

> > @@ +317,5 @@
> > > +        if (!JSID_IS_VOID(options.defineAs)) {
> > > +            if (!JS_DefinePropertyById(cx, targetScope, id, rval,
> > > +                                       JS_PropertyStub, JS_StrictPropertyStub,
> > > +                                       JSPROP_ENUMERATE))
> > > +                return false;
> > 
> > As noted in comment 9, this requires braces.
> 
> Actually what it says is: "I tend to think that mixing first-class
> conditional logic with error-checking is confusing. Also, multi-line
> conditionals require braces, even if the consequent is a single line."
> I interpreted it as I should break the original one if into two if's for
> better separation.

Yep, that part is totally right.

> Since I did that it is no longer a multi line condition
> hence I don't need the braces there. What do I get wrong?

But the second one still _is_ a multi-line conditional, because of the newlines in the argument list for JS_DefinePropertyById. I would define a multiline conditional as any conditional statement where the opening paren is not on the same line as the closing paren. See [1]. And in this case, note that the opening brace can go on the last line of the conditional, because of the "visual separation". Does that make sense?

Let me know your thoughts on all of this. I'm happy to do it in whatever forum you want - this bug, email, IRC, vidyo, or in-person in January. And again, thanks for all your hard work. :-)

[1] https://wiki.mozilla.org/JavaScript:SpiderMonkey:C_Coding_Style#Other_whitespace

Comment 14

[Gabor Krizsanits (INACTIVE)]

(In reply to Bobby Holley (:bholley) from comment #13)
> I understand entirely. I felt really bad about comment 11 after I posted it.
> You're one the best and most dedicated XPConnect hackers, and I think I came
> off as pretty ungrateful. I'm really sorry about that, and hope you'll
> forgive me. :-(

No worries, and thanks for this positive feedback.

> done: http://www.johnkeiser.com/cgi-bin/jst-review-cgi.pl ;-)

I will take a look at this, maybe it helps... Probably won't hurt :)

> Let me know your thoughts on all of this. I'm happy to do it in whatever
> forum you want - this bug, email, IRC, vidyo, or in-person in January. And
> again, thanks for all your hard work. :-)

I think the best if we put this topic off from this thread and I'll answer in an email
instead (sometime during this week). In short I made the same conclusion as you that we
should discuss these type of things more often. Also, I'm very open in general for
constructive critics so do not hesitate to point out possible flaws about my work in
general. Continuous improvement what our profession is all about after all.

Comment 15

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #14)
> > done: http://www.johnkeiser.com/cgi-bin/jst-review-cgi.pl ;-)
> 
> I will take a look at this, maybe it helps... Probably won't hurt :)

Well, it's designed to do reviews à la jst circa 2002, so it's likely to insist on a slightly-antiquated Gecko style, which doesn't really match up with what we use in XPConnect at all. :P

Comment 16

[Gabor Krizsanits (INACTIVE)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/80d1a749ea68

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/80d1a749ea68