[PATCH 1/2] jscript: Handle detached scope objects 'this' in Object.toString.
Object.prototype.toString is unique in that it's a builtin that accepts any value as 'this', even null or undefined or non-JS objects (and returns [object Object] for them). However, for detached scopes, it returns JS_E_OBJECT_EXPECTED. This is important because it shows that builtins don't follow the path specified in `ECMA-262 3rd Edition 11.2.3.7` (which is handled in exec_source now) which would otherwise replace 'this' with null, and toString can handle null just fine. Without this, the assertion triggers. Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- Note that ES5 is different (next patch). dlls/jscript/object.c | 3 ++- dlls/jscript/tests/api.js | 2 ++ dlls/jscript/tests/lang.js | 3 +++ 3 files changed, 7 insertions(+), 1 deletion(-) diff --git a/dlls/jscript/object.c b/dlls/jscript/object.c index a675e45..453ea95 100644 --- a/dlls/jscript/object.c +++ b/dlls/jscript/object.c @@ -73,8 +73,9 @@ static HRESULT Object_toString(script_ctx_t *ctx, jsval_t vthis, WORD flags, uns str = L"[object Object]"; }else if(names[jsdisp->builtin_info->class]) { str = names[jsdisp->builtin_info->class]; + }else if(jsdisp->builtin_info->class == JSCLASS_NONE) { + hres = JS_E_OBJECT_EXPECTED; }else { - assert(jsdisp->builtin_info->class != JSCLASS_NONE); FIXME("jsdisp->builtin_info->class = %d\n", jsdisp->builtin_info->class); hres = E_FAIL; } diff --git a/dlls/jscript/tests/api.js b/dlls/jscript/tests/api.js index e81bbea..85677a1 100644 --- a/dlls/jscript/tests/api.js +++ b/dlls/jscript/tests/api.js @@ -2634,6 +2634,8 @@ testException(function() {null.toString();}, "E_OBJECT_EXPECTED"); testException(function() {RegExp.prototype.toString.call(new Object());}, "E_REGEXP_EXPECTED"); testException(function() {/a/.lastIndex();}, "E_NOT_FUNC"); testException(function() {"a".length();}, "E_NOT_FUNC"); +testException(function() {((function() { var f = Number.prototype.toString; return (function() { return f(); }); })())();}, "E_NOT_NUM"); +testException(function() {((function() { var f = Object.prototype.toString; return (function() { return f(); }); })())();}, "E_OBJECT_EXPECTED"); testException(function() { return arguments.callee(); }, "E_STACK_OVERFLOW"); diff --git a/dlls/jscript/tests/lang.js b/dlls/jscript/tests/lang.js index cf08423..67e4576 100644 --- a/dlls/jscript/tests/lang.js +++ b/dlls/jscript/tests/lang.js @@ -310,6 +310,9 @@ argumentsTest(); ok(callAsExprTest.arguments === null, "callAsExprTest.arguments = " + callAsExprTest.arguments); })(1,2); +tmp = ((function() { var f = function() {return this}; return (function() { return f(); }); })())(); +ok(tmp === this, "detached scope function call this != global this"); + tmp = (function() {1;})(); ok(tmp === undefined, "tmp = " + tmp); tmp = eval("1;"); -- 2.34.1
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/jscript/function.c | 18 ++++++++++++++++-- dlls/mshtml/tests/es5.js | 15 +++++++++++++++ 2 files changed, 31 insertions(+), 2 deletions(-) diff --git a/dlls/jscript/function.c b/dlls/jscript/function.c index 12511bb..d82482c 100644 --- a/dlls/jscript/function.c +++ b/dlls/jscript/function.c @@ -247,16 +247,30 @@ void detach_arguments_object(jsdisp_t *args_disp) jsdisp_release(frame->arguments_obj); } +/* ECMA-262 5.1 Edition 11.2.3.6 */ +static jsval_t setup_this(script_ctx_t *ctx, jsval_t vthis) +{ + jsdisp_t *jsdisp; + + if(ctx->version < SCRIPTLANGUAGEVERSION_ES5) + return vthis; + if(is_object_instance(vthis) && (jsdisp = to_jsdisp(get_object(vthis))) && jsdisp->builtin_info->class == JSCLASS_NONE) + return jsval_undefined(); + return vthis; +} + HRESULT Function_invoke(jsdisp_t *func_this, IDispatch *jsthis, WORD flags, unsigned argc, jsval_t *argv, jsval_t *r) { FunctionInstance *function; + jsval_t vthis; TRACE("func %p this %p\n", func_this, jsthis); assert(is_class(func_this, JSCLASS_FUNCTION)); function = function_from_jsdisp(func_this); - return function->vtbl->call(function->dispex.ctx, function, jsthis ? jsval_disp(jsthis) : jsval_null(), flags, argc, argv, r); + vthis = setup_this(function->dispex.ctx, jsthis ? jsval_disp(jsthis) : jsval_null()); + return function->vtbl->call(function->dispex.ctx, function, vthis, flags, argc, argv, r); } static HRESULT Function_get_length(script_ctx_t *ctx, jsdisp_t *jsthis, jsval_t *r) @@ -483,7 +497,7 @@ HRESULT Function_value(script_ctx_t *ctx, jsval_t vthis, WORD flags, unsigned ar return E_FAIL; } - return function->vtbl->call(ctx, function, vthis, flags, argc, argv, r); + return function->vtbl->call(ctx, function, setup_this(ctx, vthis), flags, argc, argv, r); } HRESULT Function_get_value(script_ctx_t *ctx, jsdisp_t *jsthis, jsval_t *r) diff --git a/dlls/mshtml/tests/es5.js b/dlls/mshtml/tests/es5.js index 8238e5d..fb756ac 100644 --- a/dlls/mshtml/tests/es5.js +++ b/dlls/mshtml/tests/es5.js @@ -1784,6 +1784,21 @@ sync_test("let scope instances", function() { ok(f() == 2, "f() = " + f()); }); +sync_test("builtins detached scope this", function() { + try { + ((function() { var f = Number.prototype.toString; return (function() { return f(); }); })())(); + }catch(ex) { + var n = ex.number >>> 0; + ok(n === JS_E_NUMBER_EXPECTED, "Number.toString threw " + n); + } + + var r = ((function() { var f = Object.prototype.toString; return (function() { return f(); }); })())(); + ok(r === "[object Undefined]", "Object.toString returned " + r); + + var r = ((function() { return (function() { return this; }); })())(); + ok(r === window, "detached scope this = " + r); +}); + sync_test("functions scope", function() { function f(){ return 1; } function f(){ return 2; } -- 2.34.1
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=114136 Your paranoid android. === w8adm (32 bit report) === mshtml: events.c:1089: Test failed: unexpected call img_onerror events: Timeout === w10pro64_en_AE_u8 (64 bit report) === mshtml: htmldoc.c:2541: Test failed: unexpected call UpdateUI htmldoc.c:2853: Test failed: unexpected call Exec_UPDATECOMMANDS htmldoc.c:350: Test failed: expected Exec_SETTITLE htmldoc.c:2859: Test failed: unexpected call Exec_SETTITLE === w7u_el (32 bit report) === mshtml: script.c:644: Test failed: L"/index.html?es5.js:date_now: unexpected Date.now() result 1651682004099 expected 1651682004162"
Hi Gabriel, On 5/4/22 18:10, Gabriel Ivăncescu wrote:
diff --git a/dlls/jscript/object.c b/dlls/jscript/object.c index a675e45..453ea95 100644 --- a/dlls/jscript/object.c +++ b/dlls/jscript/object.c @@ -73,8 +73,9 @@ static HRESULT Object_toString(script_ctx_t *ctx, jsval_t vthis, WORD flags, uns str = L"[object Object]"; }else if(names[jsdisp->builtin_info->class]) { str = names[jsdisp->builtin_info->class]; + }else if(jsdisp->builtin_info->class == JSCLASS_NONE) { + hres = JS_E_OBJECT_EXPECTED; }else { - assert(jsdisp->builtin_info->class != JSCLASS_NONE); FIXME("jsdisp->builtin_info->class = %d\n", jsdisp->builtin_info->class); hres = E_FAIL; }
It seems like something is still not working as intended and I'm not sure if it's the right place to fix it. Why would it be specific to detached scopes? I was wondering why you need so complicated test and tried the attached one. It works fine on Windows, but fails with your patch. Thanks, Jacek
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=114224 Your paranoid android. === build (build log) === error: patch failed: dlls/jscript/tests/api.js:2636 Task: Patch failed to apply === debian11 (build log) === error: patch failed: dlls/jscript/tests/api.js:2636 Task: Patch failed to apply === debian11 (build log) === error: patch failed: dlls/jscript/tests/api.js:2636 Task: Patch failed to apply
On 05/05/2022 18:18, Jacek Caban wrote:
Hi Gabriel,
On 5/4/22 18:10, Gabriel Ivăncescu wrote:
diff --git a/dlls/jscript/object.c b/dlls/jscript/object.c index a675e45..453ea95 100644 --- a/dlls/jscript/object.c +++ b/dlls/jscript/object.c @@ -73,8 +73,9 @@ static HRESULT Object_toString(script_ctx_t *ctx, jsval_t vthis, WORD flags, uns str = L"[object Object]"; }else if(names[jsdisp->builtin_info->class]) { str = names[jsdisp->builtin_info->class]; + }else if(jsdisp->builtin_info->class == JSCLASS_NONE) { + hres = JS_E_OBJECT_EXPECTED; }else { - assert(jsdisp->builtin_info->class != JSCLASS_NONE); FIXME("jsdisp->builtin_info->class = %d\n", jsdisp->builtin_info->class); hres = E_FAIL; }
It seems like something is still not working as intended and I'm not sure if it's the right place to fix it. Why would it be specific to detached scopes? I was wondering why you need so complicated test and tried the attached one. It works fine on Windows, but fails with your patch.
Thanks,
Jacek
Interesting, I need to investigate it more, but I'm pretty sure that detached scopes are not supposed to be actual objects ever exposed to jscript code, which is why it's the way it is. Honestly, I don't particularly care about toString, it's just the only builtin I could think off the top of my head that could be tested for this. Because right now we handle it in exec_source, which is only for interpreted functions... but it's a good test case of such behavior. Do you think a fix like in 2/2 is more appropriate (also for quirks modes, but with null instead of undefined)? And perhaps fix whatever is causing toString to fail, which seems unrelated I guess. That might mean moving it out of exec_source though. Thanks, Gabriel
On 5/5/22 19:01, Gabriel Ivăncescu wrote:
On 05/05/2022 18:18, Jacek Caban wrote:
Hi Gabriel,
On 5/4/22 18:10, Gabriel Ivăncescu wrote:
diff --git a/dlls/jscript/object.c b/dlls/jscript/object.c index a675e45..453ea95 100644 --- a/dlls/jscript/object.c +++ b/dlls/jscript/object.c @@ -73,8 +73,9 @@ static HRESULT Object_toString(script_ctx_t *ctx, jsval_t vthis, WORD flags, uns str = L"[object Object]"; }else if(names[jsdisp->builtin_info->class]) { str = names[jsdisp->builtin_info->class]; + }else if(jsdisp->builtin_info->class == JSCLASS_NONE) { + hres = JS_E_OBJECT_EXPECTED; }else { - assert(jsdisp->builtin_info->class != JSCLASS_NONE); FIXME("jsdisp->builtin_info->class = %d\n", jsdisp->builtin_info->class); hres = E_FAIL; }
It seems like something is still not working as intended and I'm not sure if it's the right place to fix it. Why would it be specific to detached scopes? I was wondering why you need so complicated test and tried the attached one. It works fine on Windows, but fails with your patch.
Thanks,
Jacek
Interesting, I need to investigate it more, but I'm pretty sure that detached scopes are not supposed to be actual objects ever exposed to jscript code, which is why it's the way it is.
Yes, that's why it would be interesting to understand the root of the problem.
Honestly, I don't particularly care about toString, it's just the only builtin I could think off the top of my head that could be tested for this. Because right now we handle it in exec_source, which is only for interpreted functions... but it's a good test case of such behavior.
Do you think a fix like in 2/2 is more appropriate (also for quirks modes, but with null instead of undefined)? And perhaps fix whatever is causing toString to fail, which seems unrelated I guess.
That might mean moving it out of exec_source though.
Same story there, why do you think it's somehow related to detached scopes? The attached test fails with your patch. Thanks, Jacek
On 5/6/22 01:50, Jacek Caban wrote:
Honestly, I don't particularly care about toString, it's just the only builtin I could think off the top of my head that could be tested for this. Because right now we handle it in exec_source, which is only for interpreted functions... but it's a good test case of such behavior.
Do you think a fix like in 2/2 is more appropriate (also for quirks modes, but with null instead of undefined)? And perhaps fix whatever is causing toString to fail, which seems unrelated I guess.
That might mean moving it out of exec_source though.
Same story there, why do you think it's somehow related to detached scopes? The attached test fails with your patch.
I attached a wrong test, here is the right one. Jacek
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=114257 Your paranoid android. === build (build log) === error: patch failed: dlls/mshtml/tests/es5.js:1795 Task: Patch failed to apply === debian11 (build log) === error: patch failed: dlls/mshtml/tests/es5.js:1795 Task: Patch failed to apply === debian11 (build log) === error: patch failed: dlls/mshtml/tests/es5.js:1795 Task: Patch failed to apply
On 06/05/2022 02:56, Jacek Caban wrote:
On 5/6/22 01:50, Jacek Caban wrote:
Honestly, I don't particularly care about toString, it's just the only builtin I could think off the top of my head that could be tested for this. Because right now we handle it in exec_source, which is only for interpreted functions... but it's a good test case of such behavior.
Do you think a fix like in 2/2 is more appropriate (also for quirks modes, but with null instead of undefined)? And perhaps fix whatever is causing toString to fail, which seems unrelated I guess.
That might mean moving it out of exec_source though.
Same story there, why do you think it's somehow related to detached scopes? The attached test fails with your patch.
I attached a wrong test, here is the right one.
Jacek
Because without handling detached scopes we actually *trigger the assertion* so it's quite bad. But that's just for toString, it's not really important. I've added a test with hasOwnProperty now, which *must* fail on detached scopes, but it doesn't currently (since they're js objects). They need special handling either way. The attached test looks to me like a different issue: it passes null instead of undefined, which is an ES5 specific thing, and probably deserves a separate fix. It's not much of a problem either way. Note that we are *already* handling detached scopes in exec_source, so it's not like I'm adding custom code to deal with that separately; we already do. I'm just moving it out of exec_source because: 1) Nothing in the (specified) spec says it's only for source functions. 2) This will become a problem with proxy functions later (which is where it's needed); I'd rather not duplicate that logic in them for no reason. 3) We are already handling it in exec_source "as a special case". I'm trying to find fix for the attach problem and send it separately but honestly I can't see how such a fix would fix detached scopes at all.
Hi Gabriel, On 5/6/22 15:19, Gabriel Ivăncescu wrote:
On 06/05/2022 02:56, Jacek Caban wrote:
On 5/6/22 01:50, Jacek Caban wrote:
Honestly, I don't particularly care about toString, it's just the only builtin I could think off the top of my head that could be tested for this. Because right now we handle it in exec_source, which is only for interpreted functions... but it's a good test case of such behavior.
Do you think a fix like in 2/2 is more appropriate (also for quirks modes, but with null instead of undefined)? And perhaps fix whatever is causing toString to fail, which seems unrelated I guess.
That might mean moving it out of exec_source though.
Same story there, why do you think it's somehow related to detached scopes? The attached test fails with your patch.
I attached a wrong test, here is the right one.
Jacek
Because without handling detached scopes we actually *trigger the assertion* so it's quite bad. But that's just for toString, it's not really important. I've added a test with hasOwnProperty now, which *must* fail on detached scopes, but it doesn't currently (since they're js objects). They need special handling either way.
The attached test looks to me like a different issue: it passes null instead of undefined, which is an ES5 specific thing, and probably deserves a separate fix. It's not much of a problem either way.
I'm not convinced that it's unrelated, see below.
Note that we are *already* handling detached scopes in exec_source, so it's not like I'm adding custom code to deal with that separately; we already do. I'm just moving it out of exec_source because:
1) Nothing in the (specified) spec says it's only for source functions. 2) This will become a problem with proxy functions later (which is where it's needed); I'd rather not duplicate that logic in them for no reason. 3) We are already handling it in exec_source "as a special case".
I'm trying to find fix for the attach problem and send it separately but honestly I can't see how such a fix would fix detached scopes at all.
The main problem of those patches is that they seem like random special cases fixing specific tests without enough understanding of the problem. Tests that I attached were meant to help you find the root cause of problems, but v2 is not really looking convincing yet. For example, do we really want to use builtin info class type for this? I'm attaching another test. BTW, any time detached scopes need special treatment is a worrying sign. There is no such thing as detached scope in the spec. It's purely our implementation detail (it's in fact just an optimization). If they have observable differences in behaviour, that's alarming on its own. Jacek
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=114515 Your paranoid android. === build (build log) === error: patch failed: dlls/mshtml/tests/es5.js:1798 Task: Patch failed to apply === debian11 (build log) === error: patch failed: dlls/mshtml/tests/es5.js:1798 Task: Patch failed to apply === debian11 (build log) === error: patch failed: dlls/mshtml/tests/es5.js:1798 Task: Patch failed to apply
On 12/05/2022 13:45, Jacek Caban wrote:
Hi Gabriel,
On 5/6/22 15:19, Gabriel Ivăncescu wrote:
On 06/05/2022 02:56, Jacek Caban wrote:
On 5/6/22 01:50, Jacek Caban wrote:
Honestly, I don't particularly care about toString, it's just the only builtin I could think off the top of my head that could be tested for this. Because right now we handle it in exec_source, which is only for interpreted functions... but it's a good test case of such behavior.
Do you think a fix like in 2/2 is more appropriate (also for quirks modes, but with null instead of undefined)? And perhaps fix whatever is causing toString to fail, which seems unrelated I guess.
That might mean moving it out of exec_source though.
Same story there, why do you think it's somehow related to detached scopes? The attached test fails with your patch.
I attached a wrong test, here is the right one.
Jacek
Because without handling detached scopes we actually *trigger the assertion* so it's quite bad. But that's just for toString, it's not really important. I've added a test with hasOwnProperty now, which *must* fail on detached scopes, but it doesn't currently (since they're js objects). They need special handling either way.
The attached test looks to me like a different issue: it passes null instead of undefined, which is an ES5 specific thing, and probably deserves a separate fix. It's not much of a problem either way.
I'm not convinced that it's unrelated, see below.
Note that we are *already* handling detached scopes in exec_source, so it's not like I'm adding custom code to deal with that separately; we already do. I'm just moving it out of exec_source because:
1) Nothing in the (specified) spec says it's only for source functions. 2) This will become a problem with proxy functions later (which is where it's needed); I'd rather not duplicate that logic in them for no reason. 3) We are already handling it in exec_source "as a special case".
I'm trying to find fix for the attach problem and send it separately but honestly I can't see how such a fix would fix detached scopes at all.
The main problem of those patches is that they seem like random special cases fixing specific tests without enough understanding of the problem. Tests that I attached were meant to help you find the root cause of problems, but v2 is not really looking convincing yet. For example, do we really want to use builtin info class type for this? I'm attaching another test.
BTW, any time detached scopes need special treatment is a worrying sign. There is no such thing as detached scope in the spec. It's purely our implementation detail (it's in fact just an optimization). If they have observable differences in behaviour, that's alarming on its own.
Jacek
Right. The spec I referenced (but also the code in exec_source for 3rd edition) does state that it's replaced with null/undefined under certain conditions. One of them being not a reference. Since detached scopes aren't supposed to exist, doesn't that apply to them? I don't know if builtin_info is the best way to handle it either, I was just moving the logic out of exec_source (which uses that), because this needs to happen for all function calls, not just interpreted functions, that's all. Oh, and the reason I added the JS_GLOBAL workaround is to avoid potential regressions, for the moment. It will be completely removed when the JS global becomes window (which is how it should work, but need it to be a proper JS object first with prototype etc, so it will be a while). I'm not looking to fix builtin functions specially here. The main motivation is for proxy functions later which depend on the same "logic", it just doesn't make sense to keep it in exec_source and specific to interpreted functions. But for now we only have builtin functions to test with, other than exec_source (since bind functions replace 'this' anyway, so they can't be tested). Well I will look into it but tbh, fixing builtins (at least in jscript, not sure about mshtml) seems to require some sort of extra internal flag passed in call/apply when invoking, to ALL builtins. I don't think it's worth it. Maybe I can figure out a different way, but either way, that logic definitely needs to be moved out of exec_source, whether it's fixed differently or not... Thanks, Gabriel
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
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
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
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=114256 Your paranoid android. === build (build log) === error: patch failed: dlls/mshtml/tests/es5.js:1795 Task: Patch failed to apply === debian11 (build log) === error: patch failed: dlls/mshtml/tests/es5.js:1795 Task: Patch failed to apply === debian11 (build log) === error: patch failed: dlls/mshtml/tests/es5.js:1795 Task: Patch failed to apply
participants (3)
-
Gabriel Ivăncescu -
Jacek Caban -
Marvin