Use asString instead of toWTFString, toString, or getString when we already checked isString
Created attachment 297185 [details] Patch
Comment on attachment 297185 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=297185&action=review > Source/JavaScriptCore/bindings/ScriptValue.cpp:64 > + return InspectorValue::create(asString(value)->value(&scriptState)); The `value(exec)` call is now exception checked in JSC code because it can resolve the rope string and could throw OOM error. Sometimes, we did not handle this exception and call CRASH() instead (e.g. ES6 Map's rehashing. If rehashing fails due to OOM, we have no way to recover the state of VM.). But anyway, we need to take some reaction for OOM error. (Of course, we should insert exception checks after getString(exec)). I think we still did not apply this rewriting to the outside of JSC, but we need to follow this policy eventually. Mark, is my understanding correct? Or, another way is adding the helper function (like, valueOrCrash()) that calls CRASH when OOM happens.
Oops, good point. I will put back the exception handling I removed.
Using asString is still better for,the other reasons.
Comment on attachment 297185 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=297185&action=review >> Source/JavaScriptCore/bindings/ScriptValue.cpp:64 >> + return InspectorValue::create(asString(value)->value(&scriptState)); > > The `value(exec)` call is now exception checked in JSC code because it can resolve the rope string and could throw OOM error. > Sometimes, we did not handle this exception and call CRASH() instead (e.g. ES6 Map's rehashing. If rehashing fails due to OOM, we have no way to recover the state of VM.). But anyway, we need to take some reaction for OOM error. > (Of course, we should insert exception checks after getString(exec)). > > I think we still did not apply this rewriting to the outside of JSC, but we need to follow this policy eventually. > Mark, is my understanding correct? > > Or, another way is adding the helper function (like, valueOrCrash()) that calls CRASH when OOM happens. I think it's correct to use asStting() here as Darin has implemented it. If the call to value(&scriptState) throws an exception, it will simply propagate to the caller. This is no different than the getString() call before. As for exception checks in general, I don't think we need to overly worry about it yet. I'm still working on enabling it for all of JSC. Once that is done, the tests (and bots) will start catching any regressions. Instead, we should just continue to focus on writing optimal code. But if there are already throwScopes and exception checks in place, please don't remove them. In the case of jsMapHash() specifically, maybe we should change that to a RELEASE_ASSERT on !scope.exception() if there's really no way to remedy the failure to hash. Alternatively, maybe we can teach the code to hash a rope without resolving it?
Created attachment 297279 [details] Patch
The new patch makes no changes to exception handling. I agree that we have inconsistent handing of the out-of-memory exception. There is code that will ignore it and probably should not, and we have no exhaustive way to test it, nor a good idiom for when we want to crash if it happens. I’m not tackling that problem here at all; just doing what I set out to do so that we use typecasts instead of a more complex conversion function when we already checked that something is a string.
Comment on attachment 297185 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=297185&action=review >>> Source/JavaScriptCore/bindings/ScriptValue.cpp:64 >>> + return InspectorValue::create(asString(value)->value(&scriptState)); >> >> The `value(exec)` call is now exception checked in JSC code because it can resolve the rope string and could throw OOM error. >> Sometimes, we did not handle this exception and call CRASH() instead (e.g. ES6 Map's rehashing. If rehashing fails due to OOM, we have no way to recover the state of VM.). But anyway, we need to take some reaction for OOM error. >> (Of course, we should insert exception checks after getString(exec)). >> >> I think we still did not apply this rewriting to the outside of JSC, but we need to follow this policy eventually. >> Mark, is my understanding correct? >> >> Or, another way is adding the helper function (like, valueOrCrash()) that calls CRASH when OOM happens. > > I think it's correct to use asStting() here as Darin has implemented it. If the call to value(&scriptState) throws an exception, it will simply propagate to the caller. This is no different than the getString() call before. > > As for exception checks in general, I don't think we need to overly worry about it yet. I'm still working on enabling it for all of JSC. Once that is done, the tests (and bots) will start catching any regressions. Instead, we should just continue to focus on writing optimal code. But if there are already throwScopes and exception checks in place, please don't remove them. > > In the case of jsMapHash() specifically, maybe we should change that to a RELEASE_ASSERT on !scope.exception() if there's really no way to remedy the failure to hash. Alternatively, maybe we can teach the code to hash a rope without resolving it? Thank you for your clarification. So for now, just replacing to the asString() code seems nice. (Not dropping the exception check). > In the case of jsMapHash() specifically, maybe we should change that to a RELEASE_ASSERT on !scope.exception() if there's really no way to remedy the failure to hash. Alternatively, maybe we can teach the code to hash a rope without resolving it? Avoiding rope strings for JSMap keys seems nice idea! It would be nice if it is done in the separate patch.
Comment on attachment 297279 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=297279&action=review > Source/WebCore/bindings/js/ArrayValue.cpp:93 > if (indexedValue.isUndefinedOrNull() || !indexedValue.isString()) I think it can be `if (!indexedValue.isString())`.
Comment on attachment 297279 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=297279&action=review >> Source/WebCore/bindings/js/ArrayValue.cpp:93 >> if (indexedValue.isUndefinedOrNull() || !indexedValue.isString()) > > I think it can be `if (!indexedValue.isString())`. It definitely can. I see a lot of similar mistakes throughout this file. On the other hand, there is a reasonable chance we will delete this class entirely, or at least most of the functions in it, soon. So my idea was to just do the asString changes and leave the rest of the code alone for now. But I will make this particular change before landing the patch.
Committed r209906: <http://trac.webkit.org/changeset/209906>