Last Comment Bug 763225 - "Assertion failure: compartment mismatched" with Components.lookupMethod(v, "onclick")
: "Assertion failure: compartment mismatched" with Components.lookupMethod(v, "...
Status: VERIFIED FIXED
[advisory-tracking+]
: assertion, testcase
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: Trunk
: x86_64 Mac OS X
: -- critical (vote)
: mozilla16
Assigned To: Boris Zbarsky [:bz] (Out June 25-July 6)
:
Mentors:
: 720714 763977 (view as bug list)
Depends on:
Blocks: 326633 594645 cpg 659350 720714
  Show dependency treegraph
 
Reported: 2012-06-09 13:04 PDT by Jesse Ruderman
Modified: 2012-12-11 11:59 PST (History)
15 users (show)
bzbarsky: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
verified
verified
verified
14+
verified


Attachments
testcase (419 bytes, text/html)
2012-06-09 13:04 PDT, Jesse Ruderman
no flags Details
stack trace (12.57 KB, text/plain)
2012-06-09 13:04 PDT, Jesse Ruderman
no flags Details
Take a bit more care about our compartments in SetJSEventListenerToJsval. (2.63 KB, patch)
2012-06-13 20:04 PDT, Boris Zbarsky [:bz] (Out June 25-July 6)
bugs: review+
bobbyholley: review+
Details | Diff | Review
Take a bit more care about our compartments in SetJSEventListenerToJsval. (2.50 KB, patch)
2012-06-14 10:23 PDT, Boris Zbarsky [:bz] (Out June 25-July 6)
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
lukasblakk+bugs: approval‑mozilla‑esr10+
Details | Diff | Review

Description Jesse Ruderman 2012-06-09 13:04:08 PDT
Created attachment 631690 [details]
testcase

Assertion failure: compartment mismatched, at js/src/jscntxtinlines.h:237
Comment 1 Jesse Ruderman 2012-06-09 13:04:23 PDT
Created attachment 631691 [details]
stack trace
Comment 2 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-06-11 19:49:32 PDT
So..

We call lookupMethod on a cross-compartment wrapper.  This returns what looks like a Function object created in the compartment of the caller.  That is, it's created in the parent page's compartment.

Then we call this method.  That lands us, through NS_InvokeByIndex_P in nsInlineEventHandlersTearoff::SetOnclick and hence in nsINode::SetOnclick.  At this point cx and v are both in the the parent page compartment.

The nsINode::SetOnclick code looks something like this:

    JSObject *obj = GetWrapper();
    if (!obj) {
      /* Just silently do nothing */
      return NS_OK;
    }
    return elm->SetJSEventListenerToJsval(nsGkAtoms::onclick, cx, obj, v);

because it's not expecting to be called while cx is in compartments other than the node's own compartment.  This probably means it fails for XrayWrapper too, by the way...  But ignoring that, now we have cx and v in one compartment (parent page) and obj in a different one (subframe).  The fact that these are different compartments is the result of cpg.

In any case, now it's just a time-bomb that happens to blow up when we call nsJSUtils::GetStaticScriptContext which calls JS_GetGlobalForObject which asserts that cx and the object are in the same compartment.  Which they're not.

So as far as fixing goes:

1)  LookupMethod should perhaps enter the compartment of the underlying object while creating the function object and return a cross-compartment wrapper in this case.

2)  We should figure out whether SetOn* and GetOn* need to be able to deal with being called in compartments that don't match the element's compartment and if so how they should behave.
Comment 3 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-06-12 10:26:32 PDT
*** Bug 763977 has been marked as a duplicate of this bug. ***
Comment 4 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-06-12 23:36:15 PDT
Looking over the code, I think we should aim to store things that are in the node's compartment in the nsIJSEventListener.  What that means is that the setter needs to:

1)  Enter the node's compartment as needed.
2)  JS_WrapValue the argument after that, again as needed.

