REGRESSION(r168256): JSString can get 8-bit flag wrong when re-using AtomicStrings.
authorakling@apple.com <akling@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 18 Aug 2014 21:33:01 +0000 (21:33 +0000)
committerakling@apple.com <akling@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 18 Aug 2014 21:33:01 +0000 (21:33 +0000)
<https://webkit.org/b/133574>
<rdar://problem/18051847>

Source/JavaScriptCore:

The optimization that resolves JSRopeStrings into an existing
AtomicString (to save time and memory by avoiding StringImpl allocation)
had a bug that it wasn't copying the 8-bit flag from the AtomicString.

This could lead to a situation where a 16-bit StringImpl containing
only 8-bit characters is sitting in the AtomicString table, is found
by the rope resolution optimization, and gives you a rope that thinks
it's all 8-bit, but has a fiber with 16-bit characters.

Resolving that rope will then yield incorrect results.

This was all caught by an assertion, but very hard to reproduce.

Test: js/dopey-rope-with-16-bit-propertyname.html

Reviewed by Darin Adler.

* runtime/JSString.cpp:
(JSC::JSRopeString::resolveRopeToAtomicString):
(JSC::JSRopeString::resolveRopeToExistingAtomicString):
* runtime/JSString.h:
(JSC::JSString::setIs8Bit):
(JSC::JSString::toExistingAtomicString):

LayoutTests:

Add a tests that creates a 16-bit AtomicString with only 8-bit characters,
then tiers up into baseline JIT and uses that string as part of a rope-within-a-rope
and serializes that rope to get an incorrect concatenation.

Reviewed by Darin Adler.

* js/dopey-rope-with-16-bit-propertyname-expected.txt: Added.
* js/dopey-rope-with-16-bit-propertyname.html: Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@172727 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/js/dopey-rope-with-16-bit-propertyname-expected.txt [new file with mode: 0644]
LayoutTests/js/dopey-rope-with-16-bit-propertyname.html [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/runtime/JSString.cpp
Source/JavaScriptCore/runtime/JSString.h

index e1b1eff..602db67 100644 (file)
@@ -1,3 +1,18 @@
+2014-08-18  Andreas Kling  <akling@apple.com>
+
+        REGRESSION(r168256): JSString can get 8-bit flag wrong when re-using AtomicStrings.
+        <https://webkit.org/b/133574>
+        <rdar://problem/18051847>
+
+        Add a tests that creates a 16-bit AtomicString with only 8-bit characters,
+        then tiers up into baseline JIT and uses that string as part of a rope-within-a-rope
+        and serializes that rope to get an incorrect concatenation.
+
+        Reviewed by Darin Adler.
+
+        * js/dopey-rope-with-16-bit-propertyname-expected.txt: Added.
+        * js/dopey-rope-with-16-bit-propertyname.html: Added.
+
 2014-08-18  Vivek Galatage  <vivek.vg@samsung.com>
 
         Implement CanvasRenderingContext2D direction attribute
diff --git a/LayoutTests/js/dopey-rope-with-16-bit-propertyname-expected.txt b/LayoutTests/js/dopey-rope-with-16-bit-propertyname-expected.txt
new file mode 100644 (file)
index 0000000..6f25f19
--- /dev/null
@@ -0,0 +1,10 @@
+Test that a 16-bit AtomicString containing only 8-bit characters doesn't confuse the JIT into thinking it's an 8-bit AtomicString.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS globalRope is 'foo.zest'
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/js/dopey-rope-with-16-bit-propertyname.html b/LayoutTests/js/dopey-rope-with-16-bit-propertyname.html
new file mode 100644 (file)
index 0000000..d94b4b5
--- /dev/null
@@ -0,0 +1,36 @@
+<!DOCTYPE html>
+<html>
+<head>
+<meta charset="utf-8">
+<script src="../resources/js-test-pre.js"></script>
+</head>
+<body>
+<script>
+
+description("Test that a 16-bit AtomicString containing only 8-bit characters doesn't confuse the JIT into thinking it's an 8-bit AtomicString.");
+
+o = {};
+
+stringWithEmoji = "zest😐";
+var test16bit = stringWithEmoji.substring(0, 4);
+
+o[test16bit] = "this makes it an AtomicString";
+
+globalRope = "";
+
+function jittable(a, b) {
+    for (var i = 0; i < 5000; ++i) {
+        poisonedRope = a + b;
+        o[poisonedRope] = 1;
+        globalRope = "foo." + poisonedRope;
+    }
+}
+
+jittable("ze", "st");
+
+shouldBe("globalRope", "'foo.zest'");
+
+</script>
+<script src="../resources/js-test-post.js"></script>
+</body>
+</html>
index 1141932..d03c76d 100644 (file)
@@ -1,3 +1,33 @@
+2014-08-18  Andreas Kling  <akling@apple.com>
+
+        REGRESSION(r168256): JSString can get 8-bit flag wrong when re-using AtomicStrings.
+        <https://webkit.org/b/133574>
+        <rdar://problem/18051847>
+
+        The optimization that resolves JSRopeStrings into an existing
+        AtomicString (to save time and memory by avoiding StringImpl allocation)
+        had a bug that it wasn't copying the 8-bit flag from the AtomicString.
+
+        This could lead to a situation where a 16-bit StringImpl containing
+        only 8-bit characters is sitting in the AtomicString table, is found
+        by the rope resolution optimization, and gives you a rope that thinks
+        it's all 8-bit, but has a fiber with 16-bit characters.
+
+        Resolving that rope will then yield incorrect results.
+
+        This was all caught by an assertion, but very hard to reproduce.
+
+        Test: js/dopey-rope-with-16-bit-propertyname.html
+
+        Reviewed by Darin Adler.
+
+        * runtime/JSString.cpp:
+        (JSC::JSRopeString::resolveRopeToAtomicString):
+        (JSC::JSRopeString::resolveRopeToExistingAtomicString):
+        * runtime/JSString.h:
+        (JSC::JSString::setIs8Bit):
+        (JSC::JSString::toExistingAtomicString):
+
 2014-08-18  Saam Barati  <sbarati@apple.com>
 
         The parser should generate AST nodes the var declarations with no initializers
index ff3b628..3ea2b7e 100644 (file)
@@ -161,6 +161,7 @@ void JSRopeString::resolveRopeToAtomicString(ExecState* exec) const
     if (m_length > maxLengthForOnStackResolve) {
         resolveRope(exec);
         m_value = AtomicString(m_value);
+        setIs8Bit(m_value.impl()->is8Bit());
         return;
     }
 
@@ -168,10 +169,12 @@ void JSRopeString::resolveRopeToAtomicString(ExecState* exec) const
         LChar buffer[maxLengthForOnStackResolve];
         resolveRopeInternal8(buffer);
         m_value = AtomicString(buffer, m_length);
+        setIs8Bit(m_value.impl()->is8Bit());
     } else {
         UChar buffer[maxLengthForOnStackResolve];
         resolveRopeInternal16(buffer);
         m_value = AtomicString(buffer, m_length);
+        setIs8Bit(m_value.impl()->is8Bit());
     }
 
     clearFibers();
@@ -193,6 +196,7 @@ AtomicStringImpl* JSRopeString::resolveRopeToExistingAtomicString(ExecState* exe
         resolveRope(exec);
         if (AtomicStringImpl* existingAtomicString = AtomicString::find(m_value.impl())) {
             m_value = *existingAtomicString;
+            setIs8Bit(m_value.impl()->is8Bit());
             clearFibers();
             return existingAtomicString;
         }
@@ -204,6 +208,7 @@ AtomicStringImpl* JSRopeString::resolveRopeToExistingAtomicString(ExecState* exe
         resolveRopeInternal8(buffer);
         if (AtomicStringImpl* existingAtomicString = AtomicString::find(buffer, m_length)) {
             m_value = *existingAtomicString;
+            setIs8Bit(m_value.impl()->is8Bit());
             clearFibers();
             return existingAtomicString;
         }
@@ -212,6 +217,7 @@ AtomicStringImpl* JSRopeString::resolveRopeToExistingAtomicString(ExecState* exe
         resolveRopeInternal16(buffer);
         if (AtomicStringImpl* existingAtomicString = AtomicString::find(buffer, m_length)) {
             m_value = *existingAtomicString;
+            setIs8Bit(m_value.impl()->is8Bit());
             clearFibers();
             return existingAtomicString;
         }
index e77b220..2213f82 100644 (file)
@@ -187,7 +187,7 @@ namespace JSC {
             
         bool isRope() const { return m_value.isNull(); }
         bool is8Bit() const { return m_flags & Is8Bit; }
-        void setIs8Bit(bool flag)
+        void setIs8Bit(bool flag) const
         {
             if (flag)
                 m_flags |= Is8Bit;
@@ -201,7 +201,7 @@ namespace JSC {
         bool tryHashConsLock();
         void releaseHashConsLock();
 
-        unsigned m_flags;
+        mutable unsigned m_flags;
             
         // A string is represented either by a String or a rope of fibers.
         unsigned m_length;
@@ -477,6 +477,7 @@ namespace JSC {
             return static_cast<AtomicStringImpl*>(m_value.impl());
         if (AtomicStringImpl* existingAtomicString = AtomicString::find(m_value.impl())) {
             m_value = *existingAtomicString;
+            setIs8Bit(m_value.impl()->is8Bit());
             return existingAtomicString;
         }
         return nullptr;