Per bug 1097468 comment 19, we need to have a way to avoid this trap instead of |popPermissions| workaround.

pushPermissions() will create a undoStack[1] to clear the permissions and this undoStack will be popped by popPermissions() or flushPermissions()(it will call popPermissions actually). If we use other way(e.g. removePermission()) to clear the permissions we set, instead of popPermissions(), then SimpleTest.finish() will never be terminated. The reason is that flushPermissions()[2] will remove all the permissions in undoStack by popPermissions() that wait a "perm-changed" signal. However, the permissions were already removed by other way(e.g. removePermission()), so flushPermissions() will never received the "perm-changed" signal [1] https://dxr.mozilla.org/mozilla-central/source/testing/specialpowers/content/specialpowersAPI.js#833 [2] https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/SimpleTest.js#1010

I think you shouldn't use SpecialPowers.removePermission at all in mochitest. See discussion in bug 951272. However, this seems indeed like a bug.

would the concept of removePermissions remove the permissions from the stack to flush? Ideally we would just have a way to cherry pick permissions from that stack and remove them. Likewise we should include a timeout in the waiting for perm-changed and generate an error when we timeout. This error can be used for test developers to change their tests accordingly if this is the intended behavior.

(In reply to Martijn Wargers [:mwargers] (QA) from comment #2) > I think you shouldn't use SpecialPowers.removePermission at all in > mochitest. See discussion in bug 951272. > However, this seems indeed like a bug. Hi, Martijn Thanks for your comment. Yes, removePermissions() may be not a good example. I just want to reproduce the situation bug 1097468 comment 18 faced in a simplest way. I think the real situations is: The items in UndoStack will be executed before flushPermissions()[1] by uninstalling the app[2] and fire "perm-changed" signals[3]. Thus, when SimpleTest.finish() call flushPermissions() and try to execute the items in UndoStack that were already executed by [2], the flushPermissions() can't receive "perm-changed" signals[3]. [1] https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/SimpleTest.js#1010 [2] https://dxr.mozilla.org/mozilla-central/source/dom/apps/Webapps.js#929 [3] https://dxr.mozilla.org/mozilla-central/source/extensions/cookie/nsPermissionManager.cpp#1745

(In reply to Joel Maher (:jmaher) from comment #3) > would the concept of removePermissions remove the permissions from the stack > to flush? Ideally we would just have a way to cherry pick permissions from > that stack and remove them. Likewise we should include a timeout in the > waiting for perm-changed and generate an error when we timeout. This error > can be used for test developers to change their tests accordingly if this is > the intended behavior. Hi, Joel Maher It's glad to get your feedback! Yes, removePermissions() may not be a good choice to reproduce this bug. The real situation faced is explained in comment #4. Would you let me know which one do you suggest in the below cases: 1) Make permissionObserver to monitor all the "perm-changed" signals. If the items in UndoStack are executed by others(not specialpowersAPI.js itself), then the items will be removed from UndoStack. 2) Use a timeout mechanism before adding a permissionObserver to wait "perm-changed" signal. If the "perm-changed" signal is not fired after timeout, then the behavior is normal or not, will be decided by developers(Does it need to prompt: "forget to call |popPermissions| after pushing?"). or any other better case than these?

I think observing all perm-changed is interesting although it could be problematic. One concern here is that we pushPermission(), the change is caught and it is on our stack, then we remove app which catches more permission changes. These would be caught and put on the stack. Now we have two permission changes on the stack for the same thing, both of which we don't need and undoing them could be problematic. When we pop them off, we would add the permission first, then remove it- in fact this might work pretty good. timeout mechanism- my original thought, ensures that folks writing tests are explicit about what permissions they require and expect. Regarding the timeout, anytime we hit it should be a failure with a clear message about what perm-changed we didn't get and your suggestion about 'forgot to call ...' seems great. Either approach or some hybrid would be nice. Permissions are different than preferences, we just happen to have similar approaches for both to date.

Comment on attachment 8600734 [details] [diff] [review] [v1] make permissionObserver listen all 'perm-changed' signals to operate the undostack (In reply to Joel Maher (:jmaher) from comment #6) > I think observing all perm-changed is interesting although it could be > problematic. One concern here is that we pushPermission(), the change is > caught and it is on our stack, then we remove app which catches more > permission changes. These would be caught and put on the stack. Now we > have two permission changes on the stack for the same thing, both of which > we don't need and undoing them could be problematic. When we pop them off, > we would add the permission first, then remove it- in fact this might work > pretty good. Hi, Joel Could you review this patch? I would be grateful for any feedback you could give me :) In this patch, |permissionObserver| is able to listen all "perm-changed" signals includes but is not limited to specialpowersAPI.js itself. To keep the original design of |permissionObserver|, flag |_applyingPermissions| is used to distinguish the "perm-changed" signal is trigger from specialpowersAPI.js itself or not. If the "perm-changed" signal is fired by itself, then code flow is same as before. Otherwise, it will ckeck whether the operation is duplicate in undostack or not. If it's duplicate, it means that we don't need to do it again(others has already executed this action). Thus, the duplicate action in undostack will be removed.

Move permissionObserver to SpecialPowersObserver.js to listen all 'perm-changed' signals because SpecialPowersAPI.js is not in chrome-process

> Created attachment 8601311 [details] [diff] [review] Hi, Joel, Could you take a look at this patch? It seems that the calling of this._addMessageListener in specialpowersAPI.js will lead a "TypeError: this._addMessageListener is not a function" in dom/power/test/browser_wakelocks.js[1] Is there any way to avoid this error? [1]https://treeherder.mozilla.org/#/jobs?repo=try&revision=cd618cd38a8c

If this code is all in specialpowersobserver.js, it could be that you need this.messageManager.addMessageListener. We define _addMessageListener in specialpowers proper, but the observer is the observer service in the chrome space. Could that be the issue?

(In reply to Joel Maher (:jmaher) from comment #11) Thx for your help. I find the reason is that there is no implementation of _addMessageListener in ChromePowers.js[1]. [1] https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/ChromePowers.js#20

I was trying to get rid of ChromePowers.js in bug 1041563. ChromePowers.js is basically a stub, because specialpowers.js gave some problems in browser-chrome/chrome mochitests. But it should be possible to merge the code and then get rid of ChromePowers.js.

(In reply to Martijn Wargers [:mwargers] (QA) from comment #14) > I was trying to get rid of ChromePowers.js in bug 1041563. > ChromePowers.js is basically a stub, because specialpowers.js gave some > problems in browser-chrome/chrome mochitests. But it should be possible to > merge the code and then get rid of ChromePowers.js. Thanks for your message! I take a work-around solution now. I think this patch could be revised after ChromePowers.js is removed.

Comment on attachment 8602042 [details] [diff] [review] [v3] bug-1149868: move permissionObserver to SpecialPowersObserverand make it listen all 'perm-changed' signals to operate the undostack if specialpowers is in content process Hi, Joel Could you review this patch? p.s. I already tested on try-server[1]. [1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=d8e8a52c2024

Comment on attachment 8602042 [details] [diff] [review] [v3] bug-1149868: move permissionObserver to SpecialPowersObserverand make it listen all 'perm-changed' signals to operate the undostack if specialpowers is in content process Review of attachment 8602042 [details] [diff] [review]: ----------------------------------------------------------------- Ideally I would like the modifications to the test to be in a new file: test_SpecialPowersPushAppPermissions.html I believe that should be fairly easy to do. ::: testing/mochitest/tests/Harness_sanity/test_SpecialPowersPushPermissions.html @@ +160,5 @@ > + > +// Setup the prefrences and permissions > +function allowManagingApps() { > + SpecialPowers.pushPermissions([ > + //{ "type": "embed-apps", "allow": true, "context": document }, why is this commented out? ::: testing/specialpowers/components/SpecialPowersObserver.js @@ +129,5 @@ > var obs = Services.obs; > obs.removeObserver(this, "chrome-document-global-created"); > obs.removeObserver(this, "http-on-modify-request"); > obs.removeObserver(this, "xpcom-shutdown"); > + this._registerObservers._topics.forEach(function(element){ nit: space between ){ @@ +195,5 @@ > + this._topics.push(topic); > + Services.obs.addObserver(this, topic, false); > + } > + }, > + _sendAsyncMessageToSpecialpowers: function(topic, msg) { nit: capitalize powers -> _sendAsyncMessageToSpecialPowers @@ +203,5 @@ > + var msg = { aData: aData }; > + switch (aTopic) { > + case "perm-changed": > + var permission = aSubject.QueryInterface(Ci.nsIPermission); > + msg.permission = { appId: permission.appId, type: permission.type }; do we always have an appId available for a permission? I assume there are cases unrelated to webapps or firefoxOS where we would add a permission. @@ +205,5 @@ > + case "perm-changed": > + var permission = aSubject.QueryInterface(Ci.nsIPermission); > + msg.permission = { appId: permission.appId, type: permission.type }; > + default: > + this._sendAsyncMessageToSpecialpowers(aTopic, msg); why not just do: this._self._sendAsyncMessage('specialpowers-'+topic, msg); this would save an extra function that doesn't do much. Can we not just do: this._sendAsyncMessage(...); ? ::: testing/specialpowers/content/specialpowersAPI.js @@ +894,5 @@ > + observe: function (aSubject, aTopic, aData) > + { > + if (aTopic == "perm-changed") { > + var permission = aSubject.QueryInterface(Ci.nsIPermission); > + this._self._permissionObserver.observe(permission, aData); I would rather see _self be call _specialPowersAPI

(In reply to Joel Maher (:jmaher) from comment #17) > Comment on attachment 8602042 [details] [diff] [review] > [v3] bug-1149868: move permissionObserver to SpecialPowersObserverand make > it listen all 'perm-changed' signals to operate the undostack if > specialpowers is in content process > > Review of attachment 8602042 [details] [diff] [review]: > ----------------------------------------------------------------- Thanks for your review. > Ideally I would like the modifications to the test to be in a new file: > test_SpecialPowersPushAppPermissions.html > > I believe that should be fairly easy to do. I agree with you. > ::: > testing/mochitest/tests/Harness_sanity/test_SpecialPowersPushPermissions.html > @@ +160,5 @@ > > + > > +// Setup the prefrences and permissions > > +function allowManagingApps() { > > + SpecialPowers.pushPermissions([ > > + //{ "type": "embed-apps", "allow": true, "context": document }, > > why is this commented out? Oh, it's a typo! I don't use the mozApp iframe here, sp I will remove it. > ::: testing/specialpowers/components/SpecialPowersObserver.js > @@ +129,5 @@ > > var obs = Services.obs; > > obs.removeObserver(this, "chrome-document-global-created"); > > obs.removeObserver(this, "http-on-modify-request"); > > obs.removeObserver(this, "xpcom-shutdown"); > > + this._registerObservers._topics.forEach(function(element){ > > nit: space between ){ ok! > @@ +195,5 @@ > > + this._topics.push(topic); > > + Services.obs.addObserver(this, topic, false); > > + } > > + }, > > + _sendAsyncMessageToSpecialpowers: function(topic, msg) { > > nit: capitalize powers -> _sendAsyncMessageToSpecialPowers > > @@ +205,5 @@ > > + case "perm-changed": > > + var permission = aSubject.QueryInterface(Ci.nsIPermission); > > + msg.permission = { appId: permission.appId, type: permission.type }; > > + default: > > + this._sendAsyncMessageToSpecialpowers(aTopic, msg); > > why not just do: > this._self._sendAsyncMessage('specialpowers-'+topic, msg); > > this would save an extra function that doesn't do much. Can we not just do: > this._sendAsyncMessage(...); ? I will follow your advice to save an extra function. > @@ +203,5 @@ > > + var msg = { aData: aData }; > > + switch (aTopic) { > > + case "perm-changed": > > + var permission = aSubject.QueryInterface(Ci.nsIPermission); > > + msg.permission = { appId: permission.appId, type: permission.type }; > > do we always have an appId available for a permission? I assume there are > cases unrelated to webapps or firefoxOS where we would add a permission. The permission has an appId to be tracked by permissionManager. The appId here is to distinguish the owner of received perm-changed signal. Please consider the case like: 1. We install an app 2. We push the permissions to the app installed and the document the document's appId is 0, and the appId of appContext is assumed to be 1001. After pushing the following permissions: SpecialPowers.pushPermissions([{ "type": "pUnknow", "allow": true, "context": document }], callback()); SpecialPowers.pushPermissions([{ "type": "pUnknow", "allow": true, "context": appContext }], callback()); the UndoStack will have 2 rows and each contains a the same type permission item. 3. uninstall the app the app's permission(pUnknow) will be removed after uninstalling and we should remove the corresponding item(pUnknow) from UndoStack. If we only compare the permission's type with the item's type in UndoStack, we can not find which one should be deleted because two items have the same type. > ::: testing/specialpowers/content/specialpowersAPI.js > @@ +894,5 @@ > > + observe: function (aSubject, aTopic, aData) > > + { > > + if (aTopic == "perm-changed") { > > + var permission = aSubject.QueryInterface(Ci.nsIPermission); > > + this._self._permissionObserver.observe(permission, aData); > > I would rather see _self be call _specialPowersAPI OK!

Comment on attachment 8606185 [details] [diff] [review] [v4] move permissionObserver to SpecialPowersObserverand make it listen all 'perm-changed' signals to operate the undostack if specialpowers is in content process Review of attachment 8606185 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for addressing my previous comments! I do have a few questions, marking as r- for now, but could change to r+ as I understand my concerns via replies. ::: testing/mochitest/tests/Harness_sanity/test_SpecialPowersPushAppPermissions.html @@ +37,5 @@ > + SpecialPowers.pushPermissions([ > + { "type": "pAppPermission-4", "allow": true, "context": document }, > + { "type": "pAppPermission-6", "allow": true, "context": document }, > + ], allowManagingApps); > + } why do you split 1/3 and 4/6 up? @@ +86,5 @@ > + SpecialPowers.pushPermissions([ > + { "type": "pAppPermission-4", "allow": true, "context": context }, > + { "type": "pAppPermission-5", "allow": true, "context": context }, > + { "type": "pAppPermission-6", "allow": true, "context": context } > + ], testPermissionsForApp); why do you split 1/2/3 and 4/5/6 up? ::: testing/specialpowers/content/specialpowersAPI.js @@ +959,5 @@ > + undo.type == permission.type) { > + // Remove this undo item if it has been done by others(not > + // specialpowers itself.) > + undos.splice(j,1); > + --j; when we change undos and fiddle with the iterator 'j', does this work with a length of 1? when do we evaluate 'j < undos.length'? I assume undos.length is evaluated once when we enter the loop, but as we splice it and play with the iterator, we could get in a situation where we end up accessing undos[length] which would be undefined. This is also a concern for the outler loop with 'i' and this._self._permissionsUndoStack.

(In reply to Joel Maher (:jmaher) from comment #20) > Comment on attachment 8606185 [details] [diff] [review] > [v4] move permissionObserver to SpecialPowersObserverand make it listen all > 'perm-changed' signals to operate the undostack if specialpowers is in > content process > > Review of attachment 8606185 [details] [diff] [review]: > ----------------------------------------------------------------- > ::: > testing/mochitest/tests/Harness_sanity/test_SpecialPowersPushAppPermissions. > html > @@ +37,5 @@ > > + SpecialPowers.pushPermissions([ > > + { "type": "pAppPermission-4", "allow": true, "context": document }, > > + { "type": "pAppPermission-6", "allow": true, "context": document }, > > + ], allowManagingApps); > > + } > > why do you split 1/3 and 4/6 up? > > @@ +86,5 @@ > > + SpecialPowers.pushPermissions([ > > + { "type": "pAppPermission-4", "allow": true, "context": context }, > > + { "type": "pAppPermission-5", "allow": true, "context": context }, > > + { "type": "pAppPermission-6", "allow": true, "context": context } > > + ], testPermissionsForApp); > > why do you split 1/2/3 and 4/5/6 up? > Oh, It's my arbitrary test to demonstrate why I need to use appId to compare. Yes, it's indeed redundant. I change it to a simpler one in this patch. > ::: testing/specialpowers/content/specialpowersAPI.js > @@ +959,5 @@ > > + undo.type == permission.type) { > > + // Remove this undo item if it has been done by others(not > > + // specialpowers itself.) > > + undos.splice(j,1); > > + --j; > > when we change undos and fiddle with the iterator 'j', does this work with a > length of 1? when do we evaluate 'j < undos.length'? I assume undos.length > is evaluated once when we enter the loop, but as we splice it and play with > the iterator, we could get in a situation where we end up accessing > undos[length] which would be undefined. > > This is also a concern for the outler loop with 'i' and > this._self._permissionsUndoStack. I think it will be clearer if I use break-style here. I am testing this new patch on tryserver now. I might need your help after the testing. Thanks for your careful review! It make this work better!

Comment on attachment 8607967 [details] [diff] [review] [v5] move permissionObserver to SpecialPowersObserverand make it listen all 'perm-changed' signals to operate the undostack if specialpowers is in content process Review of attachment 8607967 [details] [diff] [review]: ----------------------------------------------------------------- please clean up the two nits and thanks for coming back with good patches. You can carry over the r+ and land/checkin-needed. ::: testing/specialpowers/content/specialpowersAPI.js @@ +949,5 @@ > + this._nextCallback = null; > + } > + } else { > + var find = false; > + for (var i = 0 ; !find && i < this._self._permissionsUndoStack.length ; i++) { two small nits: 1: change find -> found; small english difference 2: i=0; <- notice no space between 0 and ';'. This applies to the j loop below as well. I normally see: while(!found) { if (i == this._self._permissionsUndoStack.length) { found = true; break; } ... your version is fine as well!

Clear nits and carry r+ test on tryserver: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f13a331f64e7