644325 - addMessageListener with an function object causes a compartment mismatch

Status: VERIFIED FIXED

(Core :: IPC, defect)

Description

[Josh Matthews [:jdm]]

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]]

Attachment: Backtrace

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

I have no idea how to fix compartment mismatch problems.

Comment 3

[Josh Matthews [:jdm]]

Attachment: Wrap message receiver to avoid compartment errors.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

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?)]

is this showing up on crash stats?

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

(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]]

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]]

Please land on mozilla-central, then we can talk about whether it blocks a stability/security update

Comment 9

[Johnny Stenback (:jst)]

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) (inactive)]

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]]

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) (inactive)]

To be clear, the aObjectsArray question was in addition to the other stuff I said.

Comment 13

[Josh Matthews [:jdm]]

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][[e-mail]]]

http://hg.mozilla.org/mozilla-central/rev/7e3efb1073a9

Comment 15

[Josh Matthews [:jdm]]

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]]

Attachment: Patch

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Oh. Sorry. I needed the patch since test.xul didn't work
without it.

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 532029 [details] [diff] [review]
Patch

Looks good. Thanks.

Comment 19

[Kevin Brosnan [Ex-Mozilla]]

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? http://people.mozilla.com/tmp/fennec-644325-6.0a1.en-US.eabi-arm.apk

Comment 20

[Josh Matthews [:jdm]]

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]]

http://hg.mozilla.org/mozilla-central/rev/5a47cc68c631 - the backout
http://hg.mozilla.org/mozilla-central/rev/3ff03c212c7f - the new, correct patch

Comment 22

[Josh Matthews [:jdm]]

Comment on attachment 532029 [details] [diff] [review]
Patch

This fixes a crash for certain uses of the API we expect addons to use.

Comment 23

[Josh Matthews [:jdm]]

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]]

Typo: comment 23 should have read "understood AND fixed"

Comment 25

[LegNeato]

Comment on attachment 532029 [details] [diff] [review]
Patch

Approved for releases/mozilla-aurora. Please land ASAP

Comment 26

[Josh Matthews [:jdm]]

http://hg.mozilla.org/releases/mozilla-aurora/rev/045847071b97

Comment 27

[Ioana (away)]

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]]

Yes, that is the button I meant, and that is the expected result.

Comment 29

[Ioana (away)]

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.