Make ThreadTimers' timer heap use WeakPtr instead of a raw pointer.
<rdar://problem/46893946>
Created attachment 357931 [details] WIP This replaces ThreadTimer's timer heap by WeakPtr but this still causes a null pointer crash in TimerHeapLessThanFunction::compare when WeakPtr is cleared.
Attachment 357931 [details] did not pass style-queue: ERROR: Source/WebCore/platform/Timer.cpp:61: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] ERROR: Source/WebCore/platform/Timer.cpp:188: The parameter name "a" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/platform/Timer.cpp:188: The parameter name "b" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 3 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 357980 [details] WIP2
Attachment 357980 [details] did not pass style-queue: ERROR: Source/WebCore/platform/Timer.cpp:95: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] ERROR: Source/WebCore/platform/Timer.h:37: Bad include order. Mixing system and custom headers. [build/include_order] [4] Total errors found: 2 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 358010 [details] WIP3
Attachment 358010 [details] did not pass style-queue: ERROR: Source/WebCore/platform/Timer.cpp:76: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] ERROR: Source/WebCore/platform/Timer.cpp:430: Use 'WTF::Optional<>' instead of 'std::optional<>'. [runtime/wtf_optional] [4] ERROR: Source/WebCore/platform/Timer.cpp:445: Use 'WTF::Optional<>' instead of 'std::optional<>'. [runtime/wtf_optional] [4] ERROR: Source/WebCore/platform/Timer.h:37: Bad include order. Mixing system and custom headers. [build/include_order] [4] Total errors found: 4 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 358055 [details] Adds the hardening
Comment on attachment 358055 [details] Adds the hardening View in context: https://bugs.webkit.org/attachment.cgi?id=358055&action=review > Source/WebCore/platform/Timer.cpp:85 > + RefPtr<ThreadTimerHeapItem>* m_pointer; What is the point of using a RefPtr? > Source/WebCore/platform/Timer.cpp:107 > + RefPtr<ThreadTimerHeapItem>& m_reference; What is the point of using a RefPtr? > Source/WebCore/platform/Timer.cpp:138 > + RefPtr<ThreadTimerHeapItem> timerA = a.copyRef(); Why the ref’ing? here and the line below? Where are these deref’ed()?
Comment on attachment 358055 [details] Adds the hardening View in context: https://bugs.webkit.org/attachment.cgi?id=358055&action=review >> Source/WebCore/platform/Timer.cpp:85 >> + RefPtr<ThreadTimerHeapItem>* m_pointer; > > What is the point of using a RefPtr? To match the type of the heap, which is now Vector<RefPtr<ThreadTimerHeapItem>>. >> Source/WebCore/platform/Timer.cpp:107 >> + RefPtr<ThreadTimerHeapItem>& m_reference; > > What is the point of using a RefPtr? Ditto. >> Source/WebCore/platform/Timer.cpp:138 >> + RefPtr<ThreadTimerHeapItem> timerA = a.copyRef(); > > Why the ref’ing? here and the line below? Where are these deref’ed()? I guess we could use std::exchange here.
Comment on attachment 358055 [details] Adds the hardening View in context: https://bugs.webkit.org/attachment.cgi?id=358055&action=review r=me Kinda of a bummer that it's still pretty complicated, but seems to give us the robustness we want. > Source/WebCore/platform/ThreadTimers.cpp:82 > + if (m_firingTimers) { > + m_pendingSharedTimerFireTime = MonotonicTime { }; > + m_sharedTimer->stop(); > + } We can just move this down below the null timer removal, and keep the old shared behavior. > Source/WebCore/platform/ThreadTimers.cpp:84 > + RefPtr<ThreadTimerHeapItem> firstItemWithValidTimer; We don't need this out of band state. We can just use m_timerHeap.first().
Created attachment 358766 [details] Patch for landing
Comment on attachment 358766 [details] Patch for landing Wait for EWS.
Created attachment 358771 [details] Patch for landing
Comment on attachment 358771 [details] Patch for landing Wait for EWS.
Attachment 358771 [details] did not pass style-queue: ERROR: Source/WebCore/platform/Timer.cpp:110: The parameter name "a" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 1 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 358771 [details] Patch for landing Clearing flags on attachment: 358771 Committed r239814: <https://trac.webkit.org/changeset/239814>
All reviewed patches have been landed. Closing bug.
(In reply to Ryosuke Niwa from comment #10) > Comment on attachment 358055 [details] > Adds the hardening > > View in context: > https://bugs.webkit.org/attachment.cgi?id=358055&action=review > > >> Source/WebCore/platform/Timer.cpp:85 > >> + RefPtr<ThreadTimerHeapItem>* m_pointer; > > > > What is the point of using a RefPtr? > > To match the type of the heap, which is now > Vector<RefPtr<ThreadTimerHeapItem>>. > This seems silly, but maybe I missed something. If it makes you feel warm and fuzzy then it has a purpose :/ > >> Source/WebCore/platform/Timer.cpp:107 > >> + RefPtr<ThreadTimerHeapItem>& m_reference; > > > > What is the point of using a RefPtr? > > Ditto. > Similarly, silly, but maybe I missed something. If it makes you feel warm and fuzzy then it has a purpose. :/ > >> Source/WebCore/platform/Timer.cpp:138 > >> + RefPtr<ThreadTimerHeapItem> timerA = a.copyRef(); > > > > Why the ref’ing? here and the line below? Where are these deref’ed()? > > I guess we could use std::exchange here. You didn’t answer either question :(
(In reply to Daniel Bates from comment #19) > (In reply to Ryosuke Niwa from comment #10) > > Comment on attachment 358055 [details] > > Adds the hardening > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=358055&action=review > > > > >> Source/WebCore/platform/Timer.cpp:85 > > >> + RefPtr<ThreadTimerHeapItem>* m_pointer; > > > > > > What is the point of using a RefPtr? > > > > To match the type of the heap, which is now > > Vector<RefPtr<ThreadTimerHeapItem>>. > > > > This seems silly, but maybe I missed something. If it makes you feel warm > and fuzzy then it has a purpose :/ Why?TimerHeapPointer and TimerHeapReference are pointer & reference types used to define a std::iterator for push_heap / pop_heap: std::iterator<std::random_access_iterator_tag, RefPtr<ThreadTimerHeapItem>, ptrdiff_t, TimerHeapPointer, TimerHeapReference> What is the point of avoid using RefPtr<ThreadTimerHeapItem> in this case? What we have is, NOT, RefPtr<ThreadTimerHeapItem> but rather a pointer and a reference to RefPtr<ThreadTimerHeapItem>. We have these two classes in std::iterator in order to override assignment operators to update the heap index in each ThreadTimerHeapItem.
Comment on attachment 358771 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=358771&action=review > Source/WebCore/platform/Timer.h:99 > + bool m_wasDeleted { false }; Sadly this change increased padding of all classes that use timers: +992 < 56> WebCore::Timer m_layoutTimer +992 < 48> WebCore::TimerBase WebCore::TimerBase +992 < 8> __vtbl_ptr_type * _vptr +1000 < 8> WTF::MonotonicTime m_unalignedNextFireTime +1000 < 8> double m_value +1008 < 8> WTF::Seconds m_repeatInterval +1008 < 8> double m_value +1016 < 1> bool m_wasDeleted +1017 < 7> <PADDING: 7 bytes> This effectively reverted r234581.
Comment on attachment 358771 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=358771&action=review >> Source/WebCore/platform/Timer.h:99 >> + bool m_wasDeleted { false }; > > Sadly this change increased padding of all classes that use timers: > > +992 < 56> WebCore::Timer m_layoutTimer > +992 < 48> WebCore::TimerBase WebCore::TimerBase > +992 < 8> __vtbl_ptr_type * _vptr > +1000 < 8> WTF::MonotonicTime m_unalignedNextFireTime > +1000 < 8> double m_value > +1008 < 8> WTF::Seconds m_repeatInterval > +1008 < 8> double m_value > +1016 < 1> bool m_wasDeleted > +1017 < 7> <PADDING: 7 bytes> > > This effectively reverted r234581. On the other hand, I removed one word by removing m_heapIndex, m_wasDeleted, and m_heapInsertionOrder so TimerBase should actually be smaller now? Having said that, we might be able to delete m_wasDeleted if we've categorically fixed timer related crashes, which I'm hoping to get a proof of soon from the crash tracer.