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