Last Comment Bug 644325 - addMessageListener with an function object causes a compartment mismatch
: addMessageListener with an function object causes a compartment mismatch
: assertion
Product: Core
Classification: Components
Component: IPC (show other bugs)
: Trunk
: x86 Linux
: -- normal (vote)
: ---
Assigned To: Josh Matthews [:jdm]
Depends on:
Blocks: 664821
  Show dependency treegraph
Reported: 2011-03-23 14:00 PDT by Josh Matthews [:jdm]
Modified: 2011-08-26 06:03 PDT (History)
12 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Backtrace (6.79 KB, text/plain)
2011-03-23 14:01 PDT, Josh Matthews [:jdm]
no flags Details
Wrap message receiver to avoid compartment errors. (1.18 KB, patch)
2011-03-23 14:51 PDT, Josh Matthews [:jdm]
jst: review+
Details | Diff | Splinter Review
Patch (2.01 KB, patch)
2011-05-12 14:31 PDT, Josh Matthews [:jdm]
mrbkap: review+
christian: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Josh Matthews [:jdm] 2011-03-23 14:00:58 PDT
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.
Comment 1 Josh Matthews [:jdm] 2011-03-23 14:01:18 PDT
Created attachment 521301 [details]
Comment 2 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-03-23 14:28:03 PDT
I have no idea how to fix compartment mismatch problems.
Comment 3 Josh Matthews [:jdm] 2011-03-23 14:51:23 PDT
Created attachment 521319 [details] [diff] [review]
Wrap message receiver to avoid compartment errors.
Comment 4 Mark Finkle (:mfinkle) (use needinfo?) 2011-03-23 21:02:58 PDT
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.
Comment 5 Brad Lassey [:blassey] (use needinfo?) 2011-03-23 21:06:31 PDT
is this showing up on crash stats?
Comment 6 Mark Finkle (:mfinkle) (use needinfo?) 2011-03-23 22:46:33 PDT
(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.
Comment 7 Josh Matthews [:jdm] 2011-03-23 23:02:39 PDT
My STR jives with your theory.  The assertion I was seeing was in the content process using the global message manager.
Comment 8 Daniel Veditz [:dveditz] 2011-04-13 10:43:12 PDT
Please land on mozilla-central, then we can talk about whether it blocks a stability/security update
Comment 9 Johnny Stenback (:jst, 2011-04-14 20:38:52 PDT
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.
Comment 10 Blake Kaplan (:mrbkap) 2011-04-19 16:44:44 PDT
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+.
Comment 11 Josh Matthews [:jdm] 2011-04-20 11:22:06 PDT
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.
Comment 12 Blake Kaplan (:mrbkap) 2011-04-23 13:53:30 PDT
To be clear, the aObjectsArray question was in addition to the other stuff I said.
Comment 13 Josh Matthews [:jdm] 2011-04-23 16:14:19 PDT
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.
Comment 14 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-05-12 05:24:02 PDT
Comment 15 Josh Matthews [:jdm] 2011-05-12 14:29:00 PDT
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.
Comment 16 Josh Matthews [:jdm] 2011-05-12 14:31:02 PDT
Created attachment 532029 [details] [diff] [review]
Comment 17 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-05-12 15:49:48 PDT
Oh. Sorry. I needed the patch since test.xul didn't work
without it.
Comment 18 Blake Kaplan (:mrbkap) 2011-05-16 02:56:17 PDT
Comment on attachment 532029 [details] [diff] [review]

Looks good. Thanks.
Comment 19 Kevin Brosnan [:kbrosnan] 2011-05-18 15:03:09 PDT
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?
Comment 20 Josh Matthews [:jdm] 2011-05-18 23:59:46 PDT
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.
Comment 21 Josh Matthews [:jdm] 2011-05-30 12:01:48 PDT - the backout - the new, correct patch
Comment 22 Josh Matthews [:jdm] 2011-06-28 15:19:54 PDT
Comment on attachment 532029 [details] [diff] [review]

This fixes a crash for certain uses of the API we expect addons to use.
Comment 23 Josh Matthews [:jdm] 2011-06-29 11:58:55 PDT
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.
Comment 24 Josh Matthews [:jdm] 2011-06-29 11:59:26 PDT
Typo: comment 23 should have read "understood AND fixed"
Comment 25 christian 2011-06-30 14:27:55 PDT
Comment on attachment 532029 [details] [diff] [review]

Approved for releases/mozilla-aurora. Please land ASAP
Comment 27 Ioana (away) 2011-08-26 04:59:00 PDT
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
Comment 28 Josh Matthews [:jdm] 2011-08-26 05:53:02 PDT
Yes, that is the button I meant, and that is the expected result.
Comment 29 Ioana (away) 2011-08-26 06:03:00 PDT
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.

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