Commit 9f06edfe authored by haraken@chromium.org's avatar haraken@chromium.org
Browse files

[V8] String wrappers should be marked Independent

https://bugs.webkit.org/show_bug.cgi?id=91251

Reviewed by Adam Barth.

Currently V8 String wrappers are not marked Independent.
By marking them Independent, they can be reclaimed by the scavenger GC.
Although I couldn't find a case where this change reduces memory usage,
this change would be important for upcoming changes in string conversion
between V8 and WebKit (https://bugs.webkit.org/show_bug.cgi?id=91850).

'm_lastStringImpl = 0' in StringCache::remove() is important.
Look at the following code:

    static void cachedStringCallback(v8::Persistent<v8::Value> wrapper, void* parameter)
    {
        ...;
        stringCache()->remove(stringImpl);
        wrapper.Dispose();
    }

    void StringCache::remove(StringImpl* stringImpl)
    {
        ...
        if (m_lastStringImpl.get() == stringImpl)
            m_lastStringImpl = 0;
    }

    v8::Local<v8::String> v8ExternalString(StringImpl* stringImpl, v8::Isolate* isolate)
    {
        if (m_lastStringImpl.get() == stringImpl) {
            return v8::Local<v8::String>::New(m_lastV8String); // m_lastV8String points to a wrapper object that was accessed most recently.
        }
        return v8ExternalStringSlow(stringImpl, isolate);
    }

Without 'm_lastStringImpl = 0', already disposed m_lastV8String can be used
in v8ExternalString(). This was a cause of the crashes of r122614.

Tests: At the initial commit of this patch (r122614),
       the following tests had been broken due to missing 'm_lastStringImpl = 0'.
       fast/workers/worker-location.html
       Dromaeo/cssquery-jquery.html
       Dromaeo/jslib-event-jquery.html
       Dromaeo/jslib-style-jquery.html
       Dromaeo/jslib-style-prototype.html

* bindings/v8/V8Binding.cpp:
(WebCore::StringCache::remove):
(WebCore::StringCache::v8ExternalStringSlow):


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@123500 268f45cc-cd09-0410-ab3c-d52691b4dbfc
parent 887c7bfe
2012-07-24 Kentaro Hara <haraken@chromium.org>
[V8] String wrappers should be marked Independent
https://bugs.webkit.org/show_bug.cgi?id=91251
Reviewed by Adam Barth.
Currently V8 String wrappers are not marked Independent.
By marking them Independent, they can be reclaimed by the scavenger GC.
Although I couldn't find a case where this change reduces memory usage,
this change would be important for upcoming changes in string conversion
between V8 and WebKit (https://bugs.webkit.org/show_bug.cgi?id=91850).
'm_lastStringImpl = 0' in StringCache::remove() is important.
Look at the following code:
static void cachedStringCallback(v8::Persistent<v8::Value> wrapper, void* parameter)
{
...;
stringCache()->remove(stringImpl);
wrapper.Dispose();
}
void StringCache::remove(StringImpl* stringImpl)
{
...
if (m_lastStringImpl.get() == stringImpl)
m_lastStringImpl = 0;
}
v8::Local<v8::String> v8ExternalString(StringImpl* stringImpl, v8::Isolate* isolate)
{
if (m_lastStringImpl.get() == stringImpl) {
return v8::Local<v8::String>::New(m_lastV8String); // m_lastV8String points to a wrapper object that was accessed most recently.
}
return v8ExternalStringSlow(stringImpl, isolate);
}
Without 'm_lastStringImpl = 0', already disposed m_lastV8String can be used
in v8ExternalString(). This was a cause of the crashes of r122614.
Tests: At the initial commit of this patch (r122614),
the following tests had been broken due to missing 'm_lastStringImpl = 0'.
fast/workers/worker-location.html
Dromaeo/cssquery-jquery.html
Dromaeo/jslib-event-jquery.html
Dromaeo/jslib-style-jquery.html
Dromaeo/jslib-style-prototype.html
* bindings/v8/V8Binding.cpp:
(WebCore::StringCache::remove):
(WebCore::StringCache::v8ExternalStringSlow):
2012-07-24 Tommy Widenflycht <tommyw@google.com>
 
MediaStream API: Update MediaStreamTrack to match the specification
......@@ -467,6 +467,10 @@ void StringCache::remove(StringImpl* stringImpl)
{
ASSERT(m_stringCache.contains(stringImpl));
m_stringCache.remove(stringImpl);
// Make sure that already disposed m_lastV8String is not used in
// StringCache::v8ExternalString().
if (m_lastStringImpl.get() == stringImpl)
m_lastStringImpl = 0;
}
v8::Local<v8::String> StringCache::v8ExternalStringSlow(StringImpl* stringImpl, v8::Isolate* isolate)
......@@ -493,6 +497,7 @@ v8::Local<v8::String> StringCache::v8ExternalStringSlow(StringImpl* stringImpl,
return newString;
stringImpl->ref();
wrapper.MarkIndependent();
wrapper.MakeWeak(stringImpl, cachedStringCallback);
m_stringCache.set(stringImpl, *wrapper);
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment