720771 - (ctypes.finalize) Support finalization for CData objects (ctypes.finalize)

Status: RESOLVED FIXED

(Core :: js-ctypes, defect)

Description

[Jason Orendorff [:jorendorff]]

Most C APIs require explicitly freeing system resources. Therefore code that uses js-ctypes must explicitly free resources it's done using. There are two big problems with this:

  (1) It's easy to make a mistake and forget to free a resource;

  (2) JS code can be terminated without running finally blocks, so no matter
      how hard you try, your explicit free code might be skipped.

Taras proposes a new ctypes.finalize API to serve as a backstop here.

    // declare a few functions
    var open = libc.declare("open", ...);
    var close = libc.declare("close", ...);

    // open a file
    var fd_raw = open(filename, flags);

    // now create an object that will close the file when it is GC'd
    var fd = ctypes.finalize(fd_raw, close)

This object fd should be valid anywhere a CData object would be valid; the only difference is that when fd is garbage-collected, we automatically call

    close(fd);

The object should have a couple methods:
    fd.dispose();  // call the finalizer, throw away the data, become empty
    fd.forget();   // just throw away the data and become empty

Note that

  * the finalizer function must be C, and has to take a single argument

  * the order in which finalizers get called is arbitrary

  * you can't have JS code run at finalize-time; we can't run JS during GC

  * you should still explicitly release stuff as much as possible, since
    there's no telling when the next full GC will happen

  * it is important to actually use fd, not fd_raw, because otherwise
    it could be closed before you're done with it

Comment 1

[Jason Orendorff [:jorendorff]]

Implementation:

  - Make a new JSClass, unrelated to CData and CType. Call it CDataWithFinalizer.
    It has two reserved slots, cval and finalizer.

  - Make a function ctypes.finalize(val, finalizer) that returns a new
    CDataWithFinalizer. It needs to check that finalizer is actually a
    C function with one argument, and throw a TypeError if it isn't.

    Then it needs to compute cval = ImplicitConvert(val, T) where T is
    the argument type. This produces cval which is just a chunk of
    memory. (ImplicitConvert can't be called safely during GC,
    so I think this must be done when finalize() is called rather than
    at finalize time.) Store cval and finalizer in the new object's
    reserved slots.

  - The CDataWithFinalizer_finalize hook just calls the finalizer,
    then frees the memory we had set aside for cval.

Feel free to rename the thing.

Comment 2

[Jason Orendorff [:jorendorff]]

This will need tests. The existing ctypes tests actually build a shared library; you can easily add a few functions to that.

A note about testing GC-related features: GC is of course nondeterministic (we even do some conservative GC). You can do something like:

    // create some garbage
    for (var i = 0; i < 5; i++)
        ctypes.finalize(i, my_c_finalizer);

    gc();

    // check that at least one of the items got collected.
    var n = 0;
    for (var i = 0; i < 5; i++)
        n += check_my_c_finalizer_was_called(i);
    assert(n > 0);

A simple loop typically uses the same memory locations in at least a few of the iterations, so we expect at least some of the garbage to be shaken loose. This is not a very satisfactory sort of test but we do what we can.

Comment 3

[Jason Orendorff [:jorendorff]]

Oh, and here's how to make it so that a CDataWithFinalizer can be passed anywhere a CData could be passed.

See the spec for ImplicitConvert here:
  https://wiki.mozilla.org/Jsctypes/api#Conversions

We want to add a bullet point to the spec:

  * If val is a CDataWithFinalizer object and t is the finalizer argument type,
    return the value stored in val's cval field.

The change can be made in js::ctypes::ImplicitConvert in CTypes.cpp. Should be pretty easy.

Comment 4

[(dormant account)]

forget() should return ConvertToJS(cval).

Comment 5

[(dormant account)]

This will work automagically for cleaning up 32/64bit/etc values.

Whatever close/free/release function you pass in, it's a ctypes function, so it has type information.  You can check that it has one argument, and you can look at the argument type.  so you just call GetSize() on that, allocate that size chunk of memory.

Comment 6

[David Teller [:Yoric] - still alive but not very active]

Why not attach the finalizer to the type declaration?

Something along the lines of:
    var close = libc.declare("close", ctypes.void, ctypes.int64_t);
    var type_fd = ctypes.attach_finalizer(ctypes.int64_t, close);

    // declare a few functions
    var open = libc.declare("open", type_fd, ...);

Comment 7

[(dormant account)]

