Remove JSString::SafeView and replace its uses with StringViewWithUnderlyingString.
authormark.lam@apple.com <mark.lam@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 16 Nov 2016 00:19:54 +0000 (00:19 +0000)
committermark.lam@apple.com <mark.lam@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 16 Nov 2016 00:19:54 +0000 (00:19 +0000)
https://bugs.webkit.org/show_bug.cgi?id=164777

Reviewed by Geoffrey Garen.

JSString::SafeView no longer achieves its intended goal to make it easier to
handle strings safely.  Its clients still need to do explicit exception checks in
order to be correct.  We'll remove it and replace its uses with
StringViewWithUnderlyingString instead which serves to gets the a StringView
(which is what we really wanted from SafeView) and keeps the backing String alive
while the view is in use.

Also added some missing exception checks.

* jsc.cpp:
(printInternal):
(functionDebug):
* runtime/ArrayPrototype.cpp:
(JSC::arrayProtoFuncJoin):
* runtime/FunctionConstructor.cpp:
(JSC::constructFunctionSkippingEvalEnabledCheck):
* runtime/IntlCollatorPrototype.cpp:
(JSC::IntlCollatorFuncCompare):
* runtime/JSGenericTypedArrayViewPrototypeFunctions.h:
(JSC::genericTypedArrayViewProtoFuncJoin):
* runtime/JSGlobalObjectFunctions.cpp:
(JSC::toStringView):
(JSC::globalFuncParseFloat):
* runtime/JSONObject.cpp:
(JSC::JSONProtoFuncParse):
* runtime/JSString.h:
(JSC::JSString::SafeView::is8Bit): Deleted.
(JSC::JSString::SafeView::length): Deleted.
(JSC::JSString::SafeView::SafeView): Deleted.
(JSC::JSString::SafeView::get): Deleted.
(JSC::JSString::view): Deleted.
* runtime/StringPrototype.cpp:
(JSC::stringProtoFuncRepeatCharacter):
(JSC::stringProtoFuncCharAt):
(JSC::stringProtoFuncCharCodeAt):
(JSC::stringProtoFuncIndexOf):
(JSC::stringProtoFuncNormalize):

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

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/jsc.cpp
Source/JavaScriptCore/runtime/ArrayPrototype.cpp
Source/JavaScriptCore/runtime/FunctionConstructor.cpp
Source/JavaScriptCore/runtime/IntlCollatorPrototype.cpp
Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h
Source/JavaScriptCore/runtime/JSGlobalObjectFunctions.cpp
Source/JavaScriptCore/runtime/JSONObject.cpp
Source/JavaScriptCore/runtime/JSString.h
Source/JavaScriptCore/runtime/StringPrototype.cpp

index bb0b89fbff1c4f5c75cfbd7797805dc3b3614280..d9b5cd0ce5ae601fe5bbe6de446c82fb1a44165e 100644 (file)
@@ -1,3 +1,48 @@
+2016-11-15  Mark Lam  <mark.lam@apple.com>
+
+        Remove JSString::SafeView and replace its uses with StringViewWithUnderlyingString.
+        https://bugs.webkit.org/show_bug.cgi?id=164777
+
+        Reviewed by Geoffrey Garen.
+
+        JSString::SafeView no longer achieves its intended goal to make it easier to
+        handle strings safely.  Its clients still need to do explicit exception checks in
+        order to be correct.  We'll remove it and replace its uses with
+        StringViewWithUnderlyingString instead which serves to gets the a StringView
+        (which is what we really wanted from SafeView) and keeps the backing String alive
+        while the view is in use.
+
+        Also added some missing exception checks.
+
+        * jsc.cpp:
+        (printInternal):
+        (functionDebug):
+        * runtime/ArrayPrototype.cpp:
+        (JSC::arrayProtoFuncJoin):
+        * runtime/FunctionConstructor.cpp:
+        (JSC::constructFunctionSkippingEvalEnabledCheck):
+        * runtime/IntlCollatorPrototype.cpp:
+        (JSC::IntlCollatorFuncCompare):
+        * runtime/JSGenericTypedArrayViewPrototypeFunctions.h:
+        (JSC::genericTypedArrayViewProtoFuncJoin):
+        * runtime/JSGlobalObjectFunctions.cpp:
+        (JSC::toStringView):
+        (JSC::globalFuncParseFloat):
+        * runtime/JSONObject.cpp:
+        (JSC::JSONProtoFuncParse):
+        * runtime/JSString.h:
+        (JSC::JSString::SafeView::is8Bit): Deleted.
+        (JSC::JSString::SafeView::length): Deleted.
+        (JSC::JSString::SafeView::SafeView): Deleted.
+        (JSC::JSString::SafeView::get): Deleted.
+        (JSC::JSString::view): Deleted.
+        * runtime/StringPrototype.cpp:
+        (JSC::stringProtoFuncRepeatCharacter):
+        (JSC::stringProtoFuncCharAt):
+        (JSC::stringProtoFuncCharCodeAt):
+        (JSC::stringProtoFuncIndexOf):
+        (JSC::stringProtoFuncNormalize):
+
 2016-11-15  Filip Pizlo  <fpizlo@apple.com>
 
         Unreviewed, remove bogus assertion.
index a18975756259c73303b07bf2e7763736dfd98e4a..9fbd22b22e1f4a9b62b3837b8deb26943640e309 100644 (file)
@@ -1531,6 +1531,9 @@ JSInternalPromise* GlobalObject::moduleLoaderFetch(JSGlobalObject* globalObject,
 
 static EncodedJSValue printInternal(ExecState* exec, FILE* out)
 {
+    VM& vm = exec->vm();
+    auto scope = DECLARE_THROW_SCOPE(vm);
+
     if (test262AsyncTest) {
         JSValue value = exec->argument(0);
         if (value.isString() && WTF::equal(asString(value)->value(exec).impl(), "Test262:AsyncTestComplete"))
@@ -1543,7 +1546,9 @@ static EncodedJSValue printInternal(ExecState* exec, FILE* out)
             if (EOF == fputc(' ', out))
                 goto fail;
 
-        if (fprintf(out, "%s", exec->uncheckedArgument(i).toString(exec)->view(exec).get().utf8().data()) < 0)
+        auto viewWithString = exec->uncheckedArgument(i).toString(exec)->viewWithUnderlyingString(*exec);
+        RETURN_IF_EXCEPTION(scope, encodedJSValue());
+        if (fprintf(out, "%s", viewWithString.view.utf8().data()) < 0)
             goto fail;
     }
 
@@ -1569,7 +1574,11 @@ EncodedJSValue JSC_HOST_CALL functionDumpCallFrame(ExecState* exec)
 
 EncodedJSValue JSC_HOST_CALL functionDebug(ExecState* exec)
 {
-    fprintf(stderr, "--> %s\n", exec->argument(0).toString(exec)->view(exec).get().utf8().data());
+    VM& vm = exec->vm();
+    auto scope = DECLARE_THROW_SCOPE(vm);
+    auto viewWithString = exec->argument(0).toString(exec)->viewWithUnderlyingString(*exec);
+    RETURN_IF_EXCEPTION(scope, encodedJSValue());
+    fprintf(stderr, "--> %s\n", viewWithString.view.utf8().data());
     return JSValue::encode(jsUndefined());
 }
 
index 5add8143f37bbd8c8999666269a0d6c2dff18909..ad5dce24586c4b31cde595ba2b1f7f6e30b371a9 100644 (file)
@@ -667,7 +667,11 @@ EncodedJSValue JSC_HOST_CALL arrayProtoFuncJoin(ExecState* exec)
         return JSValue::encode(slowJoin(*exec, thisObject, jsSeparator, length64));
     }
 
-    return JSValue::encode(fastJoin(*exec, thisObject, jsSeparator->view(exec).get(), length));
+    auto viewWithString = jsSeparator->viewWithUnderlyingString(*exec);
+    RETURN_IF_EXCEPTION(scope, encodedJSValue());
+
+    scope.release();
+    return JSValue::encode(fastJoin(*exec, thisObject, viewWithString.view, length));
 }
 
 EncodedJSValue JSC_HOST_CALL arrayProtoFuncPop(ExecState* exec)
index 0a1a34c66ca2de3aeac0a84e15a51011a9cab205..267cdb187bf7e5e3f03eeaee1c664365427d67f9 100644 (file)
@@ -123,13 +123,19 @@ JSObject* constructFunctionSkippingEvalEnabledCheck(
         builder.append(prefix);
         builder.append(functionName.string());
         builder.append('(');
-        builder.append(args.at(0).toString(exec)->view(exec).get());
+        auto viewWithString = args.at(0).toString(exec)->viewWithUnderlyingString(*exec);
+        RETURN_IF_EXCEPTION(scope, nullptr);
+        builder.append(viewWithString.view);
         for (size_t i = 1; i < args.size() - 1; i++) {
             builder.appendLiteral(", ");
-            builder.append(args.at(i).toString(exec)->view(exec).get());
+            auto viewWithString = args.at(i).toString(exec)->viewWithUnderlyingString(*exec);
+            RETURN_IF_EXCEPTION(scope, nullptr);
+            builder.append(viewWithString.view);
         }
         builder.appendLiteral(") {\n");
-        builder.append(args.at(args.size() - 1).toString(exec)->view(exec).get());
+        viewWithString = args.at(args.size() - 1).toString(exec)->viewWithUnderlyingString(*exec);
+        RETURN_IF_EXCEPTION(scope, nullptr);
+        builder.append(viewWithString.view);
         builder.appendLiteral("\n}}");
         program = builder.toString();
     }
index ff82568f608279c7c81fd9a1d70cbdfcd938156b..73ae062bcedcc6efc2a9b27ad4d1fce81650b90d 100644 (file)
@@ -98,7 +98,12 @@ static EncodedJSValue JSC_HOST_CALL IntlCollatorFuncCompare(ExecState* state)
     RETURN_IF_EXCEPTION(scope, encodedJSValue());
 
     // 9. Return CompareStrings(collator, X, Y).
-    return JSValue::encode(collator->compareStrings(*state, x->view(state).get(), y->view(state).get()));
+    auto xViewWithString = x->viewWithUnderlyingString(*state);
+    RETURN_IF_EXCEPTION(scope, encodedJSValue());
+    auto yViewWithString = y->viewWithUnderlyingString(*state);
+    RETURN_IF_EXCEPTION(scope, encodedJSValue());
+    scope.release();
+    return JSValue::encode(collator->compareStrings(*state, xViewWithString.view, yViewWithString.view));
 }
 
 EncodedJSValue JSC_HOST_CALL IntlCollatorPrototypeGetterCompare(ExecState* state)
index ed325688f0b257a577fcc18d8b39c70270242695..25b9092c6a5f6c0062739e5dbb7496878050d39c 100644 (file)
@@ -287,7 +287,9 @@ EncodedJSValue JSC_HOST_CALL genericTypedArrayViewProtoFuncJoin(VM& vm, ExecStat
 
     if (thisObject->isNeutered())
         return throwVMTypeError(exec, scope, typedArrayBufferHasBeenDetachedErrorMessage);
-    return joinWithSeparator(separatorString->view(exec).get());
+    auto viewWithString = separatorString->viewWithUnderlyingString(*exec);
+    RETURN_IF_EXCEPTION(scope, encodedJSValue());
+    return joinWithSeparator(viewWithString.view);
 }
 
 template<typename ViewClass>
index ad34dfa68b194b6bf175f8ec3dbc814f9b14331c..d0021fe4121f41281641dcc5c743be708f1bc031 100644 (file)
@@ -65,10 +65,9 @@ static ALWAYS_INLINE typename std::result_of<CallbackWhenNoException(StringView)
     JSString* string = value.toStringOrNull(exec);
     if (UNLIKELY(!string))
         return { };
-    JSString::SafeView view = string->view(exec);
-    StringView stringView = view.get();
+    auto viewWithString = string->viewWithUnderlyingString(*exec);
     RETURN_IF_EXCEPTION(scope, { });
-    return callback(stringView);
+    return callback(viewWithString.view);
 }
 
 template<unsigned charactersCount>
@@ -718,7 +717,8 @@ EncodedJSValue JSC_HOST_CALL globalFuncParseInt(ExecState* exec)
 
 EncodedJSValue JSC_HOST_CALL globalFuncParseFloat(ExecState* exec)
 {
-    return JSValue::encode(jsNumber(parseFloat(exec->argument(0).toString(exec)->view(exec).get())));
+    auto viewWithString = exec->argument(0).toString(exec)->viewWithUnderlyingString(*exec);
+    return JSValue::encode(jsNumber(parseFloat(viewWithString.view)));
 }
 
 EncodedJSValue JSC_HOST_CALL globalFuncDecodeURI(ExecState* exec)
index b2151300e0b6c4d171995a786c572a822ea2a771..bc49e3da3592ebd76f6d4d85a8ad6538512dc62e 100644 (file)
@@ -761,10 +761,9 @@ EncodedJSValue JSC_HOST_CALL JSONProtoFuncParse(ExecState* exec)
 
     if (!exec->argumentCount())
         return throwVMError(exec, scope, createError(exec, ASCIILiteral("JSON.parse requires at least one parameter")));
-    JSString::SafeView source = exec->uncheckedArgument(0).toString(exec)->view(exec);
-    RETURN_IF_EXCEPTION(scope, encodedJSValue());
-    StringView view = source.get();
+    auto viewWithString = exec->uncheckedArgument(0).toString(exec)->viewWithUnderlyingString(*exec);
     RETURN_IF_EXCEPTION(scope, encodedJSValue());
+    StringView view = viewWithString.view;
 
     JSValue unfiltered;
     LocalScope localScope(vm);
index 8115e204670805b72ad1eae9e45b09ad16e65ef6..7cd70f6231574193318b7e8ae9b49e08e3a0a70f 100644 (file)
@@ -150,8 +150,6 @@ public:
     AtomicString toAtomicString(ExecState*) const;
     RefPtr<AtomicStringImpl> toExistingAtomicString(ExecState*) const;
 
-    class SafeView;
-    SafeView view(ExecState*) const;
     StringViewWithUnderlyingString viewWithUnderlyingString(ExecState&) const;
 
     inline bool equal(ExecState*, JSString* other) const;
@@ -475,24 +473,6 @@ private:
     friend JSString* jsString(ExecState*, const String&, const String&, const String&);
 };
 
-class JSString::SafeView {
-public:
-    explicit SafeView(ExecState&, const JSString&);
-    StringView get() const;
-
-    bool is8Bit() const { return m_string->is8Bit(); }
-    unsigned length() const { return m_string->length(); }
-
-private:
-    ExecState& m_state;
-
-    // The following pointer is marked "volatile" to make the compiler leave it on the stack
-    // or in a register as long as this object is alive, even after the last use of the pointer.
-    // That's needed to prevent garbage collecting the string and possibly deleting the block
-    // with the characters in it, and then using the StringView after that.
-    const JSString* volatile m_string;
-};
-
 JS_EXPORT_PRIVATE JSString* jsStringWithCacheSlowCase(VM&, StringImpl&);
 
 inline const StringImpl* JSString::tryGetValueImpl() const
@@ -759,22 +739,6 @@ inline bool JSString::isSubstring() const
     return isRope() && static_cast<const JSRopeString*>(this)->isSubstring();
 }
 
-inline JSString::SafeView::SafeView(ExecState& state, const JSString& string)
-    : m_state(state)
-    , m_string(&string)
-{
-}
-
-inline StringView JSString::SafeView::get() const
-{
-    return m_string->unsafeView(m_state);
-}
-
-ALWAYS_INLINE JSString::SafeView JSString::view(ExecState* exec) const
-{
-    return SafeView(*exec, *this);
-}
-
 // --- JSValue inlines ----------------------------
 
 inline bool JSValue::toBoolean(ExecState* exec) const
