[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 <leslie_alistair(a)hotmail.com>

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#\""
+                L"         xmlns:rdf=\"http://www.w3.org/1999/02/22-rdf-syntax-ns#\""
+                L"         xmlns:dcterms=\"http://purl.org/dc/terms/\""
+                L"         xmlns:foaf=\"http://xmlns.com/foaf/0.1/\" >"
+                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();
-- 
GitLab

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

[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?

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

[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.

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

[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.

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