101867 2012-11-11 14:22:38 -0800 Crash in conversion of empty OpaqueJSString to Identifier 2012-11-30 08:59:58 -0800 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 103747 1 allan.jensen allan.jensen ggaren hausmann jturcotte msaboff webkit-ews webkit.review.bot oldest_to_newest 763884 0 allan.jensen 2012-11-11 14:22:38 -0800 KWallets integration QtWebKit causes crashes on entering a number of webpages. The backtrace goes through convertValueToQVariant and in a normal backtrace looks like it ends in OpaqueJSString::identifier. When compiled with more debugging info though, the crash does occur somewhat deeper but is still triggered by what happens in OpaqueJSString. The problem seems to be the construction of an Identifier containing a null-string, which is invalid. 763887 1 173518 allan.jensen 2012-11-11 14:31:52 -0800 Created attachment 173518 Patch 764276 2 173518 hausmann 2012-11-12 04:53:27 -0800 Comment on attachment 173518 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=173518&action=review > Source/JavaScriptCore/API/OpaqueJSString.cpp:54 > if (!this || !m_string.length()) > - return Identifier(globalData, static_cast<const char*>(0)); > + return Identifier(Identifier::EmptyIdentifier); I don't know this code well enough to feel comfortable reviewing the patch. It seems quite evil, but then again, this function is already checking for !this, so maybe this is okay. 764470 3 msaboff 2012-11-12 09:03:30 -0800 (In reply to comment #0) > KWallets integration QtWebKit causes crashes on entering a number of webpages. > > The backtrace goes through convertValueToQVariant and in a normal backtrace looks like it ends in OpaqueJSString::identifier. When compiled with more debugging info though, the crash does occur somewhat deeper but is still triggered by what happens in OpaqueJSString. > > The problem seems to be the construction of an Identifier containing a null-string, which is invalid. I think the change is fine, but could you provide a detailed stack trace? Want to see if you should be getting to this place with a null OpaqueJSString in the first place. 764538 4 allan.jensen 2012-11-12 10:13:06 -0800 (In reply to comment #3) > (In reply to comment #0) > > KWallets integration QtWebKit causes crashes on entering a number of webpages. > > > > The backtrace goes through convertValueToQVariant and in a normal backtrace looks like it ends in OpaqueJSString::identifier. When compiled with more debugging info though, the crash does occur somewhat deeper but is still triggered by what happens in OpaqueJSString. > > > > The problem seems to be the construction of an Identifier containing a null-string, which is invalid. > > I think the change is fine, but could you provide a detailed stack trace? Want to see if you should be getting to this place with a null OpaqueJSString in the first place. I don't think it is null, it is only empty. Note the check is "!m_string.length()". 764547 5 173518 ggaren 2012-11-12 10:23:34 -0800 Comment on attachment 173518 Patch The old code tried to create a null string. The new code tries to create an empty string. This seems like a change in behavior. Why should we change fundamental low-level behavior that has been this way for four years because of a crash in the Qt port? 764569 6 allan.jensen 2012-11-12 10:55:29 -0800 (In reply to comment #5) > (From update of attachment 173518 [details]) > The old code tried to create a null string. The new code tries to create an empty string. This seems like a change in behavior. > > Why should we change fundamental low-level behavior that has been this way for four years because of a crash in the Qt port? Because the code is wrong? 764584 7 allan.jensen 2012-11-12 11:01:45 -0800 I admit this bug report is too low in information for how low-level the change is. I will provide a detailed backtrace tomorrow, which should help to establish how general the problem is. 764726 8 ggaren 2012-11-12 13:10:54 -0800 > Because the code is wrong? Identifier does support null strings. Why did you choose to use the empty string instead of the null string? Are you aware that there's a difference between the empty string and the null string? 764779 9 allan.jensen 2012-11-12 13:50:15 -0800 (In reply to comment #8) > > Because the code is wrong? > > Identifier does support null strings. Why did you choose to use the empty string instead of the null string? Are you aware that there's a difference between the empty string and the null string? Calling Identifier(GlobalData*, const char*) will because there is no such direct constructor end up calling Identifier(GlobalData*, String&) which calls Identifier(GlobalData*, StringImpl*) which calls Identifier::addSlowCase(GlobalData, StringImpl* r) which contains the assertion ASSERT(r->length()); and finally crashing when the null-string is added to the identifier table. I returned the empty string identifier because that is what is generated for both null and empty strings input by all of the constructor except the default constructor and the StringImpl* constructor which asserts against it. Since the case I was running into was an empty string turning into a null-string identifier. It could also be solved by converting null strings to null string identifiers and empty strings to empty string identifiers. But converting an empty-string to an invalid null string that is asserted against and is not good, even if it has been broken for 4 years. 764965 10 ggaren 2012-11-12 16:40:16 -0800 > It could also be solved by converting null strings to null string identifiers and empty strings to empty string identifiers. Indeed. Why did you decide against that? 766534 11 allan.jensen 2012-11-14 00:05:37 -0800 (In reply to comment #10) > > It could also be solved by converting null strings to null string identifiers and empty strings to empty string identifiers. > > Indeed. Why did you decide against that? Because the other constructors calls the one of add() functions which return an empty-string identifier for all strings of length 0. Since only the default constructor returned a null string identifier it lookslike a pattern where the null-string identifer is only meant to represent an uninitialized identifier, which makes sense if identifiers are mainly used as keys in tables where you need to be able to recognize defined from undefined entries. Under such a pattern a function returning an identifier should always return a defined identifer, so non-null. Of course if I am wrong about the pattern for Identifiers I expected developers who know better to correct me. 767855 12 174383 allan.jensen 2012-11-15 02:39:23 -0800 Created attachment 174383 Patch Rename bug and split empty and null string handling 767865 13 174383 eflews.bot 2012-11-15 02:46:03 -0800 Comment on attachment 174383 Patch Attachment 174383 did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14845403 767866 14 174383 webkit-ews 2012-11-15 02:46:49 -0800 Comment on attachment 174383 Patch Attachment 174383 did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14826987 767867 15 174383 webkit-ews 2012-11-15 02:47:05 -0800 Comment on attachment 174383 Patch Attachment 174383 did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14857158 767884 16 174387 allan.jensen 2012-11-15 03:00:51 -0800 Created attachment 174387 Patch 772848 17 jturcotte 2012-11-21 05:49:08 -0800 Blocking the release since this crashes the fancybrowser example. 780151 18 174387 webkit.review.bot 2012-11-30 08:59:54 -0800 Comment on attachment 174387 Patch Clearing flags on attachment: 174387 Committed r136246: <http://trac.webkit.org/changeset/136246> 780152 19 webkit.review.bot 2012-11-30 08:59:58 -0800 All reviewed patches have been landed. Closing bug. 173518 2012-11-11 14:31:52 -0800 2012-11-15 02:39:20 -0800 Patch bug-101867-20121111232955.patch text/plain 1543 allan.jensen U3VidmVyc2lvbiBSZXZpc2lvbjogMTM0MTc0CmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvQ2hhbmdlTG9nIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwppbmRleCAy OTZjNzNhOGY4ZTBmZWYwY2FlY2FiMWM4YTE2NDA2MDc5MjU3MGY0Li5iZGFjNzliYzIzMWRhMWY2 M2ViZjY5MzkyMGM4N2Q4OGEyZTBmZjU4IDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwxNSBAQAorMjAxMi0xMS0xMSAgQWxsYW4gU2FuZGZlbGQgSmVuc2VuICA8YWxsYW4uamVu c2VuQGRpZ2lhLmNvbT4KKworICAgICAgICBDcmFzaCBkdXJpbmcgY29udmVyc2lvbiBvZiBlbGVt ZW50IHRvIFFWYXJpYW50CisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVn LmNnaT9pZD0xMDE4NjcKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKwor ICAgICAgICBSZXR1cm4gYSB2YWxpZCBlbXB0eS1zdHJpbmcgaWRlbnRpZmllciBpbnN0ZWFkIG9m IHRyeWluZyBidWlsZCBvbmUgb3V0IG9mIGEgbnVsbCBwb2ludGVyLgorCisgICAgICAgICogQVBJ L09wYXF1ZUpTU3RyaW5nLmNwcDoKKyAgICAgICAgKE9wYXF1ZUpTU3RyaW5nOjppZGVudGlmaWVy KToKKwogMjAxMi0xMS0xMSAgS2VuaWNoaSBJc2hpYmFzaGkgIDxiYXNoaUBjaHJvbWl1bS5vcmc+ CiAKICAgICAgICAgV1RGU3RyaW5nOjp1dGY4KCkgc2hvdWxkIGhhdmUgYSBtb2RlIG9mIGNvbnZl cnNpb24gdG8gdXNlIHJlcGxhY2VtZW50IGNoYXJhY3RlcgpkaWZmIC0tZ2l0IGEvU291cmNlL0ph dmFTY3JpcHRDb3JlL0FQSS9PcGFxdWVKU1N0cmluZy5jcHAgYi9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvQVBJL09wYXF1ZUpTU3RyaW5nLmNwcAppbmRleCBhYzdjZmQxNmE0ZWEyNDkwM2YzODQyYzRj NzFiM2FlOGQ1NjI2Mzc4Li5iNDE3MzkxNzdlYTQ1YWZmODk1OTMzMjcxNGE3NjU2OGJlNWMyYWQz IDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvQVBJL09wYXF1ZUpTU3RyaW5nLmNw cAorKysgYi9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvQVBJL09wYXF1ZUpTU3RyaW5nLmNwcApAQCAt NTEsNyArNTEsNyBAQCBTdHJpbmcgT3BhcXVlSlNTdHJpbmc6OnN0cmluZygpIGNvbnN0CiBJZGVu dGlmaWVyIE9wYXF1ZUpTU3RyaW5nOjppZGVudGlmaWVyKEpTR2xvYmFsRGF0YSogZ2xvYmFsRGF0 YSkgY29uc3QKIHsKICAgICBpZiAoIXRoaXMgfHwgIW1fc3RyaW5nLmxlbmd0aCgpKQotICAgICAg ICByZXR1cm4gSWRlbnRpZmllcihnbG9iYWxEYXRhLCBzdGF0aWNfY2FzdDxjb25zdCBjaGFyKj4o MCkpOworICAgICAgICByZXR1cm4gSWRlbnRpZmllcihJZGVudGlmaWVyOjpFbXB0eUlkZW50aWZp ZXIpOwogCiAgICAgaWYgKG1fc3RyaW5nLmlzOEJpdCgpKQogICAgICAgICByZXR1cm4gSWRlbnRp ZmllcihnbG9iYWxEYXRhLCBtX3N0cmluZy5jaGFyYWN0ZXJzOCgpLCBtX3N0cmluZy5sZW5ndGgo KSk7Cg== 174383 2012-11-15 02:39:23 -0800 2012-11-15 03:00:47 -0800 Patch bug-101867-20121115113656.patch text/plain 1748 allan.jensen U3VidmVyc2lvbiBSZXZpc2lvbjogMTM0NzU1CmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvQ2hhbmdlTG9nIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwppbmRleCBj YjU2NGY5ZjlhMWU3NjU4OTUwNDZhYmU1MDU0YjAxZjNkNTgxNWZlLi4wMzQ4ZDdjZWM0OTJkODA2 NWUwMGE5MzNmMmYzYWE5NjhmMWI5MDE5IDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwxNyBAQAorMjAxMi0xMS0xNSAgQWxsYW4gU2FuZGZlbGQgSmVuc2VuICA8YWxsYW4uamVu c2VuQGRpZ2lhLmNvbT4KKworICAgICAgICBDcmFzaCBpbiBjb252ZXJzaW9uIG9mIGVtcHR5IE9w YXF1ZUpTU3RyaW5nIHRvIElkZW50aWZpZXIgCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQu b3JnL3Nob3dfYnVnLmNnaT9pZD0xMDE4NjcKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkg KE9PUFMhKS4KKworICAgICAgICBUaGUgY29uc3RydWN0b3IgY2FsbCB1c2VkIGZvciBib3RoIG51 bGwgYW5kIGVtcHR5IE9wYXF1ZUpTU3RyaW5ncyByZXN1bHRzCisgICAgICAgIGluIGFuIGFzc2Vy dGlvbiB2b2lsYXRpb24gYW5kIGNyYXNoLiBUaGlzIHBhdGNoIGluc3RlYWQgdXNlcyB0aGUgSWRl bnRpZmllcgorICAgICAgICBjb25zdHJ1Y3RvcnMgd2hpY2ggYXJlIHNwZWNpZmljYWxseSBmb3Ig bnVsbCBhbmQgZW1wdHkgSWRlbnRpZmllci4KKworICAgICAgICAqIEFQSS9PcGFxdWVKU1N0cmlu Zy5jcHA6CisgICAgICAgIChPcGFxdWVKU1N0cmluZzo6aWRlbnRpZmllcik6CisKIDIwMTItMTEt MTQgIEJyZW50IEZ1bGdoYW0gIDxiZnVsZ2hhbUB3ZWJraXQub3JnPgogCiAgICAgICAgIFtXaW5k b3dzLCBXaW5DYWlyb10gVW5yZXZpZXdlZCBidWlsZCBmaXguCmRpZmYgLS1naXQgYS9Tb3VyY2Uv SmF2YVNjcmlwdENvcmUvQVBJL09wYXF1ZUpTU3RyaW5nLmNwcCBiL1NvdXJjZS9KYXZhU2NyaXB0 Q29yZS9BUEkvT3BhcXVlSlNTdHJpbmcuY3BwCmluZGV4IGFjN2NmZDE2YTRlYTI0OTAzZjM4NDJj NGM3MWIzYWU4ZDU2MjYzNzguLmFlYzE4ZmYwMDIwNjU2N2IxMmY1MWYxMDUxZWNkNGEwMDZiOTQz MzEgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9BUEkvT3BhcXVlSlNTdHJpbmcu Y3BwCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9BUEkvT3BhcXVlSlNTdHJpbmcuY3BwCkBA IC01MCw4ICs1MCwxMSBAQCBTdHJpbmcgT3BhcXVlSlNTdHJpbmc6OnN0cmluZygpIGNvbnN0CiAK IElkZW50aWZpZXIgT3BhcXVlSlNTdHJpbmc6OmlkZW50aWZpZXIoSlNHbG9iYWxEYXRhKiBnbG9i YWxEYXRhKSBjb25zdAogewotICAgIGlmICghdGhpcyB8fCAhbV9zdHJpbmcubGVuZ3RoKCkpCi0g ICAgICAgIHJldHVybiBJZGVudGlmaWVyKGdsb2JhbERhdGEsIHN0YXRpY19jYXN0PGNvbnN0IGNo YXIqPigwKSk7CisgICAgaWYgKCF0aGlzIHx8IG1fc3RyaW5nLmlzTnVsbCgpKQorICAgICAgICBy ZXR1cm4gSWRlbnRpZmllcigpOworCisgICAgaWYgKG1fc3RyaW5nLmlzRW1wdHkoKSkKKyAgICAg ICAgcmV0dXJuIElkZW50aWZpZXIoRW1wdHlJZGVudGlmaWVyKTsKIAogICAgIGlmIChtX3N0cmlu Zy5pczhCaXQoKSkKICAgICAgICAgcmV0dXJuIElkZW50aWZpZXIoZ2xvYmFsRGF0YSwgbV9zdHJp bmcuY2hhcmFjdGVyczgoKSwgbV9zdHJpbmcubGVuZ3RoKCkpOwo= 174387 2012-11-15 03:00:51 -0800 2012-11-30 08:59:53 -0800 Patch bug-101867-20121115115823.patch text/plain 1760 allan.jensen U3VidmVyc2lvbiBSZXZpc2lvbjogMTM0NzU1CmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvQ2hhbmdlTG9nIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwppbmRleCBj YjU2NGY5ZjlhMWU3NjU4OTUwNDZhYmU1MDU0YjAxZjNkNTgxNWZlLi4wMzQ4ZDdjZWM0OTJkODA2 NWUwMGE5MzNmMmYzYWE5NjhmMWI5MDE5IDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwxNyBAQAorMjAxMi0xMS0xNSAgQWxsYW4gU2FuZGZlbGQgSmVuc2VuICA8YWxsYW4uamVu c2VuQGRpZ2lhLmNvbT4KKworICAgICAgICBDcmFzaCBpbiBjb252ZXJzaW9uIG9mIGVtcHR5IE9w YXF1ZUpTU3RyaW5nIHRvIElkZW50aWZpZXIgCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQu b3JnL3Nob3dfYnVnLmNnaT9pZD0xMDE4NjcKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkg KE9PUFMhKS4KKworICAgICAgICBUaGUgY29uc3RydWN0b3IgY2FsbCB1c2VkIGZvciBib3RoIG51 bGwgYW5kIGVtcHR5IE9wYXF1ZUpTU3RyaW5ncyByZXN1bHRzCisgICAgICAgIGluIGFuIGFzc2Vy dGlvbiB2b2lsYXRpb24gYW5kIGNyYXNoLiBUaGlzIHBhdGNoIGluc3RlYWQgdXNlcyB0aGUgSWRl bnRpZmllcgorICAgICAgICBjb25zdHJ1Y3RvcnMgd2hpY2ggYXJlIHNwZWNpZmljYWxseSBmb3Ig bnVsbCBhbmQgZW1wdHkgSWRlbnRpZmllci4KKworICAgICAgICAqIEFQSS9PcGFxdWVKU1N0cmlu Zy5jcHA6CisgICAgICAgIChPcGFxdWVKU1N0cmluZzo6aWRlbnRpZmllcik6CisKIDIwMTItMTEt MTQgIEJyZW50IEZ1bGdoYW0gIDxiZnVsZ2hhbUB3ZWJraXQub3JnPgogCiAgICAgICAgIFtXaW5k b3dzLCBXaW5DYWlyb10gVW5yZXZpZXdlZCBidWlsZCBmaXguCmRpZmYgLS1naXQgYS9Tb3VyY2Uv SmF2YVNjcmlwdENvcmUvQVBJL09wYXF1ZUpTU3RyaW5nLmNwcCBiL1NvdXJjZS9KYXZhU2NyaXB0 Q29yZS9BUEkvT3BhcXVlSlNTdHJpbmcuY3BwCmluZGV4IGFjN2NmZDE2YTRlYTI0OTAzZjM4NDJj NGM3MWIzYWU4ZDU2MjYzNzguLjkwYmMxYzA5NTRmMjZkODI5N2Y0Y2UwNTExMWIxN2ZkYjhmYTU5 MTEgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9BUEkvT3BhcXVlSlNTdHJpbmcu Y3BwCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9BUEkvT3BhcXVlSlNTdHJpbmcuY3BwCkBA IC01MCw4ICs1MCwxMSBAQCBTdHJpbmcgT3BhcXVlSlNTdHJpbmc6OnN0cmluZygpIGNvbnN0CiAK IElkZW50aWZpZXIgT3BhcXVlSlNTdHJpbmc6OmlkZW50aWZpZXIoSlNHbG9iYWxEYXRhKiBnbG9i YWxEYXRhKSBjb25zdAogewotICAgIGlmICghdGhpcyB8fCAhbV9zdHJpbmcubGVuZ3RoKCkpCi0g ICAgICAgIHJldHVybiBJZGVudGlmaWVyKGdsb2JhbERhdGEsIHN0YXRpY19jYXN0PGNvbnN0IGNo YXIqPigwKSk7CisgICAgaWYgKCF0aGlzIHx8IG1fc3RyaW5nLmlzTnVsbCgpKQorICAgICAgICBy ZXR1cm4gSWRlbnRpZmllcigpOworCisgICAgaWYgKG1fc3RyaW5nLmlzRW1wdHkoKSkKKyAgICAg ICAgcmV0dXJuIElkZW50aWZpZXIoSWRlbnRpZmllcjo6RW1wdHlJZGVudGlmaWVyKTsKIAogICAg IGlmIChtX3N0cmluZy5pczhCaXQoKSkKICAgICAgICAgcmV0dXJuIElkZW50aWZpZXIoZ2xvYmFs RGF0YSwgbV9zdHJpbmcuY2hhcmFjdGVyczgoKSwgbV9zdHJpbmcubGVuZ3RoKCkpOwo=