865984 - Hole in static analysis with scriptable IDL methods

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Steve Fink [:sfink] [:s:]]

bz found a hole in the static analysis. Consider OpenSignedJARFileTask::CallCallback. The static analysis claims that this function cannot GC.

From IRC:

	Function looks like this:
	745 virtual void CallCallback(nsresult rv)
	746 {
	747 (void) mCallback->OpenSignedJARFileFinished(rv, mZipReader, mSignerCert);
	748 }

	and we have:

	751 const nsCOMPtr<nsIOpenSignedJARFileCallback> mCallback;
	And there are no in-tree C++ implementations of nsIOpenSignedJARFileCallback
	but...
	http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#2262
	That second argument to openSignedJARFileAsync
	is a JS function

	bz	And the IDL for openSignedJARFileAsync is:
	289 void openSignedJARFileAsync(in nsIFile aJarFile,
	290 in nsIOpenSignedJARFileCallback callback);

The problem is synthetic vtables -- that IDL method is scriptable.

Comment 1

[Boris Zbarsky [:bzbarsky]]

So the issue is that for scriptable interfaces XPConnect will go ahead and synthesize objects with the right vtable.

In terms of how the vtable is synthesized...

There's a class called nsXPTCStubBase xpcom/reflect/xptcall/src/xptcprivate.h which declares a whole bunch of virtual functions.  Then there are per-ABI implementations of these virtual functions in xpcom/reflect/xptcall/src/md/*/xptcstubs*.cpp.

Looking at the x86-64 darwin setup, say, what these do is set up a call to SharedStub and pass the method index.  And that then calls PrepareAndDispatch, passing along the method index and the nsXPTCStubBase* from which the xptiInterfaceEntry* can be gotten and then it does the whole aOuter->CallMethod thing, which lands in nsXPCWrappedJS::CallMethod which calls nsXPCWrappedJSClass::CallMethod which goes off and calls into JS.  At least that's what things look like to me...

Comment 2

[Brian Hackett [Laid off!]]

After discussion with bz on IRC I patched the analysis to treat all virtual calls on nsISupports subclasses (except AddRef/Release) as indirect calls that can potentially GC.  This only adds 12 new hazards on my test box for the browser, and has a nice bonus of reducing the size of the GC function backtrace file by 24x (!) due to the greatly shortened stacks.

https://hg.mozilla.org/integration/mozilla-inbound/rev/4c14d5cbce06

Comment 3

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4c14d5cbce06