1634846 - MessageChannel can't work with TaskQueue

Status: RESOLVED FIXED

(Core :: IPC, enhancement)

Description

[Jean-Yves Avenard [:jya]]

MessageChannel::Open takes an nsIEventTarget ;

When opening, it dispatches a task on this nsIEventTarget that calls MessageChannel::OnOpenAsSlave [1]

When OnOpenAsSlave runs, it stores the actual PRThread the task is running on [2]. And then in various places will assert that the current worker thread is the same as the actual thread it's running on [3].

When using a ThreadPool or a TaskQueue, you're never guaranteed that the actual thread the code is running on will always be the same.

So at present, while MessageChannel takes a nsIEventTarget as constructor, this nsIEventTarget can only ever be a nsThread.

[1] https://searchfox.org/mozilla-central/source/ipc/glue/MessageChannel.cpp#874
[2] https://searchfox.org/mozilla-central/source/ipc/glue/MessageChannel.cpp#904
[3] https://searchfox.org/mozilla-central/source/ipc/glue/MessageChannel.h#550

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:jld, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 2

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

MessageChannel (and IPDL in general) inherently needs a serial event target, so it will never work with ThreadPool. But in principle it (and the associated checks that things are happening on the correct thread) could be generalized to work with abstract threads, like TaskQueue.

Comment 3

[Jean-Yves Avenard [:jya]]

(In reply to Jed Davis [:jld] ⟨⏰|UTC-6⟩ ⟦he/him⟧ from comment #2)

MessageChannel (and IPDL in general) inherently needs a serial event target, so it will never work with ThreadPool. But in principle it (and the associated checks that things are happening on the correct thread) could be generalized to work with abstract threads, like TaskQueue.

TaskQueue is a nsISerialEventTarget object.

The main issue is what MessageChannel is doing to ensure you're on the right thread; It simply compares pointers of the PR_Thread which is the obviously wrong thing to do.

It could store the original nsISerialEventTarget is use is IsOnThread() method instead.

This would make it work with TaskQueue.

Comment 4

[Jean-Yves Avenard [:jya]]

(In reply to Jed Davis [:jld] ⟨⏰|UTC-6⟩ ⟦he/him⟧ from comment #2)

MessageChannel (and IPDL in general) inherently needs a serial event target, so it will never work with ThreadPool. But in principle it (and the associated checks that things are happening on the correct thread) could be generalized to work with abstract threads, like TaskQueue.

I should have provided more details about the bug originally mentioning ThreadPool.

This is the only implementation we have that is a nsIEventTarget and not a nsISerialEventTarget ;

The prototyping of IToplevelProtocol methods are to use nsIEventTarget ; which is nonsensical really as it must be a nsISerialEventTarget.

Comment 5

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1634846 - P1. Remove unused method. r?nika,jld

And the less use of MessageLoop, the better.

Comment 6

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1634846 - P2. Make ipc's MessageChannel works with TaskQueue, r?nika,jld

We now pass the target nsISerialEventTarget all the way down to MessageChannel::CommonThreadOpenInit so that we don't have to rely on GetCurrentSerialEventTarget() to return the proper object (see bug 1637500).

Even when bug 1637500 will be fully fixed, it avoids any ambiguities as to what target thread we want to use.

Depends on D80654

Comment 7

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1634846 - P2. Make ipc's MessageChannel works with TaskQueue, r?nika,jld

We no longer rely of having a message loop for the worker thread.

Comment 8

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1634846 - P3. Get around NS_INLINE_DECL_REFCOUNTING not working with TaskQueue. r=nika!,froydnj!

NS_INLINE_DECL_REFCOUNTING macro doesn't properly work when the object is used on a thread that isn't backed by a single PRThread (such as TaskQueue). See bug 1648031.

The resolution of this issue is rather complex, and outside the scope of this series of change.

So for now, we create a new macro NS_INLINE_DECL_REFCOUNTING_ONEVENTTHREAD which will use a different mechanism to ensure the thread-safe usage of a class.

Comment 9

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1634846 - P4. Make RemoteDecoder use a background taskqueue. r?mattwoodrow

There's a small race that can happen when the remote decoder gets shutdown during xpcom shutdown; that would cause GetCurrentSerialEventTarget to return null. Leading to an assertion failure in ActorLifecycleProxy thread-safety check when PRemoteDecoderManagerParent gets destroyed.

So we use a background taskqueue instead and cleanup a bit the threading code in there allowed thanks to the TaskQueue ability to not require an explicit shutdown.

Comment 10

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1634846 - P5. Fix data race on STS loop start. r?valentin,honza

The STS dispatched the first event before setting mThread member. It was possible that the STS::Run() task got started before mThread was set and causing one of the nsISerialEventTarget methods to read mThread/mInitialised/mShuttingDown value as null or zero and they would have returned an error.

We rewrite the access to mThread to be lockless.

Comment 11

[Phabricator Automation]

Comment on attachment 9159699 [details]
Bug 1634846 - P5. Fix data race on STS loop start. r?valentin,honza

Revision D81334 was moved to bug 1648898. Setting attachment 9159699 [details] to obsolete.

Comment 12

[Phabricator Automation]

Comment on attachment 9159605 [details]
Bug 1634846 - P4. Make RemoteDecoder use a background taskqueue. r?mattwoodrow

Revision D81287 was moved to bug 1649294. Setting attachment 9159605 [details] to obsolete.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d539408a0048
P1. Remove unused method. r=nika,jld
https://hg.mozilla.org/integration/autoland/rev/bca79526745d
P2. Make ipc's MessageChannel works with TaskQueue, r=nika
https://hg.mozilla.org/integration/autoland/rev/5863f9c5229f
P3. Get around NS_INLINE_DECL_REFCOUNTING not working with TaskQueue. r=nika,froydnj

Comment 14

[Narcis Beleuzu [:NarcisB]]

Backed out for webgl-conf crashes

Backout link: https://hg.mozilla.org/integration/autoland/rev/ed44d4e62140475b5daba4e7db38808dfea95960
Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=307988602&repo=autoland&lineNumber=3484

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/681e77b6fe1b
P1. Remove unused method. r=nika,jld
https://hg.mozilla.org/integration/autoland/rev/ffa41c2b08b3
P2. Make ipc's MessageChannel works with TaskQueue, r=nika
https://hg.mozilla.org/integration/autoland/rev/132e64564bdd
P3. Get around NS_INLINE_DECL_REFCOUNTING not working with TaskQueue. r=nika,froydnj

Comment 16

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/681e77b6fe1b
https://hg.mozilla.org/mozilla-central/rev/ffa41c2b08b3
https://hg.mozilla.org/mozilla-central/rev/132e64564bdd