837060 - this != window at global context on Web Console

Status: RESOLVED FIXED

(DevTools :: Console, defect, P3)

Description

[Pablo Terradillos]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:20.0) Gecko/20130127 Firefox/20.0
Build ID: 20130127042016

Steps to reproduce:

1. Open Web Console (Tools - Web Developer - Web Console)
2. Write "this === window", "this == window", "this !== window" and "this != window"


Actual results:

this === window returns false
this == window returns false
this !== window returns true
this != window returns true


Expected results:

this === window should return true
this == window should return true
this !== window should return false
this != window should return false

Comment 1

[Mihai Sucan [:msucan]]

This is most-likely a dupe of some other bug.

Issue is known and it is "by design". We are changing how eval works in bug 783499. After that bug lands |this| should be equal to |window|. In my current testing, it's not... but that's because of a different bug which needs more investigation.

Comment 3

[Mihai Sucan [:msucan]]

This bug is still happening. Jim, do you have any ideas why this != window?

Comment 4

[Brandon Benvie [:benvie]]

Another observable difference:

> var wm = new WeakMap
> wm.set(window, 1)
>> undefined
> wm.set(this, 1)
>>TypeError: cannot use the given object as a weak map key

Comment 5

[Jim Blandy :jimb]

That's really weird. The Debugger API's eval functions make you pass a 'this' value explicitly; are we somehow passing a wrapped window?

Comment 6

[Mihai Sucan [:msucan]]

