550982 - Fix REFERENT slot not to keep more objects alive than necessary

Status: RESOLVED FIXED

(Core :: js-ctypes, defect, P1)

Description

[Jason Orendorff [:jorendorff]]

Right now the REFERENT slot can point to a chain of objects, and in particular the REFERENT slot of `ptr.contents` points back to `ptr`.

Instead, a CData object's REFERENT slot should be either

 - JSVAL_NULL if the object owns its own buffer;

 - otherwise, a reference to the complete object that owns the buffer,
   if known;

 - otherwise JSVAL_VOID or some other arbitrary value to indicate that
   we don't know who owns the buffer (it might be owned by C or by a
   CData object that we don't have a reference to).

The slot only serves two purposes: to protect base objects from GC and to determine whether the object owns its buffer (to determine whether to free it, during finalization).

Then we could assert in CData::Create that the baseObj, if provided, owns its own buffer, and that the source argument points into it.

Comment 1

[[e-mail]]

Per bug 513788, at some point we sorta decided that we want a PointerType to hold its CData referent alive (i.e. the object the PointerType points into). In this case, the PointerType object owns its own buffer (the void* pointer itself), but it also has a referent that it wants to keep alive.

The rationale for this is that we can then autoconvert JS strings to char.ptr's. (We presently only do this for ffi function params, where we have an opportunity to manually manage lifetime.) When autoconverting, we need to allocate a buffer as a char.array(), and then take .address() to get the char.ptr. Thus the PointerType needs to keep the char.array() alive.

We probably want to have a referent in the general case of .address(). The original idea was not to do this because it imparts a false sense of safety, which is void when the PointerType comes from some value escaped from C.

Comment 2

[[e-mail]]

Attachment: patch v1

Alas, we can't overload SLOT_REFERENT as suggested; it'll work now, but not when we allow library and closure objects to go into that slot in bug 513778. (Reason: CData::Finalize() would need to know what type of object SLOT_REFERENT is in order to know whether to assume ownership, but to do so it must JS_GET_CLASS the object, which is verboten in a finalizer.)

Thus, this keeps it real simple and adds a SLOT_OWNS, and makes 'ptr.contents' not refer to 'ptr'. Covered by existing tests.

Comment 3

[[e-mail]]

Comment on attachment 433677 [details] [diff] [review]
patch v1

Kicking to benjamn.

Comment 4

[Ben Newman (:bnewman) (:benjamn)]

Comment on attachment 433677 [details] [diff] [review]
patch v1

>@@ -2791,17 +2794,17 @@ PointerType::ContentsGetter(JSContext* c
> 
>   void* data = *static_cast<void**>(CData::GetData(cx, obj));
>   if (data == NULL) {
>     JS_ReportError(cx, "cannot read contents of null pointer");
>     return JS_FALSE;
>   }
> 
>   jsval result;
>-  if (!ConvertToJS(cx, baseType, obj, data, false, &result))
>+  if (!ConvertToJS(cx, baseType, NULL, data, false, false, &result))

Just a sanity check.  This is the only functionality change introduced by this patch, right?  And

(In reply to comment #2)
> Thus, this... makes 'ptr.contents'
> not refer to 'ptr'.

is what you're doing by changing obj to NULL, if I'm following you correctly?

> // Create a new CData object of type 'typeObj' containing binary data supplied
>-// in 'source', optionally with a referent CData object 'baseObj'.
>+// in 'source', optionally with a referent object 'refObj'.
---8<---
>+// * If an object 'refObj' is supplied, the new CData object stores the
>+//   referent object in a reserved slot for GC safety, such that 'refObj' will
>+//   be held alive by the resulting CData object.
>+// * If 'ownResult' is true, the CData object will allocate an appropriately
>+//   sized buffer, and free it upon finalization. If 'source' data is supplied,
>+//   the data will be copied from 'source' into the buffer; otherwise, the
>+//   entirety of the new buffer will be initialized to zero.
>+// * If 'ownResult' is false, the new CData's buffer refers to a slice of
>+//   another CData's buffer given by 'refObj'. 'source' data must be provided,
>+//   and the new CData's buffer will refer to 'source'.
>+JSObject*
>+CData::Create(JSContext* cx,
>+              JSObject* typeObj,
>+              JSObject* refObj,
>+              void* source,
>+              bool ownResult)
> {
---8<---
>+  JS_ASSERT(!refObj || (!ownResult && CData::IsCData(cx, refObj)));

This last assertion is stronger than what the comments seem to require.  My instinct is that you should add something to the comments about the relationship between refObj and ownResult (they exclude one another, right?).  Also, I would prefer breaking up the logic, so that you have the simple implication

    JS_ASSERT(!refObj || CData::IsCData(cx, refObj));

followed by one of

    JS_ASSERT(!refObj || !ownResult);

or

    JS_ASSERT(!refObj == ownResult);

depending upon whether or not it's ok for !refObj && !ownResult to be true.

>-  // root the base object, if any
>-  if (!JS_SetReservedSlot(cx, dataObj, SLOT_REFERENT, OBJECT_TO_JSVAL(baseObj)))
>+  // Stash the referent object, if any, for GC safety.
>+  if (refObj &&
>+      !JS_SetReservedSlot(cx, dataObj, SLOT_REFERENT, OBJECT_TO_JSVAL(refObj)))
>+    return NULL;

Do we not want to set the REFERENT slot to JSVAL_NULL if !refObj?

Comment 5

[[e-mail]]

(In reply to comment #4)

> Just a sanity check.  This is the only functionality change introduced by this
> patch, right?  And

Yes, it's supposed to be. :)

> (In reply to comment #2)
> > Thus, this... makes 'ptr.contents'
> > not refer to 'ptr'.
> 
> is what you're doing by changing obj to NULL, if I'm following you correctly?

Right.

> >+  JS_ASSERT(!refObj || (!ownResult && CData::IsCData(cx, refObj)));
> 
> This last assertion is stronger than what the comments seem to require.  My
> instinct is that you should add something to the comments about the
> relationship between refObj and ownResult (they exclude one another, right?). 
> Also, I would prefer breaking up the logic, so that you have the simple
> implication

It's asserting that either a) refObj is nonnull, in which case refObj must be a CData and we cannot own our own buffer; or b) refObj is null, in which case we may or may not own our own buffer. I can break it up and tweak the comment to be clearer.

To make this extra fun, bug 513778 changes this condition: it adds c) refObj is nonnull, and refObj is *not* a CData (meaning it is a library or closure object), then we must own our own buffer.

> Do we not want to set the REFERENT slot to JSVAL_NULL if !refObj?

jsengine inits it to JSVAL_VOID, which is fine by me.

Comment 6

[Ben Newman (:bnewman) (:benjamn)]

Comment on attachment 433677 [details] [diff] [review]
patch v1

(In reply to comment #5)
> It's asserting that either a) refObj is nonnull, in which case refObj must be a
> CData and we cannot own our own buffer; or b) refObj is null, in which case we
> may or may not own our own buffer. I can break it up and tweak the comment to
> be clearer.

Ok, then breaking it up into

    JS_ASSERT(!refObj || CData::IsCData(cx, refObj));
    JS_ASSERT(!refObj || !ownResult);

would be my suggestion.  This is perfectly equivalent to what you had before, but more readable, IMO.

r=me with that.

Comment 7

[[e-mail]]

P1, need this for 1.9.3.

Comment 8

[[e-mail]]

Comment on attachment 433677 [details] [diff] [review]
patch v1

http://hg.mozilla.org/mozilla-central/rev/d08ad90e7bc5

Comment 9

[[e-mail]]

Fixed, with requested changes.