Bug 165025 - Fix exception scope verification failures in JSONObject.cpp.
Summary: Fix exception scope verification failures in JSONObject.cpp.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords:
Depends on:
Blocks: 162351
  Show dependency treegraph
 
Reported: 2016-11-21 22:53 PST by Mark Lam
Modified: 2016-11-22 11:17 PST (History)
7 users (show)

See Also:


Attachments
proposed patch. (11.81 KB, patch)
2016-11-21 23:02 PST, Mark Lam
sbarati: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>.