1242774 - Seeking in media with video overhang can crash browser

Status: VERIFIED FIXED

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

Description

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

Attachment: VideoOverhang.webm

When seeking in a media with video longer than the audio a null pointer can occur. This can be done manually by rapidly seeking around where the audio finishes in such a file.

To do this with the attached file rapidly seek around the 4 second mark (as the 5 is displayed). This appears to be reproducible if the file is streamed or read from disk.

This appears to affect the nightly (46+), but not Aurora or Beta.

Comment 1

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

Can't repro on Linux nightly. What is your platform?

Comment 2

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

(In reply to JW Wang [:jwwang] from comment #1)
> Can't repro on Linux nightly. What is your platform?

OSX

Comment 3

[Alastor Wu [:alwu]]

Attachment: Crash stack

Comment 4

[Chris Peterson [:cpeterson]]

The crash stack points to a MozPromise crash in a cubeb callback:

Thread 71 Crashed:: com.apple.audio.IOThread.client
0   XUL                           	0x0000000102a95251 void mozilla::MozPromise<bool, nsresult, false>::Private::Resolve<bool&>(bool&&&, char const*) + 17 (Mutex.h:69)
1   XUL                           	0x0000000102a8f667 mozilla::media::DecodedAudioDataSink::Drained() + 103 (RefPtr.h:40)
2   XUL                           	0x00000001029b86e7 mozilla::AudioStream::StateCallback(cubeb_state) + 199 (Mutex.h:75)
3   XUL                           	0x0000000103a2c739 audiounit_output_callback + 201 (cubeb_audiounit.c:133)
...

Comment 5

[Alastor Wu [:alwu]]

Attachment: Log

Comment 6

[Alastor Wu [:alwu]]

Attachment: Part 1 : Don't resolve promise again when the callback calls drain()

[Crash reason]
As the audio backend in OSX calls drain() twice, then we tried to resolve the promise which had already been resolved and released in previous drain().

[Solution]
Replace Resolve() with ResolveIfExists().

Comment 7

[Alastor Wu [:alwu]]

Attachment: Part 2 : Add the crash test

Comment 8

[Alastor Wu [:alwu]]

We have an easier reproducing way, and it's also used in my crash test.

STR.
1. Play video
2. Pause video when it becomes silent (blue screen)
3. Play video again 
4. *Crash*

Comment 9

[Alastor Wu [:alwu]]

Attachment: Part 2 : Add the crash test

Only add crash test to other platforms, 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2898d3b3e797

Comment 10

[Alastor Wu [:alwu]]

Comment on attachment 8715721 [details] [diff] [review]
Part 1 : Don't resolve promise again when the callback calls drain()

Hi, JW,
Could you help me review these patches?
I'll modify the comment in Drained() after getting the try-server result in comment9, then I would file a new bug and mention what platform has this problem. 
Very appreciate!

Comment 11

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

Comment on attachment 8715721 [details] [diff] [review]
Part 1 : Don't resolve promise again when the callback calls drain()

Review of attachment 8715721 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/mediasink/DecodedAudioDataSink.cpp
@@ +278,5 @@
>  void
>  DecodedAudioDataSink::Drained()
>  {
>    SINK_LOG("Drained");
> +  // In osx, the audio backend would triggled Drained() twice, then it would

s/would triggled Drained/could trigger Drained/.

'would' means it will happen for sure.

@@ +280,5 @@
>  {
>    SINK_LOG("Drained");
> +  // In osx, the audio backend would triggled Drained() twice, then it would
> +  // cause the crash because the promise had already been resolve and free.
> +  // We'll need to solve it in bugXXXXXX.

'bugXXXXXX' means nothing. You can use 'FIXME' or 'TODO' to indicate a future work item.

Comment 12

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

Comment on attachment 8716145 [details] [diff] [review]
Part 2 : Add the crash test

Review of attachment 8716145 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/test/crashtests/audio-drained.html
@@ +1,1 @@
> +<html class="reftest-wait">

You need to call document.documentElement.removeAttribute("class") to finish the test. See oscillator-ended-1.html for an example.

@@ +1,3 @@
> +<html class="reftest-wait">
> +<head>
> +  <title>Bug 1242774 : video crashed if replay after audio drained</title>

Avoid using a term like 'drained' which is a term used by underlying implementation. You can say something like "video crashed if pause and play again after audio track ends".

@@ +9,5 @@
> +    dump("### Error : " + msg + "\n");
> +  }
> +}
> +
> +var SILENT_PART_START_TIME = 4.5;

Let's call this "AUDIO_END_TIME".

@@ +11,5 @@
> +}
> +
> +var SILENT_PART_START_TIME = 4.5;
> +var video = document.createElement('video');
> +video.src = "videoCrash.webm";

