Bug 115352

Created attachment 200009 [details] Patch

Comment on attachment 200009 [details] Patch Hrm. It seems odd that we let code outside of WebCore create our private GStreamer elements.

I suppose this is related with some adaptive streaming scenario? We plan soon to switch the http source element to webkit+http scheme btw, bug 108088

(In reply to comment #2) > (From update of attachment 200009 [details]) > Hrm. It seems odd that we let code outside of WebCore create our private GStreamer elements. GStreamer will use the webkit source element when creating elements using gst_element_make_from_uri for uris supported by the webkit source element (http://).

(In reply to comment #3) > I suppose this is related with some adaptive streaming scenario? > > We plan soon to switch the http source element to webkit+http scheme btw, bug 108088 Yes, the threading issues were found when testing DASH and MSS support for gstreamer which uses gst-plugins-bad GstUriDownloader which in turn uses the webkit source element when creating http:// elements with gst_element_make_from_uri. GstUriDownloader uses its own threads to download data which leads to some races when using the webkit gst source element.

(In reply to comment #5) > (In reply to comment #3) > > I suppose this is related with some adaptive streaming scenario? > > > > We plan soon to switch the http source element to webkit+http scheme btw, bug 108088 > > Yes, the threading issues were found when testing DASH and MSS support for gstreamer which uses gst-plugins-bad GstUriDownloader which in turn uses the webkit source element when creating http:// elements with gst_element_make_from_uri. GstUriDownloader uses its own threads to download data which leads to some races when using the webkit gst source element. This is bad news if the element is interacting with WebCore at all. Interactions with WebCore should all be on the main thread (the WebCore thread) and mutexes around critical sections in the element are not enough to prevent badness.

(In reply to comment #6) > (In reply to comment #5) > > (In reply to comment #3) > > > I suppose this is related with some adaptive streaming scenario? > > > > > > We plan soon to switch the http source element to webkit+http scheme btw, bug 108088 > > > > Yes, the threading issues were found when testing DASH and MSS support for gstreamer which uses gst-plugins-bad GstUriDownloader which in turn uses the webkit source element when creating http:// elements with gst_element_make_from_uri. GstUriDownloader uses its own threads to download data which leads to some races when using the webkit gst source element. > > This is bad news if the element is interacting with WebCore at all. Interactions with WebCore should all be on the main thread (the WebCore thread) and mutexes around critical sections in the element are not enough to prevent badness. This patch makes sure all WebCore methods are called from the main thread using g_timeout_add_full (assuming the glib main thread is the same as WebCore main thread for webkit-gtk). This isn't true without this patch though.

Created attachment 200022 [details] Patch

(In reply to comment #8) > Created an attachment (id=200022) [details] > Patch This new patch removes the dependency on bug 115351.

The patch looks basically good to me and also makes conceptionally sense, independent of WebKit exposing an URI handler for http:// etc or not. It's also broken and not threadsafe in the standard scenarios.

Note that many of the locks there are not actually needed (but won't hurt, maybe performance a bit). Don't have the time now to carefully check which are not needed, but there seems to be some confusion about which thread is (or which threads are) handling which function, and where concurrency can happen at all. It's a bit too cautious :)

Comment on attachment 200022 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=200022&action=review > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:453 > + gst_app_src_set_size(priv->appsrc, -1); Any reason why you move these at the very end? > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:-632 > - return g_strdup(WEBKIT_WEB_SRC(handler)->priv->uri); A const gchar* function returning a copy a copy of a string? That looks wrong, but it was like that before already :) But then this is 0.10 code, which was broken exactly because of things like this. So decide for yourself if you want to leak here or potentially crash > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:865 > + GST_OBJECT_UNLOCK(src); I think these locks are actually unnecessary, as that function will only be called right after creating the instance and then never again. But they don't hurt

(In reply to comment #12) > (From update of attachment 200022 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=200022&action=review > > > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:453 > > + gst_app_src_set_size(priv->appsrc, -1); > > Any reason why you move these at the very end? > IIRC, it was because of the locks, this call doesn't need to be done under locks and I refactored this method a bit to avoid locking/unlocking many times. > > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:-632 > > - return g_strdup(WEBKIT_WEB_SRC(handler)->priv->uri); > > A const gchar* function returning a copy a copy of a string? That looks wrong, but it was like that before already :) But then this is 0.10 code, which was broken exactly because of things like this. So decide for yourself if you want to leak here or potentially crash > Yep, the issue here is that the uri may change, thus we need a lock/dup to get the uri. But as you said this is for 0.10 only so I would rather leak than crash. > > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:865 > > + GST_OBJECT_UNLOCK(src); > > I think these locks are actually unnecessary, as that function will only be called right after creating the instance and then never again. But they don't hurt Yeah, not change here, but let me know if you want me to remove these extra locks, I wouldn't mind tbh, they won't hurt anyway :)

Comment on attachment 200022 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=200022&action=review > Source/WebCore/ChangeLog:3 > + Make sure gstreamer source element is thread-safe. Sorry to be annoying about this but the Changelog entry format is not correct, please check other entries. Usually thee structure is: bug title bug link Reviewed by... description of change > Source/WebCore/ChangeLog:15 > + No new tests (OOPS!). If no new test is added please remove this line. > Source/WebCore/ChangeLog:18 > + (_WebKitWebSrcPrivate): It would bee good, I think, to have a little summary of each function/method change. >>> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:865 >>> + GST_OBJECT_UNLOCK(src); >> >> I think these locks are actually unnecessary, as that function will only be called right after creating the instance and then never again. But they don't hurt > > Yeah, not change here, but let me know if you want me to remove these extra locks, I wouldn't mind tbh, they won't hurt anyway :) Right, please remove these locks :) > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1075 > + uri = g_strdup(m_src->priv->uri); I think it'd make sense to use a GOwnPtr<gchar> here > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1089 > + uri = g_strdup(m_src->priv->uri); Ditto

Comment on attachment 200022 [details] Patch r- as per comments above

Created attachment 202292 [details] Patch (In reply to comment #14) > > Source/WebCore/ChangeLog:3 > > + Make sure gstreamer source element is thread-safe. > > Sorry to be annoying about this but the Changelog entry format is not correct, please check other entries. Usually thee structure is: > > bug title > bug link > > Reviewed by... > > description of change > Np, thanks for the review. Changelog updated. > > Source/WebCore/ChangeLog:15 > > + No new tests (OOPS!). > > If no new test is added please remove this line. > Done > > Source/WebCore/ChangeLog:18 > > + (_WebKitWebSrcPrivate): > > It would bee good, I think, to have a little summary of each function/method change. > All changes are adding/removing locks here and there and making sure some methods are called from main thread, so not updated here as it would be difficult to describe changes per function/method. > >>> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:865 > >>> + GST_OBJECT_UNLOCK(src); > >> > >> I think these locks are actually unnecessary, as that function will only be called right after creating the instance and then never again. But they don't hurt > > > > Yeah, not change here, but let me know if you want me to remove these extra locks, I wouldn't mind tbh, they won't hurt anyway :) > > Right, please remove these locks :) > Done > > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1075 > > + uri = g_strdup(m_src->priv->uri); > > I think it'd make sense to use a GOwnPtr<gchar> here > Done > > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1089 > > + uri = g_strdup(m_src->priv->uri); > > Ditto Done Thanks everyone for the review.

Comment on attachment 202292 [details] Patch Clearing flags on attachment: 202292 Committed r151021: <http://trac.webkit.org/changeset/151021>

All reviewed patches have been landed. Closing bug.

Re-opened since this is blocked by bug 117924

Created attachment 208654 [details] Updated patch against upstream master So, first of all, this is the updated patch against upstream master with conflicts fixed, etc. I investigated the regressions on #117141 and #117688 and here goes my findings: 1 - Bug #117688 (http/tests/security/video-poster-cross-origin-crash.html crash on GTK bots): Running this test with both upstream/master and upstream/master with the updated patch applied I get the exactly same results: $ TEST_RUNNER_TEST_PLUGIN_PATH=./WebKitBuild/Debug/TestNetscapePlugin/.libs/libtestnetscapeplugin.so TEST_RUNNER_INJECTED_BUNDLE_FILENAME=WebKitBuild/Debug/Libraries/.libs/libTestRunnerInjectedBundle.so Tools/jhbuild/jhbuild-wrapper --gtk run ./WebKitBuild/Debug/Programs/WebKitTestRunner LayoutTests/http/tests/security/video-poster-cross-origin-crash.html Gtk-Message: Failed to load module "overlay-scrollbar" Gtk-Message: Failed to load module "canberra-gtk-module" Gtk-Message: Failed to load module "overlay-scrollbar" Gtk-Message: Failed to load module "canberra-gtk-module" Content-Type: text/plain >>> #EOF #EOF #EOF ** (WebKitTestRunner:24868): CRITICAL **: socket_embed_hook: assertion `spi_global_register != NULL' failed LEAK: 1 WebContext LEAK: 8 WebCoreNode 2 - Bug #117141 (fast/js/cyclic-proto.html crash on Qt bots): This bug seems to be crashing on LocalStorage and I can't see how this patch is related. That said I believe this bug is not related to the crashes, but if someone can enlighten me here or get me some better backtrace with the crashes I may be able to fix any possible regression. It is good to note that this patch fixes a bunch of thread related issues with the webkit gstreamer source element.

(In reply to comment #20 > 2 - Bug #117141 (fast/js/cyclic-proto.html crash on Qt bots): > This bug seems to be crashing on LocalStorage and I can't see how this patch is related. Sorry, now I saw the full backtrace there, both bugs are crashing in the same place, when calling "request.setHTTPReferrer(priv->player->referrer());" from webKitWebSrcStart. I will try to reproduce this crash here again.

Comment on attachment 208654 [details] Updated patch against upstream master Ignore this patch, I am working on an updated patch.

(In reply to comment #22) > (From update of attachment 208654 [details]) > Ignore this patch, I am working on an updated patch. I have a semi updated patch that should fix this bug, but the final changes here depend on input on bug #73743 (added some comments there). Once we solve the issue with my comments on bug #73743 I will push a new patch here.

Created attachment 208788 [details] Updated patch against upstream master This patch should fix the regressions with bug #117141 and bug #117688 by removing the timeout sources (e.g. scheduled start) when state changes to NULL as stop is scheduled to run only on mainloop. It should also fix another issue where the player was being reset on seek. As discussed on IRC this patch introduces 2 separate code branches, one where the player is available and using CachedResourceLoader and another one where there is no player involved using a ResourceHandle. I ran some tests with and without this patch and couldn't find any regression. I also tested the patch with a series of videos (including DASH that requires a new gst version) and everything seems to be working fine. Please let me know what you think. Also see comments on bug #73743 and bug #108088.

Marking as WONTFIX as the patch is not required.

(In reply to comment #25) > Marking as WONTFIX as the patch is not required. Arg, this should be on bug 115351, sorry, reopening.

Created attachment 208831 [details] Updated patch against upstream master Same as last patch but without reverting fix for bug 116533 (I was basing on an old git version where bug 116533 was not fixed). I am also investigating bug 85994 and may update this patch again.

Comment on attachment 208831 [details] Updated patch against upstream master View in context: https://bugs.webkit.org/attachment.cgi?id=208831&action=review > Source/WebCore/ChangeLog:7 > + Patch by Andre Moreira Magalhaes <andre.magalhaes@collabora.co.uk> on 2013-08-14 > + Reviewed by Philippe Normand. Please, don't submit this as reviewed, since it contains new code that hasn't been reviewed. > Source/WebCore/ChangeLog:37 > + * platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp: > + (_WebKitWebSrcPrivate): > + (webkit_web_src_init): > + (webKitWebSrcFinalize): > + (webKitWebSrcSetProperty): > + (webKitWebSrcGetProperty): > + (webKitWebSrcStop): > + (webKitWebSrcStart): > + (webKitWebSrcChangeState): > + (webKitWebSrcQueryWithParent): > + (webKitWebSrcGetUri): > + (webKitWebSrcSetUri): > + (webKitWebSrcNeedDataMainCb): > + (webKitWebSrcEnoughDataMainCb): > + (webKitWebSrcSeekMainCb): > + (webKitWebSrcSeekDataCb): > + (webKitWebSrcSetMediaPlayer): > + (StreamingClient::StreamingClient): > + (StreamingClient::~StreamingClient): > + (StreamingClient::didReceiveResponse): > + (StreamingClient::didReceiveData): > + (StreamingClient::didFinishLoading): > + (StreamingClient::wasBlocked): > + (StreamingClient::cannotShowURL): This doesn't look up to date. You should explain briefly here what you are changing. > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:71 > +class StreamingClientHandler { > + public: > + StreamingClientHandler(WebKitWebSrc*); > + virtual ~StreamingClientHandler(); > + > + char* getOrCreateReadBuffer(size_t requestedSize, size_t& actualSize); > + void responseReceived(const ResourceResponse&); > + void dataReceived(const char*, int); > + void notifyFinished(); > + > + private: > + WebKitWebSrc* m_src; > +}; I wonder if we could add the common implementation to the base class instead of using a new class. > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:73 > +// common StreamingClient interface Comments should start with capital letter and finish with a period. > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:80 > + virtual bool loadFailed() const = 0; > + virtual void setDefersLoading(bool) = 0; > +}; So, instead of an abstract class, we could probably add the common implementation here. > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:98 > WebKitWebSrc* m_src; This is already in the handler, maybe we could add a getter to the handler, or if we end up using a base class, move this to the base class as protected member. > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:99 > + StreamingClientHandler* m_handler; If we use the handler this should be OwnPtr<StreamingClientHandler> > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:124 > + > + WebKitWebSrc* m_src; > + StreamingClientHandler* m_handler; Ditto. > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:-288 > - if (priv->buffer) { > -#ifdef GST_API_VERSION_1 > - unmapGstBuffer(priv->buffer.get()); > -#endif > - priv->buffer.clear(); > - } Why don't we need this anymore? > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:371 > + GST_OBJECT_LOCK(src); > priv->iradioMode = g_value_get_boolean(value); > + GST_OBJECT_UNLOCK(src); I wonder if we could add a simple GstMutexLocker class, to simply the locks, you wouldn't need to unlock the object on every function return, for example. > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:439 > +// must be called on main thread and with object unlocked Comments should start with capital letter and finish with a period. > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:443 > + gboolean seeking; Declare this when first used, and use bool instead of gboolean. > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:500 > +// must be called on main thread and with object unlocked Comments should start with capital letter and finish with a period. > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:558 > + CachedResourceLoader* loader = priv->player->cachedResourceLoader(); > + if (loader) > + priv->client = new CachedResourceStreamingClient(src, loader, request); if (CachedResourceLoader* loader = priv->player->cachedResourceLoader()) priv->client = new CachedResourceStreamingClient(src, loader, request); > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:607 > + priv->startID = g_timeout_add_full(G_PRIORITY_DEFAULT, 0, (GSourceFunc) webKitWebSrcStart, gst_object_ref(src), (GDestroyNotify) gst_object_unref); You can use g_idle_add_full with G_PRIORITY_DEFAULT instead of a timeout source with 0 delay. You should use C++ style casts for GSourceFunc and GDestroyNotify. > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:615 > + priv->stopID = g_timeout_add_full(G_PRIORITY_DEFAULT, 0, (GSourceFunc) webKitWebSrcStop, gst_object_ref(src), (GDestroyNotify) gst_object_unref); Ditto. > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:638 > + if ((format == GST_FORMAT_BYTES) && (src->priv->size > 0)) { I know this was already like this, but now that you are modifying it we can take advantage to remove the extra parentheses. > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:926 > +// StreamingClientHandler This comment looks redundant to me. > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:935 > + gst_object_unref(m_src); > + m_src = 0; We should use GRefPtr for this > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1124 > +// CachedResourceStreamingClient This comment looks redundant too. > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1127 > + , m_handler(new StreamingClientHandler(src)) This should use OwnPtr and adopt the pointer here. > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1138 > + gst_object_unref(m_src); > + m_src = 0; This should use GRefPtr. I think this should be done in the parent class or the handler, but not in both places. > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1151 > + return (!m_resource); I don't think the parentheses are need here. > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1191 > +// ResourceHandleStreamingClient This comment looks also redundant. > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1194 > + , m_handler(new StreamingClientHandler(src)) Same comment about OwnPtr

Created attachment 209121 [details] Patch (In reply to comment #28) > (From update of attachment 208831 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=208831&action=review > > > Source/WebCore/ChangeLog:7 > > + Patch by Andre Moreira Magalhaes <andre.magalhaes@collabora.co.uk> on 2013-08-14 > > + Reviewed by Philippe Normand. > > Please, don't submit this as reviewed, since it contains new code that hasn't been reviewed. > Fixed. > > Source/WebCore/ChangeLog:37 > > + * platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp: > > + (_WebKitWebSrcPrivate): > > + (webkit_web_src_init): > > + (webKitWebSrcFinalize): > > + (webKitWebSrcSetProperty): > > + (webKitWebSrcGetProperty): > > + (webKitWebSrcStop): > > + (webKitWebSrcStart): > > + (webKitWebSrcChangeState): > > + (webKitWebSrcQueryWithParent): > > + (webKitWebSrcGetUri): > > + (webKitWebSrcSetUri): > > + (webKitWebSrcNeedDataMainCb): > > + (webKitWebSrcEnoughDataMainCb): > > + (webKitWebSrcSeekMainCb): > > + (webKitWebSrcSeekDataCb): > > + (webKitWebSrcSetMediaPlayer): > > + (StreamingClient::StreamingClient): > > + (StreamingClient::~StreamingClient): > > + (StreamingClient::didReceiveResponse): > > + (StreamingClient::didReceiveData): > > + (StreamingClient::didFinishLoading): > > + (StreamingClient::wasBlocked): > > + (StreamingClient::cannotShowURL): > > This doesn't look up to date. You should explain briefly here what you are changing. > Fixed. > > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:71 > > +class StreamingClientHandler { > > + public: > > + StreamingClientHandler(WebKitWebSrc*); > > + virtual ~StreamingClientHandler(); > > + > > + char* getOrCreateReadBuffer(size_t requestedSize, size_t& actualSize); > > + void responseReceived(const ResourceResponse&); > > + void dataReceived(const char*, int); > > + void notifyFinished(); > > + > > + private: > > + WebKitWebSrc* m_src; > > +}; > > I wonder if we could add the common implementation to the base class instead of using a new class. > Done, made StreamingClient a base class now. > > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:73 > > +// common StreamingClient interface > > Comments should start with capital letter and finish with a period. > Fixed, removed this one. > > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:80 > > + virtual bool loadFailed() const = 0; > > + virtual void setDefersLoading(bool) = 0; > > +}; > > So, instead of an abstract class, we could probably add the common implementation here. > Done. > > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:98 > > WebKitWebSrc* m_src; > > This is already in the handler, maybe we could add a getter to the handler, or if we end up using a base class, move this to the base class as protected member. > Done, using from base class now. > > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:99 > > + StreamingClientHandler* m_handler; > > If we use the handler this should be OwnPtr<StreamingClientHandler> > No handler anymore. > > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:124 > > + > > + WebKitWebSrc* m_src; > > + StreamingClientHandler* m_handler; > > Ditto. > Ditto :) > > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:-288 > > - if (priv->buffer) { > > -#ifdef GST_API_VERSION_1 > > - unmapGstBuffer(priv->buffer.get()); > > -#endif > > - priv->buffer.clear(); > > - } > > Why don't we need this anymore? > This is not needed. The thing here is that the element will go to NULL state before dispose, thus stop will be called and all resources freed. If we kept this code and the element dispose were called before setting the state to NULL many other members (everything reset on stop) would leak, not only the buffer. > > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:371 > > + GST_OBJECT_LOCK(src); > > priv->iradioMode = g_value_get_boolean(value); > > + GST_OBJECT_UNLOCK(src); > > I wonder if we could add a simple GstMutexLocker class, to simply the locks, you wouldn't need to unlock the object on every function return, for example. > Added a new class GMutexLocker (generic) in WTF/wtf/gobject/ as discussed with Gustavo Noronha and changed the gst webkitsrc code to use it instead. > > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:439 > > +// must be called on main thread and with object unlocked > > Comments should start with capital letter and finish with a period. > Done. > > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:443 > > + gboolean seeking; > > Declare this when first used, and use bool instead of gboolean. > Done. > > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:500 > > +// must be called on main thread and with object unlocked > > Comments should start with capital letter and finish with a period. > Done. > > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:558 > > + CachedResourceLoader* loader = priv->player->cachedResourceLoader(); > > + if (loader) > > + priv->client = new CachedResourceStreamingClient(src, loader, request); > > if (CachedResourceLoader* loader = priv->player->cachedResourceLoader()) > priv->client = new CachedResourceStreamingClient(src, loader, request); > > > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:607 > > + priv->startID = g_timeout_add_full(G_PRIORITY_DEFAULT, 0, (GSourceFunc) webKitWebSrcStart, gst_object_ref(src), (GDestroyNotify) gst_object_unref); > > You can use g_idle_add_full with G_PRIORITY_DEFAULT instead of a timeout source with 0 delay. You should use C++ style casts for GSourceFunc and GDestroyNotify. > Done. > > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:615 > > + priv->stopID = g_timeout_add_full(G_PRIORITY_DEFAULT, 0, (GSourceFunc) webKitWebSrcStop, gst_object_ref(src), (GDestroyNotify) gst_object_unref); > > Ditto. > Done, also changed other places that were using g_timeout_add to use g_idle_add. > > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:638 > > + if ((format == GST_FORMAT_BYTES) && (src->priv->size > 0)) { > > I know this was already like this, but now that you are modifying it we can take advantage to remove the extra parentheses. > Done. > > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:926 > > +// StreamingClientHandler > > This comment looks redundant to me. > Removed. > > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:935 > > + gst_object_unref(m_src); > > + m_src = 0; > > We should use GRefPtr for this > Done. > > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1124 > > +// CachedResourceStreamingClient > > This comment looks redundant too. > Removed. > > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1127 > > + , m_handler(new StreamingClientHandler(src)) > > This should use OwnPtr and adopt the pointer here. > Not needed as we dont have a handler anymore. > > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1138 > > + gst_object_unref(m_src); > > + m_src = 0; > > This should use GRefPtr. I think this should be done in the parent class or the handler, but not in both places. > Done. > > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1151 > > + return (!m_resource); > > I don't think the parentheses are need here. > Removed. > > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1191 > > +// ResourceHandleStreamingClient > > This comment looks also redundant. > Removed. > > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1194 > > + , m_handler(new StreamingClientHandler(src)) > > Same comment about OwnPtr Also not needed anymore. Thanks for the review, new patch attached. I also ran some manual tests and tested the regressions on bug #117141 and bug #117688 and everything looks fine. $ Tools/Scripts/run-webkit-tests --no-new-test-results --debug-rwt-logging --no-show-results --no-sample-on-timeout --results-directory layout-test-results --debug --webkit-test-runner --gtk fast/js/custom-constructors.html fast/js/cyclic-proto.html --iteration=100 ... 17:39:00.966 22661 Testing completed, Exit status: 0 => Results: 200/200 tests passed (100.0%) => Tests to be fixed (0): => Tests that will only be fixed if they crash (WONTFIX) (0):

Attachment 209121 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/GNUmakefile.list.am', u'Source/WTF/wtf/gobject/GMutexLocker.h', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp']" exit_code: 1 Source/WTF/wtf/gobject/GMutexLocker.h:25: Alphabetical sorting problem. [build/include_order] [4] Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:783: One line control clauses should not use braces. [whitespace/braces] [4] Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:817: One line control clauses should not use braces. [whitespace/braces] [4] Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:899: Declaration has space between type name and * in WebKitWebSrc *src [whitespace/declaration] [3] Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:920: Declaration has space between type name and * in WebKitWebSrc *src [whitespace/declaration] [3] Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1021: Declaration has space between type name and * in WebKitWebSrc *src [whitespace/declaration] [3] Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1073: Declaration has space between type name and * in WebKitWebSrc *src [whitespace/declaration] [3] Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1131: Declaration has space between type name and * in WebKitWebSrc *src [whitespace/declaration] [3] Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1197: Declaration has space between type name and * in WebKitWebSrc *src [whitespace/declaration] [3] Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1206: Declaration has space between type name and * in WebKitWebSrc *src [whitespace/declaration] [3] Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1220: Declaration has space between type name and * in WebKitWebSrc *src [whitespace/declaration] [3] Total errors found: 11 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 209128 [details] Patch Fix buildbot issues.

Attachment 209128 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/GNUmakefile.list.am', u'Source/WTF/wtf/gobject/GMutexLocker.h', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp']" exit_code: 1 Source/WTF/wtf/gobject/GMutexLocker.h:25: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 209131 [details] Patch ... and again, now Tools/Scripts/check-webkit-style returns no error.

Comment on attachment 209131 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=209131&action=review > Source/WTF/wtf/gobject/GMutexLocker.h:39 > + inline GMutexLocker() > + : m_mutex(0) > + { > + } This is unused. > Source/WTF/wtf/gobject/GMutexLocker.h:42 > + : m_mutex(0) m_mutex(mutex) > Source/WTF/wtf/gobject/GMutexLocker.h:62 > + inline void lock(GMutex *mutex) > + { > + m_mutex = mutex; > + if (m_mutex) > + g_mutex_lock(m_mutex); > + } > + > + inline void unlock() > + { > + if (m_mutex) { > + g_mutex_unlock(m_mutex); > + m_mutex = 0; > + } > + } Looks like this should be private. You have nothing preventing bad use with the current design: GMutexLocker foo; foo.lock(bar); foo.lock(baz); > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:56 > +/* Premisses: > + * - webkitsrc may be created from any thread inside gstreamer > + * - webkitsrc may be created without a player involved (e.g.: dash gstreamer element > + * internally downloading stream fragments) > + * - streaming client holds reference to src, so that src is never deleted while client exists > + * - if the src exists, appsrc also exists > + * - streaming client is created on start > + * - streaming client is deleted on stop and cancels resource > + * - streaming client callbacks are always invoked from main thread > + */ Wrong comment style. Honestly this comment does not help. It should have explanation on why your premisses matter. > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:60 > +// StreamingClient base class. Useless comment. > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:86 > + bool loadFailed() const; > + void setDefersLoading(bool); Missing Overrides. > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:93 > + char* getOrCreateReadBuffer(CachedResource*, size_t requestedSize, size_t& actualSize); > + void responseReceived(CachedResource*, const ResourceResponse&); > + void dataReceived(CachedResource*, const char*, int); > + void notifyFinished(CachedResource*); ditto. > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:106 > + bool loadFailed() const; > + void setDefersLoading(bool); ditto. > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:117 > + char* getOrCreateReadBuffer(size_t requestedSize, size_t& actualSize); > + void willSendRequest(ResourceHandle*, ResourceRequest&, const ResourceResponse&); > + void didReceiveResponse(ResourceHandle*, const ResourceResponse&); > + void didReceiveData(ResourceHandle*, const char*, int, int); > + void didFinishLoading(ResourceHandle*, double /*finishTime*/); > + void didFail(ResourceHandle*, const ResourceError&); > + void wasBlocked(ResourceHandle*); > + void cannotShowURL(ResourceHandle*); ditto. > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:492 > +// Must be called on main thread and with object unlocked. You should have assertions instead of a comment. > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:549 > + if (priv->player) { > + if (CachedResourceLoader* loader = priv->player->cachedResourceLoader()) > + priv->client = new CachedResourceStreamingClient(src, loader, request); This looks odd. If you have priv->player but no loader, you don't create the client? Is that intended?

(In reply to comment #34) > (From update of attachment 209131 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=209131&action=review > > > Source/WTF/wtf/gobject/GMutexLocker.h:39 > > + inline GMutexLocker() > > + : m_mutex(0) > > + { > > + } > > This is unused. > > > Source/WTF/wtf/gobject/GMutexLocker.h:42 > > + : m_mutex(0) > > m_mutex(mutex) > > > Source/WTF/wtf/gobject/GMutexLocker.h:62 > > + inline void lock(GMutex *mutex) > > + { > > + m_mutex = mutex; > > + if (m_mutex) > > + g_mutex_lock(m_mutex); > > + } > > + > > + inline void unlock() > > + { > > + if (m_mutex) { > > + g_mutex_unlock(m_mutex); > > + m_mutex = 0; > > + } > > + } > > Looks like this should be private. > You have nothing preventing bad use with the current design: > > GMutexLocker foo; > foo.lock(bar); > foo.lock(baz); > Tbh, I didn't want to create this class, I just did it cause Carlos requested on comment #28. We could make the "lock" method private but we need a public "unlock" method at least as there are some cases we need to unlock it manually. What do you suggest here? Should I make "lock" private? Should I revert this change and use GST_OBJECT_LOCK as it was before? Note that having only a public "unlock" method would mean that once the GMutexLocker is unlocked it cannot be reused anymore, e.g.: { GMutexLocker foo(bar); doSomethingThatRequiresLock(); foo.unlock(); doSomethingElse(); // cannot lock bar again using "foo" locker, need to create a new one. GMutexLocker foo1(bar); doSomethingThatRequiresLock(); } > > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:56 > > +/* Premisses: > > + * - webkitsrc may be created from any thread inside gstreamer > > + * - webkitsrc may be created without a player involved (e.g.: dash gstreamer elementissues > > + * internally downloading stream fragments) > > + * - streaming client holds reference to src, so that src is never deleted while client exists > > + * - if the src exists, appsrc also exists > > + * - streaming client is created on start > > + * - streaming client is deleted on stop and cancels resource > > + * - streaming client callbacks are always invoked from main thread > > + */ > > Wrong comment style. > > Honestly this comment does not help. It should have explanation on why your premisses matter. > > > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:60 > > +// StreamingClient base class. > > Useless comment. > > > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:86 > > + bool loadFailed() const; > > + void setDefersLoading(bool); > > Missing Overrides. > > > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:93 > > + char* getOrCreateReadBuffer(CachedResource*, size_t requestedSize, size_t& actualSize); > > + void responseReceived(CachedResource*, const ResourceResponse&); > > + void dataReceived(CachedResource*, const char*, int); > > + void notifyFinished(CachedResource*); > > ditto. > > > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:106 > > + bool loadFailed() const; > > + void setDefersLoading(bool); > > ditto. > > > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:117 > > + char* getOrCreateReadBuffer(size_t requestedSize, size_t& actualSize); > > + void willSendRequest(ResourceHandle*, ResourceRequest&, const ResourceResponse&); > > + void didReceiveResponse(ResourceHandle*, const ResourceResponse&); > > + void didReceiveData(ResourceHandle*, const char*, int, int); > > + void didFinishLoading(ResourceHandle*, double /*finishTime*/); > > + void didFail(ResourceHandle*, const ResourceError&); > > + void wasBlocked(ResourceHandle*); > > + void cannotShowURL(ResourceHandle*); > > ditto. > > > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:492 > > +// Must be called on main thread and with object unlocked. > > You should have assertions instead of a comment. > > > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:549 > > + if (priv->player) { > > + if (CachedResourceLoader* loader = priv->player->cachedResourceLoader()) > > + priv->client = new CachedResourceStreamingClient(src, loader, request); > > This looks odd. If you have priv->player but no loader, you don't create the client? > Is that intended? I will work on a fix for the other comments in the meantime.

Created attachment 209462 [details] GMutexLocker.h attempt (In reply to comment #35) > (In reply to comment #34) > > (From update of attachment 209131 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=209131&action=review > > > > > Source/WTF/wtf/gobject/GMutexLocker.h:39 > > > + inline GMutexLocker() > > > + : m_mutex(0) > > > + { > > > + } > > > > This is unused. > > > > > Source/WTF/wtf/gobject/GMutexLocker.h:42 > > > + : m_mutex(0) > > > > m_mutex(mutex) > > > > > Source/WTF/wtf/gobject/GMutexLocker.h:62 > > > + inline void lock(GMutex *mutex) > > > + { > > > + m_mutex = mutex; > > > + if (m_mutex) > > > + g_mutex_lock(m_mutex); > > > + } > > > + > > > + inline void unlock() > > > + { > > > + if (m_mutex) { > > > + g_mutex_unlock(m_mutex); > > > + m_mutex = 0; > > > + } > > > + } > > > > Looks like this should be private. > > You have nothing preventing bad use with the current design: > > > > GMutexLocker foo; > > foo.lock(bar); > > foo.lock(baz); > > > Tbh, I didn't want to create this class, I just did it cause Carlos requested on comment #28. > > We could make the "lock" method private but we need a public "unlock" method at least as there are some cases we need to unlock it manually. > > What do you suggest here? Should I make "lock" private? Should I revert this change and use GST_OBJECT_LOCK as it was before? > > Note that having only a public "unlock" method would mean that once the GMutexLocker is unlocked it cannot be reused anymore, e.g.: > > { > GMutexLocker foo(bar); > doSomethingThatRequiresLock(); > foo.unlock(); > > doSomethingElse(); > > // cannot lock bar again using "foo" locker, need to create a new one. > GMutexLocker foo1(bar); > doSomethingThatRequiresLock(); > } > Please check the attached GMutexLocker.h and let me know what you think and I will update the patch accordingly. The new class design is more similar to QMutexLocker.h (see http://qt.gitorious.org/qt/qt/blobs/HEAD/src/corelib/thread/qmutex.h)

Created attachment 209471 [details] Patch New patch addressing all suggestions from comment #34, including updated GMutexLocker.

Attachment 209471 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/GNUmakefile.list.am', u'Source/WTF/wtf/gobject/GMutexLocker.h', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp']" exit_code: 1 Source/WTF/wtf/gobject/GMutexLocker.h:38: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 1 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 209473 [details] Patch (In reply to comment #37) > Created an attachment (id=209471) [details] > Patch > > New patch addressing all suggestions from comment #34, including updated GMutexLocker. ... and new one passing check-style, I always forget :/.

Comment on attachment 209473 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=209473&action=review > Source/WTF/wtf/gobject/GMutexLocker.h:36 > + inline explicit GMutexLocker(GMutex *mutex) Misplaced * :) > Source/WTF/wtf/gobject/GMutexLocker.h:48 > + g_mutex_lock(m_mutex); I wonder if instead of _lock here we could _try_lock and ASSERT on failure, would it make sense?

Created attachment 209653 [details] Patch (In reply to comment #40) > (From update of attachment 209473 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=209473&action=review > > > Source/WTF/wtf/gobject/GMutexLocker.h:36 > > + inline explicit GMutexLocker(GMutex *mutex) > > Misplaced * :) > Arg, fixed, strange that check-style didn't catch it :/. > > Source/WTF/wtf/gobject/GMutexLocker.h:48 > > + g_mutex_lock(m_mutex); > > I wonder if instead of _lock here we could _try_lock and ASSERT on failure, would it make sense? hmm, I don't think we want that actually. "trylock" would just return FALSE if the mutex is already locked by another thread instead of blocking. Quoting from gmutex docs: void g_mutex_lock (GMutex *mutex); Locks mutex. If mutex is already locked by another thread, the current thread will block until mutex is unlocked by the other thread. gboolean g_mutex_trylock (GMutex *mutex); Tries to lock mutex. If mutex is already locked by another thread, it immediately returns FALSE. Otherwise it locks mutex and returns TRUE.

Comment on attachment 209653 [details] Patch Alright, thank you!

Comment on attachment 209653 [details] Patch Clearing flags on attachment: 209653 Committed r154683: <http://trac.webkit.org/changeset/154683>

All reviewed patches have been landed. Closing bug.