Bug 18976 - REGRESSION (r26474): WebKit fails jQuery test 64 core module: text(String) subtest 1 Check escaped text (createTextNode)
Summary: REGRESSION (r26474): WebKit fails jQuery test 64 core module: text(String) su...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 525.x (Safari 3.1)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Nobody
URL: http://jquery.com/test/
Keywords: InRadar, NeedsReduction, Regression
: 18101 (view as bug list)
Depends on:
Blocks:
 
Reported: 2008-05-09 14:15 PDT by David Kilzer (:ddkilzer)
Modified: 2008-05-14 22:24 PDT (History)
4 users (show)

See Also:


Attachments
Test case (635 bytes, text/html)
2008-05-09 18:40 PDT, David Kilzer (:ddkilzer)
no flags Details
Patch v1 (11.12 KB, patch)
2008-05-14 09:45 PDT, David Kilzer (:ddkilzer)
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Kilzer (:ddkilzer) 2008-05-09 14:15:48 PDT
* SUMMARY
When running the jQuery test suite, WebKit (in the form of Safari 3.1.1 on Leopard 10.5.x), fails one of the tests:

64. core module: text(String) (1, 3, 4)
    1. Check escaped text

This is either a real failure or an evangelism issue.

* STEPS TO REPRODUCE
1. Launch Safari/WebKit.
2. Open URL:  http://jquery.com/test/
3. Wait for test to complete.

* RESULTS
WebKit fails 1 of 1157 tests (see SUMMARY for details).

* REGRESSION
Unknown.  Only tested with Safari 3.1.1.
Comment 1 David Kilzer (:ddkilzer) 2008-05-09 14:20:59 PDT
<rdar://problem/5924793>
Comment 2 David Kilzer (:ddkilzer) 2008-05-09 18:40:37 PDT
The issue is how document.createTextNode() encodes text.
Comment 3 David Kilzer (:ddkilzer) 2008-05-09 18:40:58 PDT
Created attachment 21052 [details]
Test case
Comment 4 David Kilzer (:ddkilzer) 2008-05-10 06:28:45 PDT
Comment on attachment 21052 [details]
Test case

This test case is bad.

When the Expected and Actual lines match, the test still fails.
Comment 5 David Kilzer (:ddkilzer) 2008-05-10 06:31:58 PDT
(In reply to comment #0)
> * REGRESSION
> Unknown.  Only tested with Safari 3.1.1.

This is a regression as it works with Safari 3.0.4 (523.12.2) on Mac OS X 10.4.11 (8S165).

I also have a fix.

Comment 6 David Kilzer (:ddkilzer) 2008-05-10 06:52:02 PDT
The bisect-builds script with Safari 2.0.4 reports:

Works: r26359  Fails: r26570

Comment 7 David Kilzer (:ddkilzer) 2008-05-10 06:54:57 PDT
Regression occurred in r26474 (noted in comments):

http://trac.webkit.org/projects/webkit/changeset/26474

Comment 8 David Kilzer (:ddkilzer) 2008-05-14 09:32:31 PDT
Qt developers:  The following test results will likely need to be updated after this bug is fixed:

LayoutTests/platform/qt/fast/dom/dom-parse-serialize-expected.txt
LayoutTests/platform/qt/fast/xsl/xslt-text-expected.txt

Note that LayoutTests/fast/xsl/xslt-text-expected.txt is a text dump, while LayoutTests/platform/qt/fast/xsl/xslt-text-expected.txt is a render tree dump.  You may be able to use the common test results and remove the Qt-specific results.

Comment 9 David Kilzer (:ddkilzer) 2008-05-14 09:45:14 PDT
Created attachment 21127 [details]
Patch v1

Proposed fix.
Comment 10 Darin Adler 2008-05-14 09:52:33 PDT
Comment on attachment 21127 [details]
Patch v1

Putting these strings into separate functions might make the code slightly slower unnecessarily. There's no particular reason these need to be shared globals, and I had originally used a C-style string because I thought that would be fast enough; widening the characters from 8-bit to 16-bit should be fast enough. And using globals at all (not new to this patch) creates unnecessary threading issues if we ever start tackling them the way we are in JavaScriptCore.

But aside from that entirely-theoretical worry, looks perfect.

r=me
Comment 11 Tor Arne Vestbø 2008-05-14 09:55:13 PDT
Thanks for the heads up David.
Comment 12 David Kilzer (:ddkilzer) 2008-05-14 13:38:36 PDT
Committed revision 33451.  Removed unnecessary optimization per Comment #10.

Tor/Simon:  Please update Qt test results per Comment #8.  Thanks!

Comment 13 Alexey Proskuryakov 2008-05-14 22:24:01 PDT
*** Bug 18101 has been marked as a duplicate of this bug. ***