1472580 - Clicking the audio tab indicator should unnblock autoplay

Chris Pearce [:cpearce (Not reading bugmail)]

Assignee

Description

•

6 years ago

If the user opens a tab in the background, and that tab tries to play audible media, we'll show the audio tab indicator in the tab bar. If the user clicks on it to start playback, we'll then hit the block autoplay logic and maybe show a doorhanger in that tab asking for permission. The doorhanger won't show until that tab is brought to the foreground, so in other words, when the user clicks the "play" button, we'll actually not play. So we need the tab audio indicator to override the autoplay blocking logic.

This is spun off from bug 1463919.

Comment hidden (mozreview-request)

(This patch was first presented for review in bug 1463919, I've split it off
into its own bug here).

If the user opens a tab in the background, and that tab tries to play media,
we'll delay playing that media until the tab is brought to the foreground.
But the user can explicitly start playback of such delayed media by clicking
the "play" icon we show in the tab indicator. Then if autoplay is disabled,
we'll block the play (unless the origin is whitelisted). This is bad, as the
user has clearly indicated intent to play media in this tab.

So this patch "gesture activates" the root content document when the tab audio
indicator play button is pressed. This means the block autoplay logic will
behave as if there's been a user gesture in the tab (mouse click or keypress),
and not block the play. Gesture activation state is per document, so it does
not persist across document loads.

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

Comment hidden (mozreview-request)

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

Mike Conley (:mconley) (:⚙️)

Comment 3

•

6 years ago

mozreview-review

Comment on attachment 8989096 [details]
Bug 1472580 - Gesture activate documents which are played via the tab audio indicator.

https://reviewboard.mozilla.org/r/254160/#review261364

::: toolkit/content/browser-content.js:533
(Diff revision 1)
>        case "resumeMedia":
> +        // User has clicked the tab audio indicator to play a delayed
> +        // media. That's clear user intent to play, so gesture activate
> +        // the content document tree so that the block-autoplay logic
> +        // allows the media to autoplay.
> +        content.window.document.notifyUserGestureActivation();

I _think_ `content.window` is redundant, and you can just do `content.document` here.

Attachment #8989096 - Flags: review?(mconley) → review+

Mike Conley (:mconley) (:⚙️)

Comment 4

•

6 years ago

mozreview-review

Comment on attachment 8989097 [details]
Bug 1472580 - Test that starting play from tab audio indicator overrides block autoplay.

https://reviewboard.mozilla.org/r/254162/#review261366

Thanks for the test!

