[PATCH 1/2] jscript: Handle detached scope objects 'this' in Object.toString.

[Gabriel Ivăncescu]

Hi Jacek,

After analyzing the spec and the current code, I think my patches are 
correct.

So, the patch you attached this time fails because 'window' is not a JS 
object (proxy), so Object.toString returns [object Object] since it's a 
non-JS object, but it *is* passed to it properly as 'window', so I don't 
think there's any problem? It seems it's unrelated. With proxies this is 
fixed, actually. It's the same as Object.prototype.toString.call(window) 
which returns [object Object] right now.

What happens in that test is that 'this' is replaced with the global 
object (window), this.f assigns to the window, so this.f() uses window 
as 'this', which is correct and works as expected.

I've been reading the spec, which seems to differ quite a bit between 
ES3 and ES5 in this matter, but it basically has similar end result for us:

3rd Edition 10.1.6: This mentions the 'Activation Object', which in our 
case is the "abstract" scope, whether detached or not. It mentions:

`The activation object is purely a specification mechanism. It is 
impossible for an ECMAScript program to access the activation object. It 
can access members of the activation object, but not the activation 
object itself.`

...which is what I mentioned earlier that the scope object itself should 
never be accessible. exec_source and the part of the patch I moved 
mentions 11.2.3.7, which says:

`If Result(6) is an activation object, Result(7) is null. Otherwise, 
Result(7) is the same as Result(6).`

this means that the way it's done (checking for scope object, or NONE 
class), it is pretty much correct and follows the spec. The GLOBAL check 
is due to native jscript (not ES5) using two objects for the "global", 
so it has to return the host object instead, if any, instead of the js 
global. This might be a quirk on Windows implementation rather than 
something in the spec, but anyway it's how it works right now.

This is why my patch adds the ES5 workaround for this; once the global 
object *is* both 'window' and the js global, this is cleanly removed.



ES5 is different. 11.2.3.6 and 11.2.3.7 basically say that:

If the evaluated MemberExpression is a "PropertyReference", use its base 
as 'this'. Otherwise, set it to 'undefined'.

(there's also a case where base is an "Environment Record", where it 
uses the "ImplicitThisValue" of the record, but in all cases it's 
'undefined', so it's the same thing, except for 'with' statements, which 
are handled already IIRC).

So what does this mean? As far as I can see, a "PropertyReference" is 
when you do a call like:

a.foo()

instead of

foo()

Both are handled right now in interp_call_member, via exprval_call, but 
the ref type is different. For the former, we have EXPRVAL_IDREF, and 
for latter we have EXPRVAL_STACK_REF.

This actually does the right thing: EXPRVAL_STACK_REF is clearly not a 
"PropertyReference", while EXPRVAL_IDREF is clearly a 
"PropertyReference", and I'm not sure about EXPRVAL_JSVAL. Anyway, the 
former passes NULL to disp_call_value, which is correct for a 
non-PropertyReference. It is fixed by resent patch 2/2, since in ES5 it 
treats it as undefined instead of null, but otherwise works as intended 
(null is pretty much same as undefined for almost all functions, except 
for rare ones like Object.toString).

The latter uses disp_call which ends up calling it with proper 'this'. 
In your patch, this is actually 'window' which is correct.


With that said, I think the patches are correct as-is. They follow 
exactly what the spec says, except that maybe it could be moved to 
interpreter instead of call itself, but I don't think it's worth it. The 
code currently *does* seem to handle the spec properly, it just needs 
some minor tweaks. Do you have other suggestions?

Perhaps do you think it's better to handle this in exprval_call itself 
somehow? Because that's where we *truly* have distinction between 
PropertyReference and other kind of refs. This would need extending 
disp_call, though, I don't think it's worth it as I said.

Also as far as checking for JSCLASS_NONE to figure out if it's a scope 
object: I don't mind any other type of check here, if you have a better 
idea. I just merely copied it from exec_source.

Thanks,
Gabriel