(In reply to Jim Blandy :jimb from comment #5)
> That's really weird. The Debugger API's eval functions make you pass a
> 'this' value explicitly; are we somehow passing a wrapped window?

What I do is I use the |window| object without unwrapping it. I do addDebuggee() then I use the dbgWindow object (the Debugger.Object of |window|) to invoke evalInGlobalWithBindings(strToEval, bindings). In |bindings| I do not set |this|.

If you eval |this| it is the same as |window|: [object Window]. Also when you inspect |this| you can't tell the difference from |window|.

I also believe that unwrapping might help. I tried using window.wrappedJSObject for addDebuggee(), no luck. I also tried XPCNativeWrapper.unwrap(window), still no change. I also tried evalInGlobal("this==window") (without any bindings at all) and the result is always |false|.

Anything else I should try?

Comment 7

[Brandon Benvie [:benvie]]

> (function(){ return this })() == this // false
> (function(){ return this })() == window // true

Comment 8

[Brandon Benvie [:benvie]]

Oh and:

> (function()this)() == (() => this)() // false

Pretty awesome.

Comment 9

[Joe Walker [:jwalker]]

(In reply to Brandon Benvie [:bbenvie] from comment #8)
> Oh and:
> 
> > (function()this)() == (() => this)() // false
> 
> Pretty awesome.

I imagine you saying that in a Gary Bernhardt voice.

Comment 10

[Jim Blandy :jimb]

Oh, I bet I know what's going on here. We're not outer-izing window objects properly.

(Jason, if I explain this wrong, please set me straight... Also, please see the question at the bottom.)

Suppose you have a tab, A, that opens another tab, B, in the same origin. So A has a reference to B's window. HTML5 requires that if B navigates to a new page, then A's reference to its window must now refer to the new page, with all its new properties, functions, and so on. (Let's assume this page too has the same origin, so A is permitted to see its properties.) And, if you navigate B back to the original page, all the original page's globals must reappear on the object (assuming said page is in the B/F cache, of course).

So the effect is as if navigation tears down all B's window's old properties, stashes them somewhere, wipes the window clean, and lets B's new page set up its own world on the same window, for A to see. You don't get a new window object each time B navigates to a new page. As B navigates hither and yon, it's all the same window object --- even though the window object is what hold each specific page's global variables and functions. The window object's identity is more closely tied to the tab than to any specific page.

This suggests an obvious way to implement navigation: simply save all the window's properties off to the side somewhere when we navigate, and then restore them when we navigate back. But that would be... slow? difficult to do correctly? ugly? ... I don't really know, but there were compelling reasons not to do this, even pre-compartments. And when you do introduce compartments, it's flatly unworkable: if you're trying to put different pages' objects in different compartments, you simply cannot use a single object as the global for all of B's pages: which compartment would it be in?

So, what we actually do is split windows into 'outer' and 'inner' window objects. Outer window objects have no properties of their own, but instead proxy all their property references onto an inner window object. Inner window objects actually hold pages' global variables as their properties. JavaScript is only allowed to refer to outer window objects; 'window', the global 'this', and A's reference to B's window are all outer window objects. And finally, navigation just re-points outer window objects to different inner window objects.

So an outer window object really represents a specific tab, pointing at whatever inner is selected in that tab. And an inner window object really represents a specific page, which might or might not be current in its tab. An inner window only has outer windows pointed at it when its page is current.

Obviously, there's exactly one inner window object in each page's compartment. When we say "compartment per global", the inner is the global we mean.

Outer window objects are trickier. Remember that their job is to point to the inner window of whatever page a particular tab is currently displaying. I believe what happens is that each compartment that needs one gets its own outer window object representing a given tab, and navigating that tab re-points all of its extant outer window objects to the new inner window. So outer windows are like cross-compartment wrappers that can change their referents.

Since a page's code is only supposed to run when the tab is actually showing it, we can let scope chains for evaluation point directly to the inner object. So global variable lookup actually doesn't have to deal with outer windows.

(Now, it seems to me that an outer window that was created to pointing to its own compartment's inner window doesn't ever actually need to be re-pointed: whenever its page isn't current, its compartment's code shouldn't be running, so nothing can tell if it's pointed at the right inner window or not. But perhaps we re-point them anyway, for consistency. That would mean that they shift back and forth between being cross-compartment wrappers and intra-compartment proxies; weird. I think this is the case, and we use JS_Transplant for that transition.)

-------

Anyway, back to this bug:

When we add a global as a debuggee, if we've been handed an outer window object, we grab its current inner window and add that as a debuggee. That's the right thing, as the inner window is what appears on scope chains, which is what we really care about when making debuggee/non-debuggee distinctions.

But it looks like Debugger.prototype.addDebuggee then turns around and returns a Debugger.Object instance whose referent is the inner window. If we then pass that as the 'this' value to an invocation function like Debugger.Frame.prototype.evalInFrame or Debugger.Object.prototype.evalInGlobal, then those will dereference the D.O and introduce the raw inner window reference to the debuggee JavaScript code. Something will certainly go wrong eventually. Was this bug re-opened because you tried the === comparison, or because of some other symptom that you simplified to the === comparison?

So the reason for the weird equality results is that the 'this' value is coming from the evalInFrame or evalInGlobal call, and is an inner window, whereas the implicit 'this' value passed to non-strict-mode functions is an outer window, as is 'window'.

Debugger should never leak inner windows. I think the fix is for Debugger to ensure that Debugger.Object instances only ever refer to outer windows. This may require some care in the code that deals with scope chains, but if it's at all practical, we should not expose the inner/outer distinction in Debugger.

Jason, does that sound like a reasonable policy to you?

This is going to be a pain to test, as we can't use js shell or xpcshell tests... grumble.

Comment 11

[Jim Blandy :jimb]

Attachment: Assert that evalInGlobal{,withBindings} doesn't pass inner window objects as 'this' values.

As an experiment, if my guess above is correct, then

1) the bug should not occur if there are debuggee frames on the stack (stopped at a breakpoint, say), because the console uses evalInFrame in that case, which I'm pretty sure computes 'this' correctly, since it just uses the same code everything else does; and

2) with this patch applied, you should get a crash if you try the expressions above.

Note that fixing that assertion isn't enough: the 'eval{,InGlobal}withBindings' and 'call' and 'apply' and 'defineProperty' and 'setVariable' and many other functions will still happily introduce inner windows into the debuggee, which is always wrong.

Comment 12

[Masatoshi Kimura [:emk]]

Nit: Outer windows are called as 'WindowProxy' in the spec.

Comment 13

[Jim Blandy :jimb]

(In reply to Masatoshi Kimura [:emk] from comment #12)
> Nit: Outer windows are called as 'WindowProxy' in the spec.

Right. "Inner" and "outer" windows are SpiderMonkey terminology; see, for example, JS_ObjectToInnerObject and JS_ObjectToOuterObject.

Comment 14

[Jason Orendorff [:jorendorff]]

> Debugger should never leak inner windows. I think the fix is for Debugger to ensure
> that Debugger.Object instances only ever refer to outer windows. This may require
> some care in the code that deals with scope chains, but if it's at all practical,
> we should not expose the inner/outer distinction in Debugger.

Offhand, I think it would be really misleading for object environments to claim their .object is a WindowProxy when actually it's a global. You could have a reference to a Function object and examining its scope could give results that are just wrong.

Also if you ask the Debugger for the list of debuggees, and you're debugging several globals in the same WindowProxy, what should it return? An array with the same WindowProxy several times?

These are two different kinds of object. It's user visible; making it non-debugger-visible would be a mistake.

Honestly I think the fix is just:

 - outerize thisv in DebuggerGenericEval

If you wanted to go just a little further, two more thoughts:

 - inner globals could have a .windowProxy property

 - we could check in unwrapDebuggeeValue, and if someone's trying to pass an inner
   window to user code, which is always a mistake, throw.

While we're here, I should mention that an [[Invoke]] internal method is being added to ES6; it will handle qualified method calls like `x.y(z)`. (There are a few use cases.) I don't know if we want to do anything special in the debugger for that; if we did, it would be another place where we'd need to outerize thisv.

Comment 15

[Jim Blandy :jimb]

Jason and I discussed this a bit on IRC; the conclusions we reached:

- In the immediate term, evalInGlobal{,withBindings} ought to 'outerize' the global before passing it as 'this'.

- In the long term, Debugger.Object instances should have:

  - a .currentGlobal accessor (if the referent is a WindowProxy, return its current
    global), and 

  - a .windowProxy accessor (if the referent is a global, return the WindowProxy to
    which it belongs)

We can't cover up the distinction between WindowProxies and globals, because they have a one-to-many relationship. So we should expose it responsibly.

Comment 16

[Mihai Sucan [:msucan]]

(In reply to Jim Blandy :jimb from comment #11)
> 2) with this patch applied, you should get a crash if you try the
> expressions above.

Indeed. Assertion failure: !IsInnerObject(scope), at /home/mihai/src/mozilla/fx-team/js/src/vm/Debugger.cpp:4199

Comment 17

[Jim Blandy :jimb]

Attachment: Test that Debugger.Object.prototype.evalInGlobal{,withBindings} properly outerizes 'this'.

Comment 18

[Jim Blandy :jimb]

Attachment: Make Debugger.Object.prototype.evalInGlobal{,withBindings} outerize the global before using it as 'this'.

Comment 19

[Jim Blandy :jimb]

Try push for those two patches:
https://tbpl.mozilla.org/?tree=Try&rev=383fc08c96ea

Comment 20

[Jim Blandy :jimb]

Try looks good.

Comment 21

[Jason Orendorff [:jorendorff]]

Comment on attachment 767843 [details] [diff] [review]
Test that Debugger.Object.prototype.evalInGlobal{,withBindings} properly outerizes 'this'.

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

Please also test that:

- the scope chain is *not* outerized,
  that is, g1w.evalInGlobal(someCode) runs in g1's scope
  even if the corresponding WindowProxy has navigated to
  a different global.

- evalInGlobal on an *outer* object fails
  (or whatever it is we decided on).

Comment 22

[Jason Orendorff [:jorendorff]]

Comment on attachment 767845 [details] [diff] [review]
Make Debugger.Object.prototype.evalInGlobal{,withBindings} outerize the global before using it as 'this'.

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

::: js/src/vm/Debugger.cpp
@@ +4222,4 @@
>          if (!env)
>              return false;
>      } else {
> +        thisv = ObjectValue(*GetOuterObject(cx, scope));

Instead, do what js::Execute does just before calling ExecuteKernel:

    JSObject *thisObj = JSObject::thisObject(cx, scopeChain);
    if (!thisObj)
        return false;
    Value thisv = ObjectValue(*thisObj);

Add a comment if you think future readers might need it.

And while you're in here... :) you could remove the declaration of EvaluateInEnv from the header file, right? And then I think you could merge the two functions.

And... if you can stand it... make ExecuteKernel do

  JS_ASSERT_IF(thisv.isObject(),
               GetOuterObject(cx, &thisv.toObject()) == &thisv.toObject());

and let the try server chew on it.

Comment 23

[Masatoshi Kimura [:emk]]

Any progress?

Comment 24

[Jim Blandy :jimb]

(In reply to Jason Orendorff [:jorendorff] from comment #22)
> Instead, do what js::Execute does just before calling ExecuteKernel:
> 
>     JSObject *thisObj = JSObject::thisObject(cx, scopeChain);
>     if (!thisObj)
>         return false;
>     Value thisv = ObjectValue(*thisObj);
> 
> Add a comment if you think future readers might need it.

Done, with comment.

> And while you're in here... :) you could remove the declaration of
> EvaluateInEnv from the header file, right? And then I think you could merge
> the two functions.

That's still used by OldDebugAPI.cpp, unfortunately.

> And... if you can stand it... make ExecuteKernel do
> 
>   JS_ASSERT_IF(thisv.isObject(),
>                GetOuterObject(cx, &thisv.toObject()) == &thisv.toObject());
> 
> and let the try server chew on it.

Done.

Comment 25

[Jim Blandy :jimb]

(In reply to Jason Orendorff [:jorendorff] from comment #21)
> Please also test that:
> 
> - the scope chain is *not* outerized,
>   that is, g1w.evalInGlobal(someCode) runs in g1's scope
>   even if the corresponding WindowProxy has navigated to
>   a different global.

Done; see toolkit/devtools/server/tests/mochitest/test_evalInGlobal-outerized_this.html.

> 
> - evalInGlobal on an *outer* object fails
>   (or whatever it is we decided on).

Done, as above.

Comment 26

[Jim Blandy :jimb]

Attachment: Explain in detail why we reject objects that might appear to be globals.

Comment 27

[Jim Blandy :jimb]

Attachment: Test that Debugger.Object.prototype.evalInGlobal{,withBindings} properly outerizes 'this'.

Comment 28

[Jim Blandy :jimb]

Attachment: Make Debugger.Object.prototype.evalInGlobal{,withBindings} outerize the global before using it as 'this'.

Comment 29

[Jim Blandy :jimb]

Attachment: Make Debugger.Object.prototype.evalInGlobal{,withBindings} outerize the global before using it as 'this'.

Sorry; patch wasn't fresh.

Comment 30

[Jim Blandy :jimb]

Fresh try push for the whole stack of three:
https://tbpl.mozilla.org/?tree=Try&rev=5820469809f6

Comment 31

[Jason Orendorff [:jorendorff]]

Comment on attachment 810196 [details] [diff] [review]
Explain in detail why we reject objects that might appear to be globals.

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

::: js/src/js.msg
@@ +374,5 @@
>  MSG_DEF(JSMSG_MUST_REPORT_UNDEFINED,  321, 0, JSEXN_TYPEERR, "proxy must report undefined for a non-configurable accessor property without a getter")
>  MSG_DEF(JSMSG_CANT_SET_NW_NC,         322, 0, JSEXN_TYPEERR, "proxy can't successfully set a non-writable, non-configurable property")
>  MSG_DEF(JSMSG_CANT_SET_WO_SETTER,     323, 0, JSEXN_TYPEERR, "proxy can't succesfully set an accessor property without a setter")
>  MSG_DEF(JSMSG_DEBUG_BAD_REFERENT,     324, 2, JSEXN_TYPEERR, "{0} does not refer to {1}")
> +MSG_DEF(JSMSG_DEBUG_WRAPPER_IN_WAY,   325, 3, JSEXN_TYPEERR, "{0} is {1}{2}a global object, but a direct reference is required")

"direct reference" is a little misleading here, since what the user really needs to do, in order for their code to work, is pass a Debugger.Object. No concrete suggestion, sorry...

Comment 32

[Jim Blandy :jimb]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f0b167d86c5e
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c590f0469cb
https://hg.mozilla.org/integration/mozilla-inbound/rev/66e0cac2533d

Comment 33

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f0b167d86c5e
https://hg.mozilla.org/mozilla-central/rev/8c590f0469cb
https://hg.mozilla.org/mozilla-central/rev/66e0cac2533d