Bug 224215

Summary: Array's toString() is incorrect if join() is non-callable
Product: WebKit Reporter: Alexey Shvayka <ashvayka>
Component: JavaScriptCoreAssignee: Alexey Shvayka <ashvayka>
Status: RESOLVED FIXED    
Severity: Trivial CC: darin, ews-watchlist, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch ysuzuki: review+, ews-feeder: commit-queue-

Description Alexey Shvayka 2021-04-05 16:38:36 PDT
Array's toString() is incorrect if join() is non-callable
Comment 1 Alexey Shvayka 2021-04-05 17:13:06 PDT
Created attachment 425225 [details]
Patch
Comment 2 Yusuke Suzuki 2021-04-05 17:36:51 PDT
Comment on attachment 425225 [details]
Patch

r=me
Comment 3 Alexey Shvayka 2021-04-06 13:20:47 PDT
Committed r275544 (236200@main): <https://commits.webkit.org/236200@main>
Comment 4 Radar WebKit Bug Importer 2021-04-06 13:21:42 PDT
<rdar://problem/76281426>
Comment 5 Darin Adler 2021-04-06 14:41:23 PDT
Comment on attachment 425225 [details]
Patch

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

> Source/JavaScriptCore/runtime/ObjectPrototype.cpp:322
> +    if (thisValue.isUndefined())
> +        return vm.smallStrings.undefinedObjectString();
> +    if (thisValue.isNull())
> +        return vm.smallStrings.nullObjectString();

Isn’t it an error to return after invoking DECLARE_THROW_SCOPE without using RELEASE_AND_RETURN? Must admit I am confused about the ThrowScope idioms.
Comment 6 Mark Lam 2021-04-06 15:03:41 PDT
Comment on attachment 425225 [details]
Patch

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

>> Source/JavaScriptCore/runtime/ObjectPrototype.cpp:322
>> +        return vm.smallStrings.nullObjectString();
> 
> Isn’t it an error to return after invoking DECLARE_THROW_SCOPE without using RELEASE_AND_RETURN? Must admit I am confused about the ThrowScope idioms.

It's not if the statement you evaluate here cannot throw.  The "release" in RELEASE_AND_RETURN means that this ThrowScope is releasing the responsibility to check for any exceptions from this point on till the return, and letting the caller handle it.  This makes sense if the last statement we evaluate can throw.  If you don't release the responsibility, and you don't check for an exception, but the statement can throw, then that's an unhandled exception because you still have the responsibility and you didn't do the check when one is needed.

But if the statement cannot throw, then all is well because no check is needed after all before we return though we still have the responsibility.