1259450 - Make the compositor thread a nsThread instead of a chromium thread

Status: RESOLVED DUPLICATE of bug 1634253

(Core :: Graphics: Layers, defect, P3)

Description

[Kartikaya Gupta (email:[e-mail])]

We keep running into different bugs with the chromium MessageLoop code, and as khuey keeps telling us, we should stop using it. In the APZ code the main reason we ended up using the MessageLoop code is because the compositor thread is a chromium thread/MessageLoop [1]. We should convert it to a nsThread and that will unblock a bunch of other conversion.

http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/CompositorBridgeParent.cpp?rev=acd6d1120ea1#200

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: change the compositor thread to run an xpcom event loop as well

The tree is frighteningly green, but perhaps I did not run enough tests:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cc942159466f8e1a104bbe0f8055d6ce54ab02cb

This change does not quite accomplish the stated purpose of the bug.  We could
do an alternative:

1. Commit this patch in a separate bug.
2. Compositor thread now has an XPCOM event loop running.
3. Convert all dispatches through CompositorThread::Loop to go through
   NS_GetCurrentThread()->Dispatch() or equivalent.
3a. Stamp out uses of GetCompositorThread.
4. Figure out how to make nsThread start threads with
   TYPE_MOZILLA_NONMAINUITHREAD loops.
5. Move CompositorThread over to an actual nsThread.
6. Fin.

botond, WDYT?  Tagging jimm as well for review, since he wrote the original
support for this kind of message loop, and he might have opinions on its use?

Comment 2

[Nathan Froyd [:froydnj]]

Also, bleh, I guess this only does the trick on Windows, depending on what the default is?

Comment 3

[Botond Ballo [:botond]]

Comment on attachment 9011558 [details] [diff] [review]
change the compositor thread to run an xpcom event loop as well

Review of attachment 9011558 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not really an authority on this code, but the attempt seems reasonable to me, can't think of any obvious issues, so I'm inclined to give it a try if it's passing tests.

Let's run it by Kats as well in case he's aware of any issues this may bump into.

::: gfx/layers/ipc/CompositorThread.cpp
@@ +96,5 @@
>     * such the thread is a gui thread, and must process a windows message queue or
> +   * risk deadlocks. Chromium's TYPE_UI would do what we want here, but it's also
> +   * convenient to have an XPCOM event loop running; TYPE_MOZILLA_NONMAINUITHREAD
> +   * provides both desirable behaviors. */
> +  options.message_loop_type = MessageLoop::TYPE_MOZILLA_NONMAINUITHREAD;

Do we want to set it to TYPE_MOZILLA_NONMAINTHREAD (without the UI) on non-Windows platforms?

Comment 4

[Botond Ballo [:botond]]

(In reply to Nathan Froyd [:froydnj] from comment #2)
> Also, bleh, I guess this only does the trick on Windows, depending on what
> the default is?

Right, the default is TYPE_DEFAULT, hence my suggestion about using TYPE_MOZILLA_NONMAINTHREAD on other platforms.

(In reply to Nathan Froyd [:froydnj] from comment #1)
> This change does not quite accomplish the stated purpose of the bug.  We
> could
> do an alternative:
> 
> 1. Commit this patch in a separate bug.
> 2. Compositor thread now has an XPCOM event loop running.
> 3. Convert all dispatches through CompositorThread::Loop to go through
>    NS_GetCurrentThread()->Dispatch() or equivalent.
> 3a. Stamp out uses of GetCompositorThread.
> 4. Figure out how to make nsThread start threads with
>    TYPE_MOZILLA_NONMAINUITHREAD loops.
> 5. Move CompositorThread over to an actual nsThread.
> 6. Fin.
> 
> botond, WDYT?

That sounds like a reasonable plan to me, though there may be subtleties I'm not familiar with / overlooking. In any case, it seems worth giving it a try.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

So my concern here would be that step 3 in the plan (conversion of dispatches) might be done piecemeal which might result in weird interleavings of events. IIRC the XPCOM loop and the chromium event loop are separate and so if previously things were just getting posted to the chromium loop they would get run in that order, but now if some events are going to the XPCOM loop they may run out-of-order with events on the chromium loop which would introduce weird race conditions and such. Do you know if that's still a problem? It's been a while since I had to deal with that so my memory might be faulty.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 9011558 [details] [diff] [review]
change the compositor thread to run an xpcom event loop as well

Review of attachment 9011558 [details] [diff] [review]:
-----------------------------------------------------------------

Clearing flag pending comment 5. If that comment is not a concern then I have no issues with this patch.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Forward-duping to bug 1634253.