674255 - add API to SpecialPowers that allows a callback from an NSObserverService observation

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Ian Melven :imelven]

the Mochitests for CSP use nsObserverService to determine when CSP has blocked something from being loaded. this requires the use of enablePrivilege('UniversalXPConnect') which we want to move away from. instead we use the SpecialPowers object. this means for these tests SpecialPowers needs an API that lets you set a callback when a particular event is observed.

i discussed this briefly with sicking and we agreed on an API along the lines of SpecialPowers.addObserver(<thing to observe>, <callback function in mochitest>)

a SpecialPowers.removeObserver() is required also, as each test must clean up its observers before exiting (to avoid leaks during test suite runs)

Comment 1

[Josh Matthews [:jdm]]

I cover this in bug 668283.

Comment 2

[Ian Melven :imelven]

(In reply to comment #1)
> I cover this in bug 668283.

awesome, i'll make this a duplicate of that bug and mark the other CSP bugs dependent on bug 668283. Thanks !

Comment 4

[Ian Melven :imelven]

Attachment: port the needed piece of SpecialPowers.js from jdm's patch for 668283

this is a piece of jdm's work in bug 668283 broken out so i can try to land CSP bugs that depend on this

Comment 5

[Ian Melven :imelven]

Ted, please route to another reviewer if you're not the right person, i guessed based on hg log :)

Comment 6

[Ian Melven :imelven]

Attachment: port the needed piece of SpecialPowers.js from jdm's patch for 668283

updated for bitrot

Comment 7

[(not currently active) Ted Mielczarek]

Comment on attachment 559349 [details] [diff] [review]
port the needed piece of SpecialPowers.js from jdm's patch for 668283

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

::: testing/mochitest/specialpowers/content/specialpowers.js
@@ +423,5 @@
> +  can_QI: function(obj) {
> +    return obj.QueryInterface !== undefined;
> +  },
> +   
> +  do_QueryInterfaceAndGet: function(obj, iface, props) {

I know you just ported this from jdm's patch, but this API is really confusing to me. I had to go look up the example usage in his other patch to figure out what exactly it did.

I don't have a problem with exposing do_QueryInterface, but we need to figure out a saner way to expose "get some set of properties off of this object" than the API here.

Comment 8

[Josh Matthews [:jdm]]

The API for getPrivilegedProps that I came up with in one of my other many patches is probably a better choice here in conjunction with do_QueryInterface. The result would look like |getPrivilegedProps(do_QueryInterface(obj, Ci.nsIFoo), "thing.stuff.foo")|.

Comment 9

[(not currently active) Ted Mielczarek]

Yeah, I like that.

Comment 10

[Ian Melven :imelven]

Attachment: patch v3

switched to getPrivilegedProps after review and discussion, thanks !

Comment 11

[Ian Melven :imelven]

Attachment: patch v4

i tested out the failure case in the CSP bug depending on this and found i messed up getPrivilegedProps in the previous patch, this one works as desired.

Comment 12

[(not currently active) Ted Mielczarek]

Comment on attachment 563491 [details] [diff] [review]
patch v4

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

::: testing/mochitest/specialpowers/content/specialpowers.js
@@ +417,5 @@
> +  removeObserver: function(obs, notification) {
> +    var obsvc = Cc['@mozilla.org/observer-service;1']
> +                   .getService(Ci.nsIObserverService);
> +    obsvc.removeObserver(obs, notification);
> +  },

These look good, I'm just wondering if we'll need a more e10s version of this in the future: something that lets you add/remove observers in the parent process, with forwarding of the notifications.

@@ +431,5 @@
> +  getPrivilegedProps: function(obj, props) {
> +    let i;
> +	
> +    for (i = 0; i < props.length; i++)
> +	  obj = obj[props[i]];

This isn't the same API that jdm talked about in comment 8. Did he just misremember what he had proposed? I really think I'd prefer passing "foo.bar" instead of ["foo", "bar"]. It's only a split away, shouldn't be that hard.

@@ +439,5 @@
> +        return new String(obj);
> +      default:
> +        return obj;
> +    }
> +  }

Could you add a simple test for some of these APIs to the SpecialPowers sanity tests?
http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/test_SpecialPowersExtension.html?force=1

It's probably a pain to test add/removeObserver, but at least the other ones would be nice.

Comment 13

[Josh Matthews [:jdm]]

I never actually wrote a version that used "a.b.c", but as mentioned, splitting is easy.

Comment 14

[(not currently active) Ted Mielczarek]

Comment on attachment 563491 [details] [diff] [review]
patch v4

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

r=me if you change the getPrivilegedProps API to take a string like "foo.bar" instead of a list.

Comment 15

[Ian Melven :imelven]

have the split change ready to go, will throw together some tests for the non-observer things and send for feedback.

Comment 16

[Ian Melven :imelven]

Attachment: patch v5

changed to "foo.bar" syntax for getPrivilegedProps, added some tests for the non-observer stuff added to SpecialPowers. Ted, can you take a quick look at the tests and see if they make sense please ? :) If all looks good, this should be ready to land (i maybe should try pushing them to Try as well too ? )

Comment 17

[cmtalbert]

(In reply to Ian Melven :imelven from comment #16)
> Created attachment 563891 [details] [diff] [review] [diff] [details] [review]
> patch v5
> 
> changed to "foo.bar" syntax for getPrivilegedProps, added some tests for the
> non-observer stuff added to SpecialPowers. Ted, can you take a quick look at
> the tests and see if they make sense please ? :) If all looks good, this
> should be ready to land (i maybe should try pushing them to Try as well too
> ? )

Hi Ian, thanks for working on these patches.  We've noticed failures in try that we haven't seen locally with special powers APIs, so it would be good to run these changes through try while you wait for review, and see how things go.  thanks again!

Comment 18

[Ian Melven :imelven]

Attachment: patch v6

unbitrotted for a try push, ted, do these tests make sense ?

Comment 19

[Ian Melven :imelven]

https://tbpl.mozilla.org/?usebuildbot=1&tree=Try&rev=a1354ace2df7

lots of orange, a couple red and a purple

i could use some help figuring out if any of this is expected or caused by my changes (i'm assuming the infrastructure exception isn't ? )

Comment 20

[Josh Matthews [:jdm]]

None of the oranges look like your patch caused them.

Comment 21

[Ian Melven :imelven]

discussed with jdm on IRC and learned a lot of very useful tips about working with tbpl and try ! if these tests seem ok, this should be ready to land, I think.

Comment 22

[(not currently active) Ted Mielczarek]

Comment on attachment 564406 [details] [diff] [review]
patch v6

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

Looks good, thanks! Please document these new methods on MDN once you land:
https://developer.mozilla.org/en/SpecialPowers

Comment 23

[Ian Melven :imelven]

Attachment: patch v7

unbitrot including move of special powers API's to specialpowersAPI.js, ready to land

Comment 24

[Ian Melven :imelven]

should be ready to land, marking checkin-needed

Comment 25

[Ian Melven :imelven]

Comment on attachment 566933 [details] [diff] [review]
patch v7

carrying over r+ from ted as the patch is the same apart from the file the new apis are in - let me know if this isn't kosher please !

Comment 26

[Brandon Sterne (:bsterne)]

Pushed to mozilla-inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/5f9ff53ccaab

Comment 27

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5f9ff53ccaab