Open Bug 751926 Opened 13 years ago Updated 7 months ago

Move handling of different compartments mostly out of bindings and into XrayWrapper

Categories

(Core :: DOM: Core & HTML, defect, P5)

Product:
Core
Component:
DOM: Core & HTML
Platform:
x86
macOS
Type:
defect

Tracking

()

People

(Reporter: bzbarsky, Unassigned)

References

(Blocks 1 open bug)

Details

(Keywords: perf)

Right now, every time we return a JSObject from the DOM bindings, we have to either call JS_WrapObject or at least compare the object's compartment to the current compartment and if they differ call JS_WrapObject. There are two situations that necessitate this: 1) XrayWrapper. Since this invokes the DOM methods in the chrome compartment, pretty much everything coming out of the DOM needs to be wrapped in this situation. 2) Methods/properties that can actually return cross-window objects (.contentDocument and .contentWindow on iframes, .top and .parent on Window; that might be about it... though window.* might need it for script running in a no-longer-current inner). I think what we should do is make XrayWrapper handle case 1 above, add a special annotation for methods that fall into case 2, and generate bindings that assume (and assert) that everything is same-compartment in all other cases. The one drawback is that we have to be sure to identify all the things that fall into case #2. But with CPG, pretty much any test that exercises the functionality would trigger the asserts I suggest, unless the relevant properties only _sometimes_ (and in fact rarely) like the window.* thing I mention above.
adoptNode and importNode and perhaps insertBefore/appendChild might be on the list of "can return things in a different compartment". We'd need to check.
Keywords: perf
Note that even if we do the above there are still issues with trying to use the passed-in cx to create objects. They can end up in the wrong compartment, leading to compartment mismatch hell.
Boris and I discussed this over skype for a while yesterday. Boris' major concern was object creation and storing is too easy to get wrong, because it requires the implementor/reviewer to realize that cx might not be in the same compartment as the wrapper. In particular: (1) objects created with JS_NewFoo would end up in the caller's compartment, leading to compartment mismatches or wrapper slowness down the line. (2) heap-storing JS objects passed as arguments is error prone, since they might be used later in conjunction with the wrapper and thus need to be wrapped. Boris' strawman proposal was to put _two_ compartments on the JSContext, one for security decisions and one for everything else. JSAutoCompartment would set both, so they'd almost always be in sync, _except_ for the Xray case. This would allow us to inherit the security characteristics of the compartment of the caller, but still manipulate objects in the compartment of the callee. I'm somewhat resistant to this proposal, in particular because it significantly complicates our our security model and makes it harder to grok (note that an inscrutable security model understood only by XPConnect gurus has been a major pain point in the past). Furthermore, it weakens our invariants, because code that has and manipulates the wrapper no longer necessarily has same security relationship with the wrappee as the one described by the wrapper's handler.
Note that object creation / storing isn't the common case, but is common enough to be an issue. The big problem here is that we often don't realize we got it wrong until somebody (most likely an extension) invokes the relevant DOM operation via an XrayWrapper from chrome, leading to compartment mismatches and such. Given that, my counter-proposal is to build debug-only checks into the system that allows us to catch these errors at development time. Note that this still places the burned of worrying about compartments on anyone creating / storing JS objects, but IMO that's ok, as long as they know when the get it wrong. For (1), we can have a debug-only bit on the runtime that forbids object allocation of new objects (making us assert when we do). JSAutoCompartment would clear the bit, meaning that someone would have to explicitly enter a compartment before creating new typed arrays etc. This should catch those kinds of errors. As for (2): we already need a special container for JS Object heap pointers given GGC and such. As such, we can just have a special kind of DOM JS heap pointer type that does the relevant checking to make sure that an explicit compartment has been entered (and the value has been appropriately wrapped) post-binding-entry. The underbelly of the implementation here might get a little nasty, but it'll be debug-only code, and should catch developers from making these kinds of mistakes. Thoughts, Boris?
Sorry for the lag; I wanted to make sure I had a chance to think this through. I can maybe live with the proposal for (1). It would require us to add a bunch of compartment-entering to binding code that we don't have to have right now (e.g. we'd have to enter compartments for every single string return value, which we definitely don't have to do at the moment. Or I guess we could have the bindings unset the debug-only bit or something. JSAutoCompartment would need to restore the bit to the original state (in debug builds) in its destructor, right? We would also need to deal with nested invocations through XrayWrapper and whatnot somehow. But yes, if we can make that sort of thing work, such that simply doing the wrong thing and actually exercising it in test code would fail in a debug build that would be sufficient for #1. For (2), there is no clarity from the JS people on what the heck heap pointers will need to look like with GGC. Last I asked, I was told to just use raw object pointers. I suppose we can require that everyone in the DOM uses some magic gcthing container that does the right asserts. Olli, Peter, thoughts?
Adding some kind of C++ wrapper around raw JSObject* member variables would be nice. That way we could also guarantee that the object is unmarkgray'ed when needed. Handling the nsWrapperCache's mWrapperPtrBits might be a tiny bit tricky, since it packs JSObject* and flags to uintptr_t.
Wrapper cache is a special case that we already handle in all the relevant ways. In particular, no one is manually storing random argument-provided JSObjects in nsWrapperCache. This would be for things like page-provided callbacks and whatnot...
(In reply to Boris Zbarsky (:bz) from comment #5) > I can maybe live with the proposal for (1). It would require us to add a > bunch of compartment-entering to binding code that we don't have to have > right now (e.g. we'd have to enter compartments for every single string > return value, which we definitely don't have to do at the moment. Or I > guess we could have the bindings unset the debug-only bit or something. Yeah, I'm totally down with special-casing that stuff. Because this is debug-only code, it doesn't matter if the semantics are a bit wonky, as long as it helps us catch the errors we're trying to catch. > JSAutoCompartment would need to restore the bit to the original state (in > debug builds) in its destructor, right? We would also need to deal with > nested invocations through XrayWrapper and whatnot somehow. Yeah. I think the old value can just live on the JSAutoCompartment and we can restore it.
> Yeah. I think the old value can just live on the JSAutoCompartment and we can restore it. That may not be enough for nested Xray invocations, fwiw.
Maybe I'm missing something, but in the #2 case, once window.top and friends are accessors, backed by JS functions, you shouldn't need to do anything special. The accessors should return same-compartment pointers, then the JS engine will see if anything cross-compartment happened and wrap accordingly. Why isn't that the case?
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #10) > Maybe I'm missing something, but in the #2 case, once window.top and friends > are accessors, backed by JS functions, you shouldn't need to do anything > special. The accessors should return same-compartment pointers, then the JS > engine will see if anything cross-compartment happened and wrap accordingly. > Why isn't that the case? Because the JS engine won't automatically rewrap, it will assert. It assumes that if you actually want to return a cross-compartment object that you will explicitly do so by calling JS_WrapObject (of course, once you're in cross-compartment land, the cross-compartment wrappers will happily do the rewrapping for you).
> The accessors should return same-compartment pointers Yes. The discussion is about HOW exactly they should go about doing that. Keep in mind that these accessors are JSNatives and that the JS engine blindly trusts JS natives to do the right thing.
I've thought about this some more, and I think our chances of making sure that if C++ object X has a pointer to C++ object Y as a member then they're same-compartment, with a few exceptions, are zero. Specifically, at least the following cases are definitely SOL as things are right now: 1) Any static nodelist, because its elements can be adopted while it's holding on to them. 2) Any dynamic nodelist rooted at a node that gets adopted. 3) DOMSVGPointList, because you can insert DOMSVGPoints that were created in a different window. 4) Anything hanging off a node that we forget to reparent when the element is adopted. I bet there are many more. So what I think we should do instead is forget about trying to do this in general and just explicitly annotate cases in which the performance of the same-compartment check actually matters _and_ we can guarantee that there won't be compartment mismatches in IDL or something. :(
(In reply to Boris Zbarsky (:bz) from comment #13) > I've thought about this some more, and I think our chances of making sure > that if C++ object X has a pointer to C++ object Y as a member then they're > same-compartment, with a few exceptions, are zero. Same-compartment in what sense? In those examples the objects will remain same-compartment in the strict sense - they'll be transformed into proxies that are cross-compartment wrappers for some other object. So performance may be affected, but correctness is not. And IIUC the majority of the pain this bug is dealing with is correctness.
> Same-compartment in what sense? In the sense the C++ code cares about: not having to JS_WrapObject before every single thing you do. > they'll be transformed into proxies Only if we make sure to keep calling JS_WrapObject all over the place. > And IIUC the majority of the pain this bug is dealing with is correctness. No, the majority of the pain I care about is that doing things correctly right now means your performance is either completely abysmal or just kinda sucky depending on how much effort you put into it.
(In reply to Boris Zbarsky (:bz) from comment #15) > > Same-compartment in what sense? > > In the sense the C++ code cares about: not having to JS_WrapObject before > every single thing you do. > > > they'll be transformed into proxies > > Only if we make sure to keep calling JS_WrapObject all over the place. The proxy transformation happens during transplant, with the swap() call. When, say, a nodelist's element is adopted into another compartment, the relevant object just turns into a cross-compartment wrapper with no manual intervention. You know this, though, so I'm guessing I'm not properly understanding your concerns in comment 13.
> The proxy transformation happens during transplant Let's take my simple example from comment 13 item 1: static nodelist. This is basically an array of Element*. If one of those elements is adopted, we will reparent the wrapper and store the new wrapper, not a proxy of any sort in its wrapper cache. After this if someone indexes into the list, esp code running in the compartment of the list, we're in a situation where cx and the lists' GetWrapper() are in one compartment, but GetWrapper() on the element is in another compartment. So the [i] operation on the list has to ensure that it calls JS_WrapObject as needed, to handle this situation.
Ahah! Yeah, I forgot that the mechanism for getting the wrapper here would be the wrapper cache (which we update) rather than through any heap-stored JSObject pointers. Yeah, this does seem to be a pretty bad problem. I'm surprised it hasn't bitten us more. :-(
Thanks to Jesse, today I encountered the concept of setting the __proto__ of a node to a DOM prototype from a different global, at which point all the DOM methods will be invoked in the compartment of that proto (since that's where they live, yay compartment-per-global) and the "obj" passed in will be a cross-compartment wrapper for the node. At which point we HAVE to deal with compartments in the binding code, of course. So there's just no way to do this bug as filed, sadly, since the issue is not xray-specific. We _may_ be able to get around that by moving the same-compartment check up into the genericGetter/Method and skipping it during wrapping in the specialized method if the IDL member is explicitly annotated as allowed to skip the check. That would impose some additional overhead on the generic methods, unfortunately, and still require explicit annotation in IDL, but I think it should work, because any sort of non-same-compartment cases would have a wrapper object for the node and hence would have to go through the generic methods, right?
(In reply to Boris Zbarsky (:bz) from comment #19) > Thanks to Jesse, today I encountered the concept of setting the __proto__ of > a node to a DOM prototype from a different global, at which point all the > DOM methods will be invoked in the compartment of that proto (since that's > where they live, yay compartment-per-global) and the "obj" passed in will be > a cross-compartment wrapper for the node. At which point we HAVE to deal > with compartments in the binding code, of course. Well, I don't think we have to support mutable proto per se - for that case I think it'd be fine to just throw when we discover that the object isn't a DOM node (since it's a cross-compartment wrapper). But in general I think we should support same-origin call/apply, which means that we should support CCWs (that aren't security wrappers). > We _may_ be able to get around that by moving the same-compartment check up > into the genericGetter/Method and skipping it during wrapping in the > specialized method if the IDL member is explicitly annotated as allowed to > skip the check. That would impose some additional overhead on the generic > methods, unfortunately, and still require explicit annotation in IDL, but I > think it should work, because any sort of non-same-compartment cases would > have a wrapper object for the node and hence would have to go through the > generic methods, right? I'm not familiar with the genericGetter/Method stuff. Can you explain?
> Well, I don't think we have to support mutable proto per se Web sites depend on it last we checked, fwiw. But yes, same-origin but cross-window call/apply would have the same exact issue. > I'm not familiar with the genericGetter/Method stuff. Can you explain? Each of our binding methods/getters/setters has two parts to it. Right now, the first part extracts the underlying C++ object from the JSObject* "this" value and then calls the second part. Since this first part is identical for all the methods on a given interface, we actually have only one C++ function for it. This C++ function unwraps the "this" object to a C++ object, gets the function pointer to the second part from a reserved slot on the JSFunction* callee, and calls that function pointer. The second part of the method is what does all the work other than this-unwrapping. The reason for the split is that when IonMonkey can prove via type inference that the objects it's working with are all actual DOM objects (and not various wrappers for them, etc), it will inline an equivalent of the first part that only works on real DOM objects, that is to say a single slot read, into the jitcode and directly call the second part. This speeds things up in cases when everything involved is actual DOM objects, which is the common case. So the point is that we should be going through the genericGetter/Method/Setter stuff for any sort of "weird" case. And hence it might make sense to hoist some weirdness-handling into those...
https://bugzilla.mozilla.org/show_bug.cgi?id=1472046 Move all DOM bugs that haven’t been updated in more than 3 years and has no one currently assigned to P5. If you have questions, please contact :mdaly.
Priority: -- → P5
Component: DOM → DOM: Core & HTML
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.