Closed
Bug 1178533
Opened 9 years ago
Closed 9 years ago
Register permissions, system messages etc on navigation to signed packages
Categories
(Core Graveyard :: DOM: Apps, defect, P1)
Core Graveyard
DOM: Apps
Tracking
(blocking-b2g:2.5+, firefox44 fixed)
Tracking | Status | |
---|---|---|
firefox44 | --- | fixed |
People
(Reporter: pauljt, Assigned: arroway)
References
Details
Attachments
(1 file, 19 obsolete files)
When we navigate to signed packages, this is the equivalent to installing in the current model. We need to do all of the install registration at navigation time instead. This includes: - registering permissions - registering system messages This does not include Web Activity registration, which is planned to be done at time the content is Pinned. As part of this bug, we also need to add code to keep these registrations updated. This could be removal on cache eviction, or perhaps just ensuring they are updated on each navigation.
Reporter | ||
Comment 1•9 years ago
|
||
PS I'm not sure if DOM:apps makes sense here - does this belong more in core:networking? This is going to be a FxOS only thing so I'm not sure.
Updated•9 years ago
|
Assignee: nobody → stephouillon
Reporter | ||
Updated•9 years ago
|
Reporter | ||
Updated•9 years ago
|
Priority: -- → P1
Reporter | ||
Updated•9 years ago
|
blocking-b2g: --- → 2.5+
Comment 2•9 years ago
|
||
(In reply to Paul Theriault [:pauljt] from comment #0) > When we navigate to signed packages, this is the equivalent to installing in > the current model. We need to do all of the install registration at > navigation time instead. This includes: > - registering permissions > - registering system messages > > This does not include Web Activity registration, which is planned to be done > at time the content is Pinned. > > As part of this bug, we also need to add code to keep these registrations > updated. This could be removal on cache eviction, or perhaps just ensuring > they are updated on each navigation. I propose to create a module to deal with all the "app installation" things like permissions and system messages. Other than that, it can also set the proper origin to this app, just what Bug 1178526 is intended to do. Following are the reasons: 1) Origin set up is (kind of) a part of installation. 2) The entire manifest is supposed to pass to the install function so it's natural and convenient to get the origin, whereas it's hard and not transparent for necko to do that. Waiting for Stephanie back and have a discussion then.
Comment 3•9 years ago
|
||
Hi Stéphanie, Just forget about the 'origin' stuff I mentioned in comment 2. We might only need to deal with the permission registration and the system message things. So will you create some XPCOM like what I propose or you have other thoughts? Probably I will call your functions somewhere like [1]. Thanks! [1] https://github.com/elefant/gecko-dev/commit/4c7adf22035e999339a5603c048d1f637afac865#diff-f6a3710a33233135e9cc56c1a50c8269R774
Flags: needinfo?(stephouillon)
Assignee | ||
Comment 4•9 years ago
|
||
Hi Henry, thanks for the heads-up, yesterday I was catching up with the nsec mailing list I was not following before and saw the emails were you and others were talking about that. Indeed, I think that I'll go with what you proposed (with a js xpcom) and come back soon with a first work-in-progress patch. Thanks!
Flags: needinfo?(stephouillon)
Assignee | ||
Comment 5•9 years ago
|
||
This patch implements an installPackagedApp() method to register permissions. There are two points at least yet to be solved regarding registering permissions: 1) getting the correct origin for a signed package via the package-identifier field in the manifest (I don't know if it's ready to be used yet? I tested the code by navigating to [1]). 2) generating a correct localId for the installed app (right now, it's a harcoded value to make the poc work). I'm not quite sure of the way it should be done since in the case of an installed package, it's related to the part of the code involving DOMApplicationRegistry (should I use the same way to generate an id via makeAppId() and use the same list of ids?) This patch is integrated with hchang's work on this branch [2]. @Henry: The following patches: 2.a-Fixing-permissions-install-and-adding-return-status.patch and 2.b-Add-return-status-when-calling-InstallPackagedWebapp.patch can be applied on your repository [3] to get the same code, + some modifications in the PackagedAppService.* files. [1] http://people.mozilla.org/~fdesre/packages/loqui.pak!//index.html [2] https://github.com/arroway/gecko-dev/commits/Bug1178533-permission [3] https://github.com/elefant/gecko-dev/commits/Bug1178533-permission
Flags: needinfo?(hchang)
Attachment #8656083 -
Flags: feedback?(fabrice)
Assignee | ||
Comment 6•9 years ago
|
||
Assignee | ||
Comment 7•9 years ago
|
||
Assignee | ||
Comment 8•9 years ago
|
||
Ah, and the tests are useless so far apart from testing the syntax when running, I forgot to remove the code completely.
Comment 9•9 years ago
|
||
Comment on attachment 8656083 [details] [diff] [review] bug-1178533-Add-nsIInstallPackagedWebapp-for-registe.patch Review of attachment 8656083 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/apps/PermissionsInstaller.jsm @@ +42,4 @@ > * A function called if an error occurs > * @returns void > **/ > + you have some trailing whitespace issues in this patch. @@ +223,5 @@ > value: aPermValue, > + browserFlag: false, > + kind: aKind, > + //TODO: hack for poc. How to generate correct localId? > + localId: "13371337", I'm not sure if all the http packages will share the same id or not. @@ +224,5 @@ > + browserFlag: false, > + kind: aKind, > + //TODO: hack for poc. How to generate correct localId? > + localId: "13371337", > + isCachedPackagedWebapp: true, that's can't be `true` all the time, right? ::: dom/newapps/InstallPackagedWebapp.js @@ +21,5 @@ > + classDescription: "InstallPackagedWebapp JavaScript XPCOM Component", > + classID: Components.ID("{5cc6554a-5421-4a5e-b8c2-c62e8b7f4f3f}"), > + contractID: "@mozilla.org/newapps/installpackagedwebapp;1", > + > + QueryInterface: XPCOMUtils.generateQI([Components.interfaces.nsIInstallPackagedWebapp]), nit: s/Components.interfaces/Ci @@ +22,5 @@ > + classID: Components.ID("{5cc6554a-5421-4a5e-b8c2-c62e8b7f4f3f}"), > + contractID: "@mozilla.org/newapps/installpackagedwebapp;1", > + > + QueryInterface: XPCOMUtils.generateQI([Components.interfaces.nsIInstallPackagedWebapp]), > + trailing whitespace @@ +36,5 @@ > + **/ > + > + installPackagedWebapp: function(manifestContent, aOrigin, aManifestURL) { > + > + var aManifest = JSON.parse(manifestContent); s/var/let @@ +37,5 @@ > + > + installPackagedWebapp: function(manifestContent, aOrigin, aManifestURL) { > + > + var aManifest = JSON.parse(manifestContent); > + whitespace @@ +46,5 @@ > + manifest: aManifest, > + manifestURL: aManifestURL, > + origin: aOrigin, > + isPreinstalled: false, > + kind: "" //empty if not trusted hosted app let's remove that. @@ +49,5 @@ > + isPreinstalled: false, > + kind: "" //empty if not trusted hosted app > + }, false, function() { > + debug("Error installing permissions for " + aOrigin); > + return false; here only your inner function returns `false`, not installPackagedWebapp. I think you'll have to make it all async... ::: dom/permission/PermissionSettings.jsm @@ +72,5 @@ > + > + let app; > + // Test if app is cached (signed streamable package) or installed via DOMApplicationRegistry > + if (aData.isCachedPackagedWebapp) { > + app = {localId: aData.localId, kind: aData.kind}; hm, `kind` is useless here. That's a leftover from THA that we should clean up (http://mxr.mozilla.org/mozilla-central/source/dom/apps/PermissionsTable.jsm#644 doesn't use it anymore).
Attachment #8656083 -
Flags: feedback?(fabrice)
Comment 10•9 years ago
|
||
(In reply to Stephanie Ouillon [:arroway] from comment #5) > Created attachment 8656083 [details] [diff] [review] > bug-1178533-Add-nsIInstallPackagedWebapp-for-registe.patch > > This patch implements an installPackagedApp() method to register permissions. > > There are two points at least yet to be solved regarding registering > permissions: > > 1) getting the correct origin for a signed package via the > package-identifier field in the manifest (I don't know if it's ready to be > used yet? I tested the code by navigating to [1]). > Not yet :( We are investigating Bug 1178526. There's already some progress on it. > 2) generating a correct localId for the installed app (right now, it's a > harcoded value to make the poc work). I'm not quite sure of the way it > should be done since in the case of an installed package, it's related to > the part of the code involving DOMApplicationRegistry (should I use the same > way to generate an id via makeAppId() and use the same list of ids?) > The app id will be obtained by the loadcontext. If you navigate the page from browser app, the app id would be the browser app's app id. My understanding is in the long run the use of app id will be obsoleted but not for now (at least 2.5) > This patch is integrated with hchang's work on this branch [2]. > > @Henry: > The following patches: > 2.a-Fixing-permissions-install-and-adding-return-status.patch > and > 2.b-Add-return-status-when-calling-InstallPackagedWebapp.patch > > can be applied on your repository [3] to get the same code, + some > modifications in the PackagedAppService.* files. > > [1] http://people.mozilla.org/~fdesre/packages/loqui.pak!//index.html > [2] https://github.com/arroway/gecko-dev/commits/Bug1178533-permission > [3] https://github.com/elefant/gecko-dev/commits/Bug1178533-permission Great! I'll take it a look and give it a test. Thanks :)
Flags: needinfo?(hchang)
Assignee | ||
Comment 11•9 years ago
|
||
Attachment #8656083 -
Attachment is obsolete: true
Attachment #8656700 -
Flags: feedback?(fabrice)
Assignee | ||
Comment 12•9 years ago
|
||
Assignee | ||
Comment 13•9 years ago
|
||
Assignee | ||
Comment 14•9 years ago
|
||
Thanks for the review Fabrice, I tried to adress your comments in path v2 (the diff with the previous patch is 3.a). See my comments below: (In reply to [:fabrice] Fabrice Desré from comment #9) > Comment on attachment 8656083 [details] [diff] [review] > bug-1178533-Add-nsIInstallPackagedWebapp-for-registe.patch > > Review of attachment 8656083 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/apps/PermissionsInstaller.jsm > @@ +42,4 @@ > > * A function called if an error occurs > > * @returns void > > **/ > > + > > you have some trailing whitespace issues in this patch. Should be fixed now. > > @@ +223,5 @@ > > value: aPermValue, > > + browserFlag: false, > > + kind: aKind, > > + //TODO: hack for poc. How to generate correct localId? > > + localId: "13371337", > > I'm not sure if all the http packages will share the same id or not. Based on Henry's comment, I now get the appId from the loadcontext. Concretely, I'm now getting it as an argument of installPackagedWebapp(). I need to see with Henry if my implementation is correct (see patch 3.b). > > @@ +224,5 @@ > > + browserFlag: false, > > + kind: aKind, > > + //TODO: hack for poc. How to generate correct localId? > > + localId: "13371337", > > + isCachedPackagedWebapp: true, > > that's can't be `true` all the time, right? Right, now I'm passing it as an argument from installPackagedWebapp when calling PermissionsInstaller.addPermission(). > > ::: dom/newapps/InstallPackagedWebapp.js > @@ +21,5 @@ > > + classDescription: "InstallPackagedWebapp JavaScript XPCOM Component", > > + classID: Components.ID("{5cc6554a-5421-4a5e-b8c2-c62e8b7f4f3f}"), > > + contractID: "@mozilla.org/newapps/installpackagedwebapp;1", > > + > > + QueryInterface: XPCOMUtils.generateQI([Components.interfaces.nsIInstallPackagedWebapp]), > > nit: s/Components.interfaces/Ci fixed > > @@ +22,5 @@ > > + classID: Components.ID("{5cc6554a-5421-4a5e-b8c2-c62e8b7f4f3f}"), > > + contractID: "@mozilla.org/newapps/installpackagedwebapp;1", > > + > > + QueryInterface: XPCOMUtils.generateQI([Components.interfaces.nsIInstallPackagedWebapp]), > > + > > trailing whitespace fixed > > @@ +36,5 @@ > > + **/ > > + > > + installPackagedWebapp: function(manifestContent, aOrigin, aManifestURL) { > > + > > + var aManifest = JSON.parse(manifestContent); > > s/var/let fixed > > @@ +37,5 @@ > > + > > + installPackagedWebapp: function(manifestContent, aOrigin, aManifestURL) { > > + > > + var aManifest = JSON.parse(manifestContent); > > + > > whitespace fixed > > @@ +46,5 @@ > > + manifest: aManifest, > > + manifestURL: aManifestURL, > > + origin: aOrigin, > > + isPreinstalled: false, > > + kind: "" //empty if not trusted hosted app > > let's remove that. done > > @@ +49,5 @@ > > + isPreinstalled: false, > > + kind: "" //empty if not trusted hosted app > > + }, false, function() { > > + debug("Error installing permissions for " + aOrigin); > > + return false; > > here only your inner function returns `false`, not installPackagedWebapp. I > think you'll have to make it all async... I modified this part by throwing an exception instead. > > ::: dom/permission/PermissionSettings.jsm > @@ +72,5 @@ > > + > > + let app; > > + // Test if app is cached (signed streamable package) or installed via DOMApplicationRegistry > > + if (aData.isCachedPackagedWebapp) { > > + app = {localId: aData.localId, kind: aData.kind}; > > hm, `kind` is useless here. That's a leftover from THA that we should clean > up > (http://mxr.mozilla.org/mozilla-central/source/dom/apps/PermissionsTable. > jsm#644 doesn't use it anymore). I've just removed it from my code, should I open a new bug and fix this in a different patch? (it sounds better than cleaning ii in this bug).
Comment 15•9 years ago
|
||
Comment on attachment 8656700 [details] [diff] [review] v2-bug-1178533-Add-nsIInstallPackagedWebapp-for-registe.patch Review of attachment 8656700 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/apps/PermissionsInstaller.jsm @@ +42,4 @@ > * A function called if an error occurs > * @returns void > **/ > + undo this change @@ +217,5 @@ > + aPermValue, > + aOrigin, > + aManifestURL, > + aAppId, > + aIsCachedPackage) { Why don't you keep the same signature and just update the list of used properties? ::: dom/newapps/InstallPackagedWebapp.js @@ +36,5 @@ > + * The app id > + * @returns boolean > + **/ > + > + installPackagedWebapp: function(manifestContent, aOrigin, aManifestURL, aAppId) { nit: s/manifestContent/aManifestContent @@ +39,5 @@ > + > + installPackagedWebapp: function(manifestContent, aOrigin, aManifestURL, aAppId) { > + > + try { > + var isSuccess = true; s/var/let @@ +40,5 @@ > + installPackagedWebapp: function(manifestContent, aOrigin, aManifestURL, aAppId) { > + > + try { > + var isSuccess = true; > + let aManifest = JSON.parse(manifestContent); let manifest = ... @@ +42,5 @@ > + try { > + var isSuccess = true; > + let aManifest = JSON.parse(manifestContent); > + //TODO: get package identifier from the manifest to build > + //the signed packaged origin. nit: space after // @@ +52,5 @@ > + appId: aAppId, > + isPreinstalled: false, > + isCachedPackage: true > + }, false, function() { > + throw "Error installing permissions in PermissionsInstaller"; that will throw *after* this method returns, so is that useful compared to just Cu.reportError(...)? @@ +61,5 @@ > + return isSuccess; > + } > + catch(ex) { > + Cu.reportError(ex); > + return !isSuccess; you always want to return false here, right? ::: dom/permission/PermissionSettings.jsm @@ +104,4 @@ > } > > if (aAllowAllChanges || > + this._isChangeAllowed(principal, aData.type, aData.value)) { I guess this change should be in another patch ;)
Attachment #8656700 -
Flags: feedback?(fabrice)
Assignee | ||
Comment 16•9 years ago
|
||
Attachment #8656084 -
Attachment is obsolete: true
Attachment #8656086 -
Attachment is obsolete: true
Attachment #8656700 -
Attachment is obsolete: true
Attachment #8656701 -
Attachment is obsolete: true
Attachment #8656702 -
Attachment is obsolete: true
Attachment #8661313 -
Flags: review?(fabrice)
Assignee | ||
Comment 17•9 years ago
|
||
Assignee | ||
Comment 18•9 years ago
|
||
Assignee | ||
Comment 19•9 years ago
|
||
Comment on attachment 8661314 [details] [diff] [review] 2.Register-permissions-to-signed-packages.patch Hello Valentin, I saw you reviewed Henry's work for bug 1178525, this patch is adding some code in PackagedAppService. Can I give it to you to review or should I ask somebody else? Thx!
Attachment #8661314 -
Flags: review?(valentin.gosu)
Assignee | ||
Comment 20•9 years ago
|
||
Henry, would you have any idea who I could ask for a review for the modifications you made in caps/nsIScriptSecurityManager (Bobby Holley maybe as I saw you requested in bug 1163254?)? Thx!
Flags: needinfo?(hchang)
Assignee | ||
Comment 21•9 years ago
|
||
@fabrice: the new version of the patch is hopefully addressing your previous comments, as well as fixing some issues such as loading the module in b2g (this happens to be useful sometimes...). There is currently a bug related to parent side permissions checking (cf email on the new security list), waiting to be fixed with bug 1191740.
Comment 22•9 years ago
|
||
Comment on attachment 8661314 [details] [diff] [review] 2.Register-permissions-to-signed-packages.patch Review of attachment 8661314 [details] [diff] [review]: ----------------------------------------------------------------- r=me with these few nits. ::: netwerk/protocol/http/PackagedAppService.cpp @@ +862,5 @@ > LOG(("Install this packaged app.")); > + bool value = false; > + bool *isSuccess = &value; > + > + nsCOMPtr<nsIInstallPackagedWebapp> installer = do_GetService("@mozilla.org/newapps/installpackagedwebapp;1"); Try to keep it under 80 chars per line if possible. @@ +881,5 @@ > + installer->InstallPackagedWebapp(mManifestContent.get(), > + packageOrigin.get(), > + manifestURL.get(), > + mAppId, > + isSuccess); Can't you pass &value here, and avoid the extra variable?
Attachment #8661314 -
Flags: review?(valentin.gosu) → review+
Comment 23•9 years ago
|
||
Comment on attachment 8661313 [details] [diff] [review] 1.bug-1178533-Add-nsIInstallPackagedWebapp-for-registe.patch Review of attachment 8661313 [details] [diff] [review]: ----------------------------------------------------------------- If you choose to use nsIScriptSecurityManager.createCodebasePrincipalFromOrigin for signed package in PermissionSettings.jsm, then you don't need to pass the appId/inBrowser in. The origin you pass in already contain all of those information (assume you use GetOrigin in the patch part 2) and you can get them after you create the principal based on the origin. ::: dom/apps/PermissionsInstaller.jsm @@ +201,4 @@ > origin: aApp.origin, > manifestURL: aApp.manifestURL, > value: aPermValue, > + browserFlag: false || aApp.browserFlag, Same here. We actually don't need the browserFlag here according to your design in PermissionSettings.jsm::_internalAddPermission. ::: dom/newapps/InstallPackagedWebapp.js @@ +50,5 @@ > + manifest: manifest, > + manifestURL: aManifestURL, > + origin: aOrigin, > + localId: aAppId, > + browserFlag: true, I think we don't need localId and browserFlag here since they will not be used if isCachedPackage is true. ::: dom/permission/PermissionSettings.jsm @@ +72,5 @@ > + let principal; > + // Test if app is cached (signed streamable package) or installed via DOMApplicationRegistry > + if (aData.isCachedPackage) { > + // If the app is from packaged web app, the origin includes origin attributes already. > + app = {localId: aData.localId}; We can obtain the appId from the principal that we are gonna create based on origin below.
Comment 24•9 years ago
|
||
(In reply to Stephanie Ouillon [:arroway] from comment #20) > Henry, > would you have any idea who I could ask for a review for the modifications > you made in caps/nsIScriptSecurityManager (Bobby Holley maybe as I saw you > requested in bug 1163254?)? Thx! Yes I think Bobby Holley is very suitable for reviewing the change :)
Flags: needinfo?(hchang)
Comment 25•9 years ago
|
||
(In reply to Valentin Gosu [:valentin] from comment #22) > Comment on attachment 8661314 [details] [diff] [review] > 2.Register-permissions-to-signed-packages.patch Also, I would love to see some tests for this functionality. See netwerk/test/unit/test_packaged_app_*.js and netwerk/test/mochitest/test_web_packaged_app.html
Assignee | ||
Comment 26•9 years ago
|
||
Attachment #8661313 -
Attachment is obsolete: true
Attachment #8661313 -
Flags: review?(fabrice)
Attachment #8661836 -
Flags: review?(fabrice)
Assignee | ||
Comment 27•9 years ago
|
||
Assignee | ||
Comment 28•9 years ago
|
||
Hello Bobby, could you have a look at this patch? It defines a method to create the principal from an origin passed from netwerk/protocol/http/PackagedAppService.cpp (see patch 0002), and is called in dom/permission/PermissionSettings.jsm (see patch 0001). Thx!
Attachment #8661316 -
Attachment is obsolete: true
Attachment #8661845 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 29•9 years ago
|
||
Comment on attachment 8661837 [details] [diff] [review] 0002-Bug-1178533-Register-permissions-to-signed-packages-.patch r=valentin Valentin, thanks for the review, I addressed your comments. Not sure if I need to r? you again on this new patch, but doing it anyway to be sure.
Attachment #8661837 -
Flags: review?(valentin.gosu)
Assignee | ||
Comment 30•9 years ago
|
||
Henry, I addressed your comments, it should look much better that way. Thx for the review!
Flags: needinfo?(hchang)
Comment 31•9 years ago
|
||
Comment on attachment 8661845 [details] [diff] [review] 0003-Bug-1178533-Use-origin-to-create-the-principal-which.patch Review of attachment 8661845 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for cleaning up this API gap! Please move the guts of this method into a new overload over BasePrincipal::CreateCodebasePrincipal which takes an nsACString. Also, please remove principalFromOrigin from BrowserUtils.jsm and fix up the consumers. It would also be good to move the checks for "[" and "moz-nullprincipal" into nsScriptSecurityManager::CreateCodebasePrincipalsFromOrigin, and assert against those cases in BasePrincipal::CreateCodebasePrincipla.
Attachment #8661845 -
Flags: review?(bobbyholley) → review-
Comment 32•9 years ago
|
||
Comment on attachment 8661837 [details] [diff] [review] 0002-Bug-1178533-Register-permissions-to-signed-packages-.patch r=valentin Review of attachment 8661837 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Stephanie Ouillon [:arroway] from comment #29) > Comment on attachment 8661837 [details] [diff] [review] > 0002-Bug-1178533-Register-permissions-to-signed-packages-.patch r=valentin > > Valentin, thanks for the review, I addressed your comments. > Not sure if I need to r? you again on this new patch, but doing it anyway to > be sure. Thanks! After an r+ you needn't ask for another review if changes are trivial.
Attachment #8661837 -
Flags: review?(valentin.gosu) → review+
Comment 33•9 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #31) > Comment on attachment 8661845 [details] [diff] [review] > 0003-Bug-1178533-Use-origin-to-create-the-principal-which.patch > > Review of attachment 8661845 [details] [diff] [review]: > ----------------------------------------------------------------- > > Thanks for cleaning up this API gap! > > Please move the guts of this method into a new overload over > BasePrincipal::CreateCodebasePrincipal which takes an nsACString. > > Also, please remove principalFromOrigin from BrowserUtils.jsm and fix up the > consumers. It would also be good to move the checks for "[" and > "moz-nullprincipal" into > nsScriptSecurityManager::CreateCodebasePrincipalsFromOrigin, and assert > against those cases in BasePrincipal::CreateCodebasePrincipla. Thanks for the review, Bobby! Hi Stephanie, Just let me know if you need help addressing these comments :)
Flags: needinfo?(hchang) → needinfo?(stephouillon)
Comment 35•9 years ago
|
||
In response to the question on IRC - comment 31 is talking about adding an overload on the C++-only BasePrincipal class, not on nsIScriptSecurityManager. See elsewhere how all the JS-facing APIs funnel into BasePrincipal::CreateCodebasePrincipal.
Comment 36•9 years ago
|
||
Comment on attachment 8661836 [details] [diff] [review] 0001-Bug-1178533-Add-nsIInstallPackagedWebapp-for-registe.patch Review of attachment 8661836 [details] [diff] [review]: ----------------------------------------------------------------- f+ because I really want to see full tests checking that permissions are set up properly. ::: dom/apps/PermissionsInstaller.jsm @@ +192,4 @@ > * The permission value. > * @param object aApp > * The just-installed app configuration. > + * The properties used are manifestURL, origin, appId, isCachedPackage nit: add full stop at the end of sentence. ::: dom/newapps/InstallPackagedWebapp.js @@ +23,5 @@ > + classInfo: XPCOMUtils.generateCI({ > + classID: Components.ID("{5cc6554a-5421-4a5e-b8c2-c62e8b7f4f3f}"), > + classDescription: "InstallPackagedWebapp JavaScript XPCOM Component", > + interfaces: [Ci.nsIInstallPackagedWebapp] > + }), you don't need classInfo @@ +51,5 @@ > + isPreinstalled: false, > + isCachedPackage: true > + }, false); > + > + // TODO: register app handlers (system msg) Can you file a bug for that and add the bug number here? ::: dom/newapps/test/xpcshell/test_install.js @@ +30,5 @@ > + aOrigin = "http://test.com"; > + aManifestURL = "http://test.com/manifest.json"; > + manifestString = JSON.stringify(manifest); > + res = mod.installPackagedWebapp(manifestString, aOrigin, aManifestURL, appId); > + equal(res, true); You're not really testing that we set permissions here, just that nothing is blowing up ;) ::: dom/permission/PermissionSettings.jsm @@ +69,5 @@ > _internalAddPermission: function _internalAddPermission(aData, aAllowAllChanges, aCallbacks) { > // TODO: Bug 1196644 - Add signPKg parameter into PermissionSettings.jsm > + let app; > + let principal; > + // Test if app is cached (signed streamable package) or installed via DOMApplicationRegistry nit: full stop at end of comment.
Attachment #8661836 -
Flags: review?(fabrice) → feedback+
Assignee | ||
Comment 37•9 years ago
|
||
Thanks for the review Fabrice. With this new patch, I adressed your comments and wrote some (more useful) xpcshell unit tests. To check the permissions, I adapted getPermission and removepermission in PermissionSettings to support how we're using the origin to get the principal and the isCachedPackage flag.
Attachment #8661836 -
Attachment is obsolete: true
Attachment #8663077 -
Flags: review?(fabrice)
Assignee | ||
Comment 38•9 years ago
|
||
Hi Bobby, I refactored the code according to your comments (thanks for your precision about BasePrincipal btw :) and I removed the principalFromOrigin function in BrowserUtils.jsm.
Attachment #8661845 -
Attachment is obsolete: true
Attachment #8663078 -
Flags: review?(bobbyholley)
Comment 39•9 years ago
|
||
Comment on attachment 8663078 [details] [diff] [review] 0003-Bug-1178533-Use-origin-to-create-the-principal-which.patch Review of attachment 8663078 [details] [diff] [review]: ----------------------------------------------------------------- Looks great. Please check which of the JS files starting importing BrowserUtils.jsm in order to use principalFromOrigin, and try removing the import (you'll need a try push to make sure nothing else started depending on it in the mean time). r=me with that. Thanks for the patch!
Attachment #8663078 -
Flags: review?(bobbyholley) → review+
Comment 40•9 years ago
|
||
Comment on attachment 8663077 [details] [diff] [review] 0001-Bug-1178533-Add-nsIInstallPackagedWebapp-for-registe.patch Review of attachment 8663077 [details] [diff] [review]: ----------------------------------------------------------------- r=me with nits fixed ::: b2g/installer/package-manifest.in @@ +737,3 @@ > ; ANGLE on Win32 > #ifdef XP_WIN32 > #ifndef HAVE_64BIT_BUILD I think you will have to also add these to firefox's package-manifest.in to make packages work with mulet. And you need to add them to b2gdroid/installer/package-manifest.in ::: dom/newapps/interfaces/nsIInstallPackagedWebapp.idl @@ +8,5 @@ > +interface nsIInstallPackagedWebapp : nsISupports > +{ > + boolean installPackagedWebapp(in string manifestContent, > + in string aOrigin, > + in string aManifestURL); nit: either aXyz or xzy everywhere.
Comment 41•9 years ago
|
||
Comment on attachment 8663077 [details] [diff] [review] 0001-Bug-1178533-Add-nsIInstallPackagedWebapp-for-registe.patch Review of attachment 8663077 [details] [diff] [review]: ----------------------------------------------------------------- r=me with nits fixed ::: b2g/installer/package-manifest.in @@ +737,3 @@ > ; ANGLE on Win32 > #ifdef XP_WIN32 > #ifndef HAVE_64BIT_BUILD I think you will have to also add these to firefox's package-manifest.in to make packages work with mulet. And you need to add them to b2gdroid/installer/package-manifest.in ::: dom/newapps/interfaces/nsIInstallPackagedWebapp.idl @@ +8,5 @@ > +interface nsIInstallPackagedWebapp : nsISupports > +{ > + boolean installPackagedWebapp(in string manifestContent, > + in string aOrigin, > + in string aManifestURL); nit: either aXyz or xzy everywhere.
Attachment #8663077 -
Flags: review?(fabrice) → review+
Assignee | ||
Comment 42•9 years ago
|
||
Thanks for your reviews! Uploading final patch in hg format with nits fixed, and pushing to try server as soon as I get back commit access to it.
Attachment #8661314 -
Attachment is obsolete: true
Attachment #8661837 -
Attachment is obsolete: true
Attachment #8663077 -
Attachment is obsolete: true
Attachment #8663078 -
Attachment is obsolete: true
Assignee | ||
Comment 43•9 years ago
|
||
Try server: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a0df212ac17a
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 44•9 years ago
|
||
Attachment #8664216 -
Attachment is obsolete: true
Assignee | ||
Comment 45•9 years ago
|
||
(uploaded patch r+ with a commit message without try commands)
Comment 46•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/8bbdb80f67e0
Keywords: checkin-needed
Comment 47•9 years ago
|
||
sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=14564460&repo=mozilla-inbound
Flags: needinfo?(stephouillon)
Comment 48•9 years ago
|
||
Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/f721299f9dca
Comment 49•9 years ago
|
||
It seems the test failed on Android platform
Assignee | ||
Comment 50•9 years ago
|
||
There was some lines missing in package-manifest.in for android, I'll push the fix on the try server to test.
Flags: needinfo?(stephouillon)
Assignee | ||
Comment 51•9 years ago
|
||
Fixed the issue on android and passed the tests on try (https://treeherder.mozilla.org/#/jobs?repo=try&revision=a56a1f61f49d)/.
Attachment #8664399 -
Attachment is obsolete: true
Assignee | ||
Comment 52•9 years ago
|
||
Attachment #8664854 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 53•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b12584fad334
Keywords: checkin-needed
Comment 54•9 years ago
|
||
Hi Stephanie, sorry had to back this out again, seems this was causing test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=14625971&repo=mozilla-inbound
Flags: needinfo?(stephouillon)
Comment 55•9 years ago
|
||
Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/75056331fc4b
Assignee | ||
Comment 56•9 years ago
|
||
The tests are passing on the emulator, but it's triggering an assertion failure in the cycle collector when the xpcom thread is shutdown, apparently because it tries to do a cleanup when it's too late. I'm working on fixing it. My understanding is that it affects only the emulator so far because of perf issues. logcat: I/GonkMemoryPressure( 1093): Observed XPCOM shutdown. I/Gecko ( 1093): [1093] ###!!! ASSERTION: Failed Dispatch after xpcom-shutdown-threads: 'false', file /home/arroway/dev/B2G-flame/gecko/xpcom/threads/nsThread.cpp, line 587 E/Gecko ( 1093): mozalloc_abort: [1093] ###!!! ASSERTION: Failed Dispatch after xpcom-shutdown-threads: 'false', file /home/arroway/dev/B2G-flame/gecko/xpcom/threads/nsThread.cpp, line 587 F/MOZ_CRASH( 1093): Hit MOZ_CRASH() at /home/arroway/dev/B2G-flame/gecko/memory/mozalloc/mozalloc_abort.cpp:33
Flags: needinfo?(stephouillon)
Assignee | ||
Comment 57•9 years ago
|
||
Attachment #8664855 -
Attachment is obsolete: true
Assignee | ||
Comment 58•9 years ago
|
||
Finally fixed the issue on the emulator. Waiting for try server results here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=17191998435e
Attachment #8667477 -
Attachment is obsolete: true
Assignee | ||
Comment 59•9 years ago
|
||
Resultats are green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dfffb42a1d6a except for the builds which failed for graphene and horizon, but it looks like an infrastructure issue. I hope this one is the good one.
Comment 61•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/08360a185f3f
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox44:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Updated•7 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•