Closed
Bug 516077
Opened 15 years ago
Closed 15 years ago
Be strict about JS argument types
Categories
(Core :: js-ctypes, defect)
Core
js-ctypes
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
status1.9.2 | --- | beta1-fixed |
People
(Reporter: jorendorff, Assigned: jorendorff)
References
Details
Attachments
(1 file)
26.26 KB,
patch
|
Details | Diff | Splinter Review |
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•15 years ago
|
||
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.
Assignee | ||
Comment 2•15 years ago
|
||
Assignee | ||
Comment 3•15 years ago
|
||
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•15 years ago
|
||
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•15 years ago
|
||
Fixed on trunk and 1.9.2 per bug 513783.
Assignee: nobody → jorendorff
Status: NEW → RESOLVED
Closed: 15 years ago
status1.9.2:
--- → beta1-fixed
Resolution: --- → FIXED
Updated•15 years ago
|
Product: Other Applications → Core
You need to log in
before you can comment on or make changes to this bug.
Description
•