97767 2012-09-27 02:20:56 -0700 [V8] StringCache::v8ExternalString() can return a stale persistent handle 2012-09-27 21:27:30 -0700 1 1 1 Unclassified WebKit WebCore JavaScript 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 haraken haraken abarth dcarney japhet webkit.review.bot oldest_to_newest 729338 0 haraken 2012-09-27 02:20:56 -0700 For details, see the Chromium bug: http://code.google.com/p/chromium/issues/detail?id=151902 StringCache::v8ExternalString() can return a stale persistent handle in the following scenario: (1) Assume that StringImpl A with value "foo" is in m_stringCache. (2) StringImpl B with value "foo" is accessed. At this point, m_lastStringImpl points to B, and m_lastV8String points to B's handle. (3) A minor GC is triggered and a weak callback is called back for StringImpl A. At this point, "foo" is removed from m_stringCache. A's handle is disposed. However, m_lastV8String is not cleared because m_lastStringImpl (i.e. StringImpl B) is not equal to StringImpl A. As a result, m_lastV8String points to a stale persistent handle. (4) The persistent handle is eventually reused in V8 and made weak again. (5) StringImpl B with value "foo" is accessed. Then StringCache::v8ExternalString() returns the stale persistent handle, which is already used for another purpose. To solve the problem, we need to clear m_stringImpl and m_lastV8String when any string wrapper is disposed. Specifically, we need to change the code like this: static void cachedStringCallback(v8::Persistent<v8::Value> wrapper, void* parameter) { StringImpl* stringImpl = static_cast<StringImpl*>(parameter); V8PerIsolateData::current()->stringCache()->remove(stringImpl); wrapper.Dispose(); stringImpl->deref(); } void StringCache::remove(StringImpl* stringImpl) { m_stringCache.remove(stringImpl); if (m_lastStringImpl.get() == stringImpl) { // Remove this line. m_lastStringImpl = 0; m_lastV8String.Clear(); } } Note: Removing the line might be stronger than is needed. Instead of removing the line, we can just replace the line with 'if (m_lastV8String == wrapper)'. However, just in case (for correctness), I'd prefer removing the line. Given that GC won't happen so frequently, clearing the cache in every weak callback won't affect performance. 729341 1 165956 haraken 2012-09-27 02:23:29 -0700 Created attachment 165956 Patch 729633 2 165956 abarth 2012-09-27 09:49:40 -0700 Comment on attachment 165956 Patch I'm marking this patch r+, but I asked haraken about whether some information in crbug.com/151902 would make it possible to write a test for this patch. 729668 3 abarth 2012-09-27 10:22:55 -0700 Apparently the example we have is really fragile and depends on details of the allocator and GC to trigger. 729895 4 165956 haraken 2012-09-27 15:00:01 -0700 Comment on attachment 165956 Patch > Apparently the example we have is really fragile and depends on details of the allocator and GC to trigger. Yeah, it depends on the behavior of a minor GC, which is difficult to test. 729905 5 165956 webkit.review.bot 2012-09-27 15:04:51 -0700 Comment on attachment 165956 Patch Rejecting attachment 165956 from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1 ERROR: /mnt/git/webkit-commit-queue/Source/WebKit/mac/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://queues.webkit.org/results/14054068 729981 6 165956 webkit.review.bot 2012-09-27 16:25:21 -0700 Comment on attachment 165956 Patch Rejecting attachment 165956 from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog Failed to merge in the changes. Patch failed at 0001 Results page should warn about time-dependent distributions When you have resolved this problem run "git rebase --continue". If you would prefer to skip this patch, instead run "git rebase --skip". To restore the original branch and stop rebasing run "git rebase --abort". rebase refs/remotes/origin/master: command returned error: 1 Died at Tools/Scripts/update-webkit line 164. Full output: http://queues.webkit.org/results/14038844 730013 7 165956 webkit.review.bot 2012-09-27 16:52:37 -0700 Comment on attachment 165956 Patch Rejecting attachment 165956 from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1 Last 500 characters of output: Kit/chromium/third_party/yasm/source/patched-yasm --revision 154708 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' 51>At revision 154708. ________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' ________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' Updating webkit projects from gyp files... Full output: http://queues.webkit.org/results/14036965 730015 8 haraken 2012-09-27 16:53:09 -0700 hmm, let me try to land it later. 730222 9 165956 webkit.review.bot 2012-09-27 21:27:27 -0700 Comment on attachment 165956 Patch Clearing flags on attachment: 165956 Committed r129849: <http://trac.webkit.org/changeset/129849> 730223 10 webkit.review.bot 2012-09-27 21:27:30 -0700 All reviewed patches have been landed. Closing bug. 165956 2012-09-27 02:23:29 -0700 2012-09-27 21:27:27 -0700 Patch bug-97767-20120927182247.patch text/plain 3895 haraken U3VidmVyc2lvbiBSZXZpc2lvbjogMTI5NzIyCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggYWExZTYxN2UwM2U4MTA2 MGM0YmUzNTZlZDUwYjA5ZjEwNzNmN2JjMS4uMjcxN2ExMWE2ODcwY2JjZjg0ODkzN2I0YmQyNmNm NmRkMjZmOGYzMCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDU4IEBACisyMDEyLTA5LTI3ICBLZW50 YXJvIEhhcmEgIDxoYXJha2VuQGNocm9taXVtLm9yZz4KKworICAgICAgICBbVjhdIFN0cmluZ0Nh Y2hlOjp2OEV4dGVybmFsU3RyaW5nKCkgY2FuIHJldHVybiBhIHN0YWxlIHBlcnNpc3RlbnQgaGFu ZGxlCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD05Nzc2 NworCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIEZvciBk ZXRhaWxzLCBzZWUgdGhlIENocm9taXVtIGJ1ZzogaHR0cDovL2NvZGUuZ29vZ2xlLmNvbS9wL2No cm9taXVtL2lzc3Vlcy9kZXRhaWw/aWQ9MTUxOTAyCisKKyAgICAgICAgU3RyaW5nQ2FjaGU6OnY4 RXh0ZXJuYWxTdHJpbmcoKSBjYW4gcmV0dXJuIGEgc3RhbGUgcGVyc2lzdGVudCBoYW5kbGUKKyAg ICAgICAgaW4gdGhlIGZvbGxvd2luZyBzY2VuYXJpbzoKKworICAgICAgICAoMSkgQXNzdW1lIHRo YXQgU3RyaW5nSW1wbCBBIHdpdGggdmFsdWUgImZvbyIgaXMgaW4gbV9zdHJpbmdDYWNoZS4KKyAg ICAgICAgKDIpIFN0cmluZ0ltcGwgQiB3aXRoIHZhbHVlICJmb28iIGlzIGFjY2Vzc2VkLiBBdCB0 aGlzIHBvaW50LCBtX2xhc3RTdHJpbmdJbXBsCisgICAgICAgIHBvaW50cyB0byBCLCBhbmQgbV9s YXN0VjhTdHJpbmcgcG9pbnRzIHRvIEIncyBoYW5kbGUuCisgICAgICAgICgzKSBBIG1pbm9yIEdD IGlzIHRyaWdnZXJlZCBhbmQgYSB3ZWFrIGNhbGxiYWNrIGlzIGNhbGxlZCBiYWNrIGZvciBTdHJp bmdJbXBsIEEuCisgICAgICAgIEF0IHRoaXMgcG9pbnQsICJmb28iIGlzIHJlbW92ZWQgZnJvbSBt X3N0cmluZ0NhY2hlLiBBJ3MgaGFuZGxlIGlzIGRpc3Bvc2VkLgorICAgICAgICBIb3dldmVyLCBt X2xhc3RWOFN0cmluZyBpcyBub3QgY2xlYXJlZCBiZWNhdXNlIG1fbGFzdFN0cmluZ0ltcGwgKGku ZS4gU3RyaW5nSW1wbCBCKQorICAgICAgICBpcyBub3QgZXF1YWwgdG8gU3RyaW5nSW1wbCBBLiBB cyBhIHJlc3VsdCwgbV9sYXN0VjhTdHJpbmcgcG9pbnRzIHRvIGEgc3RhbGUKKyAgICAgICAgcGVy c2lzdGVudCBoYW5kbGUuCisgICAgICAgICg0KSBUaGUgcGVyc2lzdGVudCBoYW5kbGUgaXMgZXZl bnR1YWxseSByZXVzZWQgaW4gVjggYW5kIG1hZGUgd2VhayBhZ2Fpbi4KKyAgICAgICAgKDUpIFN0 cmluZ0ltcGwgQiB3aXRoIHZhbHVlICJmb28iIGlzIGFjY2Vzc2VkLiBUaGVuIFN0cmluZ0NhY2hl Ojp2OEV4dGVybmFsU3RyaW5nKCkKKyAgICAgICAgcmV0dXJucyB0aGUgc3RhbGUgcGVyc2lzdGVu dCBoYW5kbGUsIHdoaWNoIGlzIGFscmVhZHkgdXNlZCBmb3IgYW5vdGhlciBwdXJwb3NlLgorCisg ICAgICAgIFRvIHNvbHZlIHRoZSBwcm9ibGVtLCB3ZSBuZWVkIHRvIGNsZWFyIG1fc3RyaW5nSW1w bCBhbmQgbV9sYXN0VjhTdHJpbmcgd2hlbiBhbnkKKyAgICAgICAgc3RyaW5nIHdyYXBwZXIgaXMg ZGlzcG9zZWQuIFNwZWNpZmljYWxseSwgd2UgbmVlZCB0byBjaGFuZ2UgdGhlIGNvZGUgbGlrZSB0 aGlzOgorCisgICAgICAgICAgc3RhdGljIHZvaWQgY2FjaGVkU3RyaW5nQ2FsbGJhY2sodjg6OlBl cnNpc3RlbnQ8djg6OlZhbHVlPiB3cmFwcGVyLCB2b2lkKiBwYXJhbWV0ZXIpCisgICAgICAgICAg eworICAgICAgICAgICAgU3RyaW5nSW1wbCogc3RyaW5nSW1wbCA9IHN0YXRpY19jYXN0PFN0cmlu Z0ltcGwqPihwYXJhbWV0ZXIpOworICAgICAgICAgICAgVjhQZXJJc29sYXRlRGF0YTo6Y3VycmVu dCgpLT5zdHJpbmdDYWNoZSgpLT5yZW1vdmUoc3RyaW5nSW1wbCk7CisgICAgICAgICAgICB3cmFw cGVyLkRpc3Bvc2UoKTsKKyAgICAgICAgICAgIHN0cmluZ0ltcGwtPmRlcmVmKCk7CisgICAgICAg ICAgfQorCisgICAgICAgICAgdm9pZCBTdHJpbmdDYWNoZTo6cmVtb3ZlKFN0cmluZ0ltcGwqIHN0 cmluZ0ltcGwpCisgICAgICAgICAgeworICAgICAgICAgICAgbV9zdHJpbmdDYWNoZS5yZW1vdmUo c3RyaW5nSW1wbCk7CisgICAgICAgICAgICBpZiAobV9sYXN0U3RyaW5nSW1wbC5nZXQoKSA9PSBz dHJpbmdJbXBsKSB7ICAvLyBSZW1vdmUgdGhpcyBsaW5lLgorICAgICAgICAgICAgICAgIG1fbGFz dFN0cmluZ0ltcGwgPSAwOworICAgICAgICAgICAgICAgIG1fbGFzdFY4U3RyaW5nLkNsZWFyKCk7 CisgICAgICAgICAgICB9CisgICAgICAgICAgfQorCisgICAgICAgIE5vdGU6IFJlbW92aW5nIHRo ZSBsaW5lIG1pZ2h0IGJlIHN0cm9uZ2VyIHRoYW4gaXMgbmVlZGVkLiBJbnN0ZWFkIG9mIHJlbW92 aW5nCisgICAgICAgIHRoZSBsaW5lLCB3ZSBjYW4ganVzdCByZXBsYWNlIHRoZSBsaW5lIHdpdGgg J2lmIChtX2xhc3RWOFN0cmluZyA9PSB3cmFwcGVyKScuCisgICAgICAgIEhvd2V2ZXIsIGp1c3Qg aW4gY2FzZSAoZm9yIGNvcnJlY3RuZXNzKSwgSSdkIHByZWZlciByZW1vdmluZyB0aGUgbGluZS4K KyAgICAgICAgR2l2ZW4gdGhhdCBHQyB3b24ndCBoYXBwZW4gc28gZnJlcXVlbnRseSwgY2xlYXJp bmcgdGhlIGNhY2hlIGluIGV2ZXJ5IHdlYWsgY2FsbGJhY2sKKyAgICAgICAgd29uJ3QgYWZmZWN0 IHBlcmZvcm1hbmNlLgorCisgICAgICAgIE5vIHRlc3RzIGJlY2F1c2UgaXQgZGVwZW5kcyBvbiB0 aGUgR0MgYmVoYXZpb3IgYW5kIEkgY291bGRuJ3QgcmVwcm9kdWNlIHRoZSBidWcuCisKKyAgICAg ICAgKiBiaW5kaW5ncy92OC9WOFZhbHVlQ2FjaGUuY3BwOgorICAgICAgICAoV2ViQ29yZTo6U3Ry aW5nQ2FjaGU6OnJlbW92ZSk6CisKIDIwMTItMDktMjYgIFlvc2hpZnVtaSBJbm91ZSAgPHlvc2lu QGNocm9taXVtLm9yZz4KIAogICAgICAgICBbRm9ybXNdIENvcHkgVGltZUlucHV0VHlwZS57Y3Bw LGh9IHRvIEJhc2VNdWx0aXBsZUZpZWxkc0RhdGVBbmRUaW1lSW5wdXRUeXBlLntjcHAsaH0KZGlm ZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL2JpbmRpbmdzL3Y4L1Y4VmFsdWVDYWNoZS5jcHAgYi9T b3VyY2UvV2ViQ29yZS9iaW5kaW5ncy92OC9WOFZhbHVlQ2FjaGUuY3BwCmluZGV4IDFjMjUxMDY2 NDNlMDc5MGNkOGM2YTRmZTFlYjkwOWFlOGQ0N2Y5MjYuLmM2MzZmMmMwMWZiY2E0ZTEzMTA2OThk OWMyMzZmNWM5NzAzZjMzNmMgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL2JpbmRpbmdzL3Y4 L1Y4VmFsdWVDYWNoZS5jcHAKKysrIGIvU291cmNlL1dlYkNvcmUvYmluZGluZ3MvdjgvVjhWYWx1 ZUNhY2hlLmNwcApAQCAtNTUsMTAgKzU1LDcgQEAgdm9pZCBTdHJpbmdDYWNoZTo6cmVtb3ZlKFN0 cmluZ0ltcGwqIHN0cmluZ0ltcGwpCiAgICAgbV9zdHJpbmdDYWNoZS5yZW1vdmUoc3RyaW5nSW1w bCk7CiAgICAgLy8gTWFrZSBzdXJlIHRoYXQgYWxyZWFkeSBkaXNwb3NlZCBtX2xhc3RWOFN0cmlu ZyBpcyBub3QgdXNlZCBpbgogICAgIC8vIFN0cmluZ0NhY2hlOjp2OEV4dGVybmFsU3RyaW5nKCku Ci0gICAgaWYgKG1fbGFzdFN0cmluZ0ltcGwuZ2V0KCkgPT0gc3RyaW5nSW1wbCkgewotICAgICAg ICBtX2xhc3RTdHJpbmdJbXBsID0gMDsKLSAgICAgICAgbV9sYXN0VjhTdHJpbmcuQ2xlYXIoKTsK LSAgICB9CisgICAgY2xlYXJPbkdDKCk7CiB9CiAKIHY4OjpMb2NhbDx2ODo6U3RyaW5nPiBTdHJp bmdDYWNoZTo6djhFeHRlcm5hbFN0cmluZ1Nsb3coU3RyaW5nSW1wbCogc3RyaW5nSW1wbCwgdjg6 Oklzb2xhdGUqIGlzb2xhdGUpCg==