612408 - mozJSComponentLoader::ImportInto() has clause that returns NULL instead of nsresult

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Daniel Holbert [:dholbert]]

../../../../../mozilla/js/src/xpconnect/loader/mozJSComponentLoader.cpp: In member function ‘virtual nsresult mozJSComponentLoader::ImportInto(const nsACString_internal&, JSObject*, nsAXPCNativeCallContext*, JSObject**)’:
../../../../../mozilla/js/src/xpconnect/loader/mozJSComponentLoader.cpp:1498:20: warning: converting to non-pointer type ‘nsresult’ from NULL

The buggy code flagged by this is:
1388 /* [noscript] JSObjectPtr importInto(in AUTF8String registryLocation,
1389                                      in JSObjectPtr targetObj); */
1390 NS_IMETHODIMP
1391 mozJSComponentLoader::ImportInto(const nsACString & aLocation,
1392                                  JSObject * targetObj,
1393                                  nsAXPCNativeCallContext * cc,
1394                                  JSObject * *_retval)
1395 {
[...]
1497         if (!ac.enter(mContext, mod->global))
1498             return NULL;

This was added in http://hg.mozilla.org/mozilla-central/rev/0a7c7b2732cd

NULL evaluates to 0 == NS_OK.  I have no idea if that's actually what we want to be returning in this case -- whether it is or not, we should be returning an nsresult and not NULL.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Looks to me like JSAutoEnterCompartment::enter returns a bool indicating success, so I think a failure rv is what's actually wanted. It looks like it can only fail in the case of OOM, though, so this is probably unlikely to have any negative effects in practice.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: fix: s/NULL/NS_ERROR_FAILURE/

Agreed, looks like this is a (rare) failure case.  This patch just replaces 'return NULL; with 'return NS_ERROR_FAILURE;'

(It does indeed look like OOM is the main (only?) way for this to fail, but I'm not sufficiently convinced of that (or of callers caring whether it's OOM vs 'generic failure'...?) to propose using an OOM-specific error code.)

Comment 3

[Andreas Gal :gal]

Comment on attachment 505531 [details] [diff] [review]
fix: s/NULL/NS_ERROR_FAILURE/

Thanks!

Comment 4

[Daniel Holbert [:dholbert]]

http://hg.mozilla.org/tracemonkey/rev/64274de90e2d

Comment 5

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/64274de90e2d