1319087 - CacheIR: optimize cross-compartment wrappers

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P3)

Description

[Jan de Mooij [:jandem]]

We've discussed optimizing CrossCompartmentWrapper stuff (accessing simple data properties, dense elements, etc) in the JITs (bug 1245987). One of the goals for CacheIR was to make it really easy to do these things without duplicating or complicating a lot of code.

I should be able to get to this in the coming months, but if someone wants to take this on before that, I think the following should work: add GetPropIRGenerator::tryAttachCrossCompartmentWrapper that does the following:

* If the object is a CCW, unwrap it and call CanAttachNativeGetProp on the target and ensure that's a simple data property we can optimize.

* Then emit the following:

- GuardClass objId CrossCompartmentWrapper
- unwrappedId = LoadCrossCompartmentWrapperTarget objId
- GuardCompartment unwrappedId
- IR for the lookup, for instance [GuardShape unwrappedId, LoadFixedSlotResult unwrappedId]
- WrapResult
- TypeMonitorResult

* Right now the *Result ops, like LoadFixedSlotResult, will do type monitoring. We will have to factor that out into a separate op, TypeMonitorResult. I think this is a nice simplification anyway.

* The WrapResult op will just do a callVM to get a wrapper if the result register contains an object. If it's not an object, it has to do nothing.

* The GuardCompartment op is necessary I think since we now share shape trees across compartments.

* At least initially, we should only optimize this when the compartments are in the same zone, to simplify GC stuff. This will also let WrapResult get away with not doing a callVM if the result is a string (same-zone means no wrapping/copying is required for strings).

Comment 1

[Tom S [:evilpie]]

Attachment: Introduce CacheIR for ReturnResult/TypeMonitorResult

Comment 2

[Tom S [:evilpie]]

Attachment: WIP CacheIR for CrossCompartmentWrapper

Not finished yet. So I assume CrossCompratmentWrappers never have a security policy? CheckedUnwrap needs to be replaced with Wrapper::wrappedObject, I think, otherwise we unwrap too much. I also haven't implemented the GuardCompartment op yet, instead we always wrap the result.

Comment 3

[Tom S [:evilpie]]

Comment on attachment 8813413 [details] [diff] [review]
Introduce CacheIR for ReturnResult/TypeMonitorResult

I think this is okay and can land already. This depends on bug 1317703, especially for callVM in the second patch.

Comment 4

[Jan de Mooij [:jandem]]

Comment on attachment 8813413 [details] [diff] [review]
Introduce CacheIR for ReturnResult/TypeMonitorResult

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

Thanks!

Pre-existing, but in BaselineCacheIRCompiler::compile, before this line:

    // Done emitting the main IC code. Now emit the failure paths.

Please add: masm.assumeUnreachable("Should have returned from IC");

To catch bugs when we forget to emit a return.

::: js/src/jit/CacheIR.h
@@ +104,5 @@
>      _(CallNativeGetterResult)             \
> +    _(LoadUndefinedResult)                \
> +                                          \
> +    _(TypeMonitorResult)                  \
> +    _(ReturnResult)

Maybe call this Return or ReturnFromIC, so we can also use it for SETPROP/SETELEM later? Currently the SETPROP stubs return the RHS in R0, but when we get to it I think I'll change that to be like SETELEM (it leaves the RHS on the stack and the stubs don't return anything).

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7aad63193d7
Introduce CacheIR for TypeMonitorResult/ReturnFromIC. r=jandem

Comment 6

[Tom S [:evilpie]]

Attachment: v2 WIP

I think I now implemented everything as outlined in comment 0. I still need to do write some tests and there are some open question. Like do we need to trace JSCompartment*, should we maybe guard on the zone instead? I don't understand what preliminaryObjectAction_ actually means. And some small question about using R0 in emitWrapResult.

Comment 7

[Tom S [:evilpie]]

Ah I think the browser uses a different class for cross-compartment wrappers?

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/e7aad63193d7

Comment 9

[Jan de Mooij [:jandem]]

