1152439 - [Crash] [@ mozilla::StreamBuffer::TrackIter::FindMatch | mozilla::MediaStreamGraphImpl::UpdateStreamOrder ]

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Stability, defect)

Description

[Nicholas Troast [:ntroast]]

We observed the following crash signature during testing.

[@ mozilla::StreamBuffer::TrackIter::FindMatch | mozilla::MediaStreamGraphImpl::UpdateStreamOrder | mozilla::MediaStreamGraphImpl::UpdateGraph | mozilla::MediaStreamGraphImpl::OneIteration ]

Cafbot will upload the decoded minidump and extra file.

this crash was produced during stability tests which involves monkey testing for several hours and there is no clear STR for this. If we are not able to identify the issue using provided logs then please feel free to provide us a debug patch with additional logging to identify the issue.

Comment 1

[cafbot (PoC: ggrisco)]

Attachment: EXTRA file attachment -

Comment 2

[cafbot (PoC: ggrisco)]

Attachment: decoded minidump -

Comment 3

[cafbot (PoC: ggrisco)]

Observed on: 

Device: msm8909
Gonk Version: AU_LINUX_GECKO_LF.BR.1.2.3.00.00.00.000.123
Moz BuildID: 20150405002503
Manifest: https://www.codeaurora.org/cgit/quic/lf/b2g/manifest/tree/caf_AU_LINUX_GECKO_LF.BR.1.2.3.00.00.00.000.123.xml?h=release
Gecko Version: 37.0
Gaia:  http://git.mozilla.org/?p=releases/gaia.git;a=commit;h=a6351e1197d54f8624523c2db9ba1418f2aa046f
Gecko: http://git.mozilla.org/?p=releases/gecko.git;a=commit;h=6bb2afcce9872a7cbc65b4a58f752e2d5ac02345
Patches: bug 1145724, bug 1143694, bug 1146987, bug 1133398, bug 1152095, bug 1150924, bug 1133147, bug 1150271, bug 1150916

Comment 4

[cafbot (PoC: ggrisco)]

Attachment: EXTRA file attachment - AU_LINUX_GECKO_LF.BR.1.2.3.00.00.00.000.123

Comment 5

[cafbot (PoC: ggrisco)]

Attachment: decoded minidump - AU_LINUX_GECKO_LF.BR.1.2.3.00.00.00.000.123

Comment 6

[Mike Lee [:mlee]]

Hi Maire,

Please have Paul or someone else on your team pick this up. It's affecting our ability to reach the fxOS 2.2 MTBF goal per CAF's tests. If this isn't code your team handles please help route this to the team that does.

Thanks!
Mike

Comment 7

[Maire Reavy [:mreavy]]

Please see bug 1152431 comment 7: https://bugzilla.mozilla.org/show_bug.cgi?id=1152431#c7. We currently believe that this bug and that one have the same root cause.

We really need a regression range from someone who can easily repro this.  This crash does not appear to reproduce on Nightly.  (I asked for help getting a regression range in the other bug.)

Comment 8

[Mike Lee [:mlee]]

Thanks Maire.

