Here is minimal testcase: var sb = Components.utils.Sandbox(["http://mozilla.org"]); Components.utils.evalInSandbox("Components.interfaces", sb); /* Exception: Component returned failure code: 0x8007000e (NS_ERROR_OUT_OF_MEMORY) [nsIXPCComponents_Utils.evalInSandbox] undefined:2 */ Expanded principal being introduced by bug 734891.

gabor, here is an SDK branch which uses expanded principals: https://github.com/ochameau/addon-sdk/compare/master...cross-domain-content-script All tests should pass. Only one may fail "testCrossDomainFeatures" which uses multiple domains and I may have miswritten it.

Right. So this all rooted in the security check in XPCWrappedNative::CallMethod (Bug 735281). Now there is two way to fix this. Remove that check finally, which would be the proper solution imo, even though it's a bit risky... Or I can just add some custom hack for nsExpandedPrincipals to nsScriptSecurityManager::LookupPolicy to return some policy instead of throwing because it has no origin/uri. Bobby, what do you think?

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #2) > Right. So this all rooted in the security check in > XPCWrappedNative::CallMethod (Bug 735281). Now there is two way to fix this. > Remove that check finally, which would be the proper solution imo, even > though it's a bit risky... Or I can just add some custom hack for > nsExpandedPrincipals to nsScriptSecurityManager::LookupPolicy to return some > policy instead of throwing because it has no origin/uri. Bobby, what do you > think? How nasty would the hack be? I'd kind of like to limp along with this security check for the time being, given how devastating it would be if content could get unfettered Components access, I'd be in favor of keeping it in (unless there was a pressing need to remove it). I'll hopefully be making it inaccessible to content soon (by moving XBL to its own compartment/scope), at which point we can remove all this stuff.

(In reply to Bobby Holley (:bholley) from comment #3) > How nasty would the hack be? It's nothing really scary I'd say. I attached a quick fix, but it's not very clean from the diff so it's really only this check: if (nsCOMPtr<nsIExpandedPrincipal> exp = do_QueryInterface(aPrincipal)) { // For expanded principals domain origin is not defined so let's just // use the default policy dpolicy = mDefaultPolicy; } before the GetPrincipalDomainOrigin call and all the domain string magic is moved into the else branch. It is done only once per principal btw. So shall we stick to this version then?

Comment on attachment 646667 [details] [diff] [review] Bug 777705 - Default policy for expanded principals Hm, could we instead just return NULL in GetDomain like SystemPrincipal does? Anyway, this getting into caps stuff that I don't at all understand. I think we need mrbkap here.

(In reply to Bobby Holley (:bholley) from comment #6) > Comment on attachment 646667 [details] [diff] [review] > Bug 777705 - Default policy for expanded principals > > Hm, could we instead just return NULL in GetDomain like SystemPrincipal does? We do, that's the problem here. SystemPrincipal does never reach this code (for that we just return sooner allowing everything). GetPrincipalDomainOrigin will return error if cannot find an uri. So for ExpandedPrincipal I could either allow everyting like for system principal (I didn't like that plus I should do a QI in a place that is hotter) or use the default policy (so QI only once then it's cached) > > Anyway, this getting into caps stuff that I don't at all understand. I think > we need mrbkap here. Ok.

Comment on attachment 646667 [details] [diff] [review] Bug 777705 - Default policy for expanded principals This stuff needs to simply die. Do we have a bug on doing that? I wonder if we can kill the site policy stuff with less lead time than the capabilities stuff. I can't even find a use of it in our own code (testing or not) and I have a hard time believing anyone external is using it. I guess I'll send a newsgroup message asking about it. The patch is fine for me. It's unfortunate that you had to add yet another level of nesting here, but given how the code is written, I don't see a way around it.

(In reply to Blake Kaplan (:mrbkap) from comment #8) > Comment on attachment 646667 [details] [diff] [review] > Bug 777705 - Default policy for expanded principals > > This stuff needs to simply die. Do we have a bug on doing that? Sounds great :) > > The patch is fine for me. It's unfortunate that you had to add yet another > level of nesting here, but given how the code is written, I don't see a way > around it. Me neither, except maybe some big refactoring, but getting rid of the whole thing sounds so much more tempting... I might regret saying this but I'm even willing to put some work in this if someone can give me some help. Although it wont happen in this month for sure. Anyway, I added the line that was throwing to the ExpandedPrincipal related test, and fixed the line indentation in this function. (for some reason the char* parsing related magic used 2 spaces while the whole file is using 4). Tested it on try as well just in case https://tbpl.mozilla.org/?tree=Try&rev=87538bb3c5df looks good to me.

Gabor, with this try server build, I get some crashes. That ends up being one of my most complex bug recipe I cooked up so far!! The list of ingredient is quite impressive: - Two sandboxes: - One with just system principal (No idea why it works if you execute its content in scratchpad) - Another one using expanded principal, with just one origin - Use loadSubScript to load the crashing code - Use new Error() which trigger the crash Here is a scratchpad crashing code: // Here window is the toplevel xul one, so equivalent to give systemPrincipal var s1 = Components.utils.Sandbox(window); Components.utils.evalInSandbox("new " + function () { // If you take this 3 lines and put them directly in scratchpad, it works?! Components.utils.import("resource://gre/modules/Services.jsm"); // Use expanded principals here var s2 = Components.utils.Sandbox(["http://mozilla.org"]); // You need to create a file somewhere and reference it here // test.js just contains: new Error('foo'); Services.scriptloader.loadSubScript("file:///C:/test.js", s2); }, s1);

(In reply to Alexandre Poirot (:ochameau) from comment #10) > Gabor, with this try server build, I get some crashes. Seems like an unrelated bug so I just create a new bug for it. Bug 779858. Or is this something this patch has introduced? (shouldn't be...)

Not unrelated. So in this example the CheckXPCPermissions call fails, and when the error string is generated, the code does not expect the URI to be null. So my patch is not ready for sure... I also need to figure out why the CheckXPCPermissions fails in this case.

Comment on attachment 651710 [details] [diff] [review] Default policy for expanded principals Looks good.

https://tbpl.mozilla.org/?tree=Try&rev=4f6fc1845f4d https://hg.mozilla.org/integration/mozilla-inbound/rev/b48a2499872c

Alexandre, Blake, I assume it's OK if we stop exposing Components to expanded principals now, in a webextensions world?

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #17) > *in a webextensions world*? I have no idea, that's a question for webextension folks. Luca, any thoughts?

As far as I know, in the WebExtensions internals we are currently using the expanded principals for the WebExtensions Content Scripts[1] (and we are likely going to use them for the userScripts sandboxes, Bug 1437861, the userScripts will have a role similar to the WebExtensions Content Scripts, but are going to be executed in a less privileged sandbox [2]). In both this cases we don't really need Components to be exposed to these expanded principals (and we also probably don't want it at all to be available there). I'm also adding a needinfo to Kris, in case he may think of any other reasons to keep exposing Components to the expanded principals. [1]: https://searchfox.org/mozilla-central/rev/003262ae12ce937950ffb8d3b0fa520d1cc38bff/toolkit/components/extensions/ExtensionContent.jsm#539 [2]: https://reviewboard.mozilla.org/r/219858/diff/11#index_header

Yes.

Great. I landed patches that remove Components support for expanded principals.