On 6/12/2012 11:55, Ulrik Dickow wrote:
Patch 2 of 2 from http://bugs.winehq.org/show_bug.cgi?id=26226 .
- /* Emit appropriate fixme or warning in case of unsupported or invalid namespace.
* The W3C spec would like us to exit earlier for attempts to redefine the namespace for* the reserved "xmlns" attribute or prefix (http://www.w3.org/TR/xml-names/#ns-decl),* likehttp://gdome2.cs.unibo.it/gtk-doc/gdome2-gdomeelement.html#GDOME-EL-SETATTRIBUTENS,* but since native msxml3 accepts anything here, it's safest to just continue with warning.*/
The question is more like what libxml2 thinks about that. If it follows w3c here which is likely the question will be - won't it break somewhere internally in libxml2 code if we hack it that way?
- if(namespaceURI && namespaceURI[0] && node_type != NODE_ELEMENT)
- {
if(node_type == NODE_ATTRIBUTE)switch((!strcmpW(name, xmlnsW) ||!strncmpW(name, xmlnscW, sizeof(xmlnscW)/sizeof(WCHAR))) +!strcmpW(namespaceURI, w3xmlns))
Could you please simplify that, it's a bit hard to read.
* Note: We would be more in line with the native msxml if we only detected a* redundancy/conflict for prefixes existing on the element itself (in element->ns* or in the linked list element->nsDef). But as long as our *_get_xml() doesn't* eliminate redundant namespace definitions in children, this behaviour may be* an advantage in some cases. As disadvantage is that we incorrectly regard the* redefinition in element 'b' in '<a xmlns="x"><b xmlns="y"/></a>' as a conflict.* Libxml2 currently doesn't have a function or option to only search for namespaces* on the current node; but it would be easy to make a reduced version of xmlSearchNs* (currently defined inhttp://git.gnome.org/browse/libxml2/tree/tree.c#n5871).* However, most apps probably set the ns to y on node b creation, so no conflict here.*
I don't think it's better to potentially break one thing while fixing the other. And I guess I like a thought on making xmlSearchNs version that will do what we want from it. Just to clarify - is that right that libxml2 problem here is a lack of some checks that lead to duplication and conflicts later? (that are visible in doc dumps too)
+/* Convenient helper function for creating an attribute node in a given namespace.
- Note: CHECK_HR() should not be here, but in caller, to give optimal line number info.
- */
You mean EXPECT_HR here I guess.
+static HRESULT create_attribute_ns(
- IXMLDOMDocument *doc,
- const char *attr_name,
- const char *nsURI,
- IXMLDOMAttribute **attr_ptr)
+{
- HRESULT hr;
- VARIANT var;
- IXMLDOMNode *node;
- V_VT(&var) = VT_I1;
- V_I1(&var) = NODE_ATTRIBUTE;
- hr = IXMLDOMDocument_createNode(doc, var,_bstr_(attr_name),_bstr_(nsURI), &node);
- if (hr == S_OK)
- {
IXMLDOMNode_QueryInterface(node, &IID_IXMLDOMAttribute, (void**) attr_ptr);IXMLDOMNode_Release(node);- }
- return hr;
+}
Please null out pointer in case of failure, or better always before createNode().
Den 12-06-2012 13:21, Nikolay Sivov skrev:
On 6/12/2012 11:55, Ulrik Dickow wrote:
Patch 2 of 2 from http://bugs.winehq.org/show_bug.cgi?id=26226 .
- /* Emit appropriate fixme or warning in case of unsupported or invalid namespace.
* The W3C spec would like us to exit earlier for attempts to redefine the namespace for* the reserved "xmlns" attribute or prefix (http://www.w3.org/TR/xml-names/#ns-decl),* like http://gdome2.cs.unibo.it/gtk-doc/gdome2-gdomeelement.html#GDOME-EL-SETATTRIBUTENS,* but since native msxml3 accepts anything here, it's safest to just continue with warning.*/The question is more like what libxml2 thinks about that. If it follows w3c here which is likely the question will be - won't it break somewhere internally in libxml2 code if we hack it that way?
I don't think so. We have already seen that libxml2 doesn't consider the attribute "xmlns" to have any special meaning in relation to namespaces, when setting an attribute. It hasn't given any problems in my tests.
But maybe you are right that it is safer to return E_INVALIDARG, as I did in an earlier version, to guard against future increased intelligence in libxml2. Both of the applications in bug 26226 use the correct W3C namespace, so they don't care. I'll change it back.
- if(namespaceURI && namespaceURI[0] && node_type != NODE_ELEMENT)
- {
if(node_type == NODE_ATTRIBUTE)switch((!strcmpW(name, xmlnsW) ||!strncmpW(name, xmlnscW, sizeof(xmlnscW)/sizeof(WCHAR))) +!strcmpW(namespaceURI, w3xmlns))Could you please simplify that, it's a bit hard to read.
I guess you mean I simplified it too much. But yes, it can be hard to read/understand. I'll offer an expanded version using logical variables and maybe several if-then-else instead of this overly simple switch.
BTW, as a funny side-note, at first I had added a "default:" clause with a FIXME (or similar) stating that this should be impossible. 'gcc -O2' was intelligent enough to know that for logical variables a and b in C,
a + b
can only be 0 (both are false), 1 (exactly one is true) or 2 (both are true), so it completely optimized away the FIXME-string/statement from the binary.
* Note: We would be more in line with the native msxml if we only detected a* redundancy/conflict for prefixes existing on the element itself (in element->ns* or in the linked list element->nsDef). But as long as our *_get_xml() doesn't* eliminate redundant namespace definitions in children, this behaviour may be* an advantage in some cases. As disadvantage is that we incorrectly regard the* redefinition in element 'b' in '<a xmlns="x"><b xmlns="y"/></a>' as a conflict.* Libxml2 currently doesn't have a function or option to only search for namespaces* on the current node; but it would be easy to make a reduced version of xmlSearchNs* (currently defined in http://git.gnome.org/browse/libxml2/tree/tree.c#n5871).* However, most apps probably set the ns to y on node b creation, so no conflict here.*I don't think it's better to potentially break one thing while fixing the other.
In current wine, attributes named "xmlns" don't work at all in setAttributeNode (an error code is returned instead of S_OK, and no attribute is added, no matter whether a namespace is already present or not at this level).
So fixing this issue doesn't break anything that worked before. I consider it a step-wise improvement. Something better, nothing worse.
I wanted to keep that patch as small and simple as possible. setAttribute also has this "dumb" use of xmlSearchNs.
And I guess I like a thought on making xmlSearchNs version that will do what we want from it.
Ok, actually it'll only be about 10 lines of extra code, roughly, so I'll add that -- and shrink the comment, less worries, more code :).
Just to clarify - is that right that libxml2 problem here is a lack of some checks that lead to duplication and conflicts later? (that are visible in doc dumps too)
Well, yes, in a way: it doesn't check whether we try to set attributes named "xmlns" or use prefix "xmlns" or use the namespaceURI "http://www.w3.org/2000/xmlns/". But maybe it shouldn't. It's a matter of policy -- giving higher layers the freedom to enforce restrictions themselves.
It turns out that the native msxml3 doesn't enforce restrictions. As the doc dumps show, it too can give
1) duplication when setAttribute is used (but not when setAttributeNode is used)
2) conflicts between the namespaces in visible XML (get_xml) versus what get_namespaceURI tells us, both for setAttribute and setAttributeNode
The most problematic lack in libxml2 is not about treating (input) attributes specially, but rather the whole concept of treating the ns fields as "directives to print an xmlns attribute". It doesn't map well with the native msxml concept of treating namespaces as just information that this element _belongs_ to this namespace. Whether or not an xmlns attribute should be printed (returned from get_xml), depends on what part of the tree the user wants to look at. If the user asks for XML for a parent, a child in the same namespace doesn't have the xmlns attribute repeated at its level (due to ns fields).
It would be nice to have XML-generation functions in libxml2 that suppressed child xmlns output similarly. Fortunately most applications probably don't care about this extra XML (BridgeCentral doesn't).
+/* Convenient helper function for creating an attribute node in a given namespace.
- Note: CHECK_HR() should not be here, but in caller, to give optimal line number info.
- */
You mean EXPECT_HR here I guess.
Yup, sorry.
+static HRESULT create_attribute_ns(
- IXMLDOMDocument *doc,
- const char *attr_name,
- const char *nsURI,
- IXMLDOMAttribute **attr_ptr)
+{
- HRESULT hr;
- VARIANT var;
- IXMLDOMNode *node;
- V_VT(&var) = VT_I1;
- V_I1(&var) = NODE_ATTRIBUTE;
- hr = IXMLDOMDocument_createNode(doc, var, _bstr_(attr_name), _bstr_(nsURI), &node);
- if (hr == S_OK)
- {
IXMLDOMNode_QueryInterface(node, &IID_IXMLDOMAttribute, (void**) attr_ptr);IXMLDOMNode_Release(node);- }
- return hr;
+}
Please null out pointer in case of failure, or better always before createNode().
Ok.
I'll return to this patch -- merge, apply requested changes, test again -- a week from now. I'm unfortunately occupied elsewhere in the mean time. Thank you for your patience and quick & useful replies.
- Ulrik
Hello again Nikolay and others,
I'm back online. The first attachment is try 2 of the second patch, i.e. the one discussed in this thread. I hope it meets all of your concerns. I adjusted my plan slightly at this point:
Den 12-06-2012 13:55, Ulrik Dickow skrev:
Den 12-06-2012 13:21, Nikolay Sivov skrev:
On 6/12/2012 11:55, Ulrik Dickow wrote:
Patch 2 of 2 from http://bugs.winehq.org/show_bug.cgi?id=26226 . [...]
- /* Emit appropriate fixme or warning in case of unsupported or invalid namespace.
[...]
The question is more like what libxml2 thinks about that. If it follows w3c here which is likely the question will be - won't it break somewhere internally in libxml2 code if we hack it that way?
[...] But maybe you are right that it is safer to return E_INVALIDARG, as I did in an earlier version, to guard against future increased intelligence in libxml2. Both of the applications in bug 26226 use the correct W3C namespace, so they don't care. I'll change it back.
The new patch returns E_NOTIMPL instead. That seems more appropriate to me, knowing that native msxml3 returns S_OK (like the first version of my patch) and that we will have to change it back if it turns out to break a real-life application. Evil apps that try to break us intentionally don't count :).
The new patch gives exactly the same output for the tst-msxml_make_soap.c program as the old one. Consistent with this test, it also still succesfully fixes bug 26226 for BridgeCentral: BridgeCentral's SOAP requests again succeed perfectly.
The new patch changes the output of the tst-msxml_xmlns_simple.c program. The new E_NOTIMPL behaviour actually made the original program crash because I failed to do proper error checking (see second attachment if you like, patch relative to the C attached to bugzilla).
The new output of the fixed tst-msxml_xmlns_simple.c is in the third attachment. The differences to the old wine patch are in test 03 (only the FIXME text changed) and in test 04 (FAILures instead of ok, and now quite detailed FIXME texts instead of more general, invisible warnings).
Of course, only the first attachment will be sent to wine-patches, and only if no objections appear here to prevent it.
- Ulrik
On 24-06-2012 23:55, Ulrik Dickow wrote:
This resolves bug #26226.
Should be corrected to read something like "This resolves bug #26226 for some, but not all applications", as noted in my comment on http://bugs.winehq.org/show_bug.cgi?id=26226 a few minutes ago.
(I should probably also have inlined xml_search_ns_local() and "wineified" it further, e.g. "return cur" instead of "return(cur)").
- Ulrik
-
Nikolay Sivov -
Ulrik Dickow