Bug 215221 – WeakPtr threading assertion on editing/undo-manager/undo-manager-delete-stale-undo-items.html

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

[Hector Lopez]

editing/undo-manager/undo-manager-delete-stale-undo-items.html

This test is a flaky crash on macOS Debug/gpuprocess and iOS wk2 Debug according to history. First occurrence of a crash on iOS is at r262052. The first occurrence of a crash on macOS is at r259705.

History:
https://results.webkit.org/?suite=layout-tests&test=editing%2Fundo-manager%2Fundo-manager-delete-stale-undo-items.html&limit=50000&platform=ios&platform=mac&style=debug

Crash Log:
https://build.webkit.org/results/Apple%20iPadOS%2013%20Simulator%20Debug%20WK2%20(Tests)/r265322%20(3046)/editing/undo-manager/undo-manager-delete-stale-undo-items-crash-log.txt

Application Specific Information:
CoreSimulator 704.12 - Device: Managed 4 (425092F4-AB17-487D-8C79-D022DCAE0881) - Runtime: iOS 13.4 (17E255) - DeviceType: iPad (5th generation)
CRASHING TEST: editing/undo-manager/undo-manager-delete-stale-undo-items.html

Thread 0:: Dispatch queue: com.apple.main-thread
0   libsystem_kernel.dylib        	0x00000002468ff882 __psynch_cvwait + 10
1   libsystem_pthread.dylib       	0x0000000246961425 _pthread_cond_wait + 698
2   com.apple.JavaScriptCore      	0x000000024dc2cd00 WTF::ThreadCondition::wait(WTF::Mutex&) + 48 (ThreadingPOSIX.cpp:518)
3   com.apple.JavaScriptCore      	0x000000024dc2cdf5 WTF::ThreadCondition::timedWait(WTF::Mutex&, WTF::WallTime) + 133 (ThreadingPOSIX.cpp:529)
4   com.apple.JavaScriptCore      	0x000000024dbc95fb WTF::ParkingLot::parkConditionallyImpl(void const*, WTF::ScopedLambda<bool ()> const&, WTF::ScopedLambda<void ()> const&, WTF::TimeWithDynamicClockType const&) + 427 (ParkingLot.cpp:602)
5   com.apple.JavaScriptCore      	0x000000024db51900 WTF::ParkingLot::ParkResult WTF::ParkingLot::parkConditionally<bool WTF::Condition::waitUntil<WTF::Lock>(WTF::Lock&, WTF::TimeWithDynamicClockType const&)::'lambda'(), bool WTF::Condition::waitUntil<WTF::Lock>(WTF::Lock&, WTF::TimeWithDynamicClockType const&)::'lambda0'()>(void const*, WTF::Lock const&, bool WTF::Condition::waitUntil<WTF::Lock>(WTF::Lock&, WTF::TimeWithDynamicClockType const&)::'lambda0'() const&, WTF::TimeWithDynamicClockType const&) + 96 (ParkingLot.h:82)
6   com.apple.JavaScriptCore      	0x000000024db5180c bool WTF::Condition::waitUntil<WTF::Lock>(WTF::Lock&, WTF::TimeWithDynamicClockType const&) + 140 (Condition.h:76)

[Radar WebKit Bug Importer]

<rdar://problem/66632111>

[Alexey Proskuryakov]

It's a different thread crashing:

