Right now SpecialPowers will pass appId and isInBrowserElement. We should merge it into OriginAttributes.

WIP patch, still have some orange in try.

fixed more tests.

Comment on attachment 8675626 [details] [diff] [review] Patch. Review of attachment 8675626 [details] [diff] [review]: ----------------------------------------------------------------- ::: testing/specialpowers/content/SpecialPowersObserverAPI.js @@ +322,5 @@ > case "SPPermissionManager": { > let msg = aMessage.json; > > let secMan = Services.scriptSecurityManager; > + let principal = secMan.createCodebasePrincipal(this._getURI(msg.url), msg.originAttributes); The message manager knows how to serialize principals. So why don't we create the principal in the content process (in specialpowersAPI), and send bonafide principal with SPPermissionManager messages? I think that would reduce the complexity of the code in specialPowersAPI.

Thanks, that's a great idea. I should have thought of this. :P I'll also check if it's reasonable to change the SpecialPowers API to take principal as one of the arguments, for example * pushPermissions * testPermission * hasPermission * removePermission * addPermission They have an 'arg', which could be a string, or a plain JS object, or an instance of document, it looks we could replace arg with principal, so the callers could pass document.nodePrincipal, or app.principal, or construct the principal by secMan.createCodebasePrincipal, but if the change is too large, I'll file another bug to change those API entirely.

(In reply to Yoshi Huang[:allstars.chh] from comment #6) > Thanks, that's a great idea. I should have thought of this. :P > > I'll also check if it's reasonable to change the SpecialPowers API to take > principal as one of the arguments, for example > * pushPermissions > * testPermission > * hasPermission > * removePermission > * addPermission > They have an 'arg', which could be a string, or a plain JS object, or an > instance of document, > it looks we could replace arg with principal, so the callers could pass > document.nodePrincipal, or app.principal, or construct the principal by > secMan.createCodebasePrincipal, but if the change is too large, I'll file > another bug to change those API entirely. In general I think this would be great. However, it's worth noting that untrusted content cannot access nsIPrincipal objects without SpecialPowers.wrap. So I think we can support a SpecialPowers.wrap-ed instance of nsIPrincipal, but should also support passing in a document, as well as a manual { url : foo, originAttributes : { ...} } object.

This version passes principal to SpecialPowersObserverAPI.js. I haven't done the passing wrapped version of nsIPrincipal yet. However I am still trying to fix an error when running dom/permission/tests/test_alarms.html. In [1], it will pass SpecialPowers.wrap(iframe).contentDocument but the nodePrincipal of this object seems has some problems From the mochitest [21685] WARNING: 'aRv.Failed()', file /home/allstars/ssd/gecko-dev/dom/ipc/StructuredCloneData.cpp, line 72 JavaScript error: chrome://specialpowers/content/SpecialPowersObserverAPI.js, line 334: NS_ERROR_FAILURE: Failure arg 0 [nsIPermissionManager.testPermissionFromPrincipal] will spend more time on checking this. [1] : https://dxr.mozilla.org/mozilla-central/source/dom/permission/tests/file_framework.js#96

You need to do unwrapIfWrapped on the wrapped nodePrincipal.

(In reply to Bobby Holley (:bholley) from comment #7) > In general I think this would be great. However, it's worth noting that > untrusted content cannot access nsIPrincipal objects without > SpecialPowers.wrap. So I think we can support a SpecialPowers.wrap-ed > instance of nsIPrincipal, but should also support passing in a document, as > well as a manual { url : foo, originAttributes : { ...} } object. Thanks for the comments, If we still need to support passing a 'document' then I think it's easier than passing document.nodePrincipal. For the app cases, I found we only support principal in mozIApplication but not DOMApplication, it looks providing manifestURL is also easier. So I didn't add another support for passing nsIPrincipal in the end. (In reply to Bobby Holley (:bholley) from comment #9) > You need to do unwrapIfWrapped on the wrapped nodePrincipal. Yeah, it's working, thanks again :D

Comment on attachment 8677392 [details] [diff] [review] Part 1: Add originAttributes into SpecialPowers. Review of attachment 8677392 [details] [diff] [review]: ----------------------------------------------------------------- r=me with those comments addressed. ::: dom/browser-element/mochitest/browserElement_DisallowEmbedAppsInOOP.js @@ +28,5 @@ > > document.body.appendChild(iframe); > > + var context = {'url': 'http://example.org', > + 'originAttributes': {'inBrowser': true}}; You don't need to put quotation marks around the key name in a JS object literal. This occurs throughout the patch - please fix it everywhere. ::: dom/cache/test/mochitest/test_cache_orphaned_body.html @@ +46,2 @@ > } > + SpecialPowers.getStorageUsageForURI(document.documentURI, resolve, attrs); It looks like all of this code is just passing the nodePrincipal of |document|. Can you just do that? Same for all the other test_cache* things, and various other places in this patch. Please fix them. ::: testing/mochitest/tests/Harness_sanity/test_SpecialPowersPushAppPermissions.html @@ +95,5 @@ > var appId = gAppsService.getAppLocalIdByManifestURL(gApp.manifestURL); > + is(appId, 0, "appId should become NO_APP_ID"); > + // since gApp is uninstalled, calling SpecialPowers.hasPermission with the > + // app's properties (manifestURL, origin, principal, ... etc) will throw. > + // So we don't need to test hasPermission for the app. Wait, why did this change with this patch? hasPermission was working before, right? ::: testing/specialpowers/components/SpecialPowersObserver.js @@ +235,5 @@ > // so we fake these properties. > msg.permission = { > + principal: { > + appId: permission.principal.appId, > + originAttributes: {appId: permission.principal.appId} Shouldn't the appId item be removed, since it's contained within the originAttributes? ::: testing/specialpowers/content/specialpowersAPI.js @@ +1852,3 @@ > } > > + return [ principal, isSystem ]; Given that we're returning a real principal, I don't think it's necessary to return isSystem anymore, since the caller can just ask that question directly. I filed bug 1218039 to add an ".isSystemPrincipal" to nsIPrincipal.

(In reply to Bobby Holley (:bholley) from comment #14) > ::: > testing/mochitest/tests/Harness_sanity/test_SpecialPowersPushAppPermissions. > html > @@ +95,5 @@ > > var appId = gAppsService.getAppLocalIdByManifestURL(gApp.manifestURL); > > + is(appId, 0, "appId should become NO_APP_ID"); > > + // since gApp is uninstalled, calling SpecialPowers.hasPermission with the > > + // app's properties (manifestURL, origin, principal, ... etc) will throw. > > + // So we don't need to test hasPermission for the app. > > Wait, why did this change with this patch? hasPermission was working before, > right? > There are two problems in the original test case. 1. gApp.origin is 'undefined', which is type of string. 2. The original test case has thrown in the parent side JavaScript error: chrome://specialpowers/content/SpecialPowersObserverAPI.js, line 181: NS_ERROR_MALFORMED_URI: Component returned failure code: 0x804b000a (NS_ERROR_MALFORMED_URI) [nsIIOService.newURI] so inside hasPermisison in specialpowersAPI.js, the line this._sendSyncMessage('SPPermissionManager', msg)[0] actually returns undefined, instead of false. Back to the test case ok(!SpecialPowers.hasPermission('pAppPermission', context), 'pAppPermission should not have permission'); it will evaluate as true, and the test passes. So I remove the check in my patch.

Comment on attachment 8677393 [details] [diff] [review] Part 2: update QuotaManager API fors SpecialPowers. will merge this into Part 1

addressed bobby's comment and rebase on Bug 1218039 https://treeherder.mozilla.org/#/jobs?repo=try&revision=def1dc8485b4

oh, Bug 1196654 landed this morning, but I didn't notice that. will submit a Part 2 patch to rebase on Bug 1196654.

back it out: https://hg.mozilla.org/integration/b2g-inbound/rev/afa51178a780

rebase