When the RTSP playback reaches the end of stream, the system could crash at this line: http://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/rtsp/rtsp/RTSPSource.cpp#716

From the GDB backtrace log, it seems somehow the accessUnit becomes NULL in RTSPSource::onTrackDataAvailable(). The assertion MOZ_ASSERT(accessUnit != NULL) only works in debug build, so it doesn't catch this error.

Jonathan, Please help to see if you can reproduce this bug.

I doubt this bug is a recent regression. Before the patch of bug 1045062, the assertion is CHECK(accessUnit != NULL), and it should cause a crash if the assertion fails. But we didn't encounter such crash previously. https://hg.mozilla.org/mozilla-central/diff/ad7a09962cc4/netwerk/protocol/rtsp/rtsp/RTSPSource.cpp

Yes, I can reproduce. I'll look into this.

Hi Howie, This bug is critical. It shall block 2.2.

[Triage] It's a regression bug and causes the system crash.

Please try out this patch. It may work as a workaround if not solving the underlying problem. I'll explain later.

The variable accessUnit should be assigned by this line: > status_t err = dequeueAccessUnit(info->mIsAudio, &accessUnit); Then some error handling follows: > if (err == -EWOULDBLOCK) { > return; > } else if (err == INFO_DISCONTINUITY) { > ... > return; > } However, I found that dequeueAccessUnit may produce error code that is not OK, -EWOULDBLOCK nor INFO_DISCONTINUITY, so I added a third case > } else if (err != OK) { > return; > } After discussing with Benjamin and Vincent offline, we looked deeper and found that the unexpected error was ERROR_END_OF_STREAM, which should be expected because it was the end of stream. Therefore, in this revised patch, I removed the third case and let the function return when err == ERROR_END_OF_STREAM. Benjamin said we should try to combine this with our current end-of-stream mechanism that uses timer.

Jonathan, Thanks for identifying the cause. From your explanation, I can understand: RTSPSource::dequeueAccessUnit() invokes AnotherPacketSource::dequeueAccessUnit(), which could possibly return ERROR_END_OF_STREAM, which is defined in MediaErrors.h. http://androidxref.com/4.4_r1/xref/frameworks/av/include/media/stagefright/MediaErrors.h Thus, RTSPSource::onTrackAvailable() should deal with the error code ERROR_END_OF_STREAM. It looks reasonable and I'll grant the patch. But could we explain why this issue doesn't occur previously? It's better if we know what previous change caused this side effect. :)

The patch's been put on try server. https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=5a3244e29b9a I don't know why it didn't happen before. The ERROR_END_OF_STREAM comes from this method: http://androidxref.com/4.4.4_r1/xref/frameworks/av/media/libstagefright/mpeg2ts/AnotherPacketSource.cpp#256 Maybe some of our recent patches affect this or there's some change to the framework.

Jonathan, I just refreshed my memory about end-of-stream by reading my own comments several months ago. For your reference: https://bugzilla.mozilla.org/show_bug.cgi?id=1021006#c12 As discussed offline, although the patch avoids the crash, the access units remaining in the queue might cause problem for re-play after EOS. Maybe we could clean up the queue of access units in performPlaybackEnded(). Please file another bug to track this issue.

(In reply to Ethan Tseng [:ethan] from comment #11) > Jonathan, > > I just refreshed my memory about end-of-stream by reading my own comments > several months ago. > For your reference: > https://bugzilla.mozilla.org/show_bug.cgi?id=1021006#c12 > > As discussed offline, although the patch avoids the crash, the access units > remaining in the queue > might cause problem for re-play after EOS. The replay after EOS will be a seek. So maybe there is no more problem. > Maybe we could clean up the queue of access units in performPlaybackEnded(). > Please file another bug to track this issue. Agree, clear them at performPlaybackEnded() is a good idea.

(In reply to Ethan Tseng [:ethan] from comment #11) > Please file another bug to track this issue. Here it is: bug 1090838.

Couldn't repro this manually, Back-end issue

Comment on attachment 8511851 [details] [diff] [review] Let onTrackDataAvailable return when getting ERROR_END_OF_STREAM [Approval Request Comment] Bug caused by (feature/regressing bug #): Bug 1111978 User impact if declined: Potential system crash Testing completed: On v2.1 Risk to taking this patch (and alternatives if risky): None String or UUID changes made by this patch: None

Hi Bhavana, This patch prevents a terrible potential system crash from RTSP streaming. We encountered this kind of crash while working on bug 1111978. Could you help to grant this uplift request?

Comment on attachment 8511851 [details] [diff] [review] Let onTrackDataAvailable return when getting ERROR_END_OF_STREAM Approving given the patch is low risk and avoids a serious crash. Requesting QA verification once this land on 2.1.

Comment on attachment 8511851 [details] [diff] [review] Let onTrackDataAvailable return when getting ERROR_END_OF_STREAM [Approval Request Comment] Bug caused by (feature/regressing bug #): Bug 1146251 User impact if declined: Possible system crash (device reboot) Testing completed: On mozilla-b2g32_v2_0 Risk to taking this patch (and alternatives if risky): None String or UUID changes made by this patch: None Bhavana, The project Midori is based on B2G 2.0, which does not include this patch and has potential crash issue while playing RTSP streaming. The change and risk of this patch is pretty low, could we land it to 2.0?