1321410 - Crash in mozilla::dom::HTMLMediaElement::AudioChannelAgentCallback::Resume

Status: RESOLVED FIXED

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

Description

[Marcia Knous [:marcia]]

This bug was filed from the Socorro interface and is 
report bp-c89f65b3-cf4a-4b31-92f0-b72f42161130.
=============================================================

Seen while looking at crash stats: http://bit.ly/2fSGN2r. New crash which started with the 20161129171714 build. 

Possible regression range: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=adcc39e3cad0f32aba0efb478cc4a023a5dfc43f&tochange=13736e2db6eb94b02dd28cc88f2943b8109aa374

Possibly regression from Bug 1309162? ni on :alwu.

Comment 1

[Alastor Wu [:alwu]]

Attachment: Bug 1321410 - stop agent after cycle collection.

The crash reason seems the mOwner has been released, so we shouldn't call any method which would call mOwner.
The AudioChannelAgentCallback could only be called from two parts, one is from mOwner, another is from AudioChannelService.
We don't want this class be called after mOwner was released, we should disconnect it from AudioChannelService.
Calling NotifyStoppedPlaying() can unregister agent from service, so service won't call agent anymore.
Therefore, no one would call AudioChannelAgentCallback after CC happened.

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

Comment 2

[Alastor Wu [:alwu]]

Comment on attachment 8815987 [details]
Bug 1321410 - stop agent after cycle collection.

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

Comment 3

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8815987 [details]
Bug 1321410 - stop agent after cycle collection.

https://reviewboard.mozilla.org/r/96746/#review97020

::: dom/html/HTMLMediaElement.cpp:713
(Diff revision 2)
>    }
>  
>  private:
> -  ~AudioChannelAgentCallback() {};
> +  ~AudioChannelAgentCallback()
> +  {
> +    Shutdown();

I prefer to require the client to call Shutdown() explicitly and assert that in the destructor. This is also the convention of our media code.

Comment 4

[Alastor Wu [:alwu]]

Comment on attachment 8815987 [details]
Bug 1321410 - stop agent after cycle collection.

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

Comment 5

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8815987 [details]
Bug 1321410 - stop agent after cycle collection.

https://reviewboard.mozilla.org/r/96746/#review97350

::: dom/html/HTMLMediaElement.cpp:711
(Diff revision 3)
> +  {
> +    if (mAudioChannelAgent) {
> +      mAudioChannelAgent->NotifyStoppedPlaying();
> +      mAudioChannelAgent = nullptr;
> +    }
> +    mIsShutDown = true;

Assert Shutdown() is only called once.

Comment 6

[Alastor Wu [:alwu]]

Comment on attachment 8815987 [details]
Bug 1321410 - stop agent after cycle collection.

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

Comment 7

[Alastor Wu [:alwu]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=51f9e68804097abc1d31d027e70dc05b74e652da&selectedJob=32207456

Comment 8

[Alastor Wu [:alwu]]

Comment on attachment 8815987 [details]
Bug 1321410 - stop agent after cycle collection.

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

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4fe28bb060cf
stop agent after cycle collection. r=jwwang

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/4fe28bb060cf