[PATCH v2 0/5] MR965: jscript/mshtml: Enumeration fixes.
-- v2: mshtml: Enumerate document elements with name. mshtml: Do not enumerate internal dynamic props. mshtml: Fix enumerating first custom prop after builtins. jscript: Refill the props at end of enumeration in html mode and retry. jscript: Fill the builtin props for enumeration on prototypes as well. https://gitlab.winehq.org/wine/wine/-/merge_requests/965
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/jscript/dispex.c | 48 +++++++++++++++++++++++++++---------------- 1 file changed, 30 insertions(+), 18 deletions(-) diff --git a/dlls/jscript/dispex.c b/dlls/jscript/dispex.c index e25378e866b..746c5246ed1 100644 --- a/dlls/jscript/dispex.c +++ b/dlls/jscript/dispex.c @@ -610,15 +610,41 @@ HRESULT builtin_set_const(script_ctx_t *ctx, jsdisp_t *jsthis, jsval_t value) return S_OK; } +static HRESULT fill_props(jsdisp_t *obj) +{ + dispex_prop_t *prop; + HRESULT hres; + + if(obj->builtin_info->idx_length) { + unsigned i = 0, len = obj->builtin_info->idx_length(obj); + WCHAR name[12]; + + for(i = 0; i < len; i++) { + swprintf(name, ARRAY_SIZE(name), L"%u", i); + hres = find_prop_name(obj, string_hash(name), name, FALSE, &prop); + if(FAILED(hres)) + return hres; + } + } + + return S_OK; +} + static HRESULT fill_protrefs(jsdisp_t *This) { dispex_prop_t *iter, *prop; HRESULT hres; + hres = fill_props(This); + if(FAILED(hres)) + return hres; + if(!This->prototype) return S_OK; - fill_protrefs(This->prototype); + hres = fill_protrefs(This->prototype); + if(FAILED(hres)) + return hres; for(iter = This->prototype->props; iter < This->prototype->props+This->prototype->prop_cnt; iter++) { hres = find_prop_name(This, iter->hash, iter->name, FALSE, &prop); @@ -2467,23 +2493,9 @@ HRESULT jsdisp_next_prop(jsdisp_t *obj, DISPID id, enum jsdisp_enum_type enum_ty HRESULT hres; if(id == DISPID_STARTENUM) { - if(obj->builtin_info->idx_length) { - unsigned i = 0, len = obj->builtin_info->idx_length(obj); - WCHAR name[12]; - - for(i = 0; i < len; i++) { - swprintf(name, ARRAY_SIZE(name), L"%d", i); - hres = find_prop_name(obj, string_hash(name), name, FALSE, &iter); - if(FAILED(hres)) - return hres; - } - } - - if (enum_type == JSDISP_ENUM_ALL) { - hres = fill_protrefs(obj); - if(FAILED(hres)) - return hres; - } + hres = (enum_type == JSDISP_ENUM_ALL) ? fill_protrefs(obj) : fill_props(obj); + if(FAILED(hres)) + return hres; idx = 0; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/965
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> If properties were added during enumeration, for example on the prototype, they are actually visited in mshtml scripts in any mode. Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/jscript/dispex.c | 13 ++++++++----- dlls/jscript/tests/lang.js | 2 ++ dlls/mshtml/tests/documentmode.js | 20 ++++++++++++++++++++ 3 files changed, 30 insertions(+), 5 deletions(-) diff --git a/dlls/jscript/dispex.c b/dlls/jscript/dispex.c index 746c5246ed1..42f41771ef2 100644 --- a/dlls/jscript/dispex.c +++ b/dlls/jscript/dispex.c @@ -2492,16 +2492,16 @@ HRESULT jsdisp_next_prop(jsdisp_t *obj, DISPID id, enum jsdisp_enum_type enum_ty DWORD idx = id; HRESULT hres; - if(id == DISPID_STARTENUM) { + if(id == DISPID_STARTENUM || idx >= obj->prop_cnt) { hres = (enum_type == JSDISP_ENUM_ALL) ? fill_protrefs(obj) : fill_props(obj); if(FAILED(hres)) return hres; - idx = 0; + if(id == DISPID_STARTENUM) + idx = 0; + if(idx >= obj->prop_cnt) + return S_FALSE; } - if(idx >= obj->prop_cnt) - return S_FALSE; - for(iter = &obj->props[idx]; iter < obj->props + obj->prop_cnt; iter++) { if(iter->type == PROP_DELETED) continue; @@ -2513,6 +2513,9 @@ HRESULT jsdisp_next_prop(jsdisp_t *obj, DISPID id, enum jsdisp_enum_type enum_ty return S_OK; } + if(obj->ctx->html_mode) + return jsdisp_next_prop(obj, prop_to_id(obj, iter - 1), enum_type, ret); + return S_FALSE; } diff --git a/dlls/jscript/tests/lang.js b/dlls/jscript/tests/lang.js index 67e4576813b..5c201d5bf26 100644 --- a/dlls/jscript/tests/lang.js +++ b/dlls/jscript/tests/lang.js @@ -1559,6 +1559,7 @@ inobj.test2 = true; tmp = 0; for(iter in inobj) { + forinTestObj.prototype.test4 = true; arr[iter] = true; tmp++; } @@ -1567,6 +1568,7 @@ ok(tmp === 3, "for..in tmp = " + tmp); ok(arr["test1"] === true, "arr[test1] !== true"); ok(arr["test2"] === true, "arr[test2] !== true"); ok(arr["test3"] === true, "arr[test3] !== true"); +ok(arr["test4"] !== true, "arr[test4] === true"); ok((delete inobj.test1) === true, "delete inobj.test1 returned false"); ok(!("test1" in inobj), "test1 is still in inobj after delete"); diff --git a/dlls/mshtml/tests/documentmode.js b/dlls/mshtml/tests/documentmode.js index 90223d53f83..cb6d2d01ec2 100644 --- a/dlls/mshtml/tests/documentmode.js +++ b/dlls/mshtml/tests/documentmode.js @@ -634,6 +634,26 @@ sync_test("JS objs", function() { test_parses("if(false) { o.if; }", v >= 9); }); +sync_test("for..in", function() { + function ctor() {} + ctor.prototype.test2 = true; + + var arr = new Array(), obj = new ctor(), i, r; + obj.test1 = true; + + i = 0; + for(var r in obj) { + ctor.prototype.test3 = true; + arr[r] = true; + i++; + } + + ok(i === 3, "enum did " + i + " iterations"); + ok(arr["test1"] === true, "arr[test1] !== true"); + ok(arr["test2"] === true, "arr[test2] !== true"); + ok(arr["test3"] === true, "arr[test3] !== true"); +}); + sync_test("elem_by_id", function() { document.body.innerHTML = '<form id="testid" name="testname"></form>'; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/965
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=124717 Your paranoid android. === debian11 (32 bit report) === mshtml: misc: Timeout
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/mshtml/dispex.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/dlls/mshtml/dispex.c b/dlls/mshtml/dispex.c index 90650e5acc4..d7b587f21d2 100644 --- a/dlls/mshtml/dispex.c +++ b/dlls/mshtml/dispex.c @@ -1909,6 +1909,8 @@ static HRESULT WINAPI DispatchEx_GetNextDispID(IDispatchEx *iface, DWORD grfdex, } func++; } + + id = DISPID_STARTENUM; } if(This->info->desc->vtbl && This->info->desc->vtbl->next_dispid) { -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/965
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> Props allocated with dispex_get_dprop_ref or dispex_get_dynid are purely internal to our implementation and must not be enumerated. Note that in case of window, the props themselves become enumerable, but the dynamic props must still be hidden, since it's the custom prop that refers to it that must be enumerated (i.e. the DISPID must match with the custom prop, not the underlying dynamic prop backing it, which would violate the former DISPID obtained for the respective name). Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/mshtml/dispex.c | 7 +++++- dlls/mshtml/htmlwindow.c | 16 +++++++++++- dlls/mshtml/tests/documentmode.js | 42 ++++++++++++++++++++++++++++++- 3 files changed, 62 insertions(+), 3 deletions(-) diff --git a/dlls/mshtml/dispex.c b/dlls/mshtml/dispex.c index d7b587f21d2..3d3d854936e 100644 --- a/dlls/mshtml/dispex.c +++ b/dlls/mshtml/dispex.c @@ -85,6 +85,7 @@ typedef struct { } dynamic_prop_t; #define DYNPROP_DELETED 0x01 +#define DYNPROP_HIDDEN 0x02 typedef struct { DispatchEx dispex; @@ -702,6 +703,8 @@ HRESULT dispex_get_dprop_ref(DispatchEx *This, const WCHAR *name, BOOL alloc, VA if(FAILED(hres)) return hres; + if(alloc) + prop->flags |= DYNPROP_HIDDEN; *ret = &prop->var; return S_OK; } @@ -715,6 +718,7 @@ HRESULT dispex_get_dynid(DispatchEx *This, const WCHAR *name, DISPID *id) if(FAILED(hres)) return hres; + prop->flags |= DYNPROP_HIDDEN; *id = DISPID_DYNPROP_0 + (prop - This->dynamic_data->props); return S_OK; } @@ -1860,7 +1864,8 @@ static HRESULT WINAPI DispatchEx_GetMemberName(IDispatchEx *iface, DISPID id, BS static HRESULT next_dynamic_id(DispatchEx *dispex, DWORD idx, DISPID *ret_id) { - while(idx < dispex->dynamic_data->prop_cnt && dispex->dynamic_data->props[idx].flags & DYNPROP_DELETED) + while(idx < dispex->dynamic_data->prop_cnt && + (dispex->dynamic_data->props[idx].flags & (DYNPROP_DELETED | DYNPROP_HIDDEN))) idx++; if(idx == dispex->dynamic_data->prop_cnt) { diff --git a/dlls/mshtml/htmlwindow.c b/dlls/mshtml/htmlwindow.c index 02d6d374255..12f8a304d41 100644 --- a/dlls/mshtml/htmlwindow.c +++ b/dlls/mshtml/htmlwindow.c @@ -3874,6 +3874,20 @@ static HRESULT HTMLWindow_invoke(DispatchEx *dispex, DISPID id, LCID lcid, WORD 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; + HTMLInnerWindow *This = impl_from_DispatchEx(dispex); + + while(idx < This->global_prop_cnt && This->global_props[idx].type != GLOBAL_DISPEXVAR) + idx++; + if(idx >= This->global_prop_cnt) + return S_FALSE; + + *pid = idx + MSHTML_DISPID_CUSTOM_MIN; + return S_OK; +} + static compat_mode_t HTMLWindow_get_compat_mode(DispatchEx *dispex) { HTMLInnerWindow *This = impl_from_DispatchEx(dispex); @@ -3971,7 +3985,7 @@ static const event_target_vtbl_t HTMLWindow_event_target_vtbl = { HTMLWindow_get_name, HTMLWindow_invoke, NULL, - NULL, + HTMLWindow_next_dispid, HTMLWindow_get_compat_mode, NULL }, diff --git a/dlls/mshtml/tests/documentmode.js b/dlls/mshtml/tests/documentmode.js index cb6d2d01ec2..a3a0e2439cd 100644 --- a/dlls/mshtml/tests/documentmode.js +++ b/dlls/mshtml/tests/documentmode.js @@ -635,6 +635,8 @@ sync_test("JS objs", function() { }); sync_test("for..in", function() { + var v = document.documentMode, found = 0, r; + function ctor() {} ctor.prototype.test2 = true; @@ -642,7 +644,7 @@ sync_test("for..in", function() { obj.test1 = true; i = 0; - for(var r in obj) { + for(r in obj) { ctor.prototype.test3 = true; arr[r] = true; i++; @@ -652,10 +654,22 @@ sync_test("for..in", function() { ok(arr["test1"] === true, "arr[test1] !== true"); ok(arr["test2"] === true, "arr[test2] !== true"); ok(arr["test3"] === true, "arr[test3] !== true"); + + for(r in document) + if(r === "ondragstart") + found++; + ok(found === 1, "ondragstart enumerated " + found + " times in document"); + document.ondragstart = ""; + found = 0; + for(r in document) + if(r === "ondragstart") + found++; + ok(found === 1, "ondragstart enumerated " + found + " times in document after set to empty string"); }); sync_test("elem_by_id", function() { document.body.innerHTML = '<form id="testid" name="testname"></form>'; + var found; var id_elem = document.getElementById("testid"); ok(id_elem.tagName === "FORM", "id_elem.tagName = " + id_elem.tagName); @@ -665,6 +679,32 @@ sync_test("elem_by_id", function() { ok(id_elem === name_elem, "id_elem != id_elem"); else ok(name_elem === null, "name_elem != null"); + + id_elem = window.testid; + ok(id_elem.tagName === "FORM", "window.testid = " + id_elem); + + name_elem = document.testname; + ok(name_elem.tagName === "FORM", "document.testname = " + name_elem); + + for(id_elem in window) + ok(id_elem !== "testid" && id_elem != "testname", id_elem + " was enumerated in window"); + window.testid = 137; + found = false; + for(id_elem in window) { + ok(id_elem != "testname", id_elem + " was enumerated in window after set to 137"); + if(id_elem === "testid") + found = true; + } + ok(found, "testid was not enumerated in window after set to 137"); + + found = false; + for(id_elem in document) { + ok(id_elem !== "testid", "testid was enumerated in document"); + if(id_elem === "testname") + found = true; + } + todo_wine. + ok(found, "testname was not enumerated in document"); }); sync_test("doc_mode", function() { -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/965
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=124719 Your paranoid android. === w10pro64 (64 bit report) === mshtml: htmldoc.c:350: Test failed: expected Exec_SETTITLE htmldoc.c:2859: Test failed: unexpected call Exec_SETTITLE
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/mshtml/htmldoc.c | 106 ++++++++++++++++++++++++++---- dlls/mshtml/tests/documentmode.js | 1 - 2 files changed, 92 insertions(+), 15 deletions(-) diff --git a/dlls/mshtml/htmldoc.c b/dlls/mshtml/htmldoc.c index 0c96d73eb76..5a928e68fe1 100644 --- a/dlls/mshtml/htmldoc.c +++ b/dlls/mshtml/htmldoc.c @@ -4818,7 +4818,7 @@ static inline HTMLDocument *impl_from_IDispatchEx(IDispatchEx *iface) return CONTAINING_RECORD(iface, HTMLDocument, IDispatchEx_iface); } -static HRESULT dispid_from_elem_name(HTMLDocumentNode *This, BSTR name, DISPID *dispid) +static HRESULT dispid_from_elem_name(HTMLDocumentNode *This, const WCHAR *name, DISPID *dispid, BOOL enum_name) { nsIDOMNodeList *node_list; nsAString name_str; @@ -4829,19 +4829,21 @@ static HRESULT dispid_from_elem_name(HTMLDocumentNode *This, BSTR name, DISPID * if(!This->nsdoc) return DISP_E_UNKNOWNNAME; - nsAString_InitDepend(&name_str, name); - nsres = nsIDOMHTMLDocument_GetElementsByName(This->nsdoc, &name_str, &node_list); - nsAString_Finish(&name_str); - if(NS_FAILED(nsres)) - return E_FAIL; + if(!enum_name) { + nsAString_InitDepend(&name_str, name); + nsres = nsIDOMHTMLDocument_GetElementsByName(This->nsdoc, &name_str, &node_list); + nsAString_Finish(&name_str); + if(NS_FAILED(nsres)) + return E_FAIL; - nsres = nsIDOMNodeList_GetLength(node_list, &len); - nsIDOMNodeList_Release(node_list); - if(NS_FAILED(nsres)) - return E_FAIL; + nsres = nsIDOMNodeList_GetLength(node_list, &len); + nsIDOMNodeList_Release(node_list); + if(NS_FAILED(nsres)) + return E_FAIL; - if(!len) - return DISP_E_UNKNOWNNAME; + if(!len) + return DISP_E_UNKNOWNNAME; + } for(i=0; i < This->elem_vars_cnt; i++) { if(!wcscmp(name, This->elem_vars[i])) { @@ -4943,7 +4945,7 @@ static HRESULT WINAPI DocDispatchEx_GetDispID(IDispatchEx *iface, BSTR bstrName, if(hres != DISP_E_UNKNOWNNAME) return hres; - return dispid_from_elem_name(This->doc_node, bstrName, pid); + return dispid_from_elem_name(This->doc_node, bstrName, pid, FALSE); } static HRESULT WINAPI DocDispatchEx_InvokeEx(IDispatchEx *iface, DISPID id, LCID lcid, WORD wFlags, DISPPARAMS *pdp, @@ -5885,6 +5887,82 @@ static HRESULT HTMLDocumentNode_invoke(DispatchEx *dispex, DISPID id, LCID lcid, return S_OK; } +static HRESULT HTMLDocumentNode_next_dispid(DispatchEx *dispex, DISPID id, DISPID *pid) +{ + DWORD idx = (id == DISPID_STARTENUM) ? 0 : id - MSHTML_DISPID_CUSTOM_MIN + 1; + HTMLDocumentNode *This = impl_from_DispatchEx(dispex); + nsIDOMNodeList *node_list; + const PRUnichar *name; + nsIDOMElement *nselem; + nsIDOMNode *nsnode; + nsAString nsstr; + nsresult nsres; + UINT32 i, len; + HRESULT hres; + + if(!This->nsdoc) + return S_FALSE; + + while(idx < This->elem_vars_cnt) { + nsAString_InitDepend(&nsstr, This->elem_vars[idx]); + nsres = nsIDOMHTMLDocument_GetElementsByName(This->nsdoc, &nsstr, &node_list); + nsAString_Finish(&nsstr); + if(NS_FAILED(nsres)) + return map_nsresult(nsres); + + nsres = nsIDOMNodeList_GetLength(node_list, &len); + nsIDOMNodeList_Release(node_list); + if(NS_FAILED(nsres)) + return map_nsresult(nsres); + + if(len) { + *pid = idx + MSHTML_DISPID_CUSTOM_MIN; + return S_OK; + } + idx++; + } + + /* Populate possibly missing DISPIDs */ + nsAString_InitDepend(&nsstr, L"[name]"); + nsres = nsIDOMHTMLDocument_QuerySelectorAll(This->nsdoc, &nsstr, &node_list); + nsAString_Finish(&nsstr); + if(NS_FAILED(nsres)) + return map_nsresult(nsres); + + for(i = 0, hres = S_OK; SUCCEEDED(hres); i++) { + nsres = nsIDOMNodeList_Item(node_list, i, &nsnode); + if(NS_FAILED(nsres)) { + hres = map_nsresult(nsres); + break; + } + if(!nsnode) + break; + + nsres = nsIDOMNode_QueryInterface(nsnode, &IID_nsIDOMElement, (void**)&nselem); + nsIDOMNode_Release(nsnode); + if(nsres != S_OK) + continue; + + nsres = get_elem_attr_value(nselem, L"name", &nsstr, &name); + nsIDOMElement_Release(nselem); + if(NS_FAILED(nsres)) + hres = map_nsresult(nsres); + else { + hres = dispid_from_elem_name(This, name, &id, TRUE); + nsAString_Finish(&nsstr); + } + } + nsIDOMNodeList_Release(node_list); + if(FAILED(hres)) + return hres; + + if(idx >= This->elem_vars_cnt) + return S_FALSE; + + *pid = idx + MSHTML_DISPID_CUSTOM_MIN; + return S_OK; +} + static compat_mode_t HTMLDocumentNode_get_compat_mode(DispatchEx *dispex) { HTMLDocumentNode *This = impl_from_DispatchEx(dispex); @@ -5943,7 +6021,7 @@ static const event_target_vtbl_t HTMLDocumentNode_event_target_vtbl = { HTMLDocumentNode_get_name, HTMLDocumentNode_invoke, NULL, - NULL, + HTMLDocumentNode_next_dispid, HTMLDocumentNode_get_compat_mode, NULL }, diff --git a/dlls/mshtml/tests/documentmode.js b/dlls/mshtml/tests/documentmode.js index a3a0e2439cd..ad258b3df2e 100644 --- a/dlls/mshtml/tests/documentmode.js +++ b/dlls/mshtml/tests/documentmode.js @@ -703,7 +703,6 @@ sync_test("elem_by_id", function() { if(id_elem === "testname") found = true; } - todo_wine. ok(found, "testname was not enumerated in document"); }); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/965
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=124720 Your paranoid android. === w10pro64 (32 bit report) === mshtml: htmldoc.c:350: Test failed: expected Exec_SETTITLE htmldoc.c:2859: Test failed: unexpected call Exec_SETTITLE
On Sun Oct 2 17:53:38 2022 +0000, Jacek Caban wrote:
Please add a test case. So testing this revealed some other, already-existing issues.
Nevertheless, the patch is obviously correct for what it does, it's just that I had to add extra fixes. For example, for event handlers which end up using dispex_get_dprop_ref when set to string, purely due to how we implement it internally, the props themselves are already enumerable without this. However, we enumerate them twice in this case, since we also enum the internal dynamic prop, which is wrong. Similarly, for window, the property replacing the element id prop does, in fact, become enumerable. But it's *not* correct to enumerate the dynamic prop, because it will have a different DISPID (dynamic DISPID) instead of the proper one it had before. So it requires an extra fix here which I've now done. I also found some other issues with doc/window accessing elements via props, but I'll leave that for another MR. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/965#note_9959
Jacek Caban (@jacek) commented about dlls/mshtml/htmldoc.c:
return CONTAINING_RECORD(iface, HTMLDocument, IDispatchEx_iface); }
-static HRESULT dispid_from_elem_name(HTMLDocumentNode *This, BSTR name, DISPID *dispid) +static HRESULT dispid_from_elem_name(HTMLDocumentNode *This, const WCHAR *name, DISPID *dispid, BOOL enum_name)
I think it would be better to move !enum_name handling out of this function instead of adding an extra argument here. If you move that to a separated function, it could also be used in HTMLDocumentNode_next_dispid instead of duplicating it. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/965#note_10100
participants (3)
-
Gabriel Ivăncescu -
Jacek Caban (＠jacek)
-
Marvin