516077 - Be strict about JS argument types

Status: RESOLVED FIXED

(Core :: js-ctypes, defect)

Description

[Jason Orendorff [:jorendorff]]

Type-checking at the jsctypes boundary is a very, very good thing. Let's do a little more of that.

We shouldn't silently lose precision. Passing the JS value 7.5 to a parameter of type int32 should throw.

We shouldn't let out-of-range errors pass silently.  Passing 1e80 to a parameter of type int8 should throw.

We shouldn't auto-convert JS bool to native double.  It's too much of a stretch--that most likely indicates a bug in the caller's code.

We shouldn't auto-convert JS objects to simple native types like bool, int32, and string by calling .toString() or .valueOf(). fopen(window.document) should throw.

We shouldn't auto-convert strings to numbers or vice versa.

We definitely shouldn't try to convert JS booleans, numbers, or strings silently to C structs.

Treating these as errors detects bugs sooner and closer to the cause, and encourages jsctypes users to think carefully about types. All good stuff I think.

Writing the desired conversion explicitly in JS is easy. If a particular function should be lax, it's very easy to wrap it:

  const _puts = library.declare("puts", ctypes.INT, ctypes.STRING);
  function puts(s) {
    _puts(""+s);  // autoconvert to string
  }

I have a patch, but at the moment it's crashing, so I need to debug a bit. Monday, hopefully.

Comment 1

[[e-mail]]

Awesome. I've hacked nsNativeMethod to pieces (locally) in preparation for trunk landing, but if you post a patch I'll make sure it gets incorporated.

Comment 2

[Jason Orendorff [:jorendorff]]

Attachment: v1

Comment 3

[Jason Orendorff [:jorendorff]]

It was crashing because Execute wasn't checking the return value of Prepare for errors and was proceeding with the call anyway. Easily fixed.

The error messages could be improved easily. JS_ReportError takes printf-style format strings.

One thing I foresee about this is, it'll make people want the corresponding UINT types (because of the range checking on int8, int16, and int32).

The remaining implicit conversion that bothers me the most is when we convert an int64 return value to jsval and it becomes a jsdouble, with loss of precision if the number is large. (The implicit conversion from numeric jsval to float is also lossy, but that doesn't really bother me.)

Comment 4

[[e-mail]]

Incorporated your patch in bug 513783, for trunk landing - thanks! (I left the struct bits til later, but I'm hoping we can chew through that stuff real fast.)

Also added unsigned types and correspondingly appropriate range checking.

One detail I noticed while browsing JSAPI docs - JS_GetStringChars said something about not necessarily providing a UTF16-sane string. Is this something to worry about? Do we need to sanitize it?

Re int64's, we could throw if we don't have enough bits for it. What do you think? Silently changing a return value is pretty scary...

Comment 5

[[e-mail]]

Fixed on trunk and 1.9.2 per bug 513783.