Title: [261545] trunk/Source/WTF
- Revision
- 261545
- Author
- ysuz...@apple.com
- Date
- 2020-05-11 22:21:28 -0700 (Mon, 11 May 2020)
Log Message
AtomString::init should temporarily disable checks via `isMainThread` due to WebThread's inconsistent state
https://bugs.webkit.org/show_bug.cgi?id=211754
Reviewed by Dean Jackson.
When starting WebThread via StartWebThread function, we have special period between the prologue of StartWebThread and spawning WebThread actually.
In this period, `isMainThread()` returns false even if this is called on the main thread since WebThread is now enabled and we are not taking a WebThread lock.
This causes assertion hits in MainThreadLazyNeverDestroyed initialization only in WebThread platforms.
This patch takes conservative approach to fix this assertion hits: just bypass these assertions since this should not be fired. We bypass assertions by using
constructWithoutAccessCheck, which intentionally skips `isMainThread()` check for construction. In non WebThread environment, we do not lose the assertion
coverage since we already have ASSERT(isUIThread()). And ASSERT(isUIThread()) ensures that this is called in system main thread in WebThread platforms.
Unfortunately, we have no mechanism to test this effectively. WK1 LayoutTests runner cannot show assertions since WK1 runners already initialize necessary
things somewhere before calling AtomString::init() in StartWebThread. This is the same in TestWebKitAPI. For now, I manually tested that assertion does not fire.
* wtf/NeverDestroyed.h:
(WTF::LazyNeverDestroyed::construct):
(WTF::LazyNeverDestroyed::constructWithoutAccessCheck):
(WTF::LazyNeverDestroyed::storagePointerWithoutAccessCheck const):
(WTF::LazyNeverDestroyed::storagePointer const):
* wtf/text/AtomString.cpp:
(WTF::AtomString::init):
Modified Paths
Diff
Modified: trunk/Source/WTF/ChangeLog (261544 => 261545)
--- trunk/Source/WTF/ChangeLog 2020-05-12 03:52:22 UTC (rev 261544)
+++ trunk/Source/WTF/ChangeLog 2020-05-12 05:21:28 UTC (rev 261545)
@@ -1,3 +1,29 @@
+2020-05-11 Yusuke Suzuki <ysuz...@apple.com>
+
+ AtomString::init should temporarily disable checks via `isMainThread` due to WebThread's inconsistent state
+ https://bugs.webkit.org/show_bug.cgi?id=211754
+
+ Reviewed by Dean Jackson.
+
+ When starting WebThread via StartWebThread function, we have special period between the prologue of StartWebThread and spawning WebThread actually.
+ In this period, `isMainThread()` returns false even if this is called on the main thread since WebThread is now enabled and we are not taking a WebThread lock.
+ This causes assertion hits in MainThreadLazyNeverDestroyed initialization only in WebThread platforms.
+
+ This patch takes conservative approach to fix this assertion hits: just bypass these assertions since this should not be fired. We bypass assertions by using
+ constructWithoutAccessCheck, which intentionally skips `isMainThread()` check for construction. In non WebThread environment, we do not lose the assertion
+ coverage since we already have ASSERT(isUIThread()). And ASSERT(isUIThread()) ensures that this is called in system main thread in WebThread platforms.
+
+ Unfortunately, we have no mechanism to test this effectively. WK1 LayoutTests runner cannot show assertions since WK1 runners already initialize necessary
+ things somewhere before calling AtomString::init() in StartWebThread. This is the same in TestWebKitAPI. For now, I manually tested that assertion does not fire.
+
+ * wtf/NeverDestroyed.h:
+ (WTF::LazyNeverDestroyed::construct):
+ (WTF::LazyNeverDestroyed::constructWithoutAccessCheck):
+ (WTF::LazyNeverDestroyed::storagePointerWithoutAccessCheck const):
+ (WTF::LazyNeverDestroyed::storagePointer const):
+ * wtf/text/AtomString.cpp:
+ (WTF::AtomString::init):
+
2020-05-11 Basuke Suzuki <basuke.suz...@sony.com>
[bmalloc][WTF] Add computing memory size implementation for FreeBSD
Modified: trunk/Source/WTF/wtf/NeverDestroyed.h (261544 => 261545)
--- trunk/Source/WTF/wtf/NeverDestroyed.h 2020-05-12 03:52:22 UTC (rev 261544)
+++ trunk/Source/WTF/wtf/NeverDestroyed.h 2020-05-12 05:21:28 UTC (rev 261545)
@@ -118,14 +118,18 @@
template<typename... Args>
void construct(Args&&... args)
{
- ASSERT(!m_isConstructed);
AccessTraits::assertAccess();
+ constructWithoutAccessCheck(std::forward<Args>(args)...);
+ }
+ template<typename... Args>
+ void constructWithoutAccessCheck(Args&&... args)
+ {
+ ASSERT(!m_isConstructed);
#if ASSERT_ENABLED
m_isConstructed = true;
#endif
-
- MaybeRelax<T>(new (storagePointer()) T(std::forward<Args>(args)...));
+ MaybeRelax<T>(new (storagePointerWithoutAccessCheck()) T(std::forward<Args>(args)...));
}
operator T&() { return *storagePointer(); }
@@ -145,13 +149,18 @@
private:
using PointerType = typename std::remove_const<T>::type*;
- PointerType storagePointer() const
+ PointerType storagePointerWithoutAccessCheck() const
{
ASSERT(m_isConstructed);
- AccessTraits::assertAccess();
return const_cast<PointerType>(reinterpret_cast<const T*>(&m_storage));
}
+ PointerType storagePointer() const
+ {
+ AccessTraits::assertAccess();
+ return storagePointerWithoutAccessCheck();
+ }
+
template<typename PtrType, bool ShouldRelax = std::is_base_of<RefCountedBase, PtrType>::value> struct MaybeRelax {
explicit MaybeRelax(PtrType*) { }
};
Modified: trunk/Source/WTF/wtf/text/AtomString.cpp (261544 => 261545)
--- trunk/Source/WTF/wtf/text/AtomString.cpp 2020-05-12 03:52:22 UTC (rev 261544)
+++ trunk/Source/WTF/wtf/text/AtomString.cpp 2020-05-12 05:21:28 UTC (rev 261545)
@@ -148,9 +148,16 @@
nullAtomData.construct();
emptyAtomData.construct("");
- starAtomData.construct("*", AtomString::ConstructFromLiteral);
- xmlAtomData.construct("xml", AtomString::ConstructFromLiteral);
- xmlnsAtomData.construct("xmlns", AtomString::ConstructFromLiteral);
+
+ // When starting WebThread via StartWebThread function, we have special period between the prologue of StartWebThread and spawning WebThread actually.
+ // In this period, `isMainThread()` returns false even if this is called on the main thread since WebThread is now enabled and we are not taking a WebThread lock.
+ // This causes assertion hits in MainThreadLazyNeverDestroyed initialization only in WebThread platforms.
+ // We bypass this by using constructWithoutAccessCheck, which intentionally skips `isMainThread()` check for construction.
+ // In non WebThread environment, we do not lose the assertion coverage since we already have ASSERT(isUIThread()). And ASSERT(isUIThread()) ensures that this
+ // is called in system main thread in WebThread platforms.
+ starAtomData.constructWithoutAccessCheck("*", AtomString::ConstructFromLiteral);
+ xmlAtomData.constructWithoutAccessCheck("xml", AtomString::ConstructFromLiteral);
+ xmlnsAtomData.constructWithoutAccessCheck("xmlns", AtomString::ConstructFromLiteral);
});
}
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes
