1413116 - MediaRecorder should raise error when MutableBlobStorage::Append fails

Status: RESOLVED FIXED

(Core :: Audio/Video: Recording, enhancement, P2)

Description

[Andreas Pehrson [:pehrsons]]

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

Comment 1

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1413116 - Handle errors from MutableBlobStorage::Append.

Review commit: https://reviewboard.mozilla.org/r/195744/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/195744/

Comment 2

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1413116 - Consolidate MediaRecorder::Session state and avoid "start" and "error" races.

Review commit: https://reviewboard.mozilla.org/r/195746/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/195746/

Comment 3

[Andreas Pehrson [:pehrsons]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=629d8da608ccca9648964a92526afb2a0b3eb69e

Comment 4

[Andrea Marchesini [:baku]]

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?

Comment 5

[Andreas Pehrson [:pehrsons]]

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

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Comment on attachment 8924468 [details]
Bug 1413116 - Handle errors from MutableBlobStorage::Append.

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

Comment 7

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

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 8

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8924468 [details]
Bug 1413116 - Handle errors from MutableBlobStorage::Append.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195744/diff/1-2/

Comment 9

[Andreas Pehrson [:pehrsons]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195746/diff/1-2/

Comment 10

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

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

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

Comment 11

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8924468 [details]
Bug 1413116 - Handle errors from MutableBlobStorage::Append.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195744/diff/2-3/

Comment 12

[Andreas Pehrson [:pehrsons]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195746/diff/2-3/

Comment 13

[Pulsebot]

Pushed by [e-mail]:
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

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/aec9d75a9a1b
https://hg.mozilla.org/mozilla-central/rev/1935e4699d80