238712 - nsMemoryImpl::Alloc (0) in XPCConvert::JSArray2Native

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[timeless]

xpcom.dll!nsMemory::Alloc(unsigned int size=0)  Line 88	C++
>	xpc3250.dll!XPCConvert::JSArray2Native(XPCCallContext & ccx={...}, void * *
d=0x0012f234, long s=1, unsigned int count=0, unsigned int capacity=0, const
nsXPTType & type={...}, int useAllocator=0, const nsID * iid=0x0012f348,
unsigned int * pErr=0x0012f398)  Line 1735 + 0x11	C++
 	xpc3250.dll!XPCWrappedNative::CallMethod(XPCCallContext & ccx={...},
XPCWrappedNative::CallMode mode=CALL_METHOD)  Line 1981 + 0x21	C++

###!!! ASSERTION: nsMemoryImpl::Alloc of 0: 'size', file
mozilla/xpcom/base/nsMemoryImpl.cpp, line 324
Break: at file mozilla/xpcom/base/nsMemoryImpl.cpp, line 324
0 [native frame]
1 sendCallbackString(message = "")
["file:///C:/DBG/bin/components/hs_Trouble_Maker.js":867]
    totalBytes = undefined
    bytesWritten = undefined
    this = [object Object]

const sockService_ =
  Components.classes["@mozilla.org/network/socket-transport-service;1"].
  getService().QueryInterface(Components.interfaces.nsISocketTransportService);
//867:
      this.connection_ = sockService_.createTransport([], 0, "localhost",
                                                      this.port_, null);

code that causes the problem:
#define POPULATE(_mode, _t)                                                  \
    PR_BEGIN_MACRO                                                           \
        cleanupMode = _mode;                                                 \
        if(nsnull == (array = nsMemory::Alloc(capacity * sizeof(_t))))       \
        {                                                                    \
            if(pErr)                                                         \
                *pErr = NS_ERROR_OUT_OF_MEMORY;                              \
            goto failure;                                                    \
        }                                                                    \
        for(initedCount = 0; initedCount < count; initedCount++)             \
        {                                                                    \
            if(!JS_GetElement(cx, jsarray, initedCount, &current) ||         \
               !JSData2Native(ccx, ((_t*)array)+initedCount, current, type,  \
                              useAllocator, iid, pErr))                      \
                goto failure;                                                \
        }                                                                    \
    PR_END_MACRO

    case nsXPTType::T_CHAR_STR      : POPULATE(fr, char*);          break;

locals:
+	ccx	{mRefCnt={mValue=0 } _mOwningThread={mThread=0x00426080 }
mState=READY_TO_CALL ...}	XPCCallContext &
	d	0x0012f234	void * *
	s	1	long
	count	0	unsigned int
	capacity	0	unsigned int
+	type	{...}	const nsXPTType &
	useAllocator	0	int
+	iid	0x0012f348 {m0=3059557040 m1=7633 m2=4530 ...}	const nsID *
+	pErr	0x0012f398	unsigned int *
	array	0x00000000	void *
	cleanupMode	fr	XPCConvert::JSArray2Native::__l7::CleanupMode
	len	0	unsigned long
+	cx	0x00c40498 {links={next=0x01c2a6d0 {next=0x01cd6460 {next=0x01d097c8
prev=0x01c2a6d0 } prev=0x00c40498 {next=0x01c2a6d0 prev=0x00b67400 } }
prev=0x00b67400 {next=0x00c40498 {next=0x01c2a6d0 prev=0x00b67400 }
prev=0x00b64218 {next=0x00b67400 prev=0x02a945f8 } } } interpLevel=1
stackLimit=0 ...}	JSContext *
+	jsarray	0x02c99790 {map=0x02cb0c68 {nrefs=1 ops=0x00e08160 _js_ObjectOps
nslots=5 ...} slots=0x02c82934 }	JSObject *
	initedCount	1242072	unsigned int
	current	1	long

Some code in this function tolerates 0 capacity, some code (like the code here)
does not.

Comment 1

[timeless]

Another testcase (much more pathological)

SoapCall=Components.Constructor("@mozilla.org/xmlextras/soap/call;1");
SoapParam=Components.Constructor("@mozilla.org/xmlextras/soap/parameter;1");
SOAPParameter=function SOAPParameter(){
  var args=["value", "name", "namespace", "schemaType", "encoding"];
  var soapParam = new SoapParam;
  for (var i = 0; i<arguments.length; ++i)
    soapParam[args[i]]=arguments[i];
  return soapParam;
}
SOAPCall=function SOAPCall(){
  var args=[];
  var soapCall = new SoapCall;
  for (var i = 0; i<arguments.length; ++i)
    soapCall[args[i]]=arguments[i];
  return soapCall;
}
  var soapversion = 0;  //  Version 1.1

  var method = "GetLastTradePrice";

  var object = "uri:some-namespace";

  var headers = new Array(
    );
  
  var params = new Array(
    new SOAPParameter("a string", "foo"),
    new SOAPParameter(4, "bar"),
    new SOAPParameter(true, "baz"),
    new SOAPParameter(new Array(3, "another string", false, 5.235)),
    new SOAPParameter({name: "Fido", breed: "Lab", color: "Black"})
    );

  var s = new SOAPCall();
  s.encode(soapversion, method, object, headers.length, headers, params.length,
params);

Comment 2

[timeless]

Attachment: only convert array if it has elements. otherwise dp->val.p is null, which is fine

This matches the behavior of XPCVariant::InitializeData

Comment 3

[Brendan Eich [:brendan]]

I'll wait for dbradley to r=, but looks good so far.  Any other such cases?

/be

Comment 4

[David Bradley]

Comment on attachment 146277 [details] [diff] [review]
only convert array if it has elements. otherwise dp->val.p is null, which is fine

r=dbradley

If we're going to follow this pattern and passing in zero length arrays to
JSArray2Native is going to be "bad" then we should put in an assert stating
this.

I'd probably have chosen to handle this within, but it could get messy, as
JSArray2Native may not have enough knowledge to know what the caller is wanting
for empty arrays.

So this fixes it, but I'm not sure it's ideal. We can deal with that later.

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 146277 [details] [diff] [review]
only convert array if it has elements. otherwise dp->val.p is null, which is fine

rs=[e-mail].

/be

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 146277 [details] [diff] [review]
only convert array if it has elements. otherwise dp->val.p is null, which is fine

Can we get this on the branch?	Using the JS debugger on windows debug builds
(which people basing products off the branch are likely to do) is incredibly
irritating with this dialog box popping up constantly.

Comment 7

[Asa Dotzler [:asa]]

Comment on attachment 146277 [details] [diff] [review]
only convert array if it has elements. otherwise dp->val.p is null, which is fine

a=asa (on behalf of drivers) for checkin to 1.7

Comment 8

[timeless]

Comment on attachment 146277 [details] [diff] [review]
only convert array if it has elements. otherwise dp->val.p is null, which is fine

mozilla/js/src/xpconnect/src/xpcwrappedjsclass.cpp	1.77
mozilla/js/src/xpconnect/src/xpcwrappednative.cpp	1.84

mozilla/js/src/xpconnect/src/xpcwrappedjsclass.cpp	1.76.14.1
mozilla/js/src/xpconnect/src/xpcwrappednative.cpp	1.82.2.1

Comment 9

[Daniel Veditz [:dveditz]]

Why is this open? Comment 8 indicates a trunk check-in, and the trunk code
appears to have the patch. Then again the patch is marked obsolete... what's
remaining here?

Comment 10

[Wayne Mery (:wsmwk)]

timeless,
ping regarding comment 9 

Comment 11

[David Bradley]

Did it get checked into the 1.7 branch. That's the only thing I see potentially outstanding. Otherwise I'd say mark it fixed.

Comment 12

[Wayne Mery (:wsmwk)]

at this point if it didn't check into 1.7 it's no loss
=> fixed