⚠ Archived content — this site is no longer maintained.   Current WebKit documentation is at docs.webkit.org.

Changeset 249133 in webkit


Ignore:
Timestamp:
Aug 26, 2019, 11:51:49 PM (7 years ago)
Author:
ysuzuki@apple.com
Message:

[WebCore] DataCue should not use gcProtect / gcUnprotect
​https://bugs.webkit.org/show_bug.cgi?id=201170

Reviewed by Mark Lam.

JSC::gcProtect and JSC::gcUnprotect are designed for JavaScriptCore.framework and we should not use them in WebCore. It is
checking whether we are holding a JS API lock. But the caller of these API would be the C++ holder's destructor, and this should be
allowed since this destruction must happen in main thread or web thread, and this should not happen while other thread is taking JS API lock.
For example, we are destroying JSC::Strong<>, JSC::Weak<> without taking JS API lock. But since JSC::gcProtect and JSC::gcUnprotect are designed
for JavaScriptCore.framework, they are not accounting this condition, and we are hitting debug assertion in GC stress bot.

Ideally, we should convert this JSValue field to JSValueInWrappedObject. But JSValueInWrappedObject needs extra care. We should
know how the owner and JS wrapper are kept and used to use JSValueInWrappedObject correctly.

As a first step, this patch just replaces raw JSValue + gcProtect/gcUnprotect with JSC::Strong<>.

This change fixes LayoutTests/media/track/track-in-band-metadata-display-order.html crash in GC stress bot. The crash trace is the following.

Thread 0 Crashed
Dispatch queue: com.apple.main-thread 0 com.apple.JavaScriptCore 0x000000010ee3d980 WTFCrash + 16 1 com.apple.JavaScriptCore 0x000000010ee408ab WTFCrashWithInfo(int, char const*, char const*, int) + 27 2 com.apple.JavaScriptCore 0x000000010feb5327 JSC::Heap::unprotect(JSC::JSValue) + 215 3 com.apple.WebCore 0x0000000120f33b53 JSC::gcUnprotect(JSC::JSCell*) + 51 4 com.apple.WebCore 0x0000000120f329fc JSC::gcUnprotect(JSC::JSValue) + 76 5 com.apple.WebCore 0x0000000120f32968 WebCore::DataCue::~DataCue() + 88 6 com.apple.WebCore 0x0000000120f32ac5 WebCore::DataCue::~DataCue() + 21 7 com.apple.WebCore 0x0000000120f32ae9 WebCore::DataCue::~DataCue() + 25 8 com.apple.WebCore 0x0000000120f37ebf WTF::RefCounted<WebCore::TextTrackCue, std::__1::default_delete<WebCore::TextTrackCue> >::deref() const + 95 9 com.apple.WebCore 0x000000012103a345 void WTF::derefIfNotNull<WebCore::TextTrackCue>(WebCore::TextTrackCue*) + 53 10 com.apple.WebCore 0x000000012103a309 WTF::RefPtr<WebCore::TextTrackCue, WTF::DumbPtrTraits<WebCore::TextTrackCue> >::~RefPtr() + 41 11 com.apple.WebCore 0x000000012102bfc5 WTF::RefPtr<WebCore::TextTrackCue, WTF::DumbPtrTraits<WebCore::TextTrackCue> >::~RefPtr() + 21 12 com.apple.WebCore 0x00000001210e91df WTF::VectorDestructor<true, WTF::RefPtr<WebCore::TextTrackCue, WTF::DumbPtrTraits<WebCore::TextTrackCue> > >::destruct(WTF::RefPtr<WebCore::TextTrackCue, WTF::DumbPtrTraits<WebCore::TextTrackCue> >*, WTF::RefPtr<WebCore::TextTrackCue, WTF::DumbPtrTraits<WebCore::TextTrackCue> >*) + 47 13 com.apple.WebCore 0x00000001210e913d WTF::VectorTypeOperations<WTF::RefPtr<WebCore::TextTrackCue, WTF::DumbPtrTraits<WebCore::TextTrackCue> > >::destruct(WTF::RefPtr<WebCore::TextTrackCue, WTF::DumbPtrTraits<WebCore::TextTrackCue> >*, WTF::RefPtr<WebCore::TextTrackCue, WTF::DumbPtrTraits<WebCore::TextTrackCue> >*) + 29 14 com.apple.WebCore 0x00000001210e9100 WTF::Vector<WTF::RefPtr<WebCore::TextTrackCue, WTF::DumbPtrTraits<WebCore::TextTrackCue> >, 0ul, WTF::CrashOnOverflow, 16ul>::~Vector() + 64 15 com.apple.WebCore 0x00000001210e7a25 WTF::Vector<WTF::RefPtr<WebCore::TextTrackCue, WTF::DumbPtrTraits<WebCore::TextTrackCue> >, 0ul, WTF::CrashOnOverflow, 16ul>::~Vector() + 21 16 com.apple.WebCore 0x00000001210e93d3 WebCore::TextTrackCueList::~TextTrackCueList() + 51
  • html/track/DataCue.cpp:

