One of the last major differences between workers and the main thread is the event loop. Workers don't use the XPCOM event loop during the lifetime of the worker, so things like NS_DispatchToCurrentThread don't work (the runnable runs after the worker has terminated). I wrote some terrible patches to hook nsThread::PutEvent and stick them in the worker queue which I will attach, but I think we really want to just use the same abstraction we introduce for per-document event queues on workers (for non-sync stuff, sync/control messages will continue to need worker specific code).

Which APIs are going to be blocked on this? All that uses NS_DispatchToCurrentThread? (which I expect is most of them)

They're not blocked, per se, but they wont work out of the box. They'll need to deal with the worker event loop directly. But yes, anything that uses NS_DispatchToCurrentThread (or uses NS_DispatchToMainThread when it means NS_DispatchToCurrentThread).

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #0) > One of the last major differences between workers and the main thread is the > event loop. Workers don't use the XPCOM event loop during the lifetime of > the worker, so things like NS_DispatchToCurrentThread don't work (the > runnable runs after the worker has terminated). I wrote some terrible > patches to hook nsThread::PutEvent and stick them in the worker queue which > I will attach, but I think we really want to just use the same abstraction > we introduce for per-document event queues on workers (for non-sync stuff, > sync/control messages will continue to need worker specific code). So this means that it's not going to help sync IDB implementation for now I guess, only the async IDB in workers.

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #0) > One of the last major differences between workers and the main thread is the > event loop. Workers don't use the XPCOM event loop during the lifetime of > the worker, so things like NS_DispatchToCurrentThread don't work (the > runnable runs after the worker has terminated). I wrote some terrible > patches to hook nsThread::PutEvent and stick them in the worker queue which > I will attach, but I think we really want to just use the same abstraction > we introduce for per-document event queues on workers (for non-sync stuff, > sync/control messages will continue to need worker specific code). I think workers should just have a normal event loop if possible. Whatever is done with per-tab-event loops is separate issue (and those event-loops should look as much as possible the same as the main-event-loop)

If "normal" means spin the XPCOM event loop, that's not really possible. If we want to implement the HTML5 task source stuff then we're going to need some sort of new event queue abstraction anyways. Using that here would be nice. Also the patch attached that makes NS_DispatchToCurrentThread work is really terrible. If you don't believe we should do something else you should read it ;-)

