Bug 115351

Created attachment 200008 [details] Patch

Is the issue that GStreamer is calling ResourceHandle methods from another thread?

(In reply to comment #2) > Is the issue that GStreamer is calling ResourceHandle methods from another thread? Yes, see https://bugs.webkit.org/show_bug.cgi?id=115352#c5 for more details.

(In reply to comment #3) > (In reply to comment #2) > > Is the issue that GStreamer is calling ResourceHandle methods from another thread? > > Yes, see https://bugs.webkit.org/show_bug.cgi?id=115352#c5 for more details. We really cannot afford to push this complexity into the ResourceHandle. The GStreamer element needs to ensure that every interaction with WebCore is already on the WebCore thread.

(In reply to comment #4) > (In reply to comment #3) > > (In reply to comment #2) > > > Is the issue that GStreamer is calling ResourceHandle methods from another thread? > > > > Yes, see https://bugs.webkit.org/show_bug.cgi?id=115352#c5 for more details. > > We really cannot afford to push this complexity into the ResourceHandle. The GStreamer element needs to ensure that every interaction with WebCore is already on the WebCore thread. I am not sure why this is the case as the callbacks are already called asynchronously when there isn't a deferred result. What I see here is an inconsistency in the ResourceHandleSoup where it sometimes invokes the callbacks synchronously and sometimes asynchronously. The problem is not the gst player calling this from a different thread (WebCore methods are called from main thread), the problem is that the methods calling setDefersLoading is protected by a mutex and the same is true for the callbacks for data IIRC, thus invoking the data callback when calling setDefersLoading causes a deadlock in the gst source element. The same method having 2 different behaviours looks wrong to me, but feel free to reject here if you don't agree with the change.

(In reply to comment #5) > I am not sure why this is the case as the callbacks are already called asynchronously when there isn't a deferred result. What I see here is an inconsistency in the ResourceHandleSoup where it sometimes invokes the callbacks synchronously and sometimes asynchronously. The problem is not the gst player calling this from a different thread (WebCore methods are called from main thread), the problem is that the methods calling setDefersLoading is protected by a mutex and the same is true for the callbacks for data IIRC, thus invoking the data callback when calling setDefersLoading causes a deadlock in the gst source element. Hrm. I misunderstood the problem originally, but I think I understand it now. The restriction that data flow needs to happen asynchronously after calling arbitrary methods on ResourceHandle does seem like an arbitrary requirement that's particular to the GStreamer backend. These kind of unspoken contracts make it hard to refactor ResourceHandle, for instance. It seems that the media code should make as few assumptions about how the ResourceHandle works as possible (within reason, of course). The class that maintains the mutex should know when it has already acquired it or alternatively it could use recursive mutexes, if they aren't too expensive. I'm sympathetic to the desire to fix the problem here and the idea that undeferring loading should always trigger data asynchronously, but I think that should be a design choice and not part of the contract. Thus, this should probably be handled in the media layer.

(In reply to comment #6) > (In reply to comment #5) > > > I am not sure why this is the case as the callbacks are already called asynchronously when there isn't a deferred result. What I see here is an inconsistency in the ResourceHandleSoup where it sometimes invokes the callbacks synchronously and sometimes asynchronously. The problem is not the gst player calling this from a different thread (WebCore methods are called from main thread), the problem is that the methods calling setDefersLoading is protected by a mutex and the same is true for the callbacks for data IIRC, thus invoking the data callback when calling setDefersLoading causes a deadlock in the gst source element. > > Hrm. I misunderstood the problem originally, but I think I understand it now. The restriction that data flow needs to happen asynchronously after calling arbitrary methods on ResourceHandle does seem like an arbitrary requirement that's particular to the GStreamer backend. These kind of unspoken contracts make it hard to refactor ResourceHandle, for instance. It seems that the media code should make as few assumptions about how the ResourceHandle works as possible (within reason, of course). > > The class that maintains the mutex should know when it has already acquired it or alternatively it could use recursive mutexes, if they aren't too expensive. I'm sympathetic to the desire to fix the problem here and the idea that undeferring loading should always trigger data asynchronously, but I think that should be a design choice and not part of the contract. Thus, this should probably be handled in the media layer. Agree that this should be handled by the gstreamer backend itself. The problem is that the resourceHandle member itself needs to be protected by the mutex (to check it the pointer is still valid) as it can be unrefed from a different thread or something like that (I wrote this patch some time ago and don't remember the details). The same mutex is used here for both the resourceHandle and the data callback but I am sure there are other alternatives. In any case I believe this patch should be applied so that the setDefersLoading method always have the same behaviour, but this is up to you guys. Note that this only applies to the Soup resource handle.

(In reply to comment #7) > Agree that this should be handled by the gstreamer backend itself. The problem is that the resourceHandle member itself needs to be protected by the mutex (to check it the pointer is still valid) as it can be unrefed from a different thread or something like that (I wrote this patch some time ago and don't remember the details). The same mutex is used here for both the resourceHandle and the data callback but I am sure there are other alternatives. In any case I believe this patch should be applied so that the setDefersLoading method always have the same behaviour, but this is up to you guys. Note that this only applies to the Soup resource handle. ResourceHandle should only be used from a single-thread (the WebCore thread) as far as I know, so I imagine this is the root of the problem.

(In reply to comment #8) > (In reply to comment #7) > > > Agree that this should be handled by the gstreamer backend itself. The problem is that the resourceHandle member itself needs to be protected by the mutex (to check it the pointer is still valid) as it can be unrefed from a different thread or something like that (I wrote this patch some time ago and don't remember the details). The same mutex is used here for both the resourceHandle and the data callback but I am sure there are other alternatives. In any case I believe this patch should be applied so that the setDefersLoading method always have the same behaviour, but this is up to you guys. Note that this only applies to the Soup resource handle. > > ResourceHandle should only be used from a single-thread (the WebCore thread) as far as I know, so I imagine this is the root of the problem. The new patch from 115352 doesn't require this patch anymore, feel free to reject it if you feel like it, but as I said, I still believe setDefersLoading should always have the same behaviour (async).

Marking as WONTFIX as the patch is not required.

Comment on attachment 200008 [details] Patch Removing review request.