[PATCH 0/2] MR6678: jscript: Set the global object to the global host.
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> For ES5, the JS global object is "window", so any properties redefined on window will impact all the JS globals since it's the same object. This isn't just a matter of the window object "shadowing" the JS global object; deleting a JS global prop from window also deletes it from the JS global object. Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/jscript/dispex.c | 48 +++++++++++++++++++++++++++++++ dlls/jscript/engine.c | 4 +-- dlls/jscript/jscript.c | 11 ++++++- dlls/jscript/jscript.h | 1 + dlls/mshtml/script.c | 6 +++- dlls/mshtml/tests/documentmode.js | 2 -- 6 files changed, 66 insertions(+), 6 deletions(-) diff --git a/dlls/jscript/dispex.c b/dlls/jscript/dispex.c index 91fa2c00cf4..88b93debb10 100644 --- a/dlls/jscript/dispex.c +++ b/dlls/jscript/dispex.c @@ -3558,3 +3558,51 @@ IWineJSDispatchHost *get_host_dispatch(IDispatch *disp) IWineJSDispatchHost_GetOuterDispatch(host_obj->host_iface, &ret); return ret; } + +/* ECMA-262 5.1 Edition 15.1 */ +HRESULT set_js_globals(script_ctx_t *ctx, IDispatch *global) +{ + jsdisp_t *new_global = to_jsdisp(global), *old_global = ctx->global; + const builtin_prop_t *bprop, *bend; + dispex_prop_t *prop, *end, *dst; + HRESULT hres; + BOOL b; + + if(!new_global || new_global->builtin_info != &HostObject_info) + return S_OK; + + /* Make sure the builtin methods are created as functions so we can copy them */ + for(bprop = old_global->builtin_info->props, bend = bprop + old_global->builtin_info->props_cnt; bprop != bend; bprop++) { + hres = find_prop_name(old_global, string_hash(bprop->name), bprop->name, FALSE, NULL, &prop); + if(FAILED(hres)) + return hres; + } + + /* Copy the props */ + for(prop = old_global->props, end = prop + old_global->prop_cnt; prop != end; prop++) { + assert(prop->type == PROP_JSVAL || prop->type == PROP_ACCESSOR); + + if(!(dst = lookup_dispex_prop(new_global, prop->hash, prop->name, FALSE))) { + if(!(dst = alloc_prop(new_global, prop->name, PROP_DELETED, 0))) + return E_OUTOFMEMORY; + }else { + dst->flags |= PROPF_CONFIGURABLE; + delete_prop(new_global, dst, &b); + } + + dst->flags = prop->flags; + dst->type = prop->type; + if(prop->type == PROP_JSVAL) { + hres = jsval_copy(prop->u.val, &dst->u.val); + if(FAILED(hres)) + return hres; + }else { + dst->u.accessor.getter = prop->u.accessor.getter ? jsdisp_addref(prop->u.accessor.getter) : NULL; + dst->u.accessor.setter = prop->u.accessor.setter ? jsdisp_addref(prop->u.accessor.setter) : NULL; + } + } + + jsdisp_release(old_global); + ctx->global = jsdisp_addref(new_global); + return S_OK; +} diff --git a/dlls/jscript/engine.c b/dlls/jscript/engine.c index c95fcace84a..ef1682f670e 100644 --- a/dlls/jscript/engine.c +++ b/dlls/jscript/engine.c @@ -820,7 +820,7 @@ static BOOL lookup_global_members(script_ctx_t *ctx, BSTR identifier, exprval_t HRESULT hres; LIST_FOR_EACH_ENTRY(item, &ctx->named_items, named_item_t, entry) { - if(item->flags & SCRIPTITEM_GLOBALMEMBERS) { + if((item->flags & SCRIPTITEM_GLOBALMEMBERS) && item->disp != (IDispatch*)&ctx->global->IWineJSDispatch_iface) { hres = disp_get_id(ctx, item->disp, identifier, identifier, 0, &id); if(SUCCEEDED(hres)) { if(ret) @@ -3512,7 +3512,7 @@ HRESULT exec_source(script_ctx_t *ctx, DWORD flags, bytecode_t *bytecode, functi if(this_obj) { jsdisp_t *jsthis = to_jsdisp(this_obj); - if(jsthis && jsthis->builtin_info->class == JSCLASS_GLOBAL) + if(jsthis && (jsthis->builtin_info->class == JSCLASS_GLOBAL || jsthis == ctx->global)) this_obj = NULL; } diff --git a/dlls/jscript/jscript.c b/dlls/jscript/jscript.c index c42109b4143..8b486787a31 100644 --- a/dlls/jscript/jscript.c +++ b/dlls/jscript/jscript.c @@ -912,6 +912,14 @@ static HRESULT WINAPI JScript_AddNamedItem(IActiveScript *iface, WARN("object does not implement IDispatch\n"); return hres; } + + if(!wcscmp(pstrName, L"window")) { + hres = set_js_globals(This->ctx, disp); + if(FAILED(hres)) { + IDispatch_Release(disp); + return hres; + } + } } item = malloc(sizeof(*item)); @@ -949,6 +957,7 @@ static HRESULT WINAPI JScript_GetScriptDispatch(IActiveScript *iface, LPCOLESTR IDispatch **ppdisp) { JScript *This = impl_from_IActiveScript(iface); + IWineJSDispatchHost *host_disp; jsdisp_t *script_obj; TRACE("(%p)->(%s %p)\n", This, debugstr_w(pstrItemName), ppdisp); @@ -968,7 +977,7 @@ static HRESULT WINAPI JScript_GetScriptDispatch(IActiveScript *iface, LPCOLESTR if(item->script_obj) script_obj = item->script_obj; } - *ppdisp = to_disp(script_obj); + *ppdisp = (host_disp = get_host_dispatch(to_disp(script_obj))) ? (IDispatch*)host_disp : to_disp(script_obj); IDispatch_AddRef(*ppdisp); return S_OK; } diff --git a/dlls/jscript/jscript.h b/dlls/jscript/jscript.h index e9207c231d8..4ebfa313a5a 100644 --- a/dlls/jscript/jscript.h +++ b/dlls/jscript/jscript.h @@ -242,6 +242,7 @@ HRESULT init_dispex(jsdisp_t*,script_ctx_t*,const builtin_info_t*,jsdisp_t*); HRESULT init_dispex_from_constr(jsdisp_t*,script_ctx_t*,const builtin_info_t*,jsdisp_t*); HRESULT init_host_object(script_ctx_t*,IWineJSDispatchHost*,IWineJSDispatch*,UINT32,IWineJSDispatch**); HRESULT init_host_constructor(script_ctx_t*,IWineJSDispatchHost*,IWineJSDispatch*,IWineJSDispatch**); +HRESULT set_js_globals(script_ctx_t*,IDispatch*); HRESULT disp_call(script_ctx_t*,IDispatch*,DISPID,WORD,unsigned,jsval_t*,jsval_t*); HRESULT disp_call_name(script_ctx_t*,IDispatch*,const WCHAR*,WORD,unsigned,jsval_t*,jsval_t*); diff --git a/dlls/mshtml/script.c b/dlls/mshtml/script.c index 7ac26e63a3b..ee79565d7dd 100644 --- a/dlls/mshtml/script.c +++ b/dlls/mshtml/script.c @@ -1806,7 +1806,11 @@ BOOL find_global_prop(HTMLInnerWindow *window, const WCHAR *name, DWORD flags, S hres = IDispatch_QueryInterface(disp, &IID_IDispatchEx, (void**)&dispex); if(SUCCEEDED(hres)) { - hres = IDispatchEx_GetDispID(dispex, str, flags & (~fdexNameEnsure), ret_id); + /* Avoid looking into ourselves if we're the actual global object */ + if(dispex == (IDispatchEx*)&window->base.outer_window->IWineJSDispatchHost_iface) + hres = DISP_E_UNKNOWNNAME; + else + hres = IDispatchEx_GetDispID(dispex, str, flags & (~fdexNameEnsure), ret_id); IDispatchEx_Release(dispex); }else { FIXME("No IDispatchEx\n"); diff --git a/dlls/mshtml/tests/documentmode.js b/dlls/mshtml/tests/documentmode.js index 306d65e5768..a360b5ecf72 100644 --- a/dlls/mshtml/tests/documentmode.js +++ b/dlls/mshtml/tests/documentmode.js @@ -1380,7 +1380,6 @@ sync_test("delete_prop", function() { ok(r, "did not get an expected globalprop2 exception"); }else { ok(!r, "got an unexpected exception"); - todo_wine. ok(!("globalprop2" in obj), "globalprop2 is still in obj"); } @@ -1404,7 +1403,6 @@ sync_test("delete_prop", function() { ok(obj.globalprop4, "globalprop4 = " + globalprop4); r = (delete globalprop4); ok(r, "delete returned " + r); - todo_wine. ok(!("globalprop4" in obj), "globalprop4 is still in obj"); }); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6678
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/jscript/global.c | 42 +++++++++--------- dlls/jscript/set.c | 6 +-- dlls/mshtml/tests/es5.js | 96 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 120 insertions(+), 24 deletions(-) diff --git a/dlls/jscript/global.c b/dlls/jscript/global.c index 2653d72cd2b..bb2fd26335b 100644 --- a/dlls/jscript/global.c +++ b/dlls/jscript/global.c @@ -943,7 +943,7 @@ static HRESULT init_constructors(script_ctx_t *ctx, jsdisp_t *object_prototype) if(FAILED(hres)) return hres; - hres = jsdisp_define_data_property(ctx->global, L"Function", PROPF_WRITABLE, + hres = jsdisp_define_data_property(ctx->global, L"Function", PROPF_CONFIGURABLE | PROPF_WRITABLE, jsval_obj(ctx->function_constr)); if(FAILED(hres)) return hres; @@ -952,7 +952,7 @@ static HRESULT init_constructors(script_ctx_t *ctx, jsdisp_t *object_prototype) if(FAILED(hres)) return hres; - hres = jsdisp_define_data_property(ctx->global, L"Object", PROPF_WRITABLE, + hres = jsdisp_define_data_property(ctx->global, L"Object", PROPF_CONFIGURABLE | PROPF_WRITABLE, jsval_obj(ctx->object_constr)); if(FAILED(hres)) return hres; @@ -961,7 +961,7 @@ static HRESULT init_constructors(script_ctx_t *ctx, jsdisp_t *object_prototype) if(FAILED(hres)) return hres; - hres = jsdisp_define_data_property(ctx->global, L"Array", PROPF_WRITABLE, + hres = jsdisp_define_data_property(ctx->global, L"Array", PROPF_CONFIGURABLE | PROPF_WRITABLE, jsval_obj(ctx->array_constr)); if(FAILED(hres)) return hres; @@ -970,7 +970,7 @@ static HRESULT init_constructors(script_ctx_t *ctx, jsdisp_t *object_prototype) if(FAILED(hres)) return hres; - hres = jsdisp_define_data_property(ctx->global, L"Boolean", PROPF_WRITABLE, + hres = jsdisp_define_data_property(ctx->global, L"Boolean", PROPF_CONFIGURABLE | PROPF_WRITABLE, jsval_obj(ctx->bool_constr)); if(FAILED(hres)) return hres; @@ -979,7 +979,7 @@ static HRESULT init_constructors(script_ctx_t *ctx, jsdisp_t *object_prototype) if(FAILED(hres)) return hres; - hres = jsdisp_define_data_property(ctx->global, L"Date", PROPF_WRITABLE, + hres = jsdisp_define_data_property(ctx->global, L"Date", PROPF_CONFIGURABLE | PROPF_WRITABLE, jsval_obj(ctx->date_constr)); if(FAILED(hres)) return hres; @@ -988,7 +988,7 @@ static HRESULT init_constructors(script_ctx_t *ctx, jsdisp_t *object_prototype) if(FAILED(hres)) return hres; - hres = jsdisp_define_data_property(ctx->global, L"Enumerator", PROPF_WRITABLE, + hres = jsdisp_define_data_property(ctx->global, L"Enumerator", PROPF_CONFIGURABLE | PROPF_WRITABLE, jsval_obj(ctx->enumerator_constr)); if(FAILED(hres)) return hres; @@ -997,42 +997,42 @@ static HRESULT init_constructors(script_ctx_t *ctx, jsdisp_t *object_prototype) if(FAILED(hres)) return hres; - hres = jsdisp_define_data_property(ctx->global, L"Error", PROPF_WRITABLE, + hres = jsdisp_define_data_property(ctx->global, L"Error", PROPF_CONFIGURABLE | PROPF_WRITABLE, jsval_obj(ctx->error_constr)); if(FAILED(hres)) return hres; - hres = jsdisp_define_data_property(ctx->global, L"EvalError", PROPF_WRITABLE, + hres = jsdisp_define_data_property(ctx->global, L"EvalError", PROPF_CONFIGURABLE | PROPF_WRITABLE, jsval_obj(ctx->eval_error_constr)); if(FAILED(hres)) return hres; - hres = jsdisp_define_data_property(ctx->global, L"RangeError", PROPF_WRITABLE, + hres = jsdisp_define_data_property(ctx->global, L"RangeError", PROPF_CONFIGURABLE | PROPF_WRITABLE, jsval_obj(ctx->range_error_constr)); if(FAILED(hres)) return hres; - hres = jsdisp_define_data_property(ctx->global, L"ReferenceError", PROPF_WRITABLE, + hres = jsdisp_define_data_property(ctx->global, L"ReferenceError", PROPF_CONFIGURABLE | PROPF_WRITABLE, jsval_obj(ctx->reference_error_constr)); if(FAILED(hres)) return hres; - hres = jsdisp_define_data_property(ctx->global, L"RegExpError", PROPF_WRITABLE, + hres = jsdisp_define_data_property(ctx->global, L"RegExpError", PROPF_CONFIGURABLE | PROPF_WRITABLE, jsval_obj(ctx->regexp_error_constr)); if(FAILED(hres)) return hres; - hres = jsdisp_define_data_property(ctx->global, L"SyntaxError", PROPF_WRITABLE, + hres = jsdisp_define_data_property(ctx->global, L"SyntaxError", PROPF_CONFIGURABLE | PROPF_WRITABLE, jsval_obj(ctx->syntax_error_constr)); if(FAILED(hres)) return hres; - hres = jsdisp_define_data_property(ctx->global, L"TypeError", PROPF_WRITABLE, + hres = jsdisp_define_data_property(ctx->global, L"TypeError", PROPF_CONFIGURABLE | PROPF_WRITABLE, jsval_obj(ctx->type_error_constr)); if(FAILED(hres)) return hres; - hres = jsdisp_define_data_property(ctx->global, L"URIError", PROPF_WRITABLE, + hres = jsdisp_define_data_property(ctx->global, L"URIError", PROPF_CONFIGURABLE | PROPF_WRITABLE, jsval_obj(ctx->uri_error_constr)); if(FAILED(hres)) return hres; @@ -1041,7 +1041,7 @@ static HRESULT init_constructors(script_ctx_t *ctx, jsdisp_t *object_prototype) if(FAILED(hres)) return hres; - hres = jsdisp_define_data_property(ctx->global, L"Number", PROPF_WRITABLE, + hres = jsdisp_define_data_property(ctx->global, L"Number", PROPF_CONFIGURABLE | PROPF_WRITABLE, jsval_obj(ctx->number_constr)); if(FAILED(hres)) return hres; @@ -1050,7 +1050,7 @@ static HRESULT init_constructors(script_ctx_t *ctx, jsdisp_t *object_prototype) if(FAILED(hres)) return hres; - hres = jsdisp_define_data_property(ctx->global, L"RegExp", PROPF_WRITABLE, + hres = jsdisp_define_data_property(ctx->global, L"RegExp", PROPF_CONFIGURABLE | PROPF_WRITABLE, jsval_obj(ctx->regexp_constr)); if(FAILED(hres)) return hres; @@ -1059,7 +1059,7 @@ static HRESULT init_constructors(script_ctx_t *ctx, jsdisp_t *object_prototype) if(FAILED(hres)) return hres; - hres = jsdisp_define_data_property(ctx->global, L"String", PROPF_WRITABLE, + hres = jsdisp_define_data_property(ctx->global, L"String", PROPF_CONFIGURABLE | PROPF_WRITABLE, jsval_obj(ctx->string_constr)); if(FAILED(hres)) return hres; @@ -1068,7 +1068,7 @@ static HRESULT init_constructors(script_ctx_t *ctx, jsdisp_t *object_prototype) if(FAILED(hres)) return hres; - hres = jsdisp_define_data_property(ctx->global, L"VBArray", PROPF_WRITABLE, + hres = jsdisp_define_data_property(ctx->global, L"VBArray", PROPF_CONFIGURABLE | PROPF_WRITABLE, jsval_obj(ctx->vbarray_constr)); if(FAILED(hres)) return hres; @@ -1105,7 +1105,7 @@ HRESULT init_global(script_ctx_t *ctx) if(FAILED(hres)) return hres; - hres = jsdisp_define_data_property(ctx->global, L"Math", PROPF_WRITABLE, jsval_obj(math)); + hres = jsdisp_define_data_property(ctx->global, L"Math", PROPF_CONFIGURABLE | PROPF_WRITABLE, jsval_obj(math)); jsdisp_release(math); if(FAILED(hres)) return hres; @@ -1117,7 +1117,7 @@ HRESULT init_global(script_ctx_t *ctx) if(FAILED(hres)) return hres; - hres = jsdisp_define_data_property(ctx->global, L"JSON", PROPF_WRITABLE, jsval_obj(json)); + hres = jsdisp_define_data_property(ctx->global, L"JSON", PROPF_CONFIGURABLE | PROPF_WRITABLE, jsval_obj(json)); jsdisp_release(json); if(FAILED(hres)) return hres; @@ -1127,7 +1127,7 @@ HRESULT init_global(script_ctx_t *ctx) if(FAILED(hres)) return hres; - hres = jsdisp_define_data_property(ctx->global, L"ActiveXObject", PROPF_WRITABLE, + hres = jsdisp_define_data_property(ctx->global, L"ActiveXObject", PROPF_CONFIGURABLE | PROPF_WRITABLE, jsval_obj(constr)); jsdisp_release(constr); if(FAILED(hres)) diff --git a/dlls/jscript/set.c b/dlls/jscript/set.c index 5a568607a43..c114e3fd6e2 100644 --- a/dlls/jscript/set.c +++ b/dlls/jscript/set.c @@ -903,7 +903,7 @@ HRESULT init_set_constructor(script_ctx_t *ctx) if(FAILED(hres)) return hres; - hres = jsdisp_define_data_property(ctx->global, L"Set", PROPF_WRITABLE, + hres = jsdisp_define_data_property(ctx->global, L"Set", PROPF_CONFIGURABLE | PROPF_WRITABLE, jsval_obj(constructor)); jsdisp_release(constructor); if(FAILED(hres)) @@ -918,7 +918,7 @@ HRESULT init_set_constructor(script_ctx_t *ctx) if(FAILED(hres)) return hres; - hres = jsdisp_define_data_property(ctx->global, L"Map", PROPF_WRITABLE, + hres = jsdisp_define_data_property(ctx->global, L"Map", PROPF_CONFIGURABLE | PROPF_WRITABLE, jsval_obj(constructor)); jsdisp_release(constructor); if(FAILED(hres)) @@ -933,7 +933,7 @@ HRESULT init_set_constructor(script_ctx_t *ctx) if(FAILED(hres)) return hres; - hres = jsdisp_define_data_property(ctx->global, L"WeakMap", PROPF_WRITABLE, + hres = jsdisp_define_data_property(ctx->global, L"WeakMap", PROPF_CONFIGURABLE | PROPF_WRITABLE, jsval_obj(constructor)); jsdisp_release(constructor); return hres; diff --git a/dlls/mshtml/tests/es5.js b/dlls/mshtml/tests/es5.js index 3adb7cc1d61..d88f26e940a 100644 --- a/dlls/mshtml/tests/es5.js +++ b/dlls/mshtml/tests/es5.js @@ -23,6 +23,7 @@ var JS_E_NUMBER_EXPECTED = 0x800a1389; var JS_E_FUNCTION_EXPECTED = 0x800a138a; var JS_E_DATE_EXPECTED = 0x800a138e; var JS_E_OBJECT_EXPECTED = 0x800a138f; +var JS_E_UNDEFINED_VARIABLE = 0x800a1391; var JS_E_BOOLEAN_EXPECTED = 0x800a1392; var JS_E_VBARRAY_EXPECTED = 0x800a1395; var JS_E_ENUMERATOR_EXPECTED = 0x800a1397; @@ -2153,6 +2154,101 @@ sync_test("builtin_context", function() { ok(obj.length === 1, "obj.length = " + obj.length); }); +sync_test("builtin override", function() { + /* configurable */ + var builtins = [ + "ActiveXObject", + "Array", + "ArrayBuffer", + "Boolean", + "CollectGarbage", + "DataView", + "Date", + "decodeURI", + "decodeURIComponent", + "encodeURI", + "encodeURIComponent", + "Enumerator", + "Error", + "escape", + "EvalError", + "Function", + "isFinite", + "isNaN", + "JSON", + "Map", + "Math", + "Number", + "parseFloat", + "parseInt", + "RangeError", + "ReferenceError", + "RegExp", + "ScriptEngine", + "ScriptEngineBuildVersion", + "ScriptEngineMajorVersion", + "ScriptEngineMinorVersion", + "Set", + "String", + "SyntaxError", + "TypeError", + "unescape", + "URIError", + "VBArray", + "WeakMap" + ]; + + var override = { + value: 12, + configurable: true, + writable: true + }; + for(var i = 0; i < builtins.length; i++) { + var desc = Object.getOwnPropertyDescriptor(window, builtins[i]), r; + ok(desc !== undefined, "getOwnPropertyDescriptor('" + builtins[i] + "' returned undefined"); + ok(desc.configurable === true, builtins[i] + " not configurable"); + ok(desc.enumerable === false, builtins[i] + " is enumerable"); + ok(desc.writable === true, builtins[i] + " not writable"); + + r = Object.defineProperty(window, builtins[i], override); + ok(r === window, "defineProperty('" + builtins[i] + "' returned " + r); + r = Object.getOwnPropertyDescriptor(window, builtins[i]); + ok(r !== undefined, "getOwnPropertyDescriptor('" + builtins[i] + "' after override returned undefined"); + ok(r.value === 12, builtins[i] + " value = " + r.value); + + r = eval(builtins[i]); + ok(r === window[builtins[i]], "Global " + builtins[i] + " does not match redefined window." + builtins[i]); + r = (delete window[builtins[i]]); + ok(r === true, "delete window." + builtins[i] + " returned " + r); + ok(!(builtins[i] in window), builtins[i] + " in window after delete"); + try { + eval(builtins[i]); + ok(false, "expected exception retrieving global " + builtins[i] + " after delete."); + }catch(ex) { + var n = ex.number >>> 0; + ok(n === JS_E_UNDEFINED_VARIABLE, "retrieving global " + builtins[i] + " after delete threw " + n); + } + + r = Object.defineProperty(window, builtins[i], desc); + ok(r === window, "defineProperty('" + builtins[i] + "' to restore returned " + r); + } + + /* non-configurable */ + builtins = [ + "undefined", + "Infinity", + "NaN" + ]; + + for(var i = 0; i < builtins.length; i++) { + var desc = Object.getOwnPropertyDescriptor(window, builtins[i]), r; + ok(desc !== undefined, "getOwnPropertyDescriptor('" + builtins[i] + "' returned undefined"); + ok(desc.configurable === false, builtins[i] + " is configurable"); + ok(desc.enumerable === false, builtins[i] + " is enumerable"); + ok(desc.writable === false, builtins[i] + " is writable"); + } +}); + sync_test("host this", function() { var tests = [ undefined, null, external.nullDisp, function() {}, [0], "foobar", true, 42, new Number(42), external.testHostContext(true), window, document ]; var i, obj = Object.create(Function.prototype); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6678
For ES5, the JS global object is "window", so any properties redefined on window will impact all the JS globals since it's the same object. This isn't just a matter of the window object "shadowing" the JS global object; deleting a JS global prop from window also deletes it from the JS global object.
Isn't support for deleting JS global objects a matter of implementing `dispex_static_data_vtbl_t`'s `delete` for `GLOBAL_SCRIPTVAR` properties? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6678#note_85168
On Wed Oct 16 15:31:44 2024 +0000, Jacek Caban wrote:
For ES5, the JS global object is "window", so any properties redefined on window will impact all the JS globals since it's the same object. This isn't just a matter of the window object "shadowing" the JS global object; deleting a JS global prop from window also deletes it from the JS global object. Isn't support for deleting JS global objects a matter of implementing `dispex_static_data_vtbl_t`'s `delete` for `GLOBAL_SCRIPTVAR` properties? How would you delete something that's not on the window itself? I mean if you delete a GLOBAL_SCRIPTVAR, it will just get re-created when accessed, so it still "exists" (since it's just a reference to the one in the js global). Well, unless we add a new type like GLOBAL_DELETED to override it.
By the way, the current approach with find_dispid doesn't work with getOwnPropertyNames (not just in window and document, but also in stuff like prototypes and constructors and wherever else it is used). If they haven't been looked up yet, they aren't created, and thus won't be enumerated. I think it's easier to make them closer to actual js objects and avoid corner cases like this since the jscript code will handle it normally. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6678#note_85176
How would you delete something that's not on the window itself? I mean if you delete a GLOBAL_SCRIPTVAR, it will just get re-created when accessed, so it still "exists" (since it's just a reference to the one in the js global). Well, unless we add a new type like GLOBAL_DELETED to override it.
You could probably just delete it in script dispatch with `DeleteMemberByDispID` (like we forward `InvokeEx` calls). Unless I'm missing something, frames' code for exposing global variable would be broken with this patch as it depends on those being global properties.
By the way, the current approach with find_dispid doesn't work with getOwnPropertyNames (not just in window and document, but also in stuff like prototypes and constructors and wherever else it is used). If they haven't been looked up yet, they aren't created, and thus won't be enumerated. I think it's easier to make them closer to actual js objects and avoid corner cases like this since the jscript code will handle it normally.
If they are indeed intended to be enumerated, it should be doable with `next_dispid`. AFAIR, we still need to support mixing script languages in newer compat modes, so some form of `GLOBAL_SCRIPTVAR` is still needed. This MR is pushing the corner case deeper into the corner, deep enough to avoid being covered by tests, not removing it. More generally, moving more things closer to js sounds good, but we most likely will need to handle "special" properties one way or another. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6678#note_85292
On Thu Oct 17 10:39:05 2024 +0000, Jacek Caban wrote:
How would you delete something that's not on the window itself? I mean if you delete a GLOBAL_SCRIPTVAR, it will just get re-created when accessed, so it still "exists" (since it's just a reference to the one in the js global). Well, unless we add a new type like GLOBAL_DELETED to override it. You could probably just delete it in script dispatch with `DeleteMemberByDispID` (like we forward `InvokeEx` calls). Unless I'm missing something, frames' code for exposing global variable would be broken with this patch as it depends on those being global properties. By the way, the current approach with find_dispid doesn't work with getOwnPropertyNames (not just in window and document, but also in stuff like prototypes and constructors and wherever else it is used). If they haven't been looked up yet, they aren't created, and thus won't be enumerated. I think it's easier to make them closer to actual js objects and avoid corner cases like this since the jscript code will handle it normally. If they are indeed intended to be enumerated, it should be doable with `next_dispid`. AFAIR, we still need to support mixing script languages in newer compat modes, so some form of `GLOBAL_SCRIPTVAR` is still needed. This MR is pushing the corner case deeper into the corner, deep enough to avoid being covered by tests, not removing it. More generally, moving more things closer to js sounds good, but we most likely will need to handle "special" properties one way or another. Well this is a bit more hairy than I anticipated, so I'm looking into fixing some related/required stuff first. Some stuff was already broken. Anyway just handling deletion is not enough by itself.
But with respect to the "intended to be enumerated", well they aren't enumerable, but getOwnPropertyNames retrieves all props, not just enumerable ones. But it's not related to this MR anymore since I'm trying the different approach. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6678#note_85486
On Fri Oct 18 19:42:47 2024 +0000, Gabriel Ivăncescu wrote:
Well this is a bit more hairy than I anticipated, so I'm looking into fixing some related/required stuff first. Some stuff was already broken. Anyway just handling deletion is not enough by itself. But with respect to the "intended to be enumerated", well they aren't enumerable, but getOwnPropertyNames retrieves all props, not just enumerable ones. But it's not related to this MR anymore since I'm trying the different approach. I created !6721 to fix deletion first, since it was way more hairy than I anticipated, so I split it.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6678#note_85757
participants (2)
-
Gabriel Ivăncescu -
Jacek Caban (＠jacek)