Last Comment Bug 736752 - Compartment mismatch in JetPack 'test-content-proxy.testTypedArrays'
: Compartment mismatch in JetPack 'test-content-proxy.testTypedArrays'
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla14
Assigned To: :Ms2ger
:
Mentors:
Depends on:
Blocks: 550309 736699
  Show dependency treegraph
 
Reported: 2012-03-17 10:44 PDT by :Ms2ger
Modified: 2012-03-22 20:31 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Backtrace (9.81 KB, text/plain)
2012-03-20 02:33 PDT, Alexandre Poirot [:ochameau]
no flags Details
Patch v1 (2.06 KB, patch)
2012-03-20 13:57 PDT, :Ms2ger
bobbyholley: review+
Details | Diff | Review

Description :Ms2ger 2012-03-17 10:44:32 PDT
The test at

https://github.com/mozilla/addon-sdk/blob/master/packages/api-utils/tests/test-content-proxy.js#L743

hits a compartment mismatch; see

https://tbpl.mozilla.org/?noignore=1&jobname=jetpack

Hoping for smart people who can tell me what I need to do here.
Comment 1 Bobby Holley (busy) 2012-03-17 16:10:10 PDT
crash stack?

Also, when did this start happening?
Comment 2 Wes Kocher (:KWierso) 2012-03-17 16:34:11 PDT
(In reply to Bobby Holley (:bholley) from comment #1)
> crash stack?
> 
> Also, when did this start happening?

This push had JP tests as permaorange-but-really-greenish: https://tbpl.mozilla.org/?noignore=1&jobname=jetpack&rev=e5f6caa40409

The next push after that turned them permared-but-really-orangish: https://tbpl.mozilla.org/?noignore=1&jobname=jetpack&rev=ee56787a20fb
Comment 3 Alexandre Poirot [:ochameau] 2012-03-19 10:27:49 PDT
The unit test isn't perfect, may be too agressive on its checks.


Jetpack (over) use sandboxes. So in this particular test case we have 3 sandboxes and the content document:
- 1 chrome sandbox hosting the test module (not really important here)
- 1 content document
- 1 content sandbox with same principal than the document, hosting the proxy code
- 1 another similar sandbox, hosting the content script

It looks like we are hitting some wrapper behavior difference. I can't exactly tell if the new behavior is expected or not.
Here is a minimal (scratchpad) testcase that highlights what changed.
let Cu = Components.utils;

// First content sandbox that simulates the proxy code
let s1 = Cu.Sandbox(content, {wantXrays:true});
function foo(doc) {
  let canvas = doc.createElement("canvas");
  let context = canvas.getContext("2d");
  let proxy = context.getImageData(0,0, 1, 1);
  return {
    proxy: proxy,
    get data () proxy.data
  };
}
Cu.evalInSandbox(foo, s1);

// Second content sandbox that simulates the content script code
let s2 = Cu.Sandbox(content, {wantXrays:true});
function bar() {
  // Here, on nightly, it is different.
  // I assume we get some king of wrappers whereas we didn't get any visible wrapper before.
  return obj.proxy.data === obj.data;
}

// Call first sandbox code and give the result back to the second one
s2.obj = s1.foo(content.document);
let rv = Cu.evalInSandbox("("+bar+"())", s2)

// `rv` was true, now it is false
Cu.reportError(">> " + rv);
Comment 4 Bobby Holley (busy) 2012-03-19 12:09:47 PDT
(In reply to Alexandre Poirot (:ochameau) from comment #3)
> The unit test isn't perfect, may be too agressive on its checks.

Possibly, but it shouldn't matter - JS should never be able to trigger a compartment mismatch. So it's almost certainly a bug in spidermonkey or XPConnect.

If somebody posts a reduced testcase that hits the cross-compartment assertion, I'll fire up gdb to see what's going on.
Comment 5 Alexandre Poirot [:ochameau] 2012-03-20 02:33:05 PDT
Created attachment 607493 [details]
Backtrace

(In reply to Bobby Holley (:bholley) from comment #4)
> If somebody posts a reduced testcase that hits the cross-compartment
> assertion, I'll fire up gdb to see what's going on.

The script inlined in comment 3 is such testcase. It hits this assertion.
Here is the backtrace while running it manually in scratchpad.
Comment 6 :Ms2ger 2012-03-20 03:34:55 PDT
static JSBool
nsIDOMImageData_GetData(JSContext *cx, JSObject *obj, jsid id, jsval *vp)
{
    XPC_QS_ASSERT_CONTEXT_OK(cx);
    nsIDOMImageData *self;
    xpc_qsSelfRef selfref;
    if (!xpc_qsUnwrapThis(cx, obj, &self, &selfref.ptr, vp, nsnull, true))
        return JS_FALSE;
    self->GetData(vp);
    return JS_TRUE;
}

After the call, obj and *vp aren't in the same compartment (according to the bt).
Comment 7 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-03-20 07:58:01 PDT
That's because GetData needs to JS_WrapValue the retval, I like told you on IRC on Saturday.
Comment 8 :Ms2ger 2012-03-20 13:57:15 PDT
Created attachment 607709 [details] [diff] [review]
Patch v1

Something like this, perhaps
Comment 9 Phil Ringnalda (:philor) 2012-03-22 20:31:10 PDT
https://hg.mozilla.org/mozilla-central/rev/ab2ff3b5611f

Yeah, I did want to push it, but I was still at work then ;)

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