MediaRecorder should raise error when MutableBlobStorage::Append fails

RESOLVED FIXED in Firefox 58

Status

()

enhancement
P2
normal
Rank:
18
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: pehrsons, Assigned: pehrsons)

Tracking

58 Branch
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(2 attachments)

Assignee

Description

2 years ago
Bug 1412894 makes MutableBlobStorage::Append return an error if it failed writing  to a file. We do currently not handle this error.
Assignee

Updated

2 years ago
Rank: 18
Priority: -- → P2
Comment hidden (mozreview-request)

Comment 4

2 years ago
mozreview-review
Comment on attachment 8924468 [details]
Bug 1413116 - Handle errors from MutableBlobStorage::Append.

https://reviewboard.mozilla.org/r/195744/#review200878

::: dom/media/MediaRecorder.cpp:284
(Diff revision 1)
>        mSession->MaybeCreateMutableBlobStorage();
>        for (uint32_t i = 0; i < mBuffer.Length(); i++) {
>          if (!mBuffer[i].IsEmpty()) {
> -          mSession->mMutableBlobStorage->Append(mBuffer[i].Elements(),
> +          nsresult rv = mSession->mMutableBlobStorage->Append(mBuffer[i].Elements(),
> -                                                mBuffer[i].Length());
> +                                                              mBuffer[i].Length());
> +          if (NS_FAILED(rv)) {

NS_WARN_IF(NS_FAILED(rv))

::: dom/media/MediaRecorder.cpp:285
(Diff revision 1)
>        for (uint32_t i = 0; i < mBuffer.Length(); i++) {
>          if (!mBuffer[i].IsEmpty()) {
> -          mSession->mMutableBlobStorage->Append(mBuffer[i].Elements(),
> +          nsresult rv = mSession->mMutableBlobStorage->Append(mBuffer[i].Elements(),
> -                                                mBuffer[i].Length());
> +                                                              mBuffer[i].Length());
> +          if (NS_FAILED(rv)) {
> +            mSession->DoSessionEndTask(rv);

Is DoSessionEndTask able to deal with multiple calls?
Attachment #8924468 - Flags: review?(amarchesini) → review+
Assignee

Comment 5

2 years ago
mozreview-review-reply
Comment on attachment 8924468 [details]
Bug 1413116 - Handle errors from MutableBlobStorage::Append.

https://reviewboard.mozilla.org/r/195744/#review200878

> Is DoSessionEndTask able to deal with multiple calls?

It is with the other patch. That said it probably makes more sense to break the loop there and drop remaining buffers.

Comment 6

2 years ago
mozreview-review
Comment on attachment 8924468 [details]
Bug 1413116 - Handle errors from MutableBlobStorage::Append.

https://reviewboard.mozilla.org/r/195744/#review201942
Attachment #8924468 - Flags: review?(bvandyk) → review+

Comment 7

2 years ago
mozreview-review
Comment on attachment 8924469 [details]
Bug 1413116 - Consolidate MediaRecorder::Session state and avoid "start" and "error" races.

https://reviewboard.mozilla.org/r/195746/#review201950

::: dom/media/MediaRecorder.cpp:1202
(Diff revision 1)
> -  // Indicate the session had fire start event. Encoding thread only.
> +  // or Stop().
> -  bool mIsStartEventFired;
> -  // False if the InitEncoder called successfully, ensure the
> -  // ExtractRunnable/DestroyRunnable will end the session.
>    // Main thread only.
> -  bool mNeedSessionEndTask;
> +  Result<RunningState, nsresult> mRunningState;

What do you think about using an enum to track stopped? Seeing ERR(NS_OK) threw me a little when I first read it.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 10

2 years ago
mozreview-review
Comment on attachment 8924469 [details]
Bug 1413116 - Consolidate MediaRecorder::Session state and avoid "start" and "error" races.

https://reviewboard.mozilla.org/r/195746/#review203366
Attachment #8924469 - Flags: review?(bvandyk) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 13

2 years ago
Pushed by pehrsons@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/aec9d75a9a1b
Handle errors from MutableBlobStorage::Append. r=baku,SingingTree
https://hg.mozilla.org/integration/autoland/rev/1935e4699d80
Consolidate MediaRecorder::Session state and avoid "start" and "error" races. r=SingingTree

Comment 14

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/aec9d75a9a1b
https://hg.mozilla.org/mozilla-central/rev/1935e4699d80
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.