GNOME Bugzilla – Bug 773257
Async request never completes if message is cancelled before reading body data while being paused
Last modified: 2017-05-03 16:21:41 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.
Created attachment 338075 [details] [review] Patch Added unit tests and checked all other tests pass.
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.
(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 on attachment 338075 [details] [review] Patch ok, let's commit this to master at least
(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 :-)