1032457 - Allow passing callbacks to exported functions

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

See bug 1031336 comment 7. This seems like a legitimate use-case. I think we should make a default-false option to exportFunction that allows argument cloning to operate with |cloneFunctions| = true.

Comment 1

[Bobby Holley (:bholley)]

This is going to need dev documentation, probably from Will.

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

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 1 - Rename ExportOptions to ExportFunctionOptions. v1

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 2 - Separate out cloning and non-cloning function forwarders. v1

We're going to add functionality to the cloning version, and the non-cloning
version is going away.

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 3 - Stash the clone options for function forwarders on the forwarder itself. v1

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 4 - Implement the |allowCallbacks| parameter to exportFunction. v1

Comment 6

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8449049 [details] [diff] [review]
Part 3 - Stash the clone options for function forwarders on the forwarder itself. v1

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

::: js/xpconnect/src/ExportHelpers.cpp
@@ +273,5 @@
> +    if (!optionsObj)
> +        return false;
> +    js::SetFunctionNativeReserved(funObj, 1, ObjectValue(*optionsObj));
> +
> +

extra newline

::: js/xpconnect/src/xpcprivate.h
@@ +3435,5 @@
> +        JS::RootedValue val(cx);
> +        unsigned attrs = JSPROP_READONLY | JSPROP_PERMANENT;
> +        val = JS::BooleanValue(wrapReflectors);
> +        if (!JS_DefineProperty(cx, obj, "wrapReflectors", val, attrs))
> +            return nullptr;;

double ;;

Comment 7

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8449050 [details] [diff] [review]
Part 4 - Implement the |allowCallbacks| parameter to exportFunction. v1

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

I leave r? on me until we discuss this, because I have some concerns about this.

::: js/xpconnect/src/xpcprivate.h
@@ +3420,3 @@
>  
>      JS::RootedId defineAs;
> +    bool allowCallbacks;

We talked about this feature a few times and I'm really glad that we'll have it finally. But it's not immediately clear to me why do you call it allowCallbacks. While the intention of this bug is clearly to enable callback args, this patch seem to clone every methods as well... Which actually might not be a good idea, like what's the point of a cloned toString method with a |this| = null call?

So, am I overlooking something or do we clone methods too if the flag is on? Is it intentional if so?

::: js/xpconnect/tests/unit/test_exportFunction.js
@@ +54,5 @@
>      mixed = { xrayed: xrayed, xrayed2: xrayed2 };
>      tobecloned = { cloned: "cloned" };
> +    invokedCallback = false;
> +    callback = function() { invokedCallback = true; };
> +    imported(42,tobecloned, native, mixed, callback);

whoops, it's my fault but could you please add a space after "42," while here?

Comment 8

[Bobby Holley (:bholley)]

Comment on attachment 8449050 [details] [diff] [review]
Part 4 - Implement the |allowCallbacks| parameter to exportFunction. v1

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #7)
> We talked about this feature a few times and I'm really glad that we'll have
> it finally. But it's not immediately clear to me why do you call it
> allowCallbacks. While the intention of this bug is clearly to enable
> callback args, this patch seem to clone every methods as well... Which
> actually might not be a good idea, like what's the point of a cloned
> toString method with a |this| = null call?

That is a very excellent point. I will do this a different way.

Comment 9

[Bobby Holley (:bholley)]

Attachment: Part 4 - Implement the |allowCallbacks| parameter to exportFunction. v2

Comment 10

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8449644 [details] [diff] [review]
Part 4 - Implement the |allowCallbacks| parameter to exportFunction. v2

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

::: js/xpconnect/src/ExportHelpers.cpp
@@ +215,1 @@
>      if (!options.Parse())

Could you assert !cloneFunctions here?

Comment 11

[Bobby Holley (:bholley)]

Attachment: Part 3 - Implement the |allowCallbacks| parameter to exportFunction. v3

At this point, I don't think it really makes sense anymore to have
FunctionForwarderOptions inherit StackScopedCloneOptions. I'm combining Part 3
and 4 into a new patch.

Comment 12

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8450331 [details] [diff] [review]
Part 3 - Implement the |allowCallbacks| parameter to exportFunction. v3

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

Yes, I was considering asking you to change that part actually... so I like this version a lot better, thanks :)

Comment 13

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/cd5ec2d1435d
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/704be3e8cd10
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2afd5db295ee

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cd5ec2d1435d
https://hg.mozilla.org/mozilla-central/rev/704be3e8cd10
https://hg.mozilla.org/mozilla-central/rev/2afd5db295ee

Comment 15

[Will Bamberg [:wbamberg]]

I've updated https://developer.mozilla.org/en-US/docs/Components.utils.exportFunction to include allowCallbacks. Let me know if this works for you.

Comment 16

[Bobby Holley (:bholley)]

(In reply to Will Bamberg [:wbamberg] from comment #15)
> I've updated
> https://developer.mozilla.org/en-US/docs/Components.utils.exportFunction to
> include allowCallbacks. Let me know if this works for you.

Looks great! Thanks.