Closed Bug 419357 Opened 13 years ago Closed 13 years ago

nsIExtensionManager::getInstallLocation can sometimes returns null (not expected/documented)

Categories

(Toolkit :: Add-ons Manager, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: standard8, Assigned: standard8)

Details

Attachments

(1 file)

nsIExtensionManager::getInstallLocation returns null in various instances:

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/toolkit/mozapps/extensions/src/nsExtensionManager.js.in&rev=1.275&mark=4137-4139,4162,4188#4114

This isn't documented:

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/toolkit/mozapps/extensions/public/nsIExtensionManager.idl&rev=1.55&mark=242-248#242

Given that I believe the generally accepted xpcom rules are that you don't return null, but throw an error, either we need to fix the code, or fix the documentation. I don't mind which, but would prefer the code as this has caused at least one crash (http://crash-stats.mozilla.com/report/index/94fb6970-db1d-11dc-96c0-001a4bd43ed6) due to the assumption that checking the nsresult is enough.
The part of the extension manager you reference in your first link isn't the function you are talking about. You actually want:

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/toolkit/mozapps/extensions/src/nsExtensionManager.js.in&rev=1.275&mark=5321#5313

The actual getInstallLocation should only return null if there is no extension installed with the id specified. This is I think a perfectly acceptable case to return null. There is no failure here, just the caller asking about a not-present extension.
Severity: major → normal
(In reply to comment #1)
> The part of the extension manager you reference in your first link isn't the
> function you are talking about. You actually want:
> http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/toolkit/mozapps/extensions/src/nsExtensionManager.js.in&rev=1.275&mark=5321#5313

Yes, I got that wrong, thanks for the correction.

> The actual getInstallLocation should only return null if there is no extension
> installed with the id specified. This is I think a perfectly acceptable case to
> return null. There is no failure here, just the caller asking about a
> not-present extension.

So you've asked the manager for an extension installation location and it's not able to get one (because you gave it an incorrect id), but you're going to return success (i.e. here's a location for you to use)?

Like I said before, as I understand the current xpcom rules, we try not to return null but throw a failure (which this is, alright, it was caused by the caller, but its still a failure to return a valid result).

Whilst it feels wrong for the code to return null, I'm happy to accept it (because its done in other places), but can we document it in the idl, so it is clear to developers when they implement the function that they must check for null and not failure.
So null + NS_OK is acceptable, but I still think it should be documented in this case. Here's the patch, doc only.
Assignee: nobody → bugzilla
Status: NEW → ASSIGNED
Attachment #305537 - Flags: review?(dtownsend)
I have installed the nightly build of FireFTP extension. Trying to install a newer nightly build gives an error.

Trying to first uninstall the current version gives the following error:
installLocation is Null
Line 3839 in nsExtensionManager.js
var installLocation = this.getInstallLocation(item.id);

I can't seem to be able to uninstall that add-on now.
Attachment #305537 - Flags: review?(dtownsend) → review+
(In reply to comment #4)
> I have installed the nightly build of FireFTP extension. Trying to install a
> newer nightly build gives an error.

Please file a separate bug for that, although it may be related, its not part of this bug.

(In reply to comment #3)
> Created an attachment (id=305537) [details]
> Document the null return option

Patch checked in -> fixed.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.