(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #7)
> 
> We really need a regression range from someone who can easily repro this. 
> This crash does not appear to reproduce on Nightly.

Nick,
Can CAF provide a regression range for this issue?

Thanks,
Mike

Comment 9

[Nicholas Troast [:ntroast]]

(In reply to Mike Lee [:mlee] from comment #8)
> Thanks Maire.
> 
> (In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #7)
> > 
> > We really need a regression range from someone who can easily repro this. 
> > This crash does not appear to reproduce on Nightly.
> 
> Nick,
> Can CAF provide a regression range for this issue?
> 
> Thanks,
> Mike

We have seen this issue once on March 13th and once on April 8th

Comment 10

[Mike Lee [:mlee]]

Thanks Nick. Is it possible to provide more detailed build and commit information for last known good and first failed runs similar to what's provided in comment 3?

(In reply to cafbot (PoC: ggrisco) from comment #3)
> Observed on: 
> 
> Device: msm8909
> Gonk Version: AU_LINUX_GECKO_LF.BR.1.2.3.00.00.00.000.123
> Moz BuildID: 20150405002503
> Manifest:
> https://www.codeaurora.org/cgit/quic/lf/b2g/manifest/tree/
> caf_AU_LINUX_GECKO_LF.BR.1.2.3.00.00.00.000.123.xml?h=release
> Gecko Version: 37.0
> Gaia: 
> http://git.mozilla.org/?p=releases/gaia.git;a=commit;
> h=a6351e1197d54f8624523c2db9ba1418f2aa046f
> Gecko:
> http://git.mozilla.org/?p=releases/gecko.git;a=commit;
> h=6bb2afcce9872a7cbc65b4a58f752e2d5ac02345
> Patches: bug 1145724, bug 1143694, bug 1146987, bug 1133398, bug 1152095,
> bug 1150924, bug 1133147, bug 1150271, bug 1150916

Thanks,
Mike

Comment 11

[Nicholas Troast [:ntroast]]

I'm sorry. The tester did provide a note that they were randomly changing the ringtones when this crash occurred.

We saw this on AU 100 back on March 13th.

Comment 12

[Nicholas Troast [:ntroast]]

Could bug 1107534 be related?

Comment 13

[Ken Chang[:kenkai]]

What do you think? is bug 1107534 related to this bug?

Comment 14

[cafbot (PoC: ggrisco)]

Observed on: 

Device: msm8909
Gonk Version: AU_LINUX_GECKO_LF.BR.1.2.3.00.00.00.000.126
Moz BuildID: 20150406002503
Manifest: https://www.codeaurora.org/cgit/quic/lf/b2g/manifest/tree/caf_AU_LINUX_GECKO_LF.BR.1.2.3.00.00.00.000.126.xml?h=release
Gecko Version: 37.0
Gaia:  http://git.mozilla.org/?p=releases/gaia.git;a=commit;h=a6351e1197d54f8624523c2db9ba1418f2aa046f
Gecko: http://git.mozilla.org/?p=releases/gecko.git;a=commit;h=6bb2afcce9872a7cbc65b4a58f752e2d5ac02345
Patches: bug 1152361, bug 1150923, bug 1152095, bug 1150924, bug 1133147, bug 1150271, bug 1150916, bug 1148641

Comment 15

[cafbot (PoC: ggrisco)]

Attachment: EXTRA file attachment - AU_LINUX_GECKO_LF.BR.1.2.3.00.00.00.000.126

Comment 16

[cafbot (PoC: ggrisco)]

Attachment: decoded minidump - AU_LINUX_GECKO_LF.BR.1.2.3.00.00.00.000.126

Comment 17

[StevenLee[:slee]]

(In reply to Ken Chang[:ken] from comment #13)
> What do you think? is bug 1107534 related to this bug?

It looks like dangling pointer. For the minidump below, it crashed at 0x5a5a5a62 and r2 is 0x5a5a5a5a. It means that r2 has already been deleted somewhere. 

Hi ntroast,

Why do you think this bug is related to bug 1107534?

Crash address: 0x5a5a5a62
Thread 22 (crashed)
 0  libxul.so!mozilla::StreamBuffer::TrackIter::FindMatch [StreamBuffer.h : 95 + 0x0]
     r0 = 0xafd83b48    r1 = 0xb37dc680    r2 = 0x5a5a5a5a    r3 = 0x00000001

Comment 18

[Nicholas Troast [:ntroast]]

I see the Ringtones app in all of the logs, and that was the most recent bug that I know of that dealt with the Ringtones app. Just trying to look at the big picture here since we are seeing a lot of Ringtones app in these MSG bugs.

(In reply to StevenLee[:slee] from comment #17)
> (In reply to Ken Chang[:ken] from comment #13)
> > What do you think? is bug 1107534 related to this bug?
> 
> It looks like dangling pointer. For the minidump below, it crashed at
> 0x5a5a5a62 and r2 is 0x5a5a5a5a. It means that r2 has already been deleted
> somewhere. 
> 
> Hi ntroast,
> 
> Why do you think this bug is related to bug 1107534?
> 
> Crash address: 0x5a5a5a62
> Thread 22 (crashed)
>  0  libxul.so!mozilla::StreamBuffer::TrackIter::FindMatch [StreamBuffer.h :
> 95 + 0x0]
>      r0 = 0xafd83b48    r1 = 0xb37dc680    r2 = 0x5a5a5a5a    r3 = 0x00000001

Comment 19

[Greg Grisco]

We were seeing crashes like this even before bug 1107534 was fixed.  I used to be able to crash it manually by simply changing the ringtone selection multiple times.  But then at some point I wasn't able to reproduce it manually anymore.  Now we are seeing multiple crashes in ringtone with several different signatures during stability test runs.  I'll open a couple new bugs to share those logs.

Comment 20

[Pi Wei Cheng [:piwei] (inactive)]

Regression-window was added to a specific party that's not QAnalysts (see comment 8). Also, this seems to be happening on a device that we don't have. Currently we have ringtone related bugs on Flame device: bug 1147386 and bug 1139157.

Adding keyword to exclude this in our queries.

Comment 21

[StevenLee[:slee]]

(In reply to Greg Grisco from comment #19)
> We were seeing crashes like this even before bug 1107534 was fixed.  I used
> to be able to crash it manually by simply changing the ringtone selection
> multiple times.  But then at some point I wasn't able to reproduce it
About when? Maybe I can checkout the corresponsive code and see if I can reproduce on nexus.

> manually anymore.  Now we are seeing multiple crashes in ringtone with
> several different signatures during stability test runs.  I'll open a couple
> new bugs to share those logs.
Can you cc me in these bugs?

Comment 22

[Greg Grisco]

(In reply to StevenLee[:slee] from comment #21)
> (In reply to Greg Grisco from comment #19)
> > We were seeing crashes like this even before bug 1107534 was fixed.  I used
> > to be able to crash it manually by simply changing the ringtone selection
> > multiple times.  But then at some point I wasn't able to reproduce it
> About when? Maybe I can checkout the corresponsive code and see if I can
> reproduce on nexus.

I think I was seeing this on AU77 and then it went away for a long time.  You could try AU77 which had these gaia/gecko revisions:

gaia: da509caa7395d3d090ce973e8de082b4680a590d
gecko: 782bc95dce066e2b262f556adf947a9a1409e6a0

> > manually anymore.  Now we are seeing multiple crashes in ringtone with
> > several different signatures during stability test runs.  I'll open a couple
> > new bugs to share those logs.
> Can you cc me in these bugs?

Sure, I'll cc: you on these:

bug 1154030, bug 1154042, bug 1154051, bug 1152439, bug 1150918

Comment 23

[StevenLee[:slee]]

Attachment: logcat

Hi roc, 
I removed some unused log(should be from the vendor) for making the log easier to read. I found that some MediaStream was deleted more than once, ex 0xb094e280 is deleted at least thrice. Is that correct? Could it cause some problem?

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to StevenLee[:slee] from comment #23)
> I removed some unused log(should be from the vendor) for making the log
> easier to read. I found that some MediaStream was deleted more than once, ex
> 0xb094e280 is deleted at least thrice. Is that correct? Could it cause some
> problem?

Sorry, it's hard to tell because between two destruction calls for a particular address, the object might have been constructed at that address. I guess we should log the constructor too.

The mention of bug 1107534 and the Ringtones app makes me wonder whether this is a bug where we connect streams from different MSGs. I can't exactly see how the Ringtones app would trigger this, but it does create perhaps more than one new AudioContext and connect the same element to each one. I'm pretty sure that we don't have anything preventing an element from being connected to two different MSGs and then crashing (since the element's decoder MediaStream would be created in one MSG and then connected to a MediaStream in a different graph). Alastor, can you test creating two AudioContexts with different channel types and connecting the same element to both of them? We need to block that!

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: more assertions, more logging

This patch supercedes the previous patch in bug 1149494. It adds logging for MediaStream constructors as well as destructors. It also adds some asserts to check that we don't connect MediaStreams between different MediaStreamGraphs.

Can we get this into the CodeAurora tests?

Comment 26

[StevenLee[:slee]]

Attachment: patch

Hi roc,

As I know, CAF does not enable DEBUG. Do you think MOZ_CRASH help?

Comment 27

[StevenLee[:slee]]

Comment on attachment 8592836 [details]
patch

I uploaded the wrong patch.

Comment 28

[StevenLee[:slee]]

Comment on attachment 8592739 [details] [diff] [review]
more assertions, more logging

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

As comment 26, CAF doesn't enable DEBUG. We should use other way to log.

Comment 29

[Alastor Wu [:alwu]]

OK, I will test it.
keep ni for tracking.

Comment 30

[Alastor Wu [:alwu]]

Attachment: msgTest.patch

I write a test app to connect two differences MSG, but it didn't crash.
I will take a look for more details.

Comment 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: more assertions, more logging

Here's an updated patch using MOZ_RELEASE_ASSERT that should work in non-DEBUG.

Comment 32

[Nicholas Troast [:ntroast]]

I added attachment 8593277 [details] [diff] [review] to our build. I also removed the patch from bug 1150923 due to conflict, and this one I believe supersedes it.

It should be included within the next two AUs

Comment 33

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Thanks very much!!!

Comment 34

[StevenLee[:slee]]

I found some operations that could cause memory pressure. I'm not sure whether it could cause the problem. 

Repeat doing the 3 steps. 
1. Open ringtone app from settings
2. Select a ringtone to preview
3. go to homescreen

In the ringtone app, it creates an AudioElement in [1]. In step 2, it creates an AudioContext and connect to the AudioElement, [2]. In step 3, AudioContext is set to null, [3], but cycle collector finds that it cannot delete the AudioContext. That's because the AudioContext is pointed by the AudioElement. 

[1] https://git.mozilla.org/?p=releases/gaia.git;a=blob;f=apps/ringtones/js/tone_player.js;h=aa92cd42d894dc991a23ba99f9282c5cc7ca8df8;hb=refs/heads/v2.2#l17
[2] https://git.mozilla.org/?p=releases/gaia.git;a=blob;f=apps/ringtones/js/tone_player.js;h=aa92cd42d894dc991a23ba99f9282c5cc7ca8df8;hb=refs/heads/v2.2#l146
[3] https://git.mozilla.org/?p=releases/gaia.git;a=blob;f=apps/ringtones/js/tone_player.js;h=aa92cd42d894dc991a23ba99f9282c5cc7ca8df8;hb=refs/heads/v2.2#l151

Comment 35

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to StevenLee[:slee] from comment #34)
> I found some operations that could cause memory pressure. I'm not sure
> whether it could cause the problem. 
> 
> Repeat doing the 3 steps. 
> 1. Open ringtone app from settings
> 2. Select a ringtone to preview
> 3. go to homescreen
> 
> In the ringtone app, it creates an AudioElement in [1]. In step 2, it
> creates an AudioContext and connect to the AudioElement, [2]. In step 3,
> AudioContext is set to null, [3], but cycle collector finds that it cannot
> delete the AudioContext. That's because the AudioContext is pointed by the
> AudioElement. 

Are we permanently leaking? What keeps the AudioElement alive?

Comment 36

[StevenLee[:slee]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #35)
> Are we permanently leaking? What keeps the AudioElement alive?
No, but as long as the AudioElement is alive, all the MediaStreams(held by AudioContext) will not be deleted. In a simplified situation is that Gaia developers may create an AudioElement and input this AudioElement to many AudioContext. As long as the AudioElement is alive, all the AudioContexts cannot be deleted.

In this case, TonePlayer is created in a page and this page is always alive(except we kill ringtone app).
1. TonePlayer creates one AudioElement. 
2. When users select one ringtone to preview, it creates an AudioContext and link this AudioElement to the AudioContext. 
3. When it detects the window is hidden, it sets AudioContext to null.

If we repeat 2 and 3(go to homescreen then go back to ringtone app and preview), we get many AudioContexts that are related to an AudioElement.

Comment 37

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OK, how does the AudioElement keep links to many AudioContexts?

Comment 38

[StevenLee[:slee]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #37)
> OK, how does the AudioElement keep links to many AudioContexts?
In [1], this._player is the AudioElement. Since TonePlayer keeps the same AudioElement and TonePlayer keeps alive, the reference count of AudioContext will never be zero. 

[1] https://git.mozilla.org/?p=releases/gaia.git;a=blob;f=apps/ringtones/js/tone_player.js;h=aa92cd42d894dc991a23ba99f9282c5cc7ca8df8;hb=refs/heads/v2.2#l147

Comment 39

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Hmm, that doesn't answer what I asked. I think the answer to my question is that HTMLMediaElement::mOutputStreams never has elements removed from it (except for when they have mFinishWhenEnded set, which they won't here), so that array will just keep growing with new streams every time createMediaElementSource is called.

We should fix that, but I think a fix will be quite risky, so it would be good to find a workaround as well.

It seems to me that _setExclusiveMode(false) is supposed to cause the ringer AudioContext to be (eventually) destroyed, letting background music play again. However, currently it seems to me the ringer AudioContext will never be destroyed so that doesn't work. Am I right?

If so, then we might fix the crashes just by making _setExclusiveMode(false) not do anything and _setExclusiveMode(true) not creating a new AudioContext and MediaElementAudioSourceNode, if they already exist. How does that sound?

Comment 40

[StevenLee[:slee]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #39)
> Hmm, that doesn't answer what I asked. I think the answer to my question is
> that HTMLMediaElement::mOutputStreams never has elements removed from it
> (except for when they have mFinishWhenEnded set, which they won't here), so
> that array will just keep growing with new streams every time
> createMediaElementSource is called.
Sorry for late response, I spent some time tracing and verifying the scenario. Yes, I found that if I force mFinishWhenEnded to true, AudioContext will be deleted on the same operations. 

> It seems to me that _setExclusiveMode(false) is supposed to cause the ringer
> AudioContext to be (eventually) destroyed, letting background music play
> again. However, currently it seems to me the ringer AudioContext will never
> be destroyed so that doesn't work. Am I right?
> 
> If so, then we might fix the crashes just by making _setExclusiveMode(false)
> not do anything and _setExclusiveMode(true) not creating a new AudioContext
> and MediaElementAudioSourceNode, if they already exist. How does that sound?
Sure, I also want to fix the problem from the app side. I will upload a patch later. Thanks for the suggestion.

Comment 41

[StevenLee[:slee]]

Attachment: prevent creating multiple AudioContext

Hi David,

Per comment 36 and comment 39, we want ringtone app not to create multiple AudioContext. Please help review this patch. Thanks.

Comment 42

[StevenLee[:slee]]

Attachment: prevent creating multiple AudioContext

Sorry, I pasted the wrong link.

Comment 43

[David Flanagan [:djf]]

Comment on attachment 8596520 [details] [review]
prevent creating multiple AudioContext

Passing this review to Jim Porter who is in charge of the Ringtones app.

Comment 44

[Jim Porter (:squib)]

I think this is a UX change (the AudioContext stuff is designed to mute all background audio once you start previewing a ringtone, and then let the other audio resume as soon as the ringtones app is minimized), so you'll need UX feedback before I can do much with this. Personally, if things are so busted that we can't just fix the Gecko issue for 2.2, I'd rather we drop the AudioContext code entirely.

Tiffanie Shakespeare would probably be a good person to ask about the UX side.

Comment 45

[Alastor Wu [:alwu]]

Hi, Jim,
After landing the bug1027172, the audio context would be muted when its destination node doesn't have input.
Therefore, the UX designed feature doesn't take effect now. I think we need other methods to implement this feature.

Comment 46

[Jim Porter (:squib)]

If that's the case (I haven't tested it), let's just drop the AudioContext stuff entirely, since it doesn't do anything. I guess we could feed the AudioContext some empty audio data after the ringtone stops, but I've never been quite sure how to do that with AudioContext, and I'm not sure it's worth the effort anyway.

For 3.0, it sounds like this will be fixed with the new audio competing policy code.

Comment 47

[Ken Chang[:kenkai]]

Alastor, are you or Steven working on what Jim said in comment 46? Or do you have any other idea?

Comment 48

[Alastor Wu [:alwu]]

Hi, Ken,
We don't involve on this preview feature. I think that might be solved by Jim's proposal which is to send the AudioContext some empty audio data after the ringtone stops. 
As I know, the audio competing policy is not related with solving that problem. Because we just move the decision logic from the Gecko to Gaia, the background music would still be resume in the new competing rule when the ringer previews ended.

Comment 49

[StevenLee[:slee]]

Hi Nicholas,

As I know, Greg resolved many crash problem as "WORKSFORME". Can you still reproduce this issue? If not, I will remove the blocker flag. 

Thanks.

Comment 50

[Greg Grisco]

(In reply to StevenLee[:slee] from comment #49)
> Hi Nicholas,
> 
> As I know, Greg resolved many crash problem as "WORKSFORME". Can you still
> reproduce this issue? If not, I will remove the blocker flag. 
> 
> Thanks.

Hi Steven, we haven't seen this crash since AU 126.  I didn't close this bug in particular because it looked like there was a patch being worked on, but I'm ok if you want to close it for now until it reproduces again.

Comment 51

[StevenLee[:slee]]

(In reply to Greg Grisco from comment #50)
> Hi Steven, we haven't seen this crash since AU 126.  I didn't close this bug
> in particular because it looked like there was a patch being worked on, but
> I'm ok if you want to close it for now until it reproduces again.
Greg, thanks for the information.

Comment 52

[Mike Lee [:mlee]]

Steven & Jim,

Are you actively working on patch attachment 8596520 [details] [review] [1]? If not since the reported issue isn't appearing in CAF testing, please close this.

Thanks,
Mike

Comment 53

[StevenLee[:slee]]

(In reply to Mike Lee [:mlee] from comment #52)
> Steven & Jim,
> 
> Are you actively working on patch attachment 8596520 [details] [review]? If not
> since the reported issue isn't appearing in CAF testing, please close this.
OK. We will fix the AudioContext-cannot-be-released problem in bug 1156586.

Comment 54

[Jim Porter (:squib)]

Comment on attachment 8596520 [details] [review]
prevent creating multiple AudioContext

Clearing review since this bug is WONTFIX.