333307 - signature is wrong for nsComponentManagerImpl::RegisterComponent and nullchecks are improperly handled

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[timeless]

found by coverity

this can be fixed with bug 333304. and it's possible i've already filed a bug about the signature problem since i recall changing it in some tree.

the signature problem is this:
if something is an interface reachable method, which RegisterComponent is, then it should be NS_IMETHODIMP. if it's private, it can be nsresult or whatever it's declared to be. This method is declared in idl and therefore it should be properly tagged. which among other things allows one to clearly recognize that this boundary is not being properly handled.

Comment 1

[Nickolay_Ponomarev]

Attachment: fix signatures and prevent the crash using NS_ENSURE_ARG_POINTER

There was one [notxpcom] function which I didn't quite understand, so I didn't change the nsresult for it: http://landfill.mozilla.org/mxr-test/mozilla/source/xpcom/components/nsIComponentManagerObsolete.idl#77

Comment 2

[timeless]

Comment on attachment 217778 [details] [diff] [review]
fix signatures and prevent the crash using NS_ENSURE_ARG_POINTER

since this used to crash in that case anyway, this sounds reasonable to me.

Comment 3

[Nickolay_Ponomarev]

Attachment: fix signatures (+1 more) and prevent the crash using NS_ENSURE_ARG_POINTER

timeless suggested to use NS_IMETHODIMP for the notxpcom function too (because xpidl defines it as NS_IMETHOD in the generated header)

Comment 4

[Darin Fisher]

Comment on attachment 217791 [details] [diff] [review]
fix signatures (+1 more) and prevent the crash using NS_ENSURE_ARG_POINTER

Hmm, the fact that our calling convention for nsIComponentManagerObsolete has been wrong for so long, doesn't that suggest that we could just remove it?  Or, is this code trying to emmulate an older calling convention?

Comment 5

[Benjamin Smedberg]

NS_IMETHODIMP on the implementation doesn't make any difference, as long as the function is declared correctly: the declaration controls the calling convention.

Comment 6

[Darin Fisher]

Comment on attachment 217791 [details] [diff] [review]
fix signatures (+1 more) and prevent the crash using NS_ENSURE_ARG_POINTER

good, sr=darin

Comment 7

[Martijn Wargers (dead)]

Checking in xpcom/components/nsComponentManager.cpp;
/cvsroot/mozilla/xpcom/components/nsComponentManager.cpp,v  <--  nsComponentMana
ger.cpp
new revision: 1.278; previous revision: 1.277
done

Comment 8

[Nickolay_Ponomarev]

For reference, this patch accidently included a part from bug 333303.