79614 2012-02-26 18:43:33 -0800 [JSC] Memory allocation failed by fastmalloc even if heap grows successfully 2012-03-14 18:31:01 -0700 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) Other Linux RESOLVED FIXED P2 Normal --- 0 hojong.han webkit-unassigned ap barraclough ggaren laszlo.gombos levin mrowe msaboff webkit.review.bot oldest_to_newest 565256 0 hojong.han 2012-02-26 18:43:33 -0800 Heap tries to grow for memory allocation if there were not sufficient memory. At first the size, more than 256 pages, is requested to the system in order to grow the heap, but the size, less than 256 pages which can contain the demanded, is requested if it fails. Spans more than 256 pages are managed in the large list, and spans less than 256 pages are managed in the normal list, so that it's wrong to get the span only from the large list after the heap grows. 566120 1 129131 hojong.han 2012-02-27 16:49:26 -0800 Created attachment 129131 Patch 566123 2 webkit.review.bot 2012-02-27 16:53:53 -0800 Attachment 129131 did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1 Source/JavaScriptCore/wtf/FastMalloc.cpp:1736: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/JavaScriptCore/wtf/FastMalloc.cpp:1737: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/JavaScriptCore/wtf/FastMalloc.cpp:1737: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Source/JavaScriptCore/wtf/FastMalloc.cpp:1737: Use 0 instead of NULL. [readability/null] [5] Source/JavaScriptCore/wtf/FastMalloc.cpp:1740: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/JavaScriptCore/wtf/FastMalloc.cpp:1747: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] Source/JavaScriptCore/wtf/FastMalloc.cpp:1753: One space before end of line comments [whitespace/comments] [5] Source/JavaScriptCore/wtf/FastMalloc.cpp:1757: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/JavaScriptCore/wtf/FastMalloc.cpp:1759: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/JavaScriptCore/wtf/FastMalloc.cpp:1759: Use 0 instead of NULL. [readability/null] [5] Total errors found: 10 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style. 566147 3 hojong.han 2012-02-27 17:08:03 -0800 (In reply to comment #2) > If any of these errors are false positives, please file a bug against check-webkit-style. This entire file is in 2-space indenting. If it should be changed we should change the whole file at once. 573069 4 129131 ggaren 2012-03-07 11:42:03 -0800 Comment on attachment 129131 Patch I don't think it's a good idea to duplicate this code. Instead, after calling GrowHeap(), I think it would be better to call TCMalloc_PageHeap::New recursively. This is guaranteed to terminate after one level of recursion. 573432 5 130742 hojong.han 2012-03-07 18:00:53 -0800 Created attachment 130742 Patch 573436 6 webkit.review.bot 2012-03-07 18:04:09 -0800 Attachment 130742 did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1 Source/JavaScriptCore/wtf/FastMalloc.cpp:1692: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/JavaScriptCore/wtf/FastMalloc.cpp:1692: grown_heap is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Source/JavaScriptCore/wtf/FastMalloc.cpp:1732: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/JavaScriptCore/wtf/FastMalloc.cpp:1733: Use 0 instead of NULL. [readability/null] [5] Source/JavaScriptCore/wtf/FastMalloc.cpp:1742: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/JavaScriptCore/wtf/FastMalloc.cpp:1742: alloc_span is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 6 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style. 573442 7 hojong.han 2012-03-07 18:10:52 -0800 (In reply to comment #4) > (From update of attachment 129131 [details]) > I don't think it's a good idea to duplicate this code. Instead, after calling GrowHeap(), I think it would be better to call TCMalloc_PageHeap::New recursively. This is guaranteed to terminate after one level of recursion. I substituted the duplicated with goto sentence. I think so that it'd be better to call New recursively than the duplicated, but in this case it looks simpler to use goto for recursive call only once. Please let me know if there're more considerations. 576349 8 ggaren 2012-03-12 11:52:42 -0700 > I substituted the duplicated with goto sentence. "goto" turns this code into a loop that usually executes only once. We've seen in the past that GCC and LLVM generate poor code for loops that execute only once because they assume that loops will execute more than once. I think a recursive call would be better. 576350 9 ggaren 2012-03-12 11:53:53 -0700 > 1732 if (grown_heap) > 1733 return NULL; The case can never happen. If you've just grown the heap to accommodate your allocation, allocation will always succeed. I think you should remove this test. 576941 10 131497 hojong.han 2012-03-12 19:49:57 -0700 Created attachment 131497 Patch 576942 11 webkit.review.bot 2012-03-12 19:51:44 -0700 Attachment 131497 did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1 Source/JavaScriptCore/wtf/FastMalloc.cpp:1736: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style. 577108 12 hojong.han 2012-03-12 23:55:44 -0700 (In reply to comment #9) > > 1732 if (grown_heap) > > 1733 return NULL; > > The case can never happen. If you've just grown the heap to accommodate your allocation, allocation will always succeed. I think you should remove this test. My way of thinking got stuck. Your comment showed a broader way. Thanks. 578902 13 131497 ggaren 2012-03-14 16:25:10 -0700 Comment on attachment 131497 Patch r=me 579013 14 131497 webkit.review.bot 2012-03-14 18:30:56 -0700 Comment on attachment 131497 Patch Clearing flags on attachment: 131497 Committed r110805: <http://trac.webkit.org/changeset/110805> 579014 15 webkit.review.bot 2012-03-14 18:31:01 -0700 All reviewed patches have been landed. Closing bug. 129131 2012-02-27 16:49:26 -0800 2012-03-07 18:00:47 -0800 Patch bug-79614-20120228094924.patch text/plain 2234 hojong.han U3VidmVyc2lvbiBSZXZpc2lvbjogMTA5MDM5CmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvQ2hhbmdlTG9nIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwppbmRleCBk MjliZWQ5NmRlNDIxOTc1Y2MyNjBiZDQyZTg4MzAzYmFlNmE2Nzc5Li45YWNhMDQ4NDkxMGRjZDA4 ZWM0ZjY5NGVmNjdiMDI0ZGYyMGIyNTEzIDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwxNyBAQAorMjAxMi0wMi0yNyAgSGFuIEhvam9uZyAgPGhvam9uZy5oYW5Ac2Ftc3VuZy5j b20+CisKKyAgICAgICAgTWVtb3J5IGFsbG9jYXRpb24gZmFpbGVkIGJ5IEZhc3RNYWxsb2MKKyAg ICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTc5NjE0CisKKyAg ICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgTWVtb3J5IGFsbG9j YXRpb24gZmFpbGVkIGV2ZW50IGlmIHRoZSBoZWFwIGdyb3dzIHN1Y2Nlc3NmdWxseS4KKyAgICAg ICAgSXQgaXMgd3JvbmcgdG8gZ2V0IHRoZSBzcGFuIG9ubHkgZnJvbSB0aGUgbGFyZ2UgbGlzdCBh ZnRlciB0aGUgaGVhcCBncm93cywKKyAgICAgICAgYmVjYXVzZSBuZXcgc3BhbiBjb3VsZCBiZSBh ZGRlZCBpbiB0aGUgbm9ybWFsIGxpc3QuCisKKyAgICAgICAgKiB3dGYvRmFzdE1hbGxvYy5jcHA6 CisgICAgICAgIChXVEY6OlRDTWFsbG9jX1BhZ2VIZWFwOjpOZXcpOgorCiAyMDEyLTAyLTI3ICBH YXZpbiBCYXJyYWNsb3VnaCAgPGJhcnJhY2xvdWdoQGFwcGxlLmNvbT4KIAogICAgICAgICBJbXBs ZW1lbnQgc3VwcG9ydCBmb3Igb3BfbmVnYXRlIGFuZCBvcF9iaXRub3QgaW4gdGhlIERGRyBKSVQK ZGlmZiAtLWdpdCBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS93dGYvRmFzdE1hbGxvYy5jcHAgYi9T b3VyY2UvSmF2YVNjcmlwdENvcmUvd3RmL0Zhc3RNYWxsb2MuY3BwCmluZGV4IDZiOTJmNTU0MjBi ZGUwZDdlYTI5MDg5MDUxOTIxZDUyMTE2Y2VlMmYuLmY5MDU4Mjg0ZTllYzk1MjY5YjVkYzI3N2Fi NzdkNjM2MzJjZTY1MTggMTAwNjQ0Ci0tLSBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS93dGYvRmFz dE1hbGxvYy5jcHAKKysrIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL3d0Zi9GYXN0TWFsbG9jLmNw cApAQCAtMTczMyw3ICsxNzMzLDMwIEBAIGlubGluZSBTcGFuKiBUQ01hbGxvY19QYWdlSGVhcDo6 TmV3KExlbmd0aCBuKSB7CiAgICAgcmV0dXJuIE5VTEw7CiAgIH0KIAotICByZXR1cm4gQWxsb2NM YXJnZShuKTsKKyAgcmVzdWx0ID0gQWxsb2NMYXJnZShuKTsKKyAgaWYgKHJlc3VsdCAhPSBOVUxM KSB7CisgICAgQVNTRVJUX1NQQU5fQ09NTUlUVEVEKHJlc3VsdCk7CisgICAgcmV0dXJuIHJlc3Vs dDsKKyAgfSBlbHNlIGlmICgobiA8IGtNYXhQYWdlcykgJiYgIURMTF9Jc0VtcHR5KCZmcmVlX1tu XS5ub3JtYWwpKSB7CisgICAgLy8gRm91bmQgbm9ybWFsIHNwYW4KKyAgICBTcGFuKiBsbCA9ICZm cmVlX1tuXS5ub3JtYWw7CisKKyAgICByZXN1bHQgPSBsbC0+bmV4dDsKKyAgICBDYXJ2ZShyZXN1 bHQsIG4sIGZhbHNlKTsKKyNpZiBVU0VfQkFDS0dST1VORF9USFJFQURfVE9fU0NBVkVOR0VfTUVN T1JZCisgICAgLy8gVGhlIG5ld2x5IGFsbG9jYXRlZCBtZW1vcnkgaXMgZnJvbSBhIHNwYW4gdGhh dCdzIGluIHRoZSBub3JtYWwgc3BhbiBsaXN0IChhbHJlYWR5IGNvbW1pdHRlZCkuICBVcGRhdGUg dGhlCisgICAgLy8gZnJlZSBjb21taXR0ZWQgcGFnZXMgY291bnQuCisgICAgQVNTRVJUKGZyZWVf Y29tbWl0dGVkX3BhZ2VzXyA+PSBuKTsKKyAgICBmcmVlX2NvbW1pdHRlZF9wYWdlc18gLT0gbjsK KyAgICBpZiAoZnJlZV9jb21taXR0ZWRfcGFnZXNfIDwgbWluX2ZyZWVfY29tbWl0dGVkX3BhZ2Vz X3NpbmNlX2xhc3Rfc2NhdmVuZ2VfKQorICAgICAgbWluX2ZyZWVfY29tbWl0dGVkX3BhZ2VzX3Np bmNlX2xhc3Rfc2NhdmVuZ2VfID0gZnJlZV9jb21taXR0ZWRfcGFnZXNfOworI2VuZGlmICAvLyBV U0VfQkFDS0dST1VORF9USFJFQURfVE9fU0NBVkVOR0VfTUVNT1JZCisgICAgQVNTRVJUKENoZWNr KCkpOworICAgIGZyZWVfcGFnZXNfIC09IG47CisgICAgcmV0dXJuIHJlc3VsdDsKKyAgfQorCisg IHJldHVybiBOVUxMOwogfQogCiBTcGFuKiBUQ01hbGxvY19QYWdlSGVhcDo6QWxsb2NMYXJnZShM ZW5ndGggbikgewo= 130742 2012-03-07 18:00:53 -0800 2012-03-12 19:49:49 -0700 Patch bug-79614-20120308110051.patch text/plain 1871 hojong.han U3VidmVyc2lvbiBSZXZpc2lvbjogMTEwMDM5CmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvQ2hhbmdlTG9nIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwppbmRleCBh NGRhNjhkZWFhYmYyOWYxYTYxOTE5OGM0OTEzZTA1NjAwNzA2YzVkLi4xN2YyYzNjYzAyYzBmNWE0 NzI4ODc1MDJiMWE4NjI1N2U1OWEzZmE1IDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwxNyBAQAorMjAxMi0wMy0wNyAgSG9qb25nIEhhbiAgPGhvam9uZy5oYW5Ac2Ftc3VuZy5j b20+CisKKyAgICAgICAgRml4IG1lbW9yeSBhbGxvY2F0aW9uIGZhaWxlZCBieSBmYXN0bWFsbG9j CisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD03OTYxNAor CisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIE1lbW9yeSBh bGxvY2F0aW9uIGZhaWxlZCBldmVuIGlmIHRoZSBoZWFwIGdyb3dzIHN1Y2Nlc3NmdWxseS4KKyAg ICAgICAgSXQgaXMgd3JvbmcgdG8gZ2V0IHRoZSBzcGFuIG9ubHkgZnJvbSB0aGUgbGFyZ2UgbGlz dCBhZnRlciB0aGUgaGVhcCBncm93cywKKyAgICAgICAgYmVjYXVzZSBuZXcgc3BhbiBjb3VsZCBi ZSBhZGRlZCBpbiB0aGUgbm9ybWFsIGxpc3QuCisKKyAgICAgICAgKiB3dGYvRmFzdE1hbGxvYy5j cHA6CisgICAgICAgIChXVEY6OlRDTWFsbG9jX1BhZ2VIZWFwOjpOZXcpOgorCiAyMDEyLTAzLTA3 ICBBbmR5IFdpbmdvICA8d2luZ29AaWdhbGlhLmNvbT4KIAogICAgICAgICBQYXJzZXI6IElubGlu ZSBTY29wZU5vZGVEYXRhIGludG8gU2NvcGVOb2RlCmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNj cmlwdENvcmUvd3RmL0Zhc3RNYWxsb2MuY3BwIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL3d0Zi9G YXN0TWFsbG9jLmNwcAppbmRleCA2YjkyZjU1NDIwYmRlMGQ3ZWEyOTA4OTA1MTkyMWQ1MjExNmNl ZTJmLi5lNjY2OGVjOWIwZDU1YTQ5NGM3Zjg3MmNmZWQwNDVlYTVmMDk1NGIxIDEwMDY0NAotLS0g YS9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvd3RmL0Zhc3RNYWxsb2MuY3BwCisrKyBiL1NvdXJjZS9K YXZhU2NyaXB0Q29yZS93dGYvRmFzdE1hbGxvYy5jcHAKQEAgLTE2ODksNyArMTY4OSw5IEBAIEFM V0FZU19JTkxJTkUgYm9vbCBUQ01hbGxvY19QYWdlSGVhcDo6c2hvdWxkU2NhdmVuZ2UoKSBjb25z dAogaW5saW5lIFNwYW4qIFRDTWFsbG9jX1BhZ2VIZWFwOjpOZXcoTGVuZ3RoIG4pIHsKICAgQVNT RVJUKENoZWNrKCkpOwogICBBU1NFUlQobiA+IDApOworICBib29sIGdyb3duX2hlYXAgPSBmYWxz ZTsKIAorYWxsb2Nfc3BhbjoKICAgLy8gRmluZCBmaXJzdCBzaXplID49IG4gdGhhdCBoYXMgYSBu b24tZW1wdHkgbGlzdAogICBmb3IgKExlbmd0aCBzID0gbjsgcyA8IGtNYXhQYWdlczsgcysrKSB7 CiAgICAgU3BhbiogbGwgPSBOVUxMOwpAQCAtMTcyNywxMyArMTcyOSwxNyBAQCBpbmxpbmUgU3Bh biogVENNYWxsb2NfUGFnZUhlYXA6Ok5ldyhMZW5ndGggbikgewogICAgICAgcmV0dXJuIHJlc3Vs dDsKICAgfQogCisgIGlmIChncm93bl9oZWFwKQorICAgIHJldHVybiBOVUxMOworCiAgIC8vIEdy b3cgdGhlIGhlYXAgYW5kIHRyeSBhZ2FpbgogICBpZiAoIUdyb3dIZWFwKG4pKSB7CiAgICAgQVNT RVJUKENoZWNrKCkpOwogICAgIHJldHVybiBOVUxMOwogICB9CiAKLSAgcmV0dXJuIEFsbG9jTGFy Z2Uobik7CisgIGdyb3duX2hlYXAgPSB0cnVlOyAKKyAgZ290byBhbGxvY19zcGFuOwogfQogCiBT cGFuKiBUQ01hbGxvY19QYWdlSGVhcDo6QWxsb2NMYXJnZShMZW5ndGggbikgewo= 131497 2012-03-12 19:49:57 -0700 2012-03-14 18:30:56 -0700 Patch bug-79614-20120313114956.patch text/plain 1353 hojong.han U3VidmVyc2lvbiBSZXZpc2lvbjogMTEwNDEyCmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvQ2hhbmdlTG9nIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwppbmRleCBl ZjcwYzQ2MzA2MmNiZjBiMWI1ZTA3ZDQzYjNmYTczY2NmNDA2MmVkLi44ZTc5YWRhOGVlNDlmYzMw MjM4YjU3MzViYjZmYTQwMGFhYTMwZDJlIDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwxNyBAQAorMjAxMi0wMy0xMiAgSG9qb25nIEhhbiAgPGhvam9uZy5oYW5Ac2Ftc3VuZy5j b20+CisKKyAgICAgICAgRml4IG1lbW9yeSBhbGxvY2F0aW9uIGZhaWxlZCBieSBmYXN0bWFsbG9j CisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD03OTYxNAor CisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIE1lbW9yeSBh bGxvY2F0aW9uIGZhaWxlZCBldmVuIGlmIHRoZSBoZWFwIGdyb3dzIHN1Y2Nlc3NmdWxseS4KKyAg ICAgICAgSXQgaXMgd3JvbmcgdG8gZ2V0IHRoZSBzcGFuIG9ubHkgZnJvbSB0aGUgbGFyZ2UgbGlz dCBhZnRlciB0aGUgaGVhcCBncm93cywKKyAgICAgICAgYmVjYXVzZSBuZXcgc3BhbiBjb3VsZCBi ZSBhZGRlZCBpbiB0aGUgbm9ybWFsIGxpc3QuCisKKyAgICAgICAgKiB3dGYvRmFzdE1hbGxvYy5j cHA6CisgICAgICAgIChXVEY6OlRDTWFsbG9jX1BhZ2VIZWFwOjpOZXcpOgorCiAyMDEyLTAzLTEw ICBGaWxpcCBQaXpsbyAgPGZwaXpsb0BhcHBsZS5jb20+CiAKICAgICAgICAgTExJbnQgc2hvdWxk IHN1cHBvcnQgSlNWQUxVRTY0CmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvd3Rm L0Zhc3RNYWxsb2MuY3BwIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL3d0Zi9GYXN0TWFsbG9jLmNw cAppbmRleCA2YjkyZjU1NDIwYmRlMGQ3ZWEyOTA4OTA1MTkyMWQ1MjExNmNlZTJmLi5kZjY0NmVi OTkyYzcyZjEyYWYzYTAxNmZiYjEzZDhjYTNhY2MxM2FkIDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2 YVNjcmlwdENvcmUvd3RmL0Zhc3RNYWxsb2MuY3BwCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29y ZS93dGYvRmFzdE1hbGxvYy5jcHAKQEAgLTE3MzMsNyArMTczMyw3IEBAIGlubGluZSBTcGFuKiBU Q01hbGxvY19QYWdlSGVhcDo6TmV3KExlbmd0aCBuKSB7CiAgICAgcmV0dXJuIE5VTEw7CiAgIH0K IAotICByZXR1cm4gQWxsb2NMYXJnZShuKTsKKyAgcmV0dXJuIE5ldyhuKTsKIH0KIAogU3Bhbiog VENNYWxsb2NfUGFnZUhlYXA6OkFsbG9jTGFyZ2UoTGVuZ3RoIG4pIHsK