Bug 35943 – Make Document::postTask to use a single queue of tasks, to fire them in order

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Dmitry Titov]

Description of the original problem, from Dumitru Daniliuc:

"for the DB code to work, we sometimes need to guarantee that tasks run in the order in which their posted. the example we have is:

1. main thread posts task_1 to itself.
2. main thread posts task_2 to the DB thread.
3. task_2 runs on the DB thread and posts task_3 to the main thread.

without the changes i made to Document::postTask(), task_3 would be executed before task_1 on the main thread on all mac platforms, 100% of the time."

That happens because when called on the main thread, postTask would use one-shot timer (and timer queue) while if posted from another thread, it'll use callOnMainThread with it's own queue - so the tasks may execute in a different order then they are posted.

The fix would be to always use the single queue and callOnMainThread() to schedule tasks. This fix was attempted (bug 34726) and caused a regression (bug 35819). The fix for the starvation issue was discussed in https://lists.webkit.org/pipermail/webkit-dev/2010-March/011845.html.

Patch is coming.

[Dmitry Titov]

Created attachment 50353 [details]
Patch.

[Darin Adler]

Comment on attachment 50353 [details]
Patch.

> +static void timerFired(CFRunLoopTimerRef timer, void*)
> +{
> +    CFRelease(timer);
> +    WTF::dispatchFunctionsFromMainThread();
> +}
> +
> +void postTimer()
> +{
> +    ASSERT(isMainThread());
> +#if USE(WEB_THREAD)
> +    ASSERT(webThread == [NSThread currentThread]);
> +#endif
> +
> +    CFRunLoopAddTimer(CFRunLoopGetCurrent(), CFRunLoopTimerCreate(0, 0, 0, 0, 0, timerFired, 0), kCFRunLoopCommonModes);
> +}

I suggest writing this with a global variable to make sure we don't keep making a new CFRunLoopTimer object each time this is called. With a global variable we could coalesce multiple calls. The global could just be a boolean. (Unless some other level is already coalescing. I don't think so.)

Is the assertion correct? Maybe the isMainThread assertion belongs inside the #else of the USE(WEB_THREAD)?

[Alexey Proskuryakov]

Comment on attachment 50353 [details]
Patch.

With this patch, a task posted from main thread may end up being delivered under a modal dialog (if scheduleDispatchFunctionsOnMainThread has been already called to deliver a message from a worker). This wasn't the case previously

Please convince me that I'm wrong, and set back to r?.

[Dmitry Titov]

