1837570 - StateMirroring cannot guarantee ordering with other tasks before async init

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Andreas Pehrson [:pehrsons]]

State mirroring from canonicals to mirrors is mirror-initialized. This means the mirrors connect to the canonicals on the mirrors' owning thread.

In WebRTC we do this in an async step in an otherwise sync task, which exposes the object owning the canonicals to js synchronously. This can lead to (intermittent) problems if js can trigger dispatches that arrive to the target thread before the first state change task from the canonicals. The main problem is that no assumptions can be made on the state of the mirrors, and handling this would lead to unnecessary complexity.

In short there are races between async mirror setup and other types of dispatches like so:

• mirror setup:
  • async mirror init: main->target
  • Canonical::AddMirror: target->main
  • Initial statechange: main->target
• regular dispatch, say through a MediaEvent:
  • async MediaEvent init: main->target
  • Connect is sync: target
  • MediaEventProducer::Notify: main->target
• regular dispatch, through explicit Dispatch():
  • gets added in bug 1631263 so currently theoretical; main->target

Events will appear to run out of order:

• In the case of these events on the canonical side:
  • Async mirror init
  • MediaEventProducer::Notify
• The mirror side runs them in this order:
  • MediaEvent handler
  • Mirror watch handler

If there was a canonical-initialized mode (a single main->target step), or if mirrors could be added to canonicals synchronously, like how MediaEvent connects, we could make more assumptions on the state of the mirror side already when dispatching regular tasks from the canonical side, and this wouldn't be a problem.

Comment 1

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1837570 - Make WaitFor.h a global gtest helper. r?gbrown!

Comment 2

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1837570 - Add a gtest showing the ordering problem with mirror initialization. r?#xpcom-reviewers

Comment 3

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1837570 - Fix style nits in StateMirroring.h. r?#xpcom-reviewers

Comment 4

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1837570 - To StateMirroring add a canonical-initialization mode. r?#xpcom-reviewers

Comment 5

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1837570 - In AudioConduit don't move-construct mCall->mCallThread. r?bwc!

It is const and cannot be moved.

Comment 6

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1837570 - Use canonical-initiated connections for webrtc mirrors. r?bwc!

The same main thread task that may expose RTCRtpTransceiver (and its receiver
and sender) connects the conduit mirrors through an async task on the call
thread. If js does something that triggers a regular task to be dispatched to
the call thread, that task cannot make assumptions on the state of the conduit.

With this patch enabling canonical-initialization, the conduit mirrors are
connected synchronously, and a task like that mentioned above is guaranteed to
arrive after the first canonical statechange task, meaning it can make
assumptions on the state of the conduit.

Comment 7

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1837570 - Remove unused MediaDecoderStateMachine::CanonicalOutputTracks. r?alwu

Comment 8

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1837570 - In media playback move some state mirroring to (main thread) canonical-initiated connections. r?alwu

This shortens the time it takes to establish the connections for canonicals
living on the main thread, from the thread hop chain:
main thread (MediaDecoderStateMachine::Init)
-> state machine task queue (Mirror::Connect)
-> main thread (AbstractCanonical::AddMirror)
-> state machine task queue (AbstractMirror::UpdateValue)

to the thread hop chain:
main thread (MediaDecoderStateMachine::Init + Canonical::ConnectMirror)
-> state machine task queue (Mirror::AbstractUpdateValue)

No races exist here as MediaDecoderStateMachine initiates its shutdown on main
thread and continues on the task queue (where the mirrors live).

Comment 9

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1837570 - For the ReaderProxy::mDuration mirror do canonical-initiated connect. r?alwu

This shortens the time for when the mirror receives its first mirrored value
from the thread hop chain:
main thread (MediaDecoderStateMachine::Init)
-> reader task queue (Mirror::Connect)
-> state machine task queue (AbstractCanonical::AddMirror)
-> reader task queue (AbstractMirror::UpdateValue)

