Bug 116686 – [Soup] [Gstreamer] ASSERT in StreamingClient::getOrCreateReadBuffer

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Sergio Villar Senin]

This is the backtrace of the crash visiting the above url.

Program received signal SIGSEGV, Segmentation fault.
0x00007f26cc3787dc in WTFCrash () at ../../Source/WTF/wtf/Assertions.cpp:339
339	    *(int *)(uintptr_t)0xbbadbeef = 0;
(gdb) bt
#0  0x00007f26cc3787dc in WTFCrash () at ../../Source/WTF/wtf/Assertions.cpp:339
#1  0x00007f26c8409364 in StreamingClient::getOrCreateReadBuffer (this=0x1eedf20, requestedSize=8192, actualSize=@0x7fff11795ea8: 139803140448136)
    at ../../Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:931
#2  0x00007f26c847e3d4 in WebCore::ResourceHandle::ensureReadBuffer (this=0x7f2674e0f7b0) at ../../Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:311
#3  0x00007f26c848062d in WebCore::sendRequestCallback (result=0x7f26743dfdc0, data=0x7f2674e0f7b0) at ../../Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:730
#4  0x00007f26c5ef1f6b in g_task_return_now (task=0x7f26743dfdc0) at gtask.c:1105
#5  0x00007f26c5ef271e in g_task_return (task=0x7f26743dfdc0, type=<optimized out>) at gtask.c:1158
#6  g_task_return (task=0x7f26743dfdc0, type=<optimized out>) at gtask.c:1126
#7  0x00007f26c603766c in http_input_stream_ready_cb (source=0x76b040, result=0x7f267443ec50, user_data=0x7f26743dfdc0) at soup-request-http.c:124
#8  0x00007f26c5ef1f6b in g_task_return_now (task=0x7f267443ec50) at gtask.c:1105
#9  0x00007f26c5ef271e in g_task_return (task=0x7f267443ec50, type=<optimized out>) at gtask.c:1158
#10 g_task_return (task=0x7f267443ec50, type=<optimized out>) at gtask.c:1126
#11 0x00007f26c603a332 in async_send_request_return_result (item=0x7f26743c3d20, stream=0xedbbc0, error=<optimized out>) at soup-session.c:3628
#12 0x00007f26c603f80f in send_async_maybe_complete (stream=0xedbbc0, item=0x7f26743c3d20) at soup-session.c:3742
#13 try_run_until_read (item=item@entry=0x7f26743c3d20) at soup-session.c:3766
#14 0x00007f26c603f95d in read_ready_cb (msg=<optimized out>, user_data=0x7f26743c3d20) at soup-session.c:3753
#15 0x00007f26c5d41e25 in g_main_dispatch (context=0x761e90) at gmain.c:3054
#16 g_main_context_dispatch (context=context@entry=0x761e90) at gmain.c:3630
#17 0x00007f26c5d42168 in g_main_context_iterate (context=0x761e90, block=block@entry=1, dispatch=dispatch@entry=1, 
    self=<error reading variable: Unhandled dwarf expression opcode 0xfa>) at gmain.c:3701
#18 0x00007f26c5d425da in g_main_loop_run (loop=0x75e190) at gmain.c:3895
#19 0x00007f26c898b50a in WebCore::RunLoop::run () at ../../Source/WebCore/platform/gtk/RunLoopGtk.cpp:61
#20 0x00007f26c72533fc in WebKit::WebProcessMainGtk (argc=2, argv=0x7fff117963d8) at ../../Source/WebKit2/WebProcess/gtk/WebProcessMainGtk.cpp:78
#21 0x0000000000400a1c in main (argc=2, argv=0x7fff117963d8) at ../../Source/WebKit2/gtk/MainGtk.cpp:31

[Alberto Garcia]

Reproduced, I'll take a look.

[Alberto Garcia]

I actually don't understand why that assert is there, either it is
wrong or the name of the method is misleading, because it's creating a
new buffer every time (and we're most likely leaking memory).

This is called from ResourceHandle::ensureReadBuffer(), which is used
by several callbacks in ResourceHandle.

This comes from bug 115364 and bug 105552.

[Carlos Garcia Campos]

(In reply to comment #2)
> I actually don't understand why that assert is there, either it is
> wrong or the name of the method is misleading, because it's creating a
> new buffer every time (and we're most likely leaking memory).

Memory is not leaking because buffer is a GRefPtr. 

> This is called from ResourceHandle::ensureReadBuffer(), which is used
> by several callbacks in ResourceHandle.
> 
> This comes from bug 115364 and bug 105552.

Patch in bug 115364 doesn't change the logic of the gst buffer, it simply renames the method and moves the default buffer implementation from the cross platform code to the soup resource handle implementation

[Alberto Garcia]

Created attachment 203367 [details]
Patch

(In reply to comment #3)
> Memory is not leaking because buffer is a GRefPtr.

Yes, you're right.

This patch removes the assertion and returns the buffer if it
exists. I don't see any other crash or assertion being triggered and
everytime a new buffer is created it seems to be cleared correctly
afterwards.

Then we have the thing that the requestedSize parameter is completely
ignored if the buffer already exists, although in practice this is not
happening since we're using a constant value that is defined in
ResourceHandle::ensureReadBuffer()

[Alberto Garcia]

I think the original code is from Gustavo, adding him to Cc.

[Carlos Garcia Campos]

Comment on attachment 203367 [details]
Patch

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

I wonder if we could reuse the buffer also for the case of not using the soup resource handle.

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:936
> +        mapGstBuffer(buffer);

I don't know what map/unmap means in GST but we are unmapping the buffer for every data chunk in didReceiveData.

[Carlos Garcia Campos]

I had missed the leakPtr in didReceiveData, so we are in the end creating and mapping the buffer before every data chunk, and releasing it after.

[kov's GTK+ EWS bot]

Comment on attachment 203367 [details]
Patch

Attachment 203367 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/689307

[Manuel Rego Casasnovas]

I've just tested manually the patch in gtk-wk2 and it worked now, so it should be some temporal issue.

[Alberto Garcia]

sendRequestCallback() is called twice, the first one is a redirection
(there's an ensureReadBuffer() there). The second one is the call at
the end of the function.

[Carlos Garcia Campos]

It seems that the assert was actually correct and tried to detect cases where the sequence getOrCreateBuffer -> didReceivedata doesn't happen. This currently happens in case of redirection because we changed the skip_async by read_async, but we don't really read anything and then didReceiveData is not called. So the fix should be either use skip_async now that we depend on recent glib, or use a different buffer in this particular case, a skipBuffer or something, so that we don't allocate a gst buffer for nothing.

[Alberto Garcia]

Created attachment 203427 [details]
Patch

Using skip_async() to handle the redirection fixes the problem.

[Carlos Garcia Campos]

Comment on attachment 203427 [details]
Patch

Thanks!

[WebKit Commit Bot]

Comment on attachment 203427 [details]
Patch

Clearing flags on attachment: 203427

Committed r151013: <http://trac.webkit.org/changeset/151013>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.