Bug 223231 – [WebIDL] Fix convertRecord() to throw on enumerable symbol |key|

• Component: Bindings
• Status: RESOLVED FIXED

[Alexey Shvayka]

[WebIDL] Fix convertRecord() to throw on enumerable symbol |key|

[Alexey Shvayka]

Created attachment 423276 [details]
Patch

[Darin Adler]

Comment on attachment 423276 [details]
Patch

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

> Source/WebCore/bindings/js/JSDOMConvertStrings.cpp:38
> +    VM& vm = lexicalGlobalObject.vm();
> +    auto scope = DECLARE_THROW_SCOPE(vm);

Is there any performance benefit to moving this inside the if statement?

> Source/WebCore/bindings/js/JSDOMConvertStrings.cpp:103
> +    if (!string)
> +        return { };

This seems like an unimportant optimization. I would leave it out; the stringToUSVString function can handle the null string without causing any problems. Then this can just be a one-liner. Also, we generally write string.isNull() instead of !string. Not sure why.

    return stringToUSVString(identifierToString(lexicalGlobalObject, identifier));

[Alexey Shvayka]

Created attachment 423404 [details]
Patch

Add UNLIKELY, move DECLARE_THROW_SCOPE into branch, and make identifierToUSVString() a one-liner.

[Alexey Shvayka]

(In reply to Darin Adler from comment #2)

Thank you for reviewing convertRecord() changes!

> Is there any performance benefit to moving this inside the if statement?

I've found a few precedents of ThrowScope() created inside a branch, in hot inlined methods.
This won't break validation of exception checks.

Also, added an UNLIKELY, which is merely a compiler hint on how to emits jumps so common case is optimized.

> This seems like an unimportant optimization. I would leave it out; the
> stringToUSVString function can handle the null string without causing any
> problems. Then this can just be a one-liner. Also, we generally write
> string.isNull() instead of !string. Not sure why.
> 
>     return stringToUSVString(identifierToString(lexicalGlobalObject,
> identifier));

Nice, fixed.

[EWS]

Committed r274561: <https://commits.webkit.org/r274561>

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

[Radar WebKit Bug Importer]

<rdar://problem/75528251>