Bug 115351 - [Soup] Make sure ResourceHandleSoup::platformSetDefersLoading() does not call ResourceHandleClient callbacks synchronously
Summary: [Soup] Make sure ResourceHandleSoup::platformSetDefersLoading() does not call...
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-04-29 07:35 PDT by Andre Moreira Magalhaes
Modified: 2013-08-15 06:43 PDT (History)
6 users (show)

See Also:


Attachments
Patch (4.25 KB, patch)
2013-04-29 07:42 PDT, Andre Moreira Magalhaes
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andre Moreira Magalhaes 2013-04-29 07:35:21 PDT
ResourceHandleSoup::platformSetDefersLoading() per default calls the client callbacks asynchronously, unless there is a deferred result pending, in which case the client callbacks are called immediately. We should make sure ResourceHandleSoup::platformSetDefersLoading() never invokes client callbacks synchronously to avoid a possible deadlock on the gstreamer source element.

Patch to come next.
Comment 1 Andre Moreira Magalhaes 2013-04-29 07:42:58 PDT
Created attachment 200008 [details]
Patch
Comment 2 Martin Robinson 2013-04-29 07:57:16 PDT
Is the issue that GStreamer is calling ResourceHandle methods from another thread?
Comment 3 Andre Moreira Magalhaes 2013-04-29 08:28:26 PDT
(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.
Comment 4 Martin Robinson 2013-04-29 08:38:13 PDT
(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.
Comment 5 Andre Moreira Magalhaes 2013-04-29 09:00:24 PDT
(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.
Comment 6 Martin Robinson 2013-04-29 09:15:39 PDT
(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.
Comment 7 Andre Moreira Magalhaes 2013-04-29 09:34:49 PDT
(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.
Comment 8 Martin Robinson 2013-04-29 09:53:38 PDT
(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.
Comment 9 Andre Moreira Magalhaes 2013-04-29 10:14:27 PDT
(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).
Comment 10 Andre Moreira Magalhaes 2013-08-15 06:43:12 PDT
Marking as WONTFIX as the patch is not required.
Comment 11 Andre Moreira Magalhaes 2013-08-15 06:43:33 PDT
Comment on attachment 200008 [details]
Patch

Removing review request.