| Summary: | WeakPtr threading assertion on editing/undo-manager/undo-manager-delete-stale-undo-items.html | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Hector Lopez <hector_i_lopez> | ||||||
| Component: | WebCore JavaScript | Assignee: | Wenson Hsieh <wenson_hsieh> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | cdumez, ddkilzer, hi, megan_gardner, saam, thorton, webkit-bot-watchers-bugzilla, webkit-bug-importer, wenson_hsieh, ysuzuki | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Hector Lopez
2020-08-06 10:01:42 PDT
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) Created attachment 406122 [details]
Patch
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()` 🤔 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(). Created attachment 406127 [details]
Patch for landing
Committed r265354: <https://trac.webkit.org/changeset/265354> All reviewed patches have been landed. Closing bug and clearing flags on attachment 406127 [details]. |