(In reply to Tom Schuster [:evilpie] from comment #6)
> Like do we need to
> trace JSCompartment*, should we maybe guard on the zone instead?

I think we have to guard on the compartment for correctness (due to sharing shapes across compartments), but you're right that we have to be careful because the compartment could die first. We can store the compartment's global in the stub, so that it doesn't get collected, or store the JSCompartment* and then trace its global. Storing the global might be simpler and safer...

Comment 10

[Tom S [:evilpie]]

Dammit, I just remembered another problem that I wanted to discuss. The browser doesn't seem to use CrossCompartmentWrapperClass, so we probably need something like cx_->maybeWindowProxyClass() for GuardClass.

Comment 11

[Jan de Mooij [:jandem]]

(In reply to Tom Schuster [:evilpie] from comment #10)
> Dammit, I just remembered another problem that I wanted to discuss. The
> browser doesn't seem to use CrossCompartmentWrapperClass, so we probably
> need something like cx_->maybeWindowProxyClass() for GuardClass.

Yeah, if we really use a different Class for this that would make sense.

Comment 12

[Tom S [:evilpie]]

Jan do you think in our current state of things we could add a third case to TypeMonitorResult/ReturnFromIC that allows us to wrap objects? Otherwise we have to split up all of the load**Result functions that we want to support for CCW.

Comment 13

[Tom S [:evilpie]]

Attachment: v3 WIP

I can start the browser with this patch in debug mode, but I a compartment missmatch in release mode. I don't handle wrapping failures at all, because I am not sure we can even handle failures at that point. For the NativeSlot case it's probably easier to add a shared method for environment objects, wrapping and dynamic/fixed slot, but that doesn't really scale to adding many more stubs.

Comment 14

[Tom S [:evilpie]]

Oh wow, I put the wrap call in an MOZ_ASSERT, of course that doesn't do anything on a release build. :(

Comment 15

[Tom S [:evilpie]]

With the fixed patch I get quite disappointing numbers for testcase in bug 907369.

Object: ~5ms
Cross-global object: 140ms before, 120ms with my patch

Changing the property from an object to int, so we don't have to wrap anything gives us:
Object: ~3ms?
Cross-global object: 110ms before, 9ms with my patch

So invoking the wrap function totally destroys performance here. But at least primitives are fast \o/
Maybe we with a few more optimization to the C++ wrapping code this would make a bigger impact.

Comment 16

[Tom S [:evilpie]]

On the GSheet test page we attach 775 of these stubs.

Comment 17

[Tom S [:evilpie]]

Attachment: v4 WIP

Jan or anyone else looking at the GSheet, please try this. In my own profiling ProxyGetPropertyValue is only 0.1% now.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Just so we're clear, the testcase in bug 907369 is not reporting ms, except incidentally.  It's reporting ns per operation.

That 120ns with patch includes some function calls, hashtable lookups, etc, etc, right?  That's ... not terrible.  I mean, if we can make it faster so much the better, but still.

What sort of numbers do you get for the typed array?

Comment 19

[Tom S [:evilpie]]

This was discussed before in bug 1245987, but I would like to refresh this a bit. To make sure we are handling the right CCW we obviously have to check which compartment it points to. This means we have to guard on the compartment in the IC. The question is can we bake in the JSCompartment pointer in JIT code and how marking for that should work, because it doesn't seem to be a "normal" GC type. Otherwise we could use the compartments' global as an object to guard on. However that rises the question of how/if we handle cross compartment GC pointers in JIT code (ion) / ic stubs (baseline).

Comment 20

[Tom S [:evilpie]]

Another issue with my current approach of using WrapResult after Load*SlotResult and going forward is how we should handle failures. Some of the Load*Result operations clobber the IC inputs, so we can't technically fail inside WrapResult, but wrapping objects *can* fail. For Load*SlotResult I think we should just merge that into LoadAndWrap*SlotResult, but this makes it harder to generalize this approach going forward. Obviously stuff like typed arrays would still work, because we never have to wrap numbers.

Comment 21

[Jan de Mooij [:jandem]]

(In reply to Tom Schuster [:evilpie] from comment #20)
> For Load*SlotResult
> I think we should just merge that into LoadAndWrap*SlotResult, but this
> makes it harder to generalize this approach going forward.

Ion ICs already can't clobber the inputs, maybe we should just change LoadDynamicSlotResult et al in Baseline to use an extra scratch register.

> Obviously stuff like typed arrays would still work, because we never have to wrap numbers.

Same for unboxed objects, array length, etc. Supporting all of these + native slots + dense elements is probably sufficient for now. Adding extra ops for loading/wrapping slots and elements seems fine.

Comment 22

[Tom S [:evilpie]]

Attachment: v5 WIP

While looking at the CacheIR logs for Google Sheets, I noticed we still had a lot GenericProxy stubs, turns out the CCW was wrapping a WindowProxy, so now we optimize that as well.

I also changed LoadDynamicSlotResult to not clobber obj and started checking for failures after the WrapObject call.

Comment 23

[Tom S [:evilpie]]

I just pushed this to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=da878d9fd33b2ef3565ededa13726408a79f1a18&selectedJob=77190749. There are various failures related to GC and other stuff, but it doesn't seem like a total failure.

One thing did catch my eye though:
PROCESS | 870 | Assertion failure: unwrapped == UnwrapOneChecked(obj), at /home/worker/workspace/build/src/js/src/jit/CacheIR.cpp:611
TEST-UNEXPECTED-TIMEOUT | /cors/remote-origin.htm | expected OK 

So it seems like we can't just assume that CrossCompartmentWrappers can always be unwrapped? This means we probably have to check the security policy? At that point we can probably just give up on give those objects a special class, because we have to look at the Handler anyway. Bz, can you confirm that?

Comment 24

[Boris Zbarsky [:bzbarsky]]

> So it seems like we can't just assume that CrossCompartmentWrappers can always be unwrapped?

Absolutely not, if we just test them via js::IsCrossCompartmentWrapper.  That will test true for _any_ cross-compartment wrapper, including various security wrappers, etc.

So yes, you want to look at the Handler.  This optimization is only safe to do for the &js::CrossCompartmentWrapper::singleton handler.  Good thing the tests caught this!

Comment 25

[Boris Zbarsky [:bzbarsky]]

I should note that js::IsCrossCompartmentWrapper already looks at the Handler:

  Wrapper::wrapperHandler(obj)->flags() & Wrapper::CROSS_COMPARTMENT

Maybe what you really want is a js::IsTransparentObjectWrapper, by analogy with js::TransparentObjectWrapper?

A bigger problem is guards.  This is the issue efaust ran into in bug 966518.  Right now, object transplanting can mutate the handler without changing the clasp of a proxy.  :(  And yes, this can include going at least _to_ a transparent CCW; specifically on navigation to a same-origin site the old WindowProxy gets transplanted to become a transparent CCW for the new WindowProxy, iirc.

Comment 26

[Tom S [:evilpie]]

Attachment: v6 - WIP

Updated patch that guards on the CrossCompartmentWrapper::singleton handler. Because I removed the custom JSClass we also don't wrongly use WrapperOptions::setSingleton(true) anymore, which was causing the GC related assert, because of the different lifetime of handler and proxy.

The biggest remaining piece now is that the "how to handle the target's compartment check".

Comment 27

[Tom S [:evilpie]]

We seem to be running into crashes where code called from WrapObject tries to call GetPcScript, which requires a BaselineFrame. So this probably means that we can't use callWithABI, but instead have to use callVM. Not sure yet if this is going to cause any complication.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Do you have a stack to how WrapObject tries to GetPcScript?  That seems annoying...

Comment 29

[Jan de Mooij [:jandem]]

(In reply to Tom Schuster [:evilpie] from comment #27)
> So this probably means that we can't use callWithABI, but instead have to use callVM.

Yes, JSCompartment::wrap can GC so we can't use callWithABI (wrapping can also call into XPConnect and other scary places).

What if we only handle the existing-wrapper case for now? Basically call compartment->lookupWrapper() and fail if there's no wrapper. We should measure how often that succeeds/fails on Google Docs...

Comment 30

[Jon Coppeard (:jonco)]

(In reply to Tom Schuster [:evilpie] from comment #19)
> The question is can we bake in the JSCompartment
> pointer in JIT code and how marking for that should work, because it doesn't
> seem to be a "normal" GC type. 

JSCompartment pointers won't move and the compartment won't be destroyed as long as any objects in it are alive.  So as long as you trace an object in a compartment when you trace the JIT code then you can bake the compartment pointer in.

> Otherwise we could use the compartments'
> global as an object to guard on. However that rises the question of how/if
> we handle cross compartment GC pointers in JIT code (ion) / ic stubs
> (baseline).

That is trickier because this pointer can move and we need a way of updating it when this happens (in particular when the target is in a zone being collected but the JIT code is not).  The standard answer is that you use a CCW but you won't want to unwrap this every time you need to get the target.

Comment 31

[Jon Coppeard (:jonco)]

(In reply to Jon Coppeard (:jonco) from comment #30)
> as long as you trace an object in a compartment when you trace the JIT code

Or better, a CCW to that object, otherwise you hit the second issue.

Comment 32

[Tom S [:evilpie]]

Attachment: v1 - Implement a CCW stub for NativeSlots and WindowProxy

Thank you all for your feedback. Based on that I think this should be good to review now. I changed WrapObject to LookupWrapper, and indeed on Google Sheets most of the time we seem to have a cache hit \o/. Sadly they don't seem to use primitive values at all, but no matter this is still a lot faster.
Based on Jon's comment I added a comment about using RawWord for JSCompartment* and left it as is.

Comment 33

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8837577 [details] [diff] [review]
v1 - Implement a CCW stub for NativeSlots and WindowProxy

>+        unwrapped = unwrapped->compartment()->maybeGlobal();

Can you assert that before that line ToWindowIfWindowProxy(unwrapped) == unwrapped->compartment()->maybeGlobal()?

And is there a reason to write it that way as opposed to cx_->global(), since we've entered the compartment of "unwrapped" already?

Basically, just comparing this code to what we have in tryAttachWindowProxy().

>+    if (holder) {

I guess null holder means "missing property; we'll return undefined"?

>+        EnsureTrackPropertyTypes(cx_, holder, id);

Why is this caller of EnsureTrackPropertyTypes() not conditioned on IsIonEnabled?  It looks like some are and some aren't....

>+            // See the comment in StripPreliminaryObjectStubs.

I assume tryAttachWindowProxy doesn't have to do the preliminary object thing because it knows a global can't be preliminary?

I do wonder whether we could somehow share more logic with tryAttachNative here...  It's not obvious to me how.  :(

>+    writer.guardIsProxy(objId);
>+    writer.guardIsCrossCompartmentWrapper(objId);

When would you ever need the second in isolation?  That is, should the guardIsProxy() call be _inside_ guardIsCrossCompartmentWrapper?

>+    ObjOperandId wrapperTargeId = writer.loadWrapperTarget(objId);

"wrapperTargetId".

>+        unwrappedId = writer.loadWrapperTarget(wrapperTargeId);

I guess this works.  But you could do better, I think.  By this point you have guarded on us having a CCW, the compartment of the target, and the target being a WindowProxy.  Which means that you have guarded on actual obj we want being  the global of the compartment of the target, no?  As in, you can just load a literal object pointer here, afaict.  That's what tryAttachWindowProxy does.  If there's a reason to _not_ do that, it should probably documented in a comment.

>+    if (holder) {
>+        writer.wrapResult();

Want to add the MOZ_ASSERT(shape) that EmitReadSlotReturn has?  And the comment in the "else" branch?

And in general, maybe we should just add a "wrap result" arg to EmitReadSlotReturn instead of copying it here?  Not sure.

I mostly skimmed over the CacheIRCompiler bits; I'm not sure I'd catch bugs in them even if they were there.

>+CacheIRCompiler::emitWrapResult()
>+    // Wrapping failed.

Not wrapping.  Lookup of an existing wrapper, right?

So a few things might be nice here, but should be followup bugs because as you noted on IRC they apply to all non-guard failures in ICs:

1)  Jumping directly to the end of the stub chain, not just next stub, if this fails.  Because we know none of the other stubs can succeed here either.

2)  Ensuring that taking this failure path does _not_ lead to falling back to a generic stub (which can happen if we're at the stub limit, DoGetPropFallback thinks we'll attach a stub, but we obviously won't because we already have one).

+        return (JSCompartment*)readStubWord(offset, StubField::Type::RawWord); // XXX

"XXX" what?  ;)

>+    masm.branchPtr(Assembler::NotEqual, scratch, ImmPtr(compartment), failure->label()); // XX?

"XX" what?

>+    if (WrapperMap::Ptr p = cx->compartment()->lookupWrapper(ObjectValue(*obj)))

This is a little silly.  We stick it in a Value just so CrossCompartmentKey can then switch on the type of the Value.  And CrossCompartmentKey alreay has a ctor taking JSObject*.

How about we just add a lookupWrapper overload that takes JSObject*?

That said, I have a few concerns about the wrapper-lookup bit:

1)  It doesn't seem like it will do the right thing if the thing we got out of the slot read is a CCW itself.  Most simply, consider the case when it's a CCW back into the original compartment we started with.  So this testcase:

   <iframe></iframe>
   <script>
     var obj = new frames[0].Object();
     obj.foo = new Object();
     // Now read obj.foo a bunch of times
   </script>

or a bit more complicated:

   <iframe></iframe><iframe></iframe>
   <script>
     var obj = new frames[0].Object();
     obj.foo = new frames[1].Object();
     // Now read obj.foo a bunch of times
   </script>

I think we should rename LookupWrapper to be called LookupObjectRepresentationForCompartment (or something; that name sucks) and it should do the following:

1)  CheckedUnwrap() the incoming obj.
2)  Check whether the result is same-compartment with cx; if so return it.
    This handles my single-iframe testcase above.
3)  lookupWrapper the unwrapped thing.  This handles my two-iframe testcase.

If we have a way to add such tests that check whether we actually got IC hits, that would be super; I expect both cases to fail with the patch as-is.

In terms of other testing, here are some tests you should probably look into:

A)  Load a page from some URL.  Load a subframe from some other URL on the same hostname (so they are same-origin, but don't share a single JSPrincipals instance).  Exercise the IC code.  Set "document.domain = document.domain" in either the page or the subframe (but not both) and check that we now get a security exception instead of blindly applying the IC.  Set "document.domain = document.domain" in whichever of the two documents we didn't set it in before, and verify that now the IC works again.

This should all be doable in a mochitest except maybe the "hey, did we actually use the IC?" part...  But maybe we can add some sort of testing function for that?  OR have one already?

B)  A test based on something like js/xpconnect/tests/unit/test_nuke_sandbox.js but probably written as a chrome mochitest, so you have access to a window.  Create a sandbox like this: var sb = Components.utils.Sandbox(window).  This will give you a sandbox that is same-origin with you.  Then you can create some CCWs into it, exercise this IC, then Components.utils.nukeSandbox(sb).  That should make all your CCWs into dead object wrappers, iirc.  Exercise the IC again, make sure it fails.

r=me modulo all that.

Comment 34

[Boris Zbarsky [:bzbarsky]]

Er... I forgot one bit.  Back to "concerns about the wrapper-lookup bit":

2)  Gray unmarking.  Right now we ensure that cx->wrap() never takes gray input and never produces gray output.  It looks to me like we could totally end up with gray input (the identity object of the thing passed to our LookupObjectRepresentationForCompartment might be gray if the CCW target was gray) and even if it's not the CCW we look up in our wrapper map might be gray.  You should probably talk to jonco/sfink about what the right thing here is....

Comment 35

[Jon Coppeard (:jonco)]

As mentioned about, if you bake in a compartment pointer you will need to keep that compartment alive, preferably by tracing a CCW to the compartment's global.  Does the patch do this?  I'm not familiar with CacheIR and I couldn't see where this happens.

As I understand it, your WrapResult operation is doing a simplified version of wrap and just looking up an existing wrapper with LookupWrapper.  Right now, JSCompartment::wrap() asserts that the input object is not gray and calls ExposeObjectToActiveJS() on the result.  You should do the same in LookupWrapper.

Comment 36

[Boris Zbarsky [:bzbarsky]]

I don't think LookupWrapper can assert the input is non-gray.

The jitcode basically starts with an object "obj" and then does:

  target = CheckedUnwrap(obj);
  retval = readslot(target, "foo");
  return LookupWrapper(retval);

Now the way this would _normally_ happen for "obj2 = obj.foo" when obj is a CCW is we would land in CrossCompartmentWrapper::get and do wrappedObject(wrapper), which unmarks-gray the target.  So "target" in my above pseudocode would be non-gray, the thing read from the slot would be non-gray, and the wrap() call could assert non-gray input.

But in the jitcode, the "target" is possibly-gray, and I'm not sure we want to worry about unmarking it, since it really doesn't escape.  That means the "retval" is possibly-gray, and hence the input to LookupWrapper() is possibly-gray.  All this stuff is, again, non-escaping and can't GC (right?) so this is all fine.  I do think LookupWrapper should ExposeObjectToActiveJS its return value, assuming the IC can survive that.

Comment 37

[Jon Coppeard (:jonco)]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #36)
Ah right, so we're skipping the UnmarkGray when we get the CCW target so we can't depend on this.  In that case I agree, we can just do an expose on the return value in LookupWrapper.

Comment 38

[Tom S [:evilpie]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #33)
> Comment on attachment 8837577 [details] [diff] [review]
> v1 - Implement a CCW stub for NativeSlots and WindowProxy
> 
> >+        unwrapped = unwrapped->compartment()->maybeGlobal();
> 
> Can you assert that before that line ToWindowIfWindowProxy(unwrapped) ==
> unwrapped->compartment()->maybeGlobal()?
> 
> And is there a reason to write it that way as opposed to cx_->global(),
> since we've entered the compartment of "unwrapped" already?
> 
Actually I just hadn't thought of this.
> Basically, just comparing this code to what we have in
> tryAttachWindowProxy().
> 
> >+    if (holder) {
> 
> I guess null holder means "missing property; we'll return undefined"?
> 
Exactly.
> >+        EnsureTrackPropertyTypes(cx_, holder, id);
> 
> Why is this caller of EnsureTrackPropertyTypes() not conditioned on
> IsIonEnabled?  It looks like some are and some aren't....
> 
Yeah no idea.
> >+            // See the comment in StripPreliminaryObjectStubs.
> 
> I assume tryAttachWindowProxy doesn't have to do the preliminary object
> thing because it knows a global can't be preliminary?
> 
> I do wonder whether we could somehow share more logic with tryAttachNative
> here...  It's not obvious to me how.  :(
> 
> >+    writer.guardIsProxy(objId);
> >+    writer.guardIsCrossCompartmentWrapper(objId);
> 
We usually like to split up CacheIR ops in smaller operations that do one thing.
> When would you ever need the second in isolation?  That is, should the
> guardIsProxy() call be _inside_ guardIsCrossCompartmentWrapper?
> 
> >+    ObjOperandId wrapperTargeId = writer.loadWrapperTarget(objId);
> 
> "wrapperTargetId".
> 
Thanks.
> >+        unwrappedId = writer.loadWrapperTarget(wrapperTargeId);
> 
> I guess this works.  But you could do better, I think.  By this point you
> have guarded on us having a CCW, the compartment of the target, and the
> target being a WindowProxy.  Which means that you have guarded on actual obj
> we want being  the global of the compartment of the target, no?  As in, you
> can just load a literal object pointer here, afaict.  That's what
> tryAttachWindowProxy does.  If there's a reason to _not_ do that, it should
> probably documented in a comment.
> 
Loading the literal means storing that object somewhere, which is sort of tricky with objects from another compartment, so I am going to add a comment about this.
> >+    if (holder) {
> >+        writer.wrapResult();
> 
> Want to add the MOZ_ASSERT(shape) that EmitReadSlotReturn has?  And the
> comment in the "else" branch?
> 
> And in general, maybe we should just add a "wrap result" arg to
> EmitReadSlotReturn instead of copying it here?  Not sure.
> 
Yeah we can just do this.
> I mostly skimmed over the CacheIRCompiler bits; I'm not sure I'd catch bugs
> in them even if they were there.
> 
> >+CacheIRCompiler::emitWrapResult()
> >+    // Wrapping failed.
> 
> Not wrapping.  Lookup of an existing wrapper, right?
> 
> So a few things might be nice here, but should be followup bugs because as
> you noted on IRC they apply to all non-guard failures in ICs:
> 
> 1)  Jumping directly to the end of the stub chain, not just next stub, if
> this fails.  Because we know none of the other stubs can succeed here either.
> 
> 2)  Ensuring that taking this failure path does _not_ lead to falling back
> to a generic stub (which can happen if we're at the stub limit,
> DoGetPropFallback thinks we'll attach a stub, but we obviously won't because
> we already have one).
> 
I added a comment about this to bug 1328140.
> +        return (JSCompartment*)readStubWord(offset,
> StubField::Type::RawWord); // XXX
> 
> "XXX" what?  ;)
> 
I forgot to remove this after getting the okay for using raw pointers for compartments.
> >+    masm.branchPtr(Assembler::NotEqual, scratch, ImmPtr(compartment), failure->label()); // XX?
> 
> "XX" what?
> 
> >+    if (WrapperMap::Ptr p = cx->compartment()->lookupWrapper(ObjectValue(*obj)))
> 
> This is a little silly.  We stick it in a Value just so CrossCompartmentKey
> can then switch on the type of the Value.  And CrossCompartmentKey alreay
> has a ctor taking JSObject*.
> 
> How about we just add a lookupWrapper overload that takes JSObject*?
> 
Yeah.
> That said, I have a few concerns about the wrapper-lookup bit:
> 
> 1)  It doesn't seem like it will do the right thing if the thing we got out
> of the slot read is a CCW itself.  Most simply, consider the case when it's
> a CCW back into the original compartment we started with.  So this testcase:
> 
>    <iframe></iframe>
>    <script>
>      var obj = new frames[0].Object();
>      obj.foo = new Object();
>      // Now read obj.foo a bunch of times
>    </script>
> 
> or a bit more complicated:
> 
>    <iframe></iframe><iframe></iframe>
>    <script>
>      var obj = new frames[0].Object();
>      obj.foo = new frames[1].Object();
>      // Now read obj.foo a bunch of times
>    </script>
> 
> I think we should rename LookupWrapper to be called
> LookupObjectRepresentationForCompartment (or something; that name sucks) and
> it should do the following:
> 
> 1)  CheckedUnwrap() the incoming obj.
> 2)  Check whether the result is same-compartment with cx; if so return it.
>     This handles my single-iframe testcase above.
> 3)  lookupWrapper the unwrapped thing.  This handles my two-iframe testcase.
> 
> If we have a way to add such tests that check whether we actually got IC
> hits, that would be super; I expect both cases to fail with the patch as-is.
> 
> In terms of other testing, here are some tests you should probably look into:
> 
> A)  Load a page from some URL.  Load a subframe from some other URL on the
> same hostname (so they are same-origin, but don't share a single
> JSPrincipals instance).  Exercise the IC code.  Set "document.domain =
> document.domain" in either the page or the subframe (but not both) and check
> that we now get a security exception instead of blindly applying the IC. 
> Set "document.domain = document.domain" in whichever of the two documents we
> didn't set it in before, and verify that now the IC works again.
> 
> This should all be doable in a mochitest except maybe the "hey, did we
> actually use the IC?" part...  But maybe we can add some sort of testing
> function for that?  OR have one already?
> 
> B)  A test based on something like
> js/xpconnect/tests/unit/test_nuke_sandbox.js but probably written as a
> chrome mochitest, so you have access to a window.  Create a sandbox like
> this: var sb = Components.utils.Sandbox(window).  This will give you a
> sandbox that is same-origin with you.  Then you can create some CCWs into
> it, exercise this IC, then Components.utils.nukeSandbox(sb).  That should
> make all your CCWs into dead object wrappers, iirc.  Exercise the IC again,
> make sure it fails.
Thanks for your ideas!
> 
> r=me modulo all that.

