Pablo Terradillos Reporter
	Description • 12 years ago

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

	Mihai Sucan [:msucan]
	Comment 1 • 12 years ago

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.

	Mihai Sucan [:msucan]
	Comment 3 • 11 years ago

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

	Brandon Benvie [:benvie]
	Comment 4 • 11 years ago

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

	Jim Blandy :jimb Assignee
	Comment 5 • 11 years ago

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

	Mihai Sucan [:msucan]
	Comment 6 • 11 years ago

(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?

	Brandon Benvie [:benvie]
	Comment 7 • 11 years ago

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

	Brandon Benvie [:benvie]
	Comment 8 • 11 years ago

Oh and:

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

Pretty awesome.

	Joe Walker [:jwalker]
	Comment 9 • 11 years ago

(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.

	Jim Blandy :jimb Assignee
	Comment 10 • 11 years ago

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.

	Jim Blandy :jimb Assignee
	Comment 11 • 11 years ago

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.

	Masatoshi Kimura [:emk]
	Comment 12 • 11 years ago

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

	Jim Blandy :jimb Assignee
	Comment 13 • 11 years ago

(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.

	Jason Orendorff [:jorendorff]
	Comment 14 • 11 years ago

> 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.

	Jim Blandy :jimb Assignee
	Comment 15 • 11 years ago

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.

	Mihai Sucan [:msucan]
	Comment 16 • 11 years ago

(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

	Jim Blandy :jimb Assignee
	Comment 19 • 11 years ago

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

	Jim Blandy :jimb Assignee
	Comment 20 • 11 years ago

Try looks good.

	Jason Orendorff [:jorendorff]
	Comment 21 • 11 years ago

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).

	Jason Orendorff [:jorendorff]
	Comment 22 • 11 years ago

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.

	Masatoshi Kimura [:emk]
	Comment 23 • 11 years ago

Any progress?

	Jim Blandy :jimb Assignee
	Comment 24 • 11 years ago

(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.

	Jim Blandy :jimb Assignee
	Comment 25 • 11 years ago

(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.

	Jim Blandy :jimb Assignee
	Comment 29 • 11 years ago

Sorry; patch wasn't fresh.

	Jim Blandy :jimb Assignee
	Comment 30 • 11 years ago

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

	Jason Orendorff [:jorendorff]
	Comment 31 • 11 years ago

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...

	Jim Blandy :jimb Assignee
	Comment 32 • 11 years ago

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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 33 • 11 years ago

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