187370 2018-07-05 16:54:06 -0700 iOS port should define HAVE_RUNLOOP_TIMER 2018-07-12 11:36:34 -0700 1 1 1 Unclassified WebKit WebCore Misc. WebKit Local Build Unspecified Unspecified RESOLVED WONTFIX InRadar P2 Normal --- 1 ddkilzer ddkilzer andersca beidson benjamin cdumez cmarcelo dbates ews-watchlist ggaren joepeck koivisto rniwa simon.fraser timothy tsavell webkit-bug-importer oldest_to_newest 1439621 0 ddkilzer 2018-07-05 16:54:06 -0700 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>. 1439623 1 ddkilzer 2018-07-05 16:56:32 -0700 (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. 1439625 2 344381 ddkilzer 2018-07-05 17:02:30 -0700 Created attachment 344381 Patch v1 1439841 3 344381 simon.fraser 2018-07-06 11:38:30 -0700 Comment on attachment 344381 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). 1440135 4 ddkilzer 2018-07-07 09:21:23 -0700 Committed r233615: <https://trac.webkit.org/changeset/233615> 1440136 5 webkit-bug-importer 2018-07-07 09:22:19 -0700 <rdar://problem/41930765> 1440895 6 tsavell 2018-07-10 10:10:08 -0700 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: 1441494 7 ddkilzer 2018-07-12 07:21:38 -0700 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. 1441500 8 ddkilzer 2018-07-12 07:34:28 -0700 (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. 1441503 9 ddkilzer 2018-07-12 07:42:41 -0700 (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? 1441527 10 ddkilzer 2018-07-12 09:40:49 -0700 (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. 1441530 11 koivisto 2018-07-12 09:43:51 -0700 > > 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?). 1441531 12 koivisto 2018-07-12 09:47:50 -0700 There is no guarantees this stuff works with web thread. 1441532 13 ddkilzer 2018-07-12 09:54:49 -0700 (In reply to Antti Koivisto from comment #12) > There is no guarantees this stuff works with web thread. Okay, let's back it out. 1441546 14 ddkilzer 2018-07-12 10:06:27 -0700 (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? 1441559 15 koivisto 2018-07-12 10:22:18 -0700 > 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. 1441599 16 ddkilzer 2018-07-12 11:36:16 -0700 (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> 344381 2018-07-05 17:02:30 -0700 2018-07-06 11:38:30 -0700 Patch v1 bug-187370-20180705170229.patch text/plain 1549 ddkilzer U3VidmVyc2lvbiBSZXZpc2lvbjogMjMzNTQ3CmRpZmYgLS1naXQgYS9Tb3VyY2UvV1RGL0NoYW5n ZUxvZyBiL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCmluZGV4IDQ2YjI1MTUyYjZhYmZmMzIwODYzOWVm OTQ4NzJjZWZhNTAzZWUzNjQuLjdlZjJmNjRjNjk2ZWEyMTlkN2UwNDkxODQwNmI3YWEzMTY4OTBk MWQgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XVEYvQ2hh bmdlTG9nCkBAIC0xLDMgKzEsMTQgQEAKKzIwMTgtMDctMDUgIERhdmlkIEtpbHplciAgPGRka2ls emVyQGFwcGxlLmNvbT4KKworICAgICAgICBpT1MgcG9ydCBzaG91bGQgZGVmaW5lIEhBVkVfUlVO TE9PUF9USU1FUgorICAgICAgICA8aHR0cHM6Ly93ZWJraXQub3JnL2IvMTg3MzcwPgorCisgICAg ICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgICogd3RmL1BsYXRmb3Jt Lmg6CisgICAgICAgIChIQVZFX1JVTkxPT1BfVElNRVIpOiBEZWZpbmUgZm9yIFBMQVRGT1JNKENP Q09BKSwgbm90IGp1c3QKKyAgICAgICAgUExBVEZPUk0oTUFDKS4gIEFscGhhYmV0aXplLgorCiAy MDE4LTA3LTA1ICBDb21taXQgUXVldWUgIDxjb21taXQtcXVldWVAd2Via2l0Lm9yZz4KIAogICAg ICAgICBVbnJldmlld2VkLCByb2xsaW5nIG91dCByMjMzNDE3IGFuZCByMjMzNDE4LgpkaWZmIC0t Z2l0IGEvU291cmNlL1dURi93dGYvUGxhdGZvcm0uaCBiL1NvdXJjZS9XVEYvd3RmL1BsYXRmb3Jt LmgKaW5kZXggYmIxOTljNTNhMWRmOGRjOTRiNWM3YzYxODUwZjU1OTNjYjY4ZTI2Yi4uMDI3N2Iw NGYwYzRjNzVhNGFlMmYzZjVjNWVhN2I5NTg4OTM4MzlkZiAxMDA2NDQKLS0tIGEvU291cmNlL1dU Ri93dGYvUGxhdGZvcm0uaAorKysgYi9Tb3VyY2UvV1RGL3d0Zi9QbGF0Zm9ybS5oCkBAIC01NjMs MTIgKzU2MywxMyBAQAogCiAjaWYgUExBVEZPUk0oQ09DT0EpCiAKKyNkZWZpbmUgSEFWRV9EVFJB Q0UgMAorI2RlZmluZSBIQVZFX09VVF9PRl9QUk9DRVNTX0xBWUVSX0hPU1RJTkcgMQorI2RlZmlu ZSBIQVZFX1JVTkxPT1BfVElNRVIgMQogI2RlZmluZSBVU0VfQ0YgMQorI2RlZmluZSBVU0VfRklM RV9MT0NLIDEKICNkZWZpbmUgVVNFX0ZPVU5EQVRJT04gMQogI2RlZmluZSBVU0VfTkVUV09SS19D RkRBVEFfQVJSQVlfQ0FMTEJBQ0sgMQotI2RlZmluZSBIQVZFX09VVF9PRl9QUk9DRVNTX0xBWUVS X0hPU1RJTkcgMQotI2RlZmluZSBIQVZFX0RUUkFDRSAwCi0jZGVmaW5lIFVTRV9GSUxFX0xPQ0sg MQogCiAvKiBDb2NvYSBkZWZpbmVzIGEgc2VyaWVzIG9mIHBsYXRmb3JtIG1hY3JvcyBmb3IgZGVi dWdnaW5nLiAqLwogLyogU29tZSBvZiB0aGVtIGFyZSByZWFsbHkgYW5ub3lpbmcgYmVjYXVzZSB0 aGV5IHVzZSBjb21tb24gbmFtZXMgKGUuZy4gY2hlY2soKSkuICovCkBAIC01ODEsNyArNTgyLDYg QEAKICNpZiBQTEFURk9STShNQUMpCiAKICNkZWZpbmUgVVNFX0FQUEtJVCAxCi0jZGVmaW5lIEhB VkVfUlVOTE9PUF9USU1FUiAxCiAjZGVmaW5lIEhBVkVfU0VDX0tFWUNIQUlOIDEKIAogI2lmIENQ VShYODZfNjQpCg==