Bug 585124 - Fix leak [@StructType::DefineInternal]. r=Waldo, a=bsmedberg

js/src/ctypes/CTypes.cpp

@@ -2701,18 +2701,19 @@ CType::Finalize(JSContext* cx, JSObject* obj)
       delete static_cast<FunctionInfo*>(JSVAL_TO_PRIVATE(slot));
     break;
   }
-  case TYPE_struct:
-    // Free the FieldInfo array.
+
+  case TYPE_struct: {
+    // Free the FieldInfoHash table.
     ASSERT_OK(JS_GetReservedSlot(cx, obj, SLOT_FIELDINFO, &slot));
     if (!JSVAL_IS_VOID(slot)) {
       void* info = JSVAL_TO_PRIVATE(slot);
       delete static_cast<FieldInfoHash*>(info);
     }
+  }
 
     // Fall through.
   case TYPE_array: {
     // Free the ffi_type info.
-    jsval slot;
     ASSERT_OK(JS_GetReservedSlot(cx, obj, SLOT_FFITYPE, &slot));
     if (!JSVAL_IS_VOID(slot)) {
       ffi_type* ffiType = static_cast<ffi_type*>(JSVAL_TO_PRIVATE(slot));
@@ -4023,21 +4024,18 @@ StructType::DefineInternal(JSContext* cx, JSObject* typeObj, JSObject* fieldsObj
          NULL, NULL, JSPROP_READONLY | JSPROP_PERMANENT))
     return JS_FALSE;
 
-  // Create a hash of FieldInfo objects to stash on the type object.
-  FieldInfoHash* fields(new FieldInfoHash);
-  if (!fields || !fields->init(len)) {
+  // Create a FieldInfoHash to stash on the type object, and an array to root
+  // its constituents. (We cannot simply stash the hash in a reserved slot now
+  // to get GC safety for free, since if anything in this function fails we
+  // do not want to mutate 'typeObj'.)
+  AutoPtr<FieldInfoHash> fields(new FieldInfoHash);
+  Array<jsval, 16> fieldRootsArray;
+  if (!fields || !fields->init(len) || !fieldRootsArray.appendN(JSVAL_VOID, len)) {
     JS_ReportOutOfMemory(cx);
-    delete fields;
-    return JS_FALSE;
-  }
-
-  // Stash the FieldInfo hash in a reserved slot now, for GC safety of its
-  // constituents.
-  if (!JS_SetReservedSlot(cx, typeObj, SLOT_FIELDINFO,
-         PRIVATE_TO_JSVAL(fields))) {
-    delete fields;
     return JS_FALSE;
   }
+  js::AutoArrayRooter fieldRoots(cx, fieldRootsArray.length(), 
+    fieldRootsArray.begin());
 
   // Process the field types.
   size_t structSize, structAlign;
@@ -4054,6 +4052,7 @@ StructType::DefineInternal(JSContext* cx, JSObject* typeObj, JSObject* fieldsObj
       JSString* name = ExtractStructField(cx, item.jsval_value(), &fieldType);
       if (!name)
         return JS_FALSE;
+      fieldRootsArray[i] = OBJECT_TO_JSVAL(fieldType);
 
       // Make sure each field name is unique, and add it to the hash.
       FieldInfoHash::AddPtr entryPtr = fields->lookupForAdd(name);
@@ -4111,6 +4110,11 @@ StructType::DefineInternal(JSContext* cx, JSObject* typeObj, JSObject* fieldsObj
   if (!SizeTojsval(cx, structSize, &sizeVal))
     return JS_FALSE;
 
+  if (!JS_SetReservedSlot(cx, typeObj, SLOT_FIELDINFO,
+         PRIVATE_TO_JSVAL(fields.get())))
+    return JS_FALSE;
+  fields.forget();
+
   if (!JS_SetReservedSlot(cx, typeObj, SLOT_SIZE, sizeVal) ||
       !JS_SetReservedSlot(cx, typeObj, SLOT_ALIGN, INT_TO_JSVAL(structAlign)) ||
       //!JS_FreezeObject(cx, prototype) || // XXX fixme - see bug 541212!
@@ -4347,8 +4351,9 @@ StructType::BuildFieldsArray(JSContext* cx, JSObject* obj)
 
   // Prepare a new array for the 'fields' property of the StructType.
   Array<jsval, 16> fieldsVec;
-  if (!fieldsVec.resize(len))
+  if (!fieldsVec.appendN(JSVAL_VOID, len))
     return NULL;
+  js::AutoArrayRooter root(cx, fieldsVec.length(), fieldsVec.begin());
 
   for (FieldInfoHash::Range r = fields->all(); !r.empty(); r.popFront()) {
     const FieldInfoHash::Entry& entry = r.front();
@@ -4800,13 +4805,10 @@ FunctionType::Create(JSContext* cx, uintN argc, jsval* vp)
     jsuint len;
     ASSERT_OK(JS_GetArrayLength(cx, arrayObj, &len));
 
-    if (!argTypes.resize(len)) {
+    if (!argTypes.appendN(JSVAL_VOID, len)) {
       JS_ReportOutOfMemory(cx);
       return JS_FALSE;
     }
-
-    for (jsuint i = 0; i < len; ++i)
-      argTypes[i] = JSVAL_VOID;
   }
 
   // Pull out the argument types from the array, if any.
@@ -5355,14 +5357,11 @@ CClosure::ClosureStub(ffi_cif* cif, void* result, void** args, void* userData)
 
   // Set up an array for converted arguments.
   Array<jsval, 16> argv;
-  if (!argv.resize(cif->nargs)) {
+  if (!argv.appendN(JSVAL_VOID, cif->nargs)) {
     JS_ReportOutOfMemory(cx);
     return;
   }
 
-  for (JSUint32 i = 0; i < cif->nargs; ++i)
-    argv[i] = JSVAL_VOID;
-
   js::AutoArrayRooter roots(cx, argv.length(), argv.begin());
   for (JSUint32 i = 0; i < cif->nargs; ++i) {
     // Convert each argument, and have any CData objects created depend on

js/src/ctypes/CTypes.h

@@ -402,7 +402,7 @@ enum CTypeSlot {
   SLOT_ELEMENT_T = 7, // (ArrayTypes only) 'elementType' property
   SLOT_LENGTH    = 8, // (ArrayTypes only) 'length' property
   SLOT_FIELDS    = 7, // (StructTypes only) 'fields' property
-  SLOT_FIELDINFO = 8, // (StructTypes only) FieldInfo array
+  SLOT_FIELDINFO = 8, // (StructTypes only) FieldInfoHash table
   SLOT_FNINFO    = 7, // (FunctionTypes only) FunctionInfo struct
   SLOT_ARGS_T    = 8, // (FunctionTypes only) 'argTypes' property (cached)
   CTYPE_SLOTS

toolkit/components/ctypes/tests/unit/test_jsctypes.js.in

@@ -1478,6 +1478,10 @@ function run_StructType_tests() {
   do_check_throws(function() {
     opaque_t.define([{ a: ctypes.int32_t, b: ctypes.int64_t }]);
   }, Error);
+  do_check_throws(function() {
+    opaque_t.define([{ a: ctypes.int32_t }, { b: 0 }]);
+  }, Error);
+  do_check_false(opaque_t.hasOwnProperty("prototype"));
 
   // Check that circular references work with opaque structs...
   // but not crazy ones.
@@ -1490,9 +1494,14 @@ function run_StructType_tests() {
   do_check_eq(circular.a.toSource(), opaque.address().toSource());
   do_check_eq(opaque.b.toSource(), circular.toSource());
 
+  // Check that attempting to redefine a struct fails and if attempted, the
+  // original definition is preserved.
   do_check_throws(function() {
-    opaque_t.define([{ c: ctypes.int32_t }]);
+    opaque_t.define([{ c: ctypes.int32_t.array(8) }]);
   }, Error);
+  do_check_eq(opaque_t.size, circular_t.size);
+  do_check_true(opaque_t.prototype.hasOwnProperty("b"));
+  do_check_false(opaque_t.prototype.hasOwnProperty("c"));
 
   // StructType size, alignment, and offset calculations have already been
   // checked for each basic type. We do not need to check them again.