1323566 - Incorrect index usage in MediaKeySystemAccessManager::Observe()

Status: VERIFIED FIXED

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

Description

[Maks Naumov]

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:50.0) Gecko/20100101 Firefox/50.0
Build ID: 20161208153507

Steps to reproduce:

There is a typo:

for (size_t i = mRequests.Length(); i > 0; i--) {
    const size_t index = i - i; <<<<<<<<
    PendingRequest& request = mRequests[index];
      nsAutoCString message;
      MediaKeySystemStatus status =
      MediaKeySystemAccess::GetKeySystemStatus(request.mKeySystem, message);
      if (status == MediaKeySystemStatus::Cdm_not_installed) {
        // Not yet installed, don't retry. Keep waiting until timeout.
        continue;
      }
...
}

"index" will always be 0.

Proper code:
const size_t index = i - 1;

Comment 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

D'oh! Yup that's a typo. Thanks for reporting this bug!

Comment 2

[James Cheng[:JamesCheng]]

This reverse for loop reminds me of this bug https://bugzilla.mozilla.org/show_bug.cgi?id=1302881
I've fixed before.

Comment 3

[James Cheng[:JamesCheng]]

Attachment: Bug 1323566 - Fix incorrect index usage in MediaKeySystemAccessManager::Observe().

Review commit: https://reviewboard.mozilla.org/r/98726/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/98726/

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8818778 [details]
Bug 1323566 - Fix incorrect index usage in MediaKeySystemAccessManager::Observe().

https://reviewboard.mozilla.org/r/98726/#review99048

Unfortunately, this patch causes us to hit another bug that we didn't trigger before since the previous code was broken.

Steps to reproduce:
1. Toggle "Play DRM content" off and then on again in about:preferences#content.
2. Open http://bitmovin.com/mpeg-dash-hls-drm-test-player/
3. Observe assertion failure once the Widevine CDM has had time to download, with this stack:

nss3.dll!PR_Lock(PRLock * lock) Line 214	C
xul.dll!mozilla::OffTheBooksMutex::Lock() Line 382	C++
xul.dll!mozilla::BaseAutoLock<mozilla::StaticMutex>::BaseAutoLock<mozilla::StaticMutex>(mozilla::StaticMutex & aLock, mozilla::detail::GuardObjectNotifier && _notifier) Line 165	C++
xul.dll!mozilla::gmp::GeckoMediaPluginServiceChild::HasPluginForAPI(const nsACString_internal & aAPI, nsTArray<nsCString> * aTags, bool * aHasPlugin) Line 216	C++
xul.dll!mozilla::HaveGMPFor(const nsCString & aAPI, nsTArray<nsCString> && aTags) Line 222	C++
xul.dll!mozilla::dom::HavePluginForKeySystem(const nsCString & aKeySystem) Line 103	C++
xul.dll!mozilla::dom::EnsureCDMInstalled(const nsAString_internal & aKeySystem, nsACString_internal & aOutMessage) Line 118	C++
xul.dll!mozilla::dom::MediaKeySystemAccess::GetKeySystemStatus(const nsAString_internal & aKeySystem, nsACString_internal & aOutMessage) Line 167	C++
xul.dll!mozilla::dom::MediaKeySystemAccessManager::Observe(nsISupports * aSubject, const char * aTopic, const char16_t * aData) Line 276	C++
xul.dll!nsObserverList::NotifyObservers(nsISupports * aSubject, const char * aTopic, const char16_t * someData) Line 112	C++
xul.dll!nsObserverService::NotifyObservers(nsISupports * aSubject, const char * aTopic, const char16_t * aSomeData) Line 285	C++
xul.dll!mozilla::gmp::GeckoMediaPluginServiceChild::UpdateGMPCapabilities(nsTArray<mozilla::dom::GMPCapabilityData> && aCapabilities) Line 209	C++
xul.dll!mozilla::dom::ContentChild::RecvGMPsChanged(nsTArray<mozilla::dom::GMPCapabilityData> && capabilities) Line 1236	C++

The problem is that GeckoMediaPluginServiceChild::UpdateGMPCapabilities() is holding sGMPCapabilitiesMutex, and then GeckoMediaPluginServiceChild::HasPluginForAPI() tries to take the mutex again and it's not re-entrant and so it asserts.

One solution would be to release the mutex in GeckoMediaPluginServiceChild::UpdateGMPCapabilities() before we send the "gmp-changed" observer service notification.

r+ with that fixed.

Comment 5

[James Cheng[:JamesCheng]]

Comment on attachment 8818778 [details]
Bug 1323566 - Fix incorrect index usage in MediaKeySystemAccessManager::Observe().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/98726/diff/1-2/

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f14683f7fad5
Fix incorrect index usage in MediaKeySystemAccessManager::Observe(). r=cpearce

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f14683f7fad5

Comment 9

[Haik Aftandilian [:haik]]

Per bug 1333178, we need this uplifted to 52. The recursive mutex entry issue described in comment 4 causes full browser lockup when trying to watch a Widevine video for the first time.

Comment 10

[Haik Aftandilian [:haik]]

[Tracking Requested - why for this release]:
The locking issue fixed here causes a full browser hang.

Comment 11

[James Cheng[:JamesCheng]]

Comment on attachment 8818778 [details]
Bug 1323566 - Fix incorrect index usage in MediaKeySystemAccessManager::Observe().

This patch is verified, it has no conflict to merge into beta branch.

Approval Request Comment
[Feature/Bug causing the regression]:Bug 1323566, Bug 1333178
[User impact if declined]:The recursive mutex entry issue causes full browser lockup when trying to watch a Widevine video for the first time.
[Is this code covered by automated tests?]:Yes
[Has the fix been verified in Nightly?]:Yes
[Needs manual test from QE? If yes, steps to reproduce]: None.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]:No
[Why is the change risky/not risky?]: not risky. We have verified it on Nightly for a period of time.
[String changes made/needed]:None.

Comment 12

[Julien Cristau [:jcristau]]

Comment on attachment 8818778 [details]
Bug 1323566 - Fix incorrect index usage in MediaKeySystemAccessManager::Observe().

EME fix for browser hang, beta52+

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/3d7169784902

Comment 14

[Andrei Vaida [:avaida]]

Flagging this for verification, instructions in Comment 4.

Comment 15

[Mihai Boldan, Desktop QA [:mboldan]]

(In reply to Chris Pearce (:cpearce) from comment #4)
> Comment on attachment 8818778 [details]
> Bug 1323566 - Fix incorrect index usage in
> MediaKeySystemAccessManager::Observe().
> 
> https://reviewboard.mozilla.org/r/98726/#review99048
> 
> Unfortunately, this patch causes us to hit another bug that we didn't
> trigger before since the previous code was broken.
> 
> Steps to reproduce:
> 1. Toggle "Play DRM content" off and then on again in
> about:preferences#content.
> 2. Open http://bitmovin.com/mpeg-dash-hls-drm-test-player/
> 3. Observe assertion failure once the Widevine CDM has had time to download,
> with this stack:
> 
> nss3.dll!PR_Lock(PRLock * lock) Line 214	C
> xul.dll!mozilla::OffTheBooksMutex::Lock() Line 382	C++
> xul.dll!mozilla::BaseAutoLock<mozilla::StaticMutex>::BaseAutoLock<mozilla::
> StaticMutex>(mozilla::StaticMutex & aLock,
> mozilla::detail::GuardObjectNotifier && _notifier) Line 165	C++
> xul.dll!mozilla::gmp::GeckoMediaPluginServiceChild::HasPluginForAPI(const
> nsACString_internal & aAPI, nsTArray<nsCString> * aTags, bool * aHasPlugin)
> Line 216	C++
> xul.dll!mozilla::HaveGMPFor(const nsCString & aAPI, nsTArray<nsCString> &&
> aTags) Line 222	C++
> xul.dll!mozilla::dom::HavePluginForKeySystem(const nsCString & aKeySystem)
> Line 103	C++
> xul.dll!mozilla::dom::EnsureCDMInstalled(const nsAString_internal &
> aKeySystem, nsACString_internal & aOutMessage) Line 118	C++
> xul.dll!mozilla::dom::MediaKeySystemAccess::GetKeySystemStatus(const
> nsAString_internal & aKeySystem, nsACString_internal & aOutMessage) Line 167
> C++
> xul.dll!mozilla::dom::MediaKeySystemAccessManager::Observe(nsISupports *
> aSubject, const char * aTopic, const char16_t * aData) Line 276	C++
> xul.dll!nsObserverList::NotifyObservers(nsISupports * aSubject, const char *
> aTopic, const char16_t * someData) Line 112	C++
> xul.dll!nsObserverService::NotifyObservers(nsISupports * aSubject, const
> char * aTopic, const char16_t * aSomeData) Line 285	C++
> xul.dll!mozilla::gmp::GeckoMediaPluginServiceChild::
> UpdateGMPCapabilities(nsTArray<mozilla::dom::GMPCapabilityData> &&
> aCapabilities) Line 209	C++
> xul.dll!mozilla::dom::ContentChild::RecvGMPsChanged(nsTArray<mozilla::dom::
> GMPCapabilityData> && capabilities) Line 1236	C++
> 
> The problem is that GeckoMediaPluginServiceChild::UpdateGMPCapabilities() is
> holding sGMPCapabilitiesMutex, and then
> GeckoMediaPluginServiceChild::HasPluginForAPI() tries to take the mutex
> again and it's not re-entrant and so it asserts.
> 
> One solution would be to release the mutex in
> GeckoMediaPluginServiceChild::UpdateGMPCapabilities() before we send the
> "gmp-changed" observer service notification.
> 
> r+ with that fixed.

I tried to reproduce this issue using this build https://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-central-win64-debug/1482012049/ , under Windows 10x64, without any success. 

Can you please help me with more details for this issue?
- an affected build;
- exactly what is the difference between affected and unaffected builds;(what should I pay attention too)

Thanks,
Mihai

Comment 16

[Paul Silaghi, QA [:pauly]]

Reproduced the assertion on Nightly 2016-12-14 linux64-asan-debug build, Ubuntu 14.04.
Verified fixed Fx 53b2 asan-debug build.