Bug 97767 – [V8] StringCache::v8ExternalString() can return a stale persistent handle

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Kentaro Hara]

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.

[Kentaro Hara]

Created attachment 165956 [details]
Patch

[Adam Barth]

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.

[Adam Barth]

Apparently the example we have is really fragile and depends on details of the allocator and GC to trigger.

[Kentaro Hara]

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.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Kentaro Hara]

hmm, let me try to land it later.

[WebKit Review Bot]

Comment on attachment 165956 [details]
Patch

Clearing flags on attachment: 165956

Committed r129849: <http://trac.webkit.org/changeset/129849>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.