1608286 - Crash in [@ mozilla::dom::MediaKeySystemAccessManager::CheckDoesWindowSupportProtectedMedia]

Status: VERIFIED FIXED

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

Description

[[:philipp]]

This bug is for crash report bp-32f05d12-ef37-4cc8-bfcb-f117d0200109.

Top 10 frames of crashing thread:

0 xul.dll void mozilla::dom::MediaKeySystemAccessManager::CheckDoesWindowSupportProtectedMedia dom/media/eme/MediaKeySystemAccessManager.cpp:137
1 xul.dll mozilla::dom::MediaKeySystemAccessManager::Request dom/media/eme/MediaKeySystemAccessManager.cpp:119
2 xul.dll mozilla::dom::Navigator::RequestMediaKeySystemAccess dom/base/Navigator.cpp:1968
3 xul.dll static bool mozilla::dom::Navigator_Binding::requestMediaKeySystemAccess_promiseWrapper dom/bindings/NavigatorBinding.cpp:2070
4 xul.dll mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ConvertExceptionsToPromises> dom/bindings/BindingUtils.cpp:3153
5 xul.dll js::InternalCallOrConstruct js/src/vm/Interpreter.cpp:548
6 xul.dll static bool Interpret js/src/vm/Interpreter.cpp:3117
7 xul.dll static bool js::RunScript js/src/vm/Interpreter.cpp:423
8 xul.dll js::InternalCallOrConstruct js/src/vm/Interpreter.cpp:589
9 xul.dll static bool js::jit::InvokeFunction js/src/jit/VMFunctions.cpp:261

this low volume crash signature is starting to show up in firefox 72, with MOZ_CRASH(BrowserChild should only be unavailable with e10s off) that was added in bug 1594794.

Comment 1

[Ryan VanderMeulen [:RyanVM]]

Thomas or Bryce, do one of you have cycles to look at this new crash appearing in Fx72?

Comment 2

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

I don't have a lot of cycles at the moment. Thomas, do you have time to look further into this?

My thoughts based on looking at a few reports and the crash site:

• browser is null.
• Our assumption is this should only be the case in non-e10s mode. However, we appear to be hitting the crash in e10s builds based upon the reports.
• There is something else that leads to browser being null. It's not clear to me what. I wonder if we're racing the request against destroying the the window/tab the request came from.

Comment 3

[thomasmo]

Yes-- I can take a look for now

The crash metadata indicates that it is a content process and that e10s is on, so I agree that this could be a race.

@bz, Any initial guesses as to how we end up with !browser?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Hmm. So I thought I'd checked BrowserChild::GetFrom for being able to return null, but clearly I hadn't. It can absolutely return null if the window has been removed from its docshell.

I suspect something like this will crash on Windows:

  <iframe></iframe>
  <script>
    var i = document.querySelector("iframe");
    var nav = i.contentWindow.navigator;
    i.remove();
    nav.requestMediaKeySystemAccess("", []);
  </script>

Comment 5

[thomasmo]

Yes-- can confirm that the above repro hits the crash!

Working on a fix

Comment 6

[thomasmo]

Attachment: Bug 1608286 - Crash in [@ mozilla::dom::MediaKeySystemAccessManager::CheckDoesWindowSupportProtectedMedia]

This changes fixes a failfast in a call to Navigator.requestMediaKeySystemAccess for the case where there is no browser available from a window when e10s is on. This can happen when a document is disconnected from the DOM.
The fix is to reject the promise in this case.

Comment 7

[[:philipp]]

the signature is spiking up in volume during the past 48 hours on release.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b71a801eabd4
Crash in [@ mozilla::dom::MediaKeySystemAccessManager::CheckDoesWindowSupportProtectedMedia] r=bryce,bzbarsky

Comment 9

[Bogdan Tara[:bogdan_tara | bogdant]]

Backed out changeset b71a801eabd4 (Bug 1608286) on tmo's request

Backout link: https://hg.mozilla.org/integration/autoland/rev/c17aeb197b4b77cca7ac857a499263e621f64632

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d430fc486116
Crash in [@ mozilla::dom::MediaKeySystemAccessManager::CheckDoesWindowSupportProtectedMedia] r=bryce,bzbarsky

Comment 11

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

There's some discussion on the review that I think is worth distilling out here: the EME spec is not clear about what should be done if we're requesting media keysystem access in certain scenarios, like the one that arises from the code in comment 4. Thomas has created bug 1610866 to follow up around getting a consistent behaviour specified.

Comment 12

[Alexandru Michis [:malexandru]]

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

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta approval when you get a chance. It grafts cleanly as-landed.

Comment 14

[thomasmo]

Comment on attachment 9121061 [details]
Bug 1608286 - Crash in [@ mozilla::dom::MediaKeySystemAccessManager::CheckDoesWindowSupportProtectedMedia]

Beta/Release Uplift Approval Request

• User impact if declined: If declined, tabs crash in the bug's scenario. Crash hits started trending up recently.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce: If manual testing performed, crash test added here: https://searchfox.org/mozilla-central/source/dom/media/test/crashtests/1608286.html
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): TL;DR: The risk is with site compat; where the behavior would have been to either crash, the promise is now rejected.

This crash is a release assert, introduced in Bug 1594794 (Fx 72), to catch what was originally thought to be an unsupported condition. The unsupported condition is to call Navigator.requestMediaKeySystemAccess with a document that is disconnected from the DOM.
Bug 1594794 fixes a regression from Bug 1581855, in which case there would have been a NULL crash in the repro from both this bug and 1594794. Prior to the work in Bug 1581855 (< Fx71), the result was to not resolve the promise at all.
The risk, then, is that sites that provided a callback for rejection will execute code that hadn't before in this condition (which would be a crashed tab otherwise).

• String changes made/needed:

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9121061 [details]
Bug 1608286 - Crash in [@ mozilla::dom::MediaKeySystemAccessManager::CheckDoesWindowSupportProtectedMedia]

Fixes a crash spiking in Fx72. Approved for 73.0b10.

Comment 16

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/releases/mozilla-beta/rev/4d9ec26e21d4

Comment 17

[Julien Cristau [:jcristau] (back August 18)]

No crash in 73.0b10 and up.