Changeset 206405 in webkit

Timestamp:

Sep 26, 2016 4:56:37 PM (8 years ago)

Author:

mark.lam@apple.com

Message:

Add some needed CatchScopes in code that should not throw.
​https://bugs.webkit.org/show_bug.cgi?id=162584

Reviewed by Keith Miller.

• API/JSObjectRef.cpp:

(JSObjectSetProperty):

• This function already handles exceptions in its own way. We're honoring this contract and catching exceptions and passing it to the handler.

• interpreter/Interpreter.cpp:

(JSC::notifyDebuggerOfUnwinding):

• The debugger should not be throwing any exceptions.

• jsc.cpp:

(runJSC):

• the buck stops here. There's no reason an exception should propagate past here.

• profiler/ProfilerDatabase.cpp:

(JSC::Profiler::Database::save):

• If an exception was thrown while saving the database, there's nothing we can really do about it anyway. Just fail nicely and return false. This is in line with existing error checking code in Database::save() that returns false if it's not able to open the file to save to.

• runtime/ExceptionHelpers.cpp:

(JSC::createError):

• If we're not able to stringify the error value, then we'll just use the provided message as the error string. It doesn't make sense to have the Error factory throw an exception that shadows the intended exception that the client probably wants to throw (assuming that that's why the client is creating this Error object).

• runtime/JSModuleLoader.cpp:

(JSC::JSModuleLoader::finishCreation):

• The existing code already RELEASE_ASSERT that no exception was thrown. Hence, it's appropriate to use a CatchScope here.

• runtime/SamplingProfiler.cpp:

(JSC::SamplingProfiler::StackFrame::nameFromCallee):

• The sampling profiler is doing a VMInquiry get here. It should never throw an exception. Hence, we'll just use a CatchScope and assert accordingly.

Location:

trunk/Source/JavaScriptCore

Files:

• API/JSObjectRef.cpp (modified) (2 diffs)
• ChangeLog (modified) (1 diff)
• interpreter/Interpreter.cpp (modified) (2 diffs)
• jsc.cpp (modified) (2 diffs)
• profiler/ProfilerDatabase.cpp (modified) (1 diff)
• runtime/ExceptionHelpers.cpp (modified) (1 diff)
• runtime/JSModuleLoader.cpp (modified) (1 diff)
• runtime/SamplingProfiler.cpp (modified) (1 diff)

trunk/Source/JavaScriptCore/API/JSObjectRef.cpp

@@ -310 +310 @@
     }
     ExecState* exec = toJS(ctx);
-    JSLockHolder locker(exec);
+    VM& vm = exec->vm();
+    JSLockHolder locker(vm);
+    auto scope = DECLARE_CATCH_SCOPE(vm);
 
     JSObject* jsObject = toJS(object);
@@ -316 +318 @@
     JSValue jsValue = toJS(exec, value);
 
-    if (attributes && !jsObject->hasProperty(exec, name)) {
-        PropertyDescriptor desc(jsValue, attributes);
-        jsObject->methodTable()->defineOwnProperty(jsObject, exec, name, desc, false);
-    } else {
-        PutPropertySlot slot(jsObject);
-        jsObject->methodTable()->put(jsObject, exec, name, jsValue, slot);
-    }
-
+    bool doesNotHaveProperty = attributes && !jsObject->hasProperty(exec, name);
+    if (LIKELY(!scope.exception())) {
+        if (doesNotHaveProperty) {
+            PropertyDescriptor desc(jsValue, attributes);
+            jsObject->methodTable()->defineOwnProperty(jsObject, exec, name, desc, false);
+        } else {
+            PutPropertySlot slot(jsObject);
+            jsObject->methodTable()->put(jsObject, exec, name, jsValue, slot);
+        }
+    }
     handleExceptionIfNeeded(exec, exception);
 }

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2016-09-26  Mark Lam  <mark.lam@apple.com>
+
+        Add some needed CatchScopes in code that should not throw.
+        https://bugs.webkit.org/show_bug.cgi?id=162584
+
+        Reviewed by Keith Miller.
+
+        * API/JSObjectRef.cpp:
+        (JSObjectSetProperty):
+        - This function already handles exceptions in its own way.  We're honoring this
+          contract and catching exceptions and passing it to the handler.
+
+        * interpreter/Interpreter.cpp:
+        (JSC::notifyDebuggerOfUnwinding):
+        - The debugger should not be throwing any exceptions.
+
+        * jsc.cpp:
+        (runJSC):
+        - the buck stops here.  There's no reason an exception should propagate past here.
+
+        * profiler/ProfilerDatabase.cpp:
+        (JSC::Profiler::Database::save):
+        - If an exception was thrown while saving the database, there's nothing we can
+          really do about it anyway.  Just fail nicely and return false.  This is in line
+          with existing error checking code in Database::save() that returns false if
+          it's not able to open the file to save to.
+
+        * runtime/ExceptionHelpers.cpp:
+        (JSC::createError):
+        - If we're not able to stringify the error value, then we'll just use the
+          provided message as the error string.  It doesn't make sense to have the
+          Error factory throw an exception that shadows the intended exception that the
+          client probably wants to throw (assuming that that's why the client is creating
+          this Error object).
+
+        * runtime/JSModuleLoader.cpp:
+        (JSC::JSModuleLoader::finishCreation):
+        - The existing code already RELEASE_ASSERT that no exception was thrown.
+          Hence, it's appropriate to use a CatchScope here.
+
+        * runtime/SamplingProfiler.cpp:
+        (JSC::SamplingProfiler::StackFrame::nameFromCallee):
+        - The sampling profiler is doing a VMInquiry get here.  It should never throw an
+          exception.  Hence, we'll just use a CatchScope and assert accordingly.
+
 2016-09-26  Mark Lam  <mark.lam@apple.com>
 

