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

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [build_warning] fixed-in-tracemonkey)

Attachments

(1 attachment)

Assignee

Description

9 years ago
../../../../../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.
Assignee

Updated

9 years ago
OS: Linux → All
Hardware: x86_64 → All
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.
Assignee: general → nobody
Component: JavaScript Engine → XPConnect
QA Contact: general → xpconnect
Assignee

Comment 2

9 years ago
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.)
Attachment #505531 - Flags: review?(gal)

Comment 3

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

Thanks!
Attachment #505531 - Flags: review?(gal) → review+
Assignee

Updated

9 years ago
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Whiteboard: [build_warning] → [build_warning][needs landing]
Assignee

Comment 4

9 years ago
http://hg.mozilla.org/tracemonkey/rev/64274de90e2d
Whiteboard: [build_warning][needs landing] → [build_warning] fixed-in-tracemonkey
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.