Bug 192975 – ThreadTimers should not store a raw pointer in its heap

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Ryosuke Niwa]

Make ThreadTimers' timer heap use WeakPtr instead of a raw pointer.

[Radar WebKit Bug Importer]

<rdar://problem/46893946>

[Ryosuke Niwa]

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.

[EWS Watchlist]

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.

[Ryosuke Niwa]

Created attachment 357980 [details]
WIP2

[EWS Watchlist]

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.

[Ryosuke Niwa]

Created attachment 358010 [details]
WIP3

[EWS Watchlist]

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.

[Ryosuke Niwa]

Created attachment 358055 [details]
Adds the hardening

[Daniel Bates]

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()?

[Ryosuke Niwa]

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.

[Geoffrey Garen]

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().

[Ryosuke Niwa]

Created attachment 358766 [details]
Patch for landing

[Ryosuke Niwa]

Comment on attachment 358766 [details]
Patch for landing

Wait for EWS.

[Ryosuke Niwa]

Created attachment 358771 [details]
Patch for landing

[Ryosuke Niwa]

Comment on attachment 358771 [details]
Patch for landing

Wait for EWS.

[EWS Watchlist]

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.

[WebKit Commit Bot]

Comment on attachment 358771 [details]
Patch for landing

Clearing flags on attachment: 358771

Committed r239814: <https://trac.webkit.org/changeset/239814>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Daniel Bates]

(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 :(

[Ryosuke Niwa]

(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.

[Simon Fraser (smfr)]

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.

[Ryosuke Niwa]

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.