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

[Jacek Caban]

Hi Gabriel,

On 5/12/22 19:09, Gabriel Ivăncescu wrote:
> 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.`


Yes, exactly, this is why fixing it in toString does not look right, it 
should never reach this function in the first place.


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


Note that all of above describe caller side of things, while your 
patches modify function implementation. I think that this should be 
handled before it leaves the interpreter, as per spec.


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


Yes, I expect that it would be better, that seems to be the actual 
source of those problems. It should be as simple as adding an explicit 
jsthis argument to disp_call (although in this case it may be fine to 
ignore it for non-jsdisp cases), we already have it in disp_call_value. 
We could even consider passing jsthis as jsval to *disp_call* functions.


Also, particular test aside, the case of 'this.f()' is still something 
that may matter and I'd rather make sure we don't break it. Note that, 
unlike activation objects, 'this' builtin global objects can be exposed 
to scripts when host doesn't provide one.


In general, it would be good to include all tests instead of dropping 
ones that don't match your solution. For cases like 'this.f()' it's fine 
to have it with todo_wine for now.


Thanks,

Jacek