Crash in mozilla::MozPromise<T>::Private::Resolve<T>

RESOLVED FIXED in Firefox 54

Status

()

defect
P1
critical
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jwwang, Assigned: jwwang)

Tracking

({crash})

unspecified
mozilla54
Unspecified
Windows 8
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 unaffected, firefox53 unaffected, firefox54 fixed)

Details

(crash signature)

Attachments

(1 attachment)

This bug was filed from the Socorro interface and is 
report bp-854564eb-c37f-4b81-98f2-039252170221.
=============================================================
Assignee: nobody → jwwang
Blocks: 1319987
Priority: -- → P1
Attachment #8839349 - Flags: review?(jyavenard)
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)
(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.
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 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)
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().
(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
(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.
(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....
Duplicate of this bug: 1340956
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+
Thanks!
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f901f32da9e6
refactor ProcessOutput() to remove mDecodePromise and mDrainPromise. r=jya
https://hg.mozilla.org/mozilla-central/rev/f901f32da9e6
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
This still seems to be crashing.
It is ranked #18 in the Windows crashes for Nightly 20170223030204.
Flags: needinfo?(jwwang)
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)
You need to log in before you can comment on or make changes to this bug.