Bug 223236 – [SOUP] SOUP3 crashes inside soup_message_set_request_body

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Lauro Moura]

Created attachment 423293 [details]
http/tests/xmlhttprequest/upload-onloadstart-event.html crash log

http/tests/xmlhttprequest/loadstart-event-init.html
http/tests/xmlhttprequest/redirect-cross-origin-post-sync.html
http/tests/xmlhttprequest/redirect-cross-origin-post.html
http/tests/xmlhttprequest/response-access-on-error.html
http/tests/xmlhttprequest/upload-onabort-progressevent-attributes.html
http/tests/xmlhttprequest/upload-onload-event.html
http/tests/xmlhttprequest/upload-onload-progressevent-attributes.html
http/tests/xmlhttprequest/upload-onloadend-event-after-abort.html
http/tests/xmlhttprequest/upload-onloadend-event-after-load.html
http/tests/xmlhttprequest/upload-onloadstart-event.html
http/tests/xmlhttprequest/upload-onprogress-event.html
http/tests/xmlhttprequest/upload-progress-events.html
http/tests/xmlhttprequest/xmlhttprequest-sync-no-progress-events.html

These happen to crash locally and in the bots (althought not all might be run, due to early exits).

This is only a partial list. Other tests might be affected.

STDERR:
STDERR: (process:2504): libsoup-CRITICAL **: 20:42:47.243: soup_message_set_request_body: assertion 'stream == NULL || G_IS_POLLABLE_INPUT_STREAM (stream)' failed
STDERR: LEAK: 1 WebPageProxy

Tip of trace for http/tests/xmlhttprequest/upload-onloadstart-event.html (full attached)

Thread 1 (Thread 0x7efdae91e9c0 (LWP 2504)):
#0  g_logv (log_domain=0x7efdafbb7943 "libsoup", log_level=G_LOG_LEVEL_CRITICAL, format=<optimized out>, args=<optimized out>) at ../glib/gmessages.c:1413
#1  0x00007efdb0eb4973 in g_log (log_domain=<optimized out>, log_level=<optimized out>, format=<optimized out>) at ../glib/gmessages.c:1451
#2  0x00007efdbc2257f6 in WebCore::ResourceRequest::updateSoupMessageBody(_SoupMessage*, WebCore::BlobRegistryImpl&) const (this=0x7efd6c2f3788, soupMessage=0x7efd4c0083b0 [SoupMessage], blobRegistry=...
) at ../../Source/WebCore/platform/network/soup/ResourceRequestSoup.cpp:138
#3  0x00007efdbc22556c in WebCore::ResourceRequest::createSoupMessage(WebCore::BlobRegistryImpl&) const (this=0x7efd6c2f3788, blobRegistry=...) at ../../Source/WebCore/platform/network/soup/ResourceReque
stSoup.cpp:90
#4  0x00007efdb8c06b17 in WebKit::NetworkDataTaskSoup::createRequest(WebCore::ResourceRequest&&, WebKit::NetworkDataTaskSoup::WasBlockingCookies) (this=0x7efd6c2f3580, request=..., wasBlockingCookies=Web
Kit::NetworkDataTaskSoup::WasBlockingCookies::No) at ../../Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:127
#5  0x00007efdb8c065f1 in WebKit::NetworkDataTaskSoup::NetworkDataTaskSoup(WebKit::NetworkSession&, WebKit::NetworkDataTaskClient&, WebCore::ResourceRequest const&, WTF::ObjectIdentifier<WebCore::FrameId
entifierType>, WTF::ObjectIdentifier<WebCore::PageIdentifierType>, WebCore::StoredCredentialsPolicy, WebCore::ContentSniffingPolicy, WebCore::ContentEncodingSniffingPolicy, bool, bool) (this=0x7efd6c2f35
80, session=..., client=..., requestWithCredentials=..., frameID=..., pageID=..., storedCredentialsPolicy=WebCore::StoredCredentialsPolicy::Use, shouldContentSniff=WebCore::ContentSniffingPolicy::DoNotSniffContent, shouldClearReferrerOnHTTPSToHTTPRedirect=true, dataTaskIsForMainFrameNavigation=false) at ../../Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:81
#6  0x00007efdb88c1b57 in WebKit::NetworkDataTaskSoup::create(WebKit::NetworkSession&, WebKit::NetworkDataTaskClient&, WebCore::ResourceRequest const&, WTF::ObjectIdentifier<WebCore::FrameIdentifierType>, WTF::ObjectIdentifier<WebCore::PageIdentifierType>, WebCore::StoredCredentialsPolicy, WebCore::ContentSniffingPolicy, WebCore::ContentEncodingSniffingPolicy, bool, bool) (session=..., client=..., request=..., frameID=..., pageID=..., storedCredentialsPolicy=WebCore::StoredCredentialsPolicy::Use, shouldContentSniff=WebCore::ContentSniffingPolicy::DoNotSniffContent, shouldContentEncodingSniff=WebCore::ContentEncodingSniffingPolicy::DoNotSniff, shouldClearReferrerOnHTTPSToHTTPRedirect=true, dataTaskIsForMainFrameNavigation=false) at ../../Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.h:45
#7  0x00007efdb88acb5d in WebKit::NetworkDataTask::create(WebKit::NetworkSession&, WebKit::NetworkDataTaskClient&, WebKit::NetworkLoadParameters const&) (session=..., client=..., parameters=...) at ../../Source/WebKit/NetworkProcess/NetworkDataTask.cpp:60
#8  0x00007efdb88b015b in WebKit::NetworkLoad::NetworkLoad(WebKit::NetworkLoadClient&, WebCore::BlobRegistryImpl*, WebKit::NetworkLoadParameters&&, WebKit::NetworkSession&) (this=0x7efdae0e1000, client=..., blobRegistry=0x7efdae0d6138, parameters=..., networkSession=...) at ../../Source/WebKit/NetworkProcess/NetworkLoad.cpp:57