(WebCore::DataCue::DataCue):
(WebCore::DataCue::~DataCue):
(WebCore::DataCue::setData):
(WebCore::DataCue::value const):
(WebCore::DataCue::setValue):
(WebCore::DataCue::valueOrNull const):

  • html/track/DataCue.h:
Location:
trunk/Source/WebCore
Files:
3 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebCore/ChangeLog

    r249132 r249133  
     12019-08-26  Yusuke Suzuki  <ysuzuki@apple.com>
     2
     3        [WebCore] DataCue should not use gcProtect / gcUnprotect
     4        https://bugs.webkit.org/show_bug.cgi?id=201170
     5
     6        Reviewed by Mark Lam.
     7
     8        JSC::gcProtect and JSC::gcUnprotect are designed for JavaScriptCore.framework and we should not use them in WebCore. It is
     9        checking whether we are holding a JS API lock. But the caller of these API would be the C++ holder's destructor, and this should be
     10        allowed since this destruction must happen in main thread or web thread, and this should not happen while other thread is taking JS API lock.
     11        For example, we are destroying JSC::Strong<>, JSC::Weak<> without taking JS API lock. But since JSC::gcProtect and JSC::gcUnprotect are designed
     12        for JavaScriptCore.framework, they are not accounting this condition, and we are hitting debug assertion in GC stress bot.
     13
     14        Ideally, we should convert this JSValue field to JSValueInWrappedObject. But JSValueInWrappedObject needs extra care. We should
     15        know how the owner and JS wrapper are kept and used to use JSValueInWrappedObject correctly.
     16
     17        As a first step, this patch just replaces raw JSValue + gcProtect/gcUnprotect with JSC::Strong<>.
     18
     19        This change fixes LayoutTests/media/track/track-in-band-metadata-display-order.html crash in GC stress bot. The crash trace is the following.
     20
     21            Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
     22            0   com.apple.JavaScriptCore          0x000000010ee3d980 WTFCrash + 16
     23            1   com.apple.JavaScriptCore          0x000000010ee408ab WTFCrashWithInfo(int, char const*, char const*, int) + 27
     24            2   com.apple.JavaScriptCore          0x000000010feb5327 JSC::Heap::unprotect(JSC::JSValue) + 215
     25            3   com.apple.WebCore                 0x0000000120f33b53 JSC::gcUnprotect(JSC::JSCell*) + 51
     26            4   com.apple.WebCore                 0x0000000120f329fc JSC::gcUnprotect(JSC::JSValue) + 76
     27            5   com.apple.WebCore                 0x0000000120f32968 WebCore::DataCue::~DataCue() + 88
     28            6   com.apple.WebCore                 0x0000000120f32ac5 WebCore::DataCue::~DataCue() + 21
     29            7   com.apple.WebCore                 0x0000000120f32ae9 WebCore::DataCue::~DataCue() + 25
     30            8   com.apple.WebCore                 0x0000000120f37ebf WTF::RefCounted<WebCore::TextTrackCue, std::__1::default_delete<WebCore::TextTrackCue> >::deref() const + 95
     31            9   com.apple.WebCore                 0x000000012103a345 void WTF::derefIfNotNull<WebCore::TextTrackCue>(WebCore::TextTrackCue*) + 53
     32            10  com.apple.WebCore                 0x000000012103a309 WTF::RefPtr<WebCore::TextTrackCue, WTF::DumbPtrTraits<WebCore::TextTrackCue> >::~RefPtr() + 41
     33            11  com.apple.WebCore                 0x000000012102bfc5 WTF::RefPtr<WebCore::TextTrackCue, WTF::DumbPtrTraits<WebCore::TextTrackCue> >::~RefPtr() + 21
     34            12  com.apple.WebCore                 0x00000001210e91df WTF::VectorDestructor<true, WTF::RefPtr<WebCore::TextTrackCue, WTF::DumbPtrTraits<WebCore::TextTrackCue> > >::destruct(WTF::RefPtr<WebCore::TextTrackCue, WTF::DumbPtrTraits<WebCore::TextTrackCue> >*, WTF::RefPtr<WebCore::TextTrackCue, WTF::DumbPtrTraits<WebCore::TextTrackCue> >*) + 47
     35            13  com.apple.WebCore                 0x00000001210e913d WTF::VectorTypeOperations<WTF::RefPtr<WebCore::TextTrackCue, WTF::DumbPtrTraits<WebCore::TextTrackCue> > >::destruct(WTF::RefPtr<WebCore::TextTrackCue, WTF::DumbPtrTraits<WebCore::TextTrackCue> >*, WTF::RefPtr<WebCore::TextTrackCue, WTF::DumbPtrTraits<WebCore::TextTrackCue> >*) + 29
     36            14  com.apple.WebCore                 0x00000001210e9100 WTF::Vector<WTF::RefPtr<WebCore::TextTrackCue, WTF::DumbPtrTraits<WebCore::TextTrackCue> >, 0ul, WTF::CrashOnOverflow, 16ul>::~Vector() + 64
     37            15  com.apple.WebCore                 0x00000001210e7a25 WTF::Vector<WTF::RefPtr<WebCore::TextTrackCue, WTF::DumbPtrTraits<WebCore::TextTrackCue> >, 0ul, WTF::CrashOnOverflow, 16ul>::~Vector() + 21
     38            16  com.apple.WebCore                 0x00000001210e93d3 WebCore::TextTrackCueList::~TextTrackCueList() + 51
     39
     40        * html/track/DataCue.cpp:
     41        (WebCore::DataCue::DataCue):
     42        (WebCore::DataCue::~DataCue):
     43        (WebCore::DataCue::setData):
     44        (WebCore::DataCue::value const):
     45        (WebCore::DataCue::setValue):
     46        (WebCore::DataCue::valueOrNull const):
     47        * html/track/DataCue.h:
     48
    1492019-08-26  Devin Rousso  <drousso@apple.com>
    250

  • trunk/Source/WebCore/html/track/DataCue.cpp

    r243887 r249133  
    3434#include "TextTrackCueList.h"
    3535#include <JavaScriptCore/JSCInlines.h>
    36 #include <JavaScriptCore/Protect.h>
     36#include <JavaScriptCore/StrongInlines.h>
    3737#include <wtf/IsoMallocInlines.h>
    3838
    … …  
    6565    : TextTrackCue(context, start, end)
    6666    , m_type(type)
    67     , m_value(value)
     67    , m_value(context.vm(), value)
    6868{
    69     if (m_value)
    70         JSC::gcProtect(m_value);
    7169}
    7270
    7371DataCue::~DataCue()
    7472{
    75     if (m_value)
    76         JSC::gcUnprotect(m_value);
    7773}
    7874
    … …  
    9187{
    9288    m_platformValue = nullptr;
    93     if (m_value)
    94         JSC::gcUnprotect(m_value);
    95     m_value = JSC::JSValue();
    96 
     89    m_value.clear();
    9790    m_data = ArrayBuffer::create(data);
    9891}
    … …  
    165158
    166159    if (m_value)
    167         return m_value;
     160        return m_value.get();
    168161
    169162    return JSC::jsNull();
    170163}
    171164
    172 void DataCue::setValue(JSC::ExecState&, JSC::JSValue value)
     165void DataCue::setValue(JSC::ExecState& state, JSC::JSValue value)
    173166{
    174167    // FIXME: this should use a SerializedScriptValue.
    175     if (m_value)
    176         JSC::gcUnprotect(m_value);
    177     m_value = value;
    178     if (m_value)
    179         JSC::gcProtect(m_value);
    180 
     168    m_value.set(state.vm(), value);
    181169    m_platformValue = nullptr;
    182170    m_data = nullptr;
    … …  
    186174{
    187175    if (m_value)
    188         return m_value;
     176        return m_value.get();
    189177
    190178    return jsNull();

  • trunk/Source/WebCore/html/track/DataCue.h

    r244078 r249133  
    103103    String m_type;
    104104    RefPtr<SerializedPlatformRepresentation> m_platformValue;
    105     JSC::JSValue m_value;
     105    // FIXME: The following use of JSC::Strong is incorrect and can lead to storage leaks
     106    // due to reference cycles; we should use JSValueInWrappedObject instead.
     107    // https://bugs.webkit.org/show_bug.cgi?id=201173
     108    JSC::Strong<JSC::Unknown> m_value;
    106109};
    107110
Note: See TracChangeset for help on using the changeset viewer.