to the thread hop chain:
main thread (MediaDecoderStateMachine::Init)
-> state machine task queue (Canonical::ConnectMirror)
-> reader task queue (AbstractMirror::UpdateValue)

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ff50fae470fa
Make WaitFor.h a global gtest helper. r=gbrown
https://hg.mozilla.org/integration/autoland/rev/462c505677cc
Add a gtest showing the ordering problem with mirror initialization. r=xpcom-reviewers,nika
https://hg.mozilla.org/integration/autoland/rev/bd367313702f
Fix style nits in StateMirroring.h. r=emilio
https://hg.mozilla.org/integration/autoland/rev/727f6d7e1b61
To StateMirroring add a canonical-initialization mode. r=xpcom-reviewers,nika
https://hg.mozilla.org/integration/autoland/rev/7954e2994d72
In AudioConduit don't move-construct mCall->mCallThread. r=bwc
https://hg.mozilla.org/integration/autoland/rev/e9c7a524c784
Use canonical-initiated connections for webrtc mirrors. r=bwc
https://hg.mozilla.org/integration/autoland/rev/a2f55894361a
Remove unused MediaDecoderStateMachine::CanonicalOutputTracks. r=alwu
https://hg.mozilla.org/integration/autoland/rev/aeeff254fdd7
In media playback move some state mirroring to (main thread) canonical-initiated connections. r=alwu
https://hg.mozilla.org/integration/autoland/rev/1c00f9d954b4
For the ReaderProxy::mDuration mirror do canonical-initiated connect. r=alwu

Comment 11

[Norisz Fay [:noriszfay]]

Backed out for causing failures on browser_utility_audio_shutdown.js, test_seamless_looping.html, browser_audio_telemetry_content.js

Backout link

Push with failures - bc
Push with failures - mda

Failure log - bc
Failure log - mda

Comment 12

[Andreas Pehrson [:pehrsons]]

So I have a try run on a parent rev from June 9 that is green, and now I see that D181086 is making things very very orange.

Looking at the complete history of the dom/media folder this is likely due to either bug 1829054 or bug 1835804.

Comment 13

[Phabricator Automation]

Comment on attachment 9339292 [details]
Bug 1837570 - In media playback move some state mirroring to (main thread) canonical-initiated connections. r?alwu

Revision D181086 was moved to bug 1839889. Setting attachment 9339292 [details] to obsolete.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6b353412497a
Make WaitFor.h a global gtest helper. r=gbrown
https://hg.mozilla.org/integration/autoland/rev/3721ff2d5abd
Add a gtest showing the ordering problem with mirror initialization. r=xpcom-reviewers,nika
https://hg.mozilla.org/integration/autoland/rev/abe91fd244a9
Fix style nits in StateMirroring.h. r=emilio
https://hg.mozilla.org/integration/autoland/rev/ea93fa019f1a
To StateMirroring add a canonical-initialization mode. r=xpcom-reviewers,nika
https://hg.mozilla.org/integration/autoland/rev/65bfe405619f
In AudioConduit don't move-construct mCall->mCallThread. r=bwc
https://hg.mozilla.org/integration/autoland/rev/bce9b6b4d987
Use canonical-initiated connections for webrtc mirrors. r=bwc
https://hg.mozilla.org/integration/autoland/rev/6b10d7701ae5
Remove unused MediaDecoderStateMachine::CanonicalOutputTracks. r=alwu
https://hg.mozilla.org/integration/autoland/rev/7b6834c0a3b0
For the ReaderProxy::mDuration mirror do canonical-initiated connect. r=alwu

Comment 15

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1837570 - Canonical::ConnectMirror must also sync its value.

This is a fixup after a bad adaptation of a review comment on D180427 regressed
the value syncing on connect.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ec702239c313
Canonical::ConnectMirror must also sync its value.

Comment 17

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/6b353412497a
https://hg.mozilla.org/mozilla-central/rev/3721ff2d5abd
https://hg.mozilla.org/mozilla-central/rev/abe91fd244a9
https://hg.mozilla.org/mozilla-central/rev/ea93fa019f1a
https://hg.mozilla.org/mozilla-central/rev/65bfe405619f
https://hg.mozilla.org/mozilla-central/rev/bce9b6b4d987
https://hg.mozilla.org/mozilla-central/rev/6b10d7701ae5
https://hg.mozilla.org/mozilla-central/rev/7b6834c0a3b0
https://hg.mozilla.org/mozilla-central/rev/ec702239c313