trunk/Source/JavaScriptCore/interpreter/Interpreter.cpp

@@ -584 +584 @@
 {
     VM& vm = callFrame->vm();
-    auto throwScope = DECLARE_THROW_SCOPE(vm);
+    auto catchScope = DECLARE_CATCH_SCOPE(vm);
     if (Debugger* debugger = callFrame->vmEntryGlobalObject()->debugger()) {
         SuspendExceptionScope scope(&vm);
@@ -591 +591 @@
         else
             debugger->didExecuteProgram(callFrame);
-        ASSERT_UNUSED(throwScope, !throwScope.exception());
+        ASSERT_UNUSED(catchScope, !catchScope.exception());
     }
 }

trunk/Source/JavaScriptCore/jsc.cpp

@@ -2540 +2540 @@
 {
     JSLockHolder locker(vm);
+    auto scope = DECLARE_CATCH_SCOPE(*vm);
 
     int result;
@@ -2546 +2547 @@
 
     GlobalObject* globalObject = GlobalObject::create(*vm, GlobalObject::createStructure(*vm, jsNull()), options.m_arguments);
+    RETURN_IF_EXCEPTION(scope, 3);
+
     bool success = runWithScripts(globalObject, options.m_scripts, options.m_uncaughtExceptionName, options.m_alwaysDumpUncaughtException, options.m_dump, options.m_module);
     if (options.m_interactive && success)

trunk/Source/JavaScriptCore/profiler/ProfilerDatabase.cpp

@@ -135 +135 @@
 bool Database::save(const char* filename) const
 {
+    auto scope = DECLARE_CATCH_SCOPE(m_vm);
     auto out = FilePrintStream::open(filename, "w");
     if (!out)
         return false;
     
-    out->print(toJSON());
+    String data = toJSON();
+    if (UNLIKELY(scope.exception())) {
+        scope.clearException();
+        return false;
+    }
+    out->print(data);
     return true;
 }

trunk/Source/JavaScriptCore/runtime/ExceptionHelpers.cpp

@@ -237 +237 @@
 JSObject* createError(ExecState* exec, JSValue value, const String& message, ErrorInstance::SourceAppender appender)
 {
+    VM& vm = exec->vm();
+    auto scope = DECLARE_CATCH_SCOPE(vm);
+
     String errorMessage = makeString(errorDescriptionForValue(exec, value)->value(exec), ' ', message);
+    if (UNLIKELY(scope.exception())) {
+        scope.clearException();
+        errorMessage = message;
+    }
     JSObject* exception = createTypeError(exec, errorMessage, appender, runtimeTypeForValue(value));
     ASSERT(exception->isErrorInstance());

trunk/Source/JavaScriptCore/runtime/JSModuleLoader.cpp

@@ -58 +58 @@
 void JSModuleLoader::finishCreation(ExecState* exec, VM& vm, JSGlobalObject* globalObject)
 {
+    auto scope = DECLARE_CATCH_SCOPE(vm);
+
     Base::finishCreation(vm);
     ASSERT(inherits(info()));
-    auto scope = DECLARE_THROW_SCOPE(vm);
     JSMap* map = JSMap::create(exec, vm, globalObject->mapStructure());
     RELEASE_ASSERT(!scope.exception());

trunk/Source/JavaScriptCore/runtime/SamplingProfiler.cpp

@@ -592 +592 @@
         return String();
 
+    auto scope = DECLARE_CATCH_SCOPE(vm);
     ExecState* exec = callee->globalObject()->globalExec();
     auto getPropertyIfPureOperation = [&] (const Identifier& ident) -> String {
         PropertySlot slot(callee, PropertySlot::InternalMethodType::VMInquiry);
         PropertyName propertyName(ident);
-        if (callee->getPropertySlot(exec, propertyName, slot)) {
+        bool hasProperty = callee->getPropertySlot(exec, propertyName, slot);
+        ASSERT_UNUSED(scope, !scope.exception());
+        if (hasProperty) {
             if (slot.isValue()) {
                 JSValue nameValue = slot.getValue(exec, propertyName);