Currently on windows we require IPDL to occur on the UI thread because we always want to spin a nested event loop. We should only be doing that if we're actually doing IPDL on the UI thread. Otherwise we should do what we do on other platforms.

Comment on attachment 8421734 [details] [diff] [review] Deal with non-ui-thread IPDL usage on windows Review of attachment 8421734 [details] [diff] [review]: ----------------------------------------------------------------- Does this make the IPDL tests pass on Windows finally? ::: ipc/glue/MessageChannel.h @@ +159,5 @@ > protected: > // The deepest sync stack frame for this channel. > SyncStackFrame* mTopFrame; > > + bool mIsOffMainThread; I think this variable is named incorrectly, as it is I would expect this to be set once and never modified afterward. Maybe 'mIsSyncWaitingOnNonMainThread'? Something that indicates it's temporary... ::: ipc/glue/WindowsMessageLoop.cpp @@ +594,5 @@ > + gUIThreadId = GetCurrentThreadId(); > + } > + NS_ASSERTION(gUIThreadId, "ThreadId should not be 0!"); > + NS_ASSERTION(gUIThreadId == GetCurrentThreadId(), > + "Called InitUIThread multiple times on different threads!"); Might as well switch these to MOZ_ASSERT? @@ +740,5 @@ > MessageChannel::WaitForSyncNotify() > { > mMonitor->AssertCurrentThreadOwns(); > > + if (GetCurrentThreadId() != gUIThreadId) { Hrm, I can't find much online about the speed of this function, but do you think this will be as fast or faster than TLS? @@ +744,5 @@ > + if (GetCurrentThreadId() != gUIThreadId) { > + PRIntervalTime timeout = (kNoTimeout == mTimeoutMs) ? > + PR_INTERVAL_NO_TIMEOUT : > + PR_MillisecondsToInterval(mTimeoutMs); > + // XXX could optimize away this syscall for "no timeout" case if desired Yeah, I would go ahead and do this. @@ +751,5 @@ > + mIsOffMainThread = true; > + > + mMonitor->Wait(timeout); > + > + mIsOffMainThread = false; Hrm, this can be called multiple times on the same thread right? Shouldn't this do something like: bool wasOffMainThread = mIsOffMainThread; mIsOffMainThread = true; mMonitor->Wait(timeout); mIsOffMainThread = wasOffMainThread;

Thanks a lot for the quick review. (In reply to ben turner [:bent] (use the needinfo? flag!) from comment #1) > Comment on attachment 8421734 [details] [diff] [review] > Deal with non-ui-thread IPDL usage on windows > > Review of attachment 8421734 [details] [diff] [review]: > ----------------------------------------------------------------- > > Does this make the IPDL tests pass on Windows finally? > > ::: ipc/glue/MessageChannel.h > @@ +159,5 @@ > > protected: > > // The deepest sync stack frame for this channel. > > SyncStackFrame* mTopFrame; > > > > + bool mIsOffMainThread; > > I think this variable is named incorrectly, as it is I would expect this to > be set once and never modified afterward. > > Maybe 'mIsSyncWaitingOnNonMainThread'? Something that indicates it's > temporary... Sounds good! > @@ +740,5 @@ > > MessageChannel::WaitForSyncNotify() > > { > > mMonitor->AssertCurrentThreadOwns(); > > > > + if (GetCurrentThreadId() != gUIThreadId) { > > Hrm, I can't find much online about the speed of this function, but do you > think this will be as fast or faster than TLS? I personally believe so, yes. > @@ +744,5 @@ > > + if (GetCurrentThreadId() != gUIThreadId) { > > + PRIntervalTime timeout = (kNoTimeout == mTimeoutMs) ? > > + PR_INTERVAL_NO_TIMEOUT : > > + PR_MillisecondsToInterval(mTimeoutMs); > > + // XXX could optimize away this syscall for "no timeout" case if desired > > Yeah, I would go ahead and do this. I simply copied this from the existing code for cross-platform message loops, is it wise to do something additional inconsistent with what other platforms do? Alternatively I could do this for all platforms :). > > @@ +751,5 @@ > > + mIsOffMainThread = true; > > + > > + mMonitor->Wait(timeout); > > + > > + mIsOffMainThread = false; > > Hrm, this can be called multiple times on the same thread right? Shouldn't > this do something like: I don't think that's true? Doesn't this monitor simply block -this- thread until the next run.

This addresses the issues I didn't reply to with explicit questions (and fixes the syscall just for my new codepath). It also adds some new assertions and fixes a bug my old patch had where the UI thread would not be initialized for child plugins. I haven't tried any tests that aren't currently run, they might pass?

Moved one init call a little to make sure it gets executed before any IPDL code.

Comment on attachment 8422875 [details] [diff] [review] Deal with non-ui-thread IPDL usage on windows v3 Review of attachment 8422875 [details] [diff] [review]: ----------------------------------------------------------------- Ok, this looks good! Thanks. ::: ipc/glue/WindowsMessageLoop.cpp @@ +594,5 @@ > + gUIThreadId = GetCurrentThreadId(); > + } > + MOZ_ASSERT(gUIThreadId); > + MOZ_ASSERT(gUIThreadId == GetCurrentThreadId(), > + "Called InitUIThread multiple times on different threads!"); Nit: Alignment @@ +746,5 @@ > + if (GetCurrentThreadId() != gUIThreadId) { > + PRIntervalTime timeout = (kNoTimeout == mTimeoutMs) ? > + PR_INTERVAL_NO_TIMEOUT : > + PR_MillisecondsToInterval(mTimeoutMs); > + // XXX could optimize away this syscall for "no timeout" case if desired Ah, I missed that this was copied. @@ +748,5 @@ > + PR_INTERVAL_NO_TIMEOUT : > + PR_MillisecondsToInterval(mTimeoutMs); > + // XXX could optimize away this syscall for "no timeout" case if desired > + PRIntervalTime waitStart = 0; > + Nit: Extra whitespace. @@ +753,5 @@ > + if (timeout != PR_INTERVAL_NO_TIMEOUT) { > + waitStart = PR_IntervalNow(); > + } > + > + mIsSyncWaitingOnNonMainThread = true; Let's first assert that mIsSyncWaitingOnNonMainThread is false before setting here. @@ +757,5 @@ > + mIsSyncWaitingOnNonMainThread = true; > + > + mMonitor->Wait(timeout); > + > + mIsSyncWaitingOnNonMainThread = false; And assert that it's true before resetting here. @@ +1026,5 @@ > MessageChannel::NotifyWorkerThread() > { > mMonitor->AssertCurrentThreadOwns(); > + > + if (mIsSyncWaitingOnNonMainThread) { Maybe also assert here that GetCurrentThreadId() != gUIThreadId

(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #5) > Comment on attachment 8422875 [details] [diff] [review] > Deal with non-ui-thread IPDL usage on windows v3 > > Review of attachment 8422875 [details] [diff] [review]: > ----------------------------------------------------------------- > > @@ +1026,5 @@ > > MessageChannel::NotifyWorkerThread() > > { > > mMonitor->AssertCurrentThreadOwns(); > > + > > + if (mIsSyncWaitingOnNonMainThread) { > > Maybe also assert here that GetCurrentThreadId() != gUIThreadId This particular bit I didn't do. This should always come in on the IO thread I believe, this notification is not executed from the waiting thread. We could add a general assertion at the start of this function of course. Since this said 'may' I decided to leave it for now, I'll happily deal with it in a follow-up! :)

(In reply to Bas Schouten (:bas.schouten) from comment #6) No, I think that's fine. Thanks!

belated review nit - some commenting explaining these changes would have been nice!