Closed Bug 333307 Opened 14 years ago Closed 14 years ago

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

Categories

(Core :: XPCOM, defect, critical)

x86
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED

People

(Reporter: timeless, Assigned: asqueella)

References

(Blocks 1 open bug, )

Details

(Keywords: coverity, crash)

Attachments

(1 file, 1 obsolete file)

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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
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
Assignee: nobody → asqueella
Status: NEW → ASSIGNED
Attachment #217778 - Flags: review?(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.
Attachment #217778 - Flags: superreview?(darin)
Attachment #217778 - Flags: review?(timeless)
Attachment #217778 - Flags: review+
timeless suggested to use NS_IMETHODIMP for the notxpcom function too (because xpidl defines it as NS_IMETHOD in the generated header)
Attachment #217778 - Attachment is obsolete: true
Attachment #217791 - Flags: superreview?(darin)
Attachment #217791 - Flags: review+
Attachment #217778 - Flags: superreview?(darin)
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?
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 on attachment 217791 [details] [diff] [review]
fix signatures (+1 more) and prevent the crash using NS_ENSURE_ARG_POINTER

good, sr=darin
Attachment #217791 - Flags: superreview?(darin) → superreview+
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
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
For reference, this patch accidently included a part from bug 333303.
You need to log in before you can comment on or make changes to this bug.