Bug 165895 – Use asString instead of toWTFString, toString, or getString when we already checked isString

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Darin Adler]

Use asString instead of toWTFString, toString, or getString when we already checked isString

[Darin Adler]

Created attachment 297185 [details]
Patch

[Yusuke Suzuki]

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]

Oops, good point. I will put back the exception handling I removed.

[Darin Adler]

Using asString is still better for,the other reasons.

[Mark Lam]

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]

Created attachment 297279 [details]
Patch

[Darin Adler]

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 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 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 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]

Committed r209906: <http://trac.webkit.org/changeset/209906>