StateMirroring cannot guarantee ordering with other tasks before async init
Categories
(Core :: XPCOM, defect)
Tracking
()
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:
- 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.
Assignee | ||
Comment 1•1 year ago
|
||
Assignee | ||
Comment 2•1 year ago
|
||
Assignee | ||
Comment 3•1 year ago
|
||
Assignee | ||
Comment 4•1 year ago
|
||
Assignee | ||
Comment 5•1 year ago
|
||
It is const and cannot be moved.
Assignee | ||
Comment 6•1 year ago
|
||
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.
Updated•1 year ago
|
Assignee | ||
Comment 7•1 year ago
|
||
Assignee | ||
Comment 8•1 year ago
|
||
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).
Assignee | ||
Comment 9•1 year ago
|
||
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)
Assignee | ||
Updated•1 year ago
|
Updated•1 year ago
|
Comment 10•1 year ago
|
||
Comment 11•1 year ago
|
||
Backed out for causing failures on browser_utility_audio_shutdown.js, test_seamless_looping.html, browser_audio_telemetry_content.js
Assignee | ||
Comment 12•1 year ago
|
||
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•1 year ago
|
||
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•1 year ago
|
||
Assignee | ||
Comment 15•1 year ago
|
||
This is a fixup after a bad adaptation of a review comment on D180427 regressed
the value syncing on connect.
Comment 16•1 year ago
|
||
Comment 17•1 year ago
|
||
bugherder |
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
Description
•