929029 - [B2G][Settings][Sound] - crash in nsSimpleURI::Release()

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[KTucker [:KTucker][Inactive 3/4/2016]]

Attachment: RingtoneCrashLog.txt

Description:
A crash will occur when setting a song as a ringtone through settings.

Repro Steps:
1)  Updated Buri to Build ID: 20131014080339
2)  Tap on the "Settings" icon.
3)  Tap on "Sound". 
4)  Tap on the box below "Ringer".
5)  Tap on "Music". 
6)  Tap on a "Song".
7)  Tap on the "Play" icon to play the song.
8)  Tap on the "Checked box" in the upper right corner of the screen to save the song as the ringtone.

Actual:
Settings will crash. Signature: nsSimpleURI::Release()

Expected:
The song is saved as the ringtone without a crash occurring.

Environmental Variables
Device: Buri v 1.3.0 Mozilla RIL
Build ID: 20131014080339
Gecko: http://hg.mozilla.org/mozilla-central/rev/ab8e70fb76a8
Gaia: 74e7e69d98206bda5c517c0ada57d43de662913e
Platform Version: 27.0a1

Notes:
Repro frequency: 100%
See attached: logcat

Comment 1

[KTucker [:KTucker][Inactive 3/4/2016]]

This issue reproduces on 10/14 on Buri v 1.3.0 Mozilla RIL

Environmental Variables
Device: Buri v 1.3.0 Mozilla RIL
Build ID: 20131014080339
Gecko: http://hg.mozilla.org/mozilla-central/rev/ab8e70fb76a8
Gaia: 74e7e69d98206bda5c517c0ada57d43de662913e
Platform Version: 27.0a1

Settings will crash after setting a song as a ringtone.


This issue does not reproduce on the 10/15 Buri v 1.3.0 Mozilla RIL

Environmental Variables
Device: Buri v 1.3.0 Mozilla RIL
Build ID: 20131015040202
Gecko: http://hg.mozilla.org/mozilla-central/rev/febfe3c7732b
Gaia: 17e871ae1f82699793e3cd28acda805ba724a8b6
Platform Version: 27.0a1

The user can set a song as a ringtone through settings without a crash occurring.

Comment 2

[KTucker [:KTucker][Inactive 3/4/2016]]

This bug was filed from the Socorro interface and is 
report bp-61d71bce-4be1-47ef-b080-90d362131021.

Comment 3

[Maire Reavy [:mreavy]]

This isn't in my area (WebRTC or Web Audio), but this should block 1.3 IMO

Comment 4

[Jason Smith [:jsmith]]

