31180 2009-11-05 11:25:30 -0800 [v8] Do not retrieve handles in near death or empty state from the string impl cache 2010-01-18 11:29:27 -0800 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 1 antonm antonm commit-queue eric oldest_to_newest 160894 0 antonm 2009-11-05 11:25:30 -0800 Due to nested GCs, there is a low chance of weak reference callback for external string being invoked while this string still referenced. Most probably that is due to StringImpl cache (it allows to 'revive' an external string). 160895 1 42581 antonm 2009-11-05 11:29:03 -0800 Created attachment 42581 First take 160901 2 42581 abarth 2009-11-05 11:45:19 -0800 Comment on attachment 42581 First take + v8::Persistent<v8::String> handle(cachedV8String); Why a Persistent handle? Can we use a local handle here? Can we test this? 160978 3 antonm 2009-11-05 15:19:56 -0800 First of all, I've found out that my previous fix didn't make into 4.0.223.16, so I'd ask you to put this one on hold (I'd appreciate if you review+ it when found in good shape, but I don't want it to be committed, at least for now). (In reply to comment #2) > (From update of attachment 42581 [details]) > + v8::Persistent<v8::String> handle(cachedV8String); > > Why a Persistent handle? Can we use a local handle here? ::IsNearDeath is only present on Persistent handles (Locals are always alive). If you're afraid of performance applications, AFAIK Persistent ctor (but not ::New) is roughly free. > Can we test this? I am lacking knowledge of our testing infrastructure. The easiest way to check it would be to write some notable amount of C++ code (to create an object with a special weak reference callback, etc.) and expose it into JS. Have we got something like that? 161056 4 42581 abarth 2009-11-05 20:17:28 -0800 Comment on attachment 42581 First take No, we don't have any testing infrastructure like that. I don't see how we could test this change. 165370 5 42581 commit-queue 2009-11-20 11:14:01 -0800 Comment on attachment 42581 First take Rejecting patch 42581 from commit-queue. Failed to run "['git', 'svn', 'dcommit']" exit_code: 1 Committing to http://svn.webkit.org/repository/webkit/trunk ... 165380 6 42581 eric 2009-11-20 11:32:18 -0800 Comment on attachment 42581 First take This rejection makes no sense. I'll investigate. 165386 7 42581 commit-queue 2009-11-20 11:49:34 -0800 Comment on attachment 42581 First take Rejecting patch 42581 from commit-queue. Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1']" exit_code: 1 Running build-dumprendertree Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests Testing 11649 test cases. inspector/console-format.html -> crashed Exiting early after 1 failures. 9345 tests run. 490.34s total testing time 9344 test cases (99%) succeeded 1 test case (<1%) crashed 5 test cases (<1%) had stderr output 165391 8 antonm 2009-11-20 12:15:14 -0800 (In reply to comment #7) > (From update of attachment 42581 [details]) > Rejecting patch 42581 from commit-queue. > > Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', > '--quiet', '--exit-after-n-failures=1']" exit_code: 1 > Running build-dumprendertree > Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests > Testing 11649 test cases. > inspector/console-format.html -> crashed > > Exiting early after 1 failures. 9345 tests run. > 490.34s total testing time > > 9344 test cases (99%) succeeded > 1 test case (<1%) crashed > 5 test cases (<1%) had stderr output Wow, thanks a lot, but that's something new. Probably patch just rotted. I'll double check if I can reproduce it locally. Thanks again. 175411 9 eric 2009-12-28 22:40:09 -0800 Attachment 42581 was posted by a committer and has review+, assigning to Anton Muhin for commit. 178030 10 42581 eric 2010-01-06 23:37:18 -0800 Comment on attachment 42581 First take I think that rejection was bogus. Marking cq+ again. 178050 11 42581 commit-queue 2010-01-07 00:22:11 -0800 Comment on attachment 42581 First take Clearing flags on attachment: 42581 Committed r52905: <http://trac.webkit.org/changeset/52905> 178051 12 commit-queue 2010-01-07 00:22:16 -0800 All reviewed patches have been landed. Closing bug. 179376 13 antonm 2010-01-11 08:38:52 -0800 Oops. I intentionally didn't commit it as I wasn't 100% sure it's necessary to fix the bug (and that bug never reappeared since then), but it slowed us down. Let me double check and probably I would need to revert it back. 179467 14 eric 2010-01-11 12:06:05 -0800 Please do not leave patches up for review, or even worse, r+'d that you do not want committed. :) 181909 15 antonm 2010-01-18 09:59:03 -0800 (In reply to comment #14) > Please do not leave patches up for review, or even worse, r+'d that you do not > want committed. :) I see, sorry. I wanted that to be submittable on instant, but as I can see now I abused the system. The case is probably too rare to have any guidelines for it, but if there is something immediate you or any other remembers, I'd be glad to learn that :) 181935 16 eric 2010-01-18 11:29:27 -0800 Oh, it's not a big deal at all. And no, sadly we don't really have guidelines for this sort of thing. We don't really have a "ready to commit, but commit blocked" state, but we should probably eventually create one just for this kind of patch (as it's rather common). :) Thanks again for the patch! 42581 2009-11-05 11:29:03 -0800 2010-01-07 00:22:11 -0800 First take WebCore.patch text/plain 1363 antonm SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA1MDU2OCkKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTQgQEAKKzIwMDktMTEtMDUgIEFudG9uIE11aGluICA8YW50b25tQGNocm9taXVt Lm9yZz4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBE byBub3QgcmV0dXJuIHN0cmluZ3Mgd2hvc2UgaGFuZGxlcyBhcmUgaW4gbmVhciBkZWF0aCBzdGF0 ZSAodGhhdCBtZWFucworICAgICAgICB0aGV5IG1pZ2h0IGJlIEdDZWQpIG9yIGp1c3QgZW1wdHku CisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0zMTE4MAor CisgICAgICAgICogYmluZGluZ3MvdjgvVjhCaW5kaW5nLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6 OnY4RXh0ZXJuYWxTdHJpbmcpOgorCiAyMDA5LTExLTA1ICBKaWFuIExpICA8amlhbmxpQGNocm9t aXVtLm9yZz4KIAogICAgICAgICBSZXZpZXdlZCBieSBQYXZlbCBGZWxkbWFuLgpJbmRleDogV2Vi Q29yZS9iaW5kaW5ncy92OC9WOEJpbmRpbmcuY3BwCj09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvYmlu ZGluZ3MvdjgvVjhCaW5kaW5nLmNwcAkocmV2aXNpb24gNTA1MzUpCisrKyBXZWJDb3JlL2JpbmRp bmdzL3Y4L1Y4QmluZGluZy5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTI2NCw3ICsyNjQsMTEgQEAK ICAgICBTdHJpbmdDYWNoZSYgc3RyaW5nQ2FjaGUgPSBnZXRTdHJpbmdDYWNoZSgpOwogICAgIHY4 OjpTdHJpbmcqIGNhY2hlZFY4U3RyaW5nID0gc3RyaW5nQ2FjaGUuZ2V0KHN0cmluZ0ltcGwpOwog ICAgIGlmIChjYWNoZWRWOFN0cmluZykKLSAgICAgICAgcmV0dXJuIHY4OjpMb2NhbDx2ODo6U3Ry aW5nPjo6TmV3KHY4OjpIYW5kbGU8djg6OlN0cmluZz4oY2FjaGVkVjhTdHJpbmcpKTsKKyAgICB7 CisgICAgICAgIHY4OjpQZXJzaXN0ZW50PHY4OjpTdHJpbmc+IGhhbmRsZShjYWNoZWRWOFN0cmlu Zyk7CisgICAgICAgIGlmICghaGFuZGxlLklzTmVhckRlYXRoKCkgJiYgIWhhbmRsZS5Jc0VtcHR5 KCkpCisgICAgICAgICAgICByZXR1cm4gdjg6OkxvY2FsPHY4OjpTdHJpbmc+OjpOZXcoaGFuZGxl KTsKKyAgICB9CiAKICAgICB2ODo6TG9jYWw8djg6OlN0cmluZz4gbmV3U3RyaW5nID0gbWFrZUV4 dGVybmFsU3RyaW5nKHN0cmluZyk7CiAgICAgaWYgKG5ld1N0cmluZy5Jc0VtcHR5KCkpCg==