Closed Bug 1837570 Opened 1 year ago Closed 1 year ago

StateMirroring cannot guarantee ordering with other tasks before async init

Categories

(Core :: XPCOM, defect)

defect

Tracking

()

RESOLVED FIXED
116 Branch
Tracking Status
firefox116 --- fixed

People

(Reporter: pehrsons, Assigned: pehrsons)

References

(Blocks 2 open bugs)

Details

Attachments

(9 files, 1 obsolete file)

(deleted), text/x-phabricator-request
Details
(deleted), text/x-phabricator-request
Details
(deleted), text/x-phabricator-request
Details
(deleted), text/x-phabricator-request
Details
(deleted), text/x-phabricator-request
Details
(deleted), text/x-phabricator-request
Details
(deleted), text/x-phabricator-request
Details
(deleted), text/x-phabricator-request
Details
(deleted), text/x-phabricator-request
Details

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:

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.

It is const and cannot be moved.

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.

Attachment #9338265 - Attachment description: Bug 1837570 - Use canonical-initialization for conduit mirrors. r?bwc! → Bug 1837570 - Use canonical-initiated connections for webrtc mirrors. r?bwc!

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).

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)

Status: NEW → ASSIGNED
Severity: -- → S2
Pushed by pehrsons@gmail.com: 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

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

Flags: needinfo?(apehrson)

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.

Flags: needinfo?(apehrson)
Blocks: 1839889

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.

Attachment #9339292 - Attachment is obsolete: true
Pushed by pehrsons@gmail.com: 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

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

Pushed by pehrsons@gmail.com: https://hg.mozilla.org/integration/autoland/rev/ec702239c313 Canonical::ConnectMirror must also sync its value.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: