Bug 97767 - [V8] StringCache::v8ExternalString() can return a stale persistent handle
Summary: [V8] StringCache::v8ExternalString() can return a stale persistent handle
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kentaro Hara
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-09-27 02:20 PDT by Kentaro Hara
Modified: 2012-09-27 21:27 PDT (History)
4 users (show)

See Also:


Attachments
Patch (3.80 KB, patch)
2012-09-27 02:23 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kentaro Hara 2012-09-27 02:20:56 PDT
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.
Comment 1 Kentaro Hara 2012-09-27 02:23:29 PDT
Created attachment 165956 [details]
Patch
Comment 2 Adam Barth 2012-09-27 09:49:40 PDT
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.
Comment 3 Adam Barth 2012-09-27 10:22:55 PDT
Apparently the example we have is really fragile and depends on details of the allocator and GC to trigger.
Comment 4 Kentaro Hara 2012-09-27 15:00:01 PDT
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 5 WebKit Review Bot 2012-09-27 15:04:51 PDT
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 6 WebKit Review Bot 2012-09-27 16:25:21 PDT
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 7 WebKit Review Bot 2012-09-27 16:52:37 PDT
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
Comment 8 Kentaro Hara 2012-09-27 16:53:09 PDT
hmm, let me try to land it later.
Comment 9 WebKit Review Bot 2012-09-27 21:27:27 PDT
Comment on attachment 165956 [details]
Patch

Clearing flags on attachment: 165956

Committed r129849: <http://trac.webkit.org/changeset/129849>
Comment 10 WebKit Review Bot 2012-09-27 21:27:30 PDT
All reviewed patches have been landed.  Closing bug.