43282 - nsIInterfaceInfo::GetIIDForParam fails when referencing non scriptable interface

Status: VERIFIED FIXED

(Core :: XPCOM, defect, P3)

Description

[Mark Hammond [:markh] [:mhammond]]

When a typelib references another interface (via an interface forward declared 
in the IDL file), and that referenced interface is not scriptable, a crash 
occurs.

Consider the following IDL:

-- nsICrashTest.idl --
/* An interface that demonstrates a crash */
#include "nsISupports.idl"

interface nsIEventQueue;

[scriptable, uuid(2CC5D558-F337-45f9-86B8-11E8FBAEF3E6)]
interface nsICrashTestInterface : nsISupports
{
  void Test (in nsIEventQueue destQueue);
};

%{ C++
%}
-- eof -- 

nsIEventQueue is not scriptable, but is referenced in the only method.  
Attempting to get the IID for this param causes the crash, as demonstrated in 
the C++ program attached at the end.

The cause of the crash is in xptInterfaceInfo.cpp - the last line of the 
function in question:
>     return theInfo->GetIID(iid);

theInfo is NULL at this point, as the interface info for the non-scriptable 
interface has not been able to be resolved.  I would submit a patch, but I was 
unsure of the specific error result to return.

Im happy to provide this test case as mail attachments, including the makefile.

-- xpt_crash.cpp --
#include <stdio.h>
#include <nsIXPConnect.h>
#include <nsIAllocator.h>
#include <nsXPIDLString.h>
#include <nsIServiceManager.h>
#include <nsCRT.h>
#include <xptcall.h>
#include <xpt_xdr.h>


int 
main(int argc, char **argv)
{
	nsresult rv;
	rv = NS_InitXPCOM(nsnull, nsnull);
	NS_ABORT_IF_FALSE(NS_SUCCEEDED(rv), "Cant init");

	// Get the interface info manager.
	nsCOMPtr<nsIInterfaceInfoManager> iim = XPTI_GetInterfaceInfoManager();
	NS_ABORT_IF_FALSE(iim, "no iim");

	// Get the interface info for our test interface.
	nsCOMPtr<nsIInterfaceInfo> ii;
	rv = iim->GetInfoForName("nsICrashTestInterface", getter_AddRefs(ii));
	NS_ABORT_IF_FALSE(NS_SUCCEEDED(rv), "no interface info");

	// Method index == 3 - 0, 1 and 2 are from nsISupports.
	uint16 method_index = 3;
	// Get the method info for the first method
	const nsXPTMethodInfo *pmi;
	rv = ii->GetMethodInfo(method_index, &pmi);
	NS_ABORT_IF_FALSE(NS_SUCCEEDED(rv), "no method info info");

	// Get the param info
	const nsXPTParamInfo& param_info = pmi->GetParam(0);

	// Now get the IID.
	nsIID *iid;
	ii->GetIIDForParam(method_index, &param_info, &iid);
	return 0;
}
-- eof --

Comment 1

[Mark Hammond [:markh] [:mhammond]]

My apols for inlining rather than attaching the test case - this is my first 
Mozilla bug.

After looking at the code in more detail, it is apparent that GetIIDForParam is 
basically a clone of GetInfoForParam.

I have attached a proposed patch that resolves this bug, and also reduces the 
size of the code.  Alternatively, the trivial NULL pointer check change (no 
patch attached) would also obviously be acceptable.

Comment 2

[Mark Hammond [:markh] [:mhammond]]

Attachment: Proposed patch that also reduces duplicate code.

Comment 3

[Mike McCabe]

Adding myself to the CC.  I'd be happy to review this fix.

(I'm out of town 'till Tuesday, though.)

Comment 4

[Mike McCabe]

Confirming bug.  Ray - I can also take this one on, if you like.

Comment 5

[Ray Whitmer]

Sorry, I've been out.  Mike, I'd appreciate it if you would review the proposed
fix.  Thanks.

Comment 6

[Ray Whitmer]

Reassigning to Mike McCabe who volunteered and has a better handle on it than I.

Comment 7

[Mike McCabe]

Narrow, bulletproofing fix.  It's likely that we have similar cases in our tree,
and these may also bust when we try to get runtime interface info for them. 
Nominating nsbeta3, I'll evaluate this for checkin.

Comment 8

[Patrick C. Beard]

If the fix in hand is low risk, I'll approve for nsbeta3+.

Comment 9

[John Bandhauer]

I wish someone had cc'd me on this on.

I agree that this refactoring should go in.

Note that bug 46904 fixed the lack of a null check.
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/xpcom/reflect/xptinfo/src/x
ptiInterfaceInfo.cpp&line_nums=on&root=/cvsroot&#453

But, Mark's refactoring fix is better.

Comment 10

[Brendan Eich [:brendan]]

Whether or not this bug got [nsbeta3+] status, it's a patch from someone not
bound by netscape.com's rules for its employees, so the module owner must help
get it in (if the patch author lacks CVS access).  And I will gladly say
a=[e-mail] -- thanks, MarkH.

What's the status on this bug?  DavidA mentioned some kind of non-XP-ness that
makes it not quite fixed on certain platforms?

/be

Comment 11

[Mike McCabe]

Attaching slightly modified fix:

Move if (!theInfo) check into common GetInfoForParam code;
Anticipate that GetInfoForParam will return NS_ERROR_FAILURE on null and just
check against NS_FAILED(rv) in GetIIDForParam.

r=jband?

I didn't notice any non-XP-ness; looks fine to go in.

Comment 12

[Mike McCabe]

Attachment: slight mods to Mark Hammond fix

Comment 13

[John Bandhauer]

change the...
  NS_IF_ADDREF(*info = theInfo);
...to...
  NS_ADDREF(*info = theInfo);
...there's no need to check 'theInfo' twice.

Otherwise, r=jband. In it goes!

Comment 14

[Mike McCabe]

Fix checked in.

Comment 15

[Tracy Walker [:tracy]]

please verify

Comment 16

[leger]

Mark, please mark verified if all is well.

Comment 17

[Mark Hammond [:markh] [:mhammond]]

Everything is working well for me!  Thanks!

Comment 18

[Keyser Sose]

Mark Verified it.