XMLSerializer-attribute-namespace-prefix-conflicts can't produce reliable results

[Dean Jackson]

The test fast/dom/XMLSerializer-attribute-namespace-prefix-conflicts.html creates some duplicated namespaces and serializes the output. Since the unique namespace prefixes are based on an incrementing number, the actual prefixes are not reliably identical across runs.

The expected result can't simply be the text dump of a single run.

I'm going to mark the test as [ PASS FAIL ] for now.

[Dean Jackson]

TestExpectations updated in https://trac.webkit.org/r154842

[Rob Buis]

This seems to be a Debug only problem. Will have a look.

[Rob Buis]

(In reply to comment #2)
> This seems to be a Debug only problem. Will have a look.

I found a possible fix, will likely provide a patch tomorrow.

[Dean Jackson]

I'm skipping the tests for now. Please unskip in the patch. Thanks!

[Dean Jackson]

https://trac.webkit.org/r154918

[Rob Buis]

Created attachment 210178 [details]
Patch

[Rob Buis]

I debugged this a bit. It seems fastMalloc can return identical pointers for strings with differing hash values in StringImpl::createUninitializedInternalNonEmpty(). Specifically what happens on the test is that NS1 is not found so it is written out. Then for the next element, NS2 is tried but it already exists in a ancestor element. Then NS3, NS4 etc. all seem to reuse the same internal StringImpl pointer (for already existing NS2)  even though the strings are obviously different,. I wonder if I am using the API wrong or if this is a bug...

[Dean Jackson]

Comment on attachment 210178 [details]
Patch

This looks fine to me. However, we should probably check if there is a bug in AtomicString. cc-ing Ben.

[Ryosuke Niwa]

Comment on attachment 210178 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Avoid toAtomicString which sometimes returns the same internal AtomicStringImpl* for two different strings.

That definitely sounds like a serious bug!

[Ryosuke Niwa]

(In reply to comment #7)
> I debugged this a bit. It seems fastMalloc can return identical pointers for strings with differing hash values in StringImpl::createUninitializedInternalNonEmpty(). Specifically what happens on the test is that NS1 is not found so it is written out. Then for the next element, NS2 is tried but it already exists in a ancestor element. Then NS3, NS4 etc. all seem to reuse the same internal StringImpl pointer (for already existing NS2)  even though the strings are obviously different,. I wonder if I am using the API wrong or if this is a bug...

Maybe you're not retaining each AtomicString?

[Darin Adler]

(In reply to comment #10)
> Maybe you're not retaining each AtomicString?

Sounds likely. We can definitely get the same AtomicStringImpl twice if the first one is deallocated before the second one is allocated.

[Darin Adler]

Comment on attachment 210178 [details]
Patch

This patch is definitely wrong. It covers up a bug elsewhere.

[Rob Buis]

Created attachment 210194 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 210194 [details]
Patch

Clearing flags on attachment: 210194

Committed r154932: <http://trac.webkit.org/changeset/154932>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.