1505121 - Disable media decoders when recording/replaying

Status: RESOLVED FIXED

(Core Graveyard :: Web Replay, defect)

Description

[Brian Hackett [Laid off!]]

Attachment: patch

Bug 1304146 disables media elements when recording/replaying, but there are still times when media data is decoded in recording/replaying processes, which can lead to crashes.  The attached patch disables these decoders when recording/replaying.

Comment 1

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

Comment on attachment 9023056 [details] [diff] [review]
patch

Review of attachment 9023056 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with nit and more explicit messaging.

::: dom/media/MediaFormatReader.cpp
@@ +593,5 @@
>    MediaResult result = MediaResult(
>      NS_ERROR_DOM_MEDIA_FATAL_ERR,
>      nsPrintfCString("error creating %s decoder", TrackTypeToStr(aData.mTrack)));
>  
> +  // Media playback is not supported when recording or replaying. See bug 1304146,.

Nit: comma can be removed

@@ +595,5 @@
>      nsPrintfCString("error creating %s decoder", TrackTypeToStr(aData.mTrack)));
>  
> +  // Media playback is not supported when recording or replaying. See bug 1304146,.
> +  if (recordreplay::IsRecordingOrReplaying()) {
> +    return result;

I think there's value to returning a more explicit result to make it clear we've hit this case. While I would think devs would generally know if they're in recording or replay, it doesn't hurt to be clear in case they're unaware of this impact or if this feature is mistakenly enabled.

We could both log and update the error (the log could serve to replace above the comment if desired). Something like:
`LOG("Media playback is not supported while recording or replaying -- see bug 1304146. Not creating decoder");`
`return MediaResult(NS_ERROR_DOM_MEDIA_FATAL_ERR, nsPrintfCString("error creating %s decoder: media playback is disabled while recording/replaying", TrackTypeToStr(aData.mTrack)));`

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/969b0caedb4d
Disable media decoders when recording/replaying, r=bryce.

Comment 3

[Brian Hackett [Laid off!]]

(In reply to Bryce Seager van Dyk (:bryce) from comment #1)
> Comment on attachment 9023056 [details] [diff] [review]
> patch
> 
> Review of attachment 9023056 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with nit and more explicit messaging.
> 
> ::: dom/media/MediaFormatReader.cpp
> @@ +593,5 @@
> >    MediaResult result = MediaResult(
> >      NS_ERROR_DOM_MEDIA_FATAL_ERR,
> >      nsPrintfCString("error creating %s decoder", TrackTypeToStr(aData.mTrack)));
> >  
> > +  // Media playback is not supported when recording or replaying. See bug 1304146,.
> 
> Nit: comma can be removed
> 
> @@ +595,5 @@
> >      nsPrintfCString("error creating %s decoder", TrackTypeToStr(aData.mTrack)));
> >  
> > +  // Media playback is not supported when recording or replaying. See bug 1304146,.
> > +  if (recordreplay::IsRecordingOrReplaying()) {
> > +    return result;
> 
> I think there's value to returning a more explicit result to make it clear
> we've hit this case. While I would think devs would generally know if
> they're in recording or replay, it doesn't hurt to be clear in case they're
> unaware of this impact or if this feature is mistakenly enabled.
> 
> We could both log and update the error (the log could serve to replace above
> the comment if desired). Something like:
> `LOG("Media playback is not supported while recording or replaying -- see
> bug 1304146. Not creating decoder");`
> `return MediaResult(NS_ERROR_DOM_MEDIA_FATAL_ERR, nsPrintfCString("error
> creating %s decoder: media playback is disabled while recording/replaying",
> TrackTypeToStr(aData.mTrack)));`

Thanks, I included the more detailed MediaResult, but calling LOG() here doesn't work as the macro doesn't compile when called in this function (LOG's expansion uses 'this' on a templated function that doesn't instantiate for MediaFormatReader::DecoderFactory).

Comment 4

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

Ah, makes sense. I'd overlooked it is a this flavour of log macro for MediaFormatReader.

Comment 5

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/969b0caedb4d