[PATCH v2 0/1] MR6993: msxml3: Correct looping of Document Element node map.

[Alistair Leslie-Hughes (＠alesliehughes)]

The basic loop correctly finds the first element, however
the second element repeats the first node, since it was
incremented and the ns->next was skipped over.

--
  v2: msxml3: Correct looping of Document Element node map.

https://gitlab.winehq.org/wine/wine/-/merge_requests/6993

[PATCH v2 1/1] msxml3: Correct looping of Document Element node map.

[Alistair Leslie-Hughes]

From: Alistair Leslie-Hughes

The basic loop correctly finds the first element, however
the second element repeats the first node, since it was
incremented and the ns->next was skipped over.
---
 dlls/msxml3/element.c      |  3 +-
 dlls/msxml3/tests/domdoc.c | 63 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 65 insertions(+), 1 deletion(-)

diff --git a/dlls/msxml3/element.c b/dlls/msxml3/element.c
index 394a4b28201..cc56869b527 100644
--- a/dlls/msxml3/element.c
+++ b/dlls/msxml3/element.c
@@ -1819,8 +1819,9 @@ static HRESULT domelem_get_item(const xmlNodePtr node, LONG index, IXMLDOMNode *
     if (!node->nsDef)
         return S_FALSE;

-    attrIndex++;
     ns = node->nsDef;
+    if (!ns->next)
+        attrIndex++;
     for (; attrIndex < index && ns->next != NULL; attrIndex++)
         ns = ns->next;

diff --git a/dlls/msxml3/tests/domdoc.c b/dlls/msxml3/tests/domdoc.c
index 81f1235cd9b..e497fabbae0 100644
--- a/dlls/msxml3/tests/domdoc.c
+++ b/dlls/msxml3/tests/domdoc.c
@@ -14207,6 +14207,68 @@ static void test_indent(void)
     SysFreeString(str);
 }

+static void test_document_namespaces(void)
+{
+    const char *namespaces[] =
+        {
+            "xmlns:oslc_am",
+            "xmlns:rdf",
+            "xmlns:dcterms",
+            "xmlns:foaf",
+        };
+    IXMLDOMDocument *doc;
+    IXMLDOMElement *nodeRoot;
+    VARIANT_BOOL b;
+    HRESULT hr;
+    IXMLDOMNamedNodeMap *map;
+    LONG len, i;
+    BSTR nodename, str;
+
+    str = SysAllocString(L"<?xml version=\"1.0\" encoding=\"UTF-8\"?>"
+                L"<rdf:RDF xmlns:oslc_am="http://open-services.net/ns/am#%5C""
+                L"         xmlns:rdf="http://www.w3.org/1999/02/22-rdf-syntax-ns#%5C""
+                L"         xmlns:dcterms="http://purl.org/dc/terms/%5C""
+                L"         xmlns:foaf="http://xmlns.com/foaf/0.1/%5C" >"
+                L"</rdf:RDF>");
+
+    doc = create_document(&IID_IXMLDOMDocument2);
+
+    hr = IXMLDOMDocument_loadXML(doc, str, &b);
+    ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);
+    ok(b == VARIANT_TRUE, "got %d\n", b);
+
+    hr = IXMLDOMDocument_get_documentElement(doc, &nodeRoot);
+    ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);
+
+    hr = IXMLDOMElement_get_attributes(nodeRoot, &map);
+    ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);
+
+    len = -1;
+    hr = IXMLDOMNamedNodeMap_get_length(map, &len);
+    ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);
+    ok(len == 4, "len %#lx.\n", len);
+
+    for(i=0; i < len; i++)
+    {
+        IXMLDOMNode *pr_node = NULL;
+
+        hr = IXMLDOMNamedNodeMap_get_item(map, i, &pr_node);
+        ok( hr == S_OK, "Unexpected hr %#lx (%ld).\n", hr, i);
+
+        hr = IXMLDOMNode_get_nodeName(pr_node, &nodename);
+        ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);
+        ok(!lstrcmpW(nodename, _bstr_(namespaces[i])), "got %s\n", wine_dbgstr_w(nodename));
+        SysFreeString(nodename);
+    }
+
+    IXMLDOMNamedNodeMap_Release(map);
+    IXMLDOMElement_Release(nodeRoot);
+
+    IXMLDOMDocument_Release(doc);
+
+    free_bstrs();
+}
+
 static DWORD WINAPI new_thread(void *arg)
 {
     HRESULT hr = CoInitialize(NULL);
@@ -14261,6 +14323,7 @@ START_TEST(domdoc)
     test_testTransforms();
     test_namespaces_basic();
     test_namespaces_change();
+    test_document_namespaces();
     test_FormattingXML();
     test_nodeTypedValue();
     test_TransformWithLoadingLocalFile();

[Nikolay Sivov (＠nsivov)]

Nikolay Sivov (@nsivov) commented about dlls/msxml3/element.c:
>  if (!node->nsDef)
>      return S_FALSE;
>
> - attrIndex++;
> ns = node->nsDef;
> - if (!ns->next)
> -
>    attrIndex++;
> for (; attrIndex < index && ns->next != NULL; attrIndex++)
>        ns = ns->next;

This is a bit hard to read. Can we maybe incorporate that in loop conditions somehow instead of doing sort of unroll of the first iteration?

[Nikolay Sivov (＠nsivov)]

Nikolay Sivov (@nsivov) commented about dlls/msxml3/tests/domdoc.c:
> - hr = IXMLDOMNamedNodeMap_get_length(map, &len);
> - ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);
> - ok(len == 4, "len %#lx.\n", len);
> -
> - for(i=0; i < len; i++)
> - {
> -
>    IXMLDOMNode *pr_node = NULL;
> -
> -
>    hr = IXMLDOMNamedNodeMap_get_item(map, i, &pr_node);
> -
>    ok( hr == S_OK, "Unexpected hr %#lx (%ld).\n", hr, i);
> -
> -
>    hr = IXMLDOMNode_get_nodeName(pr_node, &nodename);
> -
>    ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);
> -
>    ok(!lstrcmpW(nodename, _bstr_(namespaces[i])), "got %s\n", wine_dbgstr_w(nodename));
> -
>    SysFreeString(nodename);
> - }

You don't need `_bstr_` here, just use WCHAR constants. It's also missing release for the pr_node.

[Nikolay Sivov (＠nsivov)]

Nikolay Sivov (@nsivov) commented about dlls/msxml3/tests/domdoc.c:
>  test_testTransforms();
>  test_namespaces_basic();
>  test_namespaces_change();
> - test_document_namespaces();

We have test_get_attributes(), maybe you can modify existing test data instead, or append new tests there.