On Nov 26, 2012, at 7:16 AM, Benjamin Peterson wrote: > 2012/11/26 smaug <sm...@welho.com>: >> I don't see anyone proposing to remove compartments, or anything like that. >> I'm just hoping to have an API which isn't super error prone. > > Do you have specific suggestions or at least any particular pain points?
I talked with Bobby Holley about this on IRC. There is a specific pain point. (Apologies to bholley if I botch any of this; it was a rushed conversation almost a week ago now.) When an X-ray wrapper is used to access an object in another compartment, we run the C++ code without switching to the object's compartment. So whenever a C++ XPCOM method uses the JSAPI, it has to switch compartments, just in case it's being called from chrome. If it doesn't, the code totally works fine, until it is called via an x-ray wrapper, and then we have a horrible crashing bug. This is indeed error-prone. Bug 751926 seeks to fix it. bholley thinks the nub of the problem is that C++ code shouldn't use the JSAPI directly; instead it should use C++ helper classes that automatically switch compartments as needed. I disagree. This strikes me as sweeping the problem under the rug. The root of all evil here is the x-ray wrapper behavior. An object's methods should never be called in a state where GetCurrentJSContext() returns a cx that's in some other compartment. My proposal: Part 1 - Fix bug 751926 exactly as bz originally suggested, over bholley's objections in comment 3. Make x-ray wrappers always change compartments to the wrapped object's compartment before calling its methods. (We will have to use a hack or two to keep GetSubjectPrincipal working as it currently does, since it currently relies on js::GetContextCompartment. This doesn't sound bad to me. The status quo is much worse.) Part 2 - Separately, cope better with compartment errors when they happen on an end user's machine. In debug builds, this sort of mistake should continue to assert fatally. In release builds, we should check compartments, and when we detect a mismatch: - throw a JS InternalError exception, if the API is fallible (they mostly are) - crash hard, if the API is infallible The goal here is "sanity in depth" - keep the behavior well-defined even in the face of programmer errors. I'm very receptive to complaints about usability. Compartments have made us all suffer. But I think the quickest path to correct code is to enforce the rules better and eliminate special exceptions, not to abolish the rules altogether! -j _______________________________________________ dev-tech-js-engine-internals mailing list dev-tech-js-engine-internals@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-tech-js-engine-internals