::: toolkit/content/tests/browser/browser_block_autoplay_media.js:11
(Diff revision 1)
>    SUSPENDED_BLOCK: 2,
>    SUSPENDED_PAUSE_DISPOSABLE: 3
>  };
>  
> -function check_audio_suspended(suspendedType) {
> +function check_audio_suspended(browser, suspendedType) {
> +  return ContentTask.spawn(browser, suspendedType, (suspendedType) => {

For single arguments in arrow-functions, you can forgo the parantheses. Same below.

Attachment #8989097 - Flags: review?(mconley) → review+

Comment hidden (mozreview-request)

Comment on attachment 8989096 [details]
Bug 1472580 - Gesture activate documents which are played via the tab audio indicator.

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

Comment hidden (mozreview-request)

Comment on attachment 8989097 [details]
Bug 1472580 - Test that starting play from tab audio indicator overrides block autoplay.

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

Pulsebot

Comment 7

•

6 years ago

Pushed by cpearce@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4913ce5ab6cb
Gesture activate documents which are played via the tab audio indicator. r=mconley
https://hg.mozilla.org/integration/autoland/rev/d3d4619e8133
Test that starting play from tab audio indicator overrides block autoplay. r=mconley

Cristian Brindusan [:cbrindusan]

Comment 8

•

6 years ago

Backed out 2 changesets (bug 1472580) for browser chrome LeakSanitizer failures

Backout: https://hg.mozilla.org/integration/autoland/rev/cdfb7cd94980a0e50b275d076b1ed8f5edc1c797

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=d3d4619e8133d286727a1062e27ebeb962450647&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=success&selectedJob=186295717 

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=186295717&repo=autoland&lineNumber=80894

[task 2018-07-03T22:20:19.050Z]     INFO - GECKO(4837) | Suppressions used:
[task 2018-07-03T22:20:19.051Z]     INFO - GECKO(4837) |   count      bytes template
[task 2018-07-03T22:20:19.052Z]     INFO - GECKO(4837) |    2028     182543 libc.so
[task 2018-07-03T22:20:19.053Z]     INFO - GECKO(4837) |    2066     120234 nsComponentManagerImpl
[task 2018-07-03T22:20:19.055Z]     INFO - GECKO(4837) |     201      15675 mozJSComponentLoader::LoadModule
[task 2018-07-03T22:20:19.056Z]     INFO - GECKO(4837) |     611      17713 libfontconfig.so
[task 2018-07-03T22:20:19.057Z]     INFO - GECKO(4837) |       1         29 libglib-2.0.so
[task 2018-07-03T22:20:19.058Z]     INFO - GECKO(4837) | -----------------------------------------------------
[task 2018-07-03T22:20:19.059Z]     INFO - GECKO(4837) | SUMMARY: AddressSanitizer: 277997 byte(s) leaked in 4134 allocation(s).
[task 2018-07-03T22:20:19.061Z]     INFO - GECKO(4837) | -----------------------------------------------------
[task 2018-07-03T22:20:19.062Z]     INFO - GECKO(4837) | Suppressions used:
[task 2018-07-03T22:20:19.063Z]     INFO - GECKO(4837) |   count      bytes template
[task 2018-07-03T22:20:19.064Z]     INFO - GECKO(4837) |     670      21304 nsComponentManagerImpl
[task 2018-07-03T22:20:19.066Z]     INFO - GECKO(4837) |      43       8944 mozJSComponentLoader::LoadModule
[task 2018-07-03T22:20:19.067Z]     INFO - GECKO(4837) |     611      17509 libfontconfig.so
[task 2018-07-03T22:20:19.069Z]     INFO - GECKO(4837) |      12        528 _PR_Getfd
[task 2018-07-03T22:20:19.070Z]     INFO - GECKO(4837) |       1         29 libglib-2.0.so
[task 2018-07-03T22:20:19.071Z]     INFO - GECKO(4837) | -----------------------------------------------------
[task 2018-07-03T22:20:19.073Z]     INFO - TEST-INFO | Main app process: exit 0
[task 2018-07-03T22:20:19.074Z]     INFO - TEST-INFO | LeakSanitizer | To show the addresses of leaked objects add report_objects=1 to LSAN_OPTIONS
[task 2018-07-03T22:20:19.075Z]     INFO - TEST-INFO | LeakSanitizer | This can be done in testing/mozbase/mozrunner/mozrunner/utils.py
[task 2018-07-03T22:20:19.076Z]    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at js_pod_malloc, maybe_pod_malloc, js::Shape::hashify, maybeCreateTableForLookup
[task 2018-07-03T22:20:19.078Z]    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at js_pod_calloc, maybe_pod_calloc, AllocScriptData, js::detail::CopyScript
[task 2018-07-03T22:20:19.079Z]    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at EntrySlotOrCreate, EntrySlotOrCreate, mozilla::dom::Exception_Binding::CreateInterfaceObjects, mozilla::dom::GetPerInterfaceObjectHandle
[task 2018-07-03T22:20:19.080Z]    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at js_pod_malloc, maybe_pod_malloc, NewEmptyScopeData, js::GlobalScope::create
[task 2018-07-03T22:20:19.081Z]    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at EntrySlotOrCreate, EntrySlotOrCreate, mozilla::dom::Document_Binding::CreateInterfaceObjects, mozilla::dom::GetPerInterfaceObjectHandle
[task 2018-07-03T22:20:19.082Z]    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at js_pod_malloc, maybe_pod_malloc, pod_malloc, js::RegExpStatics::create
[task 2018-07-03T22:20:19.083Z]    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at EntrySlotOrCreate, EntrySlotOrCreate, mozilla::dom::ChromeUtils_Binding::CreateInterfaceObjects, mozilla::dom::GetPerInterfaceObjectHandle
[task 2018-07-03T22:20:19.084Z]    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at EntrySlotOrCreate, EntrySlotOrCreate, mozilla::dom::DOMParser_Binding::CreateInterfaceObjects, mozilla::dom::GetPerInterfaceObjectHandle
[task 2018-07-03T22:20:19.085Z]    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at js_pod_malloc, maybe_pod_malloc, CopyScopeData, js::LexicalScope::create
[task 2018-07-03T22:20:19.086Z]    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at js_pod_malloc, maybe_pod_malloc, createTable, js::detail::HashTable
[task 2018-07-03T22:20:19.087Z]    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at js_pod_malloc, maybe_pod_malloc, pod_malloc, js::ForOfPIC::createForOfPICObject
[task 2018-07-03T22:20:19.088Z]    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at js_pod_malloc, maybe_pod_malloc, mozilla::UniquePtr, js::FunctionScope::create
[task 2018-07-03T22:20:19.089Z]    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at js_pod_realloc, maybe_pod_realloc, js::Nursery::reallocateBuffer, ReallocateObjectBuffer
[task 2018-07-03T22:20:19.090Z]    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at js_pod_malloc, maybe_pod_malloc, pod_malloc, js::LazyScript::CreateRaw
[task 2018-07-03T22:20:19.091Z]    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at js_pod_malloc, maybe_pod_malloc, NewEmptyScopeData, js::FunctionScope::create
[task 2018-07-03T22:20:19.092Z]    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at js_pod_malloc, maybe_pod_malloc, pod_malloc, make_unique
[task 2018-07-03T22:20:19.093Z]    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at mozilla::dom::Performance::CreateForMainThread, nsPIDOMWindowInner::CreatePerformanceObjectIfNeeded, nsPIDOMWindowInner::GetPerformance, mozilla::dom::Window_Binding::get_performance
[task 2018-07-03T22:20:19.094Z]    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at EntrySlotOrCreate, EntrySlotOrCreate, mozilla::dom::MessageListenerManager_Binding::CreateInterfaceObjects, mozilla::dom::GetPerInterfaceObjectHandle
[task 2018-07-03T22:20:19.095Z]    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at js_pod_malloc, maybe_pod_malloc, createTable, init
[task 2018-07-03T22:20:19.096Z]    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at AllocateProtoAndIfaceCache, mozilla::dom::CreateGlobal, mozilla::dom::Window_Binding::Wrap, nsGlobalWindowOuter::SetNewDocument
[task 2018-07-03T22:20:19.097Z]    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at Alloc, nsTSubstring, nsTSubstring, nsTSubstring
[task 2018-07-03T22:20:19.103Z]    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at js_new, MakeUnique, HashChildren, js::PropertyTree::insertChild
[task 2018-07-03T22:20:19.104Z]    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at js_pod_malloc, maybe_pod_malloc, CopyScopeData, js::GlobalScope::create
[task 2018-07-03T22:20:19.105Z]    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at js_pod_malloc, maybe_pod_malloc, tryNewTenuredObject, js::Allocate
[task 2018-07-03T22:20:19.106Z]    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at js_pod_malloc, maybe_pod_malloc, pod_malloc, AllocChars
[task 2018-07-03T22:20:19.107Z]    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at EntrySlotOrCreate, EntrySlotOrCreate, mozilla::dom::InspectorUtils_Binding::CreateInterfaceObjects, mozilla::dom::GetPerInterfaceObjectHandle
[task 2018-07-03T22:20:19.108Z]    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at EntrySlotOrCreate, EntrySlotOrCreate, mozilla::dom::URLSearchParams_Binding::CreateInterfaceObjects, mozilla::dom::GetPerInterfaceObjectHandle
[task 2018-07-03T22:20:19.109Z]    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at js_pod_malloc, maybe_pod_malloc, CopyScopeData, js::Scope::clone
[task 2018-07-03T22:20:19.110Z]    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at js_pod_malloc, maybe_pod_malloc, mozilla::UniquePtr, js::FunctionScope::clone
[task 2018-07-03T22:20:19.111Z]    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at EntrySlotOrCreate, EntrySlotOrCreate, mozilla::dom::MatchPatternSet_Binding::CreateInterfaceObjects, mozilla::dom::GetPerInterfaceObjectHandle
[task 2018-07-03T22:20:19.112Z]    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at js_pod_calloc, maybe_pod_calloc, js::ShapeTable::init, js::Shape::hashify
[task 2018-07-03T22:20:19.114Z]    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at js_pod_malloc, maybe_pod_malloc, pod_malloc, js::ObjectGroup::newPlainObject
[task 2018-07-03T22:20:19.115Z]    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at EntrySlotOrCreate, EntrySlotOrCreate, mozilla::dom::Blob_Binding::CreateInterfaceObjects, mozilla::dom::GetPerInterfaceObjectHandle
[task 2018-07-03T22:20:19.116Z]    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at js_pod_malloc, maybe_pod_malloc, pod_malloc, pod_malloc
[task 2018-07-03T22:20:19.117Z]    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at EntrySlotOrCreate, EntrySlotOrCreate, mozilla::dom::XMLDocument_Binding::CreateInterfaceObjects, mozilla::dom::GetPerInterfaceObjectHandle
[task 2018-07-03T22:20:19.118Z]    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at AllocateProtoAndIfaceCache, xpc::CreateGlobalObject, XPCWrappedNative::WrapNewGlobal, xpc::InitClassesWithNewWrappedGlobal
[task 2018-07-03T22:20:19.119Z]    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at js_pod_malloc, maybe_pod_malloc, pod_malloc, make_pod_array
[task 2018-07-03T22:20:19.120Z]    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at EntrySlotOrCreate, EntrySlotOrCreate, mozilla::dom::MozQueryInterface_Binding::CreateInterfaceObjects, mozilla::dom::GetPerInterfaceObjectHandle
[task 2018-07-03T22:20:19.121Z]    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at js_pod_calloc, maybe_pod_calloc, AllocScriptData, JSScript::partiallyInit
[task 2018-07-03T22:20:19.122Z]    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at js_pod_malloc, maybe_pod_malloc, pod_malloc, js::Nursery::allocateBuffer
[task 2018-07-03T22:20:19.123Z]    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at js_pod_malloc, maybe_pod_malloc, pod_malloc, js::TypeNewScript::make
[task 2018-07-03T22:20:19.125Z]    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at js_pod_calloc, maybe_pod_calloc, js::TypeNewScript::maybeAnalyze, js::CreateThisForFunctionWithProto
[task 2018-07-03T22:20:19.125Z]    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at EntrySlotOrCreate, EntrySlotOrCreate, mozilla::dom::NodeFilter_Binding::CreateInterfaceObjects, mozilla::dom::GetPerInterfaceObjectHandle
[task 2018-07-03T22:20:19.126Z]    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at ProtoAndIfaceCache, AllocateProtoAndIfaceCache, mozilla::dom::CreateGlobal, mozilla::dom::Window_Binding::Wrap
[task 2018-07-03T22:20:19.127Z]    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at Realloc, nsTArray_base, AppendElement, mozilla::dom::ChromeUtils::GenerateQI
[task 2018-07-03T22:20:19.128Z]    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at ProtoAndIfaceCache, AllocateProtoAndIfaceCache, xpc::CreateGlobalObject, XPCWrappedNative::WrapNewGlobal
[task 2018-07-03T22:20:19.129Z]    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at EntrySlotOrCreate, EntrySlotOrCreate, mozilla::dom::ChildSHistory_Binding::CreateInterfaceObjects, mozilla::dom::GetPerInterfaceObjectHandle
[task 2018-07-03T22:20:19.129Z]    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at js_pod_calloc, maybe_pod_calloc, partiallyInit, JSScript::initFunctionPrototype
[task 2018-07-03T22:20:19.130Z]    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at EntrySlotOrCreate, EntrySlotOrCreate, mozilla::dom::File_Binding::CreateInterfaceObjects, mozilla::dom::GetPerInterfaceObjectHandle
[task 2018-07-03T22:20:19.131Z]    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at js_pod_malloc, maybe_pod_malloc, CreateTraceList, DefineSimpleTypeDescr
[task 2018-07-03T22:20:19.131Z]    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at js_pod_malloc, maybe_pod_malloc, CreateFunctionPrototype, js::GlobalObject::resolveConstructor
[task 2018-07-03T22:20:19.132Z]    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at js_pod_malloc, maybe_pod_malloc, pod_malloc, js::frontend::CreateScriptSourceObject
[task 2018-07-03T22:20:19.132Z]    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at js_pod_malloc, maybe_pod_malloc, CopyScopeData, js::VarScope::create
[task 2018-07-03T22:20:19.133Z]    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at mozilla::dom::ChromeUtils::GenerateQI, mozilla::dom::ChromeUtils_Binding::generateQI, CallJSNative, js::InternalCallOrConstruct
[task 2018-07-03T22:20:19.134Z]    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at js_pod_malloc, maybe_pod_malloc, pod_malloc, js::ObjectGroup::allocationSiteGroup
[task 2018-07-03T22:20:19.134Z]    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at js_pod_malloc, maybe_pod_malloc, make_pod_array, JSExternalString::ensureFlat
[task 2018-07-03T22:20:19.135Z]     INFO - runtests.py | Application ran for: 0:05:11.974015

Flags: needinfo?(cpearce)

Comment hidden (mozreview-request)

The front end code can't always guarantee to give us an allow/cancel response
to a permission request. In particular in these cases:
* if we close a tab while showing a doorhanger, or
* if we navigate a tab while showing a doorhanger, or
* if the permission prompt requested in a background tab and never shown.

Handling all of these cases is problematic; we don't get events for all of
these where it's easy and cheap to determine that we should cancel the
permission request.

Canceling the permission request is important in the autoplay-media permission
request case as there's objects waiting on the resolution of the permission
request, and they leak in ASan builds while running chrome tests if the Gecko
size of the permission request doesn't get a notification telling it to stop
waiting.

But we can however rely on the doorhanger code to drop its reference to the
nsIContentPermissionRequest object that we pass to it when the doorhanger goes
away. So we can cancel the permission request in our
nsIContentPermissionRequest's implementation's destructor in order to easily
catch all the above cases.

In order to do that, we need to split AutoplayRequest into two; one part being
the implementation of nsIContentPermissionRequest (AutoplayPermissionRequest),
and the other part being the code to own the PromiseHolder and manage the
permission request (AutoplayPermissionManager).

AutoplayPermissionRequest keeps a weak reference to AutoplayPermissionManager,
so that it can tell the AutoplayPermissionManager to reject the request promise
when it's destroyed.

This fixes the ASan leak for which I got backed out from earlier in this bug,
and also fixes the cases above.

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

Comment hidden (mozreview-request)

Comment on attachment 8989096 [details]
Bug 1472580 - Gesture activate documents which are played via the tab audio indicator.

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

Comment hidden (mozreview-request)

Comment on attachment 8989097 [details]
Bug 1472580 - Test that starting play from tab audio indicator overrides block autoplay.

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

Olli Pettay [:smaug][bugs@pettay.fi](vacation until July 14)

Comment 12

•

6 years ago

mozreview-review

Comment on attachment 8991572 [details]
Bug 1472580 - Ensure we always get a allow/cancel response to an autoplay media permission request.

https://reviewboard.mozilla.org/r/256500/#review263380

I think this needs some minor tweaks to be easier to follow.

::: dom/base/nsGlobalWindowInner.cpp:8169
(Diff revision 1)
>    }
>    return rootTreeItem->GetDocument()->GetInnerWindow();
>  }
>  
> -already_AddRefed<mozilla::AutoplayRequest>
> +already_AddRefed<mozilla::AutoplayPermissionManager>
>  nsPIDOMWindowInner::GetAutoplayRequest()

This is weird. Method named GetAutoplayRequest returns something totally different.

::: dom/base/nsGlobalWindowInner.cpp:8177
(Diff revision 1)
>    nsPIDOMWindowInner* window = GetTopLevelInnerWindow(this);
>    if (!window) {
>      return nullptr;
>    }
>    if (!window->mAutoplayRequest) {
> -    window->mAutoplayRequest = AutoplayRequest::Create(nsGlobalWindowInner::Cast(window));
> +    window->mAutoplayRequest =

similar here. mAutoplayRequest stores some different kind of object in it.

::: dom/html/AutoplayPermissionManager.h:51
(Diff revision 1)
> +  // rejects in the content process after an IPC round trip).
>    RefPtr<GenericPromise> RequestWithPrompt();
>  
> +  // Called by AutoplayPermissionRequest to approve the play request,
> +  // and resolve the promise.
> +  void Resolve();

Since this code doesn't use Promises but MozPromises which are totally different things, I'd prefer if at least the comment would describe precisely what is being resolved, so MozPromise, not promise.
But I think also the names of the methods, Resolve() and Reject() should be something else, describing what they really do.

Attachment #8991572 - Flags: review?(bugs) → review-

Comment hidden (mozreview-request)

Comment on attachment 8991572 [details]
Bug 1472580 - Ensure we always get a allow/cancel response to an autoplay media permission request.

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

Olli Pettay [:smaug][bugs@pettay.fi](vacation until July 14)

Comment 14

•

6 years ago

(interesting how totally unreadable MozReview manages to make that patch. Downloading the patch works)

Olli Pettay [:smaug][bugs@pettay.fi](vacation until July 14)

Comment 15

•

6 years ago

mozreview-review

Comment on attachment 8991572 [details]
Bug 1472580 - Ensure we always get a allow/cancel response to an autoplay media permission request.

https://reviewboard.mozilla.org/r/256500/#review263850

::: dom/html/AutoplayPermissionRequest.h:21
(Diff revision 2)
> +class AutoplayPermissionManager;
> +
> +// The AutoplayPermissionRequest is the object we pass off to the chrome JS
> +// code to represent a request for permission to autoplay. Unfortunately the
> +// front end code doesn't guarantee to give us an approve/cancel callback in
> +// all cases. So the AutoplayPermissionManager keeps a weak reference to the

AutoplayPermissionManager does not seem to keep any reference to the request, but request keeps a weak reference to the manager.

::: dom/html/AutoplayPermissionRequest.h:27
(Diff revision 2)
> +// AutoplayPermissionRequest. If chrome JS dismisses the permission request for
> +// whatever reason (tab closed, user presses ESC, navigation, etc), the
> +// permission UI code will drop its reference to the AutoplayPermissionRequest
> +// and it will be destroyed. AutoplayPermissionRequest's destructor calls back
> +// the AutoplayPermissionManager with a cancel operation, so we can thus
> +// guarantee to always get an approve or cancel callback.

Doesn't seem to be quite right. mManager is WeakPtr, so the manager may actually go away before approving or canceling.

Attachment #8991572 - Flags: review?(bugs) → review+

Comment hidden (mozreview-request)

Comment on attachment 8989096 [details]
Bug 1472580 - Gesture activate documents which are played via the tab audio indicator.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254160/diff/3-4/

Comment hidden (mozreview-request)

Comment on attachment 8989097 [details]
Bug 1472580 - Test that starting play from tab audio indicator overrides block autoplay.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254162/diff/3-4/

Comment hidden (mozreview-request)

Comment on attachment 8991572 [details]
Bug 1472580 - Ensure we always get a allow/cancel response to an autoplay media permission request.

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

Chris Pearce [:cpearce (Not reading bugmail)]

Assignee

Comment 19

•

6 years ago

mozreview-review-reply

Comment on attachment 8991572 [details]
Bug 1472580 - Ensure we always get a allow/cancel response to an autoplay media permission request.

https://reviewboard.mozilla.org/r/256500/#review263850

> AutoplayPermissionManager does not seem to keep any reference to the request, but request keeps a weak reference to the manager.

I've adjusted the comments here to match reality. Thanks for the review!

Pulsebot

Comment 20

•

6 years ago

Pushed by cpearce@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a2c0973c0454
Gesture activate documents which are played via the tab audio indicator. r=mconley
https://hg.mozilla.org/integration/autoland/rev/a1a2ee0ee145
Test that starting play from tab audio indicator overrides block autoplay. r=mconley
https://hg.mozilla.org/integration/autoland/rev/9b4b53a14538
Ensure we always get a allow/cancel response to an autoplay media permission request. r=smaug

Andrei Ciure[:aciure]

Comment 21

•

6 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/a2c0973c0454
https://hg.mozilla.org/mozilla-central/rev/a1a2ee0ee145
https://hg.mozilla.org/mozilla-central/rev/9b4b53a14538

Status: NEW → RESOLVED

Closed: 6 years ago

status-firefox63: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla63

Chris Pearce [:cpearce (Not reading bugmail)]

Assignee

Updated

•

6 years ago

Flags: needinfo?(cpearce)

Gyula Palko[:gpalko]

Comment 22

•

6 years ago

Verified on Nightly 63.0a1(20180823100106), media with sound is played if the 'play tab' button is clicked.

Status: RESOLVED → VERIFIED

status-firefox63: fixed → verified

Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)

Updated

•

5 years ago

Depends on: 1521760

Bug 1472580 - Gesture activate documents which are played via the tab audio indicator. 6 years ago Chris Pearce [:cpearce (Not reading bugmail)] 59 bytes, text/x-review-board-request	mconley : review+	Details
Bug 1472580 - Test that starting play from tab audio indicator overrides block autoplay. 6 years ago Chris Pearce [:cpearce (Not reading bugmail)] 59 bytes, text/x-review-board-request	mconley : review+	Details
Bug 1472580 - Ensure we always get a allow/cancel response to an autoplay media permission request. 6 years ago Chris Pearce [:cpearce (Not reading bugmail)] 59 bytes, text/x-review-board-request	smaug : review+	Details