1135772 - Null out the return value on error in QI in nsXPCWrappedJS

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Andrew McCreight [:mccr8]]

From bug 723248:

(In reply to David :Bienvenu from comment #12)
> I did a quick look through the mailnews and mozilla code, and the mailnews
> code is all OK (once I fixed nsMsgDatabase.cpp) and almost all the mozilla
> code looks fine, though there are a few suspicious things:
> 
> nsXPCWrappedJS::QueryInterface return an error without nulling the out param
> if !isValid().
> 
> AudioSession::QueryInterface() fails to null the out param on error.
> 
> same for ReadbackManagerD3D10::QueryInterface and
> COMMessageFilter::QueryInterface
> 
> nsXTFElementWrapper::QueryInterface is a little suspicious

Comment 1

[Andrew McCreight [:mccr8]]

AudioSession and COMMessageFilter look like they are not XPCOM classes, so I will leave them alone.  I'm guessing they are regular Windows COM classes.  ReadbackManagerD3D10 does not seem to exist, but I assume it was similar.  nsXTFElementWrapper seems to not exist any more, so maybe this can just be about the nsXPCWrappedJS QI.

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: Return null on failure in nsXPCWrappedJS's QueryInterface.

try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=22b490430a79

This is in XPConnect, but just some QI changes, so maybe you can review.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8568241 [details] [diff] [review]
Return null on failure in nsXPCWrappedJS's QueryInterface.

> nsXPCWrappedJS::QueryInterface(REFNSIID aIID, void** aInstancePtr)
> {
>+    *aInstancePtr = nullptr;
>+
>     if (nullptr == aInstancePtr) {
>-        NS_PRECONDITION(0, "null pointer");
>+        NS_PRECONDITION(false, "null pointer");
>         return NS_ERROR_NULL_POINTER;
>     }
if (nullptr == aInstancePtr) {
makes now no sense.

So, move 
*aInstancePtr = nullptr; after the 'if' check or perhaps just remove
the 'if'

Comment 4

[Andrew McCreight [:mccr8]]

Good point.  For some reason I failed to realize that was the return value.  I just moved it later to avoid possibly triggering a null deref in some random place.

https://hg.mozilla.org/integration/mozilla-inbound/rev/448d1795215f

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/448d1795215f