Bug 160154

Summary: various math operations don't properly check for an exception after calling toNumber() on the lhs
Product: WebKit Reporter: Saam Barati <saam>
Component: JavaScriptCoreAssignee: Saam Barati <saam>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, commit-queue, fpizlo, ggaren, gskachkov, keith_miller, mark.lam, msaboff, oliver, sukolsak, ticaiolima, ysuzuki
Priority: P2    
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch none

Description Saam Barati 2016-07-24 20:39:05 PDT 
...
Comment 1 Saam Barati 2016-08-05 16:16:24 PDT 
Created attachment 285460 [details]
patch
Comment 2 Mark Lam 2016-08-05 16:24:36 PDT 
Comment on attachment 285460 [details]
patch

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

r=me.

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:441
>      RETURN_WITH_PROFILING(result, {

Is it acceptable to update the ArithProfile here based on a wrong result if right.toNumber() throws?

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:456
>      RETURN_WITH_PROFILING(result, {

Ditto.
Comment 3 Saam Barati 2016-08-05 16:29:04 PDT 
Comment on attachment 285460 [details]
patch

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

>> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:441
>>      RETURN_WITH_PROFILING(result, {
> 
> Is it acceptable to update the ArithProfile here based on a wrong result if right.toNumber() throws?

It is always OK to update it. However, this code isn't actually doing that.
RETURN_WITH_PROFILING always does CHECK_EXCEPTION before updating the profile.
Comment 4 Saam Barati 2016-08-05 17:27:42 PDT 
Comment on attachment 285460 [details]
patch

Thanks for the review
Comment 5 WebKit Commit Bot 2016-08-05 17:48:13 PDT 
Comment on attachment 285460 [details]
patch

Clearing flags on attachment: 285460

Committed r204206: <http://trac.webkit.org/changeset/204206>
Comment 6 WebKit Commit Bot 2016-08-05 17:48:18 PDT 
All reviewed patches have been landed.  Closing bug.