Currently HAVE_RUNLOOP_TIMER is only defined for the macOS port. The iOS port should define it as well as we expect both platforms to behave similarly with similar capabilities. Currently, RunLoopTimer<> is only used in two places: 1. Source/WebCore/loader/DocumentLoader.h for DocumentLoaderTimer type used by DocumentLoader.m_dataLoadTimer. See <https://trac.webkit.org/r146239> (previously used in Source/WebCore/loader/MainResourceLoader.h). See <https://trac.webkit.org/r41845> for introduction of HAVE_RUNLOOP_TIMER and use in MainResourceLoader.h. 2. Source/WebCore/platform/network/DataURLDecoder.cpp for DecodingResultDispatcher.m_timer. See <https://trac.webkit.org/r191720>.
(In reply to David Kilzer (:ddkilzer) from comment #0) > Currently HAVE_RUNLOOP_TIMER is only defined for the macOS port. > > The iOS port should define it as well as we expect both platforms to behave > similarly with similar capabilities. > > Currently, RunLoopTimer<> is only used in two places: > > 1. Source/WebCore/loader/DocumentLoader.h for DocumentLoaderTimer type used > by DocumentLoader.m_dataLoadTimer. > See <https://trac.webkit.org/r146239> (previously used in > Source/WebCore/loader/MainResourceLoader.h). > See <https://trac.webkit.org/r41845> for introduction of > HAVE_RUNLOOP_TIMER and use in MainResourceLoader.h. It's not clear to me whether DocumentLoader.m_dataLoadTimer still needs to be a RunLoopTimer based on the original reasoning from r41845. > 2. Source/WebCore/platform/network/DataURLDecoder.cpp for > DecodingResultDispatcher.m_timer. > See <https://trac.webkit.org/r191720>. This seems like it may be a performance win since the iOS port will stop scheduling tasks on the main thread that could run on a background thread instead.
Created attachment 344381 [details] Patch v1
Comment on attachment 344381 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=344381&action=review > Source/WTF/wtf/Platform.h:566 > +#define HAVE_DTRACE 0 It seems like HAVE_DTRACE can be removed. There are no instances of HAVE(DTRACE) anywhere in the project. (Also Cocoa platforms do have dtrace).
Committed r233615: <https://trac.webkit.org/changeset/233615>
<rdar://problem/41930765>
It looks like after <https://trac.webkit.org/changeset/233615/webkit> we are getting constant crashes on iOS debug WK2 from an assertion. Test History: https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=fast%2Fevents%2Fbeforeunload-prompt.html link to output: https://build.webkit.org/builders/Apple%20iOS%2011%20Simulator%20Debug%20WK2%20(Tests)/builds/5269/steps/layout-test/logs/stdio Example of assertion failure: 09:48:20.623 23609 worker/1 fast/events/beforeunload-prompt.html crashed, (stderr lines): 09:48:20.623 23609 ASSERTION FAILED: m_uncommittedState.provisionalURL.isEmpty() 09:48:20.623 23609 /Volumes/Data/slave/ios-simulator-11-debug/build/Source/WebKit/UIProcess/PageLoadState.cpp(252) : void WebKit::PageLoadState::didStartProvisionalLoad(const Transaction::Token &, const WTF::String &, const WTF::String &) 09:48:20.623 23609 1 0x107c689d9 WTFCrash 09:48:20.623 23609 2 0x10eb5265f WebKit::PageLoadState::didStartProvisionalLoad(WebKit::PageLoadState::Transaction::Token const&, WTF::String const&, WTF::String const&) 09:48:20.623 23609 3 0x10f19d322 WebKit::WebPageProxy::didStartProvisionalLoadForFrame(unsigned long long, unsigned long long, WebCore::URL&&, WebCore::URL&&, WebKit::UserData const&) 09:48:20.623 23609 4 0x10f26e418 void IPC::callMemberFunctionImpl<WebKit::WebPageProxy, void (WebKit::WebPageProxy::*)(unsigned long long, unsigned long long, WebCore::URL&&, WebCore::URL&&, WebKit::UserData const&), std::__1::tuple<unsigned long long, unsigned long long, WebCore::URL, WebCore::URL, WebKit::UserData>, 0ul, 1ul, 2ul, 3ul, 4ul>(WebKit::WebPageProxy*, void (WebKit::WebPageProxy::*)(unsigned long long, unsigned long long, WebCore::URL&&, WebCore::URL&&, WebKit::UserData const&), std::__1::tuple<unsigned long long, unsigned long long, WebCore::URL, WebCore::URL, WebKit::UserData>&&, std::__1::integer_sequence<unsigned long, 0ul, 1ul, 2ul, 3ul, 4ul>) 09:48:20.623 23609 5 0x10f26df50 void IPC::callMemberFunction<WebKit::WebPageProxy, void (WebKit::WebPageProxy::*)(unsigned long long, unsigned long long, WebCore::URL&&, WebCore::URL&&, WebKit::UserData const&), std::__1::tuple<unsigned long long, unsigned long long, WebCore::URL, WebCore::URL, WebKit::UserData>, std::__1::integer_sequence<unsigned long, 0ul, 1ul, 2ul, 3ul, 4ul> >(std::__1::tuple<unsigned long long, unsigned long long, WebCore::URL, WebCore::URL, WebKit::UserData>&&, WebKit::WebPageProxy*, void (WebKit::WebPageProxy::*)(unsigned long long, unsigned long long, WebCore::URL&&, WebCore::URL&&, WebKit::UserData const&)) 09:48:20.623 23609 6 0x10f252c6c void IPC::handleMessage<Messages::WebPageProxy::DidStartProvisionalLoadForFrame, WebKit::WebPageProxy, void (WebKit::WebPageProxy::*)(unsigned long long, unsigned long long, WebCore::URL&&, WebCore::URL&&, WebKit::UserData const&)>(IPC::Decoder&, WebKit::WebPageProxy*, void (WebKit::WebPageProxy::*)(unsigned long long, unsigned long long, WebCore::URL&&, WebCore::URL&&, WebKit::UserData const&)) 09:48:20.623 23609 7 0x10f24a3aa WebKit::WebPageProxy::didReceiveMessage(IPC::Connection&, IPC::Decoder&) 09:48:20.623 23609 8 0x10e920da8 IPC::MessageReceiverMap::dispatchMessage(IPC::Connection&, IPC::Decoder&) 09:48:20.623 23609 9 0x10e80c324 WebKit::ChildProcessProxy::dispatchMessage(IPC::Connection&, IPC::Decoder&) 09:48:20.623 23609 10 0x10f3d414d WebKit::WebProcessProxy::didReceiveMessage(IPC::Connection&, IPC::Decoder&) 09:48:20.623 23609 11 0x10e81e473 IPC::Connection::dispatchMessage(IPC::Decoder&) 09:48:20.623 23609 12 0x10e811e68 IPC::Connection::dispatchMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >) 09:48:20.623 23609 13 0x10e81bc2c IPC::Connection::dispatchIncomingMessages() 09:48:20.623 23609 14 0x10e836532 IPC::Connection::enqueueIncomingMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)::$_14::operator()() 09:48:20.623 23609 15 0x10e836459 WTF::Function<void ()>::CallableWrapper<IPC::Connection::enqueueIncomingMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)::$_14>::call() 09:48:20.624 23609 16 0x107c8db2b WTF::Function<void ()>::operator()() const 09:48:20.624 23609 17 0x107ce0af3 WTF::RunLoop::performWork() 09:48:20.624 23609 18 0x107ce13f4 WTF::RunLoop::performWork(void*) 09:48:20.624 23609 19 0x114028bb1 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ 09:48:20.624 23609 20 0x11400d4af __CFRunLoopDoSources0 09:48:20.624 23609 21 0x11400ca6f __CFRunLoopRun 09:48:20.624 23609 22 0x11400c30b CFRunLoopRunSpecific 09:48:20.624 23609 23 0x112d49b4a -[NSRunLoop(NSRunLoop) runMode:beforeDate:] 09:48:20.624 23609 24 0x107303ee4 WTR::TestController::platformRunUntil(bool&, double) 09:48:20.624 23609 25 0x1072d82e9 WTR::TestController::runUntil(bool&, double) 09:48:20.624 23609 26 0x1072d789e WTR::TestController::resetStateToConsistentValues(WTR::TestOptions const&) 09:48:20.624 23609 27 0x107305e91 WTR::TestInvocation::invoke() 09:48:20.624 23609 28 0x1072e20b8 WTR::TestController::runTest(char const*) 09:48:20.624 23609 29 0x1072e3124 WTR::TestController::runTestingServerLoop() 09:48:20.624 23609 30 0x1072d20b6 WTR::TestController::run() 09:48:20.624 23609 31 0x1072d1a5d WTR::TestController::TestController(int, char const**) 09:48:20.632 22449 [14775/46877] fast/events/beforeunload-prompt.html failed unexpectedly (WebKitTestRunnerApp crashed [pid=25865]) 09:48:20.628 23609 worker/1 killing driver 09:48:20.633 23609 worker/1 fast/events/beforeunload-prompt.html failed:
We should back this change out if it causes assertions. I’m sad that this was (again) not caught in EWS due to lack of iOS Debug tests.
(In reply to Truitt Savell from comment #6) > It looks like after <https://trac.webkit.org/changeset/233615/webkit> > > we are getting constant crashes on iOS debug WK2 from an assertion. > > Test History: > https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard. > html#showAllRuns=true&tests=fast%2Fevents%2Fbeforeunload-prompt.html Strange that it's just this one test: fast/events/beforeunload-prompt.html Also, it appears the GTK Linux 64-bit Debug tests have been failing on this test for a longer period of time.
(In reply to David Kilzer (:ddkilzer) from comment #8) > (In reply to Truitt Savell from comment #6) > > It looks like after <https://trac.webkit.org/changeset/233615/webkit> > > > > we are getting constant crashes on iOS debug WK2 from an assertion. > > > > Test History: > > https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard. > > html#showAllRuns=true&tests=fast%2Fevents%2Fbeforeunload-prompt.html > > Strange that it's just this one test: fast/events/beforeunload-prompt.html > > Also, it appears the GTK Linux 64-bit Debug tests have been failing on this > test for a longer period of time. Perhaps some state that isn't being cleared out from a previous test?
(In reply to David Kilzer (:ddkilzer) from comment #9) > (In reply to David Kilzer (:ddkilzer) from comment #8) > > (In reply to Truitt Savell from comment #6) > > > It looks like after <https://trac.webkit.org/changeset/233615/webkit> > > > > > > we are getting constant crashes on iOS debug WK2 from an assertion. > > > > > > Test History: > > > https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard. > > > html#showAllRuns=true&tests=fast%2Fevents%2Fbeforeunload-prompt.html > > > > Strange that it's just this one test: fast/events/beforeunload-prompt.html > > > > Also, it appears the GTK Linux 64-bit Debug tests have been failing on this > > test for a longer period of time. > > Perhaps some state that isn't being cleared out from a previous test? Running the test by itself 10 times doesn't cause a crash: $ ./Tools/Scripts/run-webkit-tests --debug --sim --iterations 10 fast/events/beforeunload-prompt.html Running the test with the previous test (alphabetically) in the directory 100 times each doesn't crash: $ ./Tools/Scripts/run-webkit-tests --debug --sim --iterations 100 fast/events/beforeunload-dom-manipulation-crash.html fast/events/beforeunload-prompt.html I think we should figure out which other test is causing this crash and fix/block it.
> > 2. Source/WebCore/platform/network/DataURLDecoder.cpp for > > DecodingResultDispatcher.m_timer. > > See <https://trac.webkit.org/r191720>. > > This seems like it may be a performance win since the iOS port will stop > scheduling tasks on the main thread that could run on a background thread > instead. There is no performance win, callOnMainThread and Timer fire in the same thread. Generally I don't think we should ever make this change. As far as I remember the RunLoopTimer hack exist solely to deal with some weird nested runloop cases with load delegates that are not relevant on iOS. Rather we should figure out if we can get rid of RunLoopTimer on Mac too (it may be needed for wk1 only?).
There is no guarantees this stuff works with web thread.
(In reply to Antti Koivisto from comment #12) > There is no guarantees this stuff works with web thread. Okay, let's back it out.
(In reply to Antti Koivisto from comment #11) > > > 2. Source/WebCore/platform/network/DataURLDecoder.cpp for > > > DecodingResultDispatcher.m_timer. > > > See <https://trac.webkit.org/r191720>. > > > > This seems like it may be a performance win since the iOS port will stop > > scheduling tasks on the main thread that could run on a background thread > > instead. > > There is no performance win, callOnMainThread and Timer fire in the same > thread. In iOS WK1 (whenever WebThread is running), WebCore::Timer always fires on the WebThread even if scheduled on other threads. (BTW, that's why WebCore::Timer can't be used in the UI Process on iOS, although that's not a factor here.) > Generally I don't think we should ever make this change. As far as I > remember the RunLoopTimer hack exist solely to deal with some weird nested > runloop cases with load delegates that are not relevant on iOS. Rather we > should figure out if we can get rid of RunLoopTimer on Mac too (it may be > needed for wk1 only?). RunLoopTimer was added back in 2009 by Timothy Hatcher in <https://trac.webkit.org/r41845> (see Comment #0). Are you referring to its use in Source/WebCore/platform/network/DataURLDecoder.cpp?
> RunLoopTimer was added back in 2009 by Timothy Hatcher in > <https://trac.webkit.org/r41845> (see Comment #0). > > Are you referring to its use in > Source/WebCore/platform/network/DataURLDecoder.cpp? Well, <rdar://problem/6687342> has the history. Data URLs featured as a reason already back then. I supposed the main point is "Without this change you can’t use WebKit in a non-standard run loop mode" which is not something we care about on iOS.
(In reply to David Kilzer (:ddkilzer) from comment #4) > Committed r233615: <https://trac.webkit.org/changeset/233615> Backed out here: Committed r233774: <https://trac.webkit.org/changeset/233774>