1110546 - Passing null to Sandbox constructor should create a null principal

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

The current state of the art for getting a null principal is:

Cc["@mozilla.org/nullprincipal;1"].createInstance(Ci.nsIPrincipal);

Which is sucky, and encourages people to do things like passing "about:blank" to the sandbox constructor.

The C++ API to creating sandboxes actually does the right thing when you pass it null. We just need to make the Cu API not barf.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Allow null as the principal argument for the sandbox constructor. v1

Comment 2

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=0fc2543dad95

Comment 3

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8535317 [details] [diff] [review]
Allow null as the principal argument for the sandbox constructor. v1

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

Not a big fun of using js Null as an argument in general, but the current alternative is indeed painfully complicated for such a simple thing... Ideally the API would only take the options object and then we could just default the nullprincipal, but we are too late for that change. Another option would be to change the string parsing function and let it parse "null-principal" and "system-principal" (or just "null"/"system") strings, but since system is always easy in practice (we just pass in |this|) I'm fine with this. (little bit still leaning toward the string based approach because I believe passing Null in JS is a lot less conventional than in C++ but I leave this up to you to decide)

Comment 4

[Bobby Holley (:bholley)]

Comment on attachment 8535317 [details] [diff] [review]
Allow null as the principal argument for the sandbox constructor. v1

Irakli, any thoughts on this as an API?

Comment 5

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Comment on attachment 8535317 [details] [diff] [review]
Allow null as the principal argument for the sandbox constructor. v1

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

Seems reasonable to me & I don't expect that to break anything on SDK side. Although I have to say I would much rather have API like `Sandbox({ wantComponents: false, principal: window })` that way if `principal` omitted it can default to null principal (I presume number of passed args can be used to support legacy API as well).

Another option to make gabro happy would be to built upon expanded principal and treat `Sandbox([])` as null principal.

Either way all of the options seem good enough to me.

Comment 6

[Bobby Holley (:bholley)]

Comment on attachment 8535317 [details] [diff] [review]
Allow null as the principal argument for the sandbox constructor. v1

sr timeout. Pushing to inbound.

https://hg.mozilla.org/integration/mozilla-inbound/rev/36f9d3196503

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/36f9d3196503

Comment 8

[Will Bamberg [:wbamberg]]

I've added this, and cleaned up the stuff on specifying principals while I'm about it: https://developer.mozilla.org/en-US/docs/Components.utils.Sandbox#principal.

Gabor, would you mind taking a look?

Comment 9

[Gabor Krizsanits (INACTIVE)]

(In reply to Will Bamberg [:wbamberg] from comment #8)
> I've added this, and cleaned up the stuff on specifying principals while I'm
> about it:
> https://developer.mozilla.org/en-US/docs/Components.utils.Sandbox#principal.
> 
> Gabor, would you mind taking a look?

This looks perfect, thanks Will.