1470932 - Crash in mozilla::MediaManager::IsActivelyCapturingOrHasAPermission

Status: RESOLVED FIXED

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

Description

[Andreas Pehrson [:pehrsons]]

This bug was filed from the Socorro interface and is
report bp-3fa5ecd8-a4b2-44e9-8331-a19730180622.
=============================================================

Top 10 frames of crashing thread:

0 xul.dll mozilla::MediaManager::IsActivelyCapturingOrHasAPermission dom/media/MediaManager.cpp:3951
1 xul.dll mozilla::dom::AutoplayPolicy::IsMediaElementAllowedToPlay dom/media/AutoplayPolicy.cpp:41
2 xul.dll mozilla::dom::HTMLMediaElement::CanActivateAutoplay dom/html/HTMLMediaElement.cpp:6121
3 xul.dll mozilla::dom::HTMLMediaElement::AddRemoveSelfReference dom/html/HTMLMediaElement.cpp:6618
4 xul.dll static void NotifyActivityChanged dom/base/nsDocument.cpp:4566
5 xul.dll nsIDocument::EnumerateActivityObservers dom/base/nsDocument.cpp:9451
6 xul.dll nsIDocument::SetScriptGlobalObject dom/base/nsDocument.cpp:4774
7 xul.dll nsGlobalWindowOuter::SetNewDocument dom/base/nsGlobalWindowOuter.cpp:1924
8 xul.dll nsDocumentViewer::SetDocumentInternal layout/base/nsDocumentViewer.cpp:2042
9 xul.dll nsFrameLoader::CreateStaticClone dom/base/nsFrameLoader.cpp:2811
(...)
33 xul.dll static bool mozilla::dom::WindowBinding::print dom/bindings/WindowBinding.cpp:2653

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

This looks like a new crasher in 60, likely a latent issue triggered by bug 1182329 which landed in 60.

To me it looks like `rv` is NS_OK but `mgr` is nullptr after the code at [1]:
```
  nsCOMPtr<nsIPermissionManager> mgr =
    do_GetService(NS_PERMISSIONMANAGER_CONTRACTID, &rv);
```

This is surprising. There should be an invariant that mgr is not null when rv is NS_OK.

An interesting pattern across all the reports is that `mozilla::dom::WindowBinding::print` is on the stack. Perhaps this can be reproduced by trying to print a page with an autoplaying media element. The child process just needs a MediaManager present first...

Comment 1

[Andreas Pehrson [:pehrsons]]

Chris, can you take a look?

Comment 2

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

(In reply to Andreas Pehrson [:pehrsons] (PTO) from comment #0)
> To me it looks like `rv` is NS_OK but `mgr` is nullptr after the code at [1]:
> ```
>   nsCOMPtr<nsIPermissionManager> mgr =
>     do_GetService(NS_PERMISSIONMANAGER_CONTRACTID, &rv);
> ```


I think what's happening here is we're crashing on the line before. I think when we print we create a clone of the document, and while we're cloning, we end up in here due to document activity change observers being called and the GetExtantDoc() call [1] returns null, and since we're not null checking the GetExtantDoc() call there we deref null and crash on the call to get the principal. We can actually get the principal from the window by calling nsGlobalWindowInner::GetPrincipal() without having to go through the extant document.


[1[ https://hg.mozilla.org/releases/mozilla-release/annotate/785d242a5b01d5f1094882aa2144d8e5e2791e06/dom/media/MediaManager.cpp#l3925

Comment 3

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

Attachment: Bug 1470932 - Get principal from nsGlobalWindowInner directly in MediaManaager::IsActivelyCapturingOrHasAPermission().

nsGlobalWindowInner::GetExtantDoc() can potentially return null, and
we're not null checking its return value in
MediaManaager::IsActivelyCapturingOrHasAPermission() which I think is the
cause of this crash.

So instead of getting the principal from the window's extant doc, we can
get the principal directly from the nsGlobalWindowInner.

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

Comment 4

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

Comment on attachment 8989625 [details]
Bug 1470932 - Get principal from nsGlobalWindowInner directly in MediaManaager::IsActivelyCapturingOrHasAPermission().

https://reviewboard.mozilla.org/r/254652/#review262674

Lgtm, except I don't understand the significance of us using window->GetExtantDoc()->NodePrincipal() in the first place instead of window->GetPrincipal().

I'd feel better if security or dom person looked at it. Maybe baku?

Comment 5

[Andrea Marchesini [:baku]]

Comment on attachment 8989625 [details]
Bug 1470932 - Get principal from nsGlobalWindowInner directly in MediaManaager::IsActivelyCapturingOrHasAPermission().

https://reviewboard.mozilla.org/r/254652/#review262758

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/192fb5d1f926
Get principal from nsGlobalWindowInner directly in MediaManaager::IsActivelyCapturingOrHasAPermission(). r=baku

Comment 7

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/192fb5d1f926

Comment 8

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

Setting ni? to remember to consider uplifting...

Comment 9

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

Comment on attachment 8989625 [details]
Bug 1470932 - Get principal from nsGlobalWindowInner directly in MediaManaager::IsActivelyCapturingOrHasAPermission().

Approval Request Comment
[Feature/Bug causing the regression]: Block autoplay
[User impact if declined]: Users who have enabled block autoplay on Android (where it's supported in release already) and users who have preffed on block autoplay on desktop (where it's not supported in release yet) will continue to get crashes under some conditions, which I think may be related to printing, but I can't be sure.
[Is this code covered by automated tests?]: We have mochitests covering this feature, but I couldn't find STR to repro this specific crash.
[Has the fix been verified in Nightly?]: No, we don't have STR to verify against, and we weren't getting crash reports for this in Nightly either before or after the fix landed.
[Needs manual test from QE? If yes, steps to reproduce]:  No.
[List of other uplifts needed for the feature/fix]: Just this.
[Is the change risky?]: Low risk.
[Why is the change risky/not risky?]: This patch basically just adds a null check.
[String changes made/needed]: None.

Comment 10

[Sylvestre Ledru [:Sylvestre]]

Too small volume for 61.

Comment 11

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8989625 [details]
Bug 1470932 - Get principal from nsGlobalWindowInner directly in MediaManaager::IsActivelyCapturingOrHasAPermission().

No crashes on beta 62, but the fix doesn't look risky and we may as well stop potential crashes once 62 releases. This should be in beta 10.

Comment 12

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

https://hg.mozilla.org/releases/mozilla-beta/rev/6f47058039b8

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Doesn't sound like this is something we need to worry about on ESR60.