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)
Toolkit
Add-ons Manager
Tracking
()
RESOLVED
FIXED
mozilla15
People
(Reporter: Unfocused, Assigned: Unfocused)
References
Details
(Whiteboard: [fixed-in-fx-team])
Attachments
(1 file)
(deleted),
patch
|
mossop
:
review+
|
Details | Diff | Splinter Review |
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()
Assignee | ||
Comment 1•13 years ago
|
||
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 2•13 years ago
|
||
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+
Assignee | ||
Comment 3•12 years ago
|
||
(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;|
Assignee | ||
Comment 4•12 years ago
|
||
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla15
Comment 5•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment 6•11 years ago
|
||
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.
Description
•