396119 - Trivial errors in XPCOMUtils.jsm

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Manish Singh]

Attachment: Fixes for errors

I've noticed two problems in XPCOMUtils.jsm:

* The error result logic in getClassObject is reversed
* The call to catMan.deleteCategoryEntry is missing its 3rd argument, so that'll fail.

Patch attached fixes both.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Can you fix http://lxr.mozilla.org/seamonkey/source/js/src/xpconnect/loader/XPCOMUtils.jsm#241 too while you're at it (CR not being defined)? :)

Comment 2

[Manish Singh]

Attachment: New patch, fixes the 3rd issue brought up too

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

I don't get why the the throws are reversed...

If you ask for an interface that isn't implemented (like in QI, for example), you get NO_INTERFACE.

If you then ask for a class that isn't in the list you throw NOT_IMPLEMENTED. How is that incorrect?

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 280910 [details] [diff] [review]
New patch, fixes the 3rd issue brought up too

The nsGenericFactory code only validates aIID by calling QI on the factory object it creates, which means a failure would result in NS_ERROR_NO_INTERFACE if it failed (http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/xpcom/glue/nsGenericFactory.cpp&rev=1.66&mark=376#346 ). However, I think the sample code is throwing NS_ERROR_NOT_IMPLEMENTED in that case because it's trying to indicate that it doesn't support returning nsIClassInfos (only nsIFactories), which seems reasonable. A comment to explain would help, though.

Throwing NS_ERROR_FACTORY_NOT_REGISTERED, or sticking with the current NS_ERROR_NOT_IMPLEMENTED seems like the way to go for a cid mismatch. That needs to be changed in the patch (also in the sample).

The rest of the patch looks good.

Comment 5

[Manish Singh]

Attachment: New patch, hopefully addresses confusion

Per discussion with Gavin on IRC, throwing NS_ERROR_NOT_IMPLEMENTED if we don't get an iid of nsIFactory makes sense, since getClassInfo can give you an nsIFactory or an nsIClassInfo, but this code doesn't handle the later case, so we throw NOT_IMPLEMENTED.

Per discussion with Shaver, he says to mirror the C++ version:
http://mxr.mozilla.org/seamonkey/source/xpcom/glue/nsGenericFactory.cpp#380

So I'm throwing NS_ERROR_FACTORY_NOT_REGISTERED in case none of the cids match.

Once there's a satisfactory review, I'll make a bug to make the xpcom JS samples match whatever finally goes in here.

Comment 6

[Manish Singh]

Heh, Gavin commented just as I was attaching the new patch.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 281371 [details] [diff] [review]
New patch, hopefully addresses confusion

Exactly! :)

Thanks!

Comment 8

[Matthew (lilmatt) Willis]

Patch landed on trunk

-> FIXED