Thread 11 Crashed:: Heap Helper Thread
0   com.apple.JavaScriptCore      	0x000000024db3cb8e WTFCrash + 14 (Assertions.cpp:295)
1   com.apple.WebCore             	0x000000025532237b WTFCrashWithInfo(int, char const*, char const*, int) + 27
2   com.apple.WebCore             	0x0000000258be6939 WTF::WeakPtr<WebCore::UndoManager, WTF::EmptyCounter>::operator->() const + 153 (WeakPtr.h:107)
3   com.apple.WebCore             	0x0000000258be6883 WebCore::UndoItem::document() const + 67 (UndoItem.cpp:64)
4   com.apple.WebCore             	0x000000025769ddaf WebCore::JSUndoItemOwner::isReachableFromOpaqueRoots(JSC::Handle<JSC::Unknown>, void*, JSC::SlotVisitor&, char const**) + 111 (JSUndoItemCustom.cpp:43)
5   com.apple.JavaScriptCore      	0x000000024edeb238 void JSC::WeakBlock::specializedVisit<JSC::PreciseAllocation>(JSC::PreciseAllocation&, JSC::SlotVisitor&) + 360 (WeakBlock.cpp:125)
6   com.apple.JavaScriptCore      	0x000000024edeb072 JSC::WeakBlock::visit(JSC::SlotVisitor&) + 194 (WeakBlock.cpp:147)
7   com.apple.JavaScriptCore      	0x000000024edd306e JSC::WeakSet::visit(JSC::SlotVisitor&) + 62 (WeakSet.h:117)
8   com.apple.JavaScriptCore      	0x000000024edd3020 JSC::MarkedSpace::visitWeakSets(JSC::SlotVisitor&)::$_10::operator()(JSC::WeakSet*) const + 32 (MarkedSpace.cpp:279)
9   com.apple.JavaScriptCore      	0x000000024edc340f void WTF::SentinelLinkedList<JSC::WeakSet, WTF::BasicRawSentinelNode<JSC::WeakSet, WTF::DumbPtrTraits<JSC::WeakSet> > >::forEach<JSC::MarkedSpace::visitWeakSets(JSC::SlotVisitor&)::$_10>(JSC::MarkedSpace::visitWeakSets(JSC::SlotVisitor&)::$_10 const&) + 95 (SentinelLinkedList.h:108)
10  com.apple.JavaScriptCore      	0x000000024edc3356 JSC::MarkedSpace::visitWeakSets(JSC::SlotVisitor&) + 54 (MarkedSpace.cpp:281)
11  com.apple.JavaScriptCore      	0x000000024ed7b643 JSC::Heap::addCoreConstraints()::$_35::operator()(JSC::SlotVisitor&) const + 67 (Heap.cpp:2805)
12  com.apple.JavaScriptCore      	0x000000024ed7b5d3 WTF::Detail::CallableWrapper<JSC::Heap::addCoreConstraints()::$_35, void, JSC::SlotVisitor&>::call(JSC::SlotVisitor&) + 51 (Function.h:52)
13  com.apple.JavaScriptCore      	0x000000024eddcb0a WTF::Function<void (JSC::SlotVisitor&)>::operator()(JSC::SlotVisitor&) const + 154 (Function.h:83)

[Wenson Hsieh]

Created attachment 406122 [details]
Patch

[Devin Rousso]

Comment on attachment 406122 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=406122&action=review

r=me

> Source/WebCore/page/UndoItem.cpp:44
> +    if (undoManager)

NIT: perhaps use a ternary `m_document = undoManager ? makeWeakPtr(undoManager->document()) : { };`?

> Source/WebCore/page/UndoItem.cpp:55
> +    m_undoManager = nullptr;
> +    m_document = nullptr;

I wonder if there's any significant difference/preference between ` = nullptr` or `.clear()` 🤔

[Wenson Hsieh]

Thanks for the review!

(In reply to Devin Rousso from comment #4)
> Comment on attachment 406122 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=406122&action=review
> 
> r=me
> 
> > Source/WebCore/page/UndoItem.cpp:44
> > +    if (undoManager)
> 
> NIT: perhaps use a ternary `m_document = undoManager ?
> makeWeakPtr(undoManager->document()) : { };`?

I wanted to do that originally, but it leads to:

./page/UndoItem.cpp:44:69: error: initializer list cannot be used on the right hand side of operator ':'
    m_document = undoManager ? makeWeakPtr(undoManager->document()) : { };

I suppose I could make it something like this instead:

m_document = undoManager ? makeWeakPtr(undoManager->document()) : WeakPtr<Document> { };

> 
> > Source/WebCore/page/UndoItem.cpp:55
> > +    m_undoManager = nullptr;
> > +    m_document = nullptr;
> 
> I wonder if there's any significant difference/preference between ` =
> nullptr` or `.clear()` 🤔

Now that I think about it, *maybe* ::clear() is a tiny bit more performant? (since it just destroys the inner m_impl instead of having to create a new WeakPtr). I’ll change this to clear().

[Wenson Hsieh]

Created attachment 406127 [details]
Patch for landing

[EWS]

Committed r265354: <https://trac.webkit.org/changeset/265354>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 406127 [details].