Last Comment Bug 758415 - Implement per-origin Xray expando sharing for Wrapped Natives
: Implement per-origin Xray expando sharing for Wrapped Natives
Status: RESOLVED FIXED
: addon-compat
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla16
Assigned To: Bobby Holley (:bholley) (busy with Stylo)
:
Mentors:
: 757639 766718 (view as bug list)
Depends on: 761121
Blocks: 752764 757639 760118 761695 775983
  Show dependency treegraph
 
Reported: 2012-05-24 15:24 PDT by Bobby Holley (:bholley) (busy with Stylo)
Modified: 2012-07-20 08:23 PDT (History)
24 users (show)
bobbyholley: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed


Attachments
Part 1 - Refactor slim wrapper reserved slots so that we can use the same slot for expando objects in the non-slim case. v1 (8.47 KB, patch)
2012-06-04 09:19 PDT, Bobby Holley (:bholley) (busy with Stylo)
mrbkap: review+
Details | Diff | Splinter Review
Part 2 - Implement expando object infrastructure for WN Xrays. v1 (8.07 KB, patch)
2012-06-04 09:19 PDT, Bobby Holley (:bholley) (busy with Stylo)
mrbkap: review+
Details | Diff | Splinter Review
Part 3 - Copy expando objects during object transplanting. v1 (4.39 KB, patch)
2012-06-04 09:20 PDT, Bobby Holley (:bholley) (busy with Stylo)
mrbkap: review+
Details | Diff | Splinter Review
Part 4 - Expose AutoIdVector wrapping. v1 (1.66 KB, patch)
2012-06-04 09:20 PDT, Bobby Holley (:bholley) (busy with Stylo)
mrbkap: review+
Details | Diff | Splinter Review
Part 5 - Switch WN Xrays to use the new expando infrastructure. v1 (4.71 KB, patch)
2012-06-04 09:20 PDT, Bobby Holley (:bholley) (busy with Stylo)
mrbkap: review+
Details | Diff | Splinter Review
Part 6 - Rip out old expando architecture. v1 (14.44 KB, patch)
2012-06-04 09:21 PDT, Bobby Holley (:bholley) (busy with Stylo)
mrbkap: review+
Details | Diff | Splinter Review
Part 7 - Remove double-wrapping infrastructure for Location objects. v1 (7.65 KB, patch)
2012-06-04 09:21 PDT, Bobby Holley (:bholley) (busy with Stylo)
mrbkap: review+
Details | Diff | Splinter Review
Part 8 - Tests. v1 (9.73 KB, patch)
2012-06-04 09:21 PDT, Bobby Holley (:bholley) (busy with Stylo)
mrbkap: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Bobby Holley (:bholley) (busy with Stylo) 2012-05-24 15:24:54 PDT
This seems to be the only solution to bug 757639 and bug 752764. But boy is it going to be some work. And it needs to happen this cycle.

When an object is accessed via an Xray wrapper in non-transparent mode, expando properties get defined on the Xray wrapper itself. This becomes a noticeable problem with CPG, because same-origin scripts don't necessarily see the same expando objects like they expect to. We solved this with a crappy workaround for Location objects in bug 739796, but that strategy has become untenable. We need something more robust.

We need a way to lookup properties, define properties, and trace a per-origin expando object. So we need either 1 runtime-wide map with two keys (viewer origin, target object) or 1 map per origin.

