599791 - CClosure::ClosureStub silently drops exceptions

Status: RESOLVED FIXED

(Core :: js-ctypes, defect)

Description

[Andreas Gal :gal]

Looks like ClosureStub silently ignores exceptions. Bad.

Comment 1

[[e-mail]]

Hmm. What else can we do here? It'll take a reported error. Is there a way to cause a reported error to be thrown as an exception other than returning JS_FALSE back to JSAPI?

Comment 2

[Brendan Eich [:brendan]]

JS API users must propagate failure, or else report the pending exception if there is one. See these APIs:

JS_IsExceptionPending(JSContext *cx);
JS_GetPendingException(JSContext *cx, jsval *vp);
JS_SetPendingException(JSContext *cx, jsval v);
JS_ClearPendingException(JSContext *cx);
JS_ReportPendingException(JSContext *cx);

Note that Report clears Clears.

/be

Comment 3

[[e-mail]]

Ah, I see. The relationship between an error and an exception was unclear to me.

It sounds like the only thing we need to do here is JS_ReportPendingException if JS_CallFunctionValue fails. And we don't need to worry about reporting an exception from other JSAPI methods that fail, because they've already reported.

It occurs to me that we should probably abort if a JSAPI method fails or a script exception is raised. We really have no way of propagating failure to the caller of the closure, which means program execution is unpredictable. (We do zero out the return value, but that doesn't mean much for safety.)

There are a few conditions that can cause failure in ClosureStub:
1) OOM. Aborting is reasonable here.
2) ConvertToJS failure, e.g. if one of the native args cannot be converted to a JS datatype. The only meaningful way this can fail is OOM.
3) The script function itself throws an exception.
4) ImplicitConvert failure, e.g. if the returned primitive cannot implicitly be converted to a native value of the specified type. This would occur if the JS function returned, say, a string primitive instead of a ctypes.char.ptr.

