1746907 - Fix a number of locking issues in GMP support

Status: RESOLVED FIXED

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

Description

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

There are a number of possible locking issues, especially around access to mGMPThread, mShuttingDown, etc.
Also adds an AssertOnGMPThread() method which is most of the changes.

Comment 1

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

Attachment: Bug 1746907: GMP cleanup r=bryce

Comment 2

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Landed: https://hg.mozilla.org/integration/autoland/rev/e2b20899353cd9eb59d68863454eba14efe27720

Backed out: https://hg.mozilla.org/integration/autoland/rev/ba26a5ced2ef60ae3f3a6ce1655f667351d8d1fd
Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&resultStatus=retry%2Ctestfailed%2Cbusted%2Cexception%2Cusercancel&revision=e2b20899353cd9eb59d68863454eba14efe27720&selectedTaskRun=bn0NYbVpR4mFSe8JunDuug.0

[task 2021-12-21T18:03:36.423Z] 18:03:36    ERROR -  /builds/worker/checkouts/gecko/dom/media/gmp/GMPService.h:64:48: error: expected ';' at end of declaration list
[task 2021-12-21T18:03:36.424Z] 18:03:36     INFO -    nsresult GetThreadLocked(nsIThread** aThread) REQUIRES(mMutex);
[task 2021-12-21T18:03:36.424Z] 18:03:36     INFO -                                                 ^
[task 2021-12-21T18:03:36.424Z] 18:03:36     INFO -                                                 ;

Comment 3

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

GMP cleanup r=bryce
https://hg.mozilla.org/integration/autoland/rev/c8e8cfd5f9d83991724bd7815d1c48e6d8dbe563
https://hg.mozilla.org/mozilla-central/rev/c8e8cfd5f9d8

Comment 4

[Karl Tomlinson (:karlt)]

(In reply to Randell Jesup [:jesup] (needinfo me) from comment #0)

There are a number of possible locking issues, especially around access to mGMPThread, mShuttingDown, etc.

What was the issue with mShuttingDown?
Where was it accessed off main thread?

Comment 5

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

The presumption was that it needed protection, because the comments in GMPServiceParent.h had it right after a "Protected by mMutex from the base class" comment (along with mPlugins). When adding GUARDED_BY(mMutex) (patch to land later, though obviously will now be revised), these spots were flagged by clang's thread-safety analysis.