[PATCH v2 2/2] msxml3: Don't unlink the XML declaration when saving document.
Signed-off-by: Dmitry Timoshkov <dmitry(a)baikal.ru> --- dlls/msxml3/domdoc.c | 3 --- dlls/msxml3/tests/domdoc.c | 4 ++-- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/dlls/msxml3/domdoc.c b/dlls/msxml3/domdoc.c index 80c32e9ba99..a81ef5f16cb 100644 --- a/dlls/msxml3/domdoc.c +++ b/dlls/msxml3/domdoc.c @@ -2534,7 +2534,6 @@ static HRESULT WINAPI domdoc_save( { domdoc *This = impl_from_IXMLDOMDocument3( iface ); xmlSaveCtxtPtr ctx = NULL; - xmlNodePtr xmldecl; HRESULT ret = S_OK; TRACE("(%p)->(%s)\n", This, debugstr_variant(&destination)); @@ -2610,9 +2609,7 @@ static HRESULT WINAPI domdoc_save( return S_FALSE; } - xmldecl = xmldoc_unlink_xmldecl(get_doc(This)); if (xmlSaveDoc(ctx, get_doc(This)) == -1) ret = S_FALSE; - xmldoc_link_xmldecl(get_doc(This), xmldecl); /* will release resources through close callback */ xmlSaveClose(ctx); diff --git a/dlls/msxml3/tests/domdoc.c b/dlls/msxml3/tests/domdoc.c index 5bc2fe1d5c3..72968fff11b 100644 --- a/dlls/msxml3/tests/domdoc.c +++ b/dlls/msxml3/tests/domdoc.c @@ -8524,6 +8524,7 @@ static void test_createProcessingInstruction(void) { static const WCHAR xml1[] = L"<?xml version=\"1.0\"?>\r\n<test/>\r\n"; static const char xml2[] = "<?xml version=\"1.0\" encoding=\"windows-1252\"?>\r\n<test/>\r\n"; + static const char xml2_wine[] = "<?xml version=\"1.0\" encoding=\"windows-1252\"?>\n<test/>\n"; IXMLDOMProcessingInstruction *pi; IXMLDOMDocument *doc; IXMLDOMNode *node; @@ -8582,8 +8583,7 @@ todo_wine ok(hr == S_OK, "got 0x%08x\n", hr); p = GlobalLock(global); p[GlobalSize(global)] = 0; -todo_wine - ok(!strcmp(p, xml2), "got %s\n", wine_dbgstr_a(p)); + ok(!strcmp(p, xml2) || !strcmp(p, xml2_wine), "got %s\n", wine_dbgstr_a(p)); GlobalUnlock(global); IStream_Release(stream); -- 2.30.2
Nikolay Sivov <nsivov(a)codeweavers.com> wrote:
Signed-off-by: Nikolay Sivov <nsivov(a)codeweavers.com>
Please don't commit this, while the patch is correct, it causes a regression: ::load()/::save() leads to a bug: the saved file has double <?xml> header, and needs another fix: doparse() should never add an xml declaration to a just loaded XML. Nikolay, do you recall why that code was added in the first place? I'll add more tests for such a case. -- Dmitry.
On 4/5/21 2:27 PM, Dmitry Timoshkov wrote:
Nikolay Sivov <nsivov(a)codeweavers.com> wrote:
Signed-off-by: Nikolay Sivov <nsivov(a)codeweavers.com> Please don't commit this, while the patch is correct, it causes a regression: ::load()/::save() leads to a bug: the saved file has double <?xml> header, and needs another fix: doparse() should never add an xml declaration to a just loaded XML. Thanks for catching that.
Nikolay, do you recall why that code was added in the first place?
I was a long time ago. I think the reason is that libxml2 does not use a separate node for prolog PI, and msxml does. After the loading we have to expose prolog node as a normal child. I don't think it always adds it, the meaning of 'doc->standalone != -1' check is to add this node only if it was present in the input.
I'll add more tests for such a case.
Nikolay Sivov <nsivov(a)codeweavers.com> wrote:
Signed-off-by: Nikolay Sivov <nsivov(a)codeweavers.com> Please don't commit this, while the patch is correct, it causes a regression: ::load()/::save() leads to a bug: the saved file has double <?xml> header, and needs another fix: doparse() should never add an xml declaration to a just loaded XML. Thanks for catching that.
Nikolay, do you recall why that code was added in the first place?
I was a long time ago. I think the reason is that libxml2 does not use a separate node for prolog PI, and msxml does. After the loading we have to expose prolog node as a normal child.
I don't think it always adds it, the meaning of 'doc->standalone != -1' check is to add this node only if it was present in the input.
Do you have an alternative solution to removing that code from doparse()? Because doing so breaks the tests. I'd appreciate a bit of help on this. -- Dmitry.
On 4/5/21 2:40 PM, Dmitry Timoshkov wrote:
Nikolay Sivov <nsivov(a)codeweavers.com> wrote:
Signed-off-by: Nikolay Sivov <nsivov(a)codeweavers.com> Please don't commit this, while the patch is correct, it causes a regression: ::load()/::save() leads to a bug: the saved file has double <?xml> header, and needs another fix: doparse() should never add an xml declaration to a just loaded XML. Thanks for catching that. Nikolay, do you recall why that code was added in the first place? I was a long time ago. I think the reason is that libxml2 does not use a separate node for prolog PI, and msxml does. After the loading we have to expose prolog node as a normal child.
I don't think it always adds it, the meaning of 'doc->standalone != -1' check is to add this node only if it was present in the input. Do you have an alternative solution to removing that code from doparse()? Because doing so breaks the tests. I'd appreciate a bit of help on this. There is no good way that I'm aware of. Node has to be there, applications expect that. If the issue is save/get_xml() output, we could reimplement node serialization similar to what libxml2, but in a way we want it.
To get rid of linking/unlinking we'll have to get rid of libxml2 as a dependency, which probably could still use libxslt. But that's really a lot of work and planning (sax, schema validation, xpath, parsing itself).
Nikolay Sivov <nsivov(a)codeweavers.com> wrote:
On 4/5/21 2:40 PM, Dmitry Timoshkov wrote:
Nikolay Sivov <nsivov(a)codeweavers.com> wrote:
Signed-off-by: Nikolay Sivov <nsivov(a)codeweavers.com> Please don't commit this, while the patch is correct, it causes a regression: ::load()/::save() leads to a bug: the saved file has double <?xml> header, and needs another fix: doparse() should never add an xml declaration to a just loaded XML. Thanks for catching that. Nikolay, do you recall why that code was added in the first place? I was a long time ago. I think the reason is that libxml2 does not use a separate node for prolog PI, and msxml does. After the loading we have to expose prolog node as a normal child.
I don't think it always adds it, the meaning of 'doc->standalone != -1' check is to add this node only if it was present in the input. Do you have an alternative solution to removing that code from doparse()? Because doing so breaks the tests. I'd appreciate a bit of help on this. There is no good way that I'm aware of. Node has to be there, applications expect that. If the issue is save/get_xml() output, we could reimplement node serialization similar to what libxml2, but in a way we want it.
To get rid of linking/unlinking we'll have to get rid of libxml2 as a dependency, which probably could still use libxslt. But that's really a lot of work and planning (sax, schema validation, xpath, parsing itself).
I'm not proposing to blindly remove the linking/unlinking the xml declaration code. What I would look into is revisit the doparse() code that adds the PI node with an XML declaration in case of doc->standalone = -2. According to libxml2 source doc->standalone = -2 indicates that the XML declaration *is* present however the standalone attribute is not, and in that case another (the 2nd one) XML declaration gets inserted as an PI node, however libxml2 couldn't figure that out which leads to creating double <?xml> header. -- Dmitry.
Dmitry Timoshkov <dmitry(a)baikal.ru> wrote:
Nikolay Sivov <nsivov(a)codeweavers.com> wrote:
On 4/5/21 2:40 PM, Dmitry Timoshkov wrote:
Nikolay Sivov <nsivov(a)codeweavers.com> wrote:
Signed-off-by: Nikolay Sivov <nsivov(a)codeweavers.com> Please don't commit this, while the patch is correct, it causes a regression: ::load()/::save() leads to a bug: the saved file has double <?xml> header, and needs another fix: doparse() should never add an xml declaration to a just loaded XML. Thanks for catching that. Nikolay, do you recall why that code was added in the first place? I was a long time ago. I think the reason is that libxml2 does not use a separate node for prolog PI, and msxml does. After the loading we have to expose prolog node as a normal child.
I don't think it always adds it, the meaning of 'doc->standalone != -1' check is to add this node only if it was present in the input. Do you have an alternative solution to removing that code from doparse()? Because doing so breaks the tests. I'd appreciate a bit of help on this. There is no good way that I'm aware of. Node has to be there, applications expect that. If the issue is save/get_xml() output, we could reimplement node serialization similar to what libxml2, but in a way we want it.
To get rid of linking/unlinking we'll have to get rid of libxml2 as a dependency, which probably could still use libxslt. But that's really a lot of work and planning (sax, schema validation, xpath, parsing itself).
I'm not proposing to blindly remove the linking/unlinking the xml declaration code. What I would look into is revisit the doparse() code that adds the PI node with an XML declaration in case of doc->standalone = -2. According to libxml2 source doc->standalone = -2 indicates that the XML declaration *is* present however the standalone attribute is not, and in that case another (the 2nd one) XML declaration gets inserted as an PI node, however libxml2 couldn't figure that out which leads to creating double <?xml> header.
I think that I've found a solution that doesn't break the tests. I'll send a new version of the patch. -- Dmitry.
participants (2)
-
Dmitry Timoshkov -
Nikolay Sivov