(In reply to David Rajchenbach Teller [:Yoric] from comment #6)
> Why not attach the finalizer to the type declaration?

Seems reasonable and like something we can do in a followup bug.

Comment 8

[Jason Orendorff [:jorendorff]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #6)
> Why not attach the finalizer to the type declaration?

I originally wanted to do it that way. See bug 688797.

taras proposed the new way, and I think it's probably better. The implementation of the typeful approach would be more tightly coupled with CTypes. This is off to the side.

The typeful way *sounds* elegant, but it might be less elegant in practice when it comes to supporting hacks and corner cases that come up all the time in C APIs. open, for example, returns -1 on error. More generally, C APIs return special values on error that must not be freed/released--and with this design you would end up having to do explicit .forget() calls before throwing. If you forget to call .forget(), the symptom is a weird crash that surfaces during the next GC cycle. Ick.

The solution to that is of course more typefulness -- the return type should express the error handling contract, and ctypes should automatically detect errors and convert them to nice JS exceptions.

Likewise, if functions like pipe(2) produce descriptors in out-params which must later be freed, the solution is still more typefulness -- the type of the pipe function should express that the argument is an out parameter, containing garbage on entry, populated with descriptors that must be later closed only on success.

Which all may come to pass in the fullness of time, but let's get the hack done first.

Comment 9

[David Teller [:Yoric] - still alive but not very active]

(In reply to Jason Orendorff [:jorendorff] from comment #8)
> The solution to that is of course more typefulness -- the return type should
> express the error handling contract, and ctypes should automatically detect
> errors and convert them to nice JS exceptions.

Yes, that's the kind of thing that I did in the JSAPI-based implementation of os.file. I'll see how easy it is to do this.

> Which all may come to pass in the fullness of time, but let's get the hack
> done first.

ok.

Comment 10

[David Teller [:Yoric] - still alive but not very active]

Attachment: Early preview of the code

Comment 11

[Jason Orendorff [:jorendorff]]

Good work, I can't wait to see it passing tests.

It looks like SLOT_DATAFINALIZER_TYPE is unused.

Comment 12

[David Teller [:Yoric] - still alive but not very active]

Attachment: Code complete

Comment 13

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 593118 [details] [diff] [review]
Code complete

I seem to have a working prototype. Still needs a testsuite, of course, and possibly some clean-up + doc.

Comment 14

[Jason Orendorff [:jorendorff]]

Comment on attachment 593118 [details] [diff] [review]
Code complete

Yes, this is roughly what we want. Go ahead.

Tests!

Comment 15

[David Teller [:Yoric] - still alive but not very active]

Attachment: Partial testsuite

Attaching a Unix testsuite. I need to investigate how I can write a Windows version.

Comment 16

[David Teller [:Yoric] - still alive but not very active]

Attachment: Complete patch

Here is a cleaned-up patch.

Comment 17

[Jason Orendorff [:jorendorff]]

Out of time for today. I'll get to it tomorrow.

Comment 18

[David Teller [:Yoric] - still alive but not very active]

Attachment: Unix testsuite

Comment 19

[Jason Orendorff [:jorendorff]]

Instead of using open, close, and unlink, try adding a few entry points to toolkit/components/ctypes/tests.

Comment 20

[Jason Orendorff [:jorendorff]]

Very nearly done here, I'll finish later tonight.

Comment 21

[Jason Orendorff [:jorendorff]]

I'm afraid this is a long review, but the patch is basically sound.

My main complaint is that there are not enough tests; more on that in a separate comment.

In CTypes.h, enum CDataFinalizerSlot:
>+  SLOT_DATAFINALIZER_JSVAL             = 1, // The value to finalize.
>+  // We hold it to permit |forget|. Can be any |jsval|.

I don't think we need this slot. More about this below.

>+  SLOT_DATAFINALIZER_VALTYPE           = 2, // The type of the value
>+  // We hold it to permit |ImplicitConvert|. Is a JSObject *

We can probably do without this slot too, since this is the same as the
type of the first argument to the finalizer, which is stored in
SLOT_DATAFINALIZER_CDATA_FINALIZER.

>   JSBool GetRuntime(JSContext* cx, uintN argc, jsval* vp);
>+
>+  JSBool ArgumentFormatter(JSContext *cx, const char *format,
>+                           JSBool fromJS, jsval **vpp, va_list *app);
> }

I don't see this defined anywhere. Delete it?

In CTypes.cpp:
> #ifdef HAVE_SSIZE_T
> #include <sys/types.h>
>-#endif
>+#endif //HAVE_SSIZE_T

Please revert this change.

> namespace ctypes {
> 
>+
> /*******************************************************************************

House style in js/src is never to have two blank lines in a row, so
please don't add this blank line.

In namespace CData:
>+  static JSBool AttachFinalizer(JSContext* cx,  uintN argc, jsval *vp);

This name seems misleading to me. People reading this code could
reasonably assume that the function attaches finalization behavior to an
existing CData object (especially since that would be a reasonable
design!). But that isn't what the function does.

This doesn't belong in the CData namespace, either. How about
js::ctypes::WithFinalizer?

Actually, since we need to have ctypes.CDataFinalizer.prototype, this
might as well just be called CDataFinalizer::construct, and instead of
telling users to call ctypes.withFinalizer(x), we can tell them to call
ctypes.CDataFinalizer(x). This would also make the code of the function
simpler; see comments on the implementation below.

Style nit: Single space between "cx," and "uintN argc".

>+  typedef struct {
>...
>+  } Private;

Use the C++ idiom:

    struct Private {
      ...
    };

In struct Private:
>+    /**
>+     * Maintained alive by a strong reference to the owner CData.
>+     * Memory handled by the garbage collector. Do not deallocate it.
>+     */
>+    FunctionInfo* ffiinfo;

The situation this comment describes is not GC-safe; see the comment
about ffiinfo on CData::AttachFinalizer, below.

>+  namespace Methods {

Heh. I don't think we need to go quite this far with namespaces--we
haven't with any of the others--but it's your call. :)

>+    /**
>+     * JavaScript function |CDataFinalizer.__proto__.dispose|
>+     */
>+    static JSBool Dispose(JSContext* cx,  uintN argc, jsval *vp);
>+    /**
>+     * JavaScript function |CDataFinalizer.__proto__.forget|
>+     */
>+    static JSBool Forget(JSContext* cx,  uintN argc, jsval *vp);
>+    /**
>+     * JavaScript function |CDataFinalizer.__proto__.toString|
>+     *
>+     * Useful mostly for debugging.
>+     */
>+    static JSBool ToString(JSContext* cx, uintN argc, jsval *vp);

These comments don't seem all that useful to me. Please feel free to
delete them if you agree. If you keep them, add blank lines between each
declaration and the following comment.

In any case "__proto__" is incorrect; you mean "prototype".

>+  }
>+  /**
>+   * Utility functions

Add a blank line before the comment.

In InitTypeClasses:
>   JSObject* CDataProto = InitCDataClass(cx, parent, CTypeProto);
>   if (!CDataProto)
>     return false;
> 
>+  JSObject* CDataFinalizerProto = InitCDataFinalizerClass(cx, parent);
>+  if (!CDataFinalizerProto)
>+    return false;
>+
>   // Link CTypeProto to CDataProto.
>   if (!JS_SetReservedSlot(cx, CTypeProto, SLOT_OURDATAPROTO,
>                           OBJECT_TO_JSVAL(CDataProto)))
>     return false;

This is between two parts of CType initialization; it would be better to
put it either before everything else or after everything else.

>       !AttachProtos(cx, protos[SLOT_STRUCTPROTO], protos) ||
>-      !AttachProtos(cx, protos[SLOT_FUNCTIONPROTO], protos))
>+      !AttachProtos(cx, protos[SLOT_FUNCTIONPROTO], protos)
>+      )
>      return false;

This change is unnecesary. Please revert it.

In ImplicitConvert:
>   // First, check if val is a CData object of type targetType.
>   JSObject* sourceData = NULL;
>   JSObject* sourceType = NULL;
>-  if (!JSVAL_IS_PRIMITIVE(val) &&
>-      CData::IsCData(cx, JSVAL_TO_OBJECT(val))) {
>+  if (!JSVAL_IS_PRIMITIVE(val)) {
>     sourceData = JSVAL_TO_OBJECT(val);
>+    if (CData::IsCData(cx, sourceData)) {

Update the comment here, since you're adding new behavior.

This introduces a bug when val is not a CData object. Note that
sourceData is checked later on in the function to see if val is a CData
object; if sourceData is non-null, it calls CData::GetData(cx, sourceData)
and so on. Make sure there is a test that checks for this bug.

In ImplicitConvert:
>+        if (!p) {
>+          return JS_FALSE;//We have called |dispose| or |forget| already.
>+        }

This must call JS_ReportErrorNumber and report an error before returning
false. Make sure there is a test that exercises this path, and make sure
it uses try/catch to make sure the error is catchable and is of a
reasonable type.

>+        memmove(buffer, p -> cargs, p -> cargs_size);

Style nit: throughout the patch, don't put spaces around "->".

In CDataFinalizer::Methods::ToString:
>+  jsval valData;
>+  if (!JS_GetReservedSlot(cx, objThis,
>+                          SLOT_DATAFINALIZER_JSVAL,
>+                          &valData)) {
>+    return JS_FALSE;
>+  }
>+
>+  if (JSVAL_IS_NULL(valData) || JSVAL_IS_VOID(valData)) {
>+    JSString *strMessage = JS_NewStringCopyZ(cx, "[CDataFinalizer - empty]");
>+    JS_SET_RVAL(cx, vp, STRING_TO_JSVAL(strMessage));
>+    return JS_TRUE;
>+  }
>+
>+  JSObject *objData;
>+  if (!JS_ValueToObject(cx, valData, &objData)) {
>+    return JS_FALSE;
>+  }
>+
>+  jsval* argv = JS_ARGV(cx, vp);
>+  jsval result;
>+  if (!JS_CallFunctionName(cx, objData,
>+                           "toString", 0, argv, &result)) {
>+    return JS_FALSE;
>+  }

Here's what we should do instead. Look at CData::ToSource and factor out
these lines:

    // Walk the types, building up the toSource() string.
    // First, we build up the type expression:
    // 't.ptr' for pointers;
    // 't.array([n])' for arrays;
    // 'n' for structs, where n = t.name, the struct's name. (We assume this is
    // bound to a variable in the current scope.)
    AutoString source;
    BuildTypeSource(cx, typeObj, true, source);
    AppendString(source, "(");
    if (!BuildDataSource(cx, typeObj, data, false, source))
      return JS_FALSE;

    AppendString(source, ")");

    result = NewUCString(cx, source);

into a function of their own, perhaps with this signature:

  JSString *
  CData::GetSourceString(JSContext *cx, JSObject *typeObj, void *data);

Then make CDataFinalizer::ToString call that, using
CDataFinalizer::GetCType(cx, objThis) to get the type.

This is about the same amount of code and allows us to drop
SLOT_DATAFINALIZER_JSVAL, so the CDataFinalizer object doesn't keep a
separate CData object alive.

>+// Utility function to access a property of an object as an object
>+// @return false if the property does not exist or is not an object
>+bool GetObjectProperty(JSContext *aCx, JSObject *aObject,
>+                       const char *aProperty, JSObject **aOutResult)
>+{
>+  jsval val;
>+  if (!JS_GetProperty(aCx, aObject, aProperty, &val)) {
>+    return false;
>+  }
>+
>+  if (!JSVAL_IS_OBJECT(val)) {
>+    return false;
>+  }
>+
>+  *aOutResult = JSVAL_TO_OBJECT(val);
>+  return true;
>+}

I think this should be unnecessary and it should be removed, but in case
you decide to keep it, (1) note that JSVAL_IS_OBJECT(v) actually returns
true if v is JSVAL_NULL, contrary to common sense; and (2) you can't
return false if an error hasn't been reported. JS_GetProperty will
report an error on failure, but JSVAL_IS_OBJECT never reports an error.

In CData::AttachFinalizer:
>+ * Pseudo-JS signature:
>+ * function(CData<T>, CData<T -> U>): CDataFinalizer<T>
>+ *          value,    finalizer

Nice comment.

>+ * This function attaches strong references to the following values:
>+ * - |value|
>+ * - |finalizer|
>+ * - the CType of |value|

It should be changed so that it only keeps strong references to
|finalizer|.

>+  if (argc < 2) {
>+    JS_ReportError(cx, "withFinalizer takes 2 arguments");
>+    return JS_FALSE;
>+  }

Unlike every other JS library, ctypes should yell at you if you pass the
wrong number of arguments. (We decided it would be good for ctypes to be
super picky since C is so error-prone.)

So instead of (argc < 2), write (argc != 2).

>+  jsval valCodePtr = argv[1];
>+  if (!JSVAL_IS_OBJECT(argv[1])) {
>+    return TypeError(cx, "an object (a CData object)", valCodePtr);
>+  }

You made a local variable for argv[1], now use it ;-)

Surprisingly JSVAL_IS_OBJECT(JSVAL_NULL) is true, so instead write:
    if (JSVAL_IS_PRIMITIVE(valCodePtr)) { ...

The error message should be:
    "a CData object of a function pointer type"
here and in the next three TypeError calls. The full error message
includes the erroneous value, so it'll be quite specific enough.

>+  //Note: Using a custom argument formatter here would be awkward (requires
>+  //a destructor just to uninstall the formatter).

Yes, argument formatters are awkward for practically every use case. :)
Just delete this comment.

>+    return TypeError(cx, "an object that can be converted to",
>+                     OBJECT_TO_JSVAL(objArgType));

This error message will seem to be truncated. Try it out and see what it
says, then fix it. (Unfortunately we don't have an convenient way to
include the expected type in the error message here.)

>+  AutoFreePtr<void>
>+    rvalue(
>+           malloc(Align(CType::GetSize(cx,
>+                                       funInfoFinalizer -> mReturnType),
>+                        sizeof(ffi_arg)))
>+           );

Be sure to include a test where the return type is void (and thus
sizeArg is 0).

Align() only works when the second argument is a power of two; please
MOZ_STATIC_ASSERT this fact and/or add a MOZ_ASSERT to that effect in
Align() itself (size_t x is a power of 2 iff x != 0 && (x & (x - 1)) == 0).

>+  JSObject *objRoot = JS_GetGlobalForScopeChain(cx);
>+  MOZ_ASSERT(objRoot);
>+
>+  JSObject *objCTypes;
>+  if (!GetObjectProperty(cx, objRoot, "ctypes", &objCTypes)) {
>+    MOZ_NOT_REACHED("no ctypes");
>+  }
>+
>+  JSObject *objCDataFinalizer;
>+  if (!GetObjectProperty(cx, objCTypes, "CDataFinalizer", &objCDataFinalizer)) {
>+    MOZ_NOT_REACHED("no CDataFinalizer");
>+  }
>+
>+  JSObject *objProto;
>+  if (!GetObjectProperty(cx, objCDataFinalizer, "prototype", &objProto)) {
>+    MOZ_NOT_REACHED("no prototype");
>+  }

If you decide to name this function ctypes.CDataFinalizer this can be
simplified: use JS_CALLEE(cx, vp) to get the function object, and use
JS_GetProperty to get its "prototype" property.

In any case, do not use MOZ_NOT_REACHED here. Instead return false on
error.

>+  // 6. Add strong pointers in slot
>+  // Used by |toString| and |forget|
>+  if (!JS_SetReservedSlot(cx, objResult,
>+                          SLOT_DATAFINALIZER_JSVAL,
>+                          valData)) {
>+    return JS_FALSE;
>+  }

(Remember to delete this code.)

>+  // Strong reference to prevent |funInfoFinalizer| from being gc-ed.
>+  if (!JS_SetReservedSlot(cx, objResult,
>+                          SLOT_DATAFINALIZER_CDATA_FINALIZER,
>+                          valCodePtr)) {
>+    return JS_FALSE;
>+  }

This does not keep funInfoFinalizer from being freed, because finalizers
are called in an arbitrary order. This CDataFinalizer and the CType
could be freed at the same time.

Therefore I think we need our own copy of the ffiinfo, not just a
pointer to this one.

>+  // Used by GetCType
>+  if (!JS_SetReservedSlot(cx, objResult,
>+                          SLOT_DATAFINALIZER_VALTYPE,
>+                          OBJECT_TO_JSVAL(objArgType))) {
>+    return JS_FALSE;
>+  }

(Remember to delete this code.)

In the comment on CDataFinalizer::Methods::Forget:
>+ * Calls the finalizer, cleans up the Private memory and releases all
>+ * strong references.

This is inaccurate; forget() is the one that doesn't call the finalizer.

In CDataFinalizer::Methods::Forget:
>+  jsval valJSData;
>+  if (!JS_GetReservedSlot(cx, obj,
>+                          SLOT_DATAFINALIZER_JSVAL,
>+                          &valJSData)) {
>+    MOZ_NOT_REACHED("Value partially finalized");
>+  }

We don't necessarily want to return a CData object here. Instead we want
to follow the ConvertToJS rules (see
<https://wiki.mozilla.org/Jsctypes/api#Conversions> --this is what
comment 4 was about, sorry that wasn't clear.

Also, instead of keeping the jsval around just in case we call forget
(which is probably going to be rare in practice), we should create the
jsval here, on demand.

Use ConvertToJS(cx, GetCType(cx, obj), NULL, p->cargs, false, true, &valJSData).

Comment on CDataFinalizer::Methods::Dispose:
>+/**
>+ * Cleanup the value.
>+ *
>+ * Preconditions: |this| must be the result of |AttachFinalizer|
>+ * and must not have gone through |Forget|/|Dispose| or |Finalize| yet.

Grammar nit: "Clean up the value."  (For some speakers, including me,
"cleanup" can only be used as a noun. Many English speakers don't care
either way. It's the same with "setup", though I think the use of
"setup" as a verb is a bit more established.)

Pedantic nit: This isn't really a precondition, since the behavior is
well-defined whether these things are true or not. Feel free to change
the wording or not as you think best.

Style nit: Right after this you have two blank lines; please delete one
of them.

In CDataFinalizer::Methods::Dispose:
>+    JS_ReportError(cx, "not a CDataFinalizer");
>+    return JS_FALSE;

I think js::ctypes::TypeError would work fine here (and a few other
places too), and it always returns false, so you can make this a
one-liner.

>+  if (!p) {
>+    JS_ReportError(cx, "Value has already undergone dispose() or forget().");
>+    return JS_FALSE;//We have called |dispose| or |forget| already.
>+  }

I think it'd be better if the error message included the word
"CDataFinalizer". It's a little unclear what it's talking about
otherwise.

Please delete the comment, which is redundant with the error message.

In CDataFinalizer::Methods::Dispose:
>+  if (!PrepareCIF(cx, p -> ffiinfo)) {
>+    return JS_FALSE;
>+  }
>+
>+  ffi_call(&p -> ffiinfo -> mCIF, FFI_FN(p -> code),
>+           p -> rvalue, &p -> cargs);

This is the tricky bit. Don't repeat it! Factor it out into a separate
function called from both places.

In CDataFinalizer::Finalize:
>+  if (!PrepareCIF(cx, p -> ffiinfo)) {
>+    MOZ_NOT_REACHED("Could not prepare CIF");
>+  }

Hmm. MOZ_NOT_REACHED is for things that really can't happen.

I'm a little worried about CDataFinalizer::Finalize calling
PrepareCIF. That function is designed to be able to fail; if nothing
else, ffi_prep_cif calls machine-dependent code that could fail for any
reason.

I guess we also have the problem that the data we're using here may
already have been GC'd.

Perhaps the ffi_cif could be a field of CDataFinalizer::Private, and it
could be prepared when the CDataFinalizer is created.

>+void
>+CDataFinalizer::Cleanup(JSContext *cx, CDataFinalizer::Private *p,
>+                        JSObject *obj)
>+{
>+  //return;

Remove the stray comment.

>+  free(p -> cargs);
>+  free(p -> rvalue);
>+  free(p);

free() is correct for p->cargs and p->rvalue, but p itself was allocated
using JS_malloc and must be freed using JS_free.

Comment 22

[Jason Orendorff [:jorendorff]]

Here are some properties that could be tested. Maybe you are already testing some of these.

- You can arrange for a non-empty CDataFinalizer to be GC'd and the finalizer
  actually gets called.
- A CDataFinalizer that is reachable survives GC and the finalizer isn't called.
- Calling withFinalizer with the wrong number of arguments throws.
- The finalizer argument type can be 32 bits in size.
- The finalizer argument type can be 64 bits in size.
- The finalizer argument type can be pointer-sized.
- The finalizer return type can be void.
- The first argument to withFinalizer can be null. It actually passes null
  to the finalizer when dispose() is called.
- If the second argument to withFinalizer is null or undefined, it throws.
- If the finalizer argument type is a 64-bit integer type, forget() returns a
  ctypes.UInt32 or ctypes.UInt64 object. 
- If the finalizer argument type is a pointer type, forget() returns a CData
  object of the appropriate type.
- After forget(), the finalizer is not called.
- A call to forget() after forget() throws.
- A call to forget() after dispose() throws.
- dispose() calls the finalizer.
- After dispose(), the finalizer is not called again.
- A call to dispose() after forget() throws.
- Thousands of CDataFinalizer objects can exist and can be GC'd; the finalizers are
  called exactly once per CDataFinalizer.
- ctypes.withFinalizer(val, my_function_ptr_t(0)) should throw an Error, rather
  than create an object that just crashes during some later GC cycle. (Alternatively
  we could just let it crash--an API design decision.)
- A CDataFinalizer that is part of a garbage cycle gets collected.
- Each error path works.
- .toString() on an empty CDataFinalizer doesn't crash.
- A non-empty CDataFinalizer can be passed to a C function.
- Trying to pass an empty CDataFinalizer to a C function throws.
- A non-empty CDataFinalizer can be passed to a function that expects a slightly
  different type, such as a larger integer type.
- The value of a non-empty CDataFinalizer can be gotten by doing e.g.
  ctypes.int32_t(ctypes.withFinalizer(42, finalizerfn)) ===> 42
- Trying to get the value of an empty CDataFinalizer that way throws.

Pick the ones you think might find bugs or catch JS engine bugs or JS GC bugs in the future. Or just do all of them. It'll take a day. Every time I do it this way, I find bugs.

Please don't test everything in one script. Make lots of small test programs (in the style of js/src/jit-test). That way it's easier for future contributors to see how to add a test, each individual test runs faster, and it's easier to figure out what is broken when a test fails.

Comment 23

[Jason Orendorff [:jorendorff]]

Oh--btw, this code should save and restore errno around the finalizer call in CDataFinalizer::Finalize. GC is infallible; we must catch and silence any errors that occur.

I just mean two extra lines of code, like this:

>+    int saved_errno = errno;
>     ... code to call finalizer ...
>+    errno = saved_errno;

You can write a test for this, either by adding entry points in toolkit/components/ctypes/tests to get and set errno or by fixing bug 684017. There are two things to test:

- finalizers that call C stdlib functions that succeed don't clobber errno
- finalizers that call C stdlib functions that fail don't clobber errno

I think dispose() should *either* save and restore errno *or* return the finalizer function's return value (so that the caller may check for errors). I have no opinion as to which is better.

Comment 24

[David Teller [:Yoric] - still alive but not very active]

Thanks for the detailed review. I have started applying your feedback. However, I have one issue regarding the ffiinfo. The reason for which I preferred copying it at least for a first version was that I had no clear idea of exactly how to manage that memory: |FunctionInfo| contains itself pointers to |ffi_cif|, to a number of |JSObject*|, etc. |ffi_cif| contains itself a number of pointers, etc.

Do you have a clear idea of exactly _what_ I should copy and _what_ I can/should deallocate?

Comment 25

[Jason Orendorff [:jorendorff]]

I *think* the rules are like this:

- libffi types never point to any ctypes data structure.

- ffi_cif and friends are actually pretty flat and simple once you chase down all the pointers.

- libffi does no memory management itself. You're free to allocate them however you want. Stack, heap, copied, shared, partially shared, whatever.

I realize this is just background information and not the "should" statements you were asking for; let's chat on IRC and fill in the rest. I'll be online this afternoon.

Comment 26

[David Teller [:Yoric] - still alive but not very active]

Attachment: Companion testsuite

Comment 27

[David Teller [:Yoric] - still alive but not very active]

(In reply to Jason Orendorff [:jorendorff] from comment #21)
> I'm afraid this is a long review, but the patch is basically sound.
> 
> My main complaint is that there are not enough tests; more on that in a
> separate comment.
> 
> In CTypes.h, enum CDataFinalizerSlot:
> >+  SLOT_DATAFINALIZER_JSVAL             = 1, // The value to finalize.
> >+  // We hold it to permit |forget|. Can be any |jsval|.
> 
> I don't think we need this slot. More about this below.
> 
> >+  SLOT_DATAFINALIZER_VALTYPE           = 2, // The type of the value
> >+  // We hold it to permit |ImplicitConvert|. Is a JSObject *
> 
> We can probably do without this slot too, since this is the same as the
> type of the first argument to the finalizer, which is stored in
> SLOT_DATAFINALIZER_CDATA_FINALIZER.

IIRC, extracting this would have required a request during finalization, which made this slot necessary. On the other hand, I am now rid of the other slots.

> >+  namespace Methods {
> 
> Heh. I don't think we need to go quite this far with namespaces--we
> haven't with any of the others--but it's your call. :)

I personally consider this more readable. So if you do not mind, I will keep it :)

> In ImplicitConvert:
> >   // First, check if val is a CData object of type targetType.
> >   JSObject* sourceData = NULL;
> >   JSObject* sourceType = NULL;
> >-  if (!JSVAL_IS_PRIMITIVE(val) &&
> >-      CData::IsCData(cx, JSVAL_TO_OBJECT(val))) {
> >+  if (!JSVAL_IS_PRIMITIVE(val)) {
> >     sourceData = JSVAL_TO_OBJECT(val);
> >+    if (CData::IsCData(cx, sourceData)) {
> 
> Update the comment here, since you're adding new behavior.
> 
> This introduces a bug when val is not a CData object. Note that
> sourceData is checked later on in the function to see if val is a CData
> object; if sourceData is non-null, it calls CData::GetData(cx, sourceData)
> and so on. Make sure there is a test that checks for this bug.

I am afraid that I do not quite understand the bug.

> >+        memmove(buffer, p -> cargs, p -> cargs_size);
> 
> Style nit: throughout the patch, don't put spaces around "->".

Why does everybody keep telling me that? :)

> >+// Utility function to access a property of an object as an object
> >+// @return false if the property does not exist or is not an object
> >+bool GetObjectProperty(JSContext *aCx, JSObject *aObject,
> >+                       const char *aProperty, JSObject **aOutResult)

> 
> I think this should be unnecessary and it should be removed, but in case
> you decide to keep it, (1) note that JSVAL_IS_OBJECT(v) actually returns
> true if v is JSVAL_NULL, contrary to common sense; and (2) you can't
> return false if an error hasn't been reported. JS_GetProperty will
> report an error on failure, but JSVAL_IS_OBJECT never reports an error.

I think that function makes sense.

> 
> In CData::AttachFinalizer:
> >+ * Pseudo-JS signature:
> >+ * function(CData<T>, CData<T -> U>): CDataFinalizer<T>
> >+ *          value,    finalizer
> 
> Nice comment.

:)

> It should be changed so that it only keeps strong references to
> |finalizer|.

Actually, now, it keeps a reference (only) to the value, which makes sense for a finalizer. I am a bit worried regarding what could happen if the finalizer is a closure, though.

> The error message should be:
>     "a CData object of a function pointer type"
> here and in the next three TypeError calls. The full error message
> includes the erroneous value, so it'll be quite specific enough.

I wanted to make it clear which part of the condition was not fulfilled, which is not that evident. 

> >+    return TypeError(cx, "an object that can be converted to",
> >+                     OBJECT_TO_JSVAL(objArgType));
> 
> This error message will seem to be truncated. Try it out and see what it
> says, then fix it. (Unfortunately we don't have an convenient way to
> include the expected type in the error message here.)


[TODO]

> >+  AutoFreePtr<void>
> >+    rvalue(
> >+           malloc(Align(CType::GetSize(cx,
> >+                                       funInfoFinalizer -> mReturnType),
> >+                        sizeof(ffi_arg)))
> >+           );
> 
> Be sure to include a test where the return type is void (and thus
> sizeArg is 0).
> 
> Align() only works when the second argument is a power of two; please
> MOZ_STATIC_ASSERT this fact and/or add a MOZ_ASSERT to that effect in
> Align() itself (size_t x is a power of 2 iff x != 0 && (x & (x - 1)) == 0).

[TODO]

> 
> >+  JSObject *objRoot = JS_GetGlobalForScopeChain(cx);
> >+  MOZ_ASSERT(objRoot);
> >+
> >+  JSObject *objCTypes;
> >+  if (!GetObjectProperty(cx, objRoot, "ctypes", &objCTypes)) {
> >+    MOZ_NOT_REACHED("no ctypes");
> >+  }
> >+
> >+  JSObject *objCDataFinalizer;
> >+  if (!GetObjectProperty(cx, objCTypes, "CDataFinalizer", &objCDataFinalizer)) {
> >+    MOZ_NOT_REACHED("no CDataFinalizer");
> >+  }
> >+
> >+  JSObject *objProto;
> >+  if (!GetObjectProperty(cx, objCDataFinalizer, "prototype", &objProto)) {
> >+    MOZ_NOT_REACHED("no prototype");
> >+  }
> 
> If you decide to name this function ctypes.CDataFinalizer this can be
> simplified: use JS_CALLEE(cx, vp) to get the function object, and use
> JS_GetProperty to get its "prototype" property.
> 
> In any case, do not use MOZ_NOT_REACHED here. Instead return false on
> error.

[TODO]


Attaching a work in progress that addresses most of your feedback.

Comment 28

[David Teller [:Yoric] - still alive but not very active]

Attachment: Complete patch

Comment 29

[David Teller [:Yoric] - still alive but not very active]

Attachment: Companion testsuite

Comment 30

[David Teller [:Yoric] - still alive but not very active]

Attachment: Complete patch

Comment 31

[David Teller [:Yoric] - still alive but not very active]

Attachment: Complete patch

Comment 32

[David Teller [:Yoric] - still alive but not very active]

Attachment: Companion testsuite

Comment 33

[David Teller [:Yoric] - still alive but not very active]

Attaching a few patches that should cover everything. TryServer tests pending.

A few questions remain open:
- how can I prevent users from attempting to attach a closure as a finalizer?
- how can I test cases with cycles? (see bug 727371)
- should I write converters for jsidToBigInteger, etc.?

Comment 34

[Mozilla RelEng Bot]

Try run for 6a1e8485ecd7 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=6a1e8485ecd7
Results (out of 14 total builds):
    exception: 12
    failure: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-6a1e8485ecd7

Comment 35

[Mozilla RelEng Bot]

Try run for c6d299c3e81c is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=c6d299c3e81c
Results (out of 14 total builds):
    failure: 14
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-c6d299c3e81c

Comment 36

[David Teller [:Yoric] - still alive but not very active]

Attachment: Complete patch

Comment 37

[David Teller [:Yoric] - still alive but not very active]

Attachment: Companion testsuite

Comment 38

[David Teller [:Yoric] - still alive but not very active]

Attachment: Companion testsuite

Attaching an updated testsuite, which now passes under Windows.

Comment 39

[Mozilla RelEng Bot]

Try run for 9e0f086c2f6e is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=9e0f086c2f6e
Results (out of 28 total builds):
    success: 25
    warnings: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-9e0f086c2f6e

Comment 40

[Jason Orendorff [:jorendorff]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #33)
> A few questions remain open:
> - how can I prevent users from attempting to attach a closure as a finalizer?

CDataFinalizer::Construct already checks for this and throws a TypeError in this case. It looks fine! (If there's not already a test for this, please add one.)

If you'd like to throw a more specific error message in this (presumably common) case, that would be OK.

I hope that answers your question.

> - how can I test cases with cycles? (see bug 727371)

Let me get back to you on this one.

> - should I write converters for jsidToBigInteger, etc.?

Uhhrrr, I don't understand the question. Do you have some jsid's that you need to convert to BigIntegers in this patch?

Comment 41

[David Teller [:Yoric] - still alive but not very active]

(In reply to Jason Orendorff [:jorendorff] from comment #40)
> (In reply to David Rajchenbach Teller [:Yoric] from comment #33)
> > A few questions remain open:
> > - how can I prevent users from attempting to attach a closure as a finalizer?
> 
> CDataFinalizer::Construct already checks for this and throws a TypeError in
> this case. It looks fine! (If there's not already a test for this, please
> add one.)

I will do that.

> If you'd like to throw a more specific error message in this (presumably
> common) case, that would be OK.

I have unfortunately no clue as to how I can detect closures.

> I hope that answers your question.
> 
> > - how can I test cases with cycles? (see bug 727371)
> 
> Let me get back to you on this one.
> 
> > - should I write converters for jsidToBigInteger, etc.?
> 
> Uhhrrr, I don't understand the question. Do you have some jsid's that you
> need to convert to BigIntegers in this patch?

Let me rephrase: I patch jsvalToInteger, jsvalToFloat, jsvalToBigInteger, jsvalToIntegerExplicit for CDataFinalizer. I just realize that I have not patched jsvalToBool or jsvalToPointerExplicit, which is probably an error. I believe that these changes are required. However, I am really not sure about jsidToBigInteger and jsidToSize, as I have no clear idea what they are used for.

Comment 42

[Jason Orendorff [:jorendorff]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #41)
> Let me rephrase: I patch jsvalToInteger, jsvalToFloat, jsvalToBigInteger,
> jsvalToIntegerExplicit for CDataFinalizer. I just realize that I have not
> patched jsvalToBool or jsvalToPointerExplicit, which is probably an error.

Oh. I don't think any of those should be modified; *everything* should go through ImplicitConvert and ExplicitConvert, so the only place you need to insert special code for converting CDataFinalizer objects is very close to the top of those two functions.

The five places where 


 I
> believe that these changes are required. However, I am really not sure about
> jsidToBigInteger and jsidToSize, as I have no clear idea what they are used
> for.

Comment 43

[Jason Orendorff [:jorendorff]]

Sorry, disregard comment 42.

(In reply to David Rajchenbach Teller [:Yoric] from comment #41)
> Let me rephrase: I patch jsvalToInteger, jsvalToFloat, jsvalToBigInteger,
> jsvalToIntegerExplicit for CDataFinalizer. I just realize that I have not
> patched jsvalToBool or jsvalToPointerExplicit, which is probably an error.

As I was saying -- jsvalToInteger and jsvalToFloat should not be modified.  Those functions are only called from ImplicitConvert and ExplicitConvert.  The main place you need to insert special code for converting CDataFinalizer objects is very close to the top of those two functions.  That will cover jsvalToInteger, jsvalToIntegerExplicit, jsvalToFloat, jsvalToBool, and jsvalToPtrExplicit all at once, plus a lot more possible conversions that you may have missed.

It's OK to patch jsvalToInteger since we have one or two callers that bypass ExplicitConvert to call it directly (yuck).

jsvalToBigInteger is never called from ImplicitConvert or ExplicitConvert, so it's OK to patch that.

> However, I am really not sure about
> jsidToBigInteger and jsidToSize, as I have no clear idea what they are used
> for.

No problem. Just ignore them. A jsid is a property name. It can't be a CDataFinalizer.

Comment 44

[David Teller [:Yoric] - still alive but not very active]

Attachment: Complete patch

Attaching a new version of the patch. This version removes jsvalToFloat conversion, moves my code to more reasonable places around jsvalToInteger conversion, implements proper |toSource| and |toString|. Note that I have applied your |toString| sugegstion of #c21 to |toSource| rather than |toString|, as I prefer a version of |toString| that favors human-readability, and hence uses the various functions |toString| defined by each types.

Comment 45

[David Teller [:Yoric] - still alive but not very active]

Attachment: Companion testsuite

Comment 46

[David Teller [:Yoric] - still alive but not very active]

Attachment: Complete patch

Attaching a new version of the patch, updated as that of bug 684017, to be able to work even if ctypes.jsm is opened within some scope.

Comment 47

[David Teller [:Yoric] - still alive but not very active]

Attachment: Companion testsuite

Comment 48

[Jason Orendorff [:jorendorff]]

I will review this Friday.

Comment 49

[Jason Orendorff [:jorendorff]]

Comment on attachment 602389 [details] [diff] [review]
Complete patch

Review of attachment 602389 [details] [diff] [review]:
-----------------------------------------------------------------

OK, mostly nits here, but a few comments about error handling that need to be addressed.

r=me with those fixed, and I'm sorry for the slow review.

::: js/src/ctypes/CTypes.cpp
@@ +192,5 @@
> +namespace CDataFinalizer {
> +  /**
> +   * Attach a C function as a finalizer to a JS object.
> +   *
> +   * This function is available from JS as |ctypes.withFinalizer|.

No javadoc-style comments please. This includes the "/**" frob.

@@ +1124,5 @@
> +  }
> +
> +  if (!JSVAL_IS_OBJECT(val) || JSVAL_IS_NULL(val)) {
> +    return false;
> +  }

This must report an error before returning false.

(JS_GetProperty only returns false on error, so the error has already been reported and it's OK to just 'return false;'. This is different because JSVAL_IS_OBJECT/NULL are just predicates; they never throw.)

Also: use JSVAL_IS_PRIMITIVE here.

@@ +2009,5 @@
> +        JS_GetPrivate(sourceData);
> +
> +      if (!p) {
> +        JS_ReportError(cx, "Attempting to convert an empty CDataFinalizer");
> +        return JS_FALSE;// We have called |dispose| or |forget| already.

In js/src, leave a space or two between code and a comment on the same line.

Actually this one would be fine on a line of its own, perhaps before the JS_ReportError.

@@ +2016,5 @@
> +      // If the types are equal, copy the buffer contained within the CData.
> +      // (Note that the buffers may overlap partially or completely.)
> +      if (CType::TypesEqual(sourceType, targetType)) {
> +
> +        memmove(buffer, p->cargs, p->cargs_size);

Remove the blank line before memmove.

Can the buffers really overlap, as the comment says? I don't think they can in this case. I'm still fine with memmove, just want the comment to be acccurate.

@@ +6331,5 @@
> +
> +    JSString *srcDispose =
> +      CData::GetSourceString(cx, JSVAL_TO_OBJECT(valCodePtrType),
> +                             &(p->code));
> +    AppendString(source, srcDispose);

Check for !srcDispose here.

@@ +6402,5 @@
> +    JS_GetPrivate(obj);
> +
> +  if (!p) {
> +    return false;// We have called |dispose| or |forget| already.
> +  }

This must report an error before returning false.

The most likely effect of returning false without reporting an error is to mysteriously terminate the calling JS code without leaving an error message anywhere.

CDataFinalizer::Methods::ToString is the only place where it's not really an error if the object has already been disposed, and there you can just check before calling GetValue.

@@ +6428,5 @@
> +  JSObject* objSelf = JSVAL_TO_OBJECT(JS_CALLEE(cx, vp));
> +  if (!objSelf) {
> +    JS_ReportError(cx, "not function ctypes.CDataFinalizer");
> +    return JS_FALSE;
> +  }

There's no need to check for null here. JS_CALLEE returns the function object that was called, and that's never null.

@@ +6593,5 @@
> + * @param p A non-null private.
> + * @param updateStatus If |true|, update |ctypes.getStatus|.
> + */
> +void
> +CDataFinalizer::CallFinalizer(JSContext *cx, CDataFinalizer::Private *p,

You can remove cx now that JS_SetReservedSlot does not require a cx argument anymore. Yay!

@@ +6597,5 @@
> +CDataFinalizer::CallFinalizer(JSContext *cx, CDataFinalizer::Private *p,
> +                              JSObject *ctypes)
> +{
> +  ffi_call(&p->CIF, FFI_FN(p->code),
> +           p->rvalue, &p->cargs);

Style nit: Fit it all on one line please.

@@ +6611,5 @@
> +  valStatus = INT_TO_JSVAL(GetLastError());
> +#else
> +  valstatus = JSVAL_VOID;
> +#endif
> +  JS_SetReservedSlot(ctypes, SLOT_ERRNO, valStatus);

GC definitely shouldn't affect ctypes.errno, right?

I think it's fine for .dispose() to affect ctypes.errno (or not -- slight preference for affecting it). But when this is called from CDataFinalizer::Finalize, we should just reset the errno/lastError afterwards and act like nothing happened.

(It's gross to be sweeping errors under the rug like this, but normally cleanup functions shouldn't fail, and this *is* a fallback mechanism, after all. Code will ordinarily call .dispose() explicitly, we hope.)

@@ +6734,5 @@
> + * @param obj Either NULL, if the object should not be cleaned up (i.e.
> + * during finalization) or a CDataFinalizer JSObject.
> + */
> +void
> +CDataFinalizer::Cleanup(JSContext *cx, CDataFinalizer::Private *p,

cx is unused now that JS_SetReservedSlot does not require a cx argument anymore.

@@ +6738,5 @@
> +CDataFinalizer::Cleanup(JSContext *cx, CDataFinalizer::Private *p,
> +                        JSObject *obj)
> +{
> +  if (!p) {
> +    return;//We have already cleaned up

space

@@ +6746,5 @@
> +  free(p->rvalue);
> +  free(p);
> +
> +  if (!obj) {
> +    return;//No slots to clean up

space

::: js/src/ctypes/CTypes.h
@@ +431,5 @@
>  
> +enum CDataFinalizerSlot {
> +  SLOT_DATAFINALIZER_VALTYPE           = 0, // The type of the value.
> +  // We hold it to permit |ImplicitConvert| and |ToSource|. Is a
> +  // |JSObject *| instantiated to a |CType|.

Style nit: It is more usual for the whole comment to appear before the value:

  enum CDataFinalizerSlot {
    // The type of the value (a CType object).
    // We hold it to permit ImplicitConvert and ToSource.
    SLOT_DATAFINALIZER_VALTYPE = 0,

    // The type of the finalizer function (a CType object).
    // We hold it to permit ToSource.
    SLOT_DATAFINALIZER_CODETYPE = 1,

(We generally don't use |CodeBars| in every possible place but just as needed to avoid confusion.)

Comment 50

[Jason Orendorff [:jorendorff]]

Comment on attachment 602391 [details] [diff] [review]
Companion testsuite

Review of attachment 602391 [details] [diff] [review]:
-----------------------------------------------------------------

Great tests. Thank you.

::: toolkit/components/ctypes/tests/jsctypes-test.cpp
@@ +421,5 @@
> +{ \
> +  fprintf(stderr, "Assertion failed at line %i\n", __LINE__); \
> +  (*(int*)NULL)++; \
> +}
> +// FIXME: Replace by JS_ASSERT - and find out how to link it.

Hmm. I think JS_ASSERT can probably work here. Just #include "jsapi.h" at the top and ask in #developers about the necessary changes to Makefile.in.

@@ +710,5 @@
> +
> +// Avoid some pollution by Windows.h
> +namespace sandbox {
> +#if defined(XP_WIN)
> +#include <Windows.h>

Lowercase <windows.h>.

You mentioned possibly pulling this out into a separate file to avoid the namespace hack, which would be great if it's no trouble. If it's any trouble, don't bother.

::: toolkit/components/ctypes/tests/unit/test_finalizer.js
@@ +230,5 @@
> +
> +  Components.utils.schedulePreciseGC(
> +    function() {
> +      // Check that _something_ has been finalized
> +      do_check_true(count_finalized(size, tc) > 0);

I understand the test isn't running yet, due to bug 727371, but why wouldn't this assert that _all_ the finalizers ran?

@@ +235,5 @@
> +      do_test_finished();
> +    }
> +  );
> +
> +  Components.utils.forceGC();

You don't actually want this forceGC call here, right? It looks like it might be code you put in while you were tinkering with the test, trying to find a way to make it run properly.

@@ +374,5 @@
> +  // Allocate |size| items without references
> +  for (let i = 0; i < size; ++i) {
> +    ref.push(ctypes.CDataFinalizer(tc.acquire(i), tc.release));
> +  }
> +  trigger_gc(); // This should trigger some finalizations

This comment is incorrect; this is the case where no finalizers should be called.

@@ +381,5 @@
> +  do_check_eq(count_finalized(size, tc), 0);
> +
> +  // Attempt to trigger any remaining finalizers, lest they break the following tests
> +  ref = [];
> +  trigger_gc();

Instead, call dispose on them?

(This kind of unwanted dependency between tests is one reason to write lots of separate small test files. What you've done here is fine though.)

::: toolkit/components/ctypes/tests/unit/test_finalizer_shouldaccept.js
@@ +101,5 @@
> +
> +/**
> + * Test that a finalizer
> + */
> +function test_errno(size)

The comment seems unfinished.

::: toolkit/components/ctypes/tests/unit/test_finalizer_shouldfail.js
@@ +69,5 @@
> +    let v = ctypes.CDataFinalizer(dispose, dispose);
> +  } catch (x) {
> +    exception = true;
> +  }
> +  do_check_true(exception);

I would have guessed wild horses could not have stopped you from factoring out the common code here. ;-)

Don't factor it on my account, I don't mind it.

@@ +155,5 @@
> +    v.dispose();
> +  } catch (x) {
> +    exception = true;
> +  }
> +  do_check_true(exception);

However in this case I think a for(i = 0; i < 4; i++) loop would be best.

Comment 51

[David Teller [:Yoric] - still alive but not very active]

(In reply to Jason Orendorff [:jorendorff] from comment #49)
> @@ +2016,5 @@
> > +      // If the types are equal, copy the buffer contained within the CData.
> > +      // (Note that the buffers may overlap partially or completely.)
> > +      if (CType::TypesEqual(sourceType, targetType)) {
> > +
> > +        memmove(buffer, p->cargs, p->cargs_size);
> 
> Remove the blank line before memmove.
> 
> Can the buffers really overlap, as the comment says? I don't think they can
> in this case. I'm still fine with memmove, just want the comment to be
> acccurate.

This was actually copied and pasted from a few lines above. I do not remember why I thought that this comment also applied to my code, though.

> @@ +6402,5 @@
> CDataFinalizer::Methods::ToString is the only place where it's not really an
> error if the object has already been disposed, and there you can just check
> before calling GetValue.

ok.

> 
> @@ +6428,5 @@
> > +  JSObject* objSelf = JSVAL_TO_OBJECT(JS_CALLEE(cx, vp));
> > +  if (!objSelf) {
> > +    JS_ReportError(cx, "not function ctypes.CDataFinalizer");
> > +    return JS_FALSE;
> > +  }
> 
> There's no need to check for null here. JS_CALLEE returns the function
> object that was called, and that's never null.

ok.

> 
> @@ +6593,5 @@
> > + * @param p A non-null private.
> > + * @param updateStatus If |true|, update |ctypes.getStatus|.
> > + */
> > +void
> > +CDataFinalizer::CallFinalizer(JSContext *cx, CDataFinalizer::Private *p,
> 
> You can remove cx now that JS_SetReservedSlot does not require a cx argument
> anymore. Yay!

\o/

> @@ +6611,5 @@
> > +  valStatus = INT_TO_JSVAL(GetLastError());
> > +#else
> > +  valstatus = JSVAL_VOID;
> > +#endif
> > +  JS_SetReservedSlot(ctypes, SLOT_ERRNO, valStatus);
> 
> GC definitely shouldn't affect ctypes.errno, right?
> 
> I think it's fine for .dispose() to affect ctypes.errno (or not -- slight
> preference for affecting it). But when this is called from
> CDataFinalizer::Finalize, we should just reset the errno/lastError
> afterwards and act like nothing happened.
> 
> (It's gross to be sweeping errors under the rug like this, but normally
> cleanup functions shouldn't fail, and this *is* a fallback mechanism, after
> all. Code will ordinarily call .dispose() explicitly, we hope.)

You are right, fixed.

Comment 52

[David Teller [:Yoric] - still alive but not very active]

I have just realized that .dispose() doesn't return anything atm, although the finalization function could have a result (e.g. libc's |close|). I have just opened a dependent bug for that feature: bug 742384.

Comment 53

[David Teller [:Yoric] - still alive but not very active]

(In reply to Jason Orendorff [:jorendorff] from comment #50)
> Comment on attachment 602391 [details] [diff] [review]
> Companion testsuite
> 
> Review of attachment 602391 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Great tests. Thank you.
> 
> ::: toolkit/components/ctypes/tests/jsctypes-test.cpp
> @@ +421,5 @@
> > +{ \
> > +  fprintf(stderr, "Assertion failed at line %i\n", __LINE__); \
> > +  (*(int*)NULL)++; \
> > +}
> > +// FIXME: Replace by JS_ASSERT - and find out how to link it.
> 
> Hmm. I think JS_ASSERT can probably work here. Just #include "jsapi.h" at
> the top and ask in #developers about the necessary changes to Makefile.in.
> 
> @@ +710,5 @@
> > +
> > +// Avoid some pollution by Windows.h
> > +namespace sandbox {
> > +#if defined(XP_WIN)
> > +#include <Windows.h>
> 
> Lowercase <windows.h>.
> 
> You mentioned possibly pulling this out into a separate file to avoid the
> namespace hack, which would be great if it's no trouble. If it's any
> trouble, don't bother.
> 
> ::: toolkit/components/ctypes/tests/unit/test_finalizer.js
> @@ +230,5 @@
> > +
> > +  Components.utils.schedulePreciseGC(
> > +    function() {
> > +      // Check that _something_ has been finalized
> > +      do_check_true(count_finalized(size, tc) > 0);
> 
> I understand the test isn't running yet, due to bug 727371, but why wouldn't
> this assert that _all_ the finalizers ran?

I suppose a stop-the-world gc would but I assumed that an incremental gc might not.

> 
> @@ +235,5 @@
> > +      do_test_finished();
> > +    }
> > +  );
> > +
> > +  Components.utils.forceGC();
> 
> You don't actually want this forceGC call here, right? It looks like it
> might be code you put in while you were tinkering with the test, trying to
> find a way to make it run properly.

Good point.

> 
> @@ +374,5 @@
> > +  // Allocate |size| items without references
> > +  for (let i = 0; i < size; ++i) {
> > +    ref.push(ctypes.CDataFinalizer(tc.acquire(i), tc.release));
> > +  }
> > +  trigger_gc(); // This should trigger some finalizations
> 
> This comment is incorrect; this is the case where no finalizers should be
> called.

right

> 
> @@ +381,5 @@
> > +  do_check_eq(count_finalized(size, tc), 0);
> > +
> > +  // Attempt to trigger any remaining finalizers, lest they break the following tests
> > +  ref = [];
> > +  trigger_gc();
> 
> Instead, call dispose on them?
> 
> (This kind of unwanted dependency between tests is one reason to write lots
> of separate small test files. What you've done here is fine though.)

Good point.


> ::: toolkit/components/ctypes/tests/unit/test_finalizer_shouldfail.js
> @@ +69,5 @@
> > +    let v = ctypes.CDataFinalizer(dispose, dispose);
> > +  } catch (x) {
> > +    exception = true;
> > +  }
> > +  do_check_true(exception);
> 
> I would have guessed wild horses could not have stopped you from factoring
> out the common code here. ;-)

:)

> Don't factor it on my account, I don't mind it.

Too late :)

>
> @@ +155,5 @@
> > +    v.dispose();
> > +  } catch (x) {
> > +    exception = true;
> > +  }
> > +  do_check_true(exception);
> 
> However in this case I think a for(i = 0; i < 4; i++) loop would be best.

Factored in a manner that I found a little more readable than a loop.

Comment 54

[David Teller [:Yoric] - still alive but not very active]

Attachment: Companion testsuite

Comment 55

[David Teller [:Yoric] - still alive but not very active]

Attachment: Complete patch

Comment 56

[David Teller [:Yoric] - still alive but not very active]

Attachment: Companion testsuite

Comment 57

[David Teller [:Yoric] - still alive but not very active]

Attachment: Complete patch

Comment 58

[David Teller [:Yoric] - still alive but not very active]

Attachment: Implementing ctypes.CDataFinalizer

Comment 59

[David Teller [:Yoric] - still alive but not very active]

Attachment: Implementing ctypes.CDataFinalizer

Comment 60

[David Teller [:Yoric] - still alive but not very active]

Attachment: Companion testsuite

Comment 61

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/33e485d0e23b
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe5b6de3c7d1

Comment 62

[Ryan VanderMeulen [:RyanVM]]

The testsuite broke the Linux64 builds. I backed it out.
http://mozillamemes.tumblr.com/post/19498220636/try-server-takes-the-beatings-so-mozilla-inbound

../../../../../toolkit/components/ctypes/tests/jsctypes-test-finalizer.cpp: In function 'RECT test_finalizer_rel_size_t_return_struct_t(size_t)':
../../../../../toolkit/components/ctypes/tests/jsctypes-test-finalizer.cpp:86:30: error: narrowing conversion of 'i' from 'size_t' to 'PRInt32' inside { }
../../../../../toolkit/components/ctypes/tests/jsctypes-test-finalizer.cpp:86:30: error: narrowing conversion of 'i' from 'size_t' to 'PRInt32' inside { }
../../../../../toolkit/components/ctypes/tests/jsctypes-test-finalizer.cpp:86:30: error: narrowing conversion of 'i' from 'size_t' to 'PRInt32' inside { }
../../../../../toolkit/components/ctypes/tests/jsctypes-test-finalizer.cpp:86:30: error: narrowing conversion of 'i' from 'size_t' to 'PRInt32' inside { }

(Also note that test_finalizer was removed, so the changes from bug 742384 were also backed out with it. Make sure your revised patch includes them.)

https://hg.mozilla.org/integration/mozilla-inbound/rev/f097e8dcb6f2

Comment 63

[David Teller [:Yoric] - still alive but not very active]

My confused apologies.

Note that the issue is actually in a patch of bug 742384, so I am re-marking this as check-in needed. Not sure about [don't close when landing on m-c], so I won't touch it.

Comment 64

[Ryan VanderMeulen [:RyanVM]]

Testsuite relanded.
https://hg.mozilla.org/integration/mozilla-inbound/rev/26eb08593f89

Comment 65

[Richard Newman [:rnewman]]

Backed out whole merge for bustage per Yoric (Bug 743574):

https://hg.mozilla.org/integration/mozilla-inbound/rev/12e42fb8e321

Comment 66

[Ryan VanderMeulen [:RyanVM]]

This wasn't backed out. I've confirmed that the new tests are running on inbound.

Comment 67

[Richard Newman [:rnewman]]

(In reply to Ryan VanderMeulen from comment #66)
> This wasn't backed out. I've confirmed that the new tests are running on
> inbound.

Drat, missed commenting on this one. Thanks again for catching.

(Fixing more flags.)

Comment 68

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/33e485d0e23b

Comment 69

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/fe5b6de3c7d1

Comment 70

[(no longer active)]

Tests backed out: https://hg.mozilla.org/mozilla-central/rev/f097e8dcb6f2

Comment 71

[(no longer active)]

Test suite: https://hg.mozilla.org/mozilla-central/rev/26eb08593f89

Comment 72

[Matt Brubeck (:mbrubeck)]

Sorry, we decided to back out this bug because of perma-orange crashes in test_finalizer.js on Linux64 PGO builds (bug 745233):
https://hg.mozilla.org/mozilla-central/rev/e1f0bb28fbb4

Comment 73

[David Teller [:Yoric] - still alive but not very active]

Attachment: Companion testsuite

Since it seems that we are using a garbage-collector sensitive to optimization levels, I have tweaked the test suite to ensure that everything is cleaned-up without waiting for gc at the end of each test phase.

Comment 74

[David Teller [:Yoric] - still alive but not very active]

Ok, TryServer confirms that tests pass.

Comment 75

[Ryan VanderMeulen [:RyanVM]]

Dave, if you want this (and bug 742384) to land on inbound for mozilla14, you'll need to request approval-mozilla-central on them (the testsuite can land without approval, though).

Comment 76

[David Teller [:Yoric] - still alive but not very active]

(In reply to Ryan VanderMeulen from comment #75)
> Dave, if you want this (and bug 742384) to land on inbound for mozilla14,
> you'll need to request approval-mozilla-central on them (the testsuite can
> land without approval, though).

As this is a non-trivial change with the theoretical possibility of busting native Fennec, it was my understanding that I should wait for mozilla15 for landing. Am I correct?

Comment 77

[Ryan VanderMeulen [:RyanVM]]

If you prefer to wait, I'm sure none of the release drivers will mind :)

Comment 78

[(dormant account)]

(In reply to David Rajchenbach Teller [:Yoric] from comment #76)
> (In reply to Ryan VanderMeulen from comment #75)
> > Dave, if you want this (and bug 742384) to land on inbound for mozilla14,
> > you'll need to request approval-mozilla-central on them (the testsuite can
> > land without approval, though).
> 
> As this is a non-trivial change with the theoretical possibility of busting
> native Fennec, it was my understanding that I should wait for mozilla15 for
> landing. Am I correct?

i don't see why this would bust native fennec.

Comment 79

[David Teller [:Yoric] - still alive but not very active]

Ok, in that case I will try and understand exactly how approval-mozilla-central works.

Comment 80

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 613555 [details] [diff] [review]
Implementing ctypes.CDataFinalizer

[Approval Request Comment]
Please see https://wiki.mozilla.org/Tree_Rules for information on mozilla-central landings that do not require approval.

Possible risk to Fennec Native 14 (if any): Patches js-ctypes to add new features. It should not alter the behavior of any existing feature, though.

Comment 81

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 615709 [details] [diff] [review]
Companion testsuite

[Approval Request Comment]
Please see https://wiki.mozilla.org/Tree_Rules for information on mozilla-central landings that do not require approval.

Possible risk to Fennec Native 14 (if any): Just a test suite.

Comment 82

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 613555 [details] [diff] [review]
Implementing ctypes.CDataFinalizer

[Triage Comment]
Merge of 14 to Aurora has completed.  Please renominate for mozilla-aurora.

Comment 83

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b19d2b0fd99d
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ab70585a7b4

Comment 84

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/b19d2b0fd99d
https://hg.mozilla.org/mozilla-central/rev/7ab70585a7b4

Comment 85

[David Teller [:Yoric] - still alive but not very active]

Note to whoever gets to write the MDN documentation: I have put together a blog post with some explanations and examples.

http://dutherenverseauborddelatable.wordpress.com/2012/05/02/c-data-finalization-in-javascript/