Bug 168570 – [GTK] Test fast/events/message-port-postMessage-recursive.html times out

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Carlos Garcia Campos]

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.

[Carlos Garcia Campos]

Created attachment 302073 [details]
Patch

[Michael Catanzaro]

I wonder what this will break!

[Carlos Garcia Campos]

(In reply to comment #2)
> I wonder what this will break!

You always so negative, I wonder what other things will fix.

[Carlos Garcia Campos]

Committed r212622: <http://trac.webkit.org/changeset/212622>

[Carlos Garcia Campos]

Reverted r212622 for reason:

Caused several test failures

Committed r212627: <http://trac.webkit.org/changeset/212627>

[Carlos Garcia Campos]

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.

[Michael Catanzaro]

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

[Carlos Garcia Campos]

Created attachment 302252 [details]
Patch

[Michael Catanzaro]

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.

[Carlos Garcia Campos]

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

[Carlos Garcia Campos]

Created attachment 302260 [details]
Patch

[Carlos Garcia Campos]

Committed r212814: <http://trac.webkit.org/changeset/212814>