Bug 213364

Summary: [GStreamer] gst_buffer_unmap: assertion 'GST_IS_BUFFER (buffer)' failed
Product: WebKit Reporter: Diego Pino <dpino>
Component: MediaAssignee: Xabier Rodríguez Calvar <calvaris>
Status: RESOLVED FIXED    
Severity: Normal CC: calvaris, cgarcia, eric.carlson, ews-watchlist, glenn, gustavo, hta, jer.noble, menard, philipj, pnormand, sergio, tommyw, vjaquez, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
pnormand: review-
Patch
pnormand: review+
Patch
none
Patch none

Description Diego Pino 2020-06-18 20:42:15 PDT
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
Comment 1 Xabier Rodríguez Calvar 2020-07-16 09:04:04 PDT
Created attachment 404443 [details]
Patch
Comment 2 Philippe Normand 2020-07-17 03:27:39 PDT
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.
Comment 3 Xabier Rodríguez Calvar 2020-07-21 00:38:51 PDT
(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.
Comment 4 Philippe Normand 2020-07-21 01:31:45 PDT
(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.
Comment 5 Xabier Rodríguez Calvar 2020-07-21 01:55:39 PDT
(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.
Comment 6 Philippe Normand 2020-07-21 07:28:38 PDT
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.
Comment 7 Xabier Rodríguez Calvar 2020-07-22 09:58:45 PDT
Created attachment 404934 [details]
Patch
Comment 8 Philippe Normand 2020-07-23 02:34:28 PDT
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?
Comment 9 Xabier Rodríguez Calvar 2020-07-23 07:07:39 PDT
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.
Comment 10 EWS 2020-07-23 07:55:24 PDT
Committed r264762: <https://trac.webkit.org/changeset/264762>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 405035 [details].
Comment 11 Radar WebKit Bug Importer 2020-07-23 07:56:15 PDT
<rdar://problem/65991836>
Comment 12 Xabier Rodríguez Calvar 2020-07-24 03:01:29 PDT
Problem persists.
Comment 13 Xabier Rodríguez Calvar 2020-07-24 03:01:55 PDT
Created attachment 405134 [details]
Patch
Comment 14 EWS 2020-07-24 03:43:49 PDT
Committed r264816: <https://trac.webkit.org/changeset/264816>

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