Bug 19121 - Namespace prefix is blindly followed when serializing
Summary: Namespace prefix is blindly followed when serializing
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P2 Major
Assignee: Rob Buis
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-05-18 22:12 PDT by Alexey Proskuryakov
Modified: 2019-02-06 09:03 PST (History)
5 users (show)

See Also:


Attachments
test case (277 bytes, text/html)
2008-05-18 22:13 PDT, Alexey Proskuryakov
no flags Details
Patch (8.05 KB, patch)
2013-06-27 14:03 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (8.78 KB, patch)
2013-06-28 15:03 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (14.00 KB, patch)
2013-07-01 18:48 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (14.57 KB, patch)
2013-07-16 18:10 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (18.06 KB, patch)
2013-07-18 14:33 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 (517.19 KB, application/zip)
2013-07-18 17:21 PDT, Build Bot
no flags Details
Patch (18.09 KB, patch)
2013-07-18 17:32 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (18.50 KB, patch)
2013-07-18 19:01 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2 (494.66 KB, application/zip)
2013-07-18 19:47 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion (775.43 KB, application/zip)
2013-07-18 23:11 PDT, Build Bot
no flags Details
Patch (19.11 KB, patch)
2013-07-19 18:42 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from APPLE-EWS-9 for win-future (261.47 KB, application/zip)
2013-07-26 18:48 PDT, Build Bot
no flags Details
Patch (19.15 KB, patch)
2013-07-30 15:08 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (19.08 KB, patch)
2013-08-28 13:23 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (19.09 KB, patch)
2013-08-28 13:59 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (19.16 KB, patch)
2013-08-28 14:31 PDT, Rob Buis
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Proskuryakov 2008-05-18 22:12:31 PDT
XMLSerializer uses whichever namespace prefix was specified when creating a node, even if it conflicts  with namespaces of other nodes, or is missing altogether.
Comment 1 Alexey Proskuryakov 2008-05-18 22:13:02 PDT
Created attachment 21224 [details]
test case
Comment 2 Rob Buis 2013-06-27 14:03:13 PDT
Created attachment 205633 [details]
Patch
Comment 3 Rob Buis 2013-06-27 14:04:48 PDT
Comment on attachment 205633 [details]
Patch

Clearing review flag since this should go on top of bug 16496.
Comment 4 Rob Buis 2013-06-28 15:03:32 PDT
Created attachment 205745 [details]
Patch
Comment 5 Rob Buis 2013-06-28 15:06:22 PDT
(In reply to comment #4)
> Created an attachment (id=205745) [details]
> Patch

Rebase against new bug 16496 patch, and add the missing testcase.
Comment 6 Rob Buis 2013-07-01 18:48:55 PDT
Created attachment 205850 [details]
Patch
Comment 7 Rob Buis 2013-07-16 18:10:08 PDT
Created attachment 206831 [details]
Patch
Comment 8 Rob Buis 2013-07-18 14:33:36 PDT
Created attachment 207022 [details]
Patch
Comment 9 Build Bot 2013-07-18 17:21:24 PDT
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
Comment 10 Build Bot 2013-07-18 17:21:25 PDT
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
Comment 11 Rob Buis 2013-07-18 17:32:01 PDT
Created attachment 207041 [details]
Patch
Comment 12 Ryosuke Niwa 2013-07-18 17:58:01 PDT
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?
Comment 13 Rob Buis 2013-07-18 18:29:09 PDT
(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.
Comment 14 Rob Buis 2013-07-18 19:01:22 PDT
Created attachment 207047 [details]
Patch
Comment 15 Build Bot 2013-07-18 19:47:47 PDT
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
Comment 16 Build Bot 2013-07-18 19:47:49 PDT
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
Comment 17 Build Bot 2013-07-18 23:11:43 PDT
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
Comment 18 Build Bot 2013-07-18 23:11:48 PDT
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
Comment 19 Rob Buis 2013-07-19 18:42:17 PDT
Created attachment 207164 [details]
Patch
Comment 20 Build Bot 2013-07-26 18:48:07 PDT
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
Comment 21 Build Bot 2013-07-26 18:48:09 PDT
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
Comment 22 Ryosuke Niwa 2013-07-30 12:50:53 PDT
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?
Comment 23 Rob Buis 2013-07-30 13:32:18 PDT
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....
Comment 24 Rob Buis 2013-07-30 15:08:10 PDT
Created attachment 207771 [details]
Patch
Comment 25 Ryosuke Niwa 2013-08-27 22:07:57 PDT
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.
Comment 26 Rob Buis 2013-08-28 13:23:17 PDT
Created attachment 209919 [details]
Patch
Comment 27 Rob Buis 2013-08-28 13:25:51 PDT
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.
Comment 28 WebKit Commit Bot 2013-08-28 13:25:58 PDT
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.
Comment 29 Ryosuke Niwa 2013-08-28 13:33:48 PDT
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?
Comment 30 Ryosuke Niwa 2013-08-28 13:35:53 PDT
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.
Comment 31 Ryosuke Niwa 2013-08-28 13:39:48 PDT
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.
Comment 32 Rob Buis 2013-08-28 13:44:15 PDT
(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.
Comment 33 Rob Buis 2013-08-28 13:59:32 PDT
Created attachment 209921 [details]
Patch
Comment 34 Rob Buis 2013-08-28 14:03:43 PDT
(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.
Comment 35 Ryosuke Niwa 2013-08-28 14:15:37 PDT
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?
Comment 36 Rob Buis 2013-08-28 14:31:32 PDT
Created attachment 209923 [details]
Patch
Comment 37 Rob Buis 2013-08-28 14:32:35 PDT
(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.
Comment 38 WebKit Commit Bot 2013-08-28 15:24:17 PDT
Comment on attachment 209923 [details]
Patch

Clearing flags on attachment: 209923

Committed r154779: <http://trac.webkit.org/changeset/154779>
Comment 39 WebKit Commit Bot 2013-08-28 15:24:21 PDT
All reviewed patches have been landed.  Closing bug.
Comment 40 Lucas Forschler 2019-02-06 09:03:50 PST
Mass moving XML DOM bugs to the "DOM" Component.