Comment 39

[Tom S [:evilpie]]

Attachment: v2 - Implement a CCW stub for NativeSlots and WindowProxy

I think I fixed most of the stuff, I just left EnsureTrackPropertyTypes alone.

Bobby: Can you please take a look at WrapObjectPure in VMFunctions.cpp? Especially if using lookupWrapper without first doing preWrap would be fine. I checked and we don't seem to do anything special for DOM objects, I think, but we still have a handful of XPCWN objects.

I am looking into writing some tests for this next, sadly we can't check yet if we attached an IC. That would be bug 1318285.

Comment 40

[Bobby Holley (:bholley)]

Comment on attachment 8839245 [details] [diff] [review]
v2 - Implement a CCW stub for NativeSlots and WindowProxy

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

Yeah, I think the characteristics of prewrap shouldn't change across multiple calls. So if prewrap returned this object before when the wrapper was created, I think it's safe to avoid calling prewrap again.

Comment 41

[Jan de Mooij [:jandem]]

Comment on attachment 8839245 [details] [diff] [review]
v2 - Implement a CCW stub for NativeSlots and WindowProxy

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

This looks great and the patch is simpler than I expected. Thanks for doing this!

::: js/src/jit/CacheIR.cpp
@@ +651,5 @@
> +    // Load the object wrapped by the CCW
> +    ObjOperandId wrapperTargetId = writer.loadWrapperTarget(objId);
> +
> +    // If the compartment of the wrapped object is different we should fail.
> +    writer.guardCompartment(wrapperTargetId, unwrapped->compartment());

