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

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Mark Banner (:standard8)]

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.

Comment 1

[Dave Townsend [:mossop]]

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.

Comment 2

[Mark Banner (:standard8)]

(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.

Comment 3

[Mark Banner (:standard8)]

Attachment: Document the null return option

So null + NS_OK is acceptable, but I still think it should be documented in this case. Here's the patch, doc only.

Comment 4

[Jose Fandos]

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.

Comment 5

[Mark Banner (:standard8)]

(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.