Bug 192975 - ThreadTimers should not store a raw pointer in its heap
Summary: ThreadTimers should not store a raw pointer in its heap
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-12-20 23:19 PST by Ryosuke Niwa
Modified: 2019-02-02 17:32 PST (History)
12 users (show)

See Also:


Attachments
WIP (14.15 KB, patch)
2018-12-20 23:22 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
WIP2 (21.36 KB, patch)
2018-12-21 14:39 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
WIP3 (27.82 KB, patch)
2018-12-21 18:53 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Adds the hardening (35.34 KB, patch)
2018-12-24 23:17 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Patch for landing (37.37 KB, patch)
2019-01-09 17:09 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Patch for landing (35.19 KB, patch)
2019-01-09 17:28 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2018-12-20 23:19:38 PST
Make ThreadTimers' timer heap use WeakPtr instead of a raw pointer.
Comment 1 Radar WebKit Bug Importer 2018-12-20 23:20:25 PST
<rdar://problem/46893946>
Comment 2 Ryosuke Niwa 2018-12-20 23:22:48 PST
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.
Comment 3 Build Bot 2018-12-20 23:25:01 PST
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.
Comment 4 Ryosuke Niwa 2018-12-21 14:39:47 PST
Created attachment 357980 [details]
WIP2
Comment 5 Build Bot 2018-12-21 14:42:59 PST
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.
Comment 6 Ryosuke Niwa 2018-12-21 18:53:39 PST
Created attachment 358010 [details]
WIP3
Comment 7 Build Bot 2018-12-21 18:55:09 PST
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.
Comment 8 Ryosuke Niwa 2018-12-24 23:17:34 PST
Created attachment 358055 [details]
Adds the hardening
Comment 9 Daniel Bates 2018-12-25 09:59:36 PST
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 10 Ryosuke Niwa 2019-01-09 11:09:04 PST
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 11 Geoffrey Garen 2019-01-09 13:07:18 PST
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().
Comment 12 Ryosuke Niwa 2019-01-09 17:09:16 PST
Created attachment 358766 [details]
Patch for landing
Comment 13 Ryosuke Niwa 2019-01-09 17:09:25 PST
Comment on attachment 358766 [details]
Patch for landing

Wait for EWS.
Comment 14 Ryosuke Niwa 2019-01-09 17:28:28 PST
Created attachment 358771 [details]
Patch for landing
Comment 15 Ryosuke Niwa 2019-01-09 17:28:42 PST
Comment on attachment 358771 [details]
Patch for landing

Wait for EWS.
Comment 16 Build Bot 2019-01-09 17:30:58 PST
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 17 WebKit Commit Bot 2019-01-09 18:27:53 PST
Comment on attachment 358771 [details]
Patch for landing

Clearing flags on attachment: 358771

Committed r239814: <https://trac.webkit.org/changeset/239814>
Comment 18 WebKit Commit Bot 2019-01-09 18:27:55 PST
All reviewed patches have been landed.  Closing bug.
Comment 19 Daniel Bates 2019-01-09 18:33:20 PST
(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 :(
Comment 20 Ryosuke Niwa 2019-01-09 19:10:44 PST
(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 21 Simon Fraser (smfr) 2019-02-02 14:31:42 PST
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 22 Ryosuke Niwa 2019-02-02 17:32:23 PST
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.