[webkit-reviews] review denied: [Bug 190778] MediaRecorder should fire dataavailable event when all tracks are ended and stop() is called : [Attachment 353327] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has denied Wendy <yuhan_wu at apple.com>'s request
for review:
Bug 190778: MediaRecorder should fire dataavailable event when all tracks are
ended and stop() is called
https://bugs.webkit.org/show_bug.cgi?id=190778

Attachment 353327: Patch

https://bugs.webkit.org/attachment.cgi?id=353327&action=review




--- Comment #37 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 353327
  --> https://bugs.webkit.org/attachment.cgi?id=353327
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=353327&action=review

I did not look too closely at the tests, will let Youenn do a full pass.
Several of my earlier comments have not been taken into account though.

> Source/WebCore/Modules/mediarecorder/BlobEvent.cpp:37
> +    auto blob = init.data.releaseNonNull();

Not needed as commented several iterations ago.

> Source/WebCore/Modules/mediarecorder/BlobEvent.cpp:47
> +    : Event(type, WTFMove(init), isTrusted)

As commented earlier, you don't need to WTFMove() init here. Then you don't
need to separate Blob parameter.

> Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:53
> +    m_tracks = WTF::map(m_stream->getTracks(), [] (const auto& track) ->
Ref<MediaStreamTrackPrivate> {

I don't think you need the "const", we like to omit it in WebKit as much as
possible. auto& track should work fine here.

> Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:54
>	   auto& privateTrack = track->privateTrack();

Unnecessary local variable, just return track->privateTrack();

> Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:117
> +	   dispatchEvent(BlobEvent::create(eventNames().dataavailableEvent,
Event::CanBubble::No, Event::IsCancelable::No, m_private->fetchData()));

This call is safe because scheduleDeferredTask() takes care of capturing a
WeakPtr and makes sure not to call this lambda if |this| has been destroyed.

> Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:118
> +	   if (!m_isActive)

However, there is a potential use-after-free here since nothing keeps you
alive. The call to dispatchEvent() will run javascript and this script may
destroy the MediaRecorder. What I would do is:
1. capture `weakThis = makeWeakPtr(*this)` in your lambda
2. `if (!weakThis || !m_isActive) return` here.

> Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:148
> +	   if (!m_isActive)

Potential use-after-free here.

> Source/WebCore/Modules/mediarecorder/MediaRecorder.h:66
>      ExceptionOr<void> start(std::optional<int>);

nit: Could we call this one startRecording() for consistency with the new one?

> Source/WebCore/platform/mediarecorder/MediaRecorderPrivate.h:52
> +protected:

I think it is considered bad practice to have protected data members in an
interface. You may want to make the data member private and have a protected
getter instead. Maybe even better, instead of a mere getter, it could be:
Locker<Lock> takeLock()
{
    return holdLock(m_bufferLock);
}

> Source/WebCore/platform/mediarecorder/MediaRecorderPrivateMock.cpp:36
> +    m_buffer.append(makeString("Video Track ID: ", track.id(), " Counter: ",
String::number(++m_counter), "\r\n---------\r\n"));

No need for String::number() here, just include
<wtf/text/StringConcatenateNumbers.h>.

> Source/WebCore/platform/mediarecorder/MediaRecorderPrivateMock.cpp:42
> +    m_buffer.append(makeString("Audio Track ID: ", track.id(), " Counter: ",
String::number(++m_counter), "\r\n---------\r\n"));

ditto.

> Source/WebCore/platform/mediarecorder/MediaRecorderPrivateMock.h:36
> +    MediaRecorderPrivateMock() { }

MediaRecorderPrivateMock() = default;

>
LayoutTests/imported/w3c/web-platform-tests/mediacapture-record/MediaRecorder-d
estroy-script-execution.html:12
> +	       document.body.removeChild(document.getElementById('subFrame'));

FYI, this is much shorter and should work just as well:
subFrame.remove();