Bug 194196 – REGRESSION (r239814): Most classes that user Timer have 7 bytes of padding after the Timer

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Simon Fraser (smfr)]

After Timer clients now get lots of padding:

+984 <184>     WebCore::FrameViewLayoutContext m_layoutContext
+984 <  8>       WebCore::FrameView & m_frameView
+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>          <-------------------- padding
+1024 <  8>                 WTF::RefPtr<WebCore::ThreadTimerHeapItem, WTF::DumbPtrTraits<WebCore::ThreadTimerHeapItem> > m_heapItem
+1024 <  8>                   WTF::DumbPtrTraits<WebCore::ThreadTimerHeapItem>::StorageType m_ptr
+1032 <  8>                 WTF::Ref<WTF::Thread, WTF::DumbPtrTraits<WTF::Thread> > m_thread
+1032 <  8>                   WTF::DumbPtrTraits<WTF::Thread>::StorageType m_ptr
+1040 <  8>             WTF::Function<void ()> m_function
+1040 <  8>                 std::__1::unique_ptr<WTF::Function<void ()>::CallableWrapperBase, std::__1::default_delete<WTF::Function<void ()>::CallableWrapperBase> > m_callableWrapper
+1040 <  8>                     std::__1::__compressed_pair<WTF::Function<void ()>::CallableWrapperBase *, std::__1::default_delete<WTF::Function<void ()>::CallableWrapperBase> > __ptr_
+1040 <  8>                         std::__1::__compressed_pair_elem<WTF::Function<void ()>::CallableWrapperBase *, 0, false> std::__1::__compressed_pair_elem<WTF::Function<void ()>::CallableWrapperBase *, 0, false>
+1040 <  8>                           WTF::Function<void ()>::CallableWrapperBase * __value_
+1040 <  1>                         std::__1::__compressed_pair_elem<std::__1::default_delete<WTF::Function<void ()>::CallableWrapperBase>, 1, true> std::__1::__compressed_pair_elem<std::__1::default_delete<WTF::Function<void ()>::CallableWrapperBase>, 1, true>
+1040 <  1>                             std::__1::default_delete<WTF::Function<void ()>::CallableWrapperBase> std::__1::default_delete<WTF::Function<void ()>::CallableWrapperBase>
+1048 < 56>         WebCore::Timer m_asynchronousTasksTimer
+1048 < 48>             WebCore::TimerBase WebCore::TimerBase
+1048 <  8>                __vtbl_ptr_type * _vptr
+1056 <  8>                 WTF::MonotonicTime m_unalignedNextFireTime
+1056 <  8>                   double m_value
+1064 <  8>                 WTF::Seconds m_repeatInterval
+1064 <  8>                   double m_value
+1072 <  1>               bool m_wasDeleted
+1073 <  7>               <PADDING: 7 bytes>          <-------------------- padding
+1080 <  8>                 WTF::RefPtr<WebCore::ThreadTimerHeapItem, WTF::DumbPtrTraits<WebCore::ThreadTimerHeapItem> > m_heapItem
...

[Simon Fraser (smfr)]

Can we remove the RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(!m_wasDeleted). I'm not sure why we'd have this on Timer and not other classes.

[Ryosuke Niwa]