index 680288181a9f4727514fee5289a28522c19bf69b..acfc584c8e1d3316fd57aa009810f2f581e59b46 100644 (file)
@@ -812,8 +812,8 @@ EncodedJSValue JSC_HOST_CALL stringProtoFuncRepeatCharacter(ExecState* exec)
     ASSERT(repeatCount >= 0);
     ASSERT(!repeatCountValue.isDouble() || repeatCountValue.asDouble() == repeatCount);
 
-    JSString::SafeView safeView = string->view(exec);
-    StringView view = safeView.get();
+    auto viewWithString = string->viewWithUnderlyingString(*exec);
+    StringView view = viewWithString.view;
     ASSERT(view.length() == 1 && !scope.exception());
     UChar character = view[0];
     scope.release();
@@ -906,10 +906,9 @@ EncodedJSValue JSC_HOST_CALL stringProtoFuncCharAt(ExecState* exec)
     JSValue thisValue = exec->thisValue();
     if (!checkObjectCoercible(thisValue))
         return throwVMTypeError(exec, scope);
-    JSString::SafeView string = thisValue.toString(exec)->view(exec);
-    RETURN_IF_EXCEPTION(scope, encodedJSValue());
-    StringView view = string.get();
+    auto viewWithString = thisValue.toString(exec)->viewWithUnderlyingString(*exec);
     RETURN_IF_EXCEPTION(scope, encodedJSValue());
+    StringView view = viewWithString.view;
     JSValue a0 = exec->argument(0);
     if (a0.isUInt32()) {
         uint32_t i = a0.asUInt32();
@@ -931,11 +930,9 @@ EncodedJSValue JSC_HOST_CALL stringProtoFuncCharCodeAt(ExecState* exec)
     JSValue thisValue = exec->thisValue();
     if (!checkObjectCoercible(thisValue))
         return throwVMTypeError(exec, scope);
-    JSString* jsString = thisValue.toString(exec);
-    RETURN_IF_EXCEPTION(scope, encodedJSValue());
-    JSString::SafeView string = jsString->view(exec);
-    StringView view = string.get();
+    auto viewWithString = thisValue.toString(exec)->viewWithUnderlyingString(*exec);
     RETURN_IF_EXCEPTION(scope, encodedJSValue());
+    StringView view = viewWithString.view;
     JSValue a0 = exec->argument(0);
     if (a0.isUInt32()) {
         uint32_t i = a0.asUInt32();
@@ -1035,7 +1032,11 @@ EncodedJSValue JSC_HOST_CALL stringProtoFuncIndexOf(ExecState* exec)
     if (thisJSString->length() < otherJSString->length() + pos)
         return JSValue::encode(jsNumber(-1));
 
-    size_t result = thisJSString->view(exec).get().find(otherJSString->view(exec).get(), pos);
+    auto thisViewWithString = thisJSString->viewWithUnderlyingString(*exec);
+    RETURN_IF_EXCEPTION(scope, encodedJSValue());
+    auto otherViewWithString = otherJSString->viewWithUnderlyingString(*exec);
+    RETURN_IF_EXCEPTION(scope, encodedJSValue());
+    size_t result = thisViewWithString.view.find(otherViewWithString.view, pos);
     if (result == notFound)
         return JSValue::encode(jsNumber(-1));
     return JSValue::encode(jsNumber(result));
@@ -2016,10 +2017,9 @@ EncodedJSValue JSC_HOST_CALL stringProtoFuncNormalize(ExecState* exec)
     JSValue thisValue = exec->thisValue();
     if (!checkObjectCoercible(thisValue))
         return throwVMTypeError(exec, scope);
-    JSString::SafeView source = thisValue.toString(exec)->view(exec);
-    RETURN_IF_EXCEPTION(scope, encodedJSValue());
-    StringView view = source.get();
+    auto viewWithString = thisValue.toString(exec)->viewWithUnderlyingString(*exec);
     RETURN_IF_EXCEPTION(scope, encodedJSValue());
+    StringView view = viewWithString.view;
 
     UNormalizationMode form = UNORM_NFC;
     // Verify that the argument is provided and is not undefined.