164436 – Error description code should be able to handle Symbol values.

Bug 164436 - Error description code should be able to handle Symbol values.

Summary: Error description code should be able to handle Symbol values.

Status:	RESOLVED FIXED

Alias:	None

Product:	WebKit
Classification:	Unclassified
Component:	JavaScriptCore (show other bugs)
Version:	WebKit Local Build
Hardware:	Unspecified Unspecified

Importance:	P2 Normal
Assignee:	Mark Lam

URL:
Keywords:	InRadar

Depends on:
Blocks:

Reported:	2016-11-04 16:07 PDT by Mark Lam
Modified:	2016-11-15 16:13 PST (History)
CC List:	9 users (show)

See Also:

Attachments
proposed patch. (3.31 KB, patch) 2016-11-04 16:14 PDT, Mark Lam	fpizlo: review+	Details \| Formatted Diff \| Diff
patch for landing. (3.65 KB, patch) 2016-11-04 16:24 PDT, Mark Lam	no flags	Details \| Formatted Diff \| Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.

Description Mark Lam 2016-11-04 16:07:44 PDT

Currently, we try to toString() the Symbol value, resulting in it throwing an exception in errorDescriptionForValue() which breaks the invariant that errorDescriptionForValue() should not throw.

Comment 1 Mark Lam 2016-11-04 16:08:38 PDT

<rdar://problem/29115583>

Comment 2 Mark Lam 2016-11-04 16:14:41 PDT

Created attachment 293952 [details]
proposed patch.

Comment 3 Saam Barati 2016-11-04 16:16:54 PDT

Comment on attachment 293952 [details]
proposed patch.

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

> JSTests/stress/error-description-on-symbols-should-not-crash.js:4
> +} catch (e) {

What's the error message show here? Can you add a test that asserts.

Comment 4 Mark Lam 2016-11-04 16:17:51 PDT

(In reply to comment #3)
> Comment on attachment 293952 [details]
> proposed patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=293952&action=review
> 
> > JSTests/stress/error-description-on-symbols-should-not-crash.js:4
> > +} catch (e) {
> 
> What's the error message show here? Can you add a test that asserts.

That's a good idea.  I will update the test.

Comment 5 Mark Lam 2016-11-04 16:24:46 PDT

Created attachment 293957 [details]
patch for landing.

Comment 6 Mark Lam 2016-11-04 17:51:36 PDT

Thanks for the reviews.  Landed in r208410: <http://trac.webkit.org/r208410>.

Comment 7 Joseph Pecoraro 2016-11-14 17:45:11 PST

Comment on attachment 293957 [details]
patch for landing.

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

> JSTests/stress/error-description-on-symbols-should-not-crash.js:1
> +//@ runFTLNoCJIT

Any particular reason this is here? I'm wondering so I can know when I should add it in future tests myself.

Comment 8 Mark Lam 2016-11-15 16:13:25 PST

(In reply to comment #7)
> Comment on attachment 293957 [details]
> patch for landing.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=293957&action=review
> 
> > JSTests/stress/error-description-on-symbols-should-not-crash.js:1
> > +//@ runFTLNoCJIT
> 
> Any particular reason this is here? I'm wondering so I can know when I
> should add it in future tests myself.

"//@ runFTLNoCJIT" means only run with the 1 test configuration that has the FTL but does not use the ConcurrentJIT.  The reason I chose this is because this test does not really benefit from running multiple test configurations.  So, I only want to run it on one configuration.  runFTLNoCJIT is a good one to choose by default.