Bug 18976

Summary: REGRESSION (r26474): WebKit fails jQuery test 64 core module: text(String) subtest 1 Check escaped text (createTextNode)
Product: WebKit Reporter: David Kilzer (:ddkilzer) <ddkilzer>
Component: DOMAssignee: Nobody <webkit-unassigned>
Severity: Normal CC: bill, hausmann, justin.garcia, vestbo
Priority: P2 Keywords: InRadar, NeedsReduction, Regression
Version: 525.x (Safari 3.1)   
Hardware: Mac   
OS: OS X 10.5   
URL: http://jquery.com/test/
Description Flags
Test case
Patch v1 darin: review+

Description David Kilzer (:ddkilzer) 2008-05-09 14:15:48 PDT
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.

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

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

Unknown.  Only tested with Safari 3.1.1.
Comment 1 David Kilzer (:ddkilzer) 2008-05-09 14:20:59 PDT
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)
> 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):


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:


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.

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. ***