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

[Gabriel Ivăncescu]

On 16/05/2022 14:51, Jacek Caban wrote:
> 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.
> 

Right, sorry, I was referring to the v2 patches I sent, which don't use 
it anymore. I should have been more explicit.

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

Hmm, I think we have to handle the "non-jsdisp" case since it's also 
triggered when the ctx doesn't match, even if it's a jsdisp. But I found 
an easier way, without extending disp_call at all: since we already 
check for a scope object (or "activation object" as called in spec), we 
know it's a jsdisp without builtins and so we can propget without 
overhead, and then disp_call_value, without having to add DISPID_THIS 
handling in disp_call...

This matches what EXPRVAL_STACK_REF does (without the propget) which is 
in the same function so, I think, it fits.

One thing to note is that we'll have to keep the GLOBAL case in 
exec_source, since this now would have wider reach and fail some 
existing tests (e.g. `var t = dispexFunc; t = t(false);` test).

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

Alright, I'll keep the test then.

Thanks,
Gabriel