Closed Bug 674255 Opened 9 years ago Closed 8 years ago

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

Categories

(Testing :: Mochitest, defect)

x86
Windows Vista
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla10

People

(Reporter: imelven, Assigned: imelven)

References

Details

(Keywords: dev-doc-needed, Whiteboard: [inbound])

Attachments

(1 file, 6 obsolete files)

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)
I cover this in bug 668283.
(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 !
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 668283
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
See Also: → 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
Attachment #557985 - Flags: review?(ted.mielczarek)
Ted, please route to another reviewer if you're not the right person, i guessed based on hg log :)
updated for bitrot
Attachment #557985 - Attachment is obsolete: true
Attachment #557985 - Flags: review?(ted.mielczarek)
Attachment #559349 - Flags: review?(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.
Attachment #559349 - Flags: review?(ted.mielczarek) → review-
Assignee: nobody → imelven
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")|.
Yeah, I like that.
Attached patch patch v3 (obsolete) — Splinter Review
switched to getPrivilegedProps after review and discussion, thanks !
Attachment #559349 - Attachment is obsolete: true
Attachment #562930 - Flags: review?(ted.mielczarek)
Attached patch patch v4 (obsolete) — Splinter Review
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.
Attachment #562930 - Attachment is obsolete: true
Attachment #562930 - Flags: review?(ted.mielczarek)
Attachment #563491 - Flags: review?(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.
I never actually wrote a version that used "a.b.c", but as mentioned, splitting is easy.
Keywords: dev-doc-needed
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.
Attachment #563491 - Flags: review?(ted.mielczarek) → review+
have the split change ready to go, will throw together some tests for the non-observer things and send for feedback.
Attached patch patch v5 (obsolete) — Splinter Review
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 ? )
Attachment #563491 - Attachment is obsolete: true
Attachment #563891 - Flags: feedback?(ted.mielczarek)
(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!
Attached patch patch v6 (obsolete) — Splinter Review
unbitrotted for a try push, ted, do these tests make sense ?
Attachment #563891 - Attachment is obsolete: true
Attachment #563891 - Flags: feedback?(ted.mielczarek)
Attachment #564406 - Flags: feedback?(ted.mielczarek)
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 ? )
None of the oranges look like your patch caused them.
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 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
Attachment #564406 - Flags: feedback?(ted.mielczarek) → feedback+
Attached patch patch v7Splinter Review
unbitrot including move of special powers API's to specialpowersAPI.js, ready to land
Attachment #564406 - Attachment is obsolete: true
Attachment #566933 - Attachment is patch: true
should be ready to land, marking checkin-needed
Keywords: checkin-needed
Flags: in-testsuite+
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 !
Attachment #566933 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5f9ff53ccaab
Status: REOPENED → RESOLVED
Closed: 9 years ago8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
You need to log in before you can comment on or make changes to this bug.