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 761099 - http: cookies sharing in the pipeline
http: cookies sharing in the pipeline
Status: RESOLVED OBSOLETE
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
git master
Other All
: Normal enhancement
: git master
Assigned To: GStreamer Maintainers
GStreamer Maintainers
: 726314 731170 (view as bug list)
Depends on:
Blocks: 775920
 
 
Reported: 2016-01-25 17:34 UTC by Thiago Sousa Santos
Modified: 2018-11-03 11:44 UTC
See Also:
GNOME target: ---
GNOME version: ---



Description Thiago Sousa Santos 2016-01-25 17:34:47 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
Comment 1 Thiago Sousa Santos 2016-01-25 17:49:45 UTC
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.
Comment 2 Thiago Sousa Santos 2016-01-25 17:57:33 UTC
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.
Comment 3 Tim-Philipp Müller 2016-01-25 18:02:18 UTC
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?
Comment 4 Thiago Sousa Santos 2016-01-25 18:04:11 UTC
(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 :)
Comment 5 Philippe Normand 2016-01-27 11:42:07 UTC
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?
Comment 6 Philippe Normand 2016-01-28 14:39:59 UTC
Thanks for the work on this Thiago, I already validated this new feature will nicely work in WebKit :)
Comment 7 Thiago Sousa Santos 2016-02-03 18:14:19 UTC
(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?
Comment 8 Philippe Normand 2016-02-04 08:13:24 UTC
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.
Comment 9 Tim-Philipp Müller 2016-11-29 09:46:42 UTC
*** Bug 726314 has been marked as a duplicate of this bug. ***
Comment 10 Sebastian Dröge (slomo) 2016-11-29 11:27:06 UTC
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?
Comment 11 Thiago Sousa Santos 2016-11-30 23:58:08 UTC
(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.
Comment 12 Nicolas Dufresne (ndufresne) 2016-12-01 01:12:41 UTC
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().
Comment 13 Sebastian Dröge (slomo) 2016-12-01 11:09:08 UTC
(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 ;)
Comment 14 Thiago Sousa Santos 2016-12-03 13:03:11 UTC
(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.
Comment 15 Thiago Sousa Santos 2016-12-08 15:30:15 UTC
Patches updated.
Comment 16 Zaidan Alaoui 2017-03-10 06:40:21 UTC
Hello Thiago Sousa Santos,

Do you plan to merge these changes into master anytime soon ?

Thanks
Comment 17 GstBlub 2017-08-15 14:56:40 UTC
*** Bug 731170 has been marked as a duplicate of this bug. ***
Comment 18 Edward Hervey 2017-11-10 09:48:22 UTC
Is this needed with the shared soup session ?
Comment 19 Sebastian Dröge (slomo) 2017-11-10 09:54:35 UTC
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.
Comment 20 GStreamer system administrator 2018-11-03 11:44:21 UTC
-- 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.