Bug 19121 – Namespace prefix is blindly followed when serializing

• Component: DOM
• Status: RESOLVED FIXED

[Alexey Proskuryakov]

XMLSerializer uses whichever namespace prefix was specified when creating a node, even if it conflicts  with namespaces of other nodes, or is missing altogether.

[Alexey Proskuryakov]

Created attachment 21224 [details]
test case

[Rob Buis]

Created attachment 205633 [details]
Patch

[Rob Buis]

Comment on attachment 205633 [details]
Patch

Clearing review flag since this should go on top of bug 16496.

[Rob Buis]

Created attachment 205745 [details]
Patch

[Rob Buis]

(In reply to comment #4)
> Created an attachment (id=205745) [details]
> Patch

Rebase against new bug 16496 patch, and add the missing testcase.

[Rob Buis]

Created attachment 205850 [details]
Patch

[Rob Buis]

Created attachment 206831 [details]
Patch

[Rob Buis]

Created attachment 207022 [details]
Patch

[Build Bot]

Comment on attachment 207022 [details]
Patch

Attachment 207022 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1110701

New failing tests:
editing/pasteboard/paste-noscript-svg.html

[Build Bot]

Created attachment 207038 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.3

[Rob Buis]

Created attachment 207041 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 207041 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=207041&action=review

> Source/WebCore/editing/MarkupAccumulator.cpp:471
> +    builder.append("NS");

why do we want to use capital letters?

> Source/WebCore/editing/MarkupAccumulator.cpp:472
> +    builder.appendNumber(m_prefixLevel++);

What if the page had already contained NS1, NS2, etc...? It won't be unique, will it?
Also, can we try to generate a prefix that resembles the namespace URL?

[Rob Buis]

(In reply to comment #12)
> (From update of attachment 207041 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=207041&action=review
> 
> > Source/WebCore/editing/MarkupAccumulator.cpp:471
> > +    builder.append("NS");

It is stated like that in http://www.w3.org/TR/DOM-Level-3-Core/namespaces-algorithms.html#normalizeDocumentAlgo:

// find a prefix following the pattern "NS" +index (starting at 1)

Hmm, seems m_prefixLevel should start at 1, something else I should fix....

> why do we want to use capital letters?
> 
> > Source/WebCore/editing/MarkupAccumulator.cpp:472
> > +    builder.appendNumber(m_prefixLevel++);
> 
> What if the page had already contained NS1, NS2, etc...? It won't be unique, will it?
> Also, can we try to generate a prefix that resembles the namespace URL?

Oops, you are right, the next line in the pseudo algorithm code is:

// make sure this prefix is not declared in the current scope.

I'll change the patch to support this as well as add a test for it.

[Rob Buis]

Created attachment 207047 [details]
Patch

[Build Bot]

Comment on attachment 207047 [details]
Patch

Attachment 207047 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1098776

New failing tests:
fast/dom/XMLSerializer-attribute-namespace-prefix-conflicts.html
svg/custom/xlink-prefix-generation-in-attributes.html

[Build Bot]

Created attachment 207052 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-12  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.3

[Build Bot]

Comment on attachment 207047 [details]
Patch

Attachment 207047 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1098826

New failing tests:
fast/dom/XMLSerializer-attribute-namespace-prefix-conflicts.html
svg/custom/xlink-prefix-generation-in-attributes.html

[Build Bot]

Created attachment 207059 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.3

[Rob Buis]

Created attachment 207164 [details]
Patch

[Build Bot]

Comment on attachment 207164 [details]
Patch

Attachment 207164 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/1253356

New failing tests:
dom/xhtml/level2/events/dispatchEvent03.xhtml
dom/svg/level3/xpath/XPathEvaluator_evaluate_INVALID_EXPRESSION_ERR.svg
dom/html/level2/events/dispatchEvent04.html
dom/xhtml/level2/events/dispatchEvent02.xhtml
dom/svg/level3/xpath/XPathEvaluator_createExpression_INVALID_EXPRESSION_ERR.svg
dom/svg/level3/xpath/XPathEvaluator_createExpression_NAMESPACE_ERR_02.svg
dom/html/level1/core/documentinvalidcharacterexceptioncreateentref.html
dom/svg/level3/xpath/XPathEvaluator_evaluate_NOT_SUPPORTED_ERR.svg
dom/xhtml/level2/events/dispatchEvent04.xhtml
dom/svg/level3/xpath/XPathEvaluator_evaluate_NAMESPACE_ERR.svg
dom/html/level2/events/dispatchEvent01.html
dom/xhtml/level2/events/dispatchEvent01.xhtml
dom/html/level1/core/hc_attrgetvalue2.html
dom/html/level2/events/dispatchEvent03.html
dom/html/level2/events/dispatchEvent02.html
dom/html/level2/core/createDocumentType04.html
dom/xhtml/level2/events/dispatchEvent05.xhtml
dom/html/level1/core/documentinvalidcharacterexceptioncreateentref1.html
dom/html/level1/core/hc_attrappendchild2.html
dom/html/level2/events/dispatchEvent06.html
css1/basic/comments.html
dom/html/level2/events/dispatchEvent07.html
dom/html/level2/core/setAttributeNS10.html
dom/html/level2/events/dispatchEvent05.html
dom/html/level2/core/createAttributeNS06.html
dom/html/level1/core/hc_attrappendchild4.html
dom/svg/level3/xpath/XPathEvaluator_createExpression_NAMESPACE_ERR_01.svg
dom/html/level1/core/documentinvalidcharacterexceptioncreatepi1.html
dom/html/level1/core/documentinvalidcharacterexceptioncreatepi.html
dom/html/level2/core/hc_namednodemapinvalidtype1.html

[Build Bot]

Created attachment 207573 [details]
Archive of layout-test-results from APPLE-EWS-9 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: APPLE-EWS-9  Port: win-future  Platform: CYGWIN_NT-6.1-WOW64-1.7.20-0.266-5-3-i686-32bit

[Ryosuke Niwa]

Comment on attachment 207164 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=207164&action=review

> Source/WebCore/editing/MarkupAccumulator.cpp:482
> +    } while (namespaces && namespaces->get(builder.toAtomicString().impl()));
> +    prefixedName.setPrefix(builder.toString());

Shouldn't we update namespaces here?

[Rob Buis]

Comment on attachment 207164 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=207164&action=review

>> Source/WebCore/editing/MarkupAccumulator.cpp:482
>> +    prefixedName.setPrefix(builder.toString());
> 
> Shouldn't we update namespaces here?

Hmm, your comment leads me to believe I chose the wrong name, how about generateUniquePrefix? The namespace handling is not part of this method. D'oh even the m_prefixLevel name should have been a hint for me :) Also it looks like the param should be const Namespaces& to indicate it will not change. Will fix....

[Rob Buis]

Created attachment 207771 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 207771 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=207771&action=review

> Source/WebCore/editing/MarkupAccumulator.cpp:123
> +    m_prefixLevel = 0;

We don't call serializeNodes more than once, do we?

> Source/WebCore/editing/MarkupAccumulator.cpp:475
> +    // find a prefix following the pattern "NS" +index (starting at 1)
> +    // make sure this prefix is not declared in the current scope.

Please capitalize words and put . at the end of each sentence.

> Source/WebCore/editing/MarkupAccumulator.cpp:482
> +    } while (namespaces.get(builder.toAtomicString().impl()));
> +    prefixedName.setPrefix(builder.toString());

It's kind of inefficient to call toAtomicString() and then toString(), isn't it?
Can't we do something like:
    AtomicString name = builder.toAtomicString();
    if (namespaces.get(name.impl()) {
        prefixedName.setPrefix(name);
        return;
    }
?

> Source/WebCore/editing/MarkupAccumulator.cpp:499
> +                // In case no prefix is given but a namespace is there, try to find the matching in-scope declaration by looking up the prefix in the namespace -> prefix mapping.
> +                // If still no prefix is found, we would lose the namespace associated with the attribute, so generate a unique prefix and namespace declaration for the prefix.

It seems redundant to repeat what the spec. says. If anything, we should add this comment to the change log instead
because it reflects the current state of the spec.

> Source/WebCore/editing/MarkupAccumulator.cpp:506
> +                // If the prefix is already mapped to a different namespace, generate a unique prefix. Otherwise two namespace declarations for the prefix
> +                // would have to be generated, violating Namespace constraint: Attributes Unique from the XMLNames specification.

Instead of adding a comment like, I would prefer defining a boolean with a descriptive variable name. e.g.
bool prefixIsAlreadyUsedForSomeotherNamespace = ns == attribute.namespaceURI().impl().

> LayoutTests/fast/dom/XMLSerializer-same-prefix-different-namespaces-conflict.html:1
> +<html>

Is it intentional that DOCTYPE is omitted?

> LayoutTests/fast/dom/XMLSerializer-same-prefix-different-namespaces-conflict.html:12
> +        var doc = document.implementation.createDocument("", "", null);
> +        var parent = doc.firstChild;
> +        var el = doc.createElementNS("ns1", "x:y");
> +        el.setAttributeNS("ns2", "x:z", "val");

Please spell out element.

[Rob Buis]

Created attachment 209919 [details]
Patch

[Rob Buis]

Thanks for the review!

(In reply to comment #25)
> (From update of attachment 207771 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=207771&action=review
> 
> > Source/WebCore/editing/MarkupAccumulator.cpp:123
> > +    m_prefixLevel = 0;
> 

I tried to fix all issues. Note that I redid the logic in appendAttribute to handle attributes that are in namespaces to be exactly like stated in:
http://www.w3.org/TR/DOM-Level-3-Core/namespaces-algorithms.html#normalizeDocumentAlgo
This hopefully makes it more easy to check that we are doing the right thing.

[WebKit Commit Bot]

Attachment 209919 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/dom/XMLSerializer-attribute-namespace-prefix-conflicts-expected.txt', u'LayoutTests/fast/dom/XMLSerializer-attribute-namespace-prefix-conflicts.html', u'LayoutTests/fast/dom/XMLSerializer-same-prefix-different-namespaces-conflict-expected.txt', u'LayoutTests/fast/dom/XMLSerializer-same-prefix-different-namespaces-conflict.html', u'LayoutTests/fast/dom/XMLSerializer-setAttributeNS-namespace-no-prefix-expected.txt', u'LayoutTests/fast/dom/XMLSerializer-setAttributeNS-namespace-no-prefix.html', u'LayoutTests/svg/custom/xlink-prefix-generation-in-attributes-expected.txt', u'LayoutTests/svg/custom/xlink-prefix-generation-in-attributes.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/editing/MarkupAccumulator.cpp', u'Source/WebCore/editing/MarkupAccumulator.h']" exit_code: 1
Source/WebCore/editing/MarkupAccumulator.cpp:512:  Line contains only semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryosuke Niwa]

Comment on attachment 209919 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=209919&action=review

> Source/WebCore/editing/MarkupAccumulator.cpp:487
> +        AtomicString name = builder.toAtomicString();

Why not use a const reference?

>> Source/WebCore/editing/MarkupAccumulator.cpp:512
>> +                    ; // declare prefix using appendNamespace
> 
> Line contains only semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]

What happened here?

[Ryosuke Niwa]

Comment on attachment 209919 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=209919&action=review

> Source/WebCore/editing/MarkupAccumulator.cpp:511
> +                if (AtomicStringImpl* prefix = namespaces ? namespaces->get(attribute.namespaceURI().impl()) : 0) {
> +                    prefixedName.setPrefix(AtomicString(prefix));
> +                } else if (!attribute.prefix().isEmpty() && !foundNS) {

There should be no curly brackets around a single-line statement.

[Ryosuke Niwa]

Comment on attachment 209919 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=209919&action=review

>>> Source/WebCore/editing/MarkupAccumulator.cpp:512
>>> +                    ; // declare prefix using appendNamespace
>> 
>> Line contains only semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
> 
> What happened here?

It seems like this case is hit when prefixIsAlreadyMappedToOtherNS is true, the prefix is present, and hasn't appeared elsewhere yet?
Perhaps we can tweak the condition (and the name) of prefixIsAlreadyMappedToOtherNS so that we don't have to check it here again.

[Rob Buis]

(In reply to comment #29)
> (From update of attachment 209919 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=209919&action=review
> 
> > Source/WebCore/editing/MarkupAccumulator.cpp:487
> > +        AtomicString name = builder.toAtomicString();
> 
> Why not use a const reference?

Sure, will fix.

> >> Source/WebCore/editing/MarkupAccumulator.cpp:512
> >> +                    ; // declare prefix using appendNamespace
> > 
> > Line contains only semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
> 
> What happened here?

I had fixed it locally but it was not part of the commit :( Will fix.

[Rob Buis]

Created attachment 209921 [details]
Patch

[Rob Buis]

(In reply to comment #31)
> (From update of attachment 209919 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=209919&action=review
> 
> >>> Source/WebCore/editing/MarkupAccumulator.cpp:512
> >>> +                    ; // declare prefix using appendNamespace
> >> 
> >> Line contains only semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
> > 
> > What happened here?
> 
> It seems like this case is hit when prefixIsAlreadyMappedToOtherNS is true, the prefix is present, and hasn't appeared elsewhere yet?
> Perhaps we can tweak the condition (and the name) of prefixIsAlreadyMappedToOtherNS so that we don't have to check it here again.

I don't understand this fully? I did notice in my previous patch prefixIsAlreadyMappedToOtherNS did not match what it does, i.e. it didn't check that a namespace was found in the first place (foundNS).
With this new patch it should follow the pseudo code 1:1. Let me know if we can easily eliminate checks, I am not seeing it so far.

[Ryosuke Niwa]

Comment on attachment 209921 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=209921&action=review

> Source/WebCore/editing/MarkupAccumulator.cpp:508
> +            if (!attribute.prefix() || !foundNS || prefixIsAlreadyMappedToOtherNS) {

What is this "!attribute.prefix()" checking here? prefix() is AtomicString&, right?

> Source/WebCore/editing/MarkupAccumulator.cpp:511
> +                else if (!attribute.prefix().isEmpty() && !foundNS) {

Can we define a new boolean like shouldDeclaredUsingAppendNamespace and then check it in the following else if instead of having this empty block?

[Rob Buis]

Created attachment 209923 [details]
Patch

[Rob Buis]

(In reply to comment #35)
> (From update of attachment 209921 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=209921&action=review
> 
> > Source/WebCore/editing/MarkupAccumulator.cpp:508
> > +            if (!attribute.prefix() || !foundNS || prefixIsAlreadyMappedToOtherNS) {
> 
> What is this "!attribute.prefix()" checking here? prefix() is AtomicString&, right?

Yeah that is possibly dangerous, using isEmpty() now.

> > Source/WebCore/editing/MarkupAccumulator.cpp:511
> > +                else if (!attribute.prefix().isEmpty() && !foundNS) {
> 
> Can we define a new boolean like shouldDeclaredUsingAppendNamespace and then check it in the following else if instead of having this empty block?

I think I see what you mean, fixed hopefully.

[WebKit Commit Bot]

Comment on attachment 209923 [details]
Patch

Clearing flags on attachment: 209923

Committed r154779: <http://trac.webkit.org/changeset/154779>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Lucas Forschler]

Mass moving XML DOM bugs to the "DOM" Component.