After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 773257 - Async request never completes if message is cancelled before reading body data while being paused
Async request never completes if message is cancelled before reading body dat...
Status: RESOLVED FIXED
Product: libsoup
Classification: Core
Component: API
2.56.x
Other Linux
: Normal normal
: ---
Assigned To: libsoup-maint@gnome.bugs
libsoup-maint@gnome.bugs
Depends on:
Blocks:
 
 
Reported: 2016-10-20 08:07 UTC by Carlos Garcia Campos
Modified: 2017-05-03 16:21 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch (9.74 KB, patch)
2016-10-20 08:13 UTC, Carlos Garcia Campos
committed Details | Review

Description Carlos Garcia Campos 2016-10-20 08:07:50 UTC
The async ready cabllack is never called in this case. I noticed this in WebKit, it happens if you open a website that is HTTP authenticated and close the tab without responding to the auth dialog. What happens internally is that we pause the message on authenticate signal, and then we cancel the message. The message and its io are properly finished, but the request async ready callback is never called, which caused the WebKit object to be leaked, because it takes a reference of the object on send_sync and releases on the async ready callback.
The sequence of libsoup internals is the following one:

 1- Message starts normally and on ready async_send_request_running is called that sets io_started = TRUE and starts the io
 2- Before message io started to read the body data, it can be authenticate or got-headers, for example, the message is paused.
 3- Next call to try_run_until_read calls soup_message_io_run_until_read() that returns G_IO_ERROR_WOULD_BLOCK because the message is paused.
 4- Because of the G_IO_ERROR_WOULD_BLOCK try_run_until_read creates a SoupMessageSource. Since the message is paused but there's io in progress, the SoupMessageSource is created witout a bae source, and therefore it's not cancellable with the passed GCancellable.
 5- The message is cancelled, soup_session_cancel_message checks that the message is paused, so it marks the item as not paused, and also unpause the message io, sets the status message, cancels the cancellable and kicks the item queue.
 6- When the item is processed again the status is changed to FINISHING and then soup_message_finished is called.
 7- async_send_request_finished() is called due to message finished signal emission, but returns early without calling async_send_request_return_result because item->io_started is TRUE (see step 1).
 8- The SoupMessageSource is still alive, but since it's paused and now message doesn't have io because it was finished, the source is never dispatched (message_source_check always returns false because paused is TRUE and io is NULL).

The problem is in this last step, I think the source should be dispatched when the io becomes NULL and the source was created paused.
Comment 1 Carlos Garcia Campos 2016-10-20 08:13:35 UTC
Created attachment 338075 [details] [review]
Patch

Added unit tests and checked all other tests pass.
Comment 2 Dan Winship 2017-02-20 21:07:32 UTC
Comment on attachment 338075 [details] [review]
Patch

FYI, I keep looking at this patch, and then thinking "that can't be right", but then I never find the time to dig into it more deeply. I'm pretty sure there are cases where io is NULL and message_source_check needs to return FALSE though.
Comment 3 Carlos Garcia Campos 2017-02-22 08:25:37 UTC
(In reply to Dan Winship from comment #2)
> Comment on attachment 338075 [details] [review] [review]
> Patch
> 
> FYI, I keep looking at this patch, and then thinking "that can't be right",
> but then I never find the time to dig into it more deeply. I'm pretty sure
> there are cases where io is NULL and message_source_check needs to return
> FALSE though.

Thanks, FWIW, I've this patch always applied locally since I wrote it and I haven't noticed any problem.
Comment 4 Dan Winship 2017-05-01 17:49:01 UTC
Comment on attachment 338075 [details] [review]
Patch

ok, let's commit this to master at least
Comment 5 Carlos Garcia Campos 2017-05-03 16:21:18 UTC
(In reply to Dan Winship from comment #4)
> Comment on attachment 338075 [details] [review] [review]
> Patch
> 
> ok, let's commit this to master at least

Yes, we have the whole cycle to test it :-)