I found 3 crashes like this when searching through some crash stacks for the cross-compartment pointer checker. 0 js::CompartmentChecker::fail js/src/jscntxtinlines.h:205 1 JS_GetStringCharsZAndLength js/src/jsapi.cpp:6055 2 JSValToNPVariant dom/plugins/base/nsJSNPRuntime.cpp:435 3 mozilla::plugins::parent::_evaluate dom/plugins/base/nsNPAPIPlugin.cpp:1599 4 mozilla::plugins::PluginScriptableObjectParent::AnswerNPN_Evaluate dom/plugins/ipc/PluginScriptableObjectParent.cpp:1197 5 mozilla::plugins::PPluginScriptableObjectParent::OnCallReceived obj-firefox/ipc/ipdl/PPluginScriptableObjectParent.cpp:776 6 mozilla::plugins::PPluginModuleParent::OnCallReceived obj-firefox/ipc/ipdl/PPluginModuleParent.cpp:1288 I'm not sure exactly what is going wrong, because the string comes from |scx->EvaluateStringWithValue|, where |scx| is the |GetScriptContextFromJSContext| of the context we're calling JS_GetStringCharsZAndLength on. https://crash-stats.mozilla.com/report/index/b5621c05-db57-412e-bdb1-b475c2130101 https://crash-stats.mozilla.com/report/index/f97d5d0f-f881-4f45-b755-efad72121229 https://crash-stats.mozilla.com/report/index/995820a8-b2b8-40c5-b050-785ab2130101

(In reply to Andrew McCreight [:mccr8] from comment #0) > I'm not sure exactly what is going wrong, because the string comes from > |scx->EvaluateStringWithValue|, where |scx| is the > |GetScriptContextFromJSContext| of the context we're calling > JS_GetStringCharsZAndLength on. That doesn't mean the cx is still in that compartment. In fact, it doesn't even mean that that cx is at the top of the cx stack anymore, because EvaluteStringWithValue does both a cx push (which includes an implicit scoped JS_SaveFrameChain/JS_RestoreFrameChain (temporarily making us look as if we'd entered no compartment at all)), and a JSAutoEnterCompartment. I'm refatoring this code in bug 824864. But I think we probably need to enter the compartment of the returned jsval here. We might even want to push the cx, though I'm not sure about that.

Ah, good point!

See also bug 803228 which could be related. I promised to try and brainstorm some testcases there but haven't yet :-(

Here are some more crashes, now that the checker has hit Aurora: https://crash-stats.mozilla.com/report/index/22e086e5-29ae-4d2e-b497-777572130116 https://crash-stats.mozilla.com/report/index/4e143557-0b3b-4388-8467-5d84b2130116 https://crash-stats.mozilla.com/report/index/ae66bcf8-cea0-40c7-ab46-e8d8d2130116 https://crash-stats.mozilla.com/report/index/594a2e98-c1ac-45a1-8e8d-6b32c2130115 https://crash-stats.mozilla.com/report/index/53f67503-0d44-4b43-a03f-b2d972130115

bsmedberg is presumbly super busy, and I can write a patch for this, but not a test case, so I'll grab this for now.

It looks like this is an old problem.

Bill suggested that this might be fixed by bug 824864, and indeed I haven't seen any crashes in recent Nightly builds. Unfortunately, that patch is huge, so we probably don't want to backport it everywhere. Bill said you shouldn't enter the compartment of a string (because you can end up in the atoms compartment), so the simple solution here isn't going to work.

Presumably we don't care about NPAPI plugins on b2g, and we're not going to get anything together in time for 19.

Not sure what release management is going to want to do here. Compartment failures can theoretically be exploited, but we don't have a specific way to reproduce this particular one. The "fix" in bug 824864 is bigger than we'd like to back-port to ESR-17 -- might even depend on other changes between 17 and 21 in unexpected ways. We don't need to push this into Firefox 20, it's ESR-17 where we need to make the call since Firefox 21 will contain a fix (we think). I guess we could look for this signature on ESR-17 and see if it even happens there.

There are no crashes prior to Firefox 20 https://crash-stats.mozilla.com/report/list?product=Firefox&query_search=signature&query_type=contains&query=js%3A%3ACompartmentChecker%3A%3Afail&reason_type=contains&date=02%2F08%2F2013%2000%3A13%3A35&range_value=2&range_unit=weeks&hang_type=any&process_type=any&do_query=1&signature=js%3A%3ACompartmentChecker%3A%3Afail%28JSCompartment*%2C%20JSCompartment*%29 But presumably the issue is there and simply not detected.

Yes, this relies on special release mode assertions that are only enabled on Nightly and Aurora.

Bill, is there any chance I could write a hacky string compartment entering thing that explicitly checks if it is the atoms compartment? I'm not sure what else to do here.

I think we just need to change EvaluateStringWithValue to wrap the result its returning in whatever the current compartment is. I think that's all the fix in bug 824864 is buying us here. Maybe I'm wrong though. Bobby?

Well, there was also a fair amount of fallout from that bug. Can't we just add a call to JS_WrapValue after the call to scx->EvaluateString?

(In reply to Bobby Holley (:bholley) from comment #15) > Well, there was also a fair amount of fallout from that bug. Can't we just > add a call to JS_WrapValue after the call to scx->EvaluateString? Yeah, that seems fine too.

As comment 10 mentions, we don't need to push this to FF 20 and since we haven't any indication (in crash reports) that this is even occurring on ESR we'll wontfix there and if it shows up we can look into options for safe backporting.

Well, these crashes will never show up on ESR17, because they only show up on Nightly and Aurora because of special runtime checks we added there. The suggested fix in comment 15 is probably small enough to consider backporting, but I'll worry about that when the patch exists.

Bill, in one of your zones patch, you just drop the same-compartment check on JS_GetStringCharsZAndLength, which is precisely the one we're trying to fix here. Do you think it is probably safe to not care about the compartments on all branches? If so, we can just won't fix this everywhere. [1] https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=759585&attachment=714648

Okay, well, it is already wontfixed everywhere, but we could rest easy knowing that it isn't really a problem anywhere...

(In reply to Andrew McCreight [:mccr8] from comment #19) > Bill, in one of your zones patch, you just drop the same-compartment check > on JS_GetStringCharsZAndLength, which is precisely the one we're trying to > fix here. Do you think it is probably safe to not care about the > compartments on all branches? If so, we can just won't fix this everywhere. Sure.