Bug 98693 - After r130344, OpaqueJSString::identifier() adds wrapped String to identifier table
Summary: After r130344, OpaqueJSString::identifier() adds wrapped String to identifier...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Michael Saboff
URL:
Keywords: InRadar
Depends on:
Blocks: 98300
  Show dependency treegraph
 
Reported: 2012-10-08 15:03 PDT by Michael Saboff
Modified: 2012-10-09 11:38 PDT (History)
0 users

See Also:


Attachments
Patch (1.50 KB, patch)
2012-10-08 15:07 PDT, Michael Saboff
no flags Details | Formatted Diff | Diff
Added Radar reference to ChangeLog (1.60 KB, patch)
2012-10-08 17:09 PDT, Michael Saboff
mrowe: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Saboff 2012-10-08 15:03:46 PDT
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.
Comment 1 Michael Saboff 2012-10-08 15:07:44 PDT
Created attachment 167626 [details]
Patch
Comment 2 Mark Rowe (bdash) 2012-10-08 16:53:17 PDT
The ChangeLog and this Bugzilla should really contain references to the Radar that motivated this change.
Comment 3 Michael Saboff 2012-10-08 17:05:30 PDT
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.
Comment 4 Michael Saboff 2012-10-08 17:09:45 PDT
Created attachment 167651 [details]
Added Radar reference to ChangeLog
Comment 5 Michael Saboff 2012-10-09 07:46:48 PDT
Committed r130761: <http://trac.webkit.org/changeset/130761>
Comment 6 Darin Adler 2012-10-09 09:58:43 PDT
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?
Comment 7 Michael Saboff 2012-10-09 11:30:11 PDT
(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.