[PATCH v2 0/2] MR9660: jscript: Treat DISPATCH_PROPERTYGET | DISPATCH_METHOD as property getter in ES5+ modes.
-- v2: mshtml: Handle DISPATCH_PROPERTYGET | DISPATCH_METHOD in dispex_value as jscript: Treat DISPATCH_PROPERTYGET | DISPATCH_METHOD as property getter https://gitlab.winehq.org/wine/wine/-/merge_requests/9660
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/jscript/dispex.c | 8 ++-- dlls/mshtml/tests/dom.c | 81 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 85 insertions(+), 4 deletions(-) diff --git a/dlls/jscript/dispex.c b/dlls/jscript/dispex.c index f39a8cd4b12..d3873ce8f2a 100644 --- a/dlls/jscript/dispex.c +++ b/dlls/jscript/dispex.c @@ -2136,10 +2136,10 @@ static HRESULT WINAPI DispatchEx_InvokeEx(IWineJSDispatch *iface, DISPID id, LCI if(pspCaller) IServiceProvider_AddRef(pspCaller); + if(wFlags == (DISPATCH_METHOD | DISPATCH_PROPERTYGET)) + wFlags = (This->ctx->version < SCRIPTLANGUAGEVERSION_ES5) ? DISPATCH_METHOD : DISPATCH_PROPERTYGET; + switch(wFlags) { - case DISPATCH_METHOD|DISPATCH_PROPERTYGET: - wFlags = DISPATCH_METHOD; - /* fall through */ case DISPATCH_METHOD: case DISPATCH_CONSTRUCT: { jsval_t *argv, buf[6], r; @@ -2712,7 +2712,7 @@ HRESULT disp_call(script_ctx_t *ctx, IDispatch *disp, DISPID id, WORD flags, uns jsdisp_release(jsdisp); flags &= ~DISPATCH_JSCRIPT_INTERNAL_MASK; - if(ret && argc) + if(ret && argc && ctx->version < SCRIPTLANGUAGEVERSION_ES5) flags |= DISPATCH_PROPERTYGET; dp.cArgs = argc; diff --git a/dlls/mshtml/tests/dom.c b/dlls/mshtml/tests/dom.c index 3235cbbaca5..861042bc6eb 100644 --- a/dlls/mshtml/tests/dom.c +++ b/dlls/mshtml/tests/dom.c @@ -11731,6 +11731,85 @@ static void test_case_insens(IHTMLDocument2 *doc) IDispatchEx_Release(dispex); } +static void test_method_vs_getter(IHTMLDocument2 *doc) +{ + DISPPARAMS dp = { 0 }; + IDispatchEx *dispex; + DISPID dispid; + HRESULT hres; + VARIANT v; + BSTR bstr; + + hres = IHTMLDocument2_QueryInterface(doc, &IID_IDispatchEx, (void**)&dispex); + ok(hres == S_OK, "Could not get IDispatchEx: %08lx\n", hres); + + V_VT(&v) = VT_EMPTY; + hres = IDispatchEx_InvokeEx(dispex, DISPID_VALUE, LOCALE_NEUTRAL, DISPATCH_METHOD | DISPATCH_PROPERTYGET, &dp, &v, NULL, NULL); + todo_wine_if(compat_mode < COMPAT_IE9) + ok(hres == S_OK, "InvokeEx failed: %08lx\n", hres); + todo_wine_if(compat_mode < COMPAT_IE9) + ok(V_VT(&v) == VT_BSTR, "V_VT = %d\n", V_VT(&v)); + VariantClear(&v); + + bstr = SysAllocString(L"body"); + hres = IDispatchEx_GetDispID(dispex, bstr, 0, &dispid); + ok(hres == S_OK, "GetDispID returned: %08lx\n", hres); + SysFreeString(bstr); + + hres = IDispatchEx_InvokeEx(dispex, dispid, LOCALE_NEUTRAL, DISPATCH_METHOD | DISPATCH_PROPERTYGET, &dp, &v, NULL, NULL); + ok(hres == S_OK, "InvokeEx failed: %08lx\n", hres); + ok(V_VT(&v) == VT_DISPATCH, "V_VT = %d\n", V_VT(&v)); + ok(V_DISPATCH(&v) != NULL, "V_DISPATCH == NULL\n"); + VariantClear(&v); + + bstr = SysAllocString(L"title"); + hres = IDispatchEx_GetDispID(dispex, bstr, 0, &dispid); + ok(hres == S_OK, "GetDispID returned: %08lx\n", hres); + SysFreeString(bstr); + + hres = IDispatchEx_InvokeEx(dispex, dispid, LOCALE_NEUTRAL, DISPATCH_METHOD | DISPATCH_PROPERTYGET, &dp, &v, NULL, NULL); + ok(hres == S_OK, "InvokeEx failed: %08lx\n", hres); + ok(V_VT(&v) == VT_BSTR, "V_VT = %d\n", V_VT(&v)); + VariantClear(&v); + + bstr = SysAllocString(L"close"); + hres = IDispatchEx_GetDispID(dispex, bstr, 0, &dispid); + ok(hres == S_OK, "GetDispID returned: %08lx\n", hres); + SysFreeString(bstr); + + hres = IDispatchEx_InvokeEx(dispex, dispid, LOCALE_NEUTRAL, DISPATCH_METHOD | DISPATCH_PROPERTYGET, &dp, &v, NULL, NULL); + ok(hres == S_OK, "InvokeEx failed: %08lx\n", hres); + if(compat_mode < COMPAT_IE9) + ok(V_VT(&v) == VT_EMPTY, "V_VT = %d\n", V_VT(&v)); + else { + ok(V_VT(&v) == VT_DISPATCH, "V_VT = %d\n", V_VT(&v)); + ok(V_DISPATCH(&v) != NULL, "V_DISPATCH == NULL\n"); + } + VariantClear(&v); + + hres = IDispatchEx_InvokeEx(dispex, dispid, LOCALE_NEUTRAL, DISPATCH_PROPERTYGET, &dp, &v, NULL, NULL); + ok(hres == S_OK, "InvokeEx failed: %08lx\n", hres); + ok(V_VT(&v) == VT_DISPATCH, "V_VT = %d\n", V_VT(&v)); + ok(V_DISPATCH(&v) != NULL, "V_DISPATCH == NULL\n"); + IDispatchEx_Release(dispex); + + hres = IDispatch_QueryInterface(V_DISPATCH(&v), &IID_IDispatchEx, (void**)&dispex); + ok(hres == S_OK, "Could not get IDispatchEx: %08lx\n", hres); + VariantClear(&v); + + hres = IDispatchEx_InvokeEx(dispex, DISPID_VALUE, LOCALE_NEUTRAL, DISPATCH_METHOD | DISPATCH_PROPERTYGET, &dp, &v, NULL, NULL); + if(compat_mode < COMPAT_IE9) + todo_wine + ok(hres == E_ACCESSDENIED, "InvokeEx returned: %08lx\n", hres); + else { + ok(hres == S_OK, "InvokeEx failed: %08lx\n", hres); + ok(V_VT(&v) == VT_BSTR, "V_VT = %d\n", V_VT(&v)); + } + VariantClear(&v); + + IDispatchEx_Release(dispex); +} + static void test_null_write(IHTMLDocument2 *doc) { HRESULT hres; @@ -13842,9 +13921,11 @@ START_TEST(dom) run_domtest(doc_blank_ie8, test_quirks_mode_perf_toJSON); run_domtest(doctype_str, test_doctype); run_domtest(case_insens_str, test_case_insens); + run_domtest(doc_blank, test_method_vs_getter); if(is_ie9plus) { compat_mode = COMPAT_IE9; run_domtest(emptydiv_ie9_str, test_docfrag); + run_domtest(doc_blank_ie9, test_method_vs_getter); compat_mode = COMPAT_NONE; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9660
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/mshtml/dispex.c | 1 + dlls/mshtml/tests/dom.c | 2 -- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/dlls/mshtml/dispex.c b/dlls/mshtml/dispex.c index 0af89aca630..a6b8afb0543 100644 --- a/dlls/mshtml/dispex.c +++ b/dlls/mshtml/dispex.c @@ -868,6 +868,7 @@ static HRESULT dispex_value(DispatchEx *This, LCID lcid, WORD flags, DISPPARAMS return This->info->vtbl->value(This, lcid, flags, params, res, ei, caller); switch(flags) { + case DISPATCH_PROPERTYGET | DISPATCH_METHOD: case DISPATCH_PROPERTYGET: V_VT(res) = VT_BSTR; hres = dispex_to_string(This, &V_BSTR(res)); diff --git a/dlls/mshtml/tests/dom.c b/dlls/mshtml/tests/dom.c index 861042bc6eb..58a409a7a77 100644 --- a/dlls/mshtml/tests/dom.c +++ b/dlls/mshtml/tests/dom.c @@ -11745,9 +11745,7 @@ static void test_method_vs_getter(IHTMLDocument2 *doc) V_VT(&v) = VT_EMPTY; hres = IDispatchEx_InvokeEx(dispex, DISPID_VALUE, LOCALE_NEUTRAL, DISPATCH_METHOD | DISPATCH_PROPERTYGET, &dp, &v, NULL, NULL); - todo_wine_if(compat_mode < COMPAT_IE9) ok(hres == S_OK, "InvokeEx failed: %08lx\n", hres); - todo_wine_if(compat_mode < COMPAT_IE9) ok(V_VT(&v) == VT_BSTR, "V_VT = %d\n", V_VT(&v)); VariantClear(&v); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9660
Jacek Caban (@jacek) commented about dlls/mshtml/dispex.c:
return This->info->vtbl->value(This, lcid, flags, params, res, ei, caller);
switch(flags) { + case DISPATCH_PROPERTYGET | DISPATCH_METHOD:
While this is probably correct here, we have more places in mshtml where we interpret `DISPATCH_PROPERTYGET | DISPATCH_METHOD` flags. Most of them do not seem very important, but things like `invoke_builtin_prop` look like they should be updated as well. I am wondering if we could reduce those flags earlier in mshtml's `IDispatchEx`, similar to what you do on the jscript side. That would mean that custom invoke implementations would not have the option to interpret the flags differently, so I am not sure if it is a good idea without testing. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9660#note_124712
On Thu Dec 4 15:39:29 2025 +0000, Jacek Caban wrote:
While this is probably correct here, we have more places in mshtml where we interpret `DISPATCH_PROPERTYGET | DISPATCH_METHOD` flags. Most of them do not seem very important, but things like `invoke_builtin_prop` look like they should be updated as well. I am wondering if we could reduce those flags earlier in mshtml's `IDispatchEx`, similar to what you do on the jscript side. That would mean that custom invoke implementations would not have the option to interpret the flags differently, so I am not sure if it is a good idea without testing. Sorry I'm not exactly sure what you mean. We already handle those cases in `invoke_builtin_prop` (and test them, `body` and `title` in the first patch, and it's also handled in `invoke_builtin_function` when we test the `close` method), so they should be correct already, or am I missing something? Do you have something specific in mind?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/9660#note_124725
On Thu Dec 4 16:59:16 2025 +0000, Gabriel Ivăncescu wrote:
Sorry I'm not exactly sure what you mean. We already handle those cases in `invoke_builtin_prop` (and test them, `body` and `title` in the first patch, and it's also handled in `invoke_builtin_function` when we test the `close` method), so they should be correct already, or am I missing something? Do you have something specific in mind? There is a case where we treat having both flags as a getter, and with this MR we'll often pass only the call flag, so we would no longer hit that case. After looking more closely, though, I think that part is fine as is.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/9660#note_124816
On Thu Dec 4 22:19:53 2025 +0000, Jacek Caban wrote:
There is a case where we treat having both flags as a getter, and with this MR we'll often pass only the call flag, so we would no longer hit that case. After looking more closely, though, I think that part is fine as is. Yeah in pre IE9 modes the behavior depends on the prop type (method vs accessor), so it can't be done generically before looking it up. I mean it's already the case upstream now, nothing added by this MR aside tests about it.
BTW the second patch isn't that important, was just a quick fix for the todo I introduced, I can defer it to after code freeze if it's problematic or requires bigger changes. Let me know if I should do that so that the gist of it goes in before code freeze tonight. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9660#note_124932
Jacek Caban (@jacek) commented about dlls/jscript/dispex.c:
jsdisp_release(jsdisp);
flags &= ~DISPATCH_JSCRIPT_INTERNAL_MASK; - if(ret && argc) + if(ret && argc && ctx->version < SCRIPTLANGUAGEVERSION_ES5)
Do we need something similar in `disp_call_value_with_caller`? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9660#note_124945
On Fri Dec 5 15:24:16 2025 +0000, Gabriel Ivăncescu wrote:
Yeah in pre IE9 modes the behavior depends on the prop type (method vs accessor), so it can't be done generically before looking it up. I mean it's already the case upstream now, nothing added by this MR aside tests about it. BTW the second patch isn't that important, was just a quick fix for the todo I introduced, I can defer it to after code freeze if it's problematic or requires bigger changes. Let me know if I should do that so that the gist of it goes in before code freeze tonight. This part is fine, thanks.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/9660#note_124947
participants (2)
-
Gabriel Ivăncescu -
Jacek Caban (＠jacek)