38112 2010-04-26 04:12:15 -0700 [Qt] QScriptValue::toString has a memory leak. 2010-05-04 06:54:13 -0700 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) All All RESOLVED FIXED Qt P2 Normal --- 31863 1 jedrzej.nowacki jedrzej.nowacki commit-queue eric jedrzej.nowacki kenneth kent.hansen oldest_to_newest 216995 0 jedrzej.nowacki 2010-04-26 04:12:15 -0700 ==11624== at 0x4024C4C: malloc (vg_replace_malloc.c:195) ==11624== by 0x40F98CB: WTF::fastMalloc(unsigned int) (FastMalloc.cpp:249) ==11624== by 0x4053ED7: WTF::FastAllocBase::operator new(unsigned int) (FastAllocBase.h:96) ==11624== by 0x406E59D: OpaqueJSString::create(JSC::UString const&) (OpaqueJSString.cpp:38) ==11624== by 0x406CB82: JSValueToStringCopy (JSValueRef.cpp:285) ==11624== by 0x405083D: QScriptValuePrivate::toString() const (qscriptvalue_p.h:460) ==11624== by 0x404EB57: QScriptValue::toString() const (qscriptvalue.cpp:333) 216998 1 54273 jedrzej.nowacki 2010-04-26 04:21:32 -0700 Created attachment 54273 Fix v1 217270 2 54273 eric 2010-04-26 14:11:57 -0700 Comment on attachment 54273 Fix v1 This looks OK, but I think it could be better. 1. We should probably have QScriptConverter handle this correctly. i.e. have a toString() method which takes a value() and context() directly, no? 2. Isn't there a RetainPtr to use here so that you never have to release the string explicitly? Then it would look something like this: RetainPtr<JSStringRef> string = JSValueToStringCopy(); return QScriptConverter::toString(string.get()); 220052 3 54912 jedrzej.nowacki 2010-05-03 03:45:19 -0700 Created attachment 54912 Fix v2 (In reply to comment #2) Thanks for review! > (From update of attachment 54273 [details]) > This looks OK, but I think it could be better. > > 1. We should probably have QScriptConverter handle this correctly. i.e. have > a toString() method which takes a value() and context() directly, no? I don't think that it is a good idea or at least not for now. - For simplicity, QtScript ignores exceptions, but it is a temporary solution, they will be exposed trough the QScriptEngine API. Methods of QScriptConverter are not good enough, for exception handling. Moreover for exception handling we will need QScriptEnginePrivate pointer, JSContextRef won't be sufficient, so it means that QScriptConverter couldn't be inlined inside QScriptEnginePrivate. - I think that context-less part of the QScriptConverter API should be moved to JSC C API as a generic JavaScript API (toArrayIndex(), toString(double)...) - For conversion, I like a function that takes argument and converts it without taking an additional factor (like context). But of course it is a matter taste. I'm going to reconsider/rethink QScriptConverter idea on exception handling implementation stage. > 2. Isn't there a RetainPtr to use here so that you never have to release the > string explicitly? Then it would look something like this: > > RetainPtr<JSStringRef> string = JSValueToStringCopy(); > return QScriptConverter::toString(string.get()); Good hint :-). I changed the path to use RetainPtr. 220571 4 54912 commit-queue 2010-05-04 06:54:07 -0700 Comment on attachment 54912 Fix v2 Clearing flags on attachment: 54912 Committed r58755: <http://trac.webkit.org/changeset/58755> 220572 5 commit-queue 2010-05-04 06:54:13 -0700 All reviewed patches have been landed. Closing bug. 54273 2010-04-26 04:21:32 -0700 2010-05-03 03:45:19 -0700 Fix v1 tostring.diff text/plain 1529 jedrzej.nowacki ZGlmZiAtLWdpdCBhL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZyBiL0phdmFTY3JpcHRDb3JlL0No YW5nZUxvZwppbmRleCBlNmFmOTY0Li5jMGJiYTJmIDEwMDY0NAotLS0gYS9KYXZhU2NyaXB0Q29y ZS9DaGFuZ2VMb2cKKysrIGIvSmF2YVNjcmlwdENvcmUvQ2hhbmdlTG9nCkBAIC0yLDYgKzIsMjAg QEAKIAogICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KIAorICAgICAgICBGaXgg YSBtZW1vcnkgbGVhayBpbnNpZGUgdGhlIFFTY3JpcHRWYWx1ZS4KKworICAgICAgICBRU2NpcHRW YWx1ZVByaXZhdGU6OnRvU3RyaW5nIHNob3VsZCByZWxlYXNlIGFsbCB0ZW1wb3JhcnkgdmFyaWFi bGVzLgorCisgICAgICAgIFtRdF0gUVNjcmlwdFZhbHVlOjp0b1N0cmluZyBoYXZlIGEgbWVtb3J5 IGxlYWsuCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0z ODExMgorCisgICAgICAgICogcXQvYXBpL3FzY3JpcHR2YWx1ZV9wLmg6CisgICAgICAgIChRU2Ny aXB0VmFsdWVQcml2YXRlOjp0b1N0cmluZyk6CisKKzIwMTAtMDQtMjYgIEplZHJ6ZWogTm93YWNr aSAgPGplZHJ6ZWoubm93YWNraUBub2tpYS5jb20+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9C T0RZIChPT1BTISkuCisKICAgICAgICAgRml4IGEgbWVtb3J5IGxlYWsgaW4gdGhlIFFTY3JpcHRT dHJpbmcuCiAKICAgICAgICAgUVNjcmlwdFN0cmluZ1ByaXZhdGUncyBjb25zdHJ1Y3RvciBzaG91 bGRuJ3QgY2FsbCBKU1N0cmluZ1JldGFpbiBhcwpkaWZmIC0tZ2l0IGEvSmF2YVNjcmlwdENvcmUv cXQvYXBpL3FzY3JpcHR2YWx1ZV9wLmggYi9KYXZhU2NyaXB0Q29yZS9xdC9hcGkvcXNjcmlwdHZh bHVlX3AuaAppbmRleCA4MzBiMzhlLi41YTFlYjAwIDEwMDY0NAotLS0gYS9KYXZhU2NyaXB0Q29y ZS9xdC9hcGkvcXNjcmlwdHZhbHVlX3AuaAorKysgYi9KYXZhU2NyaXB0Q29yZS9xdC9hcGkvcXNj cmlwdHZhbHVlX3AuaApAQCAtNDU3LDcgKzQ1NywxMCBAQCBRU3RyaW5nIFFTY3JpcHRWYWx1ZVBy aXZhdGU6OnRvU3RyaW5nKCkgY29uc3QKICAgICBjYXNlIEpTVmFsdWU6CiAgICAgY2FzZSBKU1By aW1pdGl2ZToKICAgICBjYXNlIEpTT2JqZWN0OgotICAgICAgICByZXR1cm4gUVNjcmlwdENvbnZl cnRlcjo6dG9TdHJpbmcoSlNWYWx1ZVRvU3RyaW5nQ29weShjb250ZXh0KCksIHZhbHVlKCksIC8q IGV4Y2VwdGlvbiAqLyAwKSk7CisgICAgICAgIEpTU3RyaW5nUmVmIHRtcCA9IEpTVmFsdWVUb1N0 cmluZ0NvcHkoY29udGV4dCgpLCB2YWx1ZSgpLCAvKiBleGNlcHRpb24gKi8gMCk7CisgICAgICAg IFFTdHJpbmcgcmVzdWx0ID0gUVNjcmlwdENvbnZlcnRlcjo6dG9TdHJpbmcodG1wKTsKKyAgICAg ICAgSlNTdHJpbmdSZWxlYXNlKHRtcCk7CisgICAgICAgIHJldHVybiByZXN1bHQ7CiAgICAgfQog CiAgICAgUV9BU1NFUlRfWChmYWxzZSwgInRvU3RyaW5nKCkiLCAiTm90IGFsbCBzdGF0ZXMgYXJl IGluY2x1ZGVkIGluIHRoZSBwcmV2aW91cyBzd2l0Y2ggc3RhdGVtZW50LiIpOwo= 54912 2010-05-03 03:45:19 -0700 2010-05-04 06:54:07 -0700 Fix v2 leak.diff text/plain 1734 jedrzej.nowacki ZGlmZiAtLWdpdCBhL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZyBiL0phdmFTY3JpcHRDb3JlL0No YW5nZUxvZwppbmRleCBlNmFmOTY0Li5jMGJiYTJmIDEwMDY0NAotLS0gYS9KYXZhU2NyaXB0Q29y ZS9DaGFuZ2VMb2cKKysrIGIvSmF2YVNjcmlwdENvcmUvQ2hhbmdlTG9nCkBAIC0yLDYgKzIsMjAg QEAKIAogICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KIAorICAgICAgICBGaXgg YSBtZW1vcnkgbGVhayBpbnNpZGUgdGhlIFFTY3JpcHRWYWx1ZS4KKworICAgICAgICBRU2NpcHRW YWx1ZVByaXZhdGU6OnRvU3RyaW5nIHNob3VsZCByZWxlYXNlIGFsbCB0ZW1wb3JhcnkgdmFyaWFi bGVzLgorCisgICAgICAgIFtRdF0gUVNjcmlwdFZhbHVlOjp0b1N0cmluZyBoYXZlIGEgbWVtb3J5 IGxlYWsuCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0z ODExMgorCisgICAgICAgICogcXQvYXBpL3FzY3JpcHR2YWx1ZV9wLmg6CisgICAgICAgIChRU2Ny aXB0VmFsdWVQcml2YXRlOjp0b1N0cmluZyk6CisKKzIwMTAtMDQtMjYgIEplZHJ6ZWogTm93YWNr aSAgPGplZHJ6ZWoubm93YWNraUBub2tpYS5jb20+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9C T0RZIChPT1BTISkuCisKICAgICAgICAgRml4IGEgbWVtb3J5IGxlYWsgaW4gdGhlIFFTY3JpcHRT dHJpbmcuCiAKICAgICAgICAgUVNjcmlwdFN0cmluZ1ByaXZhdGUncyBjb25zdHJ1Y3RvciBzaG91 bGRuJ3QgY2FsbCBKU1N0cmluZ1JldGFpbiBhcwpkaWZmIC0tZ2l0IGEvSmF2YVNjcmlwdENvcmUv cXQvYXBpL3FzY3JpcHR2YWx1ZV9wLmggYi9KYXZhU2NyaXB0Q29yZS9xdC9hcGkvcXNjcmlwdHZh bHVlX3AuaAppbmRleCA4MzBiMzhlLi5kM2Y4ZTFmIDEwMDY0NAotLS0gYS9KYXZhU2NyaXB0Q29y ZS9xdC9hcGkvcXNjcmlwdHZhbHVlX3AuaAorKysgYi9KYXZhU2NyaXB0Q29yZS9xdC9hcGkvcXNj cmlwdHZhbHVlX3AuaApAQCAtMjQsNiArMjQsNyBAQAogI2luY2x1ZGUgInFzY3JpcHRlbmdpbmVf cC5oIgogI2luY2x1ZGUgInFzY3JpcHR2YWx1ZS5oIgogI2luY2x1ZGUgPEphdmFTY3JpcHRDb3Jl L0phdmFTY3JpcHQuaD4KKyNpbmNsdWRlIDxKYXZhU2NyaXB0Q29yZS9KU1JldGFpblB0ci5oPgog I2luY2x1ZGUgPFF0Q29yZS9xbWF0aC5oPgogI2luY2x1ZGUgPFF0Q29yZS9xbnVtZXJpYy5oPgog I2luY2x1ZGUgPFF0Q29yZS9xc2hhcmVkZGF0YS5oPgpAQCAtNDU3LDcgKzQ1OCw4IEBAIFFTdHJp bmcgUVNjcmlwdFZhbHVlUHJpdmF0ZTo6dG9TdHJpbmcoKSBjb25zdAogICAgIGNhc2UgSlNWYWx1 ZToKICAgICBjYXNlIEpTUHJpbWl0aXZlOgogICAgIGNhc2UgSlNPYmplY3Q6Ci0gICAgICAgIHJl dHVybiBRU2NyaXB0Q29udmVydGVyOjp0b1N0cmluZyhKU1ZhbHVlVG9TdHJpbmdDb3B5KGNvbnRl eHQoKSwgdmFsdWUoKSwgLyogZXhjZXB0aW9uICovIDApKTsKKyAgICAgICAgSlNSZXRhaW5QdHI8 SlNTdHJpbmdSZWY+IHB0cihBZG9wdCwgSlNWYWx1ZVRvU3RyaW5nQ29weShjb250ZXh0KCksIHZh bHVlKCksIC8qIGV4Y2VwdGlvbiAqLyAwKSk7CisgICAgICAgIHJldHVybiBRU2NyaXB0Q29udmVy dGVyOjp0b1N0cmluZyhwdHIuZ2V0KCkpOwogICAgIH0KIAogICAgIFFfQVNTRVJUX1goZmFsc2Us ICJ0b1N0cmluZygpIiwgIk5vdCBhbGwgc3RhdGVzIGFyZSBpbmNsdWRlZCBpbiB0aGUgcHJldmlv dXMgc3dpdGNoIHN0YXRlbWVudC4iKTsK