Adding Cu.cloneInto to the exportHelpers toolset for sandboxes

RESOLVED FIXED in Firefox 30

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: gkrizsanits, Assigned: gkrizsanits)

Tracking

unspecified
mozilla31
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox30 fixed, firefox31 fixed, b2g-v1.4 fixed, b2g-v2.0 fixed)

Details

Attachments

(2 attachments)

Assignee

Description

5 years ago
I think this would be useful for content-scripts too.
Assignee

Updated

5 years ago
Assignee: nobody → gkrizsanits
Assignee

Comment 2

5 years ago
Sigh... I still found some leftover...
Attachment #8396285 - Flags: review?(bobbyholley)
Assignee

Comment 3

5 years ago
Attachment #8396287 - Flags: review?(bobbyholley)
Attachment #8396285 - Flags: review?(bobbyholley) → review+
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).
Attachment #8396287 - Flags: review?(bobbyholley) → review+
Assignee

Comment 5

5 years ago
(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?
(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. :-)
https://hg.mozilla.org/mozilla-central/rev/eb71b9829f50
https://hg.mozilla.org/mozilla-central/rev/7b56ebbcdef7
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Assignee

Comment 9

5 years ago
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
Attachment #8396287 - Flags: approval-mozilla-beta?
Attachment #8396287 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.