Bug 187370 – iOS port should define HAVE_RUNLOOP_TIMER

• Component: WebCore Misc.
• Status: RESOLVED WONTFIX

[David Kilzer (:ddkilzer)]

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>.

[David Kilzer (:ddkilzer)]

(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.

[David Kilzer (:ddkilzer)]

Created attachment 344381 [details]
Patch v1

[Simon Fraser (smfr)]

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).

[David Kilzer (:ddkilzer)]

Committed r233615: <https://trac.webkit.org/changeset/233615>

[Radar WebKit Bug Importer]

<rdar://problem/41930765>

[Truitt Savell]

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:

[David Kilzer (:ddkilzer)]

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.

[David Kilzer (:ddkilzer)]

(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.

[David Kilzer (:ddkilzer)]

(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?

[David Kilzer (:ddkilzer)]

(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.

[Antti Koivisto]

> > 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?).

[Antti Koivisto]

There is no guarantees this stuff works with web thread.

[David Kilzer (:ddkilzer)]

(In reply to Antti Koivisto from comment #12)
> There is no guarantees this stuff works with web thread.

Okay, let's back it out.

[David Kilzer (:ddkilzer)]

(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?

[Antti Koivisto]

> 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.

[David Kilzer (:ddkilzer)]

(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>