[Daniel Kolesa]

caused by: https://gitlab.gnome.org/GNOME/libsoup/-/commit/0a34d642a401c5880ec4f533250fdddc6dbb12b0

[Daniel Kolesa]

the current soup code doesn't really have any requirement for the request body to be pollable, so the soup3 webkit code assumes that it's not necessary, but the upcoming http2 code will require it to be pollable - so the restriction was put in place - so i guess the webkit code will need to be updated

[Daniel Kolesa]

i guess the thing to do here would be to just do the same as the soup2 code - i.e. read all into bytes, then set the request body from bytes rather than from the input stream...

[Carlos Garcia Campos]

(In reply to Daniel Kolesa from comment #3)
> i guess the thing to do here would be to just do the same as the soup2 code
> - i.e. read all into bytes, then set the request body from bytes rather than
> from the input stream...

That's problematic for long file uploads, for example. The WebKit stream can't be pollable because it used GFileInputStream internally that is not pollable. I can make it pollable and only return TRUE from can_poll when not having file streams. But the solution would be to make libsoup work without pollable streams, at least having fallback code that reads using the async apis into an intermediate buffer that is later used to write in the output.

[Carlos Garcia Campos]

Created attachment 423468 [details]
Patch

[Daniel Kolesa]

patch looks good to me, the rest of the fix needs to be in soup (remove the pollable stream requirement and implement a fallback path for when it's not in the http2 backend)

[Adrian Perez]

Comment on attachment 423468 [details]
Patch

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

> Source/WebCore/platform/network/soup/WebKitFormDataInputStream.cpp:137
> +    // GFileInputStream is not pollable, so the stream is only pollable if FormData doesn't contain EncodedFileData elements.

I wonder if it could be an options to check whether a GInputStream is a GUnixInputStream
(which does implement GPollableInputStream) which would make the file elements pollable,
or whether the GInputStream implements the GFileDescriptorBased interface, in which case
we could use the _get_fd() method and use GPollFD or similar 🤔️

(No need to change anything in this patch, by the way, this is just me mulling ideas
over. If you think any of this makes sense for a follow-up patch it would be nice
to write it down as a TODO and/or make a new Bugzilla issue to track it.)

> Source/WebCore/platform/network/soup/WebKitFormDataInputStream.cpp:177
> +    return g_pollable_source_new_full(stream, base.get(), cancellable);

Aren't these two lines equivalent to a plain g_pollable_input_stream_create_source() call?
As in:

   return g_pollable_input_stream_create_source(stream, cancellable);

[Carlos Garcia Campos]

Comment on attachment 423468 [details]
Patch

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

>> Source/WebCore/platform/network/soup/WebKitFormDataInputStream.cpp:137
>> +    // GFileInputStream is not pollable, so the stream is only pollable if FormData doesn't contain EncodedFileData elements.
> 
> I wonder if it could be an options to check whether a GInputStream is a GUnixInputStream
> (which does implement GPollableInputStream) which would make the file elements pollable,
> or whether the GInputStream implements the GFileDescriptorBased interface, in which case
> we could use the _get_fd() method and use GPollFD or similar 🤔️
> 
> (No need to change anything in this patch, by the way, this is just me mulling ideas
> over. If you think any of this makes sense for a follow-up patch it would be nice
> to write it down as a TODO and/or make a new Bugzilla issue to track it.)

We are always reading local files, so it's always GFileInputStream and not pollable.

>> Source/WebCore/platform/network/soup/WebKitFormDataInputStream.cpp:177
>> +    return g_pollable_source_new_full(stream, base.get(), cancellable);
> 
> Aren't these two lines equivalent to a plain g_pollable_input_stream_create_source() call?
> As in:
> 
>    return g_pollable_input_stream_create_source(stream, cancellable);

This is the implementation of g_pollable_input_stream_create_source().

[Adrian Perez]

(In reply to Carlos Garcia Campos from comment #8)
> Comment on attachment 423468 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=423468&action=review
> 
> >> Source/WebCore/platform/network/soup/WebKitFormDataInputStream.cpp:137
> >> +    // GFileInputStream is not pollable, so the stream is only pollable if FormData doesn't contain EncodedFileData elements.
> > 
> > I wonder if it could be an options to check whether a GInputStream is a GUnixInputStream
> > (which does implement GPollableInputStream) which would make the file elements pollable,
> > or whether the GInputStream implements the GFileDescriptorBased interface, in which case
> > we could use the _get_fd() method and use GPollFD or similar 🤔️
> > 
> > (No need to change anything in this patch, by the way, this is just me mulling ideas
> > over. If you think any of this makes sense for a follow-up patch it would be nice
> > to write it down as a TODO and/or make a new Bugzilla issue to track it.)
> 
> We are always reading local files, so it's always GFileInputStream and not
> pollable.

👌️

> >> Source/WebCore/platform/network/soup/WebKitFormDataInputStream.cpp:177
> >> +    return g_pollable_source_new_full(stream, base.get(), cancellable);
> > 
> > Aren't these two lines equivalent to a plain g_pollable_input_stream_create_source() call?
> > As in:
> > 
> >    return g_pollable_input_stream_create_source(stream, cancellable);
> 
> This is the implementation of g_pollable_input_stream_create_source().

Right, I did not notice at first, my brain is a bit foggy this morning 😇️

[Carlos Garcia Campos]

Committed r274640 (235467@main): <https://commits.webkit.org/235467@main>

[Radar WebKit Bug Importer]

<rdar://problem/75567967>