Closed Bug 658909 Opened 13 years ago Closed 11 years ago

Incorrect |this| binding when using call() with XRayWrappers

Categories

(Core :: XPConnect, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: ochameau, Assigned: bholley)

References

Details

(Whiteboard: [jetpack:p2])

Attachments

(22 files, 2 obsolete files)

3.51 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
9.78 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
3.53 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
1.25 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
1.49 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
10.67 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
12.11 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
3.67 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
5.56 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
938 bytes, patch
mrbkap
: review+
Details | Diff | Splinter Review
1.54 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
4.00 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
4.58 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
1.49 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
7.32 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
1.66 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
3.50 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
978 bytes, patch
mrbkap
: review+
Details | Diff | Splinter Review
2.07 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
1.99 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
6.57 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
3.74 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
While working on a big Proxy implementation for jetpack content scripts (Bug 601295), I faced this bug:
mozMatchesSelector return incorrect results (true instead of false, or the opposite) when you use mozMatchesSelector.call(node, selectors), only when you use Xraywrappers.
Otherwise it is working fine whithout wrappers or with wrappers but when you call this function directly from the node you want to match.

Example that can be runned in a JS console:
var doc = top.opener.gBrowser.contentDocument; [doc.documentElement.mozMatchesSelector.call( doc.createElement('div'), 'div'), doc.createElement('div').mozMatchesSelector('div')]

This particular statement should return true:
doc.documentElement.mozMatchesSelector.call( doc.createElement('div'), 'div')
When nsGenericElement::MozMatchesSelector is called, |this| is an HTMLHtmlElement, not an HTMLDivElement.

Andreas, Blake, what's going on there? The stack looks sort of like this:

#1  0x00007ffff5cfdfb9 in nsNSElementTearoff::MozMatchesSelector (this=
    0x7fffde4080c0, aSelector=..., aReturn=0x7fffffffa778)
    at ../../../../mozilla/content/base/src/nsGenericElement.cpp:5644
#2  0x00007ffff6d91483 in NS_InvokeByIndex_P (that=0x7fffde4080c0, methodIndex=25, 
    paramCount=2, params=0x7fffffffa760)
    at ../../../../../../../mozilla/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_x86_64_unix.cpp:195
#3  0x00007ffff65c2716 in CallMethodHelper::Invoke (this=0x7fffffffa720)
    at ../../../../../mozilla/js/src/xpconnect/src/xpcwrappednative.cpp:3141
#4  0x00007ffff65c071f in CallMethodHelper::Call (this=0x7fffffffa720)
    at ../../../../../mozilla/js/src/xpconnect/src/xpcwrappednative.cpp:2407
#5  0x00007ffff65bc832 in XPCWrappedNative::CallMethod (ccx=..., mode=CALL_METHOD)
    at ../../../../../mozilla/js/src/xpconnect/src/xpcwrappednative.cpp:2371
#6  0x00007ffff65cc5ad in XPC_WN_CallMethod (cx=0x7fffde7ed400, argc=1, vp=
    0x7fffeaf000d0)
    at ../../../../../mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp:1610
#7  0x00007ffff49f35f6 in js::CallJSNative (cx=0x7fffde7ed400, native=
    0x7ffff65cc33a <XPC_WN_CallMethod(JSContext*, uintN, jsval*)>, argc=1, vp=
    0x7fffeaf000d0) at ../../../mozilla/js/src/jscntxtinlines.h:277
#8  0x00007ffff49f68e7 in js::Invoke (cx=0x7fffde7ed400, argsRef=..., option=
    INVOKE_NORMAL) at ../../../mozilla/js/src/jsinterp.cpp:646
#9  0x00007ffff49a1a60 in js_fun_call (cx=0x7fffde7ed400, argc=1, vp=0x7fffeaf000b0)
    at ../../../mozilla/js/src/jsfun.cpp:2149

so js_fun_call is basically jumping directly into the XPC_WN_CallMethod code.  The |obj| in the latter is the Proxy XrayWrapper.    Then we create the XPCCallContext, which calls XPCWrappedNative::GetWrappedNativeOfJSObject which does the whole 

  1759  // If we were passed a function object then we need to find the correct
  1760  // wrapper out of those that might be in the callee obj's proto chain.

thing, finds an XPCWrappedNative on the funobj parent, and returns that.  In other words, it completely ignores the attempt to pass in a different |this|....
Summary: mozMatchesSelector returns incorrect results with Xraywrappers → Incorrect |this| binding when using call() with XRayWrappers
The problem here is two folded. One is that XPCWrappedNative::GetWrappedNativeOfJSObject ignoring the obj (wrapped 'this') and using only the function object (wrapped mozMatchesSelector). The reason behind this is an xpconnect feature, where native functions can be bound to an object, and when the function object in the XrayWrapper is created this is what happening. Simply passing instead of the wrapper, the wrappers prototype in NewFunctionObject calls in the XrayWrapper.cpp seems to solve this problem (thanks mrbkap for the idea), and then the GetWrapperNativeOfJSObject will not ignore the 'this' obj any longer (I have not found any side effect for this change but have not tried runing all the test either yet...)

