98693 2012-10-08 15:03:46 -0700 After r130344, OpaqueJSString::identifier() adds wrapped String to identifier table 2012-10-09 11:38:07 -0700 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) All All RESOLVED FIXED InRadar P2 Normal --- 98300 1 msaboff msaboff oldest_to_newest 737094 0 msaboff 2012-10-08 15:03:46 -0700 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. 737103 1 167626 msaboff 2012-10-08 15:07:44 -0700 Created attachment 167626 Patch 737227 2 mrowe 2012-10-08 16:53:17 -0700 The ChangeLog and this Bugzilla should really contain references to the Radar that motivated this change. 737240 3 msaboff 2012-10-08 17:05:30 -0700 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. 737245 4 167651 msaboff 2012-10-08 17:09:45 -0700 Created attachment 167651 Added Radar reference to ChangeLog 737771 5 msaboff 2012-10-09 07:46:48 -0700 Committed r130761: <http://trac.webkit.org/changeset/130761> 737891 6 167651 darin 2012-10-09 09:58:43 -0700 Comment on attachment 167651 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? 737977 7 msaboff 2012-10-09 11:30:11 -0700 (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. 167626 2012-10-08 15:07:44 -0700 2012-10-08 17:09:45 -0700 Patch 98693.patch text/plain 1535 msaboff SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMTMwNjg5KQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE2IEBA CisyMDEyLTEwLTA4ICBNaWNoYWVsIFNhYm9mZiAgPG1zYWJvZmZAYXBwbGUuY29tPgorCisgICAg ICAgIEFmdGVyIHIxMzAzNDQsIE9wYXF1ZUpTU3RyaW5nOjppZGVudGlmaWVyKCkgYWRkcyB3cmFw cGVkIFN0cmluZyB0byBpZGVudGlmaWVyIHRhYmxlCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJr aXQub3JnL3Nob3dfYnVnLmNnaT9pZD05ODY5MworCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9E WSAoT09QUyEpLgorCisgICAgICAgIFVzZSBJZGVudGlmaWVyKExDaGFyKiwgbGVuZ3RoKSBvciBJ ZGVudGlmaWVyKFVDaGFyKiwgbGVuZ3RoKSBjb25zdHJ1Y3RvcnMgc28gdGhhdCB3ZSBkb24ndAor ICAgICAgICBhZGQgdGhlIFN0cmluZyBpbnN0YW5jZSBpbiB0aGUgT3BhcXVlSlNTdHJpbmcgdG8g YW55IGlkZW50aWZpZXIgdGFibGVzLgorCisgICAgICAgICogQVBJL09wYXF1ZUpTU3RyaW5nLmNw cDoKKyAgICAgICAgKE9wYXF1ZUpTU3RyaW5nOjppZGVudGlmaWVyKToKKwogMjAxMi0xMC0wOCAg TWljaGFlbCBTYWJvZmYgIDxtc2Fib2ZmQGFwcGxlLmNvbT4KIAogICAgICAgICBBZnRlciByMTMw MzQ0LCBPcGFxdWVKU1N0cmluZygpIGNyZWF0ZXMgYW4gZW1wdHkgc3RyaW5nIHdoaWNoIHNob3Vs ZCBiZSBhIG51bGwgc3RyaW5nCkluZGV4OiBTb3VyY2UvSmF2YVNjcmlwdENvcmUvQVBJL09wYXF1 ZUpTU3RyaW5nLmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvSmF2YVNjcmlwdENvcmUvQVBJL09w YXF1ZUpTU3RyaW5nLmNwcAkocmV2aXNpb24gMTMwNjY0KQorKysgU291cmNlL0phdmFTY3JpcHRD b3JlL0FQSS9PcGFxdWVKU1N0cmluZy5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTQ0LDUgKzQ0LDgg QEAgSWRlbnRpZmllciBPcGFxdWVKU1N0cmluZzo6aWRlbnRpZmllcihKUwogICAgIGlmICghdGhp cyB8fCAhbV9zdHJpbmcubGVuZ3RoKCkpCiAgICAgICAgIHJldHVybiBJZGVudGlmaWVyKGdsb2Jh bERhdGEsIHN0YXRpY19jYXN0PGNvbnN0IGNoYXIqPigwKSk7CiAKLSAgICByZXR1cm4gSWRlbnRp ZmllcihnbG9iYWxEYXRhLCBtX3N0cmluZyk7CisgICAgaWYgKG1fc3RyaW5nLmlzOEJpdCgpKQor ICAgICAgICByZXR1cm4gSWRlbnRpZmllcihnbG9iYWxEYXRhLCBtX3N0cmluZy5jaGFyYWN0ZXJz OCgpLCBtX3N0cmluZy5sZW5ndGgoKSk7CisKKyAgICByZXR1cm4gSWRlbnRpZmllcihnbG9iYWxE YXRhLCBtX3N0cmluZy5jaGFyYWN0ZXJzMTYoKSwgbV9zdHJpbmcubGVuZ3RoKCkpOwogfQo= 167651 2012-10-08 17:09:45 -0700 2012-10-09 09:58:43 -0700 Added Radar reference to ChangeLog 98693-2.patch text/plain 1639 msaboff SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMTMwNjg5KQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE4IEBA CisyMDEyLTEwLTA4ICBNaWNoYWVsIFNhYm9mZiAgPG1zYWJvZmZAYXBwbGUuY29tPgorCisgICAg ICAgIEFmdGVyIHIxMzAzNDQsIE9wYXF1ZUpTU3RyaW5nOjppZGVudGlmaWVyKCkgYWRkcyB3cmFw cGVkIFN0cmluZyB0byBpZGVudGlmaWVyIHRhYmxlCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJr aXQub3JnL3Nob3dfYnVnLmNnaT9pZD05ODY5MworICAgICAgICBSRUdSRVNTSU9OIChyMTMwMzQ0 KTogSW5zdGFsbCBmYWlsZWQgaW4gSW5zdGFsbCBFbnZpcm9ubWVudAorICAgICAgICA8cmRhcjov L3Byb2JsZW0vMTI0NTAxMTg+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISku CisKKyAgICAgICAgVXNlIElkZW50aWZpZXIoTENoYXIqLCBsZW5ndGgpIG9yIElkZW50aWZpZXIo VUNoYXIqLCBsZW5ndGgpIGNvbnN0cnVjdG9ycyBzbyB0aGF0IHdlIGRvbid0CisgICAgICAgIGFk ZCB0aGUgU3RyaW5nIGluc3RhbmNlIGluIHRoZSBPcGFxdWVKU1N0cmluZyB0byBhbnkgaWRlbnRp ZmllciB0YWJsZXMuCisKKyAgICAgICAgKiBBUEkvT3BhcXVlSlNTdHJpbmcuY3BwOgorICAgICAg ICAoT3BhcXVlSlNTdHJpbmc6OmlkZW50aWZpZXIpOgorCiAyMDEyLTEwLTA4ICBNaWNoYWVsIFNh Ym9mZiAgPG1zYWJvZmZAYXBwbGUuY29tPgogCiAgICAgICAgIEFmdGVyIHIxMzAzNDQsIE9wYXF1 ZUpTU3RyaW5nKCkgY3JlYXRlcyBhbiBlbXB0eSBzdHJpbmcgd2hpY2ggc2hvdWxkIGJlIGEgbnVs bCBzdHJpbmcKSW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9BUEkvT3BhcXVlSlNTdHJpbmcu Y3BwCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9BUEkvT3BhcXVlSlNTdHJp bmcuY3BwCShyZXZpc2lvbiAxMzA2NjQpCisrKyBTb3VyY2UvSmF2YVNjcmlwdENvcmUvQVBJL09w YXF1ZUpTU3RyaW5nLmNwcAkod29ya2luZyBjb3B5KQpAQCAtNDQsNSArNDQsOCBAQCBJZGVudGlm aWVyIE9wYXF1ZUpTU3RyaW5nOjppZGVudGlmaWVyKEpTCiAgICAgaWYgKCF0aGlzIHx8ICFtX3N0 cmluZy5sZW5ndGgoKSkKICAgICAgICAgcmV0dXJuIElkZW50aWZpZXIoZ2xvYmFsRGF0YSwgc3Rh dGljX2Nhc3Q8Y29uc3QgY2hhcio+KDApKTsKIAotICAgIHJldHVybiBJZGVudGlmaWVyKGdsb2Jh bERhdGEsIG1fc3RyaW5nKTsKKyAgICBpZiAobV9zdHJpbmcuaXM4Qml0KCkpCisgICAgICAgIHJl dHVybiBJZGVudGlmaWVyKGdsb2JhbERhdGEsIG1fc3RyaW5nLmNoYXJhY3RlcnM4KCksIG1fc3Ry aW5nLmxlbmd0aCgpKTsKKworICAgIHJldHVybiBJZGVudGlmaWVyKGdsb2JhbERhdGEsIG1fc3Ry aW5nLmNoYXJhY3RlcnMxNigpLCBtX3N0cmluZy5sZW5ndGgoKSk7CiB9Cg==