As discussed on IRC, I'm a bit worried about the compartment being freed underneath us. It might be safest to have CacheIRWriter::guardCompartment() store the target global in the stub, with a comment there explaining its purpose is to keep the target compartment alive. (Don't forget to skip the unused stub field offset in the compilers.)

::: js/src/jit/CacheIRCompiler.cpp
@@ +1441,5 @@
> +    Register obj = allocator.useRegister(masm, reader.objOperandId());
> +    Register reg = allocator.defineRegister(masm, reader.objOperandId());
> +
> +    masm.loadPtr(Address(obj, ProxyObject::offsetOfValues()), reg);
> +    masm.unboxObject(Address(reg, 0), reg);

Please add ProxyValueArray::offsetOfPrivateSlot() and use it here.

Comment 42

[Tom S [:evilpie]]

I think I am going to land this with a simple test and continue working on adding more over the coming days. For example I think we should add NukeCrossCompartmentWrapper to the shell.

Thank you all for your help with this, this turned out to be a bit more complex.

Comment 43

[Tom S [:evilpie]]

Attachment: Add nukeCCW to the shell

Comment 44

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b718178f43f
Implement a CrossCompartmentWrapper IC stub. r=bz,bholley,jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee8c9e627623
Simple CCW DOM test. r=me

