Bug 194196 - REGRESSION (r239814): Most classes that user Timer have 7 bytes of padding after the Timer
Summary: REGRESSION (r239814): Most classes that user Timer have 7 bytes of padding af...
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: 2019-02-02 14:33 PST by Simon Fraser (smfr)
Modified: 2019-03-15 16:15 PDT (History)
8 users (show)

See Also:


Attachments
WIP (4.19 KB, patch)
2019-03-14 23:56 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews105 for mac-highsierra-wk2 (2.65 MB, application/zip)
2019-03-15 01:20 PDT, Build Bot
no flags Details
Eliminates the padding (3.49 KB, patch)
2019-03-15 14:16 PDT, 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 Simon Fraser (smfr) 2019-02-02 14:33:19 PST
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
...
Comment 1 Simon Fraser (smfr) 2019-02-02 14:35:45 PST
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.
Comment 2 Ryosuke Niwa 2019-02-02 17:41:16 PST
(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.
Comment 3 Ryosuke Niwa 2019-02-02 17:42:47 PST
We may be able to set m_repeatInterval or m_unalignedNextFireTime to a special value instead of having a separate field though.
Comment 4 Simon Fraser (smfr) 2019-02-02 17:43:53 PST
That's a good idea!
Comment 5 Ryosuke Niwa 2019-02-02 17:43:59 PST
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.
Comment 6 Ryosuke Niwa 2019-03-14 23:56:51 PDT
Created attachment 364778 [details]
WIP
Comment 7 Build Bot 2019-03-15 01:20:14 PDT
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
Comment 8 Build Bot 2019-03-15 01:20:16 PDT
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
Comment 9 Ryosuke Niwa 2019-03-15 01:42:01 PDT
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)
Comment 10 Ryosuke Niwa 2019-03-15 14:16:50 PDT
Created attachment 364843 [details]
Eliminates the padding
Comment 11 Ryosuke Niwa 2019-03-15 16:12:14 PDT
Comment on attachment 364843 [details]
Eliminates the padding

Clearing flags on attachment: 364843

Committed r243022: <https://trac.webkit.org/changeset/243022>
Comment 12 Ryosuke Niwa 2019-03-15 16:12:16 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Radar WebKit Bug Importer 2019-03-15 16:15:49 PDT
<rdar://problem/48943362>