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.
Created attachment 165956 [details] Patch
Comment on attachment 165956 [details] 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.
Apparently the example we have is really fragile and depends on details of the allocator and GC to trigger.
Comment on attachment 165956 [details] 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.
Comment on attachment 165956 [details] Patch Rejecting attachment 165956 [details] 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
Comment on attachment 165956 [details] Patch Rejecting attachment 165956 [details] 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
Comment on attachment 165956 [details] Patch Rejecting attachment 165956 [details] 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
hmm, let me try to land it later.
Comment on attachment 165956 [details] Patch Clearing flags on attachment: 165956 Committed r129849: <http://trac.webkit.org/changeset/129849>
All reviewed patches have been landed. Closing bug.