The trickiest question is where the expando objects should live. Here are the options I can think of:
1 - They live in the first compartment created for an origin (or rather, whatever compartment they're first lazily created for). They hold that compartment alive as long as there are any other compartments alive for the given origin.
2 - Same as #1, but the expando objects migrate around to allow compartments to die.
3 - They live in a separate per-origin compartment. This is clean, but potentially memory-intensive. Maybe we could do something special to those compartments to make them cheaper?
4 - They live in a dedicated system-principal compartment.
5 - They live in a dedicated null-principal compartment.

Options 4 and 5 eschew a lot of compartment gymnastics, but put lots of cross-origin information in the same compartment. If we're manually curating these objects, maybe the chance that this could leak information is pretty low. Option 5 definitely seems better than option 4, but I'm not sure if it would break something somehow.

Thoughts?
Comment 1 Boris Zbarsky [:bz] 2012-05-24 18:21:35 PDT
Does it make any sense to put the expando in the compartment of the wrapee?
Comment 2 Bill McCloskey (:billm) 2012-05-24 18:24:29 PDT
I don't know this stuff at all, but I had an idea for an alternate solution.

Currently, each system compartment could have its own Xray wrapper for a given content object. When one system compartment creates an expando on its Xray wrapper, we want that property to be visible to the other system compartments. What if we just did the dumbest possible thing and created the property on all the Xray wrappers?

That is, any operation that would normally change (create/update/delete) an expando property on one of these wrappers would have a special hook. The hook would iterate over all system compartments, check if the compartment has a wrapper for the given target object, and do the update to that wrapper. This wouldn't be fast, but I'm guessing we don't care so much about speed here.

The thing I like about this is that it doesn't require any non-local reasoning. We aren't breaking any of our existing invariants or doing anything weird.
Comment 3 Boris Zbarsky [:bz] 2012-05-24 18:38:10 PDT
We don't care about squeezing out every bit of speed, but we do care if we get noticeeable UI lag from this stuff.  How long will walking over several hundred compartments as you describe take?
Comment 4 Bobby Holley (:bholley) (busy with Stylo) 2012-05-25 03:36:21 PDT
Bill's solution is good in that it punishes the less common cases (expandos on location objects, expandos on cross-origin objects, and chrome expandos on content). But I'd like to avoid it if we can think of something more performant.

(In reply to Boris Zbarsky (:bz) from comment #1)
> Does it make any sense to put the expando in the compartment of the wrapee?

Hm, that simplifies lifetime management. It's a bit scary, but maybe it's ok if we're only accessing these things from special handling code (i.e., they're never exposed to script). Offhand, I can't think of any security issues.

So we'd probably want to hang an optional object off the wrappee (lazily allocated once an expando is placed). is this feasible somehow with JSAPI? Defining properties is dynamic, but exposes them to script. Reserved slots seems like what we want, but I don't see any way in JSAPI to make an object grow a reserved slot. Is there one? If so, GC stuff would be much-simplified.

The object itself would be an index of expando objects for different origins. It would be nice if we could just make the object a dictionary keyed off of origin, but that would only work if there were a canonical representation of an origin, which I'm not sure there is (since the CheckSameOrigin code has lots of special cases). Alternatively, we could just assume that an object is unlikely to have expandos placed on it from lots of different origins, and just do a dumb linked list via reserved slots. When we want to find our expando object, we grab the underlying object, examine the appropriate slot, and traverse the linked list until we find something that compares same-origin or until we hit the end. This seems like a saner approach to me.

So the only real roadblock (modulo security concerns I haven't thought of) is that I don't know how to make a JS object grow reserved slots. Is there such a thing as an unreserved slot?
Comment 5 Boris Zbarsky [:bz] 2012-05-25 07:03:58 PDT
There is no way to add a reserved slot, but you could just add a reserved slot to all our xpconnect and new-binding objects...
Comment 6 Bobby Holley (:bholley) (busy with Stylo) 2012-05-25 08:37:50 PDT
(In reply to Boris Zbarsky (:bz) from comment #5)
> There is no way to add a reserved slot, but you could just add a reserved
> slot to all our xpconnect and new-binding objects...

Is that significant memory overhead? Or is it negligible?
Comment 7 Boris Zbarsky [:bz] 2012-05-25 09:11:15 PDT
It's 8 bytes per object.  I _think_ that's ok, personally.  For example, XPCWrappedNative is right now 11 words (44 and 88 bytes on 32-bit and 64-bit respectively), not even counting the JSObject parts....

Another option, of course, is some sort of hashtable on the compartment, indexed by wrappee or something.
Comment 8 Bobby Holley (:bholley) (busy with Stylo) 2012-05-25 09:18:03 PDT
(In reply to Boris Zbarsky (:bz) from comment #7)
> It's 8 bytes per object.  I _think_ that's ok, personally.  For example,
> XPCWrappedNative is right now 11 words (44 and 88 bytes on 32-bit and 64-bit
> respectively), not even counting the JSObject parts....

Yeah, but the DOM objects are largely slim wrappers, right? Anyway, if you're ok with it, then I am.

> Another option, of course, is some sort of hashtable on the compartment,
> indexed by wrappee or something.

Yeah. I really like the reserved slot solution though, because it means that tracing happens automatically.
Comment 9 Boris Zbarsky [:bz] 2012-05-25 09:22:13 PDT
> Yeah, but the DOM objects are largely slim wrappers, right?

Could be.  And in new bindings they certainly are, and then they're only about 40 bytes in size.

I still think that the one extra slot is probably ok, but ccing some other people who might have opinions.
Comment 10 Bill McCloskey (:billm) 2012-05-25 10:20:02 PDT
(In reply to Boris Zbarsky (:bz) from comment #3)
> We don't care about squeezing out every bit of speed, but we do care if we
> get noticeeable UI lag from this stuff.  How long will walking over several
> hundred compartments as you describe take?

I guess it would depend on how many of the system compartments have wrappers for the object being updated. If very few of them do, then you're just iterating and doing a hashtable lookup for each one. So I would guess that the entire property operation would get at most a few times slower in this case. But if we actually have to update the property in 200 system compartments, then the slowdown would probably be ~200x. Even that wouldn't slow down the UI unless you were doing a lot of them, though.

Sticking the expando object in the content compartment seems okay, too, as long as we wrap all the properties on it.
Comment 11 Bobby Holley (:bholley) (busy with Stylo) 2012-05-29 05:49:39 PDT
CCing Mano and Mak, so that they know we're working on this.
Comment 12 Bobby Holley (:bholley) (busy with Stylo) 2012-05-29 11:05:30 PDT
One issue I've been considering here is how we avoid leaking the consumer compartment (that is to say, the one placing the expandos).

When the expandos are placed, we'll allocate the hidden object in the compartment of the target / wrappee, and attach the relevant expandos. Modulo primitives, these will generally be cross-compartment wrappers back to the consumer's compartment. This is mostly good, since when the target goes away the expando objects naturally get collected as well. But what happens if the target should outlive the consumer?

This is kind of an inherent problem, because the expandos are supposed to be shared per-origin. So taking the example of system compartments (the primary consumer here), we could have a compartment that wants to go away, but whose expandos also need to be available to the ~100 other system compartments.

Here are a couple of things we could do easily:
1 - When an xray wrapper for a compartment goes away, remove all properties on the corresponding expando object that point back to that compartment.
2 - Reference-count the expando object. When all Xray wrappers to it go away, throw it away and let it be GCed.

Unfortunately, both of these apply pretty weird and confusing lifetime semantics that I don't think are acceptable. If a consumer pulls an ephemeral Xray wrapper off a DOM tree, places an expando on it, and allows that Xray wrapper to go away, the expando would be lost. This would be a regression from the current state of affairs. Currently, we store expando objects in a map in the consumer compartment private that traces (and cycle-collects) them.

What we probably want is to somehow nuke all the cross-compartment wrappers back into the consumer compartments when those compartments want to die. Knowing when a compartment "wants" to die can be tricky though. We could probably use khuey's Nuker in some cases, but it has limitations. It can't be used for content->content, and it currently only knows when windows want to die. Things like sandboxes are another story. Hm.
Comment 13 Bobby Holley (:bholley) (busy with Stylo) 2012-05-30 09:41:00 PDT
So, I've been looking into the current situation with lifetime management, and now think that maybe the new world is more or less equivalent to the old one.

Currently, the expando object lives in a map in the consumer's compartment private. It is traced during the call to TraceXPConnectRoots, and only goes away once the target WN is explicitly expired by the CC. The target WN is set as a preserved wrapper, so it only goes away when it's CCed, which only happens when the underlying node is removed from the DOM.

So, in effect, the expandos (and thus the consumer compartment) have a lifetime bounded below by the lifetime of the target object. This is more or less what things will look like in the new state of affairs, except that the tracing situation will be simplified. Huzzah.
Comment 14 Bobby Holley (:bholley) (busy with Stylo) 2012-05-30 10:20:26 PDT
I was working on this in the paris office today. Alex pointed out that this, under the current plan, will break jetpack, because jetpack _wants_ different sets for same-origin code. This used to be handled by using separate compartments, but now that we have compartment-per-global, that's the default.

So we probably need some kind of special identifier that we use in addition to the same-origin check. It's not yet clear to me whether this should go on the principal or on the compartment private. Principal makes more sense to me, but it might be more work.

What do you think, gabor?
Comment 15 Gabor Krizsanits [:krizsa :gabor] 2012-05-30 13:40:41 PDT
Can I haz access to bug 752764 ? This one is depending on it and I don't know what info I'm missing. Anyway first I really preferred putting one map with two keys and some sane API on the runtime. I'm still not against that one, because that is what really connects all these compartments.

About Bill's idea, unfortunately for jetpack addons, even with a few addons around and let's say 15 open tabs, reaching several hundreds of compartments can be very life like... 

By the way I don't think this should be system compartment specific thing, sandbox can use xray wrappers with any other origins as it's principal, so we can face the same problem there. 

First I didn't really like what Boris suggested, because it kind of breaks a principal for me. Those expando objects really belong to the xray wrapper that lives in the other compartment. And to me conceptually this is really weird because an object (the wrapper) and its properties (expandos) should be stored in the same compartment or if not at least not in another compartment that has less privileges and should not be able to see them. So now the original object is the only one (and its compartment) who cann't see those properties, yet it would store them (or they will kind of live in it's compartment). I clearly see the advantages of this solution, because in a way this is really practical, it just feels weird conceptually (to me). But on the other hand I see that this can work and I don't adore the one big map on the runtime idea either... (btw wouldn't it be easier this way to find a raw pointers to some object from the system conpartment by offsetting the wrappe?)

Jetpack. Personally I'm not happy that jetpack is depending on something like this. I will look into it if it is avoidable. This is a totally unintuitive behavior, that you want to fix here and we should not depend on it. On the other hand I don't want to break everything in jetpack ofc. So jetpack uses sandboxes, we can just add a new flag for this (when my patches are landed this should be even simpler than it is right now) and then just use a specific key in that map (like a pair of <origin, some_pointer_that_is_null_by_default>)
Comment 16 Bobby Holley (:bholley) (busy with Stylo) 2012-05-30 15:32:09 PDT
FWIW, I'm most of the way through hacking up Boris' solution.

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #15)
> Jetpack. Personally I'm not happy that jetpack is depending on something
> like this. I will look into it if it is avoidable. This is a totally
> unintuitive behavior, that you want to fix here and we should not depend on
> it.

Well, it actually seems pretty reasonable to me. Suppose we've got 2 addons that run content scripts. Each one runs with the principal of the page it's running against, but they're different addons, so they really shouldn't see each others' expandos.

> On the other hand I don't want to break everything in jetpack ofc. So
> jetpack uses sandboxes, we can just add a new flag for this (when my patches
> are landed this should be even simpler than it is right now) and then just
> use a specific key in that map (like a pair of <origin,
> some_pointer_that_is_null_by_default>)

What about somehow adding that extra bit of identity information into nsExpandedPrincipal itself? Is that stuff going to land before the branch, assuming I get to your most recent review tomorrow morning?
Comment 17 Gabor Krizsanits [:krizsa :gabor] 2012-05-31 01:09:21 PDT
(In reply to Bobby Holley (:bholley) from comment #16)
> Well, it actually seems pretty reasonable to me. Suppose we've got 2 addons
> that run content scripts. Each one runs with the principal of the page it's
> running against, but they're different addons, so they really shouldn't see
> each others' expandos.

Ok this makes sense, I forgot about this case...

> What about somehow adding that extra bit of identity information into
> nsExpandedPrincipal itself? Is that stuff going to land before the branch,
> assuming I get to your most recent review tomorrow morning?

If you want to store that identity information in principals, it should go into the nsBasePrincipal, since the nsExpandedPrincipal will not be used in every case. Only in some cases like when let's say the content-script wants to XHR to a specific server and modify the page. In simple cases it will just use the principal of the content.
I think the patches can be landed quickly from here on, and since it's adding a feature that can be used only from internally it should be relatively safe to land anyway, but let's discuss that after your review.
Comment 18 Alexandre Poirot [:ochameau] 2012-05-31 06:32:25 PDT
For the sake of clarity, here is a sample snippet that reproduce jetpack content scripts. We are basically expecting to get different expandos on two sandboxes using the same *content* origin:

  let Cu = Components.utils;

  let s1 = Cu.Sandbox(content);
  s1.window = content;
  Cu.evalInSandbox("window.foo = 'bar';", s1);

  let s2 = Cu.Sandbox(content);
  s2.window = content;
  
  // We are expecting to get `undefined` here:
  Cu.reportError(Cu.evalInSandbox("window.foo", s2));

If we decide that Sandbox contructor with default value will share expandos, we are going to break addons using old SDK versions that would not contains the opt-out option given to Sandbox.

Otherwise we are using sandboxes for CommonJS modules too. Here, we are using system principal, so expando are currently shared. It might be cool to keep the same behavior, but I don't think jetpack addons modules are relying on this. Actually, it may even be a good idea to stop sharing them between modules!
Comment 19 Bobby Holley (:bholley) (busy with Stylo) 2012-05-31 06:40:27 PDT
I'd also be fine with making expando sharing opt-in for sandboxes. Pre-CPG each sandbox had its own compartment, right? So that would maintain the old behavior.
Comment 20 Alexandre Poirot [:ochameau] 2012-05-31 07:00:08 PDT
(In reply to Bobby Holley (:bholley) from comment #19)
> I'd also be fine with making expando sharing opt-in for sandboxes. Pre-CPG
> each sandbox had its own compartment, right? So that would maintain the old
> behavior.

You are right. Expandos aren't shared:

  let Cu = Components.utils;
  let p = Components.classes["@mozilla.org/systemprincipal;1"].
            createInstance(Components.interfaces.nsIPrincipal);
  let s1 = Cu.Sandbox(p);

  s1.window = content;
  Cu.evalInSandbox("window.foo = 'bar';", s1);

  let s2 = Cu.Sandbox(p);
  s2.window = content;
  Cu.reportError(Cu.evalInSandbox("window.foo", s2));
Comment 21 Bobby Holley (:bholley) (busy with Stylo) 2012-05-31 11:54:05 PDT
So, I've got this working for WNs, I think. I still need to write some thorough test coverage and special-case sandboxes (which we decided get their own unique expando object in all cases). But hopefully that shouldn't take long.

It's possible that this could land before the branch if I finish it tonight (european time), it gets r+ed before  the weekend, and I land it monday morning (european time) before people wake up and the merge happens. But that might be pushing it.

I think it's also fine just to land after the merge, and backport it immediately. I know peter's been changing some stuff in XrayWrapper, so we should probably coordinate. It probably makes the most sense to land this stuff first, since presumably peter's changes won't be backported (and landing this first keeps the delta between the trunk landing and the aurora landing small). Or, more likely, maybe we don't conflict much if at all.

The bigger concern is what to do about nodelist proxies and new dom bindings. These require a refactoring, because currently we don't have separate expando objects for them (they're combined into a single holder object). This refactoring was planned (since, among other things, the current architecture makes it impossible for us to correctly implement getOwnPropertyDescriptor), but might turn out to be big (not sure yet), in which case it might not be desirable on aurora. So we should think about whether we're ok shipping without expando sharing for Nodelists and XHR. The impact is probably pretty limited. But we could also just backport the refactor and be done with it.
Comment 22 Bobby Holley (:bholley) (busy with Stylo) 2012-06-01 07:28:54 PDT
I've got patches for this that seem to work (for WNs only - nodelists newbindings not included). They pass all the xpconnect tests, including the ones I wrote specifically for this subject.

Pushing to try: https://tbpl.mozilla.org/?tree=Try&rev=6c52b9afe0bb
Comment 23 Bobby Holley (:bholley) (busy with Stylo) 2012-06-04 05:52:21 PDT
Added a patch for bug 760118 and pushed to try again:
https://tbpl.mozilla.org/?tree=Try&rev=9253c698dff7
Comment 24 Bobby Holley (:bholley) (busy with Stylo) 2012-06-04 05:52:44 PDT
(In reply to Bobby Holley (:bholley) from comment #23)
> Added a patch for bug 760118

Err, bug 761121.
Comment 25 Bobby Holley (:bholley) (busy with Stylo) 2012-06-04 09:19:34 PDT
Created attachment 629800 [details] [diff] [review]
Part 1 - Refactor slim wrapper reserved slots so that we can use the same slot for expando objects in the non-slim case. v1

Xray wrappers require that their wrappee be non-slim, so this works out perfectly.
Comment 26 Bobby Holley (:bholley) (busy with Stylo) 2012-06-04 09:19:52 PDT
Created attachment 629801 [details] [diff] [review]
Part 2 - Implement expando object infrastructure for WN Xrays. v1

Note: This overloads the naming of some of the existing infrastructure,
but the signatures etc are sufficient to disambiguate. The other infrastructure
goes away in a subsequent patch.

Note: We tag sandbox expandos with their global to make sure that the expandos
are never shared between sandboxes. A consequence of this scheme is that an
expando from a sandbox to an object will _always_ result in a GC edge back to
the sandbox, meaning that the sandbox is always kept alive for the lifetime of
the expando target. This could happen before, but only if a non-primitive expando
was placed (since the value of the expando would live in the consumer's
compartment). We could avoid this edge by using a reference-counted Identity()
object instead, but I suspect it's not worth worrying about.
Comment 27 Bobby Holley (:bholley) (busy with Stylo) 2012-06-04 09:20:10 PDT
Created attachment 629802 [details] [diff] [review]
Part 3 - Copy expando objects during object transplanting. v1
Comment 28 Bobby Holley (:bholley) (busy with Stylo) 2012-06-04 09:20:26 PDT
Created attachment 629803 [details] [diff] [review]
Part 4 - Expose AutoIdVector wrapping. v1
Comment 29 Bobby Holley (:bholley) (busy with Stylo) 2012-06-04 09:20:43 PDT
Created attachment 629804 [details] [diff] [review]
Part 5 - Switch WN Xrays to use the new expando infrastructure. v1
Comment 30 Bobby Holley (:bholley) (busy with Stylo) 2012-06-04 09:21:00 PDT
Created attachment 629805 [details] [diff] [review]
Part 6 - Rip out old expando architecture. v1
Comment 31 Bobby Holley (:bholley) (busy with Stylo) 2012-06-04 09:21:32 PDT
Created attachment 629806 [details] [diff] [review]
Part 7 - Remove double-wrapping infrastructure for Location objects. v1

This is more or less just a backout of bug 739796, that caused so much pain. Huzzah!
Comment 32 Bobby Holley (:bholley) (busy with Stylo) 2012-06-04 09:21:48 PDT
Created attachment 629808 [details] [diff] [review]
Part 8 - Tests. v1
Comment 33 Bobby Holley (:bholley) (busy with Stylo) 2012-06-04 09:23:44 PDT
This is green, so patches attached.

Blake, this is a pretty high-priority item, FWIW.
Comment 34 :Ms2ger (⌚ UTC+1/+2) 2012-06-04 09:34:34 PDT
Comment on attachment 629800 [details] [diff] [review]
Part 1 - Refactor slim wrapper reserved slots so that we can use the same slot for expando objects in the non-slim case. v1

Review of attachment 629800 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/src/xpcprivate.h
@@ +312,5 @@
> +
> +inline XPCWrappedNativeProto* GetSlimWrapperProto(JSObject *obj)
> +{
> +    MOZ_ASSERT(IS_SLIM_WRAPPER(obj));
> +    const js::Value &v = js::GetReservedSlot(obj, WRAPPER_MULTISLOT);

JS::Value

@@ +324,5 @@
> +{
> +    MOZ_ASSERT(IS_SLIM_WRAPPER(obj));
> +    JS_SetReservedSlot(obj, WRAPPER_MULTISLOT, JSVAL_NULL);
> +    MOZ_ASSERT(!IS_SLIM_WRAPPER(obj));
> +}

Assert that arguments are non-null in all those?
Comment 35 :Ms2ger (⌚ UTC+1/+2) 2012-06-04 09:42:47 PDT
Comment on attachment 629801 [details] [diff] [review]
Part 2 - Implement expando object infrastructure for WN Xrays. v1

Review of attachment 629801 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/src/xpcprivate.h
@@ +330,5 @@
> +inline void SetExpandoChain(JSObject *obj, JSObject *chain)
> +{
> +    MOZ_ASSERT(IS_WN_WRAPPER(obj));
> +    JS_SetReservedSlot(obj, WRAPPER_MULTISLOT, chain ? JS::ObjectValue(*chain)
> +                                                     : JSVAL_NULL);

JS::ObjectOrNullValue(chain)

::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +177,5 @@
> +    if (!chain) {
> +        XPCWrappedNative *wn =
> +          static_cast<XPCWrappedNative *>(xpc_GetJSPrivate(target));
> +        nsRefPtr<nsXPCClassInfo> ci;
> +        CallQueryInterface(wn->Native(), getter_AddRefs(ci));

Can this use do_QI?

@@ +199,5 @@
> +        // anyone else, so we tag it with the sandbox global.
> +        JSObject *consumerGlobal = js::GetGlobalForObjectCrossCompartment(wrapper);
> +        bool isSandbox = !strcmp(js::GetObjectJSClass(consumerGlobal)->name, "Sandbox");
> +        expandoObject = AttachExpandoObject(cx, target, ObjectPrincipal(wrapper),
> +                                            isSandbox ? consumerGlobal : nsnull);

A helper for

JSObject *consumerGlobal = js::GetGlobalForObjectCrossCompartment(obj);
bool isSandbox = !strcmp(js::GetObjectJSClass(consumerGlobal)->name, "Sandbox");
return isSandbox ? consumerGlobal : NULL;

?
Comment 36 :Ms2ger (⌚ UTC+1/+2) 2012-06-04 09:45:34 PDT
Comment on attachment 629802 [details] [diff] [review]
Part 3 - Copy expando objects during object transplanting. v1

Review of attachment 629802 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +207,5 @@
>  
> +bool
> +CloneExpandoChain(JSContext *cx, JSObject *dst, JSObject *src)
> +{
> +    MOZ_ASSERT(js::GetContextCompartment(cx) == js::GetObjectCompartment(dst));

js::IsObjectInContextCompartment?
Comment 37 :Ms2ger (⌚ UTC+1/+2) 2012-06-04 09:46:23 PDT
Comment on attachment 629803 [details] [diff] [review]
Part 4 - Expose AutoIdVector wrapping. v1

Review of attachment 629803 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsfriendapi.cpp
@@ +167,5 @@
>      return cx->compartment->wrap(cx, desc);
>  }
>  
> +JS_FRIEND_API(JSBool)
> +JS_WrapAutoIdVector(JSContext *cx, js::AutoIdVector &props)

JS::AutoIdVector
Comment 38 Blake Kaplan (:mrbkap) 2012-06-04 13:27:08 PDT
Comment on attachment 629800 [details] [diff] [review]
Part 1 - Refactor slim wrapper reserved slots so that we can use the same slot for expando objects in the non-slim case. v1

Review of attachment 629800 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/src/xpcpublic.h
@@ +80,5 @@
> +
> +// NB: This slot isn't actually reserved for us on globals, because SpiderMonkey
> +// uses the first N slots on globals internally. The fact that we use it for
> +// wrapped global objects is totally broken. But due to a happy coincidence, the
> +// JS engine never uses that slot. This still needs fixing though. See bug 760095.

Is this true? I thought GLOBAL_FLAGS_WITH_SLOTS saved us here (note that we use it in XPCONNECT_GLOBAL_FLAGS).
Comment 39 Blake Kaplan (:mrbkap) 2012-06-04 15:19:11 PDT
Comment on attachment 629802 [details] [diff] [review]
Part 3 - Copy expando objects during object transplanting. v1

Review of attachment 629802 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/src/XPCWrappedNative.cpp
@@ +1622,5 @@
>                  return NS_ERROR_OUT_OF_MEMORY;
>  
> +            // Expandos from other compartments are attached to the target JS object.
> +            // Copy them over, and let the old ones die a natural death.
> +            SetExpandoChain(newobj, nsnull);

This mirrors the SetReservedSlot in XPCWrappedNative::FinishInit, right?
Comment 40 Blake Kaplan (:mrbkap) 2012-06-04 15:27:06 PDT
Comment on attachment 629804 [details] [diff] [review]
Part 5 - Switch WN Xrays to use the new expando infrastructure. v1

Review of attachment 629804 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +844,5 @@
> +    // in the target compartment.
> +    if (expando) {
> +        JSAutoEnterCompartment ac;
> +        if (!ac.enter(cx, expando) ||
> +            !JS_GetPropertyDescriptorById(cx, expando, id, flags, desc))

Braces around the if body, please. I think the engine's style guide says that the brace actually goes under the 'if'.

@@ +963,5 @@
> +    if (expando) {
> +        JSAutoEnterCompartment ac;
> +        if (!ac.enter(cx, expando) ||
> +            !js::GetPropertyNames(cx, expando, flags, &props))
> +        return false;

Braces and indentation here.
Comment 41 Blake Kaplan (:mrbkap) 2012-06-04 15:33:56 PDT
Comment on attachment 629806 [details] [diff] [review]
Part 7 - Remove double-wrapping infrastructure for Location objects. v1

Review of attachment 629806 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/wrappers/WrapperFactory.cpp
@@ +383,1 @@
>          // * The object is a location object.

One of these lines is redundant.
Comment 42 Blake Kaplan (:mrbkap) 2012-06-04 15:34:45 PDT
Comment on attachment 629808 [details] [diff] [review]
Part 8 - Tests. v1

Review of attachment 629808 [details] [diff] [review]:
-----------------------------------------------------------------

rs=mrbkap
Comment 43 Bobby Holley (:bholley) (busy with Stylo) 2012-06-05 02:28:38 PDT
(In reply to Blake Kaplan (:mrbkap) from comment #38)
> Is this true? I thought GLOBAL_FLAGS_WITH_SLOTS saved us here (note that we
> use it in XPCONNECT_GLOBAL_FLAGS).

Under the current system, the global slots of the JS engine are supposed to come first, then the user slots. This will ideally change in bug 760095.
Comment 44 Bobby Holley (:bholley) (busy with Stylo) 2012-06-05 03:38:52 PDT
(In reply to Blake Kaplan (:mrbkap) from comment #39)
> > +            SetExpandoChain(newobj, nsnull);
> 
> This mirrors the SetReservedSlot in XPCWrappedNative::FinishInit, right?

Not sure I follow. |newobj| comes from JS_CloneObject, which only clones reserved slots for proxies (I wasn't sure why this was the case). This means that its reserved slots end up void (I think), and I wanted to maintain the invariant that the expando slot was never undefined (either null/JSObject for WNs, or double for slim wrappers). FinishInit is never called on transplanted objects AFAICT.
Comment 45 Bobby Holley (:bholley) (busy with Stylo) 2012-06-05 03:55:14 PDT
All feedback addressed. I'm just waiting for the stuff in bug 761121 to be resolved and then this can land.
Comment 46 Bobby Holley (:bholley) (busy with Stylo) 2012-06-05 08:02:49 PDT
Alright, all ready to go. One last try push: https://tbpl.mozilla.org/?tree=Try&rev=59ca5bf73d4c
Comment 48 Bobby Holley (:bholley) (busy with Stylo) 2012-06-05 10:12:56 PDT
*** Bug 757639 has been marked as a duplicate of this bug. ***
Comment 49 Bobby Holley (:bholley) (busy with Stylo) 2012-06-05 10:14:55 PDT
We should set the tracking flag here, since it was set on bug 757639 which I just duped to this bug.
Comment 50 Bobby Holley (:bholley) (busy with Stylo) 2012-06-05 10:21:25 PDT
Comment on attachment 629808 [details] [diff] [review]
Part 8 - Tests. v1

[Approval Request Comment]
Flagging this entire patch stack for m-a approval, per bug 757639 comment 14. It's a big change, but we just branched.
Comment 51 Blake Kaplan (:mrbkap) 2012-06-05 10:42:21 PDT
(In reply to Bobby Holley (:bholley) from comment #44)
> for WNs, or double for slim wrappers). FinishInit is never called on
> transplanted objects AFAICT.

Right, all I was saying was that this was maintaining the invariant for WNs that was taken care of by the JS_SetReservedSlot in FinishInit.
Comment 52 Bobby Holley (:bholley) (busy with Stylo) 2012-06-05 10:47:11 PDT
(In reply to Blake Kaplan (:mrbkap) from comment #51)
> Right, all I was saying was that this was maintaining the invariant for WNs
> that was taken care of by the JS_SetReservedSlot in FinishInit.

Ah, I see! Yes, that makes sense. :-)
Comment 54 Alex Keybl [:akeybl] 2012-06-11 12:45:19 PDT
Comment on attachment 629808 [details] [diff] [review]
Part 8 - Tests. v1

[Triage Comment]
Given where we are in the cycle, the possibility of add-on regressions in bug 757639, and our desire to leave bug 650353 in FF15, approving for Aurora 15.
Comment 55 Bobby Holley (:bholley) (busy with Stylo) 2012-06-13 03:48:41 PDT
We found a GC hazard in this patch - patch is waiting on review over in bug 763381. I'm going to wait until that lands before landing this on aurora.
Comment 56 Alex Keybl [:akeybl] 2012-06-19 14:13:18 PDT
Please nominate bug 763381 for aurora approval if that blocks landing this on mozilla-aurora.
Comment 58 Jason Smith [:jsmith] 2012-06-20 15:54:20 PDT
*** Bug 766718 has been marked as a duplicate of this bug. ***

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