(In reply to comment #2)
> I suggest writing this with a global variable to make sure we don't keep making
> a new CFRunLoopTimer object each time this is called. With a global variable we
> could coalesce multiple calls. The global could just be a boolean. (Unless some
> other level is already coalescing. I don't think so.)

Will do, the higher level (see callOnMainThread()) is trying to avoid
unnecessary posts, but it does not do it 100% (duplicating posts can be done
from dispatchFunctionsFromMainThread when it exits on timeout). The bool is
cheap so it's a good thing to add.


> Is the assertion correct? Maybe the isMainThread assertion belongs inside the
> #else of the USE(WEB_THREAD)?

It is correct to check for IsMainThread because in this case WTF's idea of main
thread is the same as webThread (http://trac.webkit.org/changeset/52046)

The patch also broke http/tests/appcache/top-frame-2.html, so I'll upload
updated patch when I figure that out as well.

[Dmitry Titov]

(In reply to comment #3)
> (From update of attachment 50353 [details])
> With this patch, a task posted from main thread may end up being delivered
> under a modal dialog (if scheduleDispatchFunctionsOnMainThread has been already
> called to deliver a message from a worker). This wasn't the case previously
> 
> Please convince me that I'm wrong, and set back to r?.

Wasn't it always possible? Chrome::runModal() enables the timers back under modal UI (TimerBase::fireTimersInNestedEventLoop()).  I believe it was done for showModalDialog in http://trac.webkit.org/changeset/25103

[Dmitry Titov]

Comment on attachment 50353 [details]
Patch.

Removing flags from obsolete patch.

[Alexey Proskuryakov]

Yes, I was wrong.

[Dmitry Titov]

The test LayoutTests/http/tests/appcache/top-frame-2.html now fails (at least on my machine) because it depends on the order of events fired in JS:
1. "checking" event from ApplicationCache (fired via Document::postTask())
2. "message" event from window.postMessage (fired via one-shot timer)

Before, those events were using the same queue of timers, while with this change they would use 'different' ones - so now the event from cache comes right after "onmessage" and therefore is missing from the log because "onmessage" handler stops the test.

Although it is a bit scary that the order of visible JS events would change, since ApplicationCache seems to be the only component using postTask from the main thread at the moment, it is unlikely that many users of one-shot timers would be affected beyond ApplicationCache (other users of postTask are Database and Workers, both from other threads). 

For now, I'd think that it's ok to go ahead with this change since it at least orders tasks posted by postTask(), and change the test. ApplicationCache is not yet widely used so it is not very likely that we'll break much, and it'd be good to fix postTask while it's not used wider then now. Obviously, I'd watch for possible regressions after the change...

Does it look like reasonable risk?

[Dmitry Titov]

Thanks to Alexey Proskuryakov for suggestion to look at task queue spec of HTML5 to figure out what it says about ordering of events.

Event Loop in HTML5 (http://www.whatwg.org/specs/web-apps/current-work/multipage/webappapis.html#event-loop) defines that there are "task sources" and that all tasks that belong to a single source should be executed in order, while the tasks with different sources can be executed as if they were placed in separate queues (the order of picking queues is said to be "any").

The Application Cache events are specified to be from a "networking task source" http://www.whatwg.org/specs/web-apps/current-work/multipage/offline.html#pending-application-cache-download-process-tasks). 
The window.postMessage tasks are defined to be from a "posted message task source" (http://www.whatwg.org/specs/web-apps/current-work/multipage/comms.html#posting-messages) 
The XHR events triggering postMessage in the test are from "XMLHttpRequest task source" (http://dev.w3.org/2006/webapi/XMLHttpRequest-2/#task-sources)

So from HTML5 perspective, it seems the events in question can come in any order.

Unfortunately, FF behavior is inconsistent: 3.5.8 passes the test as with this patch, while 3.6 actually fails it.

[Michael Nordman]

(In reply to comment #9)
> Thanks to Alexey Proskuryakov for suggestion to look at task queue spec of
> HTML5 to figure out what it says about ordering of events.
> 
> So from HTML5 perspective, it seems the events in question can come in any
> order.

I think its reasonable to alter the expectation for the appcache test to not be
dependent on the relative ordering of messages received from newly loaded subframes
vs appcache events. The top-frame-2 test could signal completion after both
'checking' and 'message' have been received without regard for their order.
Actually you can see https://bugs.webkit.org/show_bug.cgi?id=32047 for a patch
to the top-frame-2 test which does that alteration.

I think changes like that could make the appcache test suite more of a test
for spec compliance than a test of webkit's particular impl (which considering
chrome has a different impl is of interest to me).

[Dmitry Titov]

Created attachment 50434 [details]
patch with updated App Cache test.

I think the discussion here, in bug 32047 and the current AC spec seem to converge on treating the AppCache events as async and able to come in any order related to postMessage event in this test.
I've updated the test so it is not dependent on the order of tasks, and waits for 3 signals in any order: AC events 'checking' and 'noupdate' and a  message from the subframe.
Also test prints order of all events if loaded in browser manually.

Correction to my above statement: FF 3.6 actually behaves exactly as WebKit will after this patch, wrt the set and order of those events. I didn't clear the app cache of FF while doing experiments...

[Darin Adler]

Comment on attachment 50434 [details]
patch with updated App Cache test.

> +    if (!isTimerPosted) {
> +        isTimerPosted = true;
> +        CFRunLoopAddTimer(CFRunLoopGetCurrent(), CFRunLoopTimerCreate(0, 0, 0, 0, 0, timerFired, 0), kCFRunLoopCommonModes);
> +    }

I normally write these as early returns instead of nesting the code.

> > Is the assertion correct? Maybe the isMainThread assertion belongs inside the
> > #else of the USE(WEB_THREAD)?
>
> It is correct to check for IsMainThread because in this case WTF's idea of main
> thread is the same as webThread (http://trac.webkit.org/changeset/52046)

If that is so, then why do we need the WEB_THREAD assertion at all?

[Dmitry Titov]

(In reply to comment #12)
> (From update of attachment 50434 [details])
> > +    if (!isTimerPosted) {
> > +        isTimerPosted = true;
> > +        CFRunLoopAddTimer(CFRunLoopGetCurrent(), CFRunLoopTimerCreate(0, 0, 0, 0, 0, timerFired, 0), kCFRunLoopCommonModes);
> > +    }
> 
> I normally write these as early returns instead of nesting the code.

Fixed.

> > > Is the assertion correct? Maybe the isMainThread assertion belongs inside the
> > > #else of the USE(WEB_THREAD)?
> >
> > It is correct to check for IsMainThread because in this case WTF's idea of main
> > thread is the same as webThread (http://trac.webkit.org/changeset/52046)
> 
> If that is so, then why do we need the WEB_THREAD assertion at all?

Right. I was (a bit paranoidly) trying to verify that isMainThread and webThread are the same but this check does not really belong here and does not add clarity, so it is removed.

Landed: http://trac.webkit.org/changeset/55816