Bug 185657 - [GStreamer]: Consider GstStream(Collection) as if if was not a GInitiallyUnowned
Summary: [GStreamer]: Consider GstStream(Collection) as if if was not a GInitiallyUnowned
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks: 185787
  Show dependency treegraph
 
Reported: 2018-05-15 14:27 PDT by Thibault Saunier
Modified: 2018-05-18 15:09 PDT (History)
6 users (show)

See Also:


Attachments
[GStreamer]: Do not steal GstStreamCollection's reference to GstStream (5.13 KB, patch)
2018-05-15 14:29 PDT, Thibault Saunier
no flags Details | Formatted Diff | Diff
Fix previously reported styling issues. (5.13 KB, patch)
2018-05-15 14:39 PDT, Thibault Saunier
no flags Details | Formatted Diff | Diff
Fix the issue in GRefPtrGStreamer.cpp directly. (5.12 KB, patch)
2018-05-16 11:11 PDT, Thibault Saunier
no flags Details | Formatted Diff | Diff
Spelling fixes in the ChageLog. (5.33 KB, patch)
2018-05-16 11:25 PDT, Thibault Saunier
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Thibault Saunier 2018-05-15 14:27:09 PDT
See commit message.
Comment 1 Thibault Saunier 2018-05-15 14:29:43 PDT
Created attachment 340434 [details]
[GStreamer]: Do not steal GstStreamCollection's reference to GstStream

Before GStreamer 1.14[1] GstStream's reference was not sunk when creating
it, meaning that when we were using

    `GRefPtr<GstStream> stream = gst_stream_collection_get(collection, stream_index);`

we were actually calling `g_object_sink_ref(some_stream);` leading to taking
ownership of that (floating) reference, while we were not actually owning it as it was
owned by the GstStreamCollection itself. In the end we were ending up
unrefing an already destroyed object randomly later on (see bug #184581)

This patchs makes sure the floating reference is sunk (giving real ownership
to the GstStreamCollection in the end) for Gst < 1.14.

Starting from 1.14, the ref is sunk in gst_stream_new and we will not do anything
as we do not need to anymore.

[1] commit f119e93b47efb06ffc68c01d3e094d5346c30041 `gst: Clear floating flag in constructor of all GstObject subclasses that are not owned by any parent`
Comment 2 Build Bot 2018-05-15 14:32:39 PDT
Attachment 340434 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:635:  Declaration has space between type name and * in GstStream * tmpstream  [whitespace/declaration] [3]
ERROR: Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1335:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Thibault Saunier 2018-05-15 14:39:44 PDT
Created attachment 340435 [details]
Fix previously reported styling issues.
Comment 4 Philippe Normand 2018-05-16 07:48:48 PDT
Comment on attachment 340435 [details]
Fix previously reported styling issues.

I wonder if GRefPtrGStreamer.cpp could be patched instead?
Comment 5 Thibault Saunier 2018-05-16 11:11:10 PDT
Created attachment 340501 [details]
Fix the issue in GRefPtrGStreamer.cpp directly.
Comment 6 Thibault Saunier 2018-05-16 11:25:21 PDT
Created attachment 340505 [details]
Spelling fixes in the ChageLog.

[1] commit f119e93b47efb06ffc68c01d3e094d5346c30041 `gst: Clear floating flag in constructor of all GstObject subclasses that are not owned by any parent`
Comment 7 Philippe Normand 2018-05-16 11:47:38 PDT
Comment on attachment 340505 [details]
Spelling fixes in the ChageLog.

LGTM but I'll let Carlos check this too.
Comment 8 Carlos Garcia Campos 2018-05-16 22:58:12 PDT
Comment on attachment 340505 [details]
Spelling fixes in the ChageLog.

Ok
Comment 9 WebKit Commit Bot 2018-05-17 01:16:45 PDT
Comment on attachment 340505 [details]
Spelling fixes in the ChageLog.

Clearing flags on attachment: 340505

Committed r231891: <https://trac.webkit.org/changeset/231891>
Comment 10 WebKit Commit Bot 2018-05-17 01:16:47 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Radar WebKit Bug Importer 2018-05-17 01:17:19 PDT
<rdar://problem/40324459>