Bug 98693 – After r130344, OpaqueJSString::identifier() adds wrapped String to identifier table

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Michael Saboff]

The update in r130344 to OpaqueJSString::identifier() creates an Identifier using m_string which will add m_string to the identifier table of the passed in JSGlobalData.  This unintended side effect is incompatible with OpaqueJSString being a thread safe string abstraction.

The fix is to create a new string for the Identifier by using one of the character pointer / length based constructors.

[Michael Saboff]

Created attachment 167626 [details]
Patch

[Mark Rowe (bdash)]

The ChangeLog and this Bugzilla should really contain references to the Radar that motivated this change.

[Michael Saboff]

In Radar as <rdar://problem/12450118> (and <rdar://problem/12449570> which has been made a duplicate).

Tested this fix against <rdar://problem/12449570> and it fixes the problem.

[Michael Saboff]

Created attachment 167651 [details]
Added Radar reference to ChangeLog

[Michael Saboff]

Committed r130761: <http://trac.webkit.org/changeset/130761>

[Darin Adler]

Comment on attachment 167651 [details]
Added Radar reference to ChangeLog

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

> Source/JavaScriptCore/ChangeLog:11
> +        Use Identifier(LChar*, length) or Identifier(UChar*, length) constructors so that we don't
> +        add the String instance in the OpaqueJSString to any identifier tables.

This doesn’t seem right architecturally. What prevents callers from doing this by calling string() and then turning it into an Identifier later?

[Michael Saboff]

(In reply to comment #6)
> (From update of attachment 167651 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167651&action=review
> 
> > Source/JavaScriptCore/ChangeLog:11
> > +        Use Identifier(LChar*, length) or Identifier(UChar*, length) constructors so that we don't
> > +        add the String instance in the OpaqueJSString to any identifier tables.
> 
> This doesn’t seem right architecturally. What prevents callers from doing this by calling string() and then turning it into an Identifier later?

Created https://bugs.webkit.org/show_bug.cgi?id=98801 "After r130344, OpaqueJSString::string() shouldn't directly return the wrapped String" to track this.

Probably the best way to handle this is to create a copy of the wrapped String and return it.  This will make the code prior to 130344 except we now use a String instead of a UChar[] to store the text.