1719577 - Clean up locking in MessageChannel

Status: RESOLVED FIXED

(Core :: IPC, task)

Comment 1

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1719577 - Part 1: Use inline initializers for MessageChannel fields, r=handyman

This makes things generally more clear, and avoids a long list of initializers
in the MessageChannel constructor. In addition, some fields which are never
modified are marked as const.

Comment 2

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1719577 - Part 2: Create mMonitor eagerly in MessageChannel's constructor, r=handyman

Now that PortLink is the only MessageLink implementation, it is no longer
necessary to support sharing a single RefCountedMonitor between multiple
MessageChannels, meaning that we can construct the monitor directly in the
MessageChannel constructor. The monitor still needs to be refcounted due to
being used by the PortLink as part of the listener implementation.

Comment 3

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1719577 - Part 3: Remove dead WaitForIncomingMessage function, r=handyman

This method is never called.

Comment 4

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1719577 - Part 4: Hold the monitor for more of `MessageChannel::Open`, r=handyman

This simplifies some of the reasoning about when to hold locks by holding it
when setting all monitor-guarded fields.

Comment 5

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1719577 - Part 5: Remove the now-unused `ChannelOpening` state, r=handyman

This state was only used by the ProcessLink MessageLink implementation, and no
longer exists with the new PortLink implementation, so can be removed.

Comment 6

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1719577 - Part 6: More consistently use the monitor during MessageChannel shutdown, r=handyman

This should make the logic around clearing a MessageChannel more obviously
correct by holding the mutex when accessing fields which are traditionally
guarded by the mutex. These lock calls shouldn't introduce performance issues
as the lock should be uncontended.

Comment 7

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1719577 - Part 7: Remove outdated comment, r=handyman

This comment refers to the old ProcessLink implementation, and was missed when
that implementation was removed.

Comment 8

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1719577 - Part 8: Remove some unused methods from MessageChannel, r=handyman

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b7c5eb05364c
Part 1: Use inline initializers for MessageChannel fields, r=handyman
https://hg.mozilla.org/integration/autoland/rev/68e1d5aaf856
Part 2: Create mMonitor eagerly in MessageChannel's constructor, r=handyman
https://hg.mozilla.org/integration/autoland/rev/6c7b991eb21d
Part 3: Remove dead WaitForIncomingMessage function, r=handyman
https://hg.mozilla.org/integration/autoland/rev/cc1b04b6f68c
Part 4: Hold the monitor for more of `MessageChannel::Open`, r=handyman
https://hg.mozilla.org/integration/autoland/rev/bb78618b5c81
Part 5: Remove the now-unused `ChannelOpening` state, r=handyman
https://hg.mozilla.org/integration/autoland/rev/105c1a4bebae
Part 6: More consistently use the monitor during MessageChannel shutdown, r=handyman
https://hg.mozilla.org/integration/autoland/rev/1b5b806c2f73
Part 7: Remove outdated comment, r=handyman
https://hg.mozilla.org/integration/autoland/rev/766d12d3f3fe
Part 8: Remove some unused methods from MessageChannel, r=handyman

Comment 10

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/b7c5eb05364c
https://hg.mozilla.org/mozilla-central/rev/68e1d5aaf856
https://hg.mozilla.org/mozilla-central/rev/6c7b991eb21d
https://hg.mozilla.org/mozilla-central/rev/cc1b04b6f68c
https://hg.mozilla.org/mozilla-central/rev/bb78618b5c81
https://hg.mozilla.org/mozilla-central/rev/105c1a4bebae
https://hg.mozilla.org/mozilla-central/rev/1b5b806c2f73
https://hg.mozilla.org/mozilla-central/rev/766d12d3f3fe