JSRopeString::resolveRope() wrongly assumes that tryGetValue() passes it a valid...
authormark.lam@apple.com <mark.lam@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 21 Sep 2018 23:18:15 +0000 (23:18 +0000)
committermark.lam@apple.com <mark.lam@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 21 Sep 2018 23:18:15 +0000 (23:18 +0000)
https://bugs.webkit.org/show_bug.cgi?id=189855
<rdar://problem/44680181>

Reviewed by Filip Pizlo.

tryGetValue() always passes a nullptr to JSRopeString::resolveRope() for the
ExecState* argument.  This is intentional so that resolveRope() does not throw
in the event of an OutOfMemory error.  Hence, JSRopeString::resolveRope() should
get the VM from the cell instead of via the ExecState.

Also removed an obsolete and unused field in JSString.

* runtime/JSString.cpp:
(JSC::JSRopeString::resolveRope const):
(JSC::JSRopeString::outOfMemory const):
* runtime/JSString.h:
(JSC::JSString::tryGetValue const):

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

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/runtime/JSString.cpp
Source/JavaScriptCore/runtime/JSString.h

index ae1581a..b70c429 100644 (file)
@@ -1,3 +1,24 @@
+2018-09-21  Mark Lam  <mark.lam@apple.com>
+
+        JSRopeString::resolveRope() wrongly assumes that tryGetValue() passes it a valid ExecState.
+        https://bugs.webkit.org/show_bug.cgi?id=189855
+        <rdar://problem/44680181>
+
+        Reviewed by Filip Pizlo.
+
+        tryGetValue() always passes a nullptr to JSRopeString::resolveRope() for the
+        ExecState* argument.  This is intentional so that resolveRope() does not throw
+        in the event of an OutOfMemory error.  Hence, JSRopeString::resolveRope() should
+        get the VM from the cell instead of via the ExecState.
+
+        Also removed an obsolete and unused field in JSString.
+
+        * runtime/JSString.cpp:
+        (JSC::JSRopeString::resolveRope const):
+        (JSC::JSRopeString::outOfMemory const):
+        * runtime/JSString.h:
+        (JSC::JSString::tryGetValue const):
+
 2018-09-21  Michael Saboff  <msaboff@apple.com>
 
         Add functions to measure memory footprint to JSC
index d31ca6b..0303cbd 100644 (file)
@@ -250,7 +250,7 @@ RefPtr<AtomicStringImpl> JSRopeString::resolveRopeToExistingAtomicString(ExecSta
     return nullptr;
 }
 
-void JSRopeString::resolveRope(ExecState* exec) const
+void JSRopeString::resolveRope(ExecState* nullOrExecForOOM) const
 {
     ASSERT(isRope());
     
@@ -264,10 +264,10 @@ void JSRopeString::resolveRope(ExecState* exec) const
     if (is8Bit()) {
         LChar* buffer;
         if (auto newImpl = StringImpl::tryCreateUninitialized(length(), buffer)) {
-            exec->vm().heap.reportExtraMemoryAllocated(newImpl->cost());
+            Heap::heap(this)->reportExtraMemoryAllocated(newImpl->cost());
             m_value = WTFMove(newImpl);
         } else {
-            outOfMemory(exec);
+            outOfMemory(nullOrExecForOOM);
             return;
         }
         resolveRopeInternal8NoSubstring(buffer);
@@ -278,10 +278,10 @@ void JSRopeString::resolveRope(ExecState* exec) const
     
     UChar* buffer;
     if (auto newImpl = StringImpl::tryCreateUninitialized(length(), buffer)) {
-        exec->vm().heap.reportExtraMemoryAllocated(newImpl->cost());
+        Heap::heap(this)->reportExtraMemoryAllocated(newImpl->cost());
         m_value = WTFMove(newImpl);
     } else {
-        outOfMemory(exec);
+        outOfMemory(nullOrExecForOOM);
         return;
     }
     
@@ -380,16 +380,16 @@ void JSRopeString::resolveRopeSlowCase(UChar* buffer) const
     ASSERT(buffer == position);
 }
 
-void JSRopeString::outOfMemory(ExecState* exec) const
+void JSRopeString::outOfMemory(ExecState* nullOrExecForOOM) const
 {
-    VM& vm = exec->vm();
-    auto scope = DECLARE_THROW_SCOPE(vm);
-
     clearFibers();
     ASSERT(isRope());
     ASSERT(m_value.isNull());
-    if (exec)
-        throwOutOfMemoryError(exec, scope);
+    if (nullOrExecForOOM) {
+        VM& vm = nullOrExecForOOM->vm();
+        auto scope = DECLARE_THROW_SCOPE(vm);
+        throwOutOfMemoryError(nullOrExecForOOM, scope);
+    }
 }
 
 JSValue JSString::toPrimitive(ExecState*, PreferredPrimitiveType) const
index 290c8c7..efb3bb9 100644 (file)
@@ -159,7 +159,7 @@ public:
 
     inline bool equal(ExecState*, JSString* other) const;
     const String& value(ExecState*) const;
-    const String& tryGetValue() const;
+    inline const String& tryGetValue() const;
     const StringImpl* tryGetValueImpl() const;
     ALWAYS_INLINE unsigned length() const { return m_length; }
 
@@ -218,7 +218,6 @@ private:
     mutable uint16_t m_flags { 0 };
     // The poison is strategically placed and holds a value such that the first
     // 64 bits of JSString look like a double JSValue.
-    uint16_t m_poison { 1 };
     mutable String m_value;
 
     friend class LLIntOffsetsExtractor;
@@ -418,12 +417,14 @@ private:
     friend JSValue jsStringFromRegisterArray(ExecState*, Register*, unsigned);
     friend JSValue jsStringFromArguments(ExecState*, JSValue);
 
-    JS_EXPORT_PRIVATE void resolveRope(ExecState*) const;
+    // If nullOrExecForOOM is null, resolveRope() will be do nothing in the event of an OOM error.
+    // The rope value will remain a null string in that case.
+    JS_EXPORT_PRIVATE void resolveRope(ExecState* nullOrExecForOOM) const;
     JS_EXPORT_PRIVATE void resolveRopeToAtomicString(ExecState*) const;
     JS_EXPORT_PRIVATE RefPtr<AtomicStringImpl> resolveRopeToExistingAtomicString(ExecState*) const;
     void resolveRopeSlowCase8(LChar*) const;
     void resolveRopeSlowCase(UChar*) const;
-    void outOfMemory(ExecState*) const;
+    void outOfMemory(ExecState* nullOrExecForOOM) const;
     void resolveRopeInternal8(LChar*) const;
     void resolveRopeInternal8NoSubstring(LChar*) const;
     void resolveRopeInternal16(UChar*) const;
@@ -548,8 +549,10 @@ inline const String& JSString::value(ExecState* exec) const
 
 inline const String& JSString::tryGetValue() const
 {
-    if (isRope())
-        static_cast<const JSRopeString*>(this)->resolveRope(0);
+    if (isRope()) {
+        // Pass nullptr for the ExecState so that resolveRope does not throw in the event of an OOM error.
+        static_cast<const JSRopeString*>(this)->resolveRope(nullptr);
+    }
     return m_value;
 }