Title: [249133] trunk/Source/WebCore
- Revision
- 249133
- Author
- [email protected]
- Date
- 2019-08-26 23:51:49 -0700 (Mon, 26 Aug 2019)
Log 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 _javascript_Core.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 _javascript_Core.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._javascript_Core 0x000000010ee3d980 WTFCrash + 16
1 com.apple._javascript_Core 0x000000010ee408ab WTFCrashWithInfo(int, char const*, char const*, int) + 27
2 com.apple._javascript_Core 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:
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (249132 => 249133)
--- trunk/Source/WebCore/ChangeLog 2019-08-27 05:00:30 UTC (rev 249132)
+++ trunk/Source/WebCore/ChangeLog 2019-08-27 06:51:49 UTC (rev 249133)
@@ -1,3 +1,51 @@
+2019-08-26 Yusuke Suzuki <[email protected]>
+
+ [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 _javascript_Core.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 _javascript_Core.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._javascript_Core 0x000000010ee3d980 WTFCrash + 16
+ 1 com.apple._javascript_Core 0x000000010ee408ab WTFCrashWithInfo(int, char const*, char const*, int) + 27
+ 2 com.apple._javascript_Core 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:
+
2019-08-26 Devin Rousso <[email protected]>
Web Inspector: use more C++ keywords for defining agents
Modified: trunk/Source/WebCore/html/track/DataCue.cpp (249132 => 249133)
--- trunk/Source/WebCore/html/track/DataCue.cpp 2019-08-27 05:00:30 UTC (rev 249132)
+++ trunk/Source/WebCore/html/track/DataCue.cpp 2019-08-27 06:51:49 UTC (rev 249133)
@@ -33,7 +33,7 @@
#include "TextTrack.h"
#include "TextTrackCueList.h"
#include <_javascript_Core/JSCInlines.h>
-#include <_javascript_Core/Protect.h>
+#include <_javascript_Core/StrongInlines.h>
#include <wtf/IsoMallocInlines.h>
namespace WebCore {
@@ -64,16 +64,12 @@
DataCue::DataCue(ScriptExecutionContext& context, const MediaTime& start, const MediaTime& end, JSC::JSValue value, const String& type)
: TextTrackCue(context, start, end)
, m_type(type)
- , m_value(value)
+ , m_value(context.vm(), value)
{
- if (m_value)
- JSC::gcProtect(m_value);
}
DataCue::~DataCue()
{
- if (m_value)
- JSC::gcUnprotect(m_value);
}
RefPtr<ArrayBuffer> DataCue::data() const
@@ -90,10 +86,7 @@
void DataCue::setData(ArrayBuffer& data)
{
m_platformValue = nullptr;
- if (m_value)
- JSC::gcUnprotect(m_value);
- m_value = JSC::JSValue();
-
+ m_value.clear();
m_data = ArrayBuffer::create(data);
}
@@ -164,20 +157,15 @@
return m_platformValue->deserialize(&state);
if (m_value)
- return m_value;
+ return m_value.get();
return JSC::jsNull();
}
-void DataCue::setValue(JSC::ExecState&, JSC::JSValue value)
+void DataCue::setValue(JSC::ExecState& state, JSC::JSValue value)
{
// FIXME: this should use a SerializedScriptValue.
- if (m_value)
- JSC::gcUnprotect(m_value);
- m_value = value;
- if (m_value)
- JSC::gcProtect(m_value);
-
+ m_value.set(state.vm(), value);
m_platformValue = nullptr;
m_data = nullptr;
}
@@ -185,7 +173,7 @@
JSValue DataCue::valueOrNull() const
{
if (m_value)
- return m_value;
+ return m_value.get();
return jsNull();
}
Modified: trunk/Source/WebCore/html/track/DataCue.h (249132 => 249133)
--- trunk/Source/WebCore/html/track/DataCue.h 2019-08-27 05:00:30 UTC (rev 249132)
+++ trunk/Source/WebCore/html/track/DataCue.h 2019-08-27 06:51:49 UTC (rev 249133)
@@ -102,7 +102,10 @@
RefPtr<ArrayBuffer> m_data;
String m_type;
RefPtr<SerializedPlatformRepresentation> m_platformValue;
- JSC::JSValue m_value;
+ // FIXME: The following use of JSC::Strong is incorrect and can lead to storage leaks
+ // due to reference cycles; we should use JSValueInWrappedObject instead.
+ // https://bugs.webkit.org/show_bug.cgi?id=201173
+ JSC::Strong<JSC::Unknown> m_value;
};
DataCue* toDataCue(TextTrackCue*);
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes
