Closed
Bug 1341210
Opened 7 years ago
Closed 7 years ago
Crash in mozilla::MozPromise<T>::Private::Resolve<T>
Categories
(Core :: Audio/Video: Playback, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox52 | --- | unaffected |
firefox53 | --- | unaffected |
firefox54 | --- | fixed |
People
(Reporter: jwwang, Assigned: jwwang)
References
Details
(Keywords: crash)
Crash Data
Attachments
(1 file)
This bug was filed from the Socorro interface and is report bp-854564eb-c37f-4b81-98f2-039252170221. =============================================================
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Comment 1•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/annotate/c3cbadc5d2fa/dom/media/platforms/wmf/WMFMediaDataDecoder.cpp#l160 mDrainPromise might be empty and crash on dereferencing a null pointer.
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8839349 -
Flags: review?(jyavenard)
Comment 3•7 years ago
|
||
mozreview-review |
Comment on attachment 8839349 [details] Bug 1341210 - refactor ProcessOutput() to remove mDecodePromise and mDrainPromise. https://reviewboard.mozilla.org/r/114022/#review115536 ::: dom/media/platforms/wmf/WMFMediaDataDecoder.cpp:160 (Diff revision 1) > mDrainStatus = DrainStatus::DRAINED; > } > if (!mDecodePromise.IsEmpty()) { > mDecodePromise.Resolve(Move(results), __func__); > } else { > - mDrainPromise.Resolve(Move(results), __func__); > + mDrainPromise.ResolveIfExists(Move(results), __func__); this can never happen. We either have a decode promise pending or a drain promise. Flush can no longer interrupt a decode as it's just a task being dispatched on the taskqueue. As such, when you enter ProcessOutput, you always have either a decode promise or a drain promise. There's no alternative.
Attachment #8839349 -
Flags: review?(jyavenard)
Comment 4•7 years ago
|
||
(In reply to JW Wang [:jwwang] [:jw_wang] from comment #1) > https://hg.mozilla.org/mozilla-central/annotate/c3cbadc5d2fa/dom/media/ > platforms/wmf/WMFMediaDataDecoder.cpp#l160 > mDrainPromise might be empty and crash on dereferencing a null pointer. I don't see how that can be the case. ProcessOutput is called either there: https://dxr.mozilla.org/mozilla-central/source/dom/media/platforms/wmf/WMFMediaDataDecoder.cpp#133 or there: https://dxr.mozilla.org/mozilla-central/source/dom/media/platforms/wmf/WMFMediaDataDecoder.cpp#213 So either mDecodePromise exists or mDrainPromise exists. ProcessOutput is called asynchronously from either ProcessDecode or ProcessDrain. It can't be interrupted part-way.
Comment 5•7 years ago
|
||
seems to me that only alternative is that MF_E_NOTACCEPTING has been returned, but that code path is broken anyway and the sample would be lost. that needs fixing. But the solution proposed here isn't it.
Comment hidden (mozreview-request) |
Comment 7•7 years ago
|
||
mozreview-review |
Comment on attachment 8839349 [details] Bug 1341210 - refactor ProcessOutput() to remove mDecodePromise and mDrainPromise. https://reviewboard.mozilla.org/r/114022/#review115848 ::: dom/media/platforms/wmf/WMFMediaDataDecoder.cpp:113 (Diff revision 2) > > RefPtr<MediaDataDecoder::DecodePromise> > WMFMediaDataDecoder::ProcessDecode(MediaRawData* aSample) > { > + RefPtr<DecodePromise> p = mDecodePromise.Ensure(__func__); > + This isnt going to be sufficient, and you will get into the assert whenever MF_E_NOTACCEPTING is returned (btw, i have never seen it ever being returned before, so this is all very weird) The first run of ProcessOutput will resolve the promise. and now the promise holder is empty. on the second run of ProcessOutput the assert will be triggered as mDecodePromise.IsEmpty() now returns true
Attachment #8839349 -
Flags: review?(jyavenard)
Assignee | ||
Comment 8•7 years ago
|
||
It should be sufficient to add // Finish this decode run if ProcessOutput() has produced some outputs. if (mDecodePromise.IsEmpty()) { return p; } in between |if (hr == MF_E_NOTACCEPTING) {| and |if (FAILED(hr)) {| inside ProcessDecode().
Comment hidden (mozreview-request) |
Comment 10•7 years ago
|
||
(In reply to JW Wang [:jwwang] [:jw_wang] from comment #8) > It should be sufficient to add > > // Finish this decode run if ProcessOutput() has produced some outputs. > if (mDecodePromise.IsEmpty()) { > return p; > } I don't think so. Because coming out of ProcessOutput, the promise will always have been resolved. ProcessOutput loops until there's no more to process and return either an error or input exhausted. > > in between > |if (hr == MF_E_NOTACCEPTING) {| > and > |if (FAILED(hr)) {| > inside ProcessDecode(). I think the only approach is to start storing the decoded samples in a class member and resolve the promise outside of ProcessOutput: either in ProcessDrain or ProcessDecode With ProcessOutput now returning an HRRESULT
Assignee | ||
Comment 11•7 years ago
|
||
(In reply to Jean-Yves Avenard [:jya] from comment #10) > I don't think so. Because coming out of ProcessOutput, the promise will > always have been resolved. > ProcessOutput loops until there's no more to process and return either an > error or input exhausted. The first ProcessOutput() is called when hr is MF_E_NOTACCEPTING. When coming out of ProcessOutput() we surely have mDecodePromise resolved. So we will enter |if (mDecodePromise.IsEmpty()) {| and return. If hr is not MF_E_NOTACCEPTING, we will continue the call flow as before. Either way, we exit ProcessDecode() with mDecodePromise resolved or rejected.
Comment 12•7 years ago
|
||
(In reply to JW Wang [:jwwang] [:jw_wang] from comment #11) > (In reply to Jean-Yves Avenard [:jya] from comment #10) > > I don't think so. Because coming out of ProcessOutput, the promise will > > always have been resolved. > > ProcessOutput loops until there's no more to process and return either an > > error or input exhausted. > The first ProcessOutput() is called when hr is MF_E_NOTACCEPTING. When > coming out of ProcessOutput() we surely have mDecodePromise resolved. So we > will enter |if (mDecodePromise.IsEmpty()) {| and return. > > If hr is not MF_E_NOTACCEPTING, we will continue the call flow as before. > Either way, we exit ProcessDecode() with mDecodePromise resolved or rejected. sure, but then you input the new data without process the output. As such, the next call to Decode will also return MF_E_NOTACCEPTING and you're now always going to be one sample late....
Comment hidden (mozreview-request) |
Comment 15•7 years ago
|
||
mozreview-review |
Comment on attachment 8839349 [details] Bug 1341210 - refactor ProcessOutput() to remove mDecodePromise and mDrainPromise. https://reviewboard.mozilla.org/r/114022/#review116314
Attachment #8839349 -
Flags: review?(jyavenard) → review+
Assignee | ||
Comment 16•7 years ago
|
||
Thanks!
Comment 17•7 years ago
|
||
Pushed by jwwang@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/f901f32da9e6 refactor ProcessOutput() to remove mDecodePromise and mDrainPromise. r=jya
Comment 18•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f901f32da9e6
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment 19•7 years ago
|
||
This still seems to be crashing. It is ranked #18 in the Windows crashes for Nightly 20170223030204.
Flags: needinfo?(jwwang)
Comment 20•7 years ago
|
||
The patch went in at 2017-02-24 02:52:48 PST; it won't be in 20170223030204. 20170223030204 uses https://hg.mozilla.org/mozilla-central/rev/32dcdde1fc64fc39a9065dc4218265dbc727673f branch and doesn't include this change.
Flags: needinfo?(jwwang)
Updated•7 years ago
|
status-firefox52:
--- → unaffected
status-firefox53:
--- → unaffected
You need to log in
before you can comment on or make changes to this bug.
Description
•