1009590 - Allow cross-thread IPDL on Windows

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Bas Schouten (:bas.schouten)]

Attachment: Deal with non-ui-thread IPDL usage on windows

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 1

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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;

Comment 2

[Bas Schouten (:bas.schouten)]

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.

Comment 3

[Bas Schouten (:bas.schouten)]

Attachment: Deal with non-ui-thread IPDL usage on windows v2

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?

Comment 4

[Bas Schouten (:bas.schouten)]

Attachment: Deal with non-ui-thread IPDL usage on windows v3

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

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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

Comment 6

[Bas Schouten (:bas.schouten)]

(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! :)

Comment 7

[Bas Schouten (:bas.schouten)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/115aeaefd16b

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/115aeaefd16b

Comment 9

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to Bas Schouten (:bas.schouten) from comment #6)

No, I think that's fine. Thanks!

Comment 10

[Jim Mathies [:jimm]]

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