627943 - Compartment mismatch on exception in nsXPCWrappedJSClass::CallMethod

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Steve Fink [:sfink] [:s:]]

Probably fallout from bug 614131:

When running the Firebug test suite, I've been getting compartment mismatches between a context and its exception when Firebug is getting a callback for the exception.

The problem occurs in the cleanup of nsXPCWrappedJSClass::CallMethod(). What happens is:

1. CallMethod() starts for a context with a pending exception
2. The AutoScriptEvaluate RAII constructor grabs and remembers cx
3. JSAutoEnterCompartment::enter() wraps both the context and its exception in a new compartment. (Ok, sets the context's compartment, wraps the exception.)
4. AutoScriptEvaluate::StartEvaluating clears cx->exception and saves it to the side
5. ...stuff happens...
6. The JSAutoEnterCompartment RAII destructor fires, resetting cx->compartment but NOT resetting the exception's compartment, because cx no longer has an exception
7. The AutoScriptEvaluate RAII destructor fires, restoring the saved (wrapped) exception to cx

The result is that cx->compartment is set to the original compartment, but cx->exception is still wrapped in the new compartment.

Comment 1

[Steve Fink [:sfink] [:s:]]

Attachment: Reorder RAII objects to fix compartment mismatch

Reorder things to work. Note that this isn't the only ordering dependency; I first tried to just move JSAutoEnterCompartment to just above AutoScriptEvaluate, but it seems that AutoValueVector must come *before* JSAutoEnterCompartment. I didn't spend the time to figure out why, but the sequence is certainly brittle.

Comment 2

[Steve Fink [:sfink] [:s:]]

By the way, I think a better fix would be to modify AutoScriptEvaluate to wrap the exception after restoring it, so that the ordering isn't quite so brittle. But that would require exposing cx->wrapPendingException(), which is not available in xpcwrappedjsclass.cpp.

Comment 3

[Andreas Gal :gal]

This doesn't look right. What is setting the exception and then not wrapping it?

Comment 4

[Steve Fink [:sfink] [:s:]]

(In reply to comment #3)
> This doesn't look right. What is setting the exception and then not wrapping
> it?

Actually, it's the other way around. The AutoScriptEvaluate destructor is (re-)setting the exception and failing to *unwrap* it. So the context ends up on the starting compartment, but the exception is on the inner compartment (because it wasn't present when ~JSAutoEnterCompartment() ran to restore the original compartment to the context and any pending exception.)

Let me try again:

1. start: cx->compartment=A, cx->exception->compartment=A
2. JSAutoEnterCompartment::enter():
     cx->compartment=B;
     cx->exception->compartment=B;
3. AutoScriptEvaluate::StartEvaluating():
     tmp=cx->exception;
     cx->exception=NULL;
4. JSAutoEnterCompartment::~JSAutoEnterCompartment():
     cx->compartment=A
     (cx->exception is still NULL)
5. AutoScriptEvaluate::~AutoScriptEvaluate:
     cx->exception=tmp;
     (cx->exception->compartment is left as B)

I swapped the order of the JSAutoEnterCompartment and AutoScriptEvaluate declarations, which resulted in swapping the order of their destructors, which resulted in steps 4 and 5 being swapped, so now they look like:

4. AutoScriptEvaluate::~AutoScriptEvaluate:
     cx->exception=tmp;
     (cx->compartment and cx->exception->compartment are B)
5. JSAutoEnterCompartment::~JSAutoEnterCompartment():
     cx->compartment=A
     cx->exception->compartment=B

Another alternative would be swapping the order of steps 2 and 3, so that JSAutoEnterCompartment never sees the exception. And the best alternative would be to make JSAutoEnterCompartment's destructor call wrapPendingException if it sets the exception, but that requires exposing that operation somehow, and I'm only proposing minimal changes right now.

Comment 5

[Andreas Gal :gal]

I see. What API is AutoScriptEvaluate use to save and restore the exception? Thats where the bug is imo. We have to wrap in that api too. Your fix is valid, but that would be a more general fix. Do you agree?

Comment 6

[Steve Fink [:sfink] [:s:]]

JS_RestoreExceptionState().

You're right, that's a much better idea. I hadn't realized the exception state save/restore was within the JSAPI. I had assumed everything was done through JS_SetPendingException/JS_ClearPendingException.

I don't have time now to make the (trivial) patch, but I'll post it soon.

Thanks.

Comment 7

[Andreas Gal :gal]

Yeah, cool. Thanks Steve.

Comment 8

[Steve Fink [:sfink] [:s:]]

Attachment: Wrap exception in JS_RestoreExceptionState

You know that part where I said "(trivial)"? Please pretend I didn't.

What do we do if the wrapping fails? Or is there some reason why it will never fail because it's unwrapping rather than wrapping? (Unlikely for a public API.)

Should JS_RestoreExceptionState() return a bool? There aren't many callers, but I don't know how to handle failure for most of them.

Comment 9

[Steve Fink [:sfink] [:s:]]

Attachment: Wrap exception in JS_RestoreExceptionState

I suppose I ought to make it at least compile...

Comment 10

[Andreas Gal :gal]

Wrapping never fails, that's why cx->wrapException has a void return value. wrapException is supposed to be only called if cx->isExceptionPending(), and the worst thing that can happen is that wrapException changes the currently pending exception to OutOfMemory because it couldn't wrap it.

Comment 11

[Steve Fink [:sfink] [:s:]]

I think I might possibly sort of understand what you're saying. By "wrapping never fails", you mean "wrapping a pending exception never fails because even if the wrap() call fails, it will have set a new exception anyway"?

If so, the comment for wrapException() greatly confused me. Instead of

/*
 * Since this function is only called in the context of a pending exception,
 * the caller must subsequently take an error path. If wrapping fails, we leave
 * the exception cleared, which, in the context of an error path, will be
 * interpreted as an uncatchable exception.
 */

it would make far more sense to me to say something like

/*
 * Since this function is only called in the context of a pending
 * exception, the caller must subsequently take an error path. If
 * wrapping fails, it will set a new (uncatchable) exception that will
 * be reported in place of the original.
 */

The first comment seems to be saying that the context will be left without a pending exception at all ("we leave the exception cleared"), and the callers will be ok with that because they're in the process of reporting an error, and if the error disappears, they'll treat that as a mystery uncatchable exception ("will be interpreted as an uncatchable exception"). Which of course isn't the case at all.

Maybe it's just because people familiar with the JS code know implicitly that any failure will always result in a pending exception being set on the context?

Comment 12

[Steve Fink [:sfink] [:s:]]

Attachment: Wrap exception in JS_RestoreExceptionState

New patch assuming my new understanding is correct.

Note that I have to wrap the exception manually rather than using cx->wrapPendingException() because the only API to set the exception (setPendingException()) checks for a compartment mismatch. Would it be better to make

  setPendingException(bool wrapIfNeeded = false);

that could optionally wrap the exception for you instead of doing the check? (Or just make it do that all the time, but that would be a bit slower in the common case and remove a useful sanity check.)

Comment 13

[Brendan Eich [:brendan]]

Comment on attachment 506196 [details] [diff] [review]
Wrap exception in JS_RestoreExceptionState

Existing comment wrapped better. wm=79 and Qj in vim are your friends:

..12345678901234567890123456789012345678901234567890123456789012345678901234567890

>+ * Since this function is only called in the context of a pending

s/only called/called only/ (pre-existing problem)

>+ * exception, the caller must subsequently take an error path. If
>+ * wrapping fails, it will set a new (uncatchable) exception that will
>+ * be reported in place of the original.

More like, the failure is OOM, which is reported right away. But failure must propagate (false/null return values).

Thanks for taking on this comment. It was confusing.

/be

Comment 14

[Andreas Gal :gal]

Comment on attachment 506196 [details] [diff] [review]
Wrap exception in JS_RestoreExceptionState

># HG changeset patch
># Parent 33f07113de5fcf6bbe59ce554402b22c91cfebcb
># User Steve Fink <[e-mail]>
>
>diff --git a/js/src/jsapi.cpp b/js/src/jsapi.cpp
>--- a/js/src/jsapi.cpp
>+++ b/js/src/jsapi.cpp
>@@ -5915,20 +5915,24 @@ JS_SaveExceptionState(JSContext *cx)
>     return state;
> }
> 
> JS_PUBLIC_API(void)
> JS_RestoreExceptionState(JSContext *cx, JSExceptionState *state)
> {
>     CHECK_REQUEST(cx);
>     if (state) {
>-        if (state->throwing)
>-            JS_SetPendingException(cx, state->exception);
>-        else
>+        if (state->throwing) {
>+            Value v = Valueify(state->exception);
>+            /* On failure, wrap() will set a new uncatchable exception */
>+            if (cx->compartment->wrap(cx, &v))
>+                JS_SetPendingException(cx, Jsvalify(v));
>+        } else {
>             JS_ClearPendingException(cx);
>+        }

I think you should do this as follows:

if (cx->isExceptionPending())
    cx->clearPendingException();
if (state->throwing) {
    cx->setPendingException(state->exception);
    cx->wrapException();
}

>         JS_DropExceptionState(cx, state);
>     }
> }
> 
> JS_PUBLIC_API(void)
> JS_DropExceptionState(JSContext *cx, JSExceptionState *state)
> {
>     CHECK_REQUEST(cx);
>diff --git a/js/src/jscntxt.cpp b/js/src/jscntxt.cpp
>--- a/js/src/jscntxt.cpp
>+++ b/js/src/jscntxt.cpp
>@@ -2028,20 +2028,20 @@ error:
>     /*
>      * If we try to use the context without a selected compartment,
>      * we will crash.
>      */
>     compartment = NULL;
> }
> 
> /*
>- * Since this function is only called in the context of a pending exception,
>- * the caller must subsequently take an error path. If wrapping fails, we leave
>- * the exception cleared, which, in the context of an error path, will be
>- * interpreted as an uncatchable exception.
>+ * Since this function is only called in the context of a pending
>+ * exception, the caller must subsequently take an error path. If
>+ * wrapping fails, it will set a new (uncatchable) exception that will
>+ * be reported in place of the original.
>  */