You need to "hg add" this file.

@@ +16,5 @@
> +video.play();
> +
> +video.ontimeupdate = function () {
> +  assert(SILENT_PART_START_TIME < video.duration,
> +         "Silent start time shouldn't beyond the duration!");

should be smaller than the duration.

Comment 13

[Alastor Wu [:alwu]]

(In reply to JW Wang [:jwwang] from comment #12)
> @@ +11,5 @@
> > +}
> > +
> > +var SILENT_PART_START_TIME = 4.5;
> > +var video = document.createElement('video');
> > +video.src = "videoCrash.webm";
> 
> You need to "hg add" this file.
> 
It's already added, but it doesn't be showed in "review" mode.
Choose "overview" then you would see that file.

Comment 14

[Alastor Wu [:alwu]]

(In reply to JW Wang [:jwwang] from comment #12)
>
> You need to call document.documentElement.removeAttribute("class") to finish
> the test. See oscillator-ended-1.html for an example.
> 

I think that is only needed when you add some css attribute in the element. [1]

[1] removeAttribute : http://www.w3schools.com/jsref/met_element_removeattribute.asp

Comment 15

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

Do you mean the test can finish correctly without calling document.documentElement.removeAttribute("class") like oscillator-ended-1.html?

Comment 16

[Alastor Wu [:alwu]]

Now I am waiting the result on window, now only OSX crashed.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e58d3d1d2543&group_state=expanded

Comment 17

[Alastor Wu [:alwu]]

Attachment: Part 2 : Add the crash test

Comment 18

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

Comment on attachment 8716194 [details] [diff] [review]
Part 2 : Add the crash test

Review of attachment 8716194 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/test/crashtests/video-replay-after-audio-end.html
@@ +11,5 @@
> +}
> +
> +var AUDIO_END_TIME = 4.5;
> +var video = document.createElement('video');
> +video.src = "videoCrash.webm";

The convention is lowercase like "video-crash.webm".

Comment 19

[Alastor Wu [:alwu]]

The test also crashed on the Window, but the crash point isn't the same [1].

In the crash stack, we can find the following stack
> 2  xul.dll!mozilla::AudioStream::Resume() [AudioStream.cpp:e58d3d1d2543 : 471 + 0x8]
> 3  xul.dll!mozilla::media::DecodedAudioDataSink::SetPlaying(bool)

Is it correct to resume the AudioStream after drained?

[1] https://treeherder.mozilla.org/logviewer.html#?job_id=16373175&repo=try

Comment 20

[Alastor Wu [:alwu]]

Try-server result for all patches, we can see whether the crash happens on Window after applying the patch.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8f91bbf7a22f

Comment 21

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

It looks like to me wasapi should handle the case where wasapi_stream_start() is called after drained correctly.

Comment 22

[Alastor Wu [:alwu]]

Attachment: Part 1 : Don't resolve promise again when the callback calls drain(). r=jwwang.

Comment 23

[Alastor Wu [:alwu]]

Attachment: Part 2 : Add the crash test. r=jwwang.

Comment 24

[Alastor Wu [:alwu]]

Wait for new try-server result.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2cd5c8d563ff

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b1c27c182412
https://hg.mozilla.org/integration/mozilla-inbound/rev/561d4d620aa3

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b1c27c182412
https://hg.mozilla.org/mozilla-central/rev/561d4d620aa3

Comment 27

[Phil Ringnalda (:philor)]

Backed out the crashtest in https://hg.mozilla.org/integration/mozilla-inbound/rev/2914d1a8153c - apparently Linux32 thinks it's a little aggressive with memory, or leaves things around, or something: caused at least bug 1247122 and bug 1247506 and I think there are some other flavors of the same OOMish thing that we didn't yet get around to filing.

Comment 28

[Alastor Wu [:alwu]]

Attachment: Part 2 : Add the crash test. r=jwwang.

per comment27, skip this test on Linux.

Comment 29

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

(In reply to Alastor Wu [:alwu] from comment #28)
> Created attachment 8719359 [details] [diff] [review]
> Part 2 : Add the crash test. r=jwwang.
> 
> per comment27, skip this test on Linux.

Please file follow-up bugs to ensure they will be fixed on Linux and Windows.

Comment 30

[Alastor Wu [:alwu]]

Open the bug1248313 to track it.

Comment 31

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/decfe1d8c927

Comment 32

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/53029769feef

Comment 33

[Carsten Book [:Tomcat]]

backed out for causing https://treeherder.mozilla.org/logviewer.html#?job_id=21739600&repo=mozilla-inbound and others. please conside a try run before  next landing try

Comment 34

[Alastor Wu [:alwu]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=56b5621080df

Comment 35

[Alastor Wu [:alwu]]

All the tests are green light now.

Comment 36

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8ac1b6ca916a

Comment 37

[Carsten Book [:Tomcat]]

backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=21839282&repo=mozilla-inbound

Comment 38

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/56bed1e2398d

Comment 39

[Alastor Wu [:alwu]]

Attachment: Part 2 : Add the crash test. r=jwwang.

MozReview-Commit-ID: EBRigaPn5Du

Comment 40

[Alastor Wu [:alwu]]

Comment on attachment 8720324 [details] [diff] [review]
Part 2 : Add the crash test. r=jwwang.

Sorry, I forgot to update the new patch....

Comment 41

[Alastor Wu [:alwu]]

The try result is in the comment34, the reason of the back-out is I forgot to update the new patch.
Sorry!

Comment 42

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7da7f2d03209

Comment 43

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/7da7f2d03209

Comment 44

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

Comment on attachment 8716232 [details] [diff] [review]
Part 1 : Don't resolve promise again when the callback calls drain(). r=jwwang.

Approval Request Comment
[Feature/regressing bug #]:948267
[User impact if declined]:crash might happen during playback when video duration is longer than audio duration.
[Describe test coverage new/current, TreeHerder]:TreeHerder
[Risks and why]: low, the change is simple and has been Aurora and Central for quite a while.
[String/UUID change made/needed]:none

Comment 45

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

Regression from 46, tracking.

Comment 46

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

Comment on attachment 8716232 [details] [diff] [review]
Part 1 : Don't resolve promise again when the callback calls drain(). r=jwwang.

Crash fix, recent regression (46), includes a test. 
Yay, put it on beta too please so we don't ship this.

Comment 47

[Carsten Book [:Tomcat]]

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

Comment 48

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

This crash accounted for 1539 of 4941 of crashes recorded under signature mozilla::MozPromise<T>::Private::Resolve<T> in the period 2016-03-01 -> 2016-03-24 in 46 beta.

Comment 49

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

I have 3037 crashes in beta 2 with this signature. This fix landed the day we shipped beta 2; there wasn't a beta 3, so it should be in beta 4. We don't have a ton of data yet for beta 4 but I'd love for us to verify this fix in a couple of days.

Comment 50

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

This signature has disappeared past beta 2. Marking verified.

Comment 51

[BugBot [:suhaib / :marco/ :calixte]]

Crash volume for signature 'mozilla::MozPromise<T>::Private::Resolve<T>':
 - nightly (version 50): 1 crash from 2016-06-06.
 - aurora  (version 49): 0 crashes from 2016-06-07.
 - beta    (version 48): 4 crashes from 2016-06-06.
 - release (version 47): 35 crashes from 2016-05-31.
 - esr     (version 45): 1 crash from 2016-04-07.

Crash volume on the last weeks:
            W. N-1  W. N-2  W. N-3  W. N-4  W. N-5  W. N-6  W. N-7
 - nightly       0       0       0       0       1       0       0
 - aurora        0       0       0       0       0       0       0
 - beta          0       0       0       1       1       1       1
 - release       5       4       6       7       5       5       0
 - esr           1       0       0       0       0       0       0

Affected platform: Windows

Comment 52

[Andrew Overholt [:overholt]]

So this is fixed in 47 but we're still seeing it in 48 and 50?

Comment 53

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

Yes, I think that here, the bot should not be reporting on this bug.... The threshhold of having a few crashes like this doesn't seem worth reporting.  Calixte what do you think? I'm seeing a lot of this kind of thing (Months untouched, fixed bugs with a few crashes on release channel, getting a bot update)

Comment 54

[Sylvestre Ledru [:Sylvestre]]

I think it is noise (and would like to be able to explain why but this is a different story...)