985472 - Adding Cu.cloneInto to the exportHelpers toolset for sandboxes

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Gabor Krizsanits (INACTIVE)]

I think this would be useful for content-scripts too.

Comment 1

[Gabor Krizsanits (INACTIVE)]

https://tbpl.mozilla.org/?tree=Try&rev=0380f5dd0474

Comment 2

[Gabor Krizsanits (INACTIVE)]

Attachment: part1: name fixup in ExportFunction. v1

Sigh... I still found some leftover...

Comment 3

[Gabor Krizsanits (INACTIVE)]

Attachment: part2: cloneInto for sandbox. v1

Comment 4

[Bobby Holley (:bholley)]

Comment on attachment 8396287 [details] [diff] [review]
part2: cloneInto for sandbox. v1

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

r=me with the fixes below.

::: js/xpconnect/src/Sandbox.cpp
@@ +1144,5 @@
>          if (options.wantExportHelpers &&
>              (!JS_DefineFunction(cx, sandbox, "exportFunction", ExportFunction, 3, 0) ||
>               !JS_DefineFunction(cx, sandbox, "evalInWindow", EvalInWindow, 2, 0) ||
>               !JS_DefineFunction(cx, sandbox, "createObjectIn", CreateObjectIn, 2, 0) ||
> +             !JS_DefineFunction(cx, sandbox, "cloneInto", CloneInto, 3, 0) ||

I _think_ that nargs should probably be the minimum number of args, not including the options object. At least that's what the others do here, so we might as well be consistent.

::: js/xpconnect/src/XPCComponents.cpp
@@ +3627,5 @@
> +        return false;
> +
> +    RootedObject scope(aCx, &aScope.toObject());
> +    scope = js::CheckedUnwrap(scope);
> +    NS_ENSURE_TRUE(scope, false);

Given that this can fail now, we need an explicit JS_ReportError here, otherwise this will be misinterpretted as OOM.

Please add a test to make sure we throw something that passes /denied|insecure/.test(exn).

Comment 5

[Gabor Krizsanits (INACTIVE)]

(In reply to Bobby Holley (:bholley) from comment #4)
> I _think_ that nargs should probably be the minimum number of args, not
> including the options object. At least that's what the others do here, so we
> might as well be consistent.

The others are including the options object too if they have any. So either all are consistently good or bad. Where can I double check that?

Comment 6

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #5)
> The others are including the options object too if they have any. So either
> all are consistently good or bad. Where can I double check that?

Oh you're right. In that case just leave it for now. :-)

Comment 7

[Gabor Krizsanits (INACTIVE)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/eb71b9829f50
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7b56ebbcdef7

Comment 8

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

https://hg.mozilla.org/mozilla-central/rev/eb71b9829f50
https://hg.mozilla.org/mozilla-central/rev/7b56ebbcdef7

Comment 9

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8396287 [details] [diff] [review]
part2: cloneInto for sandbox. v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): It's a new feature. Add-on developers have a new set of API to overcome the difficulties that comes with running content-scripts with nsExpandedPrincipal. I was supposed to land this one on ff30 but it seems like it made into 31 only, which I overlooked...

User impact if declined: Add-on developers will have a missing API which make their life harder while migrating to the new world (content-scripts running with nsExpandedPrincipal)

Testing completed (on m-c, etc.): it's on mc and aurora
Risk to taking this patch (and alternatives if risky): it's not risky
String or IDL/UUID changes made by this patch: none

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b955f950df88
https://hg.mozilla.org/releases/mozilla-beta/rev/4c244576343b

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/b955f950df88
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/4c244576343b