685652 - we need a pushPermissionsEnv equivalent to pushPrefEnv in SpecialPowers

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Joel Maher ( :jmaher ) (UTC -8)]

in bug 621363, we add pushPrefEnv and this patch:
https://bugzilla.mozilla.org/attachment.cgi?id=544087

has an example of doing the same for permissions.

Comment 1

[Josh Matthews [:jdm]]

Note that the patch linked uses a nested event loop, which is bad. We want to duplicate the semantics of pushPrefEnv instead.

Comment 2

[Jason Smith [:jsmith]]

Clint - Getting automation for the install/update apps API for b2g is blocked on this (unless you know of a way to workaround the problem presented in bug 826058). Can you help find an assignee on this bug to look into this?

Comment 3

[Jonathan Griffin (:jgriffin)]

See also bug 846057 which deals with a SpecialPowers API for getting permissions.

Comment 4

[cmtalbert]

Ted and Joel, would one of you have a chance to take a look at this?

Comment 5

[(not currently active) Ted Mielczarek]

I asked about this in bug 826058 comment 27 when I was reviewing the patch there. I don't actually know how this stuff works, but assuming we can get a notification when a value has been set (like we can for preferences), this should be straightforward to add.

Comment 6

[Josh Matthews [:jdm]]

http://mxr.mozilla.org/mozilla-central/source/extensions/cookie/nsPermissionManager.cpp?force=1#1402

Comment 7

[[:fabrice] Fabrice Desré]

Do we still need this now that bug 846057 landed?

Comment 8

[Jason Smith [:jsmith]]

(In reply to Fabrice Desré [:fabrice] from comment #7)
> Do we still need this now that bug 846057 landed?

At a lesser priority, yes - if we build something similar to pushPrefEnv, we could greatly keep our tests simpler for "always run tests with these perms."

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

It'd still be nice to have this function, and have it synchronously modify the permission both in the child and the parent process.

But I think the more urgent question is if this blocks bug 826058 still. I think that is something we'll have to answer in bug 826058.

Comment 10

[Jason Smith [:jsmith]]

(In reply to Jonas Sicking (:sicking) from comment #9)
> It'd still be nice to have this function, and have it synchronously modify
> the permission both in the child and the parent process.
> 
> But I think the more urgent question is if this blocks bug 826058 still. I
> think that is something we'll have to answer in bug 826058.

Fabrice mentions in IRC that we actually do still need this bug to land the webapps tests.

Comment 11

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: add in pushPermissions to SpecialPowers (1.0)

this patch works well and I have converted an existing test case to use this.

Comment 12

[[:fabrice] Fabrice Desré]

Comment on attachment 727812 [details] [diff] [review]
add in pushPermissions to SpecialPowers (1.0)

Review of attachment 727812 [details] [diff] [review]:
-----------------------------------------------------------------

That looks ok to me, but I'd like someone more familiar with SpecialPowersAPI to review instead.

::: content/base/test/test_XHR_anon.html
@@ +170,5 @@
> +function setup() {
> +  am.init();
> +  //SpecialPowers.addPermission("systemXHR", true, document);
> +  SimpleTest.waitForExplicitFinish();
> +  SpecialPowers.pushPermissions([["systemXHR", true, document]], runTests);

I find the syntax here unintuitive. What about using an array of objects:
{ permission: "systemXHR", XXX: true, YYY: document }

::: testing/mochitest/tests/SimpleTest/TestRunner.js
@@ +480,4 @@
>      }
>  
>      SpecialPowers.executeAfterFlushingMessageQueue(function() {
> +        cleanUpCrashDumpFiles();        

nit: trailing whitespace

Comment 13

[Josh Matthews [:jdm]]

Comment on attachment 727812 [details] [diff] [review]
add in pushPermissions to SpecialPowers (1.0)

Review of attachment 727812 [details] [diff] [review]:
-----------------------------------------------------------------

I agree with Fabrice's comments.

::: content/base/test/test_XHR_anon.html
@@ +38,4 @@
>    },
>  }
>  
> +//var tests = [ test1, test2, test2a, test3, test3, test3, test4, test4, test4, test5, test5, test5 ];

Nix.

@@ +168,5 @@
>  }
>  
> +function setup() {
> +  am.init();
> +  //SpecialPowers.addPermission("systemXHR", true, document);

Nix.

::: testing/specialpowers/content/specialpowersAPI.js
@@ +497,4 @@
>      return crashDumpFiles;
>    },
>  
> +  pushPermissions: function(inPermissions, callback) {

Please document the parameters, especially the format of inPermissions.

@@ +505,5 @@
> +        var permission = inPermissions[p];
> +        var originalValue = Ci.nsIPermissionManager.UNKNOWN_ACTION;
> +        if (this.testPermission(permission[0], Ci.nsIPermissionManager.ALLOW_ACTION, permission[2])) {
> +          originalValue = Ci.nsIPermissionManager.ALLOW_ACTION;
> +        } else if (this.testPermission(permission[0], Ci.nsIPermissionManager.DENY_ACTION, permission[2])) {

Having to make two sync IPC requests to determine that we don't have a stored permission is pretty awful; I'm not sure why the SpecialPowers testPermission API is designed the way it is. Oh well.

@@ +561,5 @@
> +        }
> +        content.window.setTimeout(delayAgain, 0);
> +      }
> +      let cb = callback ? delayedCallback : null;
> +      /* Each pop will have a valid block of preferences */

I don't understand what this means.

@@ +577,5 @@
> +    this.popPermissions(callback);
> +  },
> +
> +
> +  permissionObserver: {

_permissionObserver

@@ +621,5 @@
> +    this.permissionObserver._nextCallback = function () {
> +            self._applyingPermissions = false;
> +            // Now apply any permissions that may have been queued while we were applying
> +            self._applyPermissions();
> +          }

Indentation.

Comment 14

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: add in pushPermissions to SpecialPowers (2.0)

updated with review comments, including documenting the parameters and switching to an object instead of an array.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Backed out for possibly causing OSX 10.6 mochitest-a11y failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/f22ec75a02d7

https://tbpl.mozilla.org/php/getParsedLog.php?id=21171998&tree=Mozilla-Inbound

Comment 16

[Luqman Aden [:Luqman]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #15)
> Backed out for possibly causing OSX 10.6 mochitest-a11y failures.
> https://hg.mozilla.org/integration/mozilla-inbound/rev/f22ec75a02d7
> 
> https://tbpl.mozilla.org/php/getParsedLog.php?id=21171998&tree=Mozilla-
> Inbound

I don't see how this patch could be causing those failures since it doesn't seem to touch chrome://mochitests/content/a11y/accessible/states/test_tree.xul at all.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

It does affect the overall harness, though. And indeed, the problem did go away after this backout.

Comment 18

[[:fabrice] Fabrice Desré]

This is because we call SimpleTest.finish() explicitly for OSX 10.6 only here:
https://mxr.mozilla.org/mozilla-central/source/accessible/tests/mochitest/states/test_tree.xul#98

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/68a3cd154732