Bug 136132 – [GStreamer] ASSERT failure in WebKitWebSource in StreamingClient

• Component: New Bugs
• Status: RESOLVED FIXED

[Brendan Long]

[GStreamer] Fix double-ref of WebKitWebSource in StreamingClient

[Brendan Long]

Created attachment 236937 [details]
Patch

[Brendan Long]

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.

[Brendan Long]

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.

[Brendan Long]

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.

[Philippe Normand]

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 :)

[Brendan Long]

Created attachment 236983 [details]
Patch

[Brendan Long]

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.

[Brendan Long]

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..

[Brendan Long]

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..

[Brendan Long]

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.

[Brendan Long]

Created attachment 237008 [details]
Patch

[Philippe Normand]

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? :)

[Brendan Long]

Created attachment 237086 [details]
Patch

[Brendan Long]

(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 ;)

[WebKit Commit Bot]

Comment on attachment 237086 [details]
Patch

Clearing flags on attachment: 237086

Committed r172928: <http://trac.webkit.org/changeset/172928>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.