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

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

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.

[Mark Lam]

<rdar://problem/29115583>

[Mark Lam]

Created attachment 293952 [details]
proposed patch.

[Saam Barati]

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.

[Mark Lam]

(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.

[Mark Lam]

Created attachment 293957 [details]
patch for landing.

[Mark Lam]

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

[Joseph Pecoraro]

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.

[Mark Lam]

(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.