GNOME Bugzilla – Bug 761099
http: cookies sharing in the pipeline
Last modified: 2018-11-03 11:44:21 UTC
Currently if there are multiple http source elements in a pipeline they will all use independent connections, causing their cookies to be also independent. This can be harmful when those elements are communicating with the same service and this service uses some kind of cookie-based feature, causing the connections to be rejected or not working as expected. One particular use case is for adaptive streaming for servers that set some cookie on the playlist/manifest and then check that the cookie exists in subsequent requests to the fragments. A test server that does this verification can be found at: https://github.com/thiagoss/adaptive-test-server. A few bugs that are related/duplicates of this. Decided to open a new one because the others seem to only attempt to solve partially the problem. https://bugzilla.gnome.org/show_bug.cgi?id=726314 https://bugzilla.gnome.org/show_bug.cgi?id=751371 https://bugzilla.gnome.org/show_bug.cgi?id=751372 https://bugzilla.gnome.org/show_bug.cgi?id=731170
For solving this issue, a Gstreamer CookieJar was implemented (copied from libsoup) and it can be used as a generic in-memory repository of cookies that emits a signal everytime a cookie is added/removed/modified. In this manner, other cookiejar implemenations can listen to this signal and keep their jars synchronized. It is necessary that these implementations also notify the gstreamer cookiejar when their cookies change. The cookiejar is shared through a context message. Patches are needed everywhere: core: http://cgit.freedesktop.org/~thiagoss/gstreamer/log/?h=cookies Adds new http module to gst-uninstalled base: http://cgit.freedesktop.org/~thiagoss/gst-plugins-base/log/?h=gstcookies Adds new http module that has a gstcookie and gstcookiejar. Those are copied from libsoup's implementation and: * Stripped of all functions that were not related to adding/removing cookies to the jar. * Added a RecMutex to prevent concurrent cookiejar manipulation * When one operation is being done (adding/removal) a flag is set that makes it ignore all calls to add/remove cookies to avoid that listeners of the 'changed' signal modify the cookiejar as a reflex of receiving the signal. This can happen when the cookiejar implementation emits its own version of the 'changed' signal causing an infinite recursion of cookie adding. good: http://cgit.freedesktop.org/~thiagoss/gst-plugins-good/log/?h=gst-cookies use the http module cookiejar sharing and bind it with libsoup's cookiejar to keep them synchronized bad: http://cgit.freedesktop.org/~thiagoss/gst-plugins-bad/log/?h=cookies Have a way to make uridownloader able to post need-context messages on a bus so that it can get the http context from the pipeline.
The other alternative is to use the same cookiejar sharing context message but let libraries share their own objects instead of having a central gstreamer cookies repository. This is simpler to implement but has the downside of only allowing elements that use the same http implementation to share cookies among them. So if your pipeline would have different http implementations: soup and neon, for example. Those elements would not be able to share cookies. It is unusual to have different HTTP implementations on the same pipeline, but it can happen when browsers (that have their own cookiejar) use gstreamer for some multimedia task and need to share their cookies with gstreamer.
Cool! Just drive-by commenting on the least important aspect: do we really need a new http mini lib for this? Can't we chuck it into libgstnet or pbutils or gstapp?
(In reply to Tim-Philipp Müller from comment #3) > Cool! Just drive-by commenting on the least important aspect: do we really > need a new http mini lib for this? Can't we chuck it into libgstnet or > pbutils or gstapp? Yeah, I don't mind. pbutils or net sounds fine to me. Just added a new lib to avoid deciding it early when developing :)
I think the cookie->path cannot be NULL, otherwise there will be a crash in soup_cookies_applies_to_uri(). Setting it to an empty string works though, so maybe update the gst_http_cookie_new() doc accordingly?
Thanks for the work on this Thiago, I already validated this new feature will nicely work in WebKit :)
(In reply to Philippe Normand from comment #5) > I think the cookie->path cannot be NULL, otherwise there will be a crash in > soup_cookies_applies_to_uri(). > > Setting it to an empty string works though, so maybe update the > gst_http_cookie_new() doc accordingly? So we need to protect the creation of soup cookies in souphttpsrc, right? Anything preventing us from having a null pointer in our own cookie implementation?
Yes I think it would be safer to protect the creation of soup cookies. Reading the code again, I don't think anything prevents us having a null pointer path in our own cookie implementation, the potential issue is really only when we create soup cookies from gst cookies.
*** Bug 726314 has been marked as a duplicate of this bug. ***
Just some comments about the API: * gsthttpcookiejar.h: - The "gpointer author" in the changed signal / add is not very bindings friendly. GObject*? GValue (then you could also use boxed types)? With the gpointer, how is ownership even handled? - GSList* is ugly too - What about some API to get all cookies for a specific domain, so have the filtering-code in one specific place instead of having to duplicate it - API to expire all cookies that are expired now? * gsthttpcookie.h: - Missing struct padding - Maybe make cookies immutable after creation? Or GstMiniObject-style COW?
(In reply to Sebastian Dröge (slomo) from comment #10) > Just some comments about the API: > * gsthttpcookiejar.h: > - The "gpointer author" in the changed signal / add is not very bindings > friendly. GObject*? GValue (then you could also use boxed types)? With the > gpointer, how is ownership even handled? The idea here is just for users to be able to ignore changes caused by themselves. Suppose you add a cookie, you will get a 'changed' signal. Looking at the author you can check that it was yourself and ignore that change. Otherwise you should get that new cookie and add it to your local store. Any ideas on how to solve this with bindings friendly parameter? Just use GObject*? This will make all users of the GstHttpCookieJar need to be a GObject, but we expect that it would be GstElements using it. > - GSList* is ugly too Do we have a bindings friendly list? Or is it more common to have a _size() and _nth() function pair? > - What about some API to get all cookies for a specific domain, so have > the filtering-code in one specific place instead of having to duplicate it > - API to expire all cookies that are expired now? I was trying to avoid having all cookie related functions inside this lib. It should serve mostly to unite cookies from different worlds (webkit and soup). The elements would actually be using the functions provided by those more complete libraries and the changes they do there should propagate to our library so that they can also be replicated on other cookiejars. I don't see a need to do that from gstreamer code just yet. > > * gsthttpcookie.h: > - Missing struct padding > - Maybe make cookies immutable after creation? Or GstMiniObject-style COW? Good idea, will update that.
In GTK application, when we update the value of widget and want to avoid the recursion of the callback, we often use g_signal_handler_block().
(In reply to Thiago Sousa Santos from comment #11) > (In reply to Sebastian Dröge (slomo) from comment #10) > > Just some comments about the API: > > * gsthttpcookiejar.h: > > - The "gpointer author" in the changed signal / add is not very bindings > > friendly. GObject*? GValue (then you could also use boxed types)? With the > > gpointer, how is ownership even handled? > > The idea here is just for users to be able to ignore changes caused by > themselves. Suppose you add a cookie, you will get a 'changed' signal. > Looking at the author you can check that it was yourself and ignore that > change. Otherwise you should get that new cookie and add it to your local > store. Any ideas on how to solve this with bindings friendly parameter? Just > use GObject*? This will make all users of the GstHttpCookieJar need to be a > GObject, but we expect that it would be GstElements using it. Understood. I'd make it a GObject* and store a (weak?) reference then, everything else just seems weird. > > - GSList* is ugly too > > Do we have a bindings friendly list? Or is it more common to have a _size() > and _nth() function pair? GPtrArray / GArray maybe? In most cases it's also more efficient than a linked list, not sure if here. Depends on what kind of operations are supposed to be done on this list. > > - What about some API to get all cookies for a specific domain, so have > > the filtering-code in one specific place instead of having to duplicate it > > - API to expire all cookies that are expired now? > > I was trying to avoid having all cookie related functions inside this lib. > It should serve mostly to unite cookies from different worlds (webkit and > soup). The elements would actually be using the functions provided by those > more complete libraries and the changes they do there should propagate to > our library so that they can also be replicated on other cookiejars. I don't > see a need to do that from gstreamer code just yet. Ok, can be added when needed. Just don't add code for that in every single plugin that uses this API ;)
(In reply to Nicolas Dufresne (stormer) from comment #12) > In GTK application, when we update the value of widget and want to avoid the > recursion of the callback, we often use g_signal_handler_block(). Interesting trick, didn't know about it. Anyway this can be racy in this scenarios as multiple threads can be calling the signal at the same time.
Patches updated.
Hello Thiago Sousa Santos, Do you plan to merge these changes into master anytime soon ? Thanks
*** Bug 731170 has been marked as a duplicate of this bug. ***
Is this needed with the shared soup session ?
Not "needed", but it gives additional control (and integration into external cookie storage) over what soup session sharing allows, and more importantly could be implemented for other sources too.
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-base/issues/250.