We're in very urgent need to start porting APIs to workers, so I don't think switching to a new event loop implementation is a reasonable approach here. Unless we have a plan that can be implemented relatively soon (while keeping in mind that we're short on reviewers). When I asked Bent about it, apparently one of the big problems with using a normal XPCOM event loop is that we need the control runnables. I.e. being able to dispatch runnables to a worker thread which is run even if we're in the middle of a long running script or if we're currently blocked in a sync API. There's also the magic that we do in order to do memory reporting in workers. That magic lets us access a the objects owned by a worker thread even if that worker thread is in the middle of running JS-Ctypes. Not sure if that gets in the way of using normal XPCOM event loops here.

Workers can't really ever have normal event loops since they're interruptable. I haven't looked at the patch yet but if it's just doing the thing I suggested in Toronto then I think we need to get it reviewed and landed.

(Shouldn't depend on bug 715376. The main issue there is integration with appshell and such, and we don't need to think about appshell on workers.)

My work rewriting the worker event loop probably makes this obsolete. Let's hold off on reviewing for now.

This is all I need, I think.

This patch is big, but mostly simple. I moved some code around into separate files so don't be discouraged by the size :)

Comment on attachment 833033 [details] [diff] [review] nsIEventTarget-ize the Worker event loop, v1 Yay, Blake has offered to review!

Comment on attachment 833033 [details] [diff] [review] nsIEventTarget-ize the Worker event loop, v1 Review of attachment 833033 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/workers/WorkerPrivate.cpp @@ +2626,5 @@ > > if (aRoot) { > NS_ADDREF_THIS(); > if (GetWrapperPreserveColor()) { > + PreserveWrapper(static_cast<nsDOMEventTargetHelper*>(this)); Remove @@ +5475,5 @@ > + > + { > + MutexAutoLock lock(mMutex); > + > + MOZ_ASSERT_IF(aThread, mStatus == Pending); Remove ::: dom/workers/WorkerPrivate.h @@ +181,5 @@ > > protected: > typedef mozilla::ErrorResult ErrorResult; > > + mutable SharedMutex mMutex; No!

Comment on attachment 833033 [details] [diff] [review] nsIEventTarget-ize the Worker event loop, v1 Review of attachment 833033 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for walking me through the interesting bits last week. That made this much easier to review. It would have been nice to split some of the cleanup (moving complex functions to the bottom of the file and out of line, etc.) into other patches. I have one non-nit question to answer, but I *think* it's OK, so stamping this r=mrbkap. ::: dom/workers/RuntimeService.cpp @@ +2159,5 @@ > > // Too many idle threads, just shut this one down. > if (shutdown) { > if (NS_FAILED(aThread->Shutdown())) { > + MOZ_ASSERT(false, "Failed to shutdown thread!"); MOZ_ALWAYS_TRUE() @@ +2288,5 @@ > + if (NS_SUCCEEDED(AddObserver(observer))) { > + mObserver.swap(observer); > + } > + else { > + MOZ_ASSERT(false, "This should never fail!"); MOZ_ALWAYS_TRUE() ::: dom/workers/WorkerPrivate.cpp @@ +1421,5 @@ > { > + NS_ENSURE_ARG_POINTER(aIsOnCurrentThread); > + > + nsresult rv = mWorkerPrivate->IsOnCurrentThread(aIsOnCurrentThread); > + NS_ENSURE_SUCCESS(rv, rv); Given that NS_ENSURE_SUCCESS and friends are now deprecated, why not make this simply: return mWorkerPrivate->...; ? @@ +2099,5 @@ > > JSObject* obj = WorkerBinding::Wrap(aCx, aScope, ParentAsWorkerPrivate()); > > if (mRooted) { > + PreserveWrapper(static_cast<nsDOMEventTargetHelper*>(this)); The cast is no longer needed if I'm remembering correctly. @@ +4701,5 @@ > + // This will delete |loop|! > + mSyncLoopStack.RemoveElementAt(currentLoopIndex); > + > + if (NS_FAILED(thread->PopEventQueue())) { > + MOZ_ASSERT(false, "This should never fail!"); MOZ_ALWAYS_TRUE()? @@ +4974,1 @@ > return false; Nit: This can now be written as: if (mCloseHandlerStarted && !mCloseHandlerFinished) { ScheduleKillCloseEventRunnable(aCx); } return false; @@ +5485,5 @@ > + mThread = aThread; > + > + if (!mPreStartRunnables.IsEmpty()) { > + for (uint32_t index = 0; index < mPreStartRunnables.Length(); index++) { > + if (NS_FAILED(mThread->Dispatch(mPreStartRunnables[index], Is it OK to do this in the lock? Looking, I don't see anything going the other way, so I assume it is. @@ +5691,5 @@ > + nsThreadManager::get()->GetThreadFromPRThread(mPRThread, > + getter_AddRefs(thread)); > + MOZ_ASSERT(NS_SUCCEEDED(rv)); > + MOZ_ASSERT(thread); > + Nit: trailing whitespace. ::: dom/workers/WorkerRunnable.cpp @@ +319,5 @@ > + MOZ_ASSERT(canceledCount, "Cancel() overflow!"); > + > + // The docs say that Cancel() should not be called more than once and that we > + // should throw NS_ERROR_UNEXPECTED if it is. > + return canceledCount == 1 ? NS_OK : NS_ERROR_UNEXPECTED; Nit: return (canceledCount == 1) ? ...; ::: dom/workers/WorkerRunnable.h @@ +128,5 @@ > +// This runnable is used when sending things from the main thread to the worker. > +// There is no way to modify the busy count from the main thread so don't use > +// this unless the worker already has a guaranteed non-zero busy count through > +// some other means! > +class MainThreadWorkerRunnable : public WorkerRunnable Given the comment about making sure that someone else has incremented the busy count, it seems like this is a foot-gun waiting to be fired. Looking around, I don't see any users, so I'd take it out until we find a situation where it's definitely necessary. ::: dom/workers/Workers.h @@ +204,5 @@ > +private: > + // Only created by WorkerPrivate. > + WorkerCrossThreadDispatcher(WorkerPrivate* aWorkerPrivate); > + > + // Only created by WorkerPrivate. Should this read "Only called by WorkerPrivate"?

Comment on attachment 8348442 [details] [diff] [review] More assertion fixes, v1 Review of attachment 8348442 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/workers/WorkerPrivate.cpp @@ +4262,5 @@ > ClearMainEventQueue(); > > +#ifdef DEBUG > + // Simple race check. > + PR_Sleep(PR_SecondsToInterval(1)); I talked to bent on IRC about this. I'm not really happy with checking in 1-second sleeps and he's OK with removing it. r=me with that fixed.

https://hg.mozilla.org/integration/mozilla-inbound/rev/ca8f3c046bc6 https://hg.mozilla.org/integration/mozilla-inbound/rev/38961d1068f8

https://hg.mozilla.org/mozilla-central/rev/ca8f3c046bc6 https://hg.mozilla.org/mozilla-central/rev/38961d1068f8