Olli, does that seem reasonable?
Comment 5 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-06-13 01:06:19 PDT
I think so.
Comment 6 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-06-13 01:44:35 PDT
...not that I know what JS_WrapValue does.
Comment 7 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-06-13 08:06:34 PDT
It takes as input a jsval and a JSContext which may be in different compartments and outputs a jsval that is in the same compartment as the JSContext (by creating cross-compartment wrappers as needed).
Comment 8 Daniel Veditz [:dveditz] 2012-06-13 17:20:55 PDT
is this a regression from compartment-per-global, or a pre-existing problem?
Comment 9 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-06-13 18:58:59 PDT
The fact that web content can trigger this is a regression from cpg.

chrome could trigger it before as well; that part is probably a regression from bug 659350.
Comment 10 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-06-13 20:04:01 PDT
Created attachment 632999 [details] [diff] [review]
Take a bit more care about our compartments in SetJSEventListenerToJsval.
Comment 11 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-06-13 20:27:13 PDT
Jesse, should the testcase in this bug land as a crashtest with the patch?  Or wait till later?
Comment 12 Jesse Ruderman 2012-06-13 20:57:02 PDT
Since content couldn't trigger the bug before cpg, I'd prefer for the patch to land with a testcase.
Comment 13 Bobby Holley (busy) 2012-06-14 02:35:09 PDT
Be warned - most of the compartment mismatches that we found from CPG can also be triggered pre-cpg by using document.domain to create same-origin cross-compartment scopes.
Comment 14 Bobby Holley (busy) 2012-06-14 02:41:52 PDT
Comment on attachment 632999 [details] [diff] [review]
Take a bit more care about our compartments in SetJSEventListenerToJsval.