Reporting an error seems nicer, since it gives people more information to fix the bug, but losing safety is not good. :(

Comment 4

[[e-mail]]

Spoke with jorendorff and decided to report & JS_Abort.

Comment 5

[[e-mail]]

Attachment: report & abort

Simple enough.

Comment 6

[[e-mail]]

Attachment: bonus fix

JS_Abort() is unimplemented, resulting in a link error if one tries to use it.

Comment 7

[Andreas Gal :gal]

Comment on attachment 478907 [details] [diff] [review]
report & abort

Not a big fan of the random abort() call. Is that guaranteed to fail on all obscure platforms? Looks good otherwise.

Comment 8

[[e-mail]]

It's what we use all through jstracer.cpp, so I imagine it's fairly universal.

Comment 9

[Brendan Eich [:brendan]]

(In reply to comment #3)
> Ah, I see. The relationship between an error and an exception was unclear to
> me.
> 
> It sounds like the only thing we need to do here is JS_ReportPendingException
> if JS_CallFunctionValue fails. And we don't need to worry about reporting an
> exception from other JSAPI methods that fail, because they've already reported.

What ensures they've already reported?

Without JSOPTION_DONT_REPORT_UNCAUGHT, you get an automatic report only when the failing API is returning with no JS code active on cx. Why would that differ for the JS_CallFunctionValue API call in this code from other JS API calls in the same code?

> It occurs to me that we should probably abort if a JSAPI method fails or a
> script exception is raised. We really have no way of propagating failure to the
> caller of the closure, which means program execution is unpredictable. (We do
> zero out the return value, but that doesn't mean much for safety.)

This is our new binary extension model? It's going to be a nightmare even if you ignore OOM. JS can throw. The C code has to have a failure model other than whole-process abort.

> 3) The script function itself throws an exception.
> 4) ImplicitConvert failure, e.g. if the returned primitive cannot implicitly be
> converted to a native value of the specified type. This would occur if the JS
> function returned, say, a string primitive instead of a ctypes.char.ptr.
> 
> Reporting an error seems nicer, since it gives people more information to fix
> the bug, but losing safety is not good. :(

Why can't safety be assured by setting all out params and return values to zero-ish values appropriate for the types, and leaving something like errno set to indicate the JS error?

/be

Comment 10

[Brendan Eich [:brendan]]

(In reply to comment #8)
> It's what we use all through jstracer.cpp, so I imagine it's fairly universal.

Argh! This is how bad patterns propagate. That was a short term hack and FIXME comments in the code are supposed to point to the open bug on eliminating those aborts.

OOM happens, it can happen not just with arbitrarily large allocation requests. As Chris Jones pointed out, if you aren't careful about memory pressure monitoring you can walk into it for small fixed-size allocations, even if a GC would free a lot of space.

Do not imitate those aborts as "good precedent." They should go away with the unified tracer/method-JIT stack coming after Fx4.

/be

Comment 11

[Andreas Gal :gal]

I like the errno idea.

Comment 12

[Luke Wagner [:luke]]

errno is also a classically bad pattern.  Is there no more local piece of context that can convey the error from callee to caller?

Comment 13

[Brendan Eich [:brendan]]

Modern errno is of course thread-local storage. It's hard to avoid falling into this design gravity-well with ctypes -- the C calls JS without a cx issue.

Same as XPConnect, but it has well-known singletons to pollute with pigeon-holes (see Components.lastResult and Components.returnCode).

So Luke, I agree errno is nasty but there isn't a substantially-different alternative unless we mandate or automate a cx-like parameter for all functions called via jsctypes.

/be

Comment 14

[[e-mail]]

(In reply to comment #9)
> What ensures they've already reported?

Wait, I thought it was convention for JSAPI functions to report and return JS_FALSE on failure? At least, for the JSAPI functions in question.

> This is our new binary extension model? It's going to be a nightmare even if
> you ignore OOM. JS can throw. The C code has to have a failure model other than
> whole-process abort.

Speaking of OOM alone, I agree, it kinda sucks -- we could be dealing with arbitrarily large user-defined data.

> Why can't safety be assured by setting all out params and return values to
> zero-ish values appropriate for the types, and leaving something like errno set
> to indicate the JS error?

How would we know it's an outparam that can safely be zeroed?

  char* foo = new char[20];
  memcpy(foo, "hi", 3);
  int rv = call_my_closure(&foo);
  if (rv == 0)
    char c = foo[0];

In that example, nulling 'foo' and the return value inside call_my_closure will a) leak the originally assigned 'foo' b) erroneously report success to the caller c) crash on the dereference.

I suppose you're arguing that unexpected behavior is better than crashing. But how do we know that's true? By assuming that the caller is sensible about checking not just return value, but also outparam values?

If we're serious about handling exceptions from script, then we need an API for the ctypes consumer to say "on exception, this is the value you should return". (For instance, -1.) But that's not a very complete way of doing it. (An exception handler function in script might be better, especially if we require the ctypes consumer provide one!)

Disregarding OOM, the only way we can fail here is if the ctypes consumer doesn't try/catch inside their closure, or they return an unconvertable value. These are both fairly easy things to do, as long as they know to do them. I agree, though, it will be yet another trap one can easily fall into. :(

Comment 15

[Brendan Eich [:brendan]]

(In reply to comment #14)
> (In reply to comment #9)
> > What ensures they've already reported?
> 
> Wait, I thought it was convention for JSAPI functions to report and return
> JS_FALSE on failure? At least, for the JSAPI functions in question.

No. See comment 9 second paragraph from me.

> > This is our new binary extension model? It's going to be a nightmare even if
> > you ignore OOM. JS can throw. The C code has to have a failure model other than
> > whole-process abort.
> 
> Speaking of OOM alone, I agree, it kinda sucks -- we could be dealing with
> arbitrarily large user-defined data.

This isn't a credible replacement for XPConnect, or a good FFI for JS.

> > Why can't safety be assured by setting all out params and return values to
> > zero-ish values appropriate for the types, and leaving something like errno set
> > to indicate the JS error?
> 
> How would we know it's an outparam that can safely be zeroed?

I was assuming you had "direction" (in, inout, out) information along with type information.

>   char* foo = new char[20];
>   memcpy(foo, "hi", 3);
>   int rv = call_my_closure(&foo);
>   if (rv == 0)
>     char c = foo[0];
> 
> In that example, nulling 'foo' and the return value inside call_my_closure will
> a) leak the originally assigned 'foo' b) erroneously report success to the
> caller c) crash on the dereference.

That's an inout param, have to leave it alone on error.

Do you have direction as well as type for each parameter?

/be

Comment 16

[[e-mail]]

(In reply to comment #15)
> Do you have direction as well as type for each parameter?

Aha! Now I'm with you. We do not have that information. We would have to specialcase closure declarations to make it work, which is fine. The question is whether it's worth doing. If we're serious about being robust, and supporting generic function signatures, we have to.

The information we would need is
1) The return value to use on failure
2) The value to set each argument to, for out/inout params, or possibly just assume NULL -- dangerous if the function doesn't have a return value that can indicate failure.

The provided values would have to be converted at closure declaration time, so we can throw if they're unconvertable or we run out of memory trying to convert them.

Thoughts?

Comment 17

[[e-mail]]

I think I have a happy medium here that a) gets us almost all the way to safety, b) does not require a major API change for closures.

We can add an optional arg to the closure constructor:

  let closure = ctypes.FunctionType(...).ptr(my_js_fn, -1);

where -1 is the return value to pass on failure. (This would be eagerly converted on the spot, and throw if it can't be.) We could make this a required arg, but I don't feel strongly about that.

Then, we strongly recommend in the docs and examples that the first thing people do in their js function is set outparams to sensible values, i.e. NULL where appropriate -- nothing fancy that would require potentially big allocations. Alternatively, they catch all exceptions that could be thrown inside the function.

If the js function throws proper, we hope they've supplied a default return value and return it, or return zeroed data if not (or abort?).

This should cover the common case where C callback functions have a return value that can indicate success or failure, such as a result code or a pointer.

If the above conditions are satisfied, the only way we can fail is if we OOM performing a few small allocations for the args. Or if a default return value was not supplied, and the script throws, then we're back to either aborting or being unsafe.

Is anyone happy with that?

Comment 18

[[e-mail]]

I'd note that aborting on small allocation failure is exactly what infallible malloc does, and will continue to do. Whether this will fly in jseng in this particular case is a different question, however. (We could get all fancy and preallocate a little bit of memory that we can free if we OOM in the closure, enough to either complete the allocation or perform a last-ditch GC. Nasty!)

Comment 19

[Brendan Eich [:brendan]]

(In reply to comment #18)
> I'd note that aborting on small allocation failure is exactly what infallible
> malloc does, and will continue to do.

This is tenable only if you monitor OS memory pressure and try a GC before failing.

> Whether this will fly in jseng in this
> particular case is a different question, however.

The malloc heap is shared among all the components, not just the JS engine, and the VM address space too (from which the GC virtual-allocs and commits).

E10s brings the needed monitoring and GC/cache-flushing, last I heard, but it is not there yet and it will introduce bugs and bogus failures to rely in what is not there yet.

/be

Comment 20

[Brendan Eich [:brendan]]

Someone should contrast comment 17 with what XPConnect does.

/be

Comment 21

[Benjamin Smedberg]

As much as this is a bug, I don't think this blocks.

Comment 22

[Brendan Eich [:brendan]]

(In reply to comment #20)
> Someone should contrast comment 17 with what XPConnect does.

Peterv, Johnny, Blake?

/be

Comment 23

[[e-mail]]

Reassigning to nobody. If anyone wants to work on this, feel free!

Comment 24

[Bobby Holley (:bholley)]

Taking.

Comment 25

[Bobby Holley (:bholley)]

I've spun up a patch stack to implement dwitte's strategy in comment 17. The patches here apply on top of the ones in the dependent bug.

Flagging dwitte for review. This will need sr as well.

Comment 26

[Bobby Holley (:bholley)]

Attachment: part 1 - Remove unnecessary conditional logic. v1

Comment 27

[Bobby Holley (:bholley)]

Attachment: part 2 - restructure argument handling in PointerType::ConstructData. v1

Comment 28

[Bobby Holley (:bholley)]

Attachment: part 3 - Pass errVal down into the closure constructor. v1

Comment 29

[Bobby Holley (:bholley)]

Attachment: part 4 - Do ClosureInfo cleanup with a destructor. v1

Comment 30

[Bobby Holley (:bholley)]

Attachment: part 5 - Prepare the sentinel value and store it in ClosureInfo. v1

Comment 31

[Bobby Holley (:bholley)]

Attachment: part 6 - Return the sentinel when we fail in ClosureStub. v1

Comment 32

[Bobby Holley (:bholley)]

Attachment: part 7 - Tests. v1

Comment 33

[Bobby Holley (:bholley)]

Attachment: part 7 - Tests. v2

Rebasing the tests off of the updated patch from bug 682504.

Comment 34

[Bobby Holley (:bholley)]

Over to jorendorff, per our discussion yesterday evening.

Jorendorff - the patches are nicely divided, so they shouldn't be too hard to review. Let me know if there's anything I can do to make it easier.

Comment 35

[Jason Orendorff [:jorendorff]]

Comment on attachment 556223 [details] [diff] [review]
part 2 - restructure argument handling in PointerType::ConstructData. v1

Sorry for the slow reviews. So far so good; I will get the rest of these to you on Tuesday.

Comment 36

[Bobby Holley (:bholley)]

*gentle whine* ;-)

Comment 37

[Jason Orendorff [:jorendorff]]

Comment on attachment 556226 [details] [diff] [review]
part 5 - Prepare the sentinel value and store it in ClosureInfo. v1

>+  // the caller to know about it _know_, rather than some uncertain time in the

know about it _now_

+    // Make sure the callback returns something.
+    if (CType::GetTypeCode(cx, fninfo->mReturnType) == TYPE_void_t) {
+      JS_ReportError(cx, "A void callback can't pass an error sentinel");
+      return NULL;
+    }

This returns before cinfo->errResult is initialized, right? Then the
destructor is called. It seems bad.

Please add a test for this error path.

>+    cinfo->errResult = cx->malloc_(rvSize);
>+    if (!cinfo->errResult) {
>+      JS_ReportOutOfMemory(cx);
>+      return NULL;
>+    }

cx->malloc_ will already have called ReportOutOfMemory on failure, so
you can remove that line.

>   ~ClosureInfo() {
>     if (closure)
>       ffi_closure_free(closure);
>+    if (errResult)
>+      js_free(errResult);
>   };

I'm told that calling js_free directly isn't allowed. (We have a comment
that says it is allowed, but we're getting rid of it, and make check
will actually detect that you're calling js_free and gripe at
you.)

Instead, store a JSRuntime pointer in the ClosureInfo and have the
destructor call rt->free_(errResult). (It would be even better to use
cx, but I imagine there's some reason that can't be done.)

It is OK to mix cx->malloc_ with rt->free_.

There are only two hard things in computer science: memory management, and I forget the other one.

Comment 38

[Jason Orendorff [:jorendorff]]

Comment on attachment 556227 [details] [diff] [review]
part 6 - Return the sentinel when we fail in ClosureStub. v1

This code should not leave an exception in cx.

Here's what I would change:

>   JSBool success = JS_CallFunctionValue(cx, thisObj, OBJECT_TO_JSVAL(jsfnObj),
>                                         cif->nargs, argv.begin(), &rval);
> 
>-  // If the callback doesn't return anything, we're done.
>-  if (cif->rtype == &ffi_type_void)
>-    return;
>-
>   // Convert the result. Note that we pass 'isArgument = false', such that
>   // ImplicitConvert will *not* autoconvert a JS string into a pointer-to-char
>   // type, which would require an allocation that we can't track. The JS
>   // function must perform this conversion itself and return a PointerType
>   // CData; thusly, the burden of freeing the data is left to the user.
>-  if (success)
>+  if (success && cif->rtype != &ffi_type_void)
>     success = ImplicitConvert(cx, rval, fninfo->mReturnType, result, false, NULL);
> 
>   // Something might have failed. The callee may have thrown, or it may not have
>   // returned a value that ImplicitConvert() was happy with. Depending on how
>   // prudent the consumer has been, we may or may not have a recovery plan.
>   if (!success) {
>+    // In any case, a JS exception cannot be passed to C code, so
>+    // report the exception if any and clear it from the cx.
>+    if (JS_IsExceptionPending(cx))
>+      JS_ReportPendingException(cx);
> 
>     // Good case: we have a sentinel that we can return. Copy it in place of the
>     // actual return value, and then proceed.
>     if (cinfo->errResult) {
> 
>       // The buffer we're returning might be larger than the size of the return
>       // type, due to libffi alignment issues (see above). But it should never
>       // be smaller.
>       size_t copySize = CType::GetSize(cx, fninfo->mReturnType);
>       JS_ASSERT(copySize <= rvSize);
>       memcpy(result, cinfo->errResult, copySize);
>-    }
>-
>-    // Bad case: not much we can do here. The rv is already zeroed out, so we
>-    // just report an error and hope for the best.
>-    else {
>+    } else {
>+      // Bad case: not much we can do here. The rv is already zeroed out, and
>+      // the error has already been reported, so just return and hope for the
>+      // best.
>-      JS_ReportError(cx, "JavaScript callback failed, and an error sentinel "
>-                         "was not specified.");
>       return;
>     }
>   }

r=me with those changes.

The change to the comment formatting is just to be consistent with the rest of the file. CTypes.cpp is a little quirky.

Comment 39

[Jason Orendorff [:jorendorff]]

Comment on attachment 556644 [details] [diff] [review]
part 7 - Tests. v2

As I mentioned in reviewing part 5, it would be nice to have one or two more tests of those error cases.

Actually, if it's just a couple lines of JS code, please add a test for this feature when the return type is bigger than a word. (Do we support that? If not, a test is still in order, just to check that we don't crash or assert.) It isn't super crucial; if it would take more than 20 minutes, don't worry about it.

Looks good. r=me.

I apologize for the slow review here.

Comment 40

[Bobby Holley (:bholley)]

Attachment: part 4 - Do ClosureInfo cleanup with a destructor. v2

Comment 41

[Bobby Holley (:bholley)]

Attachment: part 5 - Prepare the sentinel value and store it in ClosureInfo. v2

Comment 42

[Bobby Holley (:bholley)]

Attachment: part 6 - Return the sentinel when we fail in ClosureStub. v2

Added updated patches for 4, 5, and 6. Reflagging jorendorff for review, despite the r+.

Comment 43

[Jason Orendorff [:jorendorff]]

Comment on attachment 565000 [details] [diff] [review]
part 5 - Prepare the sentinel value and store it in ClosureInfo. v2

>+  }
>+  else
>+    cinfo->errResult = NULL;

Style nit:

+  } else {
+    cinfo->errResult = NULL;
+  }

Comment 44

[Jason Orendorff [:jorendorff]]

Comment on attachment 565001 [details] [diff] [review]
part 6 - Return the sentinel when we fail in ClosureStub. v2

>+    }
>+
>+    else {

Same style nit again. Like this:

+    } else {

r=me with that.

Comment 45

[Bobby Holley (:bholley)]

Pushed to mozilla-inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/4e1565df0b2c (and ancestors)

This had a green try run, modulo known oranges: https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=e71f6e3a3771

Comment 46

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/mozilla-central/rev/a0f0f75ed0ef
https://hg.mozilla.org/mozilla-central/rev/28321579239e
https://hg.mozilla.org/mozilla-central/rev/cb404dac5599
https://hg.mozilla.org/mozilla-central/rev/235026a2505c
https://hg.mozilla.org/mozilla-central/rev/42e41d74e180
https://hg.mozilla.org/mozilla-central/rev/de75046c6cb1
https://hg.mozilla.org/mozilla-central/rev/4e1565df0b2c

Comment 47

[Bobby Holley (:bholley)]

I just added some basic documentation for this: https://developer.mozilla.org/en/js-ctypes/js-ctypes_reference/Callbacks