Bug 165025

Summary: Fix exception scope verification failures in JSONObject.cpp.
Product: WebKit Reporter: Mark Lam <mark.lam>
Component: JavaScriptCoreAssignee: Mark Lam <mark.lam>
Status: RESOLVED FIXED    
Severity: Normal CC: fpizlo, ggaren, jfbastien, keith_miller, msaboff, saam, ysuzuki
Priority: P2    
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 162351    
Attachments:
Description Flags
proposed patch. saam: review+

Description Mark Lam 2016-11-21 22:53:42 PST 
Patch coming.
Comment 1 Mark Lam 2016-11-21 23:02:11 PST 
Created attachment 295328 [details]
proposed patch.
Comment 2 Saam Barati 2016-11-22 01:29:24 PST 
Comment on attachment 295328 [details]
proposed patch.

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

> Source/JavaScriptCore/runtime/JSONObject.cpp:233
> +    if (UNLIKELY(scope.exception()))

Why not RETURN_IF_EXCEPTION

> Source/JavaScriptCore/runtime/JSONObject.cpp:243
> +        if (UNLIKELY(scope.exception()))

Ditto

> Source/JavaScriptCore/runtime/JSONObject.cpp:278
> +    if ((stringifyResult != StringifySucceeded))

Style: Extra parens

> Source/JavaScriptCore/runtime/JSONObject.cpp:861
> +    auto throwScope = DECLARE_THROW_SCOPE(vm);

Could this be a catch scope instead?
Comment 3 Mark Lam 2016-11-22 11:15:17 PST 
Comment on attachment 295328 [details]
proposed patch.

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

>> Source/JavaScriptCore/runtime/JSONObject.cpp:233
>> +    if (UNLIKELY(scope.exception()))
> 
> Why not RETURN_IF_EXCEPTION

Because this is in a constructor, and constructors do not like "return void()".

>> Source/JavaScriptCore/runtime/JSONObject.cpp:243
>> +        if (UNLIKELY(scope.exception()))
> 
> Ditto

Same.  In a constructor.

>> Source/JavaScriptCore/runtime/JSONObject.cpp:278
>> +    if ((stringifyResult != StringifySucceeded))
> 
> Style: Extra parens

Missing an "UNLIKELY" before the inner ().  Will fix.

>> Source/JavaScriptCore/runtime/JSONObject.cpp:861
>> +    auto throwScope = DECLARE_THROW_SCOPE(vm);
> 
> Could this be a catch scope instead?

I'm not sure.  But at least one client of JSONStringify() (i.e. JSValueCreateJSONString) is expecting it to be able to throw an exception.  I'll leave it with a ThrowScope for now.
Comment 4 Mark Lam 2016-11-22 11:17:48 PST 
Thanks for the review.  Landed in r208966: <http://trac.webkit.org/r208966>.