Bug 214837 – ASSERTION FAILED: isSymbol() in Source/JavaScriptCore/runtime/JSCell.cpp(188)

• Component: JavaScriptCore
• Status: RESOLVED FIXED

Created attachment 405296 [details]
The JS file that causes a crash in the built JSC binary

I've attached the JS file that causes a crash on an asan-enabled JavaScriptCore binary. Here is how I built the binary (note: the commit hash for the version of WebKit I used is 3215a5ce92987ec8184b9133114e9f8213ae43ab):
=====================
git clone git://git.webkit.org/WebKit.git
cd WebKit
Tools/Scripts/set-webkit-configuration --asan --debug
Tools/Scripts/build-webkit --debug --jsc-only
=====================

Here is the output of JSC: 
=====================
ASSERTION FAILED: isSymbol()
../../Source/JavaScriptCore/runtime/JSCell.cpp(188) : JSC::JSObject* JSC::JSCell::toObjectSlow(JSC::JSGlobalObject*) const
Aborted
=====================

This is the function where it crashes (the arrow denotes the exact line):
=====================
JSObject* JSCell::toObjectSlow(JSGlobalObject* globalObject) const
{
    Integrity::auditStructureID(globalObject->vm(), structureID());
    ASSERT(!isObject());
    if (isString())
        return static_cast<const JSString*>(this)->toObject(globalObject);
    if (isHeapBigInt())
        return static_cast<const JSBigInt*>(this)->toObject(globalObject);
    =====> ASSERT(isSymbol());
    return static_cast<const Symbol*>(this)->toObject(globalObject);
}
=====================

[Radar WebKit Bug Importer]

<rdar://problem/66171782>

[Mark Lam]

This is not a real bug.  The test was iterating over special properties in the jsc shell that are only for testing use.  This scenario cannot manifest in any productized version of JSC because these special test properties will not be present.

I'll put together a patch to make all the special test properties non-enumerable so that they will stop tripping up people who doing blind enumeration on the jsc shell's test global object.

[Mark Lam]

Thanks for this report.  I will upload a patch to mitigate this issue shortly.

[Mark Lam]

Created attachment 405339 [details]
proposed patch.

[Darin Adler]

Comment on attachment 405339 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=405339&action=review

> Source/JavaScriptCore/jsc.cpp:477
> +    static constexpr unsigned DontEnum = 0 | PropertyAttribute::DontEnum;

What is the purpose of the "0 |" here? Is it a way to convert an enumeration value to an integer? Why is it better than static_cast?

[Mark Lam]

Comment on attachment 405339 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=405339&action=review

Thanks for the review.

>> Source/JavaScriptCore/jsc.cpp:477
>> +    static constexpr unsigned DontEnum = 0 | PropertyAttribute::DontEnum;
> 
> What is the purpose of the "0 |" here? Is it a way to convert an enumeration value to an integer? Why is it better than static_cast?

Yes, it's for converting the enumeration into an integer.  I chose it because it's less verbose than a static_cast<unsigned> cast.  It's better in my eyes because it concisely expresses the idea, but "better" here is subjective.

[Darin Adler]

Comment on attachment 405339 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=405339&action=review

>>> Source/JavaScriptCore/jsc.cpp:477
>>> +    static constexpr unsigned DontEnum = 0 | PropertyAttribute::DontEnum;
>> 
>> What is the purpose of the "0 |" here? Is it a way to convert an enumeration value to an integer? Why is it better than static_cast?
> 
> Yes, it's for converting the enumeration into an integer.  I chose it because it's less verbose than a static_cast<unsigned> cast.  It's better in my eyes because it concisely expresses the idea, but "better" here is subjective.

It’s oblique. Prettier and more elegant if you understand it, but no connotation of "converting type" unless you happen to know the idiom.

[Mark Lam]

Comment on attachment 405339 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=405339&action=review

>>>> Source/JavaScriptCore/jsc.cpp:477
>>>> +    static constexpr unsigned DontEnum = 0 | PropertyAttribute::DontEnum;
>>> 
>>> What is the purpose of the "0 |" here? Is it a way to convert an enumeration value to an integer? Why is it better than static_cast?
>> 
>> Yes, it's for converting the enumeration into an integer.  I chose it because it's less verbose than a static_cast<unsigned> cast.  It's better in my eyes because it concisely expresses the idea, but "better" here is subjective.
> 
> It’s oblique. Prettier and more elegant if you understand it, but no connotation of "converting type" unless you happen to know the idiom.

There are places where we set more than one attribute value using enums (e.g. PropertyAttribute::DontEnum | PropertyAttribute::DontDelete).  In those places, we also don't do the static_cast, and arguably, one would also have to understand the same idiom to understand why it works.  I think we should adopt this idiom.  Also, it's a very common idiom for "converting to integer" in JS code.  Given that this is JSC, the idiom should not be that foreign.

[EWS]

Committed r264991: <https://trac.webkit.org/changeset/264991>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 405339 [details].