541003 - CPOW: Support throwing arbitrary exceptions from content to chrome

Status: RESOLVED FIXED

(Core :: IPC, enhancement)

Description

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

Attachment: CPOW tests, including exception tests.

Exceptions thrown in the child process during CPOW operations should be caught and re-thrown in the parent process.  This should work for arbitrary exception values, including CPOW-wrapped exception objects/functions/ponies/&c.

See the attached tests for an example.

Comment 1

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

Comment on attachment 422673 [details] [diff] [review]
CPOW tests, including exception tests.

Note that this patch contains the complete CPOW testsuite, rather than just adding the exception-testing parts.

Comment 2

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

Attachment: Patch to implement CPOW-wrapped exceptions.

This patch, on the other hand, assumes that the main CPOW patch is already applied.  To clear up any confusion about patch versions, refer to this snapshot of my patch queue repository:

  http://github.com/benjamn/e10s/tree/bebeb9f7093271ebc08f8ecf2276ceadbe937cd5

You will also need to have new-request-model.diff (bug 540126) applied for this patch to apply cleanly, a fact also conveyed by the series file found in the above repository snapshot.

The crux of this patch is that I changed all the |JSBool ok| out-parameters to |OperationStatus status| out-parameters.  An OperationStatus is a union type representing either a JSBool or a JSVariant.  If the JSBool field is set, its value indicates the success or failure of the CPOW operation (just as the JSBool did before).  If the JSVariant field is set, the operation is implied to have failed, and the variant data represents the value of the exception that was thrown.

The stack class AutoCheckOperation helps ensure that no pending exception goes unnoticed.  It also has the nice side-effect of ensuring that statuses are never left uninitialized.

Comment 3

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

Attachment: Repairing bit-rot.

As always, refer to http://github.com/benjamn/e10s for the authoritative versions of these files.

Comment 4

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

Attachment: Basic support for child->parent exception throwing.

This patch replaces the JSBool* ok out-parameters of PObjectWrapper messages with an IPDL union called OperationStatus, which can be either a JSBool or a JSVariant.  The JSVariant case represents an exception, and implies that the ObjectWrapperParent::CPOW_* hook should return JS_FALSE.

An AutoCheckOperation class is added to ObjectWrapperParent and ObjectWrapperChild to ensure that JS_{Get,Set}PendingException are called appropriately at every return.

This patch is deficient in one regard (to my knowledge): the ObjectWrapperParent::CPOW_* hooks can still return JS_FALSE in many cases without setting an exception (but failing an assertion in ObjectWrapperParent::CheckOperation).  I will attach another patch momentarily that clears up that problem.

Comment 5

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

Attachment: Prevent returning JS_FALSE from ObjectWrapperParent::CPOW_* hooks without setting an exception.

