In some places (such as operationPutByValInternal), exception check is missing after toPropertyKey. However, since it calls toString and it could raise exceptions, we should perform exception check.
We should also write a regression test for this. I think it can be done.
(In reply to comment #1) > We should also write a regression test for this. I think it can be done. Seeing the code, I found that we cannot observe this regression. Since + Missing exception checks are placed in GetByVal codes + Workaround exception check was inserted in callGetter (http://trac.webkit.org/browser/trunk/Source/JavaScriptCore/runtime/GetterSetter.cpp#L77)
Created attachment 249171 [details] Patch
Comment on attachment 249171 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=249171&action=review Added comments. > Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:575 > + ASSERT(!exec->hadException()); Since property is always JSString. > Source/JavaScriptCore/runtime/ObjectPrototype.cpp:91 > + return JSValue::encode(jsBoolean(thisValue.toObject(exec)->hasOwnProperty(exec, propertyName))); When performing toObject onto null/undefined, it returnes JSNotAObject. This can allow us to defer exception checks live the above code.
Comment on attachment 249171 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=249171&action=review > Source/JavaScriptCore/ChangeLog:15 > + But since callGetter has conservative (defensive) exception guard, > + missing exception checks in GetByVal of JIT/DFG/LLInt cannot be observed. If they can’t be observed, the checks aren’t needed.
Comment on attachment 249171 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=249171&action=review Code generally looks good. Please don’t land the changes that are not testable/observable, though. >> Source/JavaScriptCore/ChangeLog:15 >> + missing exception checks in GetByVal of JIT/DFG/LLInt cannot be observed. > > If they can’t be observed, the checks aren’t needed. Any that can’t be observed aren’t actually bugs! > Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:576 > + auto propertyName = property.toPropertyKey(exec); > + ASSERT(!exec->hadException()); > + RETURN(baseValue.get(exec, propertyName)); I would write this: RETURN(baseValue.get(exec, asString(property)->toIdentifier(exec)); I don’t see any reason to call a function that is then going to check if property is a string. Nor do I see a need to split this out into multiple lines of code just to assert there is no exception.
Comment on attachment 249171 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=249171&action=review I've found one more issues. I'll fix it with subsequent patch. >>> Source/JavaScriptCore/ChangeLog:15 >>> + missing exception checks in GetByVal of JIT/DFG/LLInt cannot be observed. >> >> If they can’t be observed, the checks aren’t needed. > > Any that can’t be observed aren’t actually bugs! After investigating the code, I've found that the current code works fine and my change breaks it. I'll write it in LLIntSlowPaths.cpp. And I'll fix my patch with explicit comments. > Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:751 > return baseValue.get(exec, property); Reaching here, `baseValue` may be null or undefined. So baseValue.get also may cause errors. There's several source of exceptions. 1. baseValue.get can cause errors if it's null or undefined. 2. toPropertyKey can cause errors Seeing the spec, VM first checks (1) and perfoms toPropertyKey and checks (2). So the above fix cause observable incorrect behavior. var object = null; var property = { toString() { throw new Error("out"); } } object[property]; // We should see TypeError (since object is null), but with the above fix, we'll see new Error("out"); I think it's observable. I'll update the patch. >> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:576 >> + RETURN(baseValue.get(exec, propertyName)); > > I would write this: > > RETURN(baseValue.get(exec, asString(property)->toIdentifier(exec)); > > I don’t see any reason to call a function that is then going to check if property is a string. Nor do I see a need to split this out into multiple lines of code just to assert there is no exception. Looks nice. I'll fix it.
(In reply to comment #7) Typo: subsequent => updated.
Created attachment 249194 [details] Patch
Comment on attachment 249194 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=249194&action=review > Source/JavaScriptCore/dfg/DFGOperations.cpp:300 > + if (baseValue.isUndefinedOrNull()) > + return JSValue::encode(vm.throwException(exec, createNotAnObjectError(exec, baseValue))); Won't JSValue::get do this check for us? > Source/JavaScriptCore/dfg/DFGOperations.cpp:331 > + ASSERT(!JSValue(base).isUndefinedOrNull()); I don't think this ASSERT makes sense because a cell will never be undefined or null, by virtue of it being a cell. > Source/JavaScriptCore/jit/JITOperations.cpp:1425 > + if (baseValue.isUndefinedOrNull()) > + return exec->vm().throwException(exec, createNotAnObjectError(exec, baseValue)); I think get will do this for us. You should review other places and find out if these checks are really needed. I guess I'm echoing Darin's comments a bit.
Comment on attachment 249194 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=249194&action=review Thank you for your review! >> Source/JavaScriptCore/dfg/DFGOperations.cpp:300 >> + return JSValue::encode(vm.throwException(exec, createNotAnObjectError(exec, baseValue))); > > Won't JSValue::get do this check for us? Yes, but we canot use this here. JSValue::get checks null/undefined and raise an exception if |this| is null or undefined. But, in this case, I don't use it because of the following 3 reasons. (1) We need to check whether the baseValue is object coercible before executing JSValue::toPropertyKey, Consider the following script, var object = null; var called = false; var property = { toString() { called = true; retrun "value"; } }; try { object[property]; } catch (e) { } assert(!called); toPropertyKey can cause toString call. So that is observable. If we use the following implementation, auto propertyName = property.toPropertyKey(exec); if (exec->hadException()) return JSValue::encode(jsUndefined()); return JSValue::encode(baseValue.get(exec, propertyName)); The above script causes an assertion failure. (2) I think we should not use baseValue.toObject here like the following, baseValue.toObject(exec); // Check object coercible by using toObject. if (exec->hadException()) return JSValue::encode(jsUndefined()); auto propertyName = property.toPropertyKey(exec); if (exec->hadException()) return JSValue::encode(jsUndefined()); return JSValue::encode(baseValue.get(exec, propertyName)); toObject converts primitive types into wrapper objects. But it is not efficient since wrapper objects are not necessary if we look up methods from primitive values's prototype. (using synthesizePrototype is better). (3) And we need to take care that we use JSValue::get in the last sequence instead of JSObject::get. If we use JSObject that is converted by toObject, getter will be called by using this JSObject as |this|. JSObject* baseObject = baseValue.toObject(exec); if (exec->hadException()) return JSValue::encode(jsUndefined()); auto propertyName = property.toPropertyKey(exec); if (exec->hadException()) return JSValue::encode(jsUndefined()); return JSValue::encode(baseObject->get(exec, propertyName)); // Using baseObject. This is not correct to the spec. It is not correct since getter should be called with the original |this| value that may be primitive types. So, in the updated patch, I checks whether baseValue is object coercible explicitly before calling toPropertyKey. But, you're right, using if (baseValue.isUndefinedOrNull()) return JSValue::encode(vm.throwException(exec, createNotAnObjectError(exec, baseValue))); explicitly is not good. Instead, I'll propose that introduce `JSValue::requireObjectCoercible`. The implementation has one on one corresponding to RequireObjectCoercible in the spec. http://people.mozilla.org/~jorendorff/es6-draft.html#sec-requireobjectcoercible And the implementation will be the following. baseValue.requireObjectCoercible(exec); if (exec->hadException()) return JSValue::encode(jsUndefined()); auto propertyName = property.toPropertyKey(exec); if (exec->hadException()) return JSValue::encode(jsUndefined()); return JSValue::encode(baseValue.get(exec, propertyName)); What do you think of? >> Source/JavaScriptCore/dfg/DFGOperations.cpp:331 >> + ASSERT(!JSValue(base).isUndefinedOrNull()); > > I don't think this ASSERT makes sense because a cell will never be undefined or null, by virtue of it being a cell. Thanks. OK, I'll drop this. >> Source/JavaScriptCore/jit/JITOperations.cpp:1425 >> + return exec->vm().throwException(exec, createNotAnObjectError(exec, baseValue)); > > I think get will do this for us. You should review other places and find out if these checks are really needed. I guess I'm echoing Darin's comments a bit. I've commented the reason why we use this check explicitly.
Created attachment 249316 [details] Patch Updated patch. Adding JSValue::requireObjectCoercible and use it intead of explicit isUndefinedOrNull checks
Created attachment 249317 [details] Patch Updated patch. Adding JSValue::requireObjectCoercible and use it intead of explicit isUndefinedOrNull checks
Comment on attachment 249317 [details] Patch r=me
Comment on attachment 249317 [details] Patch Clearing flags on attachment: 249317 Committed r182057: <http://trac.webkit.org/changeset/182057>
All reviewed patches have been landed. Closing bug.