Closed Bug 746908 Opened 13 years ago Closed 12 years ago

Add parameter guards to AddonManager/AddonManagerPrivate API functions

Categories

(Toolkit :: Add-ons Manager, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: Unfocused, Assigned: Unfocused)

References

Details

(Whiteboard: [fixed-in-fx-team])

Attachments

(1 file)

We currently don't do much error checking for the parameters passed into the API functions of AddonManager and AddonManagerPrivate. This can lead to some unexpected behaviour. Two issues I found by adding guards: * test_plugins.js was passing a string instead of an array to getAddonsByTypes() - the test was passing by mere coincidence * update.js was passing undefined as the addon ID to addStartupChange()
Attached patch Patch v1 (deleted) — Splinter Review
Still to do: tests. I should at least test the update.js change - kinda worrying that that slipped though. Would be good to get review of the existing changes anyway though.
Assignee: nobody → bmcbride
Status: NEW → ASSIGNED
Attachment #616491 - Flags: review?(dtownsend+bugmail)
Comment on attachment 616491 [details] [diff] [review] Patch v1 Review of attachment 616491 [details] [diff] [review]: ----------------------------------------------------------------- We have a bit of a mix between using Error and Components.Exception to create errors in this code now, don't know if you want to make all that consistent but I don't think it needs to block this.
Attachment #616491 - Flags: review?(dtownsend+bugmail) → review+
(Yay catching up on bugs) (In reply to Dave Townsend (:Mossop) from comment #2) > We have a bit of a mix between using Error and Components.Exception to > create errors in this code now, don't know if you want to make all that > consistent but I don't think it needs to block this. Yea, was gonna do that in a separate bug (filed bug 759642). There's the odd case where we throw a value from Components.results without wrapping it in anything too (it's problem all over chrome code), which is equivalent to |throw 1234;|
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla15
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Blocks: 807460
If the parameters passed fail the check, it seems to block automatic add-on update checks. I had this problem with Stylish which was passing a number rather than a string. It manifested itself to me as a large drop in install counts on AMO. I think that this should be revised so that a failure doesn't blow up automatic add-on updates.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: