1505494 - Crash in mozilla::dom::IsWindowAllowedToPlay

Status: RESOLVED FIXED

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

Description

[[:philipp]]

This bug was filed from the Socorro interface and is
report bp-89ce8211-2215-475f-84f3-4d4980181031.
=============================================================

Top 10 frames of crashing thread:

0 xul.dll static bool mozilla::dom::IsWindowAllowedToPlay dom/media/AutoplayPolicy.cpp:94
1 xul.dll static bool mozilla::dom::IsMediaElementAllowedToPlay dom/media/AutoplayPolicy.cpp:148
2 xul.dll mozilla::dom::AutoplayPolicy::IsAllowedToPlay dom/media/AutoplayPolicy.cpp:194
3 xul.dll void mozilla::dom::HTMLMediaElement::UpdatePreloadAction dom/html/HTMLMediaElement.cpp:2505
4 xul.dll mozilla::dom::HTMLMediaElement::AfterSetAttr dom/html/HTMLMediaElement.cpp:4498
5 xul.dll mozilla::dom::Element::SetAttrAndNotify dom/base/Element.cpp:2780
6 xul.dll mozilla::dom::Element::SetAttr dom/base/Element.cpp:2627
7 xul.dll mozilla::dom::HTMLAudioElement::Audio dom/html/HTMLAudioElement.cpp:66
8 xul.dll static bool mozilla::dom::HTMLAudioElement_Binding::_Audio dom/bindings/HTMLAudioElementBinding.cpp:70
9 xul.dll static bool InternalConstruct js/src/vm/Interpreter.cpp:674

=============================================================

this is a tab crash signature starting to show up across platforms in firefox 64. overall it's a fairly low volume crash though.

Comment 1

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

[Tracking Requested - why for this release]: Uptick, tracking flags, and adjacency to 64 feature policy suggest regression.

Seems like `approver` is null here [1].
Missing null-check on line above calling ApproverDocOf() which appears capable of return null in two cases [2].

As for how to repro or why the uptick... I notice the lines directly above this again check for feature policy, and we recently enabled feature policy in Nightly (available with), maybe this changed things somehow? e.g. is this perhaps happening in a (sandboxed?) iframe with allow="camera;microphone" where things are now maybe more strict in edge cases?

Regardless, adding a null-check would seem prudent, and might be all we need to stop this.

[1] https://hg.mozilla.org/mozilla-central/annotate/c3f01d72374b19dab985700e9b839bb7fa956e77/dom/media/AutoplayPolicy.cpp#l94
[2] https://searchfox.org/mozilla-central/rev/17f55aee76b7c4610a974cffd3453454e0c8de7b/dom/media/AutoplayPolicy.cpp#39,45

Comment 2

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Sorry s/camera;microphone/autoplay/. Baku, do we implement feature policy for autoplay yet? [1]

[1] https://searchfox.org/mozilla-central/rev/a3894a4dcfb5d42f2e6eee6cb9faf7141879ef1a/testing/web-platform/meta/html/semantics/embedded-content/media-elements/autoplay-allowed-by-feature-policy-attribute.https.sub.html.ini#4

Comment 3

[Andrea Marchesini [:baku]]

Yes, we do. Bug 1497034.

Comment 4

[Alastor Wu [:alwu]]

Attachment: Bug 1505494 - not allow autoplay if we don't have a document.

In order to avoid the nullptr crash when using `approver`.

Comment 5

[Alastor Wu [:alwu]]

Attachment: Bug 1505494 - remove redundant codes.

There are two exactly same codes here, remove one of them.

Comment 6

[Julien Cristau [:jcristau]]

Not tracking due to low volume; but feel free to request uplift after this is fixed on trunk.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fe1869606e94
not allow autoplay if we don't have a document. r=cpearce
https://hg.mozilla.org/integration/autoland/rev/65987ae1dfca
remove redundant codes. r=cpearce

Comment 8

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/fe1869606e94
https://hg.mozilla.org/mozilla-central/rev/65987ae1dfca

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Please request Beta approval on this when you get a chance.

Comment 10

[Alastor Wu [:alwu]]

Comment on attachment 9024039 [details]
Bug 1505494 - not allow autoplay if we don't have a document.

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: block autoplay

User impact if declined: user might get crash

Is this code covered by automated tests?: No

Has the fix been verified in Nightly?: No

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): This change is only adding a null check

String changes made/needed: no

Comment 11

[Julien Cristau [:jcristau]]

Comment on attachment 9024039 [details]
Bug 1505494 - not allow autoplay if we don't have a document.

fix a low volume crash with block autoplay, approved for 64.0b10

Comment 12

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/releases/mozilla-beta/rev/466cc7c2f4c9