Yeah, the comment is much better.

> void
> JSContext::wrapPendingException()
> {
>     Value v = getPendingException();
>     clearPendingException();
>     if (compartment->wrap(this, &v))
>         setPendingException(v);

Comment 15

[Steve Fink [:sfink] [:s:]]

(In reply to comment #14)
> Comment on attachment 506196 [details] [diff] [review]
> Wrap exception in JS_RestoreExceptionState

> I think you should do this as follows:
> 
> if (cx->isExceptionPending())
>     cx->clearPendingException();
> if (state->throwing) {
>     cx->setPendingException(state->exception);
>     cx->wrapException();
> }

I can't, without changing setPendingException(). It does a compartment check, which would fail in this case. (That's why I was proposing giving setPendingException an optional param.)

Comment 16

[Andreas Gal :gal]

I see. That blows. Just use your code then and maybe JS_WrapObject so you don't need the valueify stuff. The layering seems lame here. Sorry for that.

Comment 17

[Andreas Gal :gal]

Comment on attachment 506196 [details] [diff] [review]
Wrap exception in JS_RestoreExceptionState

This patch isn't sufficient. I can an mismatch assert in JS_DropExceptionState while removing the root.

Comment 18

[Andreas Gal :gal]

Sorry for the wild goose chase Steve. You are right, your original patch is the way to go. Saving/restoring the exception is fully contained in the compartment switch so there is no need to wrap. The patch I r+-ed above fixed the assert and makes bug 627943 pass for me.

Comment 19

[Andreas Gal :gal]

I meant bug 626909.

Comment 20

[Jesse Ruderman]

My DOM fuzzer is hitting this compartment mismatch. This bug should block.

Comment 21

[Andreas Gal :gal]

Can we land this?

Comment 22

[Steve Fink [:sfink] [:s:]]

http://hg.mozilla.org/tracemonkey/rev/e13f3b909fa2

Comment 23

[Steve Fink [:sfink] [:s:]]

Backed out due to Windows template compilation problem for a different bug in same push.

http://hg.mozilla.org/tracemonkey/rev/bee0e6e72ca5

Comment 24

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/e13f3b909fa2
http://hg.mozilla.org/mozilla-central/rev/c10e63cba3b6
http://hg.mozilla.org/mozilla-central/rev/dcb713798848 (backout)
http://hg.mozilla.org/mozilla-central/rev/bee0e6e72ca5 (backout)
Note: not marking as fixed because last changeset is a backout.

Comment 25

[Steve Fink [:sfink] [:s:]]

http://hg.mozilla.org/tracemonkey/rev/fc8ec5cca4a2

Comment 26

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/fc8ec5cca4a2
http://hg.mozilla.org/mozilla-central/rev/7af02efabd07