[GStreamer] Fix double-ref of WebKitWebSource in StreamingClient
Created attachment 236937 [details]
adoptGRef() crashes if the pointer it references isn't floating, and since this code refs right before calling adoptGRef(), it's guaranteed not to be floating. Also, adoptGRef() only exists to avoid ref'ing, which makes it unnecessary in this case. GRefPtr's normal constructor will ref the pointer like we want.
Oh, and static_cast didn't work because WebKitWebSource uses GObject inheritance. Using reinterpret_cast bothers me, but the only other alternatives are a C-style case, or GST_ELEMENT_CAST, which is internally just a C-style cast. I assume an explicit reinterpret_cast is better than a hidden C-style cast, since at least it's obvious that this is "potentially scary" code.
This page causes a crash without this change:
Unfortunately, after the change it just locks up. I'll try to track that down too, but I assume it's a completely different issue.
Comment on attachment 236937 [details]
Thanks! This looks good and the change is well explained in the bug comments, perhaps the ChangeLog could also have a small description of the patch? Not everyone goes read bug comments when parsing commit logs :)
Created attachment 236983 [details]
Here you go. I feel like the description is pretty long for something so trivial, but I guess people only look in the commit logs when they need info, so it's potentially useful.
Comment on attachment 236983 [details]
I'm going to wait a minute before applying this, since it sounds like it might cause refcounting issues somehow..
Ok, so I had adoptGRef backwards: It only works if the pointer *isn't* floating. By ref'ing a floating pointer normally, we take ownership of something that we might not have wanted to. I'm trying to figure out why this is causing problems in this case. Presumably by the point of webKitWebSrcStart(), src shouldn't be floating. We don't seem to instantiate it directly though, which means this is presumably being handled by Playbin..
This ended up a lot more complicated than I expected (the explanation, not the code change). I think the correct thing to do in this case is to use gst_object_ref and gst_object_unref and not use GRefPtr at all, since adoptGRef won't let us do the right thing, and removing that ASSERT is likely to cause a lot more problems, since it's too easy to accidentally adopt a floating pointer.
Created attachment 237008 [details]
Comment on attachment 237008 [details]
View in context: https://bugs.webkit.org/attachment.cgi?id=237008&action=review
Not setting cq+ yet, is the crack word a typo in the ChangeLog? :)
> + don't construct this ourselves, I assume this is happening in Playbin.
Yes it happens at least in the HLS case when the demuxer creates a new webkitsrc element using gst_element_make_from_uri() to download a new fragment. I suppose it's similar for DASH.
> + This should work, but actually causes the page to either lock up or crack (different
Created attachment 237086 [details]
(In reply to comment #12)
> (From update of attachment 237008 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=237008&action=review
> Not setting cq+ yet, is the crack word a typo in the ChangeLog? :)
> > Source/WebCore/ChangeLog:8
> > + don't construct this ourselves, I assume this is happening in Playbin.
> Yes it happens at least in the HLS case when the demuxer creates a new webkitsrc element using gst_element_make_from_uri() to download a new fragment. I suppose it's similar for DASH.
> > Source/WebCore/ChangeLog:12
> > + This should work, but actually causes the page to either lock up or crack (different
> crack? :)
I meant that the bug was so bad it would literally cause your monitor to crack ;)
Comment on attachment 237086 [details]
Clearing flags on attachment: 237086
Committed r172928: <http://trac.webkit.org/changeset/172928>
All reviewed patches have been landed. Closing bug.