1072775 - Improve thread-safety assertions for MediaStreamGraph/GraphDriver

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, defect)

Description

[Randell Jesup [:jesup] (needinfo me)]

MSG has a lot of requirements about data access and states, but these are largely un-asserted.  Adding explicit assertions for all the major cases would greatly speed modifications to MSG by catching things before they become racy-hard-to-debug-oranges.

This is likely to be an on-going effort.

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Additional assertions for MediaStreamGraph/GraphDriver

WIP

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Additional assertions for MediaStreamGraph/GraphDriver

Added OfflineClock shutdown fixes

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/32eacf8f2efe

Comment 4

[Daniel Holbert [:dholbert]]

Backed out:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/0c8faa128dff
for fatal assertion failures in Linux64 tests like:
Assertion failure: lock->locked && pthread_equal(lock->owner, pthread_self()), at /builds/slave/m-in-l64-d-0000000000000000000/build/nsprpub/pr/src/pthreads/ptsynch.c:226

e.g.
https://tbpl.mozilla.org/php/getParsedLog.php?id=49067104&tree=Mozilla-Inbound

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/509e62e4bc1a

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/32eacf8f2efe
https://hg.mozilla.org/mozilla-central/rev/509e62e4bc1a

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8495056 [details] [diff] [review]
Additional assertions for MediaStreamGraph/GraphDriver

This needs to land with bug 1072780; see also the duplicate aurora request there:  (pasted here)

Approval Request Comment
[Feature/regressing bug #]: bug 848954

[User impact if declined]: crashes, test oranges, possible sec issues

[Describe test coverage new/current, TBPL]: some known oranges are likely due to this.  Adds considerable thread-safety assertions, which caught a bug immediately on landing (which someone had been working and had a fix for, but took them far longer to find after it caused an odd crash)

[Risks and why]: fairly low.  Cleans up locking around a number of pointers; of course in theory it may have missed something, but it added asserts about safety/locking.  Main risk would be missing a case where the assert is too draconian (had one of those on the initial landing) and causing an intermittent orange.

[String/UUID change made/needed]: none.

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

Note: should land with the regression patch in bug 1074048

Comment 9

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8495056 [details] [diff] [review]
Additional assertions for MediaStreamGraph/GraphDriver

Aurora+

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c89cf06d1f3f