Bug 120490 - XMLSerializer-attribute-namespace-prefix-conflicts can't produce reliable results
Summary: XMLSerializer-attribute-namespace-prefix-conflicts can't produce reliable res...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: XML (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Rob Buis
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-08-29 14:07 PDT by Dean Jackson
Modified: 2013-08-31 14:05 PDT (History)
5 users (show)

See Also:


Attachments
Patch (3.03 KB, patch)
2013-08-30 19:27 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (2.76 KB, patch)
2013-08-31 09:16 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 Dean Jackson 2013-08-29 14:07:39 PDT
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.
Comment 1 Dean Jackson 2013-08-29 14:11:40 PDT
TestExpectations updated in https://trac.webkit.org/r154842
Comment 2 Rob Buis 2013-08-29 14:38:29 PDT
This seems to be a Debug only problem. Will have a look.
Comment 3 Rob Buis 2013-08-29 19:27:42 PDT
(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.
Comment 4 Dean Jackson 2013-08-30 16:28:44 PDT
I'm skipping the tests for now. Please unskip in the patch. Thanks!
Comment 5 Dean Jackson 2013-08-30 16:34:04 PDT
https://trac.webkit.org/r154918
Comment 6 Rob Buis 2013-08-30 19:27:05 PDT
Created attachment 210178 [details]
Patch
Comment 7 Rob Buis 2013-08-30 19:31:55 PDT
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...
Comment 8 Dean Jackson 2013-08-30 19:35:42 PDT
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.
Comment 9 Ryosuke Niwa 2013-08-30 20:30:48 PDT
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!
Comment 10 Ryosuke Niwa 2013-08-30 20:32:57 PDT
(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?
Comment 11 Darin Adler 2013-08-31 08:16:11 PDT
(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.
Comment 12 Darin Adler 2013-08-31 08:17:07 PDT
Comment on attachment 210178 [details]
Patch

This patch is definitely wrong. It covers up a bug elsewhere.
Comment 13 Rob Buis 2013-08-31 09:16:47 PDT
Created attachment 210194 [details]
Patch
Comment 14 WebKit Commit Bot 2013-08-31 14:04:57 PDT
Comment on attachment 210194 [details]
Patch

Clearing flags on attachment: 210194

Committed r154932: <http://trac.webkit.org/changeset/154932>
Comment 15 WebKit Commit Bot 2013-08-31 14:05:00 PDT
All reviewed patches have been landed.  Closing bug.