Back in Bug 774585 we implemented GetCodeBasePrincipal to get the correct data jar on b2g. For the new security model we will update the origin in nsIPrincipal in Bug 1163254, so we should use the origin attribute instead of using appId, isBrowser.

In bug 1165162, I'm adding a method on nsIScriptSecurityManager that takes an OriginAttributes dictionary. Hopefully we can replace all the get*CodebasePrincipal methods with calls to that method.

In ipc/glue/BackgroundUtils.cpp it will call getSimpleCodebasePrincipal/getAppCodebasePrincipal base on the appId from ContentPrincipalInfo. And appId should be removed in Bug 1167100, so I'm making this bug blocks Bug 1167100.

Hi Bobby You only deprecated getCodebasePrincipal in Bug 1165162. But in this bug I have to replace - getSimpleCodePrincipal - getAppCodebasePrincipal - getLoadContextCodebasePrincipal - getDocShellCodebasePrincipal - getNoAppCodebasePrincipal - getCodebasePrincipal with createCodebasePrincipal. Is this correct? Thanks

Also, for using createCodePrincipal on Cpp side, it needs a JS Context. How would you like to proceed? Callers have to use JSContext? Or like BasePrincipal we add a Cast to convert it to nsScriptSecurityManager? Thanks

(In reply to Yoshi Huang[:allstars.chh] from comment #3) > But in this bug I have to replace > - getSimpleCodePrincipal Theoretically this should go away entirely, but there are still some scattered uses here. This principal will assert when used in a security check. Don't worry about it in this bug. > - getAppCodebasePrincipal > - getNoAppCodebasePrincipal > - getCodebasePrincipal These three should all be deprecated and replaced by usage of createCodebasePrincipal (getCodebasePrincipal is deprecated already). However, we probably shouldn't remove the APIs, since there may be addons depending on them. > - getLoadContextCodebasePrincipal > - getDocShellCodebasePrincipal These are separate really. Let's deal with them in bug 1165466. (In reply to Yoshi Huang[:allstars.chh] from comment #4) > Also, for using createCodePrincipal on Cpp side, it needs a JS Context. > How would you like to proceed? C++ consumers should invoke BasePrincipal::CreateCodebasePrincipal directly, which doesn't require a JSContext.

fixed some typos.

Comment on attachment 8649226 [details] [diff] [review] Part 1: replace getAppCodebasePrincipal Review of attachment 8649226 [details] [diff] [review]: ----------------------------------------------------------------- r=me with those comments addressed - there's a lot of them though, so please make sure to get them all! ;-) ::: b2g/components/AboutServiceWorkers.jsm @@ +158,1 @@ > Services.io.newURI(message.principal.origin, null, null), Hm, is this some sort of fake principal, rather than an nsIPrincipal? If it's an nsIPrincipal, then this .origin is wrong (we'd want .originNoSuffix). But in that case, we could just do: |let principal = message.principal|. But again, I'm not sure if this is a real principal or not. ::: b2g/components/ContentPermissionPrompt.js @@ +206,5 @@ > let notDenyAppPrincipal = function(type) { > let url = Services.io.newURI(app.origin, null, null); > + let principal = secMan.createCodebasePrincipal(url, > + {appId: request.principal.appId, > + inBrowser: false}); Same here: can we just do |let principal = request.principal;|? Also, not need to pass inBrowser: false, since that's the default. ::: docshell/base/nsDocShell.cpp @@ +9342,5 @@ > NS_ENSURE_SUCCESS(rv, rv); > + > + OriginAttributes attrs(appId, isInBrowserElement); > + nsCOMPtr<nsIPrincipal> prin = > + BasePrincipal::CreateCodebasePrincipal(aReferrer, attrs); Once we fix bug 1165466, we should make sure to just pass mOriginAttributes here directly. Can you file a followup bug to do that? ::: dom/apps/AppsUtils.jsm @@ +78,3 @@ > Services.io.newURI(this.origin, null, null), > + {appId: this.localId, > + inBrowser: false}); No need for inBrowser: false. ::: dom/apps/OfflineCacheInstaller.jsm @@ +229,5 @@ > return; > > + let attrs = {appId: aApp.localId, inBrowser: false}; > + let principal = Services.scriptSecurityManager.createCodebasePrincipal( > + app.origin, attrs); Here too. Once you do that, I think you can get rid of the intermediate value |attrs|, and just inline { appId : ... } into the call. There are various other places where this happens in this patch - I'll stop commenting on them, but please fix them all. ::: dom/base/nsGlobalWindow.cpp @@ +8588,5 @@ > MOZ_ASSERT(principal); > > uint32_t appId = principal->GetAppId(); > bool isInBrowser = principal->GetIsInBrowserElement(); > + OriginAttributes attrs(appId, isInBrowser); Please don't do this explicitly. Instead, do BasePrincipal::Cast(principal)->OriginAttributesRef(). Remember, the goal here is to try to remove as much explicit handling of appId/inBrowser as possible, so that we can easily add new ones. ::: dom/browser-element/BrowserElementParent.js @@ +836,5 @@ > // This simply returns null if there is no principal available > // for the requested uri. This is an acceptable fallback when > // calling newChannelFromURI2. > + let attrs = {appId: this._frameLoader.loadContext.appId, > + inBrowser: this._frameLoader.loadContext.isInBrowserElement}; This should pull the attributes directly from the loadContext, using the originAttributes getter we're introducing in bug 1165466. If you want to land this bug first, please file a followup bug to fix this once bug 1165466 lands. ::: dom/ipc/TabChild.cpp @@ +14,5 @@ > #endif > #include "Layers.h" > #include "ContentChild.h" > #include "TabParent.h" > +#ifdef MOZ_WIDGET_GONK No need to #ifdef this. ::: dom/permission/PermissionSettings.js @@ +36,5 @@ > PermissionSettings.prototype = { > get: function get(aPermName, aManifestURL, aOrigin, aBrowserFlag) { > debug("Get called with: " + aPermName + ", " + aManifestURL + ", " + aOrigin + ", " + aBrowserFlag); > let uri = Services.io.newURI(aOrigin, null, null); > let appID = appsService.getAppLocalIdByManifestURL(aManifestURL); So, we're going to need to update all of this code to operate properly for signedPkg, right? Please file a blocker against bug 1163254 so that we remember to do this (both this stuff and the stuff in PermissionSettings.jsm). ::: dom/quota/QuotaManager.cpp @@ +3411,5 @@ > uint32_t aAppId, > bool aInMozBrowser, > nsACString* aGroup, > nsACString* aOrigin, > bool* aIsApp) This method should take an OriginAttributes*, and fill that, rather than filling the props individually... @@ +5334,5 @@ > if (originProps.mAppId == kUnknownAppId) { > rv = secMan->GetSimpleCodebasePrincipal(uri, > getter_AddRefs(principal)); > } else { > + OriginAttributes attrs(originProps.mAppId, originProps.mInMozBrowser); And OriginProps should contain an OriginAttributes object. Please file a blocker against bug 1179985 to fix this? ::: extensions/cookie/nsPermissionManager.cpp @@ +124,5 @@ > } > > > nsresult > GetPrincipal(nsIURI* aURI, uint32_t aAppId, bool aIsInBrowserElement, nsIPrincipal** aPrincipal) Please make a note in bug 1165267 that this code is going to need to be fixed up. ::: ipc/glue/BackgroundUtils.cpp @@ +80,5 @@ > if (info.appId() == nsIScriptSecurityManager::UNKNOWN_APP_ID) { > rv = secMan->GetSimpleCodebasePrincipal(uri, getter_AddRefs(principal)); > } else { > + OriginAttributes attrs(info.appId(), info.isInBrowserElement()); > + principal = BasePrincipal::CreateCodebasePrincipal(uri, attrs); This will get fixed up in bug 1167100, right? ::: netwerk/cookie/CookieServiceParent.cpp @@ +27,5 @@ > // Ignore failures from this function, as they only affect whether we do or > // don't show a dialog box in private browsing mode if the user sets a pref. > void > CreateDummyChannel(nsIURI* aHostURI, uint32_t aAppId, bool aInMozBrowser, > bool aIsPrivate, nsIChannel **aChannel) Will this get fixed up in bug 1165267? ::: netwerk/protocol/http/HttpChannelParent.cpp @@ +446,5 @@ > if (setChooseApplicationCache) { > bool inBrowser = false; > if (mLoadContext) { > mLoadContext->GetIsInBrowserElement(&inBrowser); > } As noted elsewhere, we should be pulling these off the LoadContext once that's possible. Please file a followup against bug 1165466 (or include it in the other followup). ::: services/mobileid/MobileIdentityManager.jsm @@ +898,5 @@ > log.debug("getMobileIdAssertion ${}", aPrincipal); > > let uri = Services.io.newURI(aPrincipal.origin, null, null); > + let attrs = aPrincipal.originAttributes; > + let principal = securityManager.createCodebasePrincipal(uri, attrs); I'm confused. If this is a real nsIPrincipal, then the .newURI call is broken, because it should be passing in .originNoSuffix. And if that's the case, then we should just be able to use aPrincipal directly, right? ::: testing/specialpowers/content/SpecialPowersObserverAPI.js @@ +313,5 @@ > case "SPPermissionManager": { > let msg = aMessage.json; > > let secMan = Services.scriptSecurityManager; > + let attrs = {appId: msg.appId, inBrowser: msg.isInBrowserElement}; File a followup bug to fix up aMessage so that it sends originAttributes directly? ::: uriloader/prefetch/OfflineCacheUpdateParent.cpp @@ +92,5 @@ > return NS_ERROR_FAILURE; > > bool offlinePermissionAllowed = false; > > + OriginAttributes attrs(mAppId, mIsInBrowserElement); We need to file a bug to make OfflineCacheUpdateParent hold an OriginAttributes rather than mAppId etc, right? Please do that if we don't already have one.

Comment on attachment 8649228 [details] [diff] [review] Part 3: replace getCodebasePrincipal Review of attachment 8649228 [details] [diff] [review]: ----------------------------------------------------------------- ::: addon-sdk/source/lib/sdk/indexed-db.js @@ +34,5 @@ > > +let ssm = Cc["@mozilla.org/scriptsecuritymanager;1"] > + .getService(Ci.nsIScriptSecurityManager); > +let attrs = {appId: ssm.NO_APP_ID, inBrowser: false}; > +let principal = ssm.createCodebasePrincipal(principaluri, attrs); You don't need to pass |attrs| at all here. Just pass {}. ::: toolkit/devtools/server/actors/storage.js @@ +1333,5 @@ > + let attrs = { > + appId: Services.scriptSecurityManager.NO_APP_ID, > + inBrowser: false > + }; > + principal = Services.scriptSecurityManager.createCodebasePrincipal(uri, attrs); Same here.

Comment on attachment 8649701 [details] [diff] [review] Part 2: replace getNoAppCodebasePrincipal. Review of attachment 8649701 [details] [diff] [review]: ----------------------------------------------------------------- You can pass in {} for almost all of these, and the default will do the right thing. Given that this patch is huge, I'd rather you fix that before I read all the way through it. :-)

Comment on attachment 8649226 [details] [diff] [review] Part 1: replace getAppCodebasePrincipal Review of attachment 8649226 [details] [diff] [review]: ----------------------------------------------------------------- ::: b2g/components/AboutServiceWorkers.jsm @@ +158,1 @@ > Services.io.newURI(message.principal.origin, null, null), It's a fake principal (a plain JS object) created from https://dxr.mozilla.org/mozilla-central/source/b2g/components/AboutServiceWorkers.jsm#37 But you're right, it should use originNoSuffix. Filed Bug 1196652. ::: b2g/components/ContentPermissionPrompt.js @@ +206,5 @@ > let notDenyAppPrincipal = function(type) { > let url = Services.io.newURI(app.origin, null, null); > + let principal = secMan.createCodebasePrincipal(url, > + {appId: request.principal.appId, > + inBrowser: false}); I've discussed with Alfredo who changed here last time in Bug 853356, he suggests we still need createCodebasePrincipal as if we use request.pricipal, it might have problems for the moz browser frame. ::: docshell/base/nsDocShell.cpp @@ +9342,5 @@ > NS_ENSURE_SUCCESS(rv, rv); > + > + OriginAttributes attrs(appId, isInBrowserElement); > + nsCOMPtr<nsIPrincipal> prin = > + BasePrincipal::CreateCodebasePrincipal(aReferrer, attrs); Will add a TODO here, I think I'll cover this in Bug 1165466. ::: dom/browser-element/BrowserElementParent.js @@ +836,5 @@ > // This simply returns null if there is no principal available > // for the requested uri. This is an acceptable fallback when > // calling newChannelFromURI2. > + let attrs = {appId: this._frameLoader.loadContext.appId, > + inBrowser: this._frameLoader.loadContext.isInBrowserElement}; I'd like to land this bug first, so will add a TODO here. ::: dom/ipc/TabChild.cpp @@ +16,5 @@ > #include "ContentChild.h" > #include "TabParent.h" > +#ifdef MOZ_WIDGET_GONK > +#include "mozilla/BasePrincipal.h" > +#endif The code that using OriginAttributes is in TabChild::MaybeRequestPreinitCamera, which is wrapped by #ifdef MOZ_WIDGET_GONK, so I add ifdef here. But I look the code again I could get principal from mozIApplication, so these lines could be removed. ::: dom/permission/PermissionSettings.js @@ +36,5 @@ > PermissionSettings.prototype = { > get: function get(aPermName, aManifestURL, aOrigin, aBrowserFlag) { > debug("Get called with: " + aPermName + ", " + aManifestURL + ", " + aOrigin + ", " + aBrowserFlag); > let uri = Services.io.newURI(aOrigin, null, null); > let appID = appsService.getAppLocalIdByManifestURL(aManifestURL); Filed Bug 1196644. ::: dom/quota/QuotaManager.cpp @@ +3411,5 @@ > uint32_t aAppId, > bool aInMozBrowser, > nsACString* aGroup, > nsACString* aOrigin, > bool* aIsApp) Bug 1165217 just landed, so I don't need to change dom/quota/QuotaManager.cpp in my next patch. ::: ipc/glue/BackgroundUtils.cpp @@ +80,5 @@ > if (info.appId() == nsIScriptSecurityManager::UNKNOWN_APP_ID) { > rv = secMan->GetSimpleCodebasePrincipal(uri, getter_AddRefs(principal)); > } else { > + OriginAttributes attrs(info.appId(), info.isInBrowserElement()); > + principal = BasePrincipal::CreateCodebasePrincipal(uri, attrs); yes, will add a TODO here. ::: services/mobileid/MobileIdentityManager.jsm @@ +898,5 @@ > log.debug("getMobileIdAssertion ${}", aPrincipal); > > let uri = Services.io.newURI(aPrincipal.origin, null, null); > + let attrs = aPrincipal.originAttributes; > + let principal = securityManager.createCodebasePrincipal(uri, attrs); yes :) ::: testing/specialpowers/content/SpecialPowersObserverAPI.js @@ +313,5 @@ > case "SPPermissionManager": { > let msg = aMessage.json; > > let secMan = Services.scriptSecurityManager; > + let attrs = {appId: msg.appId, inBrowser: msg.isInBrowserElement}; Bug 1196665 ::: uriloader/prefetch/OfflineCacheUpdateParent.cpp @@ +92,5 @@ > return NS_ERROR_FAILURE; > > bool offlinePermissionAllowed = false; > > + OriginAttributes attrs(mAppId, mIsInBrowserElement); will do this in Bug 1165466.

(In reply to Yoshi Huang[:allstars.chh] from comment #14) > The code that using OriginAttributes is in > TabChild::MaybeRequestPreinitCamera, which is wrapped by #ifdef > MOZ_WIDGET_GONK, so I add ifdef here. Sure, I'm just saying that, for a non-platform-specific Gecko #include, it's cleaner to just include it unconditionally, even if it's not used on all platforms. We're not very strict about only including what we need, since Unified Builds end up merging a lot of compilation units anyway.

removed inBrowser: false

Comment on attachment 8650788 [details] [diff] [review] Part 1: replace getAppCodebasePrincipal v2. Review of attachment 8650788 [details] [diff] [review]: ----------------------------------------------------------------- ::: b2g/components/AboutServiceWorkers.jsm @@ +158,1 @@ > Services.io.newURI(message.principal.origin, null, null), forgot to add a TODO: Bug 1196652 here. will do it

add TODO

Comment on attachment 8650789 [details] [diff] [review] Part 2: replace getNoAppCodebasePrincipal. v2 Review of attachment 8650789 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/extensions/pdfjs/content/PdfStreamConverter.jsm @@ -1001,5 @@ > - // FF16 and below had getCodebasePrincipal, it was replaced by > - // getNoAppCodebasePrincipal (bug 758258). > - var resourcePrincipal = 'getNoAppCodebasePrincipal' in securityManager ? > - securityManager.getNoAppCodebasePrincipal(uri) : > - securityManager.getCodebasePrincipal(uri); PDF.js is an extension (whose canonical code lives externally), and works with multiple Firefox versions. So you should continue to do the feature-detection here. First check for createCodebasePrincipal, then for getNoAppCodebasePrincipal, then fall back to getCodebasePrincipal. ::: extensions/cookie/nsPermission.cpp @@ +170,5 @@ > + mozilla::OriginAttributes attrs; > + nsCOMPtr<nsIPrincipal> principal = mozilla::BasePrincipal::CreateCodebasePrincipal(aURI, attrs); > + if (NS_WARN_IF(!principal)) { > + return NS_ERROR_FAILURE; > + } Please just do: NS_ENSURE_TRUE(principal, NS_ERROR_FAILURE); here and anywhere else where we already have NS_ENSURE macros, which is basically all of the C++ callsites you're changing. (The plan to kill them pretty much died, and they're much more compact). ::: extensions/cookie/nsPermissionManager.cpp @@ +142,5 @@ > + return NS_ERROR_FAILURE; > + } > + > + principal.forget(aPrincipal); > + return *aPrincipal ? NS_OK : NS_ERROR_FAILURE; Given the null-check above, this should just return NS_OK, right? ::: gfx/thebes/gfxSVGGlyphs.cpp @@ +349,5 @@ > NS_ENSURE_SUCCESS(rv, rv); > > + OriginAttributes attrs; > + nsCOMPtr<nsIPrincipal> principal = > + BasePrincipal::CreateCodebasePrincipal(uri, attrs); This probably needs a null-check. ::: toolkit/components/social/SocialService.jsm @@ +155,5 @@ > let URI = Services.io.newURI(origin, null, null); > + let attrs = { > + appId: Services.scriptSecurityManager.NO_APP_ID, > + inBrowser: false > + }; You can get rid of |attrs| here, right? @@ +518,5 @@ > // force/fixup origin > let URI = Services.io.newURI(installOrigin, null, null); > + let attrs = { > + appId: Services.scriptSecurityManager.NO_APP_ID, > + inBrowser: false ANd here.

use NS_ENSURE_TRUE for checking principal

addressed bobby's comment.

add r=bholley

https://treeherder.mozilla.org/#/jobs?repo=try&revision=241c2946f4aa Adding checkin-needed for the tree is close now.

https://hg.mozilla.org/integration/mozilla-inbound/rev/8b37e978d607 https://hg.mozilla.org/integration/mozilla-inbound/rev/8d116b0d696f https://hg.mozilla.org/integration/mozilla-inbound/rev/2bd4cb790c48

https://hg.mozilla.org/mozilla-central/rev/8b37e978d607 https://hg.mozilla.org/mozilla-central/rev/8d116b0d696f https://hg.mozilla.org/mozilla-central/rev/2bd4cb790c48

This is causing a sanity blocker and will need to be backed out. Please see bug 1198308.

Bobby, this is causing a serious regression and normally I would ask the dev that wrote the patch to backout; having said that, qbackout doesn't handle this cleanly and the dev is in Taipei. Could you please back this out ASAP? This causes a serious regression where we can't web browse on B2G.

Pushed a backout. Flagging Yoshi to investigate.

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/d09ccffbeb17

Merge of backout: https://hg.mozilla.org/mozilla-central/rev/d09ccffbeb17

Comment on attachment 8651664 [details] [diff] [review] Part 1: replace getAppCodebasePrincipal v3 Review of attachment 8651664 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/ipc/TabChild.cpp @@ +1562,5 @@ > return; > } > > + nsCOMPtr<mozIApplication> app; > + nsresult rv = appsService->GetAppByLocalId(OwnAppId(), getter_AddRefs(app)); When search on the search bar, if the search content could be located, OwnAppId() will be the appId of the content, otherwise it will be 0. For example, if I search 'Contacts', search bar will show the icon of Contacts app, then I tap the icon, the OwnAppId() will be the appId of Contacts app. If I type 'foo', then it will link to Google search, then this case OwnAppId() is 0. The crash happens in the 0 case, which 'app' is NULL. In the original code, it will fail in the NS_NewURI part, and bail out in the if (NS_WARN_IF(NS_FAILED(rv))). @@ +1567,1 @@ > if (NS_WARN_IF(NS_FAILED(rv))) { I will add another check for app here to prevent the crash. if (NS_WARN_IF(NS_FAILED(rv)) || !app) {

add the !app check as mentioned in Comment 35.

Comment on attachment 8652802 [details] [diff] [review] Part 1: replace getAppCodebasePrincipal v4 Review of attachment 8652802 [details] [diff] [review]: ----------------------------------------------------------------- Wait for try to be Green first. https://treeherder.mozilla.org/#/jobs?repo=try&revision=0ed87923462f

rebase

Comment on attachment 8652802 [details] [diff] [review] Part 1: replace getAppCodebasePrincipal v4 Review of attachment 8652802 [details] [diff] [review]: ----------------------------------------------------------------- Hi, See Comment 35, I just added a !app check. Could you review this again for me? Thanks

https://hg.mozilla.org/integration/b2g-inbound/rev/0d60bb207d3e https://hg.mozilla.org/integration/b2g-inbound/rev/5a29e8bc51ca https://hg.mozilla.org/integration/b2g-inbound/rev/1f970ab08081

https://hg.mozilla.org/mozilla-central/rev/0d60bb207d3e https://hg.mozilla.org/mozilla-central/rev/5a29e8bc51ca https://hg.mozilla.org/mozilla-central/rev/1f970ab08081

Commit pushed to master at https://github.com/mozilla/addon-sdk https://github.com/mozilla/addon-sdk/commit/4d93b540bf6c0090f3f6fb3016f8780a3271f743 Bug 1165272 - Part 3: replace getCodebasePrincipal. r=bholley