Comment 46

[Jan de Mooij [:jandem]]

Comment on attachment 8840042 [details] [diff] [review]
Add nukeCCW to the shell

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

Nice.

::: js/src/jit/VMFunctions.cpp
@@ +718,5 @@
>      if (WrapperMap::Ptr p = cx->compartment()->lookupWrapper(obj)) {
>          JSObject* wrapped = &p->value().get().toObject();
>  
> +        printf("wrapped\n");
> +

rm these 2 lines

Comment 47

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6b718178f43f
https://hg.mozilla.org/mozilla-central/rev/ee8c9e627623

Comment 48

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d439fa74bf05
Add nukeCCW to the shell and test it. r=jandem

Comment 49

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d439fa74bf05

Comment 50

[Tom S [:evilpie]]

Closing this for now. With the help of nukeCCW we already fuzz found bugs in this. So I think we should be good.

Comment 51

[Steve Fink [:sfink] [:s:]]

Attachment: Add nukeCCW to the shell and test it

nukeCCW test for esr45

Comment 52

[Ryan VanderMeulen [:RyanVM]]

Yeah, to clarify, I've already uplifted this test across the supported branches to support other backports.

https://hg.mozilla.org/releases/mozilla-beta/rev/4793c93e3ddd
https://hg.mozilla.org/releases/mozilla-esr52/rev/82297fcc6f19
https://hg.mozilla.org/releases/mozilla-esr45/rev/9cd44507fd65

Comment 53

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.