(In reply to comment #4)
> This patch is deficient in one regard (to my knowledge): the
> ObjectWrapperParent::CPOW_* hooks can still return JS_FALSE in many cases
> without setting an exception (but failing an assertion in
> ObjectWrapperParent::CheckOperation).  I will attach another patch momentarily
> that clears up that problem.

As promised.

I think it's worth highlighting the with_exception function:

+template <typename RType>
+static RType
+with_exception(JSContext* cx,
+               RType rval,
+               const char* error = NULL)
+{
+    if (!JS_IsExceptionPending(cx)) {
+        jsval x = JSVAL_VOID;
+        if (!error)
+            error = "Unspecified CPOW error";
+        JSString* str = JS_NewStringCopyZ(cx, error);
+        if (str)
+            x = STRING_TO_JSVAL(str);
+        JS_SetPendingException(cx, x);
+    }
+    return rval;
+}

With this function, any failing "return XXX;" statement can be replaced with a statement of the form

  return with_exception(cx, XXX, "Something happened and it wasn't good.");

If no exception is pending yet, with_exception turns the error string into a jsval and calls JS_SetPendingException.

It's still possible for RequestRunToCompletion to fail, for instance, without setting an exception.  If you think it's worthwhile, I could split the multi-line conjunctions that conclude every ObjectWrapperParent::CPOW_* hook into multiple

  if (!condition)
    return with_exception(cx, JS_FALSE, ...);

tests, if you really think that's important.

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 430395 [details] [diff] [review]
Basic support for child->parent exception throwing.

I like it! Only a couple of comments below:

>diff --git a/js/src/ipc/ObjectWrapperChild.cpp b/js/src/ipc/ObjectWrapperChild.cpp
>+    class AutoCheckOperation {

It seems like this class and the one in the parent are extremely similar. Is there no way to share them? Or at least share the common bits?
>+        AutoCheckOperation(ObjectWrapperChild* owc,
>+                           OperationStatus* status
>+                           JS_GUARD_OBJECT_NOTIFIER_PARAM)
>+            : mObjectWrapper(owc)
>+            , mStatus(status)
>+        {
>+            JS_GUARD_OBJECT_NOTIFIER_INIT;
>+            if (mStatus->type() == OperationStatus::T__None)
>+                *mStatus = JS_FALSE;

Can you comment on why this is needed. It isn't entirely apparent to me.

>diff --git a/js/src/ipc/ObjectWrapperParent.cpp b/js/src/ipc/ObjectWrapperParent.cpp
>+void
>+ObjectWrapperParent::CheckOperation(JSContext* cx,
>+                                    OperationStatus* status)
>+{
>+    case OperationStatus::TJSBool:
>+        NS_ASSERTION(status->get_JSBool(), "Failed without setting an exception.");
>+        break;

This can actually happen -- out of memory is the most common, but a long-running script or other uncatchable error turns into a false return value with no exception set. It seems like we should deal with it without asserting.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 430397 [details] [diff] [review]
Prevent returning JS_FALSE from ObjectWrapperParent::CPOW_* hooks without setting an exception.

Ben and I chatted on IRC about this. I'm fine with this patch (IMO content processes shouldn't be able to stop the chrome process from running because they ran out of memory or similar ways of returning false without an exception), but I worried that the CPOW code isn't as careful with exceptions as it could be. In general, the JS engine maintains a pretty strong contract about what functions set what exceptions/when the caller is responsible for reporting its own exceptions. Also, JS_ReportError is almost always better than JS_NewString/JS_SetPendingException because it creates a real Error object with stack trace and other goodies.

I'll stamp the next patch.

Comment 8

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

Attachment: Patch addressing mrbkap's suggestions.

(In reply to comment #6)
/ipc/ObjectWrapperChild.cpp
> >+    class AutoCheckOperation {
> 
> It seems like this class and the one in the parent are extremely similar. Is
> there no way to share them? Or at least share the common bits?

I did my best.  Net lines of code increased, but I'm repeating myself a little less.  See what you think.

> >+            JS_GUARD_OBJECT_NOTIFIER_INIT;
> >+            if (mStatus->type() == OperationStatus::T__None)
> >+                *mStatus = JS_FALSE;
> 
> Can you comment on why this is needed. It isn't entirely apparent to me.

Rationale supplied.

> This can actually happen -- out of memory is the most common, but a
> long-running script or other uncatchable error turns into a false return value
> with no exception set. It seems like we should deal with it without asserting.

NS_WARNING ok?  Or should I just ignore this?

Comment 9

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

Here's the interdiff:
http://github.com/benjamn/e10s/blob/7cb7357b13ba12d98590a89224718ca9b17adcc9/refactor-autocheckoperation.diff

Comment 10

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

(In reply to comment #9)
> Here's the interdiff:
> http://github.com/benjamn/e10s/blob/7cb7357b13ba12d98590a89224718ca9b17adcc9/refactor-autocheckoperation.diff

Note that this interdiff shows only the changes I made to address your first suggestion (hence the name, refactor-autocheckoperation.diff).  I had already addressed the second two suggestions in cpow-exceptions.diff when I created the incremental patch shown here.

Comment 11

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

Attachment: Not making up quite so many CPOW exceptions, and using JS_ReportError instead of JS_SetPendingException.

ObjectWrapperParent::CPOW_* operations now report new exceptions in only the following cases:

1. A parent-process exception needs to be created from an OperationStatus out-parameter because the child process returned an exception.

2. Parent-process JS tried to pass an object reference to the child process that the child won't understand.

3. The Unwrap operations failed, either because a CPOW_* operation (other than CPOW_Finalize) was called after ActorDestroy, or because an object with ObjectWrapperParent::sCPOW_JSClass has an invalid private ObjectWrapperParent*.

(In reply to comment #7)
> I'll stamp the next patch.

Stamp away!

Comment 12

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

(In reply to comment #11)
> ObjectWrapperParent::CPOW_* operations now report new exceptions in only the
> following cases:

In particular, I no longer create new exceptions just because a conversion operation failed, which usually happens because of OOM.  If the JS engine wants to set an exception in those cases, it will.

Comment 13

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

Attachment: Complete CPOW tests suite, collected into one patch.

This patch is at the end of the CPOW dependency tree, so instead of filing a new bug I'm just attaching the test patch here.

Comment 14

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

Pushed to projects/electrolysis:
http://hg.mozilla.org/projects/electrolysis/rev/b4b071cca5f9

Will mark resolve when tests cycle.

Comment 15

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

Pushed tests to projects/electrolysis:
http://hg.mozilla.org/projects/electrolysis/rev/e75320dab85b

Will mark resolve when tests cycle.