Bug 1644254 Comment 8 Edit History

Note: The actual edited comment in the bug view page will always show the original commenter’s name and original timestamp.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #6)
> Ah, interesting. For some reason I assumed that after CompositorThreadHolder::Shutdown returned, the CompositorThreadHolder and in particular mCompositorThread would both be destroyed and so the compositor thread itself would have been terminated, and scheduled tasks wouldn't even run any more. But I guess that's not the case, and the ShutdownInternal call scheduled still gets to run later even though there's no reference to the nsIThread anymore?

A task dispatched on a thread keeps a reference to that thread.
So there's a reference to the nsIThread until the last task has run.

What makes things more complicated here; is that some code keep a reference to the CompositorThreadHolder; and some others on the actual thread managed by the CompositorThreadHolder.

Imho, the CompositorThreadHolder shouldn't be refcounted. Particularly because the singleton that is used by static methods gets cleared in shutdown.
And so core around that (particularly the APC and VR code) try to get around that by keeping a reference when they shouldn't.

Accessing the actual thread after CompositorThread::Shutdown has been called is one thing, attempting to keep CompositorThreadHolder alive even after it's been shutdown is unreasonable. It makes the code hard to follow and bugs are easily introduced here.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #6)
> Ah, interesting. For some reason I assumed that after CompositorThreadHolder::Shutdown returned, the CompositorThreadHolder and in particular mCompositorThread would both be destroyed and so the compositor thread itself would have been terminated, and scheduled tasks wouldn't even run any more. But I guess that's not the case, and the ShutdownInternal call scheduled still gets to run later even though there's no reference to the nsIThread anymore?

A task dispatched on a thread keeps a reference to that thread.
So there's a reference to the nsIThread until the last task has run.

What makes things more complicated here; is that some code keep a reference to the CompositorThreadHolder; and some others on the actual thread managed by the CompositorThreadHolder.

Imho, the CompositorThreadHolder shouldn't be refcounted. Particularly because the singleton that is used by static methods gets cleared in shutdown.
And so code around that (particularly the APC and VR code) try to get around that by keeping a reference when they shouldn't.

Accessing the actual thread after CompositorThread::Shutdown has been called is one thing, attempting to keep CompositorThreadHolder alive even after it's been shutdown is unreasonable. It makes the code hard to follow and bugs are easily introduced here.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #6)
> Ah, interesting. For some reason I assumed that after CompositorThreadHolder::Shutdown returned, the CompositorThreadHolder and in particular mCompositorThread would both be destroyed and so the compositor thread itself would have been terminated, and scheduled tasks wouldn't even run any more. But I guess that's not the case, and the ShutdownInternal call scheduled still gets to run later even though there's no reference to the nsIThread anymore?

A task dispatched on a thread keeps a reference to that thread.
So there's a reference to the nsIThread until the last task has run.

What makes things more complicated here; is that some code keep a reference to the CompositorThreadHolder; and some others on the actual thread managed by the CompositorThreadHolder.

Imho, the CompositorThreadHolder shouldn't be refcounted. Particularly because the singleton that is used by static methods gets cleared in shutdown.
And so code around that (particularly the APZ and VR code) try to get around that by keeping a reference when they shouldn't.

Accessing the actual thread after CompositorThread::Shutdown has been called is one thing, attempting to keep CompositorThreadHolder alive even after it's been shutdown is unreasonable. It makes the code hard to follow and bugs are easily introduced here.

Back to Bug 1644254 Comment 8