WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
165895
Use asString instead of toWTFString, toString, or getString when we already checked isString
https://bugs.webkit.org/show_bug.cgi?id=165895
Summary
Use asString instead of toWTFString, toString, or getString when we already c...
Darin Adler
Reported
2016-12-15 00:58:18 PST
Use asString instead of toWTFString, toString, or getString when we already checked isString
Attachments
Patch
(42.42 KB, patch)
2016-12-15 01:15 PST
,
Darin Adler
no flags
Details
Formatted Diff
Diff
Patch
(39.19 KB, patch)
2016-12-15 18:30 PST
,
Darin Adler
ysuzuki
: review+
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Darin Adler
Comment 1
2016-12-15 01:15:07 PST
Created
attachment 297185
[details]
Patch
Yusuke Suzuki
Comment 2
2016-12-15 02:33:34 PST
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.
Darin Adler
Comment 3
2016-12-15 07:18:41 PST
Oops, good point. I will put back the exception handling I removed.
Darin Adler
Comment 4
2016-12-15 07:19:10 PST
Using asString is still better for,the other reasons.
Mark Lam
Comment 5
2016-12-15 09:59:23 PST
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?
Darin Adler
Comment 6
2016-12-15 18:30:39 PST
Created
attachment 297279
[details]
Patch
Darin Adler
Comment 7
2016-12-15 18:32:27 PST
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.
Yusuke Suzuki
Comment 8
2016-12-15 18:35:01 PST
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.
Yusuke Suzuki
Comment 9
2016-12-15 18:44:21 PST
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())`.
Darin Adler
Comment 10
2016-12-15 19:21:41 PST
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.
Darin Adler
Comment 11
2016-12-15 20:52:59 PST
Committed
r209906
: <
http://trac.webkit.org/changeset/209906
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug