This bug was filed from the Socorro interface and is report bp-3b60537e-d6b6-4942-9e9c-d254c2140918. ============================================================= I encountered this crash on the latest 2.2 Flame KK build: Enviromental Variables: ---------------------------------------- Device: Flame 2.2 Master BuildID: 20140917223000 Gaia: d37950eb09e28aa18d0e01df9ff90574bd4337e0 Gecko: 426497473505 Version: 35.0a1 (2.2 Master) Firmware: V180 User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0 Repro Steps: 1) Update device to latest buildID: 20140917223000 2) Have more than 1 song on the SD card 3) Open music app and tap the fastforward and rewind buttons Repro Rate: 100% - 5/5 attempts

QAWanted for branch checks.

Assuming tap means "tap a lot" I can reproduce this on my Nexus 5.

Take it.

Should stop ProcessingCachedData process if decoder/reader enter shutdown stage.

This patch stops the ProcessCachedData process when MediaOmxReader enter shutdown stage. I try to use mDecoder->IsShutdown() in OmxReaderNotifyDataArrivedRunnable::NotifyDataArrived() But it only reduce the crash rate. So I create mIsShutdown variable to check if this reader has entered shutdown stage.

(In reply to Randy Lin [:rlin] from comment #6) > I try to use mDecoder->IsShutdown() in > OmxReaderNotifyDataArrivedRunnable::NotifyDataArrived() > But it only reduce the crash rate. > So I create mIsShutdown variable to check if this reader has entered > shutdown stage. This is because MediaOmxReader holds a raw pointer to |mDecoder| which becoming a dangling pointer.

Comment on attachment 8492073 [details] [diff] [review] patch v1 Review of attachment 8492073 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/omx/MediaOmxReader.cpp @@ +100,5 @@ > { > const char* buffer = mBuffer.get(); > > while (mLength) { > + if (mOmxReader->IsShutdown()) { It won't work this way. Polling incurs racing. MediaOmxReader::Shutdown() could happen in between |mOmxReader->IsShutdown()| and |mOmxReader->NotifyDataArrived()|. Also, since OmxReaderProcessCachedDataTask and OmxReaderNotifyDataArrivedRunnable hold strong reference to MediaOmxReader, the life cycle of MediaOmxReader is extended and mDecoder could become invalid during the extension. That will also result in memory corruptions.

This bug repro's on Flame KK builds: Flame 2.2, OpenC 2.2 Actual Results: Able to crash the music app by playing a song then hitting forward and rewind a couple times. Repro Rate: 6/6 Environmental Variables: Device: Flame Master KK BuildID: 20140922040649 Gaia: 3802009e1ab6c3ddfc3eb15522e3140a96b33336 Gecko: 5e704397529b Version: 35.0a1 (Master) Firmware Version: L1TC10011800 User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0 ----------------------------------------------------------------- Environmental Variables: Device: Open_C Master BuildID: 20140922040649 Gaia: 3802009e1ab6c3ddfc3eb15522e3140a96b33336 Gecko: 5e704397529b Version: 35.0a1 (Master) Firmware Version: P821A10V1.0.0B06_LOG_DL User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0 ----------------------------------------------------------------- ----------------------------------------------------------------- This bug does NOT repro on Flame kk build: Flame 2.1, Flame 2.0, Flame 2.0 KK Base Actual Result: No crash occurs when using the forward and rewind buttons in the music app. Repro Rate: 0/15 Environmental Variables: Device: Flame 2.1 KK BuildID: 20140922053548 Gaia: 689c4ad4d8c3e4aa95805a2e49ae6cf786a1ae91 Gecko: 185fc54d29c1 Version: 34.0a2 Firmware Version: L1TC10011800 User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0 ----------------------------------------------------------------- Environmental Variables: Device: Flame 2.0 KK BuildID: 20140920152249 Gaia: 0658006be8a00fdb5931ee15a0aa353a3ab231ba Gecko: c0086da55273 Version: 32.0 (2.0) Firmware Version: L1TC10011800 User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0 ----------------------------------------------------------------- Environmental Variables: Device: Flame 2.0 KK Base BuildID: 20140904160718 Gaia: 506da297098326c671523707caae6eaba7e718da Gecko: Version: 32.0 (2.0) Firmware: V180 User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

nomming to block - regression in a major feature, reproducible crash

Blocking Reason: Basic function regressed

I meant basic function causing crash in my previous comment

Hi JW, Could you take a look?

this one is correct.

Comment on attachment 8494221 [details] [diff] [review] WIP Review of attachment 8494221 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/omx/MediaOmxReader.cpp @@ +590,5 @@ > nsresult rv = mDecoder->GetResource()->ReadFromCache(buffer.get(), > aOffset, bufferLength); > NS_ENSURE_SUCCESS(rv, -1); > > + mOmxReaderNotifyDataArrivedRunnable = Per discussion offline, this could happen after Shutdown() and won't get revoked.

Thanks JW, Here is the new one, avoid to create new runnable when enter shutdown stage.

Comment on attachment 8494295 [details] [diff] [review] bug.patch v3 Review of attachment 8494295 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/omx/MediaOmxReader.cpp @@ +45,5 @@ > > void Run() > { > MOZ_ASSERT(!NS_IsMainThread()); > + if (mOmxReader) { See below. We don't need this check for we won't create an OmxReaderProcessCachedDataTask object when mOmxReader is null. @@ +125,5 @@ > // We cannot read data in the main thread because it > // might block for too long. Instead we post an IO task > // to the IO thread if there is more data available. > XRE_GetIOMessageLoop()->PostTask(FROM_HERE, > + new OmxReaderProcessCachedDataTask(mOmxReader, mOffset)); Don't even bother posting the task when mOmxReader is null. @@ +179,5 @@ > } > > void MediaOmxReader::Shutdown() > { > + if (mOmxReaderNotifyDataArrivedRunnable.get()) { Just say |if (mOmxReaderNotifyDataArrivedRunnable)|. @@ +182,5 @@ > { > + if (mOmxReaderNotifyDataArrivedRunnable.get()) { > + mOmxReaderNotifyDataArrivedRunnable->Revoke(); > + } > + MutexAutoLock lock(mMutex); Move this lock above, since mOmxReaderNotifyDataArrivedRunnable is accessed by different threads. @@ +597,5 @@ > buffer.forget(), > bufferLength, > aOffset, > + resourceLength); > + MutexAutoLock lock(mMutex); Move this lock above, since mOmxReaderNotifyDataArrivedRunnable is accessed by different threads. @@ +598,5 @@ > bufferLength, > aOffset, > + resourceLength); > + MutexAutoLock lock(mMutex); > + if (mIsShutdown) { We should check |mIsShutdown| at the top of this function and return immediately if it is true. Otherwise it is invalid to access |mDecoder| after shutdown. ::: content/media/omx/MediaOmxReader.h @@ +26,5 @@ > class TimeRanges; > } > > class AbstractMediaDecoder; > +class OmxReaderNotifyDataArrivedRunnable; You might want to have OmxReaderNotifyDataArrivedRunnable inherit nsICancelableRunnable to keep OmxReaderNotifyDataArrivedRunnable in .cpp only. @@ +44,5 @@ > protected: > android::sp<android::OmxDecoder> mOmxDecoder; > android::sp<android::MediaExtractor> mExtractor; > MP3FrameParser mMP3FrameParser; > + nsRefPtr<OmxReaderNotifyDataArrivedRunnable> mOmxReaderNotifyDataArrivedRunnable; Then we can say nsRefPtr<nsICancelableRunnable> here.

Modify mutex scope. Also changing the nsRefPtr<nsICancelableRunnable> usage in header file.

Comment on attachment 8494418 [details] [diff] [review] bug.patch v3 Review of attachment 8494418 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/omx/MediaOmxReader.cpp @@ +50,5 @@ > mOmxReader->ProcessCachedData(mOffset, false); > } > > private: > + MediaOmxReader* mOmxReader; This should be an nsRefPtr to avoid mOmxReader blows up during execution. @@ +69,5 @@ > // a task to the IO thread for retrieving the next chunk of data, and > // the IO task dispatches a runnable to the main thread for parsing the > // data. This goes on until all of the MP3 file has been parsed. > > +class OmxReaderNotifyDataArrivedRunnable : public nsICancelableRunnable We have nsCancelableRunnable. @@ +132,4 @@ > } > } > > + MediaOmxReader* mOmxReader; Use nsRefPtr here for Cancel() will release |mOmxReader|. @@ +133,5 @@ > } > > + MediaOmxReader* mOmxReader; > + // Protect mOmxReader flag, may access by decoder thread or main thread > + Mutex mMutex; It is easy to cause dead lock when using 2 locks (one from OmxReaderNotifyDataArrivedRunnable and the other from MediaOmxReader). Lock order must be carefully taken. It is better to share the same lock between OmxReaderNotifyDataArrivedRunnable and MediaOmxReader to avoid the trouble.

>>We have nsCancelableRunnable. But I have to add some data members on that class, Directly overwrite the Run() or Cancel() function can't meet our requirement.

(In reply to Randy Lin [:rlin] from comment #21) > >>We have nsCancelableRunnable. > But I have to add some data members on that class, > Directly overwrite the Run() or Cancel() function can't meet our requirement. Can't you subclass nsCancelableRunnable and add the members you need?

Fix nits, I also let OmxReaderNotifyDataArrivedRunnable::Cancel() running in main thread to remove mutex usage in this class.

Comment on attachment 8495782 [details] [diff] [review] bug.patch v4 Review of attachment 8495782 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/omx/MediaOmxReader.cpp @@ +102,5 @@ > return NS_OK; > } > > private: > void NotifyDataArrived() Since Cancel() happens on the main thread as this function, you can exit this function immediately if |mOmxReader| is null. @@ +176,5 @@ > } > > +void MediaOmxReader::CancelProcessCachedData() > +{ > + if (mOmxReaderNotifyDataArrivedRunnable) { You need to lock when accessing |mOmxReaderNotifyDataArrivedRunnable|. @@ +186,5 @@ > + MutexAutoLock lock(mMutex); > + mIsShutdown = true; > + nsCOMPtr<nsIRunnable> cancelEvent = > + NS_NewRunnableMethod(this, &MediaOmxReader::CancelProcessCachedData); > + NS_DispatchToMainThread(cancelEvent); Since OmxReaderProcessCachedDataTask also holds a strong reference to MediaOmxReader, we should also cancel it to ensure the life cycle of MediaOmxReader is not extended and destroyed in an unexpected thread. However, if it is fine to delete MediaOmxReader in an arbitrary thread, we don't need to cancel the runnables. We can just exit NotifyDataArrived() when mIsShutdown is true and this approach is much simpler. http://hg.mozilla.org/mozilla-central/annotate/98a9383dd188/content/media/omx/OmxDecoder.cpp#l83 Refer to the previous revision. OmxDecoderProcessCachedDataTask did make sure OmxDecoder is deleted on the main thread. I think we should also ensure MediaOmxReader is deleted on the main thread.

The MediaOmxReader is threadsafe reference counting class. On this patch, I create a mIsShutdown flag and let it changed on main thread. So that it only need one lock on the MediaOmxReader::ProcessCachedData. (IO/ decoder thread)

Comment on attachment 8497401 [details] [diff] [review] patch v5 Review of attachment 8497401 [details] [diff] [review]: ----------------------------------------------------------------- Overall looks good to me. Since OmxReaderProcessCachedDataTask holds a strong reference to the MediaOmxReader, is it ok for MediaOmxReader to be deleted in the IO thread? ::: content/media/omx/MediaOmxReader.cpp @@ +96,4 @@ > } > > private: > void NotifyDataArrived() Since this function runs in the main thread, you can return immediately if |mOmxReader->IsShutdown()| is true. There is no way for |mOmxReader->IsShutdown()| to become true in the middle of the while loop.

Thanks JW, This patch modified the nits on NotifyDataArrived() function. As my test result by switching the song quickly, I didn't found any crashes.

Hi Edwin, Do you have time to review that? thanks.

Comment on attachment 8497922 [details] [diff] [review] patch v5.1 Review of attachment 8497922 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/omx/MediaOmxReader.h @@ +29,5 @@ > class AbstractMediaDecoder; > > class MediaOmxReader : public MediaOmxCommonReader > { > + Mutex mMutex; Add a comment here about which fields this mutex protects. Consider putting them next to each other. @@ +92,5 @@ > virtual void SetIdle() MOZ_OVERRIDE; > > virtual void Shutdown() MOZ_OVERRIDE; > > + bool IsShutdown() { Take the mutex here instead of expecting callers to do so.

Fix nits, carry reviewer, try result https://tbpl.mozilla.org/?tree=Try&rev=1faecf6fd012

Issue verified as fixed on Flame 2.2. Actual Result: Having more than one song saved to the SD card, and pressing the fast forward and rewind buttons repeatedly no longer incurs a crash. Device: Flame 2.2 (319mb)(Kitkat Base)(Shallow Flash) Build ID: 20141121040204 Gaia: 25388c6bce932657ebf93adedf31881bfaf88c15 Gecko: 3366c0fcf9c2 Version: 36.0a1 (2.2) Firmware Version: v188-1 User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0