679494 - "compartment mismatched" when listening message event

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Alexandre Poirot [:ochameau]]

Assertion failure: compartment mismatched, at /home/pouf/mozilla-central/js/src/jscntxtinlines.h:120

when running the following script in JS console:

var w=top.opener.gBrowser.contentDocument.defaultView; 
w.addEventListener("message", function(event) {
  alert(event.data);
}, false);
w.postMessage("test");

I suppose that it only occurs when we listen to `message` event from another compartment than the event source document.

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Alex found me on IRC and was kind enough to pastebin a stack trace. The relevant bit is:

#9  0x025800a8 in js::CallJSPropertyOp (cx=0xb1d0b570, op=
    0x19b0a65 <nsIDOMMessageEvent_GetData(JSContext*, JSObject*, jsid, jsval*)>, receiver=0xad49be40, id=..., vp=0xbfffb638)

this shows us that the problem is a getter returning a value in another compartment. Some quick MXRing later shows us that the problem is that nsDOMMessageEvent::GetData passes through its jsval without wrapping it. So, we need to call JS_WrapValue before returning the data, but in order to do that, we're going to need to add [implicit_jscontext] to the IDL in order to get a cx with with to call JS_WrapValue.

Comment 2

[Bobby Holley (:bholley)]

Attachment: stacktrace

Attaching the full stack trace to give n00bs like myself a better sense of what's going on.

Comment 3

[Dave Townsend [:mossop]]

What is the priority of getting a fix here? This is causing the Jetpack tests to fail and presumably causing problems for add-ons using the functionality we expose

Comment 4

[Bobby Holley (:bholley)]

(In reply to Dave Townsend (:Mossop) from comment #3)
> What is the priority of getting a fix here? This is causing the Jetpack
> tests to fail and presumably causing problems for add-ons using the
> functionality we expose

I've got a patch. Just tweaking it a bit and I'll post it for review real soon now.

Comment 5

[Bobby Holley (:bholley)]

Attachment: Bug 679494 - Wrap value returned from nsDOMMessageEvent::GetData. v1

Comment 6

[Bobby Holley (:bholley)]

Attachment: Bug 679494 - Tests. v1

Comment 7

[Bobby Holley (:bholley)]

Comment on attachment 554168 [details] [diff] [review]
Bug 679494 - Tests. v1

Added a patch and tests. Flagging bkap for review.

Comment 8

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

Why do you need a browser chrome test here?  You should be able to do it with a chrome test with a content iframe, no?

Comment 9

[Bobby Holley (:bholley)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #8)
> Why do you need a browser chrome test here?  You should be able to do it
> with a chrome test with a content iframe, no?

Probably - I was just trying to match the test-case as much as possible. Would that be preferable? I'm not sure of the relative overhead.

Comment 10

[Bobby Holley (:bholley)]

Attachment: Bug 679494 - Tests v2

> Would that be preferable? I'm not sure of the relative overhead.

I'm going to assume from comment 8 that it is. Here's an updated version. Flagging khuey for review.

Comment 11

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

Comment on attachment 554195 [details] [diff] [review]
Bug 679494 - Tests v2

Yeah, this is better.

Chrome tests are preferable to browser-chrome tests because they run in all Gecko things and not Firefox.  We generally avoid browser-chrome tests for anything except the browser UI.

Comment 12

[Mozilla RelEng Bot]

Try run for e05ba95b7e9e is complete.
Detailed breakdown of the results available here:
    http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=e05ba95b7e9e
Results (out of 27 total builds):
    success: 25
    warnings: 2
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-e05ba95b7e9e

Comment 13

[Bobby Holley (:bholley)]

Landed on mozilla-inbound:

http://hg.mozilla.org/integration/mozilla-inbound/rev/70a6be6a9caa
http://hg.mozilla.org/integration/mozilla-inbound/rev/b1844f9cf777

Comment 14

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

http://hg.mozilla.org/mozilla-central/rev/70a6be6a9caa
http://hg.mozilla.org/mozilla-central/rev/b1844f9cf777

Comment 15

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

And this should have had a uuid bump, surely?

Comment 16

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

(In reply to Ms2ger from comment #15)
> And this should have had a uuid bump, surely?

It needs one, yes.

Comment 17

[Bobby Holley (:bholley)]

Attachment: Bug 679494 - UUID rev followup. v1

D'oh!. Attaching a followup. Flagging khuey for review.

Comment 18

[Bobby Holley (:bholley)]

Landed on mozilla-inbound:

http://hg.mozilla.org/integration/mozilla-inbound/rev/98725bc76d7e

Comment 19

[Mounir Lamouri (:mounir)]

http://hg.mozilla.org/mozilla-central/rev/98725bc76d7e

Comment 21

[LegNeato]

Why was this nominated for beta? What is the risk vs reward? We're heavily leaning against taking this due to the uuid change.

Comment 22

[Johnny Stenback (:jst)]

Comment on attachment 554905 [details] [diff] [review]
Bug 679494 - UUID rev followup. v1

We're going to wait another 6 weeks and get this in Firefox 9 since this is already in aurora (and the reasons that made me ask bholley to request approval here have since been invalidated, or at least lowered in weight).

Comment 23

[Biru [:poiru]]

The test incorrectly referenced file_679494.html instead of file_bug679494.html, fixed in:

https://hg.mozilla.org/integration/mozilla-inbound/rev/28a7a3744901

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/28a7a3744901