(In reply to Simon Fraser (smfr) from comment #1)
> Can we remove the RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(!m_wasDeleted).
> I'm not sure why we'd have this on Timer and not other classes.

Because we keep getting mysterious timer related crashes that are only explainable by UAF, thread safety, etc... which always ends up being a top crasher.

If we get data suggesting that these crashes are gone in the next iOS / macOS seed, which we may due to my latest ThreadTimerHeapItem refactoring, we can probably make this debug only again.

[Ryosuke Niwa]

We may be able to set m_repeatInterval or m_unalignedNextFireTime to a special value instead of having a separate field though.

[Simon Fraser (smfr)]

That's a good idea!

[Ryosuke Niwa]

Also note that r239814 *shrunk* the size of TimerBase because it moved a bunch of fields in TimerBase to ThreadTimerHeapItem which only exists if the timer was scheduled at least once.

[Ryosuke Niwa]

Created attachment 364778 [details]
WIP

[EWS Watchlist]

Comment on attachment 364778 [details]
WIP

Attachment 364778 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11514662

New failing tests:
http/wpt/mediarecorder/MediaRecorder-AV-audio-video-dataavailable.html

[EWS Watchlist]

Created attachment 364781 [details]
Archive of layout-test-results from ews105 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[Ryosuke Niwa]

mac-wk2 is probably nothing to do with this patch:
Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebCore             	0x0000000290002243 WTFCrashWithInfo(int, char const*, char const*, int) + 19 (Assertions.h:560)
1   com.apple.WebCore             	0x00000002903de220 initAVEncoderBitRateKey() + 128 (MediaRecorderPrivateWriterCocoa.mm:55)
2   com.apple.WebCore             	0x00000002903d8166 WebCore::MediaRecorderPrivateWriter::setAudioInput() + 38 (MediaRecorderPrivateWriterCocoa.mm:179)
3   com.apple.WebCore             	0x00000002903d806e WebCore::MediaRecorderPrivateWriter::create(WebCore::MediaStreamTrackPrivate const*, WebCore::MediaStreamTrackPrivate const*) + 574 (MediaRecorderPrivateWriterCocoa.mm:107)
4   com.apple.WebCore             	0x000000029135fb93 WebCore::MediaRecorderPrivateAVFImpl::create(WebCore::MediaStreamPrivate const&) + 643 (RefPtr.h:81)
5   com.apple.WebCore             	0x00000002909596f0 WebCore::MediaRecorder::create(WebCore::Document&, WTF::Ref<WebCore::MediaStream, WTF::DumbPtrTraits<WebCore::MediaStream> >&&, WebCore::MediaRecorder::Options&&) + 128 (memory:2588)
6   com.apple.WebCore             	0x0000000290517327 WebCore::JSDOMConstructor<WebCore::JSMediaRecorder>::construct(JSC::ExecState*) + 279 (Ref.h:59)
7   com.apple.JavaScriptCore      	0x0000000294e6570f JSC::LLInt::setUpCall(JSC::ExecState*, JSC::CodeSpecializationKind, JSC::JSValue, JSC::LLIntCallLinkInfo*) + 367 (NativeFunction.h:50)
8   com.apple.JavaScriptCore      	0x00000002947a72e7 llint_entry + 63860 (LowLevelInterpreter.asm:981)
9   com.apple.JavaScriptCore      	0x00000002947a74d8 llint_entry + 64357 (LowLevelInterpreter.asm:885)
10  com.apple.JavaScriptCore      	0x00000002947a6bd9 llint_entry + 62054 (LowLevelInterpreter.asm:885)
11  com.apple.JavaScriptCore      	0x00000002947a6bd9 llint_entry + 62054 (LowLevelInterpreter.asm:885)
12  com.apple.JavaScriptCore      	0x00000002947977c9 vmEntryToJavaScript + 200 (LowLevelInterpreter64.asm:293)
13  com.apple.JavaScriptCore      	0x0000000294d58c3f JSC::Interpreter::executeProgram(JSC::SourceCode const&, JSC::ExecState*, JSC::JSObject*) + 11295 (JITCodeInlines.h:39)
14  com.apple.JavaScriptCore      	0x0000000294fdfebb JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&) + 315 (Completion.cpp:141)

[Ryosuke Niwa]

Created attachment 364843 [details]
Eliminates the padding

[Ryosuke Niwa]

Comment on attachment 364843 [details]
Eliminates the padding

Clearing flags on attachment: 364843

Committed r243022: <https://trac.webkit.org/changeset/243022>

[Ryosuke Niwa]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/48943362>