Bug 145872 - WebCore::reportException() needs to be able to accept a raw thrown value in addition to Exception objects
Summary: WebCore::reportException() needs to be able to accept a raw thrown value in a...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-06-10 22:04 PDT by Mark Lam
Modified: 2015-06-11 22:41 PDT (History)
10 users (show)

See Also:


Attachments
the patch. (20.52 KB, patch)
2015-06-11 19:29 PDT, Mark Lam
msaboff: 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 2015-06-10 22:04:27 PDT
Patch coming.
Comment 1 Mark Lam 2015-06-10 22:04:54 PDT
<rdar://problem/21277462>
Comment 2 Mark Lam 2015-06-11 19:29:52 PDT
Created attachment 254777 [details]
the patch.
Comment 3 WebKit Commit Bot 2015-06-11 19:31:48 PDT
Attachment 254777 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/mac/WebViewDidCreateJavaScriptContext.mm:320:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Tools/TestWebKitAPI/Tests/mac/WebViewDidCreateJavaScriptContext.mm:324:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
Total errors found: 2 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Michael Saboff 2015-06-11 21:51:06 PDT
Comment on attachment 254777 [details]
the patch.

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

r=me

> Source/JavaScriptCore/ChangeLog:7
> +

Add to that m_lastException is providing the stack trace that VM::exceptionStackTrace() used to provide

> Source/WebCore/bindings/js/JSDOMBinding.cpp:190
>          exec->clearException();
> +        exec->clearLastException();

Aren't these redundant with the same calls on lines 164 & 165?
Comment 5 Mark Lam 2015-06-11 21:55:40 PDT
Comment on attachment 254777 [details]
the patch.

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

>> Source/JavaScriptCore/ChangeLog:7
>> +
> 
> Add to that m_lastException is providing the stack trace that VM::exceptionStackTrace() used to provide

Will do.

>> Source/WebCore/bindings/js/JSDOMBinding.cpp:190
>> +        exec->clearLastException();
> 
> Aren't these redundant with the same calls on lines 164 & 165?

These aren’t redundant.  They are needed in case the preceding toString() (called to make the errorMessage string) throws an exception.  reportException() is not supposed to cause a new exception to be thrown.  I will add a comment to document this.
Comment 6 Mark Lam 2015-06-11 22:41:23 PDT
Thanks for the review.  I also added more details in the JavaScriptCore ChangeLog on why we need VM::lastEcception().

Landed in r185487: <http://trac.webkit.org/r185487>.