GNOME Bugzilla – Bug 669260
Open/Save dialog in Midori browser hangs waiting for data with libsoup ver. past 2.32.2
Last modified: 2012-04-16 17:49:04 UTC
Way to reproduce (in midori browser): 1. Go to http://code.google.com/p/ossbuild/downloads/detail?name=GStreamer-WinBuilds-SDK-LGPL-x86-Beta04-0.10.7.msi&can=2&q= 2. Try to download a file by clicking link named GStreamer-WinBuilds-SDK-LGPL-x86-Beta04-0.10.7.msi 3. Instead of normal open/save dialog with mime type displayed you see grayed out dialog waiting for data. Seems to be uniform on google code, some random sites with autogenerated download links seems to behave the same way. From midori bugtracker: https://bugs.launchpad.net/midori/+bug/780133 André Stösel (ivaldi) wrote on 2011-09-15: Every time the dialog doesn't render, the mime type callback (webkit_web_view_mime_type_decision_cb) is called before the "SoupSocket::readable" signal ends. Offending commit seems to be: 97bacf3057b7c45c29a6d025c67a6dd588144e05. "SoupSocket: port to GSocketConnection/GTlsConnection" Every libsoup version past 2.32.2 is affected by this bug.
*** Bug 673596 has been marked as a duplicate of this bug. ***
It's a problem with child sources; you're running the dialog from a point that is inside the SoupSocket's GPollableSource callback. And glib is blocking that source from being able to trigger recursively, but it's not blocking that source's child GSocketSource, so that keeps triggering constantly, blocking the lower-priority redraw from being able to happen.
Created attachment 211872 [details] [review] gmain: block child sources when blocking the parent When blocking a source that has child sources, we need to consider the children blocked as well. Otherwise they will still trigger repeatedly in an inner loop started from the parent source's callback.
$ cd glib/tests/ $ git grep g_source_add_child_source $
I agree, having tests for the child-source functionality would be very useful.
Created attachment 212002 [details] [review] gmain: block child sources when blocking the parent When blocking a source that has child sources, we need to consider the children blocked as well. Otherwise they will still trigger repeatedly in an inner loop started from the parent source's callback.
Created attachment 212003 [details] [review] tests/mainloop: add a test for child sources ==== Note that while there were no child source tests in glib/tests, they were getting tested before by the gio tests (via the various sources in gio that use child sources). We just weren't testing this particular case because it had never occurred to me. But yes, having child-source-specific tests in glib/tests is good, and it even pointed out a bug in my previous patch (which blocked pollfds in child sources, but not child sources in general).
Fwiw, i can confirm that the patch fixes the issue with midori. I've imported it locally as http://www.openbsd.org/cgi-bin/cvsweb/ports/devel/glib2/patches/patch-glib_gmain_c?rev=1.12
Review of attachment 212002 [details] [review]: - g_return_if_fail (!SOURCE_BLOCKED (source)); /* Source already unblocked */ + g_return_if_fail (SOURCE_BLOCKED (source)); /* Source already unblocked */ This seems to imply the whole thing was totally broken before, right? Your test case is really good, very well thought out. The comment was very helpful for me to just understand how child sources are expected to work. I just have one minor comment on it (and feel free to keep the test case as is), it seems like the test would cover a bit more if we changed the parent timeout to say 1000, to test the case where the parent fires without the child. Would it be useful to test a full tree, i.e. sources with child sources at least some of which themselves have child sources? Not sure if that happens in reality (I assume not), but just mentioning it.
(In reply to comment #9) > Review of attachment 212002 [details] [review]: > > - g_return_if_fail (!SOURCE_BLOCKED (source)); /* Source already unblocked */ > + g_return_if_fail (SOURCE_BLOCKED (source)); /* Source already unblocked */ > > This seems to imply the whole thing was totally broken before, right? It's not as bad as it looks. SOURCE_BLOCKED() gave the right answer everywhere else, but g_main_dispatch() clears the G_HOOK_FLAG_IN_CALL flag before calling unblock_source(), so SOURCE_BLOCKED() was always FALSE there. > it seems like the test would cover a bit more if we changed the parent > timeout to say 1000, to test the case where the parent fires without the child. yeah, i could do that. > Would it be useful to test a full tree, i.e. sources with child sources at > least some of which themselves have child sources? Not sure if that happens in > reality (I assume not), but just mentioning it. It does happen. Eg, if you poll on a GTlsConnection, you get a GTlsConnectionGnutlsSource with a child GPollableSource with a child GSocketSource. (The GPollableSource is created by GSocketInputStream because GSocketSource takes a GSocketSourceFunc, but GSocketInputStream needs to expose a source with a GPollableSourceFunc instead.)
Created attachment 212154 [details] [review] gmain: Add a test for recursive child sources
(In reply to comment #11) > Created an attachment (id=212154) [details] [review] > gmain: Add a test for recursive child sources This test presently fails - I'm not sure yet whether it's me misunderstanding the expected semantics of child sources, or a bug.
Looks to me like g_source_list_add() doesn't handle the recursion.
Incidentally before I forget /mainloop/invoke is broken - its use of GCond is racy. It needs to set a boolean protected by the GCond, not try to use GCond as a flag.
(In reply to comment #12) > > gmain: Add a test for recursive child sources > > This test presently fails - I'm not sure yet whether it's me misunderstanding > the expected semantics of child sources, or a bug. Neither. It's timeout sources being tricky. (This messed me up on the first try too.) Whenever a timeout source gets dispatched, its timer gets reset. So when child_b triggers at 220, parent's timeout gets pushed back to 720. Then child_c triggers at 430, which pushes child_b's next timeout to 650 and parent's next timeout to 930, etc. (Of course in the real world, there's no good reason to chain timeout sources like we're doing here, so no one else will ever run into this. A more natural example would be something like having a GIOChannel source, with a GTimeoutSource child (to do an async read with a timeout). But that would have been a bunch more code, and wouldn't have exercised the child source codepaths any more.)
Created attachment 212159 [details] [review] gmain: Add a test for recursive child sources
(In reply to comment #15) > (In reply to comment #12) > > > gmain: Add a test for recursive child sources > > > > This test presently fails - I'm not sure yet whether it's me misunderstanding > > the expected semantics of child sources, or a bug. > > Neither. It's timeout sources being tricky. (This messed me up on the first try > too.) Whenever a timeout source gets dispatched, its timer gets reset. Got it. Did the math and confirmed it matches. (I'm interested in understanding the requirements and semantics of this code for my hopeful future of pushing mainloop subset down into glibc) So I think we're good to go with your patches?
pushed to master and presumably soon to glib-2-32. walters and I are still looking at the test_invoke() problems on IRC.