"compartment mismatched" when listening message event

RESOLVED FIXED in mozilla9

Status

()

Core
XPConnect
RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: ochameau, Assigned: bholley)

Tracking

Trunk
mozilla9
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
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.
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.
Assignee: nobody → bobbyholley+bmo
Blocks: 679539
Created attachment 553858 [details]
stacktrace

Attaching the full stack trace to give n00bs like myself a better sense of what's going on.
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
(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.
Created attachment 554167 [details] [diff] [review]
Bug 679494 - Wrap value returned from nsDOMMessageEvent::GetData. v1
Created attachment 554168 [details] [diff] [review]
Bug 679494 - Tests. v1
Attachment #554167 - Flags: review?(mrbkap)
Comment on attachment 554168 [details] [diff] [review]
Bug 679494 - Tests. v1

Added a patch and tests. Flagging bkap for review.
Attachment #554168 - Flags: review?(mrbkap)
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?
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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.
Created attachment 554195 [details] [diff] [review]
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.
Attachment #554168 - Attachment is obsolete: true
Attachment #554168 - Flags: review?(mrbkap)
Attachment #554195 - Flags: review?(khuey)
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.
Attachment #554195 - Flags: review?(khuey) → review+

Updated

6 years ago
Attachment #554167 - Flags: review?(mrbkap) → review+

Comment 12

6 years ago
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/bobbyholley@gmail.com-e05ba95b7e9e
Landed on mozilla-inbound:

http://hg.mozilla.org/integration/mozilla-inbound/rev/70a6be6a9caa
http://hg.mozilla.org/integration/mozilla-inbound/rev/b1844f9cf777
Whiteboard: [inbound]
http://hg.mozilla.org/mozilla-central/rev/70a6be6a9caa
http://hg.mozilla.org/mozilla-central/rev/b1844f9cf777
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
Version: unspecified → Trunk
And this should have had a uuid bump, surely?
Whiteboard: [inbound]
(In reply to Ms2ger from comment #15)
> And this should have had a uuid bump, surely?

It needs one, yes.
Created attachment 554905 [details] [diff] [review]
Bug 679494 - UUID rev followup. v1

D'oh!. Attaching a followup. Flagging khuey for review.
Attachment #554905 - Flags: review?(khuey)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #554905 - Flags: review?(khuey) → review+
Landed on mozilla-inbound:

http://hg.mozilla.org/integration/mozilla-inbound/rev/98725bc76d7e
Whiteboard: [inbound]
http://hg.mozilla.org/mozilla-central/rev/98725bc76d7e
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]

Updated

6 years ago
Duplicate of this bug: 688364
Attachment #554167 - Flags: approval-mozilla-beta?
Attachment #554905 - Flags: approval-mozilla-beta?
Attachment #554195 - Flags: approval-mozilla-beta?

Comment 21

6 years ago
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 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).
Attachment #554905 - Flags: approval-mozilla-beta? → approval-mozilla-beta-

Updated

6 years ago
Attachment #554195 - Flags: approval-mozilla-beta? → approval-mozilla-beta-

Updated

6 years ago
Attachment #554167 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
The test incorrectly referenced file_679494.html instead of file_bug679494.html, fixed in:

https://hg.mozilla.org/integration/mozilla-inbound/rev/28a7a3744901
https://hg.mozilla.org/mozilla-central/rev/28a7a3744901
You need to log in before you can comment on or make changes to this bug.