745184 - Pointer conversion through CDataFinalizer

Status: RESOLVED WONTFIX

(Core :: js-ctypes, defect)

Description

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

At the moment, a CDataFinalizer upon a pointer cannot be used as a pointer, which is a shame.

We should fix this.

Comment 1

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

Attachment: Implementing jsvalToPtrExplicit

Attaching a prototype patch. The |while| loop is a little strange, so I any suggestion will be welcome.

Comment 2

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

Attachment: Companion testsuite

Comment 3

[Jason Orendorff [:jorendorff]]

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

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

::: toolkit/components/ctypes/tests/jsctypes-test-finalizer.cpp
@@ +177,5 @@
> +int32_t*
> +test_finalizer_acq_int32_ptr_t(size_t i)
> +{
> +  gFinalizerTestResources[i] = 1;
> +  return (int32_t*)&gFinalizerTestResources[i];

Instead of casting here, I think you could make the type of gFinalizerTestResources int32_t*. Just a suggestion.

@@ +185,5 @@
> +void
> +test_finalizer_rel_int32_ptr_t(int32_t *i)
> +{
> +  -- (*i);
> +  if (*i < 0) {

It should be exactly 0, right?

::: toolkit/components/ctypes/tests/unit/test_finalizer_shouldaccept.js
@@ +112,5 @@
> + * Check that a finalizable of a pointer can be used as a pointer
> + */
> +function test_to_pointer()
> +{
> +  let ptr = ctypes.int32_t(2).address();

I don't remember how this turned out, but when designing the API we went back and forth over the GC-safety of stuff like this.

I don't think this is GC-safe, because I don't think the GC can tell that ptr should keep the int32_t object alive.

To make it safe, you could make an explicit edge:
  let obj = ctypes.int32_t(2);
  let ptr = obj.address();
  ptr.referent = obj;  // ptr should keep obj GC-alive

Comment 4

[Jason Orendorff [:jorendorff]]

Actually, why did you make the value of the int32_t 2? Is it supposed to be released twice in that test?

Comment 5

[Jason Orendorff [:jorendorff]]

Comment on attachment 614768 [details] [diff] [review]
Implementing jsvalToPtrExplicit

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

Clearing review flag because I think there's a better approach. If you think the approach in this patch is best, just explain (assuming my objections make any sense to you) and re-request review.

Sorry for the delay. Whenever you post a new patch, or re-request review I'll re-review immediately.

::: js/src/ctypes/CTypes.cpp
@@ +1787,5 @@
>  // Forcefully convert val to a pointer value when explicitly requested.
>  static bool
>  jsvalToPtrExplicit(JSContext* cx, jsval val, uintptr_t* result)
>  {
> +  while (true) {

I'm confused as to why the test doesn't pass already. It seems like ExplicitConvert's call to ImplicitConvert should succeed, i.e. it should be able to convert the CDataFinalizer wrapping an (int32_t *) value to (int32_t *).

Does that test fail without this patch? If so, why? (If not, perhaps obviously we should have a test that fails before, and passes after...)

I think this approach to fixing ExplicitConvert is more complex than needed. Instead just make the same change to ExplicitConvert that you made to ImplicitConvert: at the top of the function, if the value to convert is a CDataFinalizer, get the CDataFinalizer's value and convert that.

@@ +1840,5 @@
> +    }
> +    if (!CDataFinalizer::GetValue(cx, obj, &val)) {
> +      return false;
> +    }
> +    // Otherwise, loop

In JSAPI-using code, every function must either consistently report an error on failure or not. This particular function doesn't, so calling GetValue (which is fallible and does report errors) is a problem.

(However above I'm proposing that you revert all this, so maybe you won't need to look at this too closely. Just mentioning it since JSAPI error handling is a common source of mistakes and minor bugs.)

BTW, tail-recursing here would have been totally fine, instead of the loop, and I think clearer.

@@ +2185,5 @@
>          (*jscharBuffer)[sourceLength] = 0;
>          break;
>        }
>        default:
> +        return TypeError(cx, "string pointer", val);

I think this change is wrong; I think this affects the error thrown for:
  var p = ctypes.voidptr_t(0);
  x.value = "str";

It tells me this:
  TypeError: expected type pointer, got "banana"
I think that's correct, and "expected type string pointer" would be wrong.

@@ +2403,5 @@
>    case TYPE_pointer: {
>      // Convert a number, Int64 object, or UInt64 object to a pointer.
>      uintptr_t result;
>      if (!jsvalToPtrExplicit(cx, val, &result))
> +      return TypeError(cx, "number convertible to a pointer", val);

Similarly, I think this error is thrown when you do:
  ctypes.voidptr_t({});
Currently it says:
  TypeError: expected type pointer, got ({})
and I think that is more correct than "expected type number convertible to a pointer", but both are inaccurate; "pointer or integer" would be an improvement. If you really want to be totally precise, perhaps "pointer or pointer-sized integer".

Comment 6

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

On second thought, it seems that this patch is useless. Pointer conversion does seem to work fine, the only real issues (besides confusing error messages :) ) seem to be fixed by bug 748776.