[PATCH 0/5] MR6721: mshtml: Fix deletion of global window props.
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> The previous tests bailed out too early on IE8, and did not test builtin props. Also added todos for tests that we did not even test because of returning early. Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/mshtml/dispex.c | 62 ++++++++++++++++++++----------- dlls/mshtml/tests/documentmode.js | 47 +++++++++++++++++++---- 2 files changed, 80 insertions(+), 29 deletions(-) diff --git a/dlls/mshtml/dispex.c b/dlls/mshtml/dispex.c index e194024607d..65c54ed8aec 100644 --- a/dlls/mshtml/dispex.c +++ b/dlls/mshtml/dispex.c @@ -1764,6 +1764,26 @@ HRESULT dispex_call_builtin(DispatchEx *dispex, DISPID id, DISPPARAMS *dp, return call_builtin_function(dispex, func, dp, res, ei, caller); } +static VARIANT_BOOL reset_builtin_func(DispatchEx *dispex, func_info_t *func) +{ + func_obj_entry_t *entry; + + if(!dispex->dynamic_data || !dispex->dynamic_data->func_disps || + !dispex->dynamic_data->func_disps[func->func_disp_idx].func_obj) + return VARIANT_FALSE; + + entry = dispex->dynamic_data->func_disps + func->func_disp_idx; + if(V_VT(&entry->val) == VT_DISPATCH && + V_DISPATCH(&entry->val) == (IDispatch*)&entry->func_obj->dispex.IWineJSDispatchHost_iface) + return VARIANT_FALSE; + + VariantClear(&entry->val); + V_VT(&entry->val) = VT_DISPATCH; + V_DISPATCH(&entry->val) = (IDispatch*)&entry->func_obj->dispex.IWineJSDispatchHost_iface; + IDispatch_AddRef(V_DISPATCH(&entry->val)); + return VARIANT_TRUE; +} + HRESULT remove_attribute(DispatchEx *This, DISPID id, VARIANT_BOOL *success) { switch(get_dispid_type(id)) { @@ -1793,26 +1813,7 @@ HRESULT remove_attribute(DispatchEx *This, DISPID id, VARIANT_BOOL *success) /* For builtin functions, we set their value to the original function. */ if(func->func_disp_idx >= 0) { - func_obj_entry_t *entry; - - if(!This->dynamic_data || !This->dynamic_data->func_disps - || !This->dynamic_data->func_disps[func->func_disp_idx].func_obj) { - *success = VARIANT_FALSE; - return S_OK; - } - - entry = This->dynamic_data->func_disps + func->func_disp_idx; - if(V_VT(&entry->val) == VT_DISPATCH - && V_DISPATCH(&entry->val) == (IDispatch*)&entry->func_obj->dispex.IWineJSDispatchHost_iface) { - *success = VARIANT_FALSE; - return S_OK; - } - - VariantClear(&entry->val); - V_VT(&entry->val) = VT_DISPATCH; - V_DISPATCH(&entry->val) = (IDispatch*)&entry->func_obj->dispex.IWineJSDispatchHost_iface; - IDispatch_AddRef(V_DISPATCH(&entry->val)); - *success = VARIANT_TRUE; + *success = reset_builtin_func(This, func); return S_OK; } *success = VARIANT_TRUE; @@ -2304,7 +2305,8 @@ static HRESULT dispex_prop_delete(DispatchEx *dispex, DISPID id) return E_NOTIMPL; } - if(is_dynamic_dispid(id)) { + switch(get_dispid_type(id)) { + case DISPEXPROP_DYNAMIC: { DWORD idx = id - DISPID_DYNPROP_0; dynamic_prop_t *prop; @@ -2316,6 +2318,24 @@ static HRESULT dispex_prop_delete(DispatchEx *dispex, DISPID id) prop->flags |= DYNPROP_DELETED; return S_OK; } + case DISPEXPROP_BUILTIN: { + func_info_t *func; + HRESULT hres; + + if(!ensure_real_info(dispex)) + return E_OUTOFMEMORY; + + hres = get_builtin_func(dispex->info, id, &func); + if(FAILED(hres)) + return hres; + + if(func->func_disp_idx >= 0) + reset_builtin_func(dispex, func); + return S_OK; + } + default: + break; + } return S_OK; } diff --git a/dlls/mshtml/tests/documentmode.js b/dlls/mshtml/tests/documentmode.js index 306d65e5768..ae5e68126dd 100644 --- a/dlls/mshtml/tests/documentmode.js +++ b/dlls/mshtml/tests/documentmode.js @@ -1301,7 +1301,7 @@ sync_test("navigator", function() { sync_test("delete_prop", function() { var v = document.documentMode; - var obj = document.createElement("div"), r, obj2; + var obj = document.createElement("div"), r, obj2, func, prop; obj.prop1 = true; r = false; @@ -1317,6 +1317,40 @@ sync_test("delete_prop", function() { ok(!r, "got an unexpected exception"); ok(!("prop1" in obj), "prop1 is still in obj"); + /* builtin properties don't throw any exception, but are not really deleted */ + r = (delete obj.tagName); + ok(r, "delete returned " + r); + ok("tagName" in obj, "tagName deleted from obj"); + ok(obj.tagName === "DIV", "tagName = " + obj.tagName); + + prop = obj.id; + r = (delete obj.id); + ok(r, "delete returned " + r); + ok("id" in obj, "id deleted from obj"); + ok(obj.id === prop, "id = " + obj.id); + + obj.id = "1234"; + ok(obj.id === "1234", "id after set to 1234 = " + obj.id); + r = (delete obj.id); + ok(r, "delete returned " + r); + ok("id" in obj, "id deleted from obj"); + ok(obj.id === "1234", "id = " + obj.id); + + /* builtin functions get reset to their original values */ + func = function() { } + prop = obj.setAttribute; + r = (delete obj.setAttribute); + ok(r, "delete returned " + r); + ok("setAttribute" in obj, "setAttribute deleted from obj"); + ok(obj.setAttribute === prop, "setAttribute = " + obj.setAttribute); + + obj.setAttribute = func; + ok(obj.setAttribute === func, "setAttribute after set to func = " + obj.setAttribute); + r = (delete obj.setAttribute); + ok(r, "delete returned " + r); + ok("setAttribute" in obj, "setAttribute deleted from obj"); + ok(obj.setAttribute === prop, "setAttribute = " + obj.setAttribute); + /* again, this time prop1 does not exist */ r = false; try { @@ -1326,7 +1360,6 @@ sync_test("delete_prop", function() { } if(v < 9) { ok(r, "did not get an expected exception"); - return; }else { ok(!r, "got an unexpected exception"); ok(!("prop1" in obj), "prop1 is still in obj"); @@ -1337,12 +1370,6 @@ sync_test("delete_prop", function() { ok("className" in obj, "className deleted from obj"); ok(obj.className === "", "className = " + obj.className); - /* builtin propertiles don't throw any exception, but are not really deleted */ - r = (delete obj.tagName); - ok(r, "delete returned " + r); - ok("tagName" in obj, "tagName deleted from obj"); - ok(obj.tagName === "DIV", "tagName = " + obj.tagName); - obj = document.querySelectorAll("*"); ok("0" in obj, "0 is not in obj"); obj2 = obj[0]; @@ -1362,6 +1389,7 @@ sync_test("delete_prop", function() { r = true; } if(v < 9) { + todo_wine. ok(r, "did not get an expected exception"); }else { ok(!r, "got an unexpected globalprop1 exception"); @@ -1377,6 +1405,7 @@ sync_test("delete_prop", function() { r = true; } if(v < 9) { + todo_wine. ok(r, "did not get an expected globalprop2 exception"); }else { ok(!r, "got an unexpected exception"); @@ -1393,7 +1422,9 @@ sync_test("delete_prop", function() { r = true; } if(v < 9) { + todo_wine. ok(r, "did not get an expected exception"); + todo_wine. ok("globalprop3" in obj, "globalprop3 is not in obj"); }else { ok(!r, "got an unexpected globalprop3 exception"); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6721
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> Instead of E_NOTIMPL. We can't use the dispex's delete because it also applies to dynamic and builtin props and it also happens too late when deleting by name (after looking it up), where existing tests show it doesn't look up the dispid at all. Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/mshtml/htmlwindow.c | 16 ++++++++++++++++ dlls/mshtml/tests/documentmode.js | 20 ++++++++++++++++---- 2 files changed, 32 insertions(+), 4 deletions(-) diff --git a/dlls/mshtml/htmlwindow.c b/dlls/mshtml/htmlwindow.c index a4039ce08bc..731acd61cd7 100644 --- a/dlls/mshtml/htmlwindow.c +++ b/dlls/mshtml/htmlwindow.c @@ -3364,18 +3364,34 @@ static HRESULT WINAPI WindowDispEx_InvokeEx(IWineJSDispatchHost *iface, DISPID i static HRESULT WINAPI WindowDispEx_DeleteMemberByName(IWineJSDispatchHost *iface, BSTR bstrName, DWORD grfdex) { HTMLOuterWindow *This = impl_from_IWineJSDispatchHost(iface); + compat_mode_t compat_mode = dispex_compat_mode(&This->base.inner_window->event_target.dispex); TRACE("(%p)->(%s %lx)\n", This, debugstr_w(bstrName), grfdex); + if(compat_mode < COMPAT_MODE_IE8) { + /* Not implemented by IE */ + return E_NOTIMPL; + } + if(compat_mode == COMPAT_MODE_IE8) + return MSHTML_E_INVALID_ACTION; + return IWineJSDispatchHost_DeleteMemberByName(&This->base.inner_window->event_target.dispex.IWineJSDispatchHost_iface, bstrName, grfdex); } static HRESULT WINAPI WindowDispEx_DeleteMemberByDispID(IWineJSDispatchHost *iface, DISPID id) { HTMLOuterWindow *This = impl_from_IWineJSDispatchHost(iface); + compat_mode_t compat_mode = dispex_compat_mode(&This->base.inner_window->event_target.dispex); TRACE("(%p)->(%lx)\n", This, id); + if(compat_mode < COMPAT_MODE_IE8) { + /* Not implemented by IE */ + return E_NOTIMPL; + } + if(compat_mode == COMPAT_MODE_IE8) + return MSHTML_E_INVALID_ACTION; + return IWineJSDispatchHost_DeleteMemberByDispID(&This->base.inner_window->event_target.dispex.IWineJSDispatchHost_iface, id); } diff --git a/dlls/mshtml/tests/documentmode.js b/dlls/mshtml/tests/documentmode.js index ae5e68126dd..cc1ec1aa964 100644 --- a/dlls/mshtml/tests/documentmode.js +++ b/dlls/mshtml/tests/documentmode.js @@ -1380,16 +1380,29 @@ sync_test("delete_prop", function() { /* test window object and its global scope handling */ obj = window; + ok("encodeURIComponent" in obj, "encodeURIComponent not in obj"); + try { + r = (delete window.encodeURIComponent); + ok(v >= 9, "did not get an expect exception deleting encodeURIComponent"); + ok(r, "delete returned " + r); + todo_wine. + ok(!("encodeURIComponent" in obj), "encodeURIComponent is still in obj"); + }catch(ex) { + ok(v < 9, "expected exception deleting encodeURIComponent"); + ok(ex.number === 0xa01bd - 0x80000000, "deleting encodeURIComponent threw " + ex.number); + ok("encodeURIComponent" in obj, "encodeURIComponent is not in obj"); + } + obj.globalprop1 = true; ok(globalprop1, "globalprop1 = " + globalprop1); r = false; try { delete obj.globalprop1; }catch(ex) { + ok(ex.number === 0xa01bd - 0x80000000, "deleting globalprop1 threw " + ex.number); r = true; } if(v < 9) { - todo_wine. ok(r, "did not get an expected exception"); }else { ok(!r, "got an unexpected globalprop1 exception"); @@ -1402,10 +1415,10 @@ sync_test("delete_prop", function() { try { delete obj.globalprop2; }catch(ex) { + ok(ex.number === 0xa01bd - 0x80000000, "deleting globalprop2 threw " + ex.number); r = true; } if(v < 9) { - todo_wine. ok(r, "did not get an expected globalprop2 exception"); }else { ok(!r, "got an unexpected exception"); @@ -1419,12 +1432,11 @@ sync_test("delete_prop", function() { try { delete globalprop3; }catch(ex) { + ok(ex.number === 0xa01bd - 0x80000000, "deleting globalprop3 threw " + ex.number); r = true; } if(v < 9) { - todo_wine. ok(r, "did not get an expected exception"); - todo_wine. ok("globalprop3" in obj, "globalprop3 is not in obj"); }else { ok(!r, "got an unexpected globalprop3 exception"); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6721
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> When GLOBAL_DISPEXVAR is used, we store the id of the dynamic prop and invoke it using the same id. But this is not the case if we have a jsdisp, which is redirected from InvokeEx and results in an invalid id. The actual way this works on native (in IE9+) is that element/frame props are special and not part of the window object at all. They're actually looked up after the prototype is, in a special way, but that requires a revamp. Storing over it just creates an actual prop on the window (which is what we are doing here). Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- This is necessary in next patch (and tested). --- dlls/jscript/dispex.c | 14 ++++++++++++++ dlls/jscript/jsdisp.idl | 1 + dlls/mshtml/htmlwindow.c | 3 +++ 3 files changed, 18 insertions(+) diff --git a/dlls/jscript/dispex.c b/dlls/jscript/dispex.c index 91fa2c00cf4..ab7d004bf04 100644 --- a/dlls/jscript/dispex.c +++ b/dlls/jscript/dispex.c @@ -2386,6 +2386,19 @@ static HRESULT WINAPI WineJSDispatch_GetScriptGlobal(IWineJSDispatch *iface, IWi return S_OK; } +static HRESULT WINAPI WineJSDispatch_OverrideExternalProp(IWineJSDispatch *iface, const WCHAR *name, VARIANT *value) +{ + jsdisp_t *This = impl_from_IWineJSDispatch(iface); + dispex_prop_t *prop; + + prop = lookup_dispex_prop(This, string_hash(name), name, FALSE); + assert(prop != NULL && prop->type == PROP_EXTERN); + + prop->type = PROP_JSVAL; + prop->flags = PROPF_ENUMERABLE | PROPF_CONFIGURABLE | PROPF_WRITABLE; + return variant_to_jsval(This->ctx, value, &prop->u.val); +} + static IWineJSDispatchVtbl DispatchExVtbl = { DispatchEx_QueryInterface, DispatchEx_AddRef, @@ -2404,6 +2417,7 @@ static IWineJSDispatchVtbl DispatchExVtbl = { DispatchEx_GetNameSpaceParent, WineJSDispatch_Free, WineJSDispatch_GetScriptGlobal, + WineJSDispatch_OverrideExternalProp, }; jsdisp_t *as_jsdisp(IDispatch *disp) diff --git a/dlls/jscript/jsdisp.idl b/dlls/jscript/jsdisp.idl index 051f819e817..b49d0c9b30d 100644 --- a/dlls/jscript/jsdisp.idl +++ b/dlls/jscript/jsdisp.idl @@ -52,6 +52,7 @@ interface IWineJSDispatch : IDispatchEx { void Free(); HRESULT GetScriptGlobal(IWineJSDispatchHost **ret); + HRESULT OverrideExternalProp(const WCHAR *name, VARIANT *value); } [ diff --git a/dlls/mshtml/htmlwindow.c b/dlls/mshtml/htmlwindow.c index 731acd61cd7..2409b3a1288 100644 --- a/dlls/mshtml/htmlwindow.c +++ b/dlls/mshtml/htmlwindow.c @@ -3926,6 +3926,9 @@ HRESULT HTMLWindow_invoke(DispatchEx *dispex, DISPID id, LCID lcid, WORD flags, case DISPATCH_PROPERTYPUT: { DISPID dispex_id; + if(This->event_target.dispex.jsdisp) + return IWineJSDispatch_OverrideExternalProp(This->event_target.dispex.jsdisp, prop->name, params->rgvarg); + hres = dispex_get_dynid(&This->event_target.dispex, prop->name, TRUE, &dispex_id); if(FAILED(hres)) return hres; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6721
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> We have to introduce special handling for jscript, as it's probably integrated on native mshtml. We can't use GetScriptDispatch/GetDispID or GetMemberName or whatever on every lookup, as existing tests will fail (and they fail even if we change the compat mode, so it's not a compat mode thing). Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/jscript/dispex.c | 5 ++ dlls/jscript/jscript.c | 7 +++ dlls/jscript/jscript.h | 1 + dlls/jscript/jsdisp.idl | 1 + dlls/mshtml/htmlscript.h | 1 + dlls/mshtml/htmlwindow.c | 6 ++- dlls/mshtml/script.c | 51 +++++++++++++++++++ dlls/mshtml/tests/documentmode.js | 83 ++++++++++++++++++++++++++++++- 8 files changed, 152 insertions(+), 3 deletions(-) diff --git a/dlls/jscript/dispex.c b/dlls/jscript/dispex.c index ab7d004bf04..0777bc10671 100644 --- a/dlls/jscript/dispex.c +++ b/dlls/jscript/dispex.c @@ -2529,6 +2529,11 @@ jsdisp_t *iface_to_jsdisp(IDispatch *iface) : NULL; } +HRESULT jsdisp_has_prop(jsdisp_t *jsdisp, DISPID id) +{ + return get_prop(jsdisp, id) ? S_OK : DISP_E_UNKNOWNNAME; +} + HRESULT jsdisp_get_id(jsdisp_t *jsdisp, const WCHAR *name, DWORD flags, DISPID *id) { dispex_prop_t *prop; diff --git a/dlls/jscript/jscript.c b/dlls/jscript/jscript.c index c42109b4143..9d71793f6d5 100644 --- a/dlls/jscript/jscript.c +++ b/dlls/jscript/jscript.c @@ -1451,6 +1451,12 @@ static ULONG WINAPI WineJScript_Release(IWineJScript *iface) return IActiveScript_Release(&This->IActiveScript_iface); } +static HRESULT WINAPI WineJScript_GlobalPropExists(IWineJScript *iface, DISPID id) +{ + JScript *This = impl_from_IWineJScript(iface); + return jsdisp_has_prop(This->ctx->global, id); +} + static HRESULT WINAPI WineJScript_InitHostObject(IWineJScript *iface, IWineJSDispatchHost *host_obj, IWineJSDispatch *prototype, UINT32 flags, IWineJSDispatch **ret) { @@ -1469,6 +1475,7 @@ static const IWineJScriptVtbl WineJScriptVtbl = { WineJScript_QueryInterface, WineJScript_AddRef, WineJScript_Release, + WineJScript_GlobalPropExists, WineJScript_InitHostObject, WineJScript_InitHostConstructor, }; diff --git a/dlls/jscript/jscript.h b/dlls/jscript/jscript.h index e9207c231d8..bede7ca135b 100644 --- a/dlls/jscript/jscript.h +++ b/dlls/jscript/jscript.h @@ -246,6 +246,7 @@ HRESULT init_host_constructor(script_ctx_t*,IWineJSDispatchHost*,IWineJSDispatch 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*); HRESULT disp_call_value_with_caller(script_ctx_t*,IDispatch*,jsval_t,WORD,unsigned,jsval_t*,jsval_t*,IServiceProvider*); +HRESULT jsdisp_has_prop(jsdisp_t*,DISPID); HRESULT jsdisp_call_value(jsdisp_t*,jsval_t,WORD,unsigned,jsval_t*,jsval_t*); HRESULT jsdisp_call(jsdisp_t*,DISPID,WORD,unsigned,jsval_t*,jsval_t*); HRESULT jsdisp_call_name(jsdisp_t*,const WCHAR*,WORD,unsigned,jsval_t*,jsval_t*); diff --git a/dlls/jscript/jsdisp.idl b/dlls/jscript/jsdisp.idl index b49d0c9b30d..97a4e398522 100644 --- a/dlls/jscript/jsdisp.idl +++ b/dlls/jscript/jsdisp.idl @@ -82,6 +82,7 @@ interface IWineJSDispatchHost : IDispatchEx ] interface IWineJScript : IUnknown { + HRESULT GlobalPropExists(DISPID id); HRESULT InitHostObject(IWineJSDispatchHost *host_obj, IWineJSDispatch *prototype, UINT32 flags, IWineJSDispatch **ret); HRESULT InitHostConstructor(IWineJSDispatchHost *constr, IWineJSDispatch *prototype, IWineJSDispatch **ret); } diff --git a/dlls/mshtml/htmlscript.h b/dlls/mshtml/htmlscript.h index 6d608d525a1..30b46677395 100644 --- a/dlls/mshtml/htmlscript.h +++ b/dlls/mshtml/htmlscript.h @@ -48,6 +48,7 @@ IDispatch *script_parse_event(HTMLInnerWindow*,LPCWSTR); HRESULT exec_script(HTMLInnerWindow*,const WCHAR*,const WCHAR*,VARIANT*); void update_browser_script_mode(GeckoBrowser*,IUri*); BOOL find_global_prop(HTMLInnerWindow*,const WCHAR*,DWORD,ScriptHost**,DISPID*); +HRESULT global_prop_still_exists(HTMLInnerWindow*,global_prop_t*); IDispatch *get_script_disp(ScriptHost*); IActiveScriptSite *get_first_script_site(HTMLInnerWindow*); void initialize_script_global(HTMLInnerWindow*); diff --git a/dlls/mshtml/htmlwindow.c b/dlls/mshtml/htmlwindow.c index 2409b3a1288..3fbcc7dd660 100644 --- a/dlls/mshtml/htmlwindow.c +++ b/dlls/mshtml/htmlwindow.c @@ -3324,8 +3324,10 @@ HRESULT search_window_props(HTMLInnerWindow *This, const WCHAR *name, DWORD grfd for(i=0; i < This->global_prop_cnt; i++) { /* FIXME: case sensitivity */ if(!wcscmp(This->global_props[i].name, name)) { - *pid = MSHTML_DISPID_CUSTOM_MIN+i; - return S_OK; + HRESULT hres = global_prop_still_exists(This, &This->global_props[i]); + if(hres == S_OK) + *pid = MSHTML_DISPID_CUSTOM_MIN + i; + return hres; } } diff --git a/dlls/mshtml/script.c b/dlls/mshtml/script.c index 7ac26e63a3b..0cdadfa5fbc 100644 --- a/dlls/mshtml/script.c +++ b/dlls/mshtml/script.c @@ -1825,6 +1825,57 @@ BOOL find_global_prop(HTMLInnerWindow *window, const WCHAR *name, DWORD flags, S return FALSE; } +HRESULT global_prop_still_exists(HTMLInnerWindow *window, global_prop_t *prop) +{ + HRESULT hres; + + switch(prop->type) { + case GLOBAL_SCRIPTVAR: { + IWineJScript *jscript; + + if(!prop->script_host->script) + return E_UNEXPECTED; + + if(!IsEqualGUID(&CLSID_JScript, &prop->script_host->guid) || + IActiveScript_QueryInterface(prop->script_host->script, &IID_IWineJScript, (void **)&jscript) != S_OK) + return S_OK; + + hres = IWineJScript_GlobalPropExists(jscript, prop->id); + IWineJScript_Release(jscript); + return hres; + } + case GLOBAL_ELEMENTVAR: { + IHTMLElement *elem; + BSTR bstr; + + if(!(bstr = SysAllocString(prop->name))) + return E_OUTOFMEMORY; + hres = IHTMLDocument3_getElementById(&window->doc->IHTMLDocument3_iface, bstr, &elem); + SysFreeString(bstr); + if(FAILED(hres)) + return hres; + + if(!elem) + return DISP_E_UNKNOWNNAME; + IHTMLElement_Release(elem); + return S_OK; + } + case GLOBAL_FRAMEVAR: { + HTMLOuterWindow *frame; + + hres = get_frame_by_name(window->base.outer_window, prop->name, FALSE, &frame); + if(FAILED(hres)) + return (hres == DISP_E_MEMBERNOTFOUND) ? DISP_E_UNKNOWNNAME : hres; + + return frame ? S_OK : DISP_E_UNKNOWNNAME; + } + default: + break; + } + + return S_OK; +} + static BOOL is_jscript_available(void) { static BOOL available, checked; diff --git a/dlls/mshtml/tests/documentmode.js b/dlls/mshtml/tests/documentmode.js index cc1ec1aa964..ed2ad8cd187 100644 --- a/dlls/mshtml/tests/documentmode.js +++ b/dlls/mshtml/tests/documentmode.js @@ -1447,8 +1447,89 @@ 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"); + + globalprop5 = true; + ok(obj.globalprop5, "globalprop5 = " + globalprop5); + try { + r = (delete window.globalprop5); + ok(v >= 9, "did not get an expected exception deleting globalprop5"); + ok(r, "delete returned " + r); + todo_wine. + ok(!("globalprop5" in obj), "globalprop5 is still in obj"); + }catch(ex) { + ok(v < 9, "expected exception deleting globalprop5"); + ok(ex.number === 0xa01bd - 0x80000000, "deleting globalprop5 threw " + ex.number); + ok("globalprop5" in obj, "globalprop5 is not in obj"); + } + + document.body.innerHTML = '<div id="winetest"/>'; + ok("winetest" in obj, "winetest not in obj"); + try { + r = (delete window.winetest); + ok(v >= 9, "did not get an expected exception deleting winetest"); + ok(r, "delete returned " + r); + }catch(ex) { + ok(v < 9, "expected exception deleting winetest"); + ok(ex.number === 0xa01bd - 0x80000000, "deleting winetest threw " + ex.number); + } + ok("winetest" in obj, "winetest is not in obj"); + document.body.innerHTML = ""; + ok(!("winetest" in obj), "winetest is still in obj"); + + document.body.innerHTML = '<div id="foobar"/>'; + ok("foobar" in obj, "foobar not in obj"); + window.foobar = "1234"; + ok(obj.foobar === "1234", "foobar = " + obj.foobar); + document.body.innerHTML = ""; + ok("foobar" in obj, "foobar is not in obj"); + ok(obj.foobar === "1234", "foobar = " + obj.foobar); + try { + r = (delete window.foobar); + ok(v >= 9, "did not get an expected exception deleting foobar"); + ok(r, "delete returned " + r); + ok(!("foobar" in obj), "foobar is still in obj"); + }catch(ex) { + ok(v < 9, "expected exception deleting foobar"); + ok(ex.number === 0xa01bd - 0x80000000, "deleting foobar threw " + ex.number); + ok("foobar" in obj, "foobar is not in obj"); + } + + document.body.innerHTML = '<div id="barfoo"/>'; + ok("barfoo" in obj, "barfoo not in obj"); + window.barfoo = "5678"; + ok(obj.barfoo === "5678", "barfoo = " + obj.barfoo); + try { + r = (delete window.barfoo); + ok(v >= 9, "did not get an expected exception deleting barfoo"); + ok(r, "delete returned " + r); + ok(obj.barfoo !== "5678", "barfoo is still 5678"); + }catch(ex) { + ok(v < 9, "expected exception deleting barfoo"); + ok(ex.number === 0xa01bd - 0x80000000, "deleting barfoo threw " + ex.number); + ok(obj.barfoo === "5678", "barfoo = " + obj.barfoo); + } + ok("barfoo" in obj, "barfoo is not in obj"); + document.body.innerHTML = ""; + if(v < 9) + ok("barfoo" in obj, "barfoo is not in obj"); + else + ok(!("barfoo" in obj), "barfoo is still in obj"); + + document.body.innerHTML = '<iframe id="testwine"/>'; + ok("testwine" in obj, "testwine not in obj"); + try { + r = (delete window.testwine); + ok(v >= 9, "did not get an expected exception deleting testwine"); + ok(r, "delete returned " + r); + }catch(ex) { + ok(v < 9, "expected exception deleting testwine"); + ok(ex.number === 0xa01bd - 0x80000000, "deleting testwine threw " + ex.number); + } + ok("testwine" in obj, "testwine is not in obj"); + + document.body.innerHTML = ""; + ok(!("testwine" in obj), "testwine is still in obj"); }); sync_test("detached arguments", function() { -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6721
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/mshtml/htmlwindow.c | 39 +++++++++++++++++++++++++++++++ dlls/mshtml/tests/documentmode.js | 3 --- 2 files changed, 39 insertions(+), 3 deletions(-) diff --git a/dlls/mshtml/htmlwindow.c b/dlls/mshtml/htmlwindow.c index 3fbcc7dd660..48d47ebe01d 100644 --- a/dlls/mshtml/htmlwindow.c +++ b/dlls/mshtml/htmlwindow.c @@ -3974,6 +3974,44 @@ HRESULT HTMLWindow_invoke(DispatchEx *dispex, DISPID id, LCID lcid, WORD flags, return hres; } +static HRESULT HTMLWindow_delete(DispatchEx *dispex, DISPID id) +{ + HTMLInnerWindow *This = impl_from_DispatchEx(dispex); + DWORD idx = id - MSHTML_DISPID_CUSTOM_MIN; + global_prop_t *prop; + HRESULT hres = S_OK; + + if(idx >= This->global_prop_cnt) + return DISP_E_MEMBERNOTFOUND; + + prop = This->global_props + idx; + switch(prop->type) { + case GLOBAL_SCRIPTVAR: { + IDispatchEx *iface; + IDispatch *disp; + + disp = get_script_disp(prop->script_host); + if(!disp) + return E_UNEXPECTED; + + hres = IDispatch_QueryInterface(disp, &IID_IDispatchEx, (void**)&iface); + if(SUCCEEDED(hres)) { + hres = IDispatchEx_DeleteMemberByDispID(iface, prop->id); + IDispatchEx_Release(iface); + }else { + WARN("No IDispatchEx, so can't delete\n"); + hres = S_OK; + } + IDispatch_Release(disp); + break; + } + default: + break; + } + + return hres; +} + static HRESULT HTMLWindow_next_dispid(DispatchEx *dispex, DISPID id, DISPID *pid) { DWORD idx = (id == DISPID_STARTENUM) ? 0 : id - MSHTML_DISPID_CUSTOM_MIN + 1; @@ -4200,6 +4238,7 @@ static const event_target_vtbl_t HTMLWindow_event_target_vtbl = { .lookup_dispid = HTMLWindow_lookup_dispid, .find_dispid = HTMLWindow_find_dispid, .invoke = HTMLWindow_invoke, + .delete = HTMLWindow_delete, .next_dispid = HTMLWindow_next_dispid, .get_prop_desc = HTMLWindow_get_prop_desc, .get_script_global = HTMLWindow_get_script_global, diff --git a/dlls/mshtml/tests/documentmode.js b/dlls/mshtml/tests/documentmode.js index ed2ad8cd187..c1d3c26ff92 100644 --- a/dlls/mshtml/tests/documentmode.js +++ b/dlls/mshtml/tests/documentmode.js @@ -1385,7 +1385,6 @@ sync_test("delete_prop", function() { r = (delete window.encodeURIComponent); ok(v >= 9, "did not get an expect exception deleting encodeURIComponent"); ok(r, "delete returned " + r); - todo_wine. ok(!("encodeURIComponent" in obj), "encodeURIComponent is still in obj"); }catch(ex) { ok(v < 9, "expected exception deleting encodeURIComponent"); @@ -1422,7 +1421,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"); } @@ -1455,7 +1453,6 @@ sync_test("delete_prop", function() { r = (delete window.globalprop5); ok(v >= 9, "did not get an expected exception deleting globalprop5"); ok(r, "delete returned " + r); - todo_wine. ok(!("globalprop5" in obj), "globalprop5 is still in obj"); }catch(ex) { ok(v < 9, "expected exception deleting globalprop5"); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6721
Is there something I should do here, or something wrong with this MR? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6721#note_87247
Jacek Caban (@jacek) commented about dlls/mshtml/script.c:
+ return E_UNEXPECTED; + + if(!IsEqualGUID(&CLSID_JScript, &prop->script_host->guid) || + IActiveScript_QueryInterface(prop->script_host->script, &IID_IWineJScript, (void **)&jscript) != S_OK) + return S_OK; + + hres = IWineJScript_GlobalPropExists(jscript, prop->id); + IWineJScript_Release(jscript); + return hres; + } + case GLOBAL_ELEMENTVAR: { + IHTMLElement *elem; + BSTR bstr; + + if(!(bstr = SysAllocString(prop->name))) + return E_OUTOFMEMORY; I think it would be better to just store the name as `BSTR` instead.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6721#note_87304
Jacek Caban (@jacek) commented about dlls/mshtml/htmlwindow.c:
case DISPATCH_PROPERTYPUT: { DISPID dispex_id;
+ if(This->event_target.dispex.jsdisp) + return IWineJSDispatch_OverrideExternalProp(This->event_target.dispex.jsdisp, prop->name, params->rgvarg);
Could we just return something like `S_FALSE` here and handle that in jscript's dispex? We already have the proper prop pointer there, so it would not even need to do the lookup. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6721#note_87302
Jacek Caban (@jacek) commented about dlls/mshtml/script.c:
+ HRESULT hres; + + switch(prop->type) { + case GLOBAL_SCRIPTVAR: { + IWineJScript *jscript; + + if(!prop->script_host->script) + return E_UNEXPECTED; + + if(!IsEqualGUID(&CLSID_JScript, &prop->script_host->guid) || + IActiveScript_QueryInterface(prop->script_host->script, &IID_IWineJScript, (void **)&jscript) != S_OK) + return S_OK; + + hres = IWineJScript_GlobalPropExists(jscript, prop->id); + IWineJScript_Release(jscript); + return hres; The justification for a new function does not look convincing to me. We could, for example, store script global disp in `ScriptHost` iff we're hosting JS and just use its `GetDispID` here.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6721#note_87303
Jacek Caban (@jacek) commented about dlls/mshtml/tests/documentmode.js:
/* test window object and its global scope handling */ obj = window;
+ ok("encodeURIComponent" in obj, "encodeURIComponent not in obj"); + try { + r = (delete window.encodeURIComponent);
It probably makes sense to restore `encodeURIComponent` at the end of the test to avoid potentially impacting other tests. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6721#note_87301
I thought it's still a work in progress, I've seen in other channels that you weren't happy with related code. Anyway, I reviewed it; interface changes feel very ad-hoc and at least partially redundant, see other comments for details. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6721#note_87305
On Wed Nov 13 12:30:36 2024 +0000, Jacek Caban wrote:
It probably makes sense to restore `encodeURIComponent` at the end of the test to avoid potentially impacting other tests. I'd have to add it after it's fixed at the end of the patches, since the todo makes it fail to redefine in wine.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6721#note_87320
On Wed Nov 13 12:30:36 2024 +0000, Jacek Caban wrote:
The justification for a new function does not look convincing to me. We could, for example, store script global disp in `ScriptHost` iff we're hosting JS and just use its `GetDispID` here. Ok, it still feels a bit weird to me that some places will keep looking it up (to satisfy tests) while in others we can use it, but I agree the new function is also not the best way.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6721#note_87321
On Wed Nov 13 15:28:19 2024 +0000, Jacek Caban wrote:
I thought it's still a work in progress, I've seen in other channels that you weren't happy with related code. Anyway, I reviewed it; interface changes feel very ad-hoc and at least partially redundant, see other comments for details. Well yeah, I think it will require some changes after jscript dispex revamp but that will come later, for now we have to move this forward (and having the global fixes, which is the other MR this one is a prereq for, is important).
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6721#note_87322
On Wed Nov 13 15:27:06 2024 +0000, Gabriel Ivăncescu wrote:
Ok, it still feels a bit weird to me that some places will keep looking it up (to satisfy tests) while in others we can use it, but I agree the new function is also not the best way. I guess it's not cached by native to allow script engines to change it silently. We know that JS doesn't do that.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6721#note_87336
On Wed Nov 13 15:26:18 2024 +0000, Gabriel Ivăncescu wrote:
I'd have to add it after it's fixed at the end of the patches, since the todo makes it fail to redefine in wine. Nevermind I got thrown off by IE8 (since it's also tested here).
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6721#note_87349
On Wed Nov 13 15:36:32 2024 +0000, Jacek Caban wrote:
I guess it's not cached by native to allow script engines to change it silently. We know that JS doesn't do that. Well this was the perfect opportunity to add very basic impl of GetMemberProperties and tests we did not have, as we already have DISPID and it's perfect for this purpose (checking if prop still exists), with no requested flags.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6721#note_87350
participants (2)
-
Gabriel Ivăncescu -
Jacek Caban (＠jacek)