>+  // Now ensure that we're working in the compartment of aScope from now on.
>+  JSAutoEnterCompartment ac;
>+  if (js::GetObjectCompartment(handler) != js::GetObjectCompartment(aScope)) {

Why is this necessary? JSAutoEnterCompartment::enter is a no-op if cx->compartment == aScope->compartment(). Is the issue that we just don't care about the cx compartment if the two objects match (and we only enter the compartment to do the wrapping?). If so, I think that scoping the JSAutoEnterCompartment to the JS_WrapValue call would be clearer. Or we could just enter the compartment unconditionally. The overhead of the extra equality check is probably worse than the overhead of an unconditional compartment enter.

>+    MOZ_ASSERT(tempVal.isObject());
>+    handler = &tempVal.toObject();

.toObject() JS_ASSERTs isObject, so I don't think the MOZ_ASSERT here is necessary.

I don't understand the broader context of this patch, but presumably smaug does.
Comment 15 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-06-14 04:45:23 PDT
Comment on attachment 632999 [details] [diff] [review]
Take a bit more care about our compartments in SetJSEventListenerToJsval.

Does JS_WrapValue always wrap, even if the compartments are the same?
And if not, is it fast when it doesn't wrap?
Would be great if we could do this all unconditionally.
{
  JSAutoEnterCompartment ac;
  NS_ENSURE_STATE(ac.enter(cx, aScope));
  jsval tempVal = v;
  NS_ENSURE_STATE(JS_WrapValue(cx, &tempVal));
  handler = &tempVal.toObject();
}

But if JS_WrapValue does always wrap, then I guess we need to do what the patch does.
Comment 16 Bobby Holley (busy) 2012-06-14 05:25:26 PDT
(In reply to Olli Pettay [:smaug] from comment #15)
> Comment on attachment 632999 [details] [diff] [review]
> Take a bit more care about our compartments in SetJSEventListenerToJsval.
> 
> Does JS_WrapValue always wrap, even if the compartments are the same?

It does not.

> And if not, is it fast when it doesn't wrap?

Pretty fast. It still has to go through a few conditionals, but nothing heavyweight.

> Would be great if we could do this all unconditionally.

Agreed.
Comment 17 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-06-14 07:10:49 PDT
> Why is this necessary

To avoid the cost of the JS_WrapValue in the common case.

> The overhead of the extra equality check is probably worse than the overhead of an
> unconditional compartment enter.

In my testing, the overhead of the well-predicted branch is way lower than the cost of the JS_WrapValue call...

> Pretty fast. It still has to go through a few conditionals, but nothing heavyweight.

I guess it depends on your timescales.  In the binding code, JS_WrapValue is a pretty noticeable cost compared to the rest of the binding code, since it involves a slow out of line function call in addition to a bunch of conditionals.

But you might be right that on the timescale of this setter it doesn't matter.  I'll measure, and if that's the case I'll remove the conditional goop.
Comment 18 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-06-14 07:11:31 PDT
And yes, this could be triggered with document.domain pre-cpg, I bet....  Jesse?
Comment 19 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-06-14 10:13:26 PDT
So I did some measuring.

Using a typical microbenchmark setup (one element, one function, set onclick over and over again, which is dumb of course), my patch takes about 1.4 microseconds per call.  If I make the compartment stuff unconditional, it gets about 10% slower (so figure about 140ns to do the ac.enter() and JS_WrapValue bits, which about matches my experiences with profiling bindings).

Olli, preferences?  Note that in the common case (on* being set first time) the time will probably be much larger since we have to allocate the nsJSEventListener and whatnot.
Comment 20 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-06-14 10:23:57 PDT
Created attachment 633178 [details] [diff] [review]
Take a bit more care about our compartments in SetJSEventListenerToJsval.
Comment 21 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-06-14 10:25:08 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/5674051c17d7

I made the wrapvalue unconditional, per discussion with Olli.

For now landed without the testcase due to the document.domain thing.
Comment 22 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-06-14 10:26:56 PDT
Comment on attachment 633178 [details] [diff] [review]
Take a bit more care about our compartments in SetJSEventListenerToJsval.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 659350 
User impact if declined: Mostly safe, I think, though there _may_ be a security
   problem when document.domain is in use.
Testing completed (on m-c, etc.): Passes attached testcase.
Risk to taking this patch (and alternatives if risky): Low risk, imo.  But the
   other option is to just do nothing about this on branches.
String or UUID changes made by this patch: None.
Comment 23 Ed Morley [:emorley] 2012-06-15 06:12:29 PDT
https://hg.mozilla.org/mozilla-central/rev/5674051c17d7
Comment 24 Lukas Blakk [:lsblakk] use ?needinfo 2012-06-18 13:26:51 PDT
We're looking for a security rating on this bug, and also a decision from sec group as comment 22 is questioning the need for this on branches.
Comment 25 Alex Keybl [:akeybl] 2012-06-19 20:15:53 PDT
Comment on attachment 633178 [details] [diff] [review]
Take a bit more care about our compartments in SetJSEventListenerToJsval.

[Triage Comment]
Dan let us know over IRC that he plans to give this an sg:crit rating, so let's land on all branches.
Comment 26 Alex Keybl [:akeybl] 2012-06-19 20:17:22 PDT
Please also prepare a patch for the ESR branch.
Comment 28 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-06-20 21:35:20 PDT
Comment on attachment 633178 [details] [diff] [review]
Take a bit more care about our compartments in SetJSEventListenerToJsval.

This patch applies just fine to ESR.
Comment 29 Lukas Blakk [:lsblakk] use ?needinfo 2012-06-22 14:51:02 PDT
Comment on attachment 633178 [details] [diff] [review]
Take a bit more care about our compartments in SetJSEventListenerToJsval.

Thanks, we were just waiting on the beta landing before approving for ESR - please go ahead.
Comment 30 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-06-22 14:52:25 PDT
Verified fixed in 2012-06-22 Beta, Aurora, and Nightly debug builds.
Comment 31 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-06-25 20:37:39 PDT
http://hg.mozilla.org/releases/mozilla-esr10/rev/a55c87cdd4db
Comment 32 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-07-11 10:29:37 PDT
Verified fixed in Firefox 10.0.6esrpre Debug 2012-07-11 using the attached testcase.
Comment 33 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-12-11 11:59:45 PST
*** Bug 720714 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.