And here comes problem two. The real one. What we get now is an exception... Now let's see what we are trying to do here. MozMatchesSelector is a native functions, implemented in cpp, on some class. We clearly cannot allow to pass in any type of this pointer into this function, a mallicious cast inside the cpp implementation of the function can lead to all kind of nasty crashes or worse... So when we unbound the method from the object bye the js 'call', the function object we get will still know that it belongs to a HTMLHtmlElement and it won't allow a call with a different type of this pointer (HTMLDivElement in this example) that's why the exception is thrown. Clearly there are cases where we don't want to allow the call because of the reasons I mentioned earlier. 

However in this case, we could allow the call, since the HTMLDivElement DOES implement the same function, further more, the implementation is the SAME cpp function. Question is how important is to implement this feature (with these strict limitations), and how difficult it is. I investigated the problem and it turns out to be extremly difficult. Right now we only compare nsIClassInfos and if it's the same we can make the call. To make the example work, we should also check if the 'this' obj (or some of it's proto obj) implements a method with the same name (linear lookup based on name) for the same interface, and here is the hard part, that method has the very same function pointer in the vtable (so the implementation in cpp is the same). Now that last one is not easy, since we only now it's index in the original's vtable class, the index of the same function is different in the other class's vtable, and to fetch a raw functionpointer from a vtable index, we have to do some platform dependant, compilar dependant magic (domething similar is done for each platform for the actual call, the windows version is implemented in asm, and this is where we need to deal with the fact that vtables can be implemented differently by each compiler on each platform), that I would really like to avoid to be honest.

So this is the story behind this issue I would like to hear someone's opinion about it who needs this feature, and someone's who has a deep understanding of these areas in the platform code to make a decision about how to deal with this problem. Thanks.
> the function object we get will still know that it belongs to a
> HTMLHtmlElement and it won't allow a call with a different type of this
> pointer (HTMLDivElement in this example)

Yes, but this works in general, no?  You can .call() functions you rip off one element on another one and normally land in the right quickstub, for example, when there are no wrappers involved.  Why is it not working here?

Note that web pages use this all the time, and it works fine there.

> To make the example work, we should also check if the 'this' obj (or some of
> it's proto obj) implements a method with the same name

The glue code does this already, no?
(In reply to comment #3)
> > the function object we get will still know that it belongs to a
> > HTMLHtmlElement and it won't allow a call with a different type of this
> > pointer (HTMLDivElement in this example)
> 
> Yes, but this works in general, no?  You can .call() functions you rip off
> one element on another one and normally land in the right quickstub, for
> example, when there are no wrappers involved.  Why is it not working here?
> 
> Note that web pages use this all the time, and it works fine there.

Could you provide me an example? I would like to see how is it working... Maybe all these problems are solved somewhere already just not hooked into the wrapper properly...


> 
> > To make the example work, we should also check if the 'this' obj (or some of
> > it's proto obj) implements a method with the same name
> 
> The glue code does this already, no?

I don't know what do you refer to with the glue code (I'm totally new here), but this part CAN be done for sure. Maybe some code does it already, but I see here some conceptual problems. Let's say you want to call Class A's someMethod on class B instance. If class B has a different method with the same name (someMethod) what will happen? 

1., A::someMethod will be called with an instance of B in which case it might try to access some members of class A on a class B instance and it can crash... 

or

2., B::someMethod will be called instead, in which case the 'call' function will not work as expected, but at least we get a safe behaviour. 

Either way I would love to see how is this problem handled, so if you could provide me an example that works, I can debug it how is it solved, because frankly I don't have a good feeling about this.
> Could you provide me an example?

<!DOCTYPE html>
<body>
  <script>
    var b = document.body;
    alert(b.mozMatchesSelector.call(document.documentElement, "html"))
  </script>
</body>

> I don't know what do you refer to with the glue code 

The code that maps JS calls to C++ calls.  So quickstubs and normal XPConnect dispatch.

Note that the above code exercises the mozMatchesSelector quickstub, I would assume.  You'd want to comment out the mozMatchesSelector stuff in dom_quickstubs.qsconf to exercise normal XPConnnect dispatch here.

> Let's say you want to call Class A's someMethod on class B instance.

Note that someMethod is attached to an _interface_, not a _class_ in the JS reflection.

> If class B has a different method with the same name (someMethod) what will
> happen? 

An exception, since that means B doesn't implement the interface someMethod came from.  someMethod knows that it's callable on anything implementing the interface it comes from.  In the case of mozMatchesSelector, this is anything that implements the Element interface.

In particular, what will happen here conceptually is that the method will QI B to the interface involved.  If it implements that interface, great, and then you just call the relevant C++ method.  This is a virtual function call, so will jump to the right implementation (B's in this case).  If B does not implement the interface, you get an exception.

In practice, quickstubs do an end run around the QI by casting all elements to nsGenericElement.  But core XPConnect dispatch should go through the QI codepath.
First of all thanks for the help, I will try to continue it from here tomorrow and report back if stuck or found a solution for the problem.

And yeah, this is exactly what I would expect:
> 
> In particular, what will happen here conceptually is that the method will QI
> B to the interface involved.  If it implements that interface, great, and
> then you just call the relevant C++ method.  This is a virtual function
> call, so will jump to the right implementation (B's in this case).  If B
> does not implement the interface, you get an exception.

But this is what actually happening on windows for that virtual function call: NS_InvokeByIndex_P(nsISupports* that, PRUint32 methodIndex,
                 PRUint32 paramCount, nsXPTCVariant* params)
{
    __asm {
        mov     edx,paramCount      // Save paramCount for later
        test    edx,edx             // maybe we don't have any params to copy
        jz      noparams
        mov     eax,edx             
        shl     eax,3               // *= 8 (max possible param size)
        sub     esp,eax             // make space for params
        mov     ecx,esp
        push    params
        call    invoke_copy_to_stack // fastcall, ecx = d, edx = paramCount, params is on the stack
noparams:
        mov     ecx,that            // instance in ecx
        push    ecx                 // push this
        mov     edx,[ecx]           // vtable in edx
        mov     eax,methodIndex
        call    [edx][eax*4]        // stdcall, i.e. callee cleans up stack.
        mov     esp,ebp
    }
}

which is a virtual function call indeed, just not by name but by vtable index that belongs to the function object, and which can be totally different in class A and B (even if they implement the same interface) for the same function... But anycase, maybe this just works fine, I'll examine the example you gave me first.
> just not by name but by vtable index that belongs to the function object

Yes, but the vtable index should be the same for all objects implementing that interface; that's what it means to implement an interface!

In particular, the vtable lookup there is for the _interface_ pointer, not the derived-class pointer.
right, I got it now, thanks for clearing it up.
(In reply to comment #5)
> <!DOCTYPE html>
> <body>
>   <script>
>     var b = document.body;
>     alert(b.mozMatchesSelector.call(document.documentElement, "html"))

This example is a little bizarre since mozMatchesSelector is quickstubbed. One of the differences between quickstubs and regular XPConnect functions is that XPConnect functions refuse to be called on an object that doesn't share the classinfo of the object the function was originally gotten off of. I've talked to jst about relaxing this restriction, but we haven't done that yet. Perhaps we should just remove that restriction?
> Perhaps we should just remove that restriction?

That's what I would like to check, or more precisely ensure that if the restriction is removed the interface is still checked somewhere, or if it's not then check for the correct interface in XPConnect... Can you think of any side effect the removal of the restriction would cause?
Ah, I didn't realize that XPConnect dispatch did that.

I wouldn't worry about that, I guess; that restriction will automagically go away with new DOM bindings, right?
Extending that classinfo check, with an interface lookup if it's possible (funobject interface against the interfaces the current 'this' object implements) the problem can be fixed it seems. 

I will clean up this code and check the tests if it has any side effect, then create a patch out of it if it works.

On the other hand if the interface will be checked before the call later anyway, maybe if would simply swap the 'if(IS_WRAPPER_CLASS(clazz))' branch with the 'if(IS_TEAROFF_CLASS(clazz))' branch (http://mxr.mozilla.org/mozilla-central/source/js/src/xpconnect/src/xpcwrappednative.cpp#1781) we could just simple remove the classInfo check without adding any extra interface check. And simply let the call throw an exception later when it validates the this objec if it implements the right interface for the call... Does that make any sense? I'm still not sure what is the purpose of this same class info restriction, and the interface checking will be done later during the call anyway, so it should not be done here as well I guess, it feels like an unnecessary double validation...
Attached patch first attempt to fix the bug (obsolete) — Splinter Review
Ok, so I've been playing around with this for a while and the original idea, to pass the wrappers prototype instead of the wrapper in XrayWrapper.cpp's NewFunctionObject calls, was breaking some tests, so I dropped the idea.

In the end I only modified the XPCWrappedNative::GetWrappedNativeOfJSObject function. So the idea is that when it's a bound function, I check first if the provided 'this' obj is suitable for being used as the this object for the call, and if it is, I use it. (I think for a bound function it makes sense to 'break' that boundary in case of a sane js 'call' like in the example)

I really need some review here, and some help as well about two things. In the introduced IsObjectSuitableForCall, the first half of the function is basically XPCNativeMember::GetCallInfo.
The reason why I do not call the function is that XPCCallContext& ccx argument that I don't have, but actually not used in the function, is there a reason for that to be there or can I just remove that argument from the function signiture? But the real question is this: 
if (JSVAL_IS_DOUBLE(ifaceVal) && JSVAL_IS_DOUBLE(memberVal))
It really does not feel right, is there a proper way to validate if the JSObject wrapps a native function?
Assignee: nobody → gkrizsanits
Status: NEW → ASSIGNED
Attachment #550636 - Flags: review?(mrbkap)
Hey Gabor, I'll review your patch tomorrow. Sorry for the delay.
Sure, no problem. In the meanwhile I started to work on this bug https://bugzilla.mozilla.org/show_bug.cgi?id=665279 and just realized that neither you nor bz on the cc list of the bug. Could any of you take a look at the comment I put on it? I hope you guys don't mind that I need some extra help around these xpconnect/wrappers related issues at the beginning... I'm diving into this part of the platform since I'm in the jetpack team and I thought an extra hand at this area would be a big help for the team and for you guys as well in the future.
Comment on attachment 550636 [details] [diff] [review]
first attempt to fix the bug

Gabor and I talked about this yesterday. I suggested a different approach, basically allowing non-bound functions to be called on just about any object and using QI to avoid actually doing the call (as opposed to enforcing that you can only call a function that you got off of an object with the same classinfo). This is more in line with quickstubs. We're still going to restrict function calls to being from the same scope.
Attachment #550636 - Flags: review?(mrbkap)
Attached file sketch of the new approach (obsolete) —
Ok so in practice there is an issue I don't know how to resolve. So I attached the diff of the version I tried and it works usually, but there is a problem with it. In XrayWrapper if I don't pass the wrapper as the parent of the created function object any more, it means that in GetWrappedNativeOfJSObject it will work as a non bound function which is good. The problem is that there is no direct reference to the wrapper in that function object any more. So in the rare case when the obj is a ProxyObject we will not have a reference to the wrapper at all in the GetWrappedNativeOfJSObject, hence the function will fail. Example: mochi.test:8888/tests/content/base/test/test_CrossSiteXHR.html on line 647 the call will fail (loaderWindow.postMessage(req.toSource(), origin);)

I have no idea where could we store a reference to the wrapper on that function object... We cannot add an extra slot for function objects right? If we can maybe that could be one solution...
Depends on: 697775
Blocks: 697775
No longer depends on: 697775
Assignee: gkrizsanits → ejpbruel
(ignore comment 15)

(In reply to Blake Kaplan (:mrbkap) from comment #16)
> Comment on attachment 550636 [details] [diff] [review]
> first attempt to fix the bug
> 
> and using QI to avoid actually doing the call (as opposed to enforcing that
> you can only call a function that you got off of an object with the same
> classinfo). 

So the problem with this approach is the interface inheritance. When the ResolveNativeProperty of the xraywrapper looks up the method, it will use xpc context, and it will find the method on one of the interfaces. The problem is that the interface it finds not necessary the base interface for that method (XPCNativeSet::FindMember). 

So for the mozMatchesSelector it will find the method on the nsIDOMHHTMLhtmlElement interface instead of it's base nsIDOMElement. So when we do a QI later on the other object (that does implement nsIDOMElement and have the same mozMatchSelector method) the QI will fail because it only implement nsIDOMHTMLDivElement and we try QI with nsIDOMHHTMLhtmlElement.

So XPCNativeSet::FindMember should return the base most interface that has the member. (nsIDOMElement in the example)
Assignee: ejpbruel → gkrizsanits
So changing that FindMember function breaks something else... If FindMember does return the base interface that has the looked up method, hasOwnProperty might fail if there is a proto set. http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/XPCInlines.h#465

Would it be possible to use quickstubs in some way from xraywrapper (theoretically)? So once we allow a certain method call though the xraywrapper, it would use the same call path (quickstub) as without xraywrapper?
Blocks: 738244
Depends on: 763897
I'm working on this. The first chunk of patches here pass XPConnect tests. Pushing to try.
Assignee: gkrizsanits → bobbyholley+bmo
Depends on: 812693, 812669
Depends on: 819158
Attachment #550636 - Attachment is obsolete: true
Attachment #561459 - Attachment is obsolete: true
Depends on: 819507
Blocks: 832014
Blocks: 837863
Depends on: 841067
Ugh. This gives an extra incentive to try to find a way to get rid of SOWs.
Attachment #713954 - Flags: review?(mrbkap)
Attachment #713955 - Flags: review?(mrbkap)
This is a huge footgun. XPCCallContext is hot, but it's not too hot to be safe
here. Garbage XPCWN and JSObject pointers are bad.
Attachment #713956 - Flags: review?(mrbkap)
It's not clear to me why it's done this way, but it confuses our ability to
determine during wrapper lookup whether we're doing a set or a get. This aligns
the behavior with XPC_WN_CallMethod, including passing JSID_VOID for the name
(which is safe because XPCCallContext explicitly special-cases JSID_VOID and
doesn't call SetName in that case).
Attachment #713957 - Flags: review?(mrbkap)
Using JSPropertyOp means a null shape getter, whereas null means that the shape
uses the class getter. This means that stuff like window.top, which is defined
as a non-configurable |own| property in nsDOMClassInfo, was getting set up with
XPC_WN_Helper_GetProperty as its get operation. But this confused
SandboxProxyHandler, which explicitly avoids rebinding class getters/setters,
which in turn meant that our sandboxPrototype feature was relying on the crazy
prototype-climbing behavior of GetWrappedNativeOfJSObject to make stuff like
|this.top| work. We're removing this behavior, so we need to fix nsDOMClassInfo
here.

Here are some DefineProperty cases that I left with null getters/setters:
* nsDOMClassInfo::ResolveConstructor
* The child window stuff at the bottom of nsWindowSH::NewResolve
* Named item resolution in nsNamedArraySH::NewResolve
* document.all stuff (scary!)
* nsHTMLDocumentSH::NewResolve
* nsHTMLFormElementSH::NewResolve
* nsStorage2SH::NewResolve
Attachment #713959 - Flags: review?(mrbkap)
Attachment #713967 - Flags: review?(mrbkap)
The old code seems to be deciding whether we have a double-wrapped object by
checking _either_ the rv of GWNOJO _or_ the potential slim wrapper. This is
nonsensical, because double-wrapped objects are never slim wrappers.
Furthermore, that variable here is named 'proto', which further suggests
that this code is nonsensical. So let's just check for WNs.

Also, it seems pretty wack to be innerizing here before storing the jsval,
but I'm going to leave that for now.
Attachment #713968 - Flags: review?(mrbkap)
Attachment #713975 - Flags: review?(mrbkap)
Comment on attachment 713953 [details] [diff] [review]
Part 1 - Force |this| computation in SandboxCallableProxyHandler::call when using Xrays. v1

> What really matters is
>+    // the strictness of the caller

No, of the callee.  And all DOM methods act as non-strict methods.  So it's even simpler than you think, in that acting non-strict for DOM stuff is always right.

r=me
Attachment #713953 - Flags: review?(bzbarsky) → review+
Attachment #713954 - Flags: review?(mrbkap) → review+
Attachment #713955 - Flags: review?(mrbkap) → review+
Comment on attachment 713956 [details] [diff] [review]
Part 4 - Initialize the same fields in both XPCCallContext constructors. v1

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

I wonder how many cycles setting three extra pointers to null takes.

::: js/xpconnect/src/XPCCallContext.cpp
@@ +30,5 @@
> +        mFlattenedJSObject(nullptr),
> +        mWrapper(nullptr),
> +        mTearOff(nullptr)
> +
> +

Nit: nuke the extra blank lines.
Attachment #713956 - Flags: review?(mrbkap) → review+
Attachment #713957 - Flags: review?(mrbkap) → review+
Comment on attachment 713958 [details] [diff] [review]
Part 6 - Implement carefully-checked unwrapping in XPCCallContext. v1

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

::: js/xpconnect/src/XPCCallContext.cpp
@@ +147,5 @@
>      if (wrapperInitOptions == INIT_SHOULD_LOOKUP_WRAPPER) {
> +
> +        // If the object is a security wrapper, GetWrappedNativeOfJSObject can't
> +        // handle it. Do special handling here to make cross-origin Xrays work.
> +        if (!js::UnwrapObjectChecked(obj)) {

I talked to bholley on IRC about this. We're going to add a helper to make this "IsSecurityWrapper" since right now it appears that we unwrap twice which isn't actually the case.

@@ +469,5 @@
> +    JSObject *unwrapped = js::UnwrapObject(obj, /* stopAtOuter = */ false);
> +    MOZ_ASSERT(unwrapped == JS_ObjectToInnerObject(mJSContext, js::Wrapper::wrappedObject(obj)));
> +
> +    // Security wrapping always morphs slim wrappers, so we don't have to worry about them here.
> +    if (!IS_WN_WRAPPER(unwrapped))

Given the comment (and that we have a wrapper) can we simply assert this?
Attachment #713958 - Flags: review?(mrbkap) → review+
Comment on attachment 713959 [details] [diff] [review]
Part 7 - Use JS_{,Strict}PropertyOp instead of null when defining value props in nsDOMClassInfo. v1

So, this is basically just for sanity? It seems that as long as any object in nsDOMClassInfo uses a class getter, that we'll have to support this anyway...
Attachment #713959 - Flags: review?(mrbkap) → review+
Attachment #713960 - Flags: review?(mrbkap) → review+
Comment on attachment 713961 [details] [diff] [review]
Part 9 - Stop doing all the crazy stuff in GetWrappedNativeOfJSObject. v1

\o/ *holds breath*
Attachment #713961 - Flags: review?(mrbkap) → review+
Attachment #713962 - Flags: review?(mrbkap) → review+
Attachment #713963 - Flags: review?(mrbkap) → review+
(In reply to Blake Kaplan (:mrbkap) from comment #51)
> Comment on attachment 713959 [details] [diff] [review]
> Part 7 - Use JS_{,Strict}PropertyOp instead of null when defining value
> props in nsDOMClassInfo. v1
> 
> So, this is basically just for sanity?

No, see comment 32.
Comment on attachment 713964 [details] [diff] [review]
Part 12 - Remove GWNOJO for helper stubs. v1

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

::: js/xpconnect/src/XPCWrappedNativeJSOps.cpp
@@ +851,5 @@
> +    JSObject *unwrapped = js::UnwrapObjectChecked(obj, false);                \
> +    if (!unwrapped) {                                                         \
> +        JS_ReportError(cx, "Permission denied to operate on object.");        \
> +        return false;                                                         \
> +    } else if (!IS_WRAPPER_CLASS(js::GetObjectClass(unwrapped))) {            \

Nit: nuke else after return.

@@ +862,2 @@
>      }                                                                         \
>      else                                                                      \

Nit: else should go on the line with the closing brace (and its opening brace should be on the same line).
Attachment #713964 - Flags: review?(mrbkap) → review+
(In reply to Bobby Holley (:bholley) from comment #53)
> No, see comment 32.

What happens in the cases that weren't fixed up here?
Comment on attachment 713965 [details] [diff] [review]
Part 13 - Remove GWNOJO from JSObject2NativeInterface. v1

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

::: js/xpconnect/src/XPCConvert.cpp
@@ +1033,5 @@
>          // If we're looking at a security wrapper, see now if we're allowed to
>          // pass it to C++. If we are, then fall through to the code below. If
>          // we aren't, throw an exception eagerly.
> +        JSObject* inner = js::UnwrapObjectChecked(src, /* stopAtOuter = */ false);
> +        // Hack - For historical reasons, wrapped chrome JS objects have been

Nit: please add a blank link before the comment after the declaration.

@@ +1035,5 @@
>          // we aren't, throw an exception eagerly.
> +        JSObject* inner = js::UnwrapObjectChecked(src, /* stopAtOuter = */ false);
> +        // Hack - For historical reasons, wrapped chrome JS objects have been
> +        // passable as native interfaces. We'd like to fix this, but it
> +        // involves fixing the contacts API an PeerConnection to stop using

Tyop: the contacts API *and* PeerConnection
Attachment #713965 - Flags: review?(mrbkap) → review+
Comment on attachment 713966 [details] [diff] [review]
Part 14 - Remove GWNOJO from JSValToXPCException. v1

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

::: js/xpconnect/src/XPCConvert.cpp
@@ +1180,5 @@
> +        JSObject *unwrapped = js::UnwrapObjectChecked(obj, /* stopAtOuter = */ false);
> +        if (!unwrapped)
> +            return NS_ERROR_XPC_SECURITY_MANAGER_VETO;
> +        XPCWrappedNative* wrapper = IS_WN_WRAPPER(unwrapped) ? XPCWrappedNative::Get(unwrapped)
> +                                                       : nullptr;

Odd indentation here.
Attachment #713966 - Flags: review?(mrbkap) → review+
Comment on attachment 713967 [details] [diff] [review]
Part 15 - Remove GWNOJO from XPCJSID. v2

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

::: js/xpconnect/src/XPCJSID.cpp
@@ +466,5 @@
> + * This static method handles both complexities, returning either an XPCWN, a
> + * slim wrapper, a DOM object, or null.
> + */
> +static JSObject *
> +FindObjectForHasInstance(JSContext *cx, JSObject *obj)

It might be worth adding a comment pointing out explicitly this might return an object from another compartment (even though point 1 kinda mentions it).
Attachment #713967 - Flags: review?(mrbkap) → review+
(In reply to Blake Kaplan (:mrbkap) from comment #55)
> (In reply to Bobby Holley (:bholley) from comment #53)
> > No, see comment 32.
> 
> What happens in the cases that weren't fixed up here?

We will continue using the class getter, which could pose problems for anything inherited via sandboxPrototype. I tried to walk the line between fixing the stuff that needs fixing and not breaking arcane nsDOMClassInfo magic that actually relies on the class getter. The heuristics I used were admittedly fuzzy, and I'm certainly open to suggestions of other things to convert / not convert.
Comment on attachment 713968 [details] [diff] [review]
Part 16 - Remove GWNOJO from XPCVariant. v1

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

::: js/xpconnect/src/XPCVariant.cpp
@@ +27,5 @@
>      : mJSVal(aJSVal), mCCGeneration(0)
>  {
>      nsVariant::Initialize(&mData);
>      if (!JSVAL_IS_PRIMITIVE(mJSVal)) {
> +        // XXXbholley - Why are we innerizing here?

See bug 638026. Basically, the inner holds the outer alive but not vice versa. So if someone sticks an outer window in a variant, navigates the window, causes a GC, and retrieves the JS object from the variant, we can end up with a frankenwrapper. Holding onto the inner, however, *does* hold the outer alive, so it's safer to do that and outerize when we hand object back out to JS.
Attachment #713968 - Flags: review?(mrbkap) → review+
Attachment #713969 - Flags: review?(mrbkap) → review+
Attachment #713970 - Flags: review?(mrbkap) → review+
(In reply to Bobby Holley (:bholley) from comment #59)
> We will continue using the class getter, which could pose problems for
> anything inherited via sandboxPrototype. I tried to walk the line between
> fixing the stuff that needs fixing and not breaking arcane nsDOMClassInfo
> magic that actually relies on the class getter. The heuristics I used were
> admittedly fuzzy, and I'm certainly open to suggestions of other things to
> convert / not convert.

This is pretty worrying, actually. If I remember correctly, GreaseMonkey and the web console both run through this, and webdevs probably expect things like the magic name frame thing to work. I have to admit that I didn't follow the SandboxProxy stuff as closely as I should have. Is there any way that we can work around this there?
(In reply to Bobby Holley (:bholley) from comment #32)
> Here are some DefineProperty cases that I left with null getters/setters:
> * nsDOMClassInfo::ResolveConstructor
> * The child window stuff at the bottom of nsWindowSH::NewResolve

From our discussion on IRC, the child window stuff definitely needs to be converted and I *think* but I'm not sure that ResolveConstructor needs to be converted. I'll pick up reviewing this again tomorrow.
The child window stuff needs to use the class-default getter, no?  Until I fix bug 823227 and remove that code altogether, at least....
(In reply to Boris Zbarsky (:bz) from comment #63)
> The child window stuff needs to use the class-default getter, no?  Until I
> fix bug 823227 and remove that code altogether, at least....

Hm, yeah, I think that was why I did it that way.

But looking at the code, it appears that the only thing we do in nsWindowSH::GetProperty is decide whether to return NS_OK vs NS_SUCCESS_I_DID_SOMETHING. Is that even important anymore?
Oh, hmm.  I'd missed the fact that we never did any actual wrapping of the stuff in GetProperty...  That's so broken.

But no, given that just nixing that GetProperty and the corresponding WANT_GETPROPERTY flag is reasonable.
Attachment #713971 - Flags: review?(mrbkap) → review+
Attachment #713973 - Flags: review?(mrbkap) → review+
Comment on attachment 713974 [details] [diff] [review]
Part 21 - Port tearoff-handling guts of GWNOJO to XPCCallContext and remove GWNOJO. v1

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

::: js/xpconnect/src/XPCCallContext.cpp
@@ +61,5 @@
>           nullptr, nullptr);
>  }
>  
> +#define IS_TEAROFF_CLASS(clazz)                                               \
> +          ((clazz) == &XPC_WN_Tearoff_JSClass)

Nit: any reason for the double indentation?

@@ +147,5 @@
>      mState = HAVE_OBJECT;
>  
>      mTearOff = nullptr;
>      if (wrapperInitOptions == INIT_SHOULD_LOOKUP_WRAPPER) {
>  

Nit: nuke this blank line.

@@ +151,5 @@
>  
>          // If the object is a security wrapper, GetWrappedNativeOfJSObject can't
>          // handle it. Do special handling here to make cross-origin Xrays work.
> +        JSObject *unwrapped = js::UnwrapObjectChecked(obj, /* stopAtOuter = */ false);
> +        if (!unwrapped) {

Ah-ha, this is what I was missing before. I was expecting the code to look like this before. With this change, I don't think we need the new API.
Attachment #713974 - Flags: review?(mrbkap) → review+
Comment on attachment 713975 [details] [diff] [review]
Part 22 - Tests. v1

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

Marking r+. With this, all the patches in this bug have r+, but I think we need to fix the nsWindowSH::NewResolve + SandboxProxy thing before landing. I'll be ready to stamp that patch so we can get this in.

::: js/xpconnect/tests/chrome/test_bug658909.xul
@@ +30,5 @@
> +    return "(" + a.name + ", " + b.name + "): " + testName;
> +  }
> +
> +  var testFunctions = {
> +  testDocumentElement: function(a, b, name) {

Nit: please indent the functions in testFunctions.

@@ +31,5 @@
> +  }
> +
> +  var testFunctions = {
> +  testDocumentElement: function(a, b, name) {
> +    var getter = Object.prototype.__lookupGetter__.call(a.document, 'documentElement');

Please test a setter or two as well, since that's a pretty separate code path. Also, try an invalid call (getting nodeName on an attribute, or something) to make sure that we throw in all of the right places as well.
Attachment #713975 - Flags: review?(mrbkap) → review+
(In reply to Bobby Holley (:bholley) from comment #64)
> But looking at the code, it appears that the only thing we do in
> nsWindowSH::GetProperty is decide whether to return NS_OK vs
> NS_SUCCESS_I_DID_SOMETHING. Is that even important anymore?

That's broken. We need to grab the possibly new window and return it in the getter. Otherwise, we'll only ever return the first frame with that name (and hold it alive) rather than returning the current frame with that name.
(In reply to Bobby Holley (:bholley) from comment #27)
> Created attachment 713954 [details] [diff] [review]
> Part 2 - Make isSafeToUnwrap pseudo-dynamic for SOWs. v1
> 
> Ugh. This gives an extra incentive to try to find a way to get rid of SOWs.

I'm altering this patch to add:

 OnlyIfSubjectIsSystem::isSafeToUnwrap()
 {
+    if (XPCJSRuntime::Get()->XBLScopesEnabled())
+        return false;

Which effectively makes isSafeToUnwrap only dynamic if XBL scopes are off, since they no longer need to be dynamic at all in the new world. I'm carrying over review, but let me know if this is an issue.
(In reply to Blake Kaplan (:mrbkap) from comment #50)
> > +    // Security wrapping always morphs slim wrappers, so we don't have to worry about them here.
> > +    if (!IS_WN_WRAPPER(unwrapped))
> 
> Given the comment (and that we have a wrapper) can we simply assert this?

Well, we still have to handle security wrappers to other things (like new DOM objects). The comment is a bit confusing, but is intended to explain why we don't check IS_SLIM_WRAPPER. I'll update it to assert against slim wrappers.
(In reply to Bobby Holley (:bholley) from comment #69)
> Which effectively makes isSafeToUnwrap only dynamic if XBL scopes are off,
> since they no longer need to be dynamic at all in the new world. I'm
> carrying over review, but let me know if this is an issue.

This looks good.

(In reply to Bobby Holley (:bholley) from comment #70)
> DOM objects). The comment is a bit confusing, but is intended to explain why
> we don't check IS_SLIM_WRAPPER. I'll update it to assert against slim
> wrappers.

Oops, right. That makes more sense, then. I got tunnel vision.
Depends on: 850517
Blocks: 851465
Ok, this should be ready to land. I've given it a single-platform try push as well as an all-platform build push:

https://tbpl.mozilla.org/?tree=Try&rev=c921ef4eb43a
https://tbpl.mozilla.org/?tree=Try&rev=844c5f610c9d
This is green. I'm going to wait for the tree to settle down a bit and then push.
Try push with logging to see if we can get to the bottom of that marionette failure:

https://tbpl.mozilla.org/?tree=Try&rev=fa9a01a8d57f
Looking at the log from this try run, it looks like the test is dying here:

let mozContacts = window.navigator.mozContacts;
Depends on: 853283
Backed out because something in Bobby's mega-push caused Windows debug bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/b53685d06c8d
It appears the issue was a needs-clobber. Re-landed with an update to CLOBBER.
https://hg.mozilla.org/integration/mozilla-inbound/rev/6325487c131e
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff717ac709fa
https://hg.mozilla.org/integration/mozilla-inbound/rev/49b621f262f1
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b4be35b1837
https://hg.mozilla.org/integration/mozilla-inbound/rev/b20ec46f32c6
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac4b41b1020d
https://hg.mozilla.org/integration/mozilla-inbound/rev/7360b1c43300
https://hg.mozilla.org/integration/mozilla-inbound/rev/71a9b2c9e983
https://hg.mozilla.org/integration/mozilla-inbound/rev/d673018a0915
https://hg.mozilla.org/integration/mozilla-inbound/rev/52f0155a3c43
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5d11e2a9602
https://hg.mozilla.org/integration/mozilla-inbound/rev/b2716f274a94
https://hg.mozilla.org/integration/mozilla-inbound/rev/7bccfe486f98
https://hg.mozilla.org/integration/mozilla-inbound/rev/7149b64f78c9
https://hg.mozilla.org/integration/mozilla-inbound/rev/ddd198e0e288
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8ac57d4452f
https://hg.mozilla.org/integration/mozilla-inbound/rev/10c92bbb5f74
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e01a4e68072
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1ced1c166d9
https://hg.mozilla.org/integration/mozilla-inbound/rev/bebb8e1a11b4
https://hg.mozilla.org/integration/mozilla-inbound/rev/bffc0252a627
https://hg.mozilla.org/integration/mozilla-inbound/rev/d4dbfc5c23a3
https://hg.mozilla.org/integration/mozilla-inbound/rev/a83cbe4e0576
Depends on: 854139
Blocks: 854480
Depends on: 854604
Blocks: 856840
Depends on: 880070
You need to log in before you can comment on or make changes to this bug.