642156 - Validator messages for bootstrapped add-ons

Status: RESOLVED FIXED

(addons.mozilla.org Graveyard :: Developer Pages, enhancement, P4)

Description

[Kris Maglione [:kmag]]

User-Agent:       Mozilla/5.0 (X11; Linux x86_64; rv:2.0b13pre) Gecko/20110310 Firefox/4.0b13pre
Build Identifier: 

I think that it would be a good idea to flag certain actions in bootstrapped add-ons so that editors know to make sure that they're properly reversed at shutdown. Obviously the signal to noise ratio would be way too high if we flagged everything that could conceivably require cleanup, but I think the following should be safe:

• nsIComponentRegistrar.registerFactory
• nsICategoryManager.addCategoryEntry (especially when the 4th argument is not false)
• Observers: nsIObserverService.addObserver, nsIWindowWatcher.registerNotification, nsIPrefBranch2.addObserver, nsIWindowMediator.addListener
• nsIResProtocolHandler.setSubstitution
• nsIStyleSheetService.loadAndRegisterSheet
• nsIStringBundleService.createBundle, nsIStringBundleService.createExtensibleBundle
• nsIComponentRegistrar.autoRegister (this should never be used in bootstrapped extensions, for now)



Reproducible: Always

Comment 1

[Daniel Buchner [:dbuc]]

I believe this was filed under the wrong product, as the Add-ons SDK would be where your code execution occurs. The Builder just allows you to construct an add-on using the SDK, it has no ability to wrap actions at runtime/unload with the flags you are suggesting.

I am going to switch this ticket to the Add-on SDK.

Comment 2

[Kris Maglione [:kmag]]

Daniel: I may have misfiled it, but this bug is for the AMO validator and isn't especially related to SDK add-ons.

Comment 3

[Daniel Buchner [:dbuc]]

Oops, thought you were asking for the Add-ons SDK to wrap unload logging around those JS service calls if it came across them, sorry for the confusion!

Comment 4

[Wil Clouser [:clouserw]]

a?jorge

Comment 5

[Jorge Villalobos [:jorgev] (he/him)]

I'm in favor of this, but it's low priority.

Comment 6

[Kris Maglione [:kmag]]

If it's low priority, I can submit a patch. It should be fairly simple and I've been intending to modify the validator to run some particular tests on my code anyway.

Comment 7

[Jorge Villalobos [:jorgev] (he/him)]

Awesome, thanks.

Comment 8

[Kris Maglione [:kmag]]

Attachment: First draft proposed patch

Attaching first draft patch. Also fixes a bug that prevents errors from being reported for more than one XPCOM instance and adds handling for getService calls similar to getInstance calls.

This really requires that any call to QueryInterface or getInterface have the same semantics as Cc.*.createInstance to function adequately for all cases, but that's another bug I think.

Comment 9

[Kumar McMillan [:kumar]]

Hi Kris.  Thanks for the patch.  It looks pretty good to me but it needs some tests (unless I missed them).  Matt Basta can help you with that.  Can you submit a pull request on github? https://github.com/mozilla/amo-validator  There are a few other comments I have and Matt should r it as well.

Regarding printing out the traceback in the except block: I'd rather see that except block get more specific since, as you point out, it's a little too loose right now.

Comment 10

[Kris Maglione [:kmag]]

Kumar: I have informal tests but I wanted to get feedback before I formalize them. Especially relating to the lack of QI support, since the final tests will depend on it and fail without it.

And re the traceback, I think that the exception should at least be printed in all cases so that we know something unexpected is going on (a single typo was causing silent failures for me), but it would probably be better to also trigger a test failure.

Comment 11

[Matt Basta [:basta]]

r+ for the patch, didn't see anything off the wall.

Let me know if/when you want me to pull it in.

Comment 12

[Kris Maglione [:kmag]]

Attachment: Patch: Add QueryInterface/getInterface support with some minor refactoring

Argh. I see you merged already.

This patch adds QueryInterface and getInterface support for XPCOM interface tests. It also refactors part of the previous patch to better match the style of the rest of the file. Let me know if this is alright and I'll add formal tests.

Comment 13

[Matt Basta [:basta]]

No worries.

r+; I like what you did with the xpcom_constructor stuff.

Also, you might find the work on doing on my javascript-newhooks branch useful/helpful:

https://github.com/mattbasta/amo-validator/commit/3f360614dc29e0fd33658165fd4b30535ba0f423

Comment 14

[Kris Maglione [:kmag]]

Yes, that definitely does look useful. Given that, I might actually make QueryInterface additive/mutative which I was going to leave for you in another bug. :)

Comment 15

[Kris Maglione [:kmag]]

I have four relevant commits in a fork:

This adds QueryInterface support:

https://github.com/kmaglione/amo-validator/commit/eb2e1d0f11a0815d1310e4b97cb945d5ed4a957c

This makes QueryInterface additive and mutative. I added it as a separate changeset because I think it may be slightly dodgy and should be reviewed separately:

https://github.com/kmaglione/amo-validator/commit/10ce51a444ac90dbc86d68126828c39179d24638

These adds tests (and fix a small bug):

https://github.com/kmaglione/amo-validator/commit/557350d511b04f85f46b0693fa5769ebd3468793
https://github.com/kmaglione/amo-validator/commit/bd3b8758fe36271e683d1af65c5d5bf73bcb8bee

Comment 16

[Matt Basta [:basta]]

They look good to me, but I don't do official code reviews. Feel free to make a pull request and one of the Python Gods will reach down with their noodley appendages and inspect your work.

Comment 17

[Kris Maglione [:kmag]]

https://github.com/mozilla/amo-validator/pull/12

Comment 18

[Wil Clouser [:clouserw]]

(In reply to comment #16)
> They look good to me, but I don't do official code reviews. Feel free to make a
> pull request and one of the Python Gods will reach down with their noodley
> appendages and inspect your work.

You're welcome to review code!  Just so long as two eyes are on it, that's really what I'm after.  Please do.

Comment 19

[Wil Clouser [:clouserw]]

er, two *sets* of eyes.  four total please. :)

Comment 20

[Matt Basta [:basta]]

Reviewed and pushed. Looks good!

Comment 21

[Jorge Villalobos [:jorgev] (he/him)]

Reclassifying editor bugs and changing to a new whiteboard flag. Spam, spam, spam, spam...