(In reply to Maire Reavy [:mreavy] from comment #3)
> This isn't in my area (WebRTC or Web Audio), but this should block 1.3 IMO

Agreed.

Comment 5

[Ivan Tsay (:ITsay)]

Hi Hema,

Could you check if this one is on your team for the triage?

Comment 6

[Jason Smith [:jsmith]]

(In reply to Ivan Tsay (:ITsay) from comment #5)
> Hi Hema,
> 
> Could you check if this one is on your team for the triage?

FWIW - Hema's team to my understanding handles triaging Gaia bugs for Gallery, Video, Music, FMRadio, and Camera. However, that doesn't include gecko bugs.

This bug is a gecko content media crash. Very likely something that Chris Double or Sotaro would look into.

Comment 7

[Hema Koka [:hema]]

Sotaro or media recording team is most likely the best person to look into this.

Comment 8

[Ivan Tsay (:ITsay)]

(In reply to Hema Koka [:hema] from comment #7)
> Sotaro or media recording team is most likely the best person to look into
> this.

Sotaro, would you mind take a look this one and see if you know who can handle this. It's not under the area of media recording team. I also checked with CJ and he had no idea about it.

Comment 9

[Sotaro Ikeda [:sotaro]]

(In reply to Ivan Tsay (:ITsay) from comment #8)
> Sotaro, would you mind take a look this one and see if you know who can
> handle this. It's not under the area of media recording team. I also checked
> with CJ and he had no idea about it.

Okey, I am going take a look.

Comment 10

[Sotaro Ikeda [:sotaro]]

I can not re-generate the crash from a ROM built from today's master on hamachi.

Comment 11

[Jason Smith [:jsmith]]

QA Wanted for a retest

Comment 12

[Sotaro Ikeda [:sotaro]]

(In reply to ktucker from comment #2)
> This bug was filed from the Socorro interface and is 
> report bp-61d71bce-4be1-47ef-b080-90d362131021.

From the crash log, it seems mp3 regression like Bug 927884. In the crash log, the following function is present. OmxDecoderProcessCachedDataTask does mp3 parse on io thread. But I am not sure why it is a problem.

> mozilla::OmxDecoderProcessCachedDataTask::~OmxDecoderProcessCachedDataTask

Comment 13

[sarsenyev]

The crash still reproduces 100% 6/6 on the latest master Buri build 
https://crash-stats.mozilla.com/report/index/0736ee5e-5100-418f-9dac-ce6fd2131107

But I wasn't able to reproduce it when I had just a few songs on my memory card, but when I filled the memory card with 1 GB songs, the bug reproduces for 100%, as far as I open the ringtone category and choosing the "Music library", the app crashes even without choosing any songs

Device: Buri 1.3 Master build
BuildID: 20131106040203
Gaia: 3b5fe429f2414f2a9d7241b311b399033bb10612
Gecko: 9ba3faa35c96
Version: 28.0a1
Firmware Version: US_20131015

Comment 14

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

I think the problem here is that we are destroying the MediaResource on a non-main thread, and that's release the nsSimpleURI on a non-main thread, and nsSimpleURI's AddRef/Release aren't threadsafe.

MediaResource must be destroyed on the main thread. If necessary you need to dispatch an event to the main thread to release the MediaResource on the main thread, like I did in the Windows Media Foundation backend here:

http://mxr.mozilla.org/mozilla-central/source/content/media/wmf/WMFByteStream.cpp#129

Comment 15

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] Bug 929029: Cleanup OmxDecoder on main thread

Hi,

This implements the proposed fix. I haven't been able to see this bug, so could someone how is able to reproduce it try the patch?

Comment 16

[Thomas Zimmermann [:tzimmermann] [:tdz]]

'who'

Comment 17

[Sotaro Ikeda [:sotaro]]

Thanks! I could reproduce it. I am going to comfirm the patch.

Comment 18

[Sotaro Ikeda [:sotaro]]

I can not reproduce the crash by Steps in Comment 0. But could reproduce it by the following Steps.

Prepare:
   Store more than 1 hr mp3 files in sd.

Repro Steps:
1)  Tap on the "Settings" icon.
2)  Tap on "Sound". 
3)  Tap on the box below "Ringer".
4)  Tap on "Music". 
5)  Tap on the long mp3 file.
6)  Tap on the "Play" icon to play the song.
7)  Soon after 6) Tap on the "<" in the upper left corner of the screen.

continue 6) 7) until crash.

Comment 19

[Sotaro Ikeda [:sotaro]]

Thomas can you take a bug? You are actually working for the bug.

Comment 20

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Ok, I take the bug, but it's still not reproducible here.

Comment 21

[Sotaro Ikeda [:sotaro]]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #20)
> Ok, I take the bug, but it's still not reproducible here.

If mp3 file is very big and sd card is slow, the crash seems easier to reproduce. I am using more than 64 MB mp3 files.

Comment 22

[Sotaro Ikeda [:sotaro]]

The crash is still happens after applying attachment 829273 [details] [diff] [review].

Comment 23

[Sotaro Ikeda [:sotaro]]

Attachment: crash of stacktrace

Comment 24

[Sotaro Ikeda [:sotaro]]

After MediaOmxReader was destroyed, OmxDecoderProcessCachedDataTask becomes only object referencing OmxCodec. Then OmxDecoderProcessCachedDataTask's destructor triggers OmxCodec's destructor. And then FileMediaResource's destructor is called in non-main thread.

Comment 25

[Thomas Zimmermann [:tzimmermann] [:tdz]]

(In reply to Sotaro Ikeda [:sotaro] from comment #24)
> After MediaOmxReader was destroyed, OmxDecoderProcessCachedDataTask becomes
> only object referencing OmxCodec. Then OmxDecoderProcessCachedDataTask's
> destructor triggers OmxCodec's destructor. And then FileMediaResource's
> destructor is called in non-main thread.

I probably just introduced another race condition.

With the patch applied this actually shouldn't happen, because there should always be a OmxDecoderNotifyDataArrivedRunnable that still contains a reference to the OmxDecoder.

I suspect that this runnable finished before OmxDecoderProcessCachedDataTask.

Comment 26

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] Bug 929029: Cleanup OmxDecoder on main thread (v2)

This patch should not contain the race condition I described in my previous posting. I know the code is quite ugly, but does it solve the problem for you?

Comment 27

[Sotaro Ikeda [:sotaro]]

By applying attachment 829375 [details] [diff] [review], a frequency of the crash seems to be decreased. But still happens.

Comment 28

[Sotaro Ikeda [:sotaro]]

Attachment: stacktrace of the crash 2

The stacktrace is slightly different from previous one.

Comment 29

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

Perhaps a better solution is to implement a custom MediaResourse::Release that dispatches an event to the main thread to destroy the media resource when the refcount reaches 0? Then we don't need bespoke fixes like what we're doing here and in the WMF backend.

Comment 30

[Thomas Zimmermann [:tzimmermann] [:tdz]]

(In reply to Chris Pearce (:cpearce) from comment #29)
> Perhaps a better solution is to implement a custom MediaResourse::Release
> that dispatches an event to the main thread to destroy the media resource
> when the refcount reaches 0? Then we don't need bespoke fixes like what
> we're doing here and in the WMF backend.

The log in attachment 829412 [details] shows that the crash also happens on the main thread. Something else seems going wrong here.

Comment 31

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] Bug 929029: Cleanup OmxDecoder on main thread (v3)

This patch implements what cpearce describes in comment 14. I can only try different fixes, since I still cannot reproduce the problem, even with large files (~150 MiB).

Comment 32

[Sotaro Ikeda [:sotaro]]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #30)
> 
> The log in attachment 829412 [details] shows that the crash also happens on
> the main thread. Something else seems going wrong here.

It was not on main thread.

Comment 33

[Sotaro Ikeda [:sotaro]]

Attachment: stacktrace of the crash 3

A stack trace of the crash after applying attachment 830148 [details] [diff] [review].

Comment 34

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #33)
> Created attachment 830219 [details]
> stacktrace of the crash 3
> 
> A stack trace of the crash after applying attachment 830148 [details] [diff] [review]
> [review].

Crash still happens by applying attachment 830148 [details] [diff] [review].

Comment 35

[Ivan Tsay (:ITsay)]

Plus this one for v1.3 since the crash issue is critical.

Comment 36

[Jason Smith [:jsmith]]

What's the needinfo request for specifically?

Comment 37

[Thomas Zimmermann [:tzimmermann] [:tdz]]

(In reply to Sotaro Ikeda [:sotaro] from comment #32)
> (In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #30)
> > 
> > The log in attachment 829412 [details] shows that the crash also happens on
> > the main thread. Something else seems going wrong here.
> 
> It was not on main thread.

Oh, you're right. I just saw the runnable in the stack trace, but there is also the task structure involved.

Comment 38

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] Bug 929029: Release OmxDecoder on main thread (v4)

From stack 3, it looks like there is more involved here than just MediaResource.

The second patch still didn't work as expected. This patch should (hopefully) release OmxDecoder and anything it references on the main thread after the reference on the I/O thread is gone.

Comment 39

[Sotaro Ikeda [:sotaro]]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #38)
> Created attachment 830782 [details] [diff] [review]
> [01] Bug 929029: Release OmxDecoder on main thread (v4)
> 

I checked the  attachment 830782 [details] [diff] [review], the crash still happens(not often).

Comment 40

[Sotaro Ikeda [:sotaro]]

Attachment: stacktrace of the crash by applying attachment 830782

Comment 41

[Sotaro Ikeda [:sotaro]]

I feel that it might be better to use nsMainThreadPtrHandle and nsMainThreadPtrHolder to fix this problem. like camera does like following.

http://mxr.mozilla.org/mozilla-central/source/dom/camera/CameraControlImpl.h#159

Comment 42

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Sotaro,

Can I give this bug to you? I won't be able to work on it for some time, and I'm still not able to reproduce it. I tried to move the release of OmxDecoder to the main thread, but OmxDecoder is freed on the I/O thread in all stack traces you provided. Maybe there is something wrong with the reference counting of OmxDecoder.

Comment 43

[Sotaro Ikeda [:sotaro]]

NO problem. I take this bug.

Comment 44

[Sotaro Ikeda [:sotaro]]

Attachment: patch - use nsMainThreadPtrHandle in MediaResource

By applying the patch, I confirmed the crash seems to be fixed on master hamachi.

Comment 45

[Sotaro Ikeda [:sotaro]]

Attachment: patch ver2 - use nsMainThreadPtrHandle in MediaResource

Fix a nit.

Comment 46

[Sotaro Ikeda [:sotaro]]

https://tbpl.mozilla.org/?tree=Try&rev=a629087121ba
A lot of tests failed at ChannelMediaResource::OpenChannel() :-(

Comment 47

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

You should probably get roc to check this patch too. He wrote most of MediaResource and MediaCache, so he understands the assumptions the code makes best.

Comment 48

[Sotaro Ikeda [:sotaro]]

After fixed the crash, I will ask to roc.

Comment 49

[Sotaro Ikeda [:sotaro]]

Attachment: patch ver3 - use nsMainThreadPtrHandle in MediaResource

Fix as to use mChannel.get() for checking nullptr.

Comment 50

[Sotaro Ikeda [:sotaro]]

https://tbpl.mozilla.org/?tree=Try&rev=1356dc623fdd
almost green.

Comment 51

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #50)
> https://tbpl.mozilla.org/?tree=Try&rev=1356dc623fdd
> almost green.

In the orange, the following test are failed related to video/audio. It is already handled by Bug 943556. 
- /tests/toolkit/content/tests/widgets/test_videocontrols_standalone.html

Comment 52

[Sotaro Ikeda [:sotaro]]

https://tbpl.mozilla.org/?tree=Try&rev=6ba0ed9b44ab
re-tryserver. NO media related test failure.

Comment 53

[Sotaro Ikeda [:sotaro]]

Attachment: patch ver4 - use nsMainThreadPtrHandle in MediaResource

Committable patch. Carry "roc, doublec review+".

Comment 54

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/a1628c9ba4af

Comment 55

[Ryan VanderMeulen [:RyanVM]]

Backed out for crashes/hangs in test_dataChannel_basicAudio.html on all platforms. Please run this through Try before requesting checkin again.
https://hg.mozilla.org/integration/b2g-inbound/rev/d99d3514fd6a

https://tbpl.mozilla.org/php/getParsedLog.php?id=31322007&tree=B2g-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=31321907&tree=B2g-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=31322194&tree=B2g-Inbound

Comment 56

[Sotaro Ikeda [:sotaro]]

Hmm, I did not saw this crash on tryserver tests.

Comment 57

[Sotaro Ikeda [:sotaro]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5][Back 2-Dec] from comment #55)
> https://tbpl.mozilla.org/php/getParsedLog.php?id=31322007&tree=B2g-Inbound
> https://tbpl.mozilla.org/php/getParsedLog.php?id=31321907&tree=B2g-Inbound
> https://tbpl.mozilla.org/php/getParsedLog.php?id=31322194&tree=B2g-Inbound

These failure seems not related to this bug. I checked the dom/media source code. These code does not use nsMainThreadPtrHandle and nsMainThreadPtrHolder for JS objects they are referenced from non main thread.

Comment 58

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #57)
> These failure seems not related to this bug. I checked the dom/media source
> code. These code does not use nsMainThreadPtrHandle and
> nsMainThreadPtrHolder for JS objects they are referenced from non main
> thread.

Created Bug 945334.

Comment 59

[Ryan VanderMeulen [:RyanVM]]

You're right, I was being dumb. It was bug 926746 that caused it. Re-landed.
https://hg.mozilla.org/integration/b2g-inbound/rev/f9d8f53e8739

Comment 60

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f9d8f53e8739