Bug 94899 – [V8] StringCache should not return already disposed string

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

[Kentaro Hara]

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.

[Kentaro Hara]

Created attachment 160330 [details]
Patch

[Adam Barth]

Comment on attachment 160330 [details]
Patch

Ok.  This feels a bit like papering over the problem though.

[WebKit Review Bot]

Comment on attachment 160330 [details]
Patch

Clearing flags on attachment: 160330

Committed r126547: <http://trac.webkit.org/changeset/126547>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.