Bug 213364 – [GStreamer] gst_buffer_unmap: assertion 'GST_IS_BUFFER (buffer)' failed

• Component: Media
• Status: RESOLVED FIXED

[Diego Pino]

imported/w3c/web-platform-tests/encrypted-media/clearkey-mp4-setmediakeys-to-multiple-video-elements.https.html [ Crash ]

The test is a flaky crash. First crash happened at:

r262780                       NOERROR
[r262781-r262784]             UNKNOWN
r262785                       CRASH (Expected: PASS)

Crash-log: https://build.webkit.org/results/GTK%20Linux%2064-bit%20Release%20(Tests)/r263245%20(14146)/imported/w3c/web-platform-tests/encrypted-media/clearkey-mp4-setmediakeys-to-multiple-video-elements.https-crash-log.txt

Thread 1 (Thread 0x7ff6da95a2c0 (LWP 32470)):
#0  0x00007ff6dcba0ee5 in _g_log_abort (breakpoint=1) at ../glib/gmessages.c:554
#1  0x00007ff6dcba21c9 in g_logv (log_domain=0x7ff6dd51f5e0 <g_log_domain_gstreamer> "GStreamer", log_level=G_LOG_LEVEL_CRITICAL, format=<optimized out>, args=args@entry=0x7ffc881d4090) at ../glib/gmessages.c:1373
#2  0x00007ff6dcba2393 in g_log (log_domain=<optimized out>, log_level=<optimized out>, format=<optimized out>) at ../glib/gmessages.c:1415
#3  0x00007ff6e34feb67 in void WTF::__destroy_op_table<WTF::Variant<WTF::Vector<char, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>, std::unique_ptr<SoupBuffer, WTF::GPtrDeleter<SoupBuffer> >, WTF::GRefPtr<_GBytes>, WTF::RefPtr<WebCore::GstMappedBuffer, WTF::DumbPtrTraits<WebCore::GstMappedBuffer> >, WTF::FileSystemImpl::MappedFileData>, WTF::__index_sequence<0l, 1l, 2l, 3l, 4l> >::__destroy_func<3l>(WTF::Variant<WTF::Vector<char, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>, std::unique_ptr<SoupBuffer, WTF::GPtrDeleter<SoupBuffer> >, WTF::GRefPtr<_GBytes>, WTF::RefPtr<WebCore::GstMappedBuffer, WTF::DumbPtrTraits<WebCore::GstMappedBuffer> >, WTF::FileSystemImpl::MappedFileData>*) () at /app/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#4  0x00007ff6e5a10161 in WTF::Detail::CallableWrapper<WebCore::MediaPlayerPrivateGStreamer::initializationDataEncountered(WebCore::InitData&&)::{lambda()#1}, void>::~CallableWrapper() () at /app/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#5  0x00007ff6e1025dc2 in WTF::RunLoop::performWork() () at /app/webkit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18
#6  0x00007ff6e108abc9 in WTF::RunLoop::RunLoop()::{lambda(void*)#1}::_FUN(void*) () at /app/webkit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18
#7  0x00007ff6dcb9ac3e in g_main_dispatch (context=0x55b8f7078150) at ../glib/gmain.c:3309
#8  0x00007ff6dcb9ac3e in g_main_context_dispatch (context=context@entry=0x55b8f7078150) at ../glib/gmain.c:3974
#9  0x00007ff6dcb9aff0 in g_main_context_iterate (context=0x55b8f7078150, block=block@entry=1, dispatch=dispatch@entry=1, self=<optimized out>) at ../glib/gmain.c:4047
#10 0x00007ff6dcb9b2e3 in g_main_loop_run (loop=0x55b8f70a5bd0) at ../glib/gmain.c:4241
#11 0x00007ff6e108b6d0 in WTF::RunLoop::run() () at /app/webkit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18
#12 0x00007ff6e3d1314f in WebKit::WebProcessMain(int, char**) () at /app/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#13 0x00007ff6db7ce183 in __libc_start_main (main=0x55b8f6050c00 <main>, argc=4, argv=0x7ffc881d4488, init=<optimized out>, fini=<optimized out>, rtld_fini=<optimized out>, stack_end=0x7ffc881d4478) at ../csu/libc-start.c:308
#14 0x000055b8f6050c8e in _start () at ../sysdeps/x86_64/start.S:120

STDERR: Traceback (most recent call last):
STDERR: File "<string>", line 3, in <module>
STDERR: ModuleNotFoundError: No module named 'webkit'
STDERR: /home/slave/.gdbinit:7: Error in sourced command file:
STDERR: Error while executing Python code.
STDERR:
STDERR: warning: core file may not match specified executable file.
STDERR: ../gst/gstpad.c:4544:gst_pad_push_data:<qtdemux0:audio_0> Got data flow before stream-start event
STDERR: ../gst/gstpad.c:4549:gst_pad_push_data:<qtdemux0:audio_0> Got data flow before segment event
STDERR: ../gst/gstpad.c:4544:gst_pad_push_data:<qtdemux1:video_0> Got data flow before stream-start event
STDERR: ../gst/gstpad.c:4549:gst_pad_push_data:<qtdemux1:video_0> Got data flow before segment event
STDERR:
STDERR: (WebKitWebProcess:32470): GStreamer-CRITICAL **: 17:28:50.976: gst_buffer_unmap: assertion 'GST_IS_BUFFER (buffer)' failed

[Xabier Rodríguez Calvar]

Created attachment 404443 [details]
Patch

[Philippe Normand]

Comment on attachment 404443 [details]
Patch

This looks like a workaround for a bug in the InitData class. As the init data is quite small (IIRC), doesn't it pay off to have mapped buffer there? Can't we just copy it? Also I see the append() method assuming the m_payload is mutable, while the createSharedBuffer() has a comment mentioning it should be considered immutable.

[Xabier Rodríguez Calvar]

(In reply to Philippe Normand from comment #2)
> Comment on attachment 404443 [details]
> Patch
> 
> This looks like a workaround for a bug in the InitData class. As the init
> data is quite small (IIRC), doesn't it pay off to have mapped buffer there?
> Can't we just copy it?

We could buy I don't think it is necessary because of I am going to comment below.

> Also I see the append() method assuming the m_payload
> is mutable, while the createSharedBuffer() has a comment mentioning it
> should be considered immutable.

The data inside the SharedBuffer should be immutable, not the SharedBuffer itself. That's why we assert on the mapping to be not WRITE. Actually, appending SharedBuffers as we do is quite efficient as no data is copied, only segments are appended or until it is read if needed.

About keeping the mapped buffer? It is already done, it leaves inside the DataSegment of the SharedBuffer.

GstMappedBuffer needs that he buffer outlives the mapped buffer of you will have the warning you're getting (which is a symptom of a deeper problem and can lead to unwanted reads). In many cases we just create the mapped buffer, use it and let it go. In this case, it is kept and as we're seeing in this case the buffer is not outliving the mapped buffer, which creates the problem.

Considering all this, I think reffing the buffer and guaranteeing it lives protected inside the mapped buffer is the right solution.

[Philippe Normand]

(In reply to Xabier Rodríguez Calvar from comment #3)
> (In reply to Philippe Normand from comment #2)
> > Comment on attachment 404443 [details]
> > Patch
> > 
> > This looks like a workaround for a bug in the InitData class. As the init
> > data is quite small (IIRC), doesn't it pay off to have mapped buffer there?
> > Can't we just copy it?
> 
> We could buy I don't think it is necessary because of I am going to comment
> below.
> 
> > Also I see the append() method assuming the m_payload
> > is mutable, while the createSharedBuffer() has a comment mentioning it
> > should be considered immutable.
> 
> The data inside the SharedBuffer should be immutable, not the SharedBuffer
> itself. That's why we assert on the mapping to be not WRITE. Actually,
> appending SharedBuffers as we do is quite efficient as no data is copied,
> only segments are appended or until it is read if needed.
> 
> About keeping the mapped buffer? It is already done, it leaves inside the
> DataSegment of the SharedBuffer.

I meant the GstMappedBuffer which goes out of scope in the InitData constructor.

[Xabier Rodríguez Calvar]

(In reply to Philippe Normand from comment #4)
> (In reply to Xabier Rodríguez Calvar from comment #3)
> > (In reply to Philippe Normand from comment #2)
> > > Comment on attachment 404443 [details]
> > > Patch
> > > 
> > > This looks like a workaround for a bug in the InitData class. As the init
> > > data is quite small (IIRC), doesn't it pay off to have mapped buffer there?
> > > Can't we just copy it?
> > 
> > We could buy I don't think it is necessary because of I am going to comment
> > below.
> > 
> > > Also I see the append() method assuming the m_payload
> > > is mutable, while the createSharedBuffer() has a comment mentioning it
> > > should be considered immutable.
> > 
> > The data inside the SharedBuffer should be immutable, not the SharedBuffer
> > itself. That's why we assert on the mapping to be not WRITE. Actually,
> > appending SharedBuffers as we do is quite efficient as no data is copied,
> > only segments are appended or until it is read if needed.
> > 
> > About keeping the mapped buffer? It is already done, it leaves inside the
> > DataSegment of the SharedBuffer.
> 
> I meant the GstMappedBuffer which goes out of scope in the InitData
> constructor.

Yes, that one. It apparently goes out of scope, but if you follow the SharedBuffer onion, you'll see a & goes down to the DataSegment, it is implicitly becomes a Ref&& and moved into the segment.

[Philippe Normand]

Yeah right, so this is happening because the SharedBuffer doesn't keep a reference of the buffer. It would be nicer to have a wrapper shared buffer mechanism perhaps? We could then increment the buffer ref when creating the shared buffer and then have a destroy-notify callback to unref it.

[Xabier Rodríguez Calvar]

Created attachment 404934 [details]
Patch

[Philippe Normand]

Comment on attachment 404934 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.h:121
> +    uint8_t* data() { ASSERT(m_isValid); return static_cast<uint8_t*>(m_info.data); }

Should this one be moved to GstMappedOwnedBuffer?

[Xabier Rodríguez Calvar]

Created attachment 405035 [details]
Patch

(In reply to Philippe Normand from comment #8)
> Comment on attachment 404934 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=404934&action=review
> 
> > Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.h:121
> > +    uint8_t* data() { ASSERT(m_isValid); return static_cast<uint8_t*>(m_info.data); }
> 
> Should this one be moved to GstMappedOwnedBuffer?

No, owned is only for readonly, if something is for writing is the superclass.

I re-uploaded the patch with some more checks for memory safety.

[EWS]

Committed r264762: <https://trac.webkit.org/changeset/264762>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 405035 [details].

[Radar WebKit Bug Importer]

<rdar://problem/65991836>

[Xabier Rodríguez Calvar]

Problem persists.

[Xabier Rodríguez Calvar]

Created attachment 405134 [details]
Patch

[EWS]

Committed r264816: <https://trac.webkit.org/changeset/264816>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 405134 [details].