|Summary:||[GStreamer] ASSERT failure in WebKitWebSource in StreamingClient|
|Product:||WebKit||Reporter:||Brendan Long <b.long>|
|Component:||New Bugs||Assignee:||Brendan Long <b.long>|
|Severity:||Normal||CC:||commit-queue, pnormand, reyndersbrian|
|Version:||528+ (Nightly build)|
Description Brendan Long 2014-08-21 14:40:28 PDT
[GStreamer] Fix double-ref of WebKitWebSource in StreamingClient
Comment 2 Brendan Long 2014-08-21 14:45:48 PDT
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.
Comment 3 Brendan Long 2014-08-21 14:48:33 PDT
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.
Comment 4 Brendan Long 2014-08-21 14:49:37 PDT
This page causes a crash without this change: http://www.vox.com/2014/8/18/6043763/cnns-jake-tapper-on-the-police-in-ferguson-this-doesnt-make-any-sense 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 5 Philippe Normand 2014-08-22 00:03:21 PDT
Comment on attachment 236937 [details] Patch 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 :)
Comment 7 Brendan Long 2014-08-22 06:40:35 PDT
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 8 Brendan Long 2014-08-22 08:44:41 PDT
Comment on attachment 236983 [details] Patch I'm going to wait a minute before applying this, since it sounds like it might cause refcounting issues somehow..
Comment 9 Brendan Long 2014-08-22 15:27:47 PDT
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..
Comment 10 Brendan Long 2014-08-22 16:11:45 PDT
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.
Comment 12 Philippe Normand 2014-08-25 00:28:28 PDT
Comment on attachment 237008 [details] Patch 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? :)
Comment 14 Brendan Long 2014-08-25 08:35:10 PDT
(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 15 WebKit Commit Bot 2014-08-25 11:34:43 PDT
Comment on attachment 237086 [details] Patch Clearing flags on attachment: 237086 Committed r172928: <http://trac.webkit.org/changeset/172928>
Comment 16 WebKit Commit Bot 2014-08-25 11:34:46 PDT
All reviewed patches have been landed. Closing bug.