Bug 168570

Summary: [GTK] Test fast/events/message-port-postMessage-recursive.html times out
Product: WebKit Reporter: Carlos Garcia Campos <cgarcia>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, bugs-noreply, cdumez, cmarcelo, commit-queue, dbates, mcatanzaro
Priority: P2 Keywords: Gtk, LayoutTestFailure
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch mcatanzaro: review+

Description Carlos Garcia Campos 2017-02-19 02:08:21 PST
This has recently been added and the patch is good. It's just revealing a problem with our timers. The test is posting a message recursively, and also starts a timeout timer to finish the test. The timeout timer is never fired for us, because WebCore timers have lower priority than the one used by postMessage. ScriptExecutionContext uses Document::postTask, that uses scheduleOnMainThread, that uses RunLoop::dispatch(). We are not setting any priority for the timer used by RunLoop::dispatch, so it's using the default. RunLoop::dispatch is normally used to schedule tasks between threads, or just to ensure something is run in a different run loop iteration, but in general nothing urgent as a graphics redraw or something like that. It's quite common to use g_idle_add to schedule tasks between threads, so I think it makes sense to use G_PRIORITY_DEFAULT_IDLE for the RunLoop timer.
Comment 1 Carlos Garcia Campos 2017-02-19 02:10:24 PST
Created attachment 302073 [details]
Patch
Comment 2 Michael Catanzaro 2017-02-19 09:28:01 PST
I wonder what this will break!
Comment 3 Carlos Garcia Campos 2017-02-19 22:24:42 PST
(In reply to comment #2)
> I wonder what this will break!

You always so negative, I wonder what other things will fix.
Comment 4 Carlos Garcia Campos 2017-02-19 22:31:03 PST
Committed r212622: <http://trac.webkit.org/changeset/212622>
Comment 5 Carlos Garcia Campos 2017-02-20 01:11:18 PST
Reverted r212622 for reason:

Caused several test failures

Committed r212627: <http://trac.webkit.org/changeset/212627>
Comment 6 Carlos Garcia Campos 2017-02-20 01:13:15 PST
So, yes, this introduced a few failures (~10) most of them image only failures. I'm still not sure if they are actual regressions, or the patch actually revealed those bugs. But timer priorities is tricky thing that could also impact the performance, so we should think more carefully about it.
Comment 7 Michael Catanzaro 2017-02-20 07:03:47 PST
(In reply to comment #6)
> So, yes, this introduced a few failures (~10) most of them image only
> failures. I'm still not sure if they are actual regressions, or the patch
> actually revealed those bugs. But timer priorities is tricky thing that
> could also impact the performance, so we should think more carefully about
> it.

I get to be right every once in a while!

I'm actually most concerned about the original test, because it's testing the behavior of the timers themselves, and it shows us that our timers are definitely broken without your patch. But clearly we need to think about this some more and understand what's going on with the other tests. Timers are hard.
Comment 8 Carlos Garcia Campos 2017-02-21 06:49:53 PST
Created attachment 302252 [details]
Patch
Comment 9 Michael Catanzaro 2017-02-21 07:43:41 PST
Comment on attachment 302252 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=302252&action=review

> Source/WTF/wtf/glib/MainThreadGLib.cpp:38
> +class MainThreadDispatcher {

Please add a comment. You could take it from your commit message.

> Source/WTF/wtf/glib/MainThreadGLib.cpp:43
> +        m_timer.setPriority(G_PRIORITY_HIGH_IDLE + 20);

Too fragile. It has to be exactly equal to WebCore timers? Then you should fine some common header to define it in.
Comment 10 Carlos Garcia Campos 2017-02-21 08:09:51 PST
(In reply to comment #9)
> Comment on attachment 302252 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=302252&action=review
> 
> > Source/WTF/wtf/glib/MainThreadGLib.cpp:38
> > +class MainThreadDispatcher {
> 
> Please add a comment. You could take it from your commit message.

Ok.

> > Source/WTF/wtf/glib/MainThreadGLib.cpp:43
> > +        m_timer.setPriority(G_PRIORITY_HIGH_IDLE + 20);
> 
> Too fragile. It has to be exactly equal to WebCore timers? Then you should
> fine some common header to define it in.

Not really, I'm just conservative here and using at least the same priority. I still think we should re-think all the timer priorities anyway.
Comment 11 Carlos Garcia Campos 2017-02-21 08:13:36 PST
Created attachment 302260 [details]
Patch
Comment 12 Carlos Garcia Campos 2017-02-22 00:32:13 PST
Committed r212814: <http://trac.webkit.org/changeset/212814>