Closed Bug 644325 Opened 13 years ago Closed 13 years ago

addMessageListener with an function object causes a compartment mismatch


(Core :: IPC, defect)

Not set



Tracking Status
firefox6 --- fixed
firefox7 --- fixed
fennec 7+ ---


(Reporter: jdm, Assigned: jdm)



(Keywords: assertion)


(2 files, 1 obsolete file)

Dunno when this broke, but it seems that any instance of |addMessageListener("name", function(m) {})| asserts hard.  Looking at the failing check, apparently the function object is in a different compartment than the context and wrapped thisValue.

To reproduce, firefox -no-remote -P blank -chrome chrome://global/content/test-ipc.xul and click the add/remove message listener button.
Attached file Backtrace
I have no idea how to fix compartment mismatch problems.
Attachment #521319 - Flags: review?(jst)
I saw this in an add-on was I am building. With the patch applied I see no crashes and the message is delivered correctly.
Whiteboard: [fennec-rc2?]
is this showing up on crash stats?
(In reply to comment #5)
> is this showing up on crash stats?

Not that I could find.

I am wondering if using a global message manager is causes this compartment issue. I am building a bootstrapped addon, so I don't have access to a window when loading the content script. So I am using a global message manager.

Josh's STR for the crash might not jive with my theory though.

Given that we don't see these crashes in our browser-chrome tests, which use addMessageListener and JS funcs, maybe this is less of a critical issue than I originally thought.
Whiteboard: [fennec-rc2?]
My STR jives with your theory.  The assertion I was seeing was in the content process using the global message manager.
Assignee: nobody → josh
tracking-fennec: ? → 2.0-
Whiteboard: [fennec-4.1?]
Please land on mozilla-central, then we can talk about whether it blocks a stability/security update
blocking2.0: ? → ---
Comment on attachment 521319 [details] [diff] [review]
Wrap message receiver to avoid compartment errors.

This looks ok to me, but I'm not 100% sure we end up in the desired compartment here. Blake, we end up entering the this object's compartment and wrapping both the function and its values. I'm wondering if we should simply enter the function's compartment here, and wrap the this object instead? This should all be chrome code afaict.
Attachment #521319 - Flags: review?(mrbkap)
Attachment #521319 - Flags: review?(jst)
Attachment #521319 - Flags: review+
So, I'm a little confused by the compartment stuff in this function. I suspect that, like jst says, this mostly works because most of the compartments in play here are chrome.

It seems like what should happen is that we should enter |object|'s compartment as soon as we do the JSAutoEnterRequest. Also, the scope to the calls for WrapNative should probably either be |object| or JS_GetGlobalForObject(cx, object) (that has the effect of doing the WrapObject for you). My only concern then would be aObjectsArray, but if that's always in the chrome compartment, then you probably don't have to worry about it. Am I off-base? If so, I'll stamp r+.
I do not believe aObjectsArray is ever non-null right now.  That may change with bsmedberg's work to add handles to messages, but that is not landing imminently.  Therefore, I believe that aObjectsArray is always in the chrome compartment.
To be clear, the aObjectsArray question was in addition to the other stuff I said.
Right; I was just clarifying my about the one bit that you seemed unsure about.  The rest sounds sensible, and I'd whip up a patch if I actually had a machine on which I could build and a recent tree.  Unfortunately, someone else is probably going to have to do it if this patch is going to be written before June.
Attachment #521319 - Flags: review?(mrbkap)
tracking-fennec: - → 7+
Whiteboard: [fennec-4.1?]
Closed: 13 years ago
Resolution: --- → FIXED
Olli, based on Blake's comments in comment 10, I don't think the attached patch should have landed. I'm uploading a more correct patch imminently.
Resolution: FIXED → ---
Attached patch PatchSplinter Review
Attachment #521319 - Attachment is obsolete: true
Attachment #532029 - Flags: review?(mrbkap)
Oh. Sorry. I needed the patch since test.xul didn't work
without it.
Keywords: checkin-needed
Comment on attachment 532029 [details] [diff] [review]

Looks good. Thanks.
Attachment #532029 - Flags: review?(mrbkap) → review+
jdm asked for some testing of the patch in #mobile. I did about 15 min of browsing without issue. Is there something specific we should look for in the build?
I just wanted to make sure that nothing else seemed terribly broken by the changes, since I tested the case that this was originally failing on. When somebody pushes, please backout 7e3efb1073a9 and push this patch instead.
Keywords: checkin-needed
Whiteboard: [read comment 20 before pushing] - the backout - the new, correct patch
Closed: 13 years ago13 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [read comment 20 before pushing]
Comment on attachment 532029 [details] [diff] [review]

This fixes a crash for certain uses of the API we expect addons to use.
Attachment #532029 - Flags: approval-mozilla-aurora?
Blocks: 664821
Risk: it's known that this patch does cause other crashes which are understood fixed by the approved-but-dependent bug 664821. Reward: this fixes a crash in a well-understood way which can be triggered by extension authors.
Typo: comment 23 should have read "understood AND fixed"
Comment on attachment 532029 [details] [diff] [review]

Approved for releases/mozilla-aurora. Please land ASAP
Attachment #532029 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Hi Josh,

In the bug description you said "To reproduce, firefox -no-remote -P blank -chrome chrome://global/content/test-ipc.xul and click the add/remove message listener button."

I used that but there was only one button for adding listeners, the "test listener add/remove" button. Is this the button you were referring to?

An alert with the "Expected echo message" string and an "OK" button was displayed when I clicked that button. Is this the expected result?

If not, can you or anyone else please help me with a test case or with STR / guidelines that I can use to verify this fix?

Thank you
Yes, that is the button I meant, and that is the expected result.
Verified fixed on:
 Mozilla/5.0 (X11; Linux x86_64; rv:6.0) Gecko/20100101 Firefox/6.0
 Mozilla/5.0 (X11; Linux i686 on x86_64; rv:7.0) Gecko/20100101 Firefox/7.0

Used scenario:
 1. Start Firefox like this: firefox -no-remote -P blank -chrome chrome://global/content/test-ipc.xul
 2. Tap on the "test listener add/remove" button.
An alert with the "Expected echo message" string and an "OK" button are displayed when I clicked that button.
You need to log in before you can comment on or make changes to this bug.