Last Comment Bug 771429 - jQuery and $ not available in the console
: jQuery and $ not available in the console
Status: RESOLVED FIXED
[qa+]
: regression
Product: Firefox
Classification: Client Software
Component: Developer Tools: Console (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: Firefox 16
Assigned To: Boris Zbarsky [:bz]
:
Mentors:
Depends on: 780542
Blocks: 771907 726949
  Show dependency treegraph
 
Reported: 2012-07-06 00:03 PDT by Ian Bicking (:ianb)
Modified: 2013-04-22 07:42 PDT (History)
12 users (show)
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
verified
+
verified
+
fixed


Attachments
Instead of using bound functions for the functions we get off the sandbox proto, use a function proxy. That allows property gets on the functions to get through. (5.95 KB, patch)
2012-07-07 21:11 PDT, Boris Zbarsky [:bz]
bobbyholley: review-
Details | Diff | Splinter Review
Patch for beta (9.69 KB, patch)
2012-07-09 08:44 PDT, Boris Zbarsky [:bz]
bobbyholley: review+
Details | Diff | Splinter Review
Beta patch updated to comments (9.69 KB, patch)
2012-07-09 09:27 PDT, Boris Zbarsky [:bz]
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review
Aurora patch (9.64 KB, patch)
2012-07-09 09:36 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review
Aurora patch, compiling (9.64 KB, patch)
2012-07-09 09:38 PDT, Boris Zbarsky [:bz]
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
Instead of using bound functions for the functions we get off the sandbox proto, use a function proxy. That allows property gets on the functions to get through. (9.28 KB, patch)
2012-07-10 08:10 PDT, Boris Zbarsky [:bz]
bobbyholley: review-
Details | Diff | Splinter Review
Instead of using bound functions for the functions we get off the sandbox proto, use a function proxy. That allows property gets on the functions to get through. (9.84 KB, patch)
2012-07-11 13:59 PDT, Boris Zbarsky [:bz]
bobbyholley: review+
Details | Diff | Splinter Review
Interdiff from previous patch (3.24 KB, patch)
2012-07-11 14:00 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review

Description Ian Bicking (:ianb) 2012-07-06 00:03:48 PDT
On a page that uses jQuery (for instance http://jquery.com/) when I use "$" or "jQuery" I get an object or wrapper that is incorrect/incomplete.  $('selector') works, but for instance $.ajax is undefined.  Autocompletion of these properties does work.

When I enter "$" (or "jQuery") into the console it shows "> function () {[native code]}" so I'm guessing some (broken) wrapper is in effect.

Tested on Nightly, 16.0a1 (2012-07-05)
Comment 1 Kevin Dangoor 2012-07-06 08:31:27 PDT
This behavior is not in FF13 but is in 15. This will affect a lot of users.
Comment 2 Mihai Sucan [:msucan] 2012-07-06 11:23:42 PDT
This seems to be caused by bug 726949. I pulled a revision before and after that patch landed. Problem shows when it is applied.

bz / bholley: can you please look into this regression? I am not experienced with the code that changed in that bug. Thank you!
Comment 3 Bobby Holley (:bholley) (busy with Stylo) 2012-07-06 13:08:12 PDT
Appears that, from the web console, $ and window.$ are different objects.

Object.getPrototypeOf($) === this.Function.prototype
Object.getPrototypeOf(window.$) === window.Function.prototype
Comment 4 Bobby Holley (:bholley) (busy with Stylo) 2012-07-06 13:19:06 PDT
Oh right, because it's a bound method.
Comment 5 Bobby Holley (:bholley) (busy with Stylo) 2012-07-06 13:25:42 PDT
Ok, so you can reproduce this with a simple testcase outside of the web console:

$ = $.bind(window);

This is exactly analogous to what we're doing under the hood here. So the first step is for someone to figure out why that breaks jquery.
Comment 6 Mihai Sucan [:msucan] 2012-07-06 13:41:31 PDT
bholley: thank you for looking into this issue.

This happens with any global function in the page. For example, put a <script>function foobarFn() { return true; }</script> the page.

then in the web console try:

window.foobarFn;
foobarFn;

The former works, the latter doesn't.

Also try:

<script>
function foobarFn() { return true; }

foobarFn.fooObj = { boom: "test" };
</script>

Try to evaluate foobarFn.fooObj. I get undefined. If I evaluate window.foobarFn.fooObj, I get the correct result.

The Web Console doesn't try to do anything specific with the globals. We just put the window object as the sandboxPrototype.
Comment 7 Bobby Holley (:bholley) (busy with Stylo) 2012-07-06 13:52:14 PDT
(In reply to Mihai Sucan [:msucan] from comment #6)
> This happens with any global function in the page. For example, put a
> <script>function foobarFn() { return true; }</script> the page.
> 
> then in the web console try:
> 
> window.foobarFn;
> foobarFn;
> 
> The former works, the latter doesn't.

Define 'works'. Do you mean that the function is decompiled? If so, I see what you mean. But both function correctly return true.

> Try to evaluate foobarFn.fooObj. I get undefined. If I evaluate
> window.foobarFn.fooObj, I get the correct result.

Ah! So that's the issue. Right, when we rebind the function, we lose any properties that were defined on that function.
 
> The Web Console doesn't try to do anything specific with the globals. We
> just put the window object as the sandboxPrototype.

Yes, but sandboxPrototype sure does something specific - see bug 726949.

I wonder if we can get away with using the original method as the prototype for the rebound method.
Comment 8 Bobby Holley (:bholley) (busy with Stylo) 2012-07-06 14:08:06 PDT
So, as usual, the prototype strategy would _almost_ work. If we prototyped bound methods to their underlying method, then we'd have |Object.getPrototypeOf($) === window.$|. So we could correctly access |$.ajax === window.$.ajax|. But if we invoked it ($.ajax(...)), the method would end up with the bound function ($) as the this-binding rather than the window.$, which may or may not be ok.

I'm starting to think that this whole rebinding thing is a bit dubious. Maybe instead of bound methods we could return a transparent wrapper with an overridden call() trap? That would allow us to get the this-binding right, but not expose us to this whole mess here. Boris, what do you think?

Also, unless I'm mistaken, this regression is in FF14, which is shipping in a week, yeah? I'm pretty sure the code freeze has passed, so it sounds like we're shipping this regression. :-(
Comment 9 Mihai Sucan [:msucan] 2012-07-06 14:16:03 PDT
Yep, unfortunately, this is going in fx14.

Can't we write a js proxy object for the window object that we then give as the sandboxPrototype? It sounds like it could work, if I understand this correctly. Any property we'd try to access from the sandboxPrototype could go through it.


(In reply to Bobby Holley (:bholley) from comment #7)
> (In reply to Mihai Sucan [:msucan] from comment #6)
> > This happens with any global function in the page. For example, put a
> > <script>function foobarFn() { return true; }</script> the page.
> > 
> > then in the web console try:
> > 
> > window.foobarFn;
> > foobarFn;
> > 
> > The former works, the latter doesn't.
> 
> Define 'works'. Do you mean that the function is decompiled? If so, I see
> what you mean. But both function correctly return true.

works: the function decompiles, as you said.
Comment 10 Bobby Holley (:bholley) (busy with Stylo) 2012-07-06 14:26:32 PDT
(In reply to Mihai Sucan [:msucan] from comment #9)
> Yep, unfortunately, this is going in fx14.
> 
> Can't we write a js proxy object for the window object that we then give as
> the sandboxPrototype? It sounds like it could work, if I understand this
> correctly. Any property we'd try to access from the sandboxPrototype could
> go through it.

Well, we did that, except it was a C++ proxy. The issue here is that we might need a _second_ proxy. The current proxy rebinds all methods it returns to use the window object as the |this| parameter. But this obviously isn't transparent enough.

A scripted proxy _might_ also work (though scripted proxies are less transparent). More importantly though, any other consumers of sandboxPrototype wouldn't get to take advantage of it. So the fix should probably be in C++.
Comment 11 Boris Zbarsky [:bz] 2012-07-06 18:20:43 PDT
Well, ideally we'd only rebind XPConnect and quickstub methods, since those are the ones that can't deal, right?

We could handle non-quickstubbed ones by checking for the native being XPC_WN_CallMethod, but doing that for quickstubs is too much pain, of course.  And similar for DOM bindings.

For methods, I wonder what the obj in the property descriptor will be....  Building now to check.

As far as comment 8, I guess we could do that.  My proxy fu is too weak to tell for sure.
Comment 12 Boris Zbarsky [:bz] 2012-07-07 20:06:46 PDT
OK, so the obj in the property descriptor is, unsurprisingly, a proxy.
Comment 13 Boris Zbarsky [:bz] 2012-07-07 21:11:06 PDT
Created attachment 640019 [details] [diff] [review]
Instead of using bound functions for the functions we get off the sandbox proto, use a function proxy.  That allows property gets on the functions to get through.
Comment 14 Bobby Holley (:bholley) (busy with Stylo) 2012-07-08 03:20:09 PDT
I'm probably the best person to review this. Is it ready?
Comment 15 Boris Zbarsky [:bz] 2012-07-08 08:31:32 PDT
Comment on attachment 640019 [details] [diff] [review]
Instead of using bound functions for the functions we get off the sandbox proto, use a function proxy.  That allows property gets on the functions to get through.

Uh....  I thought I requested review from you when I uploaded it!  Yes, it's ready. Afaict.  Still cycling on try.
Comment 16 Alex Keybl [:akeybl] 2012-07-08 09:20:03 PDT
Does a forward fix make the most sense here? Perhaps we should just consider backing out bug 726949 (and bug 622301?) from Beta for sake of risk mitigation.
Comment 17 Boris Zbarsky [:bz] 2012-07-08 09:39:03 PDT
I think backing out bug 622301 might also involve backing out (or at least turning off) all the various DOM bindings stuff that happened in 14.  I suspect that that's riskier than this targeted fix.  For one thing, it affects web-facing code, whereas this patch only affects sandboxes....
Comment 18 Boris Zbarsky [:bz] 2012-07-08 11:54:46 PDT
Try looks good.
Comment 19 Bobby Holley (:bholley) (busy with Stylo) 2012-07-09 02:58:59 PDT
Comment on attachment 640019 [details] [diff] [review]
Instead of using bound functions for the functions we get off the sandbox proto, use a function proxy.  That allows property gets on the functions to get through.


>+// A proxy handler that lets us wrap callables and invoke them with
>+// the correct this object, while forwarding all other operations down
>+// to them directly.
>+class SandboxCallableProxyHandler : public js::AbstractWrapper {

This should inherit IndirectProxyHandler, per a patch that just landed in bug 703537. It could also inherit IndirectWrapper (IndirectProxyHandler + enter/leave traps), but we've recently decided that IndirectWrapper is useless and should be removed.


This might also involve changing a few cases of "wrappedObject" to "GetProxyTarget". The basic idea is that now we have an inheritance chain of 3 classes. BaseProxyHandler is generic, with fundamental traps as pure virtual and derived traps in terms of fundamental traps. IndirectProxyHandler : public BaseProxyHandler implements the fundamental traps to forward to a target object. DirectProxyHandler : public IndirectProxyHandler reimplements the derived traps to forward directly.

We're making this change to better match the ES5 direct proxies spec, which includes the notion of a target object. So now the only unique thing about wrappers are the policy enforcement traps. We might end up renaming them at this point. Sorry this stuff is in flux at the moment. :-(

>+public:
>+    SandboxCallableProxyHandler() : js::AbstractWrapper(0)
>+    {
>+    }
>+
>+    virtual bool call(JSContext *cx, JSObject *proxy, unsigned argc,
>+                      Value *vp);
>+};
>+
>+bool
>+SandboxCallableProxyHandler::call(JSContext *cx, JSObject *proxy, unsigned argc,
>+                                  Value *vp)
>+{
>+    // We forward the call to our underlying callable.  The callable
>+    // to forward to can be gotten via GetProxyCall.  The object it's
>+    // effectively bound to can be gotten via GetProxyExtra(0).
>+    return JS::Call(cx, js::GetProxyExtra(proxy, 0), js::GetProxyCall(proxy),
>+                    argc, JS_ARGV(cx, vp), vp);
>+}

I think we can do even better here. I think we should avoid rebinding |this| if the function was invoked without a |this| object. In fact, I think we should _only_ rebind if |this| is equal to the sandbox global. This seems to me like it would buy us 100% transparency - the objects would behave as usual, _except_ in the precise cases where we want to rebind |this|.


>+
>+static SandboxCallableProxyHandler sandboxCallableProxyHandler;
>+
>+static JSObject*
>+BindCallable(JSContext *cx, JSObject *callable, JSObject *targetObj)

Given my suggestion to bind conditionally, this isn't a perfect function name. But it's probably fine. Also, |targetObj| might be better as |thisObj| or something.

>+{
>+    MOZ_ASSERT(JS_ObjectIsCallable(cx, callable));
>+    // Our proxy is wrapping the callable.  So we need to use the
>+    // callable as the private.  We also use it as the parent (XXXbz
>+    // is there a better choice?)

Maybe the SandboxProxyHandler? I don't think it really matters.

and we need to pass it in as the
>+    // "call" so we get a function proxy.
>+    //
>+    // The object we're bound to goes in the proxy extra slot.
>+    JSObject* proxy =  js::NewProxyObject(cx, &sandboxCallableProxyHandler,
>+                                          ObjectValue(*callable), nsnull,
>+                                          callable, callable);
>+    if (!proxy) {
>+        return nsnull;
>+    }
>+
>+    js::SetProxyExtra(proxy, 0, ObjectValue(*targetObj));
>+    return proxy;
>+}
>+
> template<typename Op>
> bool BindPropertyOp(JSContext *cx, JSObject *targetObj, Op& op,
>                     PropertyDescriptor *desc, jsid id, unsigned attrFlag)
> {
>     if (!op) {
>         return true;
>     }
> 
>@@ -3074,17 +3121,17 @@ bool BindPropertyOp(JSContext *cx, JSObj
>     } else {
>         // We have an actual property op.  For getters, we use 0
>         // args, for setters we use 1 arg.
>         uint32_t args = (attrFlag == JSPROP_GETTER) ? 0 : 1;
>         func = GeneratePropertyOp(cx, desc->obj, id, args, op);
>         if (!func)
>             return false;
>     }
>-    func = JS_BindCallable(cx, func, targetObj);
>+    func = BindCallable(cx, func, targetObj);
>     if (!func)
>         return false;
>     op = JS_DATA_TO_FUNC_PTR(Op, func);
>     desc->attrs |= attrFlag;
>     return true;
> }
> 
> extern JSBool
>@@ -3127,17 +3174,17 @@ xpc::SandboxProxyHandler::getPropertyDes
>         return false;
>     if (desc->setter != xpc::holder_set &&
>         desc->setter != XPC_WN_Helper_SetProperty &&
>         !BindPropertyOp(cx, obj, desc->setter, desc, id, JSPROP_SETTER))
>         return false;
>     if (desc->value.isObject()) {
>         JSObject* val = &desc->value.toObject();
>         if (JS_ObjectIsCallable(cx, val)) {
>-            val = JS_BindCallable(cx, val, obj);
>+            val = BindCallable(cx, val, obj);
>             if (!val)
>                 return false;
>             desc->value = ObjectValue(*val);
>         }
>     }
> 
>     return true;
> }

>diff --git a/js/xpconnect/tests/chrome/test_bug771429.xul b/js/xpconnect/tests/chrome/test_bug771429.xul


>+      function f() {}
>+      f.x = 2;
>+      var Cu = Components.utils;
>+      var s = new Cu.Sandbox(window, { sandboxPrototype: window } );
>+      try {
>+        var t = Cu.evalInSandbox('f.x', s);
>+        is(t, 2, "Should have gotten the right thing back");
>+      } catch (e) {
>+        ok(false, "Should not get an exception: " + e);
>+      }

Can you also add |f.g = function() { return this; }| and |is(Cu.evalInSandbox('f.g()', s), f)| to make sure things work properly apropos comment 8?

And also, assuming we go with the more fine-grained rebinding approach I suggested, some test coverage for that would be good as well.
Comment 20 Boris Zbarsky [:bz] 2012-07-09 07:03:08 PDT
> per a patch that just landed in bug 703537.

That doesn't help for aurora and beta, does it?  I would like to focus on a patch that works there, in addition to something for m-c.

> In fact, I think we should _only_ rebind if |this| is equal to the sandbox global.

I'll take a look.  Time is kinda tight for getting this into beta in the next two hours here.

> Maybe the SandboxProxyHandler?

As the parent?  It's totally the wrong sort of object, no?  Like "not a JSObject".  Unless you meant the sandboxproxy itself, not its handler?
Comment 21 Bobby Holley (:bholley) (busy with Stylo) 2012-07-09 07:42:35 PDT
(In reply to Boris Zbarsky (:bz) from comment #20)
> > per a patch that just landed in bug 703537.
> 
> That doesn't help for aurora and beta, does it?  I would like to focus on a
> patch that works there, in addition to something for m-c.

Yeah, sure. This patch won't apply to m-c though.

> > In fact, I think we should _only_ rebind if |this| is equal to the sandbox global.
> 
> I'll take a look.  Time is kinda tight for getting this into beta in the
> next two hours here.

Oh, I thought the ship had already sailed there. If not, by all means, r=bholley on anything that improves the current situation. Though this should be a pretty easy change.


> > Maybe the SandboxProxyHandler?
> 
> As the parent?  It's totally the wrong sort of object, no?  Like "not a
> JSObject".  Unless you meant the sandboxproxy itself, not its handler?

Er, yes, sandboxproxy. But either was is fine. AFAICT. I wouldn't bother with it.
Comment 22 Boris Zbarsky [:bz] 2012-07-09 08:44:17 PDT
Created attachment 640236 [details] [diff] [review]
Patch for beta
Comment 23 Bobby Holley (:bholley) (busy with Stylo) 2012-07-09 09:15:48 PDT
Comment on attachment 640236 [details] [diff] [review]
Patch for beta


>+class SandboxCallableProxyHandler : public js::AbstractWrapper {

As noted on IRC, we should just be using js::Wrapper here.

>+bool
>+SandboxCallableProxyHandler::call(JSContext *cx, JSObject *proxy, unsigned argc,
>+                                  Value *vp)
>+{
>+    // We forward the call to our underlying callable. The callable to forward
>+    // to can be gotten via GetReservedSlot(proxy, JSSLOT_PROXY_CALL).  If our
>+    // this object is hanging out in GetProxyExtra(0), we rebind to the object
>+    // in GetProxyExtra(1).
>+    if (JS_THIS(cx, vp) == js::GetProxyExtra(proxy, 0)) {
>+        return JS::Call(cx, js::GetProxyExtra(proxy, 1),
>+                        js::GetReservedSlot(proxy, JSSLOT_PROXY_CALL),
>+                        argc, JS_ARGV(cx, vp), vp);
>+    }
>+
>+    // Just forward the call directly
>+    return JS::Call(cx, JS_THIS_VALUE(cx, vp),
>+                    js::GetReservedSlot(proxy, JSSLOT_PROXY_CALL), argc,
>+                    JS_ARGV(cx, vp), vp);

Just store |thisVal| as a local variable and avoid duplicating the call to JS::Call?

>+}
>+
>+static SandboxCallableProxyHandler sandboxCallableProxyHandler;
>+
>+// Wrap a callable such that if we're called with oldThisObj as the
>+// "this" we will instead call it with newThisObj as the this.
>+static JSObject*
>+WrapCallable(JSContext *cx, JSObject *callable, JSObject *oldThisObj,
>+             JSObject *newThisObj, JSObject *parentObj)
>+{
>+    MOZ_ASSERT(JS_ObjectIsCallable(cx, callable));
>+    // Our proxy is wrapping the callable.  So we need to use the
>+    // callable as the private.  We use the given parentObj as the
>+    // parent.
>+    //
>+    // We need to pass the given callable in as the "call" so we get a
>+    // function proxy.
>+    //
>+    // The oldThisObj object goes in the 0th proxy extra slot.
>+    //
>+    // The newThisObj goes in the 1st proxy extra slot.
>+    JSObject* proxy =  js::NewProxyObject(cx, &sandboxCallableProxyHandler,
>+                                          ObjectValue(*callable), nsnull,
>+                                          parentObj, callable);

Should we also pass the |callable| as a construct hook here rather than leaving it null?

>+    if (!proxy) {
>+        return nsnull;
>+    }
>+
>+    js::SetProxyExtra(proxy, 0, ObjectValue(*oldThisObj));
>+    js::SetProxyExtra(proxy, 1, ObjectValue(*newThisObj));
>+    return proxy;
>+}
>+
>+bool BindPropertyOp(JSContext *cx, JSObject *oldThisObj, JSObject*newThisObj,

Spacing?

> extern JSBool
>@@ -3123,28 +3188,34 @@ xpc::SandboxProxyHandler::getPropertyDes
>     // the "vp is prefilled with the value in the slot" behavior that property
>     // ops can in theory rely on, but our property op forwarder doesn't know how
>     // to make that happen.  Since we really only need to rebind the DOM methods
>     // here, not rebindings holder_get and holder_set is OK.
>     //
>     // Similarly, don't mess with XPC_WN_Helper_GetProperty and
>     // XPC_WN_Helper_SetProperty, for the same reasons: that could confuse our
>     // access to expandos when we're not doing Xrays.
>+    //
>+    // Note: the proxy's parent is the sandbox global, which is exactly what we
>+    // want for oldThisObj for WrapCallable and BindPropertyOp.  We want |obj|
>+    // as the newThisObj.
>     if (desc->getter != xpc::holder_get &&
>         desc->getter != XPC_WN_Helper_GetProperty &&
>-        !BindPropertyOp(cx, obj, desc->getter, desc, id, JSPROP_GETTER))
>+        !BindPropertyOp(cx, JS_GetParent(proxy), obj, desc->getter, desc, id,
>+                        JSPROP_GETTER, proxy))
>         return false;
>     if (desc->setter != xpc::holder_set &&
>         desc->setter != XPC_WN_Helper_SetProperty &&
>-        !BindPropertyOp(cx, obj, desc->setter, desc, id, JSPROP_SETTER))
>+        !BindPropertyOp(cx, JS_GetParent(proxy), obj, desc->setter, desc, id,
>+                        JSPROP_SETTER, proxy))
>         return false;
>     if (desc->value.isObject()) {
>         JSObject* val = &desc->value.toObject();
>         if (JS_ObjectIsCallable(cx, val)) {
>-            val = JS_BindCallable(cx, val, obj);
>+            val = WrapCallable(cx, val, JS_GetParent(proxy), obj, proxy);
>             if (!val)
>                 return false;
>             desc->value = ObjectValue(*val);

I would prefer to store oldThisObject and newThisObject directly as locals. This would also give us an opportunity to assert that oldThisObject has the sandbox global class.

r=bholley for m-a and m-b with those comments addressed.
Comment 24 Boris Zbarsky [:bz] 2012-07-09 09:26:42 PDT
> Should we also pass the |callable| as a construct hook here rather than leaving it null?

I tried it both ways, but could not see a difference.  I guess I can pass it.

Applied the other changes.
Comment 25 Boris Zbarsky [:bz] 2012-07-09 09:27:36 PDT
Created attachment 640246 [details] [diff] [review]
Beta patch updated to comments
Comment 26 Boris Zbarsky [:bz] 2012-07-09 09:29:10 PDT
Comment on attachment 640246 [details] [diff] [review]
Beta patch updated to comments

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 726949 
User impact if declined: Incorrect evaluation of some expressions in web console
Testing completed (on m-c, etc.): Passes xpconnect tests.
Risk to taking this patch (and alternatives if risky): Risk should be low.  The
   other option is to just leave this unfixed for Firefox 14.
String or UUID changes made by this patch:
Comment 27 Boris Zbarsky [:bz] 2012-07-09 09:36:56 PDT
Created attachment 640250 [details] [diff] [review]
Aurora patch
Comment 28 Boris Zbarsky [:bz] 2012-07-09 09:38:54 PDT
Created attachment 640251 [details] [diff] [review]
Aurora patch, compiling
Comment 29 Boris Zbarsky [:bz] 2012-07-09 09:40:46 PDT
Note to self: for m-c Bobby would like me to get oldThisObj and newThisObj off the function proxy's parent (which is the sandbox proxy).  One is its parent, the other is its wrapped object.
Comment 30 Alex Keybl [:akeybl] 2012-07-09 14:04:57 PDT
Comment on attachment 640246 [details] [diff] [review]
Beta patch updated to comments

[Triage Comment]
Approving based upon test coverage, the low risk profile, and developer impact.
Comment 32 Boris Zbarsky [:bz] 2012-07-09 14:46:46 PDT
Eddy, Bobby,

On m-c trying to inherit from Wrapper gets me:

  note: unimplemented pure virtual method 'toBaseProxyHandler' in
  'SandboxCallableProxyHandler'

which makes sense given the jswrapper.h bits.  What _should_ I actually be inheriting from?
Comment 33 Bobby Holley (:bholley) (busy with Stylo) 2012-07-10 02:21:23 PDT
(In reply to Boris Zbarsky (:bz) from comment #32)
> which makes sense given the jswrapper.h bits.  What _should_ I actually be
> inheriting from?

DirectWrapper. We'll be fixing this up again shortly over in bug 772138 (sorry for all the confusion, we decided that we'd made things to complicated), but in the mean time DirectWrapper is the equivalent of the old Wrapper.
Comment 34 Boris Zbarsky [:bz] 2012-07-10 08:10:53 PDT
Created attachment 640607 [details] [diff] [review]
Instead of using bound functions for the functions we get off the sandbox proto, use a function proxy.  That allows property gets on the functions to get through.
Comment 35 Bobby Holley (:bholley) (busy with Stylo) 2012-07-10 15:01:41 PDT
Comment on attachment 640607 [details] [diff] [review]
Instead of using bound functions for the functions we get off the sandbox proto, use a function proxy.  That allows property gets on the functions to get through.

>+    JSObject *thisObj = JS_THIS_OBJECT(cx, vp);
>+    if (!thisObj) {
>+        return false;
>+    }

It's valid to have an undefined this binding in ES5 strict mode:

"use strict"; (function() { alert(this); })()

Looks good otherwise.
Comment 36 Boris Zbarsky [:bz] 2012-07-10 18:58:56 PDT
OK, then we have a problem: I don't know how to do the comparison we want here.

In particular, JS_THIS_VALUE will return JSVAL_VOID for global function calls, but JS_THIS and JS_THIS_OBJECT will do non-strict boxing.  And if non-strict boxing returns null that means it threw, and we have to bail out.

Any ideas?  It sounds like what we really want is to pass through stuff without any rebinding if callee is a strict-mode scripted function.  Or something.
Comment 37 Bobby Holley (:bholley) (busy with Stylo) 2012-07-11 01:37:25 PDT
Shouldn't we just do JS_THIS_VALUE and compare it with ObjectValue(*oldTarget)? That seems like it would implicitly handle the JSVAL_VOID case (in which I think we should forward along JSVAL_VOID as the this-binding).
Comment 38 Boris Zbarsky [:bz] 2012-07-11 10:04:29 PDT
Hmm.  That sorta happens to work because the first place JS_THIS_OBJECT is called is the quickstub, and at that point the callee global is the window.  I guess we could do that and add a test can call that good enough....
Comment 39 Bobby Holley (:bholley) (busy with Stylo) 2012-07-11 13:57:45 PDT
Doesn't the same case apply when we call a regular dom method without a |this| binding in strict-mode? it seems to me like we shouldn't have to be doing any special thinking about this in the sandbox proxy case - we should just be remapping the one thing we care about, and passing everything else as-is.
Comment 40 Boris Zbarsky [:bz] 2012-07-11 13:59:20 PDT
Created attachment 641188 [details] [diff] [review]
Instead of using bound functions for the functions we get off the sandbox proto, use a function proxy.  That allows property gets on the functions to get through.
Comment 41 Boris Zbarsky [:bz] 2012-07-11 14:00:26 PDT
Created attachment 641189 [details] [diff] [review]
Interdiff from previous patch
Comment 42 Boris Zbarsky [:bz] 2012-07-11 14:03:42 PDT
> Doesn't the same case apply when we call a regular dom method without a |this| binding in
> strict-mode?

Normally this happens without crossing globals.  Here it involves a proxy that moves the call across globals without ever computing this, which is why it ends up working.
Comment 43 Bobby Holley (:bholley) (busy with Stylo) 2012-07-11 14:09:48 PDT
Comment on attachment 641188 [details] [diff] [review]
Instead of using bound functions for the functions we get off the sandbox proto, use a function proxy.  That allows property gets on the functions to get through.

r=bholley. Thanks for the interdiff.
Comment 45 Ryan VanderMeulen [:RyanVM] 2012-07-14 10:03:13 PDT
https://hg.mozilla.org/mozilla-central/rev/016584d9d91b
Comment 46 Ioana (away) 2012-07-18 09:14:25 PDT
Verified as fixed on:
Mozilla/5.0 (X11; Linux x86_64; rv:14.0) Gecko/20100101 Firefox/14.0
Mozilla/5.0 (Windows NT 5.1; rv:14.0) Gecko/20100101 Firefox/14.0
Mozilla/5.0 (X11; Linux x86_64; rv:15.0) Gecko/20100101 Firefox/15.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:15.0) Gecko/20100101 Firefox/15.0
Mozilla/5.0 (Windows NT 5.1; rv:15.0) Gecko/20100101 Firefox/15.0

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