Bug 94899 - [V8] StringCache should not return already disposed string
Summary: [V8] StringCache should not return already disposed string
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-08-23 22:37 PDT by Kentaro Hara
Modified: 2012-08-23 23:41 PDT (History)
3 users (show)

See Also:


Attachments
Patch (4.08 KB, patch)
2012-08-23 22:42 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-08-23 22:37:42 PDT
See this Chromium bug (http://code.google.com/p/chromium/issues/detail?id=143937) for details.

I investigated the crash and found that v8ExternalString() can return already disposed strings:

  class StringCache {
    v8::Local<v8::String> v8ExternalString(StringImpl* stringImpl, v8::Isolate* isolate)
    {
        if (m_lastStringImpl.get() == stringImpl) {
            ASSERT(!m_lastV8String.IsNearDeath());
            ASSERT(!m_lastV8String.IsEmpty());
            return v8::Local<v8::String>::New(m_lastV8String); // m_lastV8String might be already Disposed.
        }
        return v8ExternalStringSlow(stringImpl, isolate);
    }
  }

I couldn't find why m_lastV8String can be prematurely disposed, but the following fix will solve the crash:

  class StringCache {
    v8::Local<v8::String> v8ExternalString(StringImpl* stringImpl, v8::Isolate* isolate)
    {
        if (m_lastStringImpl.get() == stringImpl && m_lastV8String.IsWeak())
            return v8::Local<v8::String>::New(m_lastV8String);
        return v8ExternalStringSlow(stringImpl, isolate);
    }
  }

Although the ideal fix might be to fix the root cause of the premature disposal, I think that the above fix is reasonable for safety. In fact, we've so far encountered crashes caused by premature disposals (e.g. r123500). The above fix will prevent future crashes caused by premature disposals.
Comment 1 Kentaro Hara 2012-08-23 22:42:57 PDT
Created attachment 160330 [details]
Patch
Comment 2 Adam Barth 2012-08-23 22:57:43 PDT
Comment on attachment 160330 [details]
Patch

Ok.  This feels a bit like papering over the problem though.
Comment 3 WebKit Review Bot 2012-08-23 23:41:24 PDT
Comment on attachment 160330 [details]
Patch

Clearing flags on attachment: 160330

Committed r126547: <http://trac.webkit.org/changeset/126547>
Comment 4 WebKit Review Bot 2012-08-23 23:41:27 PDT
All reviewed patches have been landed.  Closing bug.