115351 2013-04-29 07:35:21 -0700 [Soup] Make sure ResourceHandleSoup::platformSetDefersLoading() does not call ResourceHandleClient callbacks synchronously 2013-08-15 06:43:33 -0700 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) Unspecified Unspecified RESOLVED WONTFIX P2 Normal --- 0 andrunko webkit-unassigned commit-queue danw darin gustavo mrobinson rakuco oldest_to_newest 882555 0 andrunko 2013-04-29 07:35:21 -0700 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. 882558 1 200008 andrunko 2013-04-29 07:42:58 -0700 Created attachment 200008 Patch 882571 2 mrobinson 2013-04-29 07:57:16 -0700 Is the issue that GStreamer is calling ResourceHandle methods from another thread? 882596 3 andrunko 2013-04-29 08:28:26 -0700 (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. 882605 4 mrobinson 2013-04-29 08:38:13 -0700 (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. 882624 5 andrunko 2013-04-29 09:00:24 -0700 (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. 882632 6 mrobinson 2013-04-29 09:15:39 -0700 (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. 882644 7 andrunko 2013-04-29 09:34:49 -0700 (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. 882653 8 mrobinson 2013-04-29 09:53:38 -0700 (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. 882663 9 andrunko 2013-04-29 10:14:27 -0700 (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). 917497 10 andrunko 2013-08-15 06:43:12 -0700 Marking as WONTFIX as the patch is not required. 917498 11 200008 andrunko 2013-08-15 06:43:33 -0700 Comment on attachment 200008 Patch Removing review request. 200008 2013-04-29 07:42:58 -0700 2013-08-15 06:43:33 -0700 Patch bug-115351-20130429154204.patch text/plain 4351 andrunko U3VidmVyc2lvbiBSZXZpc2lvbjogMTQ4NjcxCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggY2U1MDk4NzVjZGVlMWQ3 NzJkMzI3ZTQ1MzQyZWQzOTQ4MTA3OGY0My4uNjJhY2ExMjM2MmVmNjUyZTYzZThmOGY1ZmNiZjYz ZDcyYWE3YzJjYyAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDI2IEBACisyMDEzLTA0LTI5ICBBbmRy ZSBNb3JlaXJhIE1hZ2FsaGFlcyAgIDxhbmRyZS5tYWdhbGhhZXNAY29sbGFib3JhLmNvLnVrPgor CisgICAgICAgIE1ha2Ugc3VyZSBSZXNvdXJjZUhhbmRsZVNvdXA6OnBsYXRmb3JtU2V0RGVmZXJz TG9hZGluZygpIGRvZXMgbm90IGNhbGwgUmVzb3VyY2VIYW5kbGVDbGllbnQgY2FsbGJhY2tzIHN5 bmNocm9ub3VzbHkuCisKKyAgICAgICAgUmVzb3VyY2VIYW5kbGVTb3VwOjpwbGF0Zm9ybVNldERl ZmVyc0xvYWRpbmcoKSBwZXIgZGVmYXVsdCBjYWxscyB0aGUgY2xpZW50IGNhbGxiYWNrcworICAg ICAgICBhc3luY2hyb25vdXNseSwgdW5sZXNzIHRoZXJlIGlzIGEgZGVmZXJyZWQgcmVzdWx0IHBl bmRpbmcsIGluIHdoaWNoIGNhc2UgdGhlIGNsaWVudCBjYWxsYmFja3MgYXJlCisgICAgICAgIGNh bGxlZCBpbW1lZGlhdGVseS4gVGhpcyBwYXRjaCBtYWtlcyBzdXJlIFJlc291cmNlSGFuZGxlU291 cDo6cGxhdGZvcm1TZXREZWZlcnNMb2FkaW5nKCkgbmV2ZXIKKyAgICAgICAgaW52b2tlcyBjbGll bnQgY2FsbGJhY2tzIHN5bmNocm9ub3VzbHkgdG8gYXZvaWQgYSBwb3NzaWJsZSBkZWFkbG9jayBv biB0aGUgZ3N0cmVhbWVyIHNvdXJjZSBlbGVtZW50LgorCisgICAgICAgIE1ha2Ugc3VyZSBSZXNv dXJjZUhhbmRsZVNvdXA6OnBsYXRmb3JtU2V0RGVmZXJzTG9hZGluZygpIGRvZXMgbm90IGNhbGwg UmVzb3VyY2VIYW5kbGVDbGllbnQgY2FsbGJhY2tzIHN5bmNocm9ub3VzbHkKKyAgICAgICAgaHR0 cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTExNTM1MQorCisgICAgICAgIFJl dmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIE5vIG5ldyB0ZXN0cyAoT09QUyEp LgorCisgICAgICAgICogcGxhdGZvcm0vbmV0d29yay9zb3VwL1Jlc291cmNlSGFuZGxlU291cC5j cHA6CisgICAgICAgIChXZWJDb3JlOjpSZXNvdXJjZUhhbmRsZTo6c2VuZFBlbmRpbmdSZXF1ZXN0 KToKKyAgICAgICAgKF9Bc3luY1Jlc3VsdENhbGxiYWNrRGF0YSk6CisgICAgICAgIChXZWJDb3Jl KToKKyAgICAgICAgKFdlYkNvcmU6OnNlbmRBc3luY1Jlc3VsdENhbGxiYWNrKToKKyAgICAgICAg KFdlYkNvcmU6OlJlc291cmNlSGFuZGxlOjpwbGF0Zm9ybVNldERlZmVyc0xvYWRpbmcpOgorCiAy MDEzLTA0LTE4ICBTZW9ranUgS3dvbiAgPHNlb2tqdS5rd29uQGdtYWlsLmNvbT4KIAogICAgICAg ICBXZWIgSW5zcGVjdG9yOiBUb29sYmFyIGljb25zIGFyZSBkaXNwbGF5ZWQgaW5jb3JyZWN0bHkK ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL25ldHdvcmsvc291cC9SZXNvdXJj ZUhhbmRsZVNvdXAuY3BwIGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vbmV0d29yay9zb3VwL1Jl c291cmNlSGFuZGxlU291cC5jcHAKaW5kZXggNjIxNDBhMjJhMTc4YjU4Y2E1YTAzYTFiODQzYWY2 MWU2OTgyYjcxNy4uNDc2NjNhZjE4YWRlMzZkNDY2ODFiYzNjNWNlN2JiMGI4ZDg3NGM4YSAxMDA2 NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vbmV0d29yay9zb3VwL1Jlc291cmNlSGFu ZGxlU291cC5jcHAKKysrIGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vbmV0d29yay9zb3VwL1Jl c291cmNlSGFuZGxlU291cC5jcHAKQEAgLTEwMzYsNiArMTAzNiw5IEBAIGJvb2wgUmVzb3VyY2VI YW5kbGU6OnN0YXJ0KCkKIAogdm9pZCBSZXNvdXJjZUhhbmRsZTo6c2VuZFBlbmRpbmdSZXF1ZXN0 KCkKIHsKKyAgICBpZiAoIWQtPm1fc291cFJlcXVlc3QpCisgICAgICAgIHJldHVybjsKKwogI2lm IEVOQUJMRShXRUJfVElNSU5HKQogICAgIGlmIChkLT5tX3Jlc3BvbnNlLnJlc291cmNlTG9hZFRp bWluZygpKQogICAgICAgICBkLT5tX3Jlc3BvbnNlLnJlc291cmNlTG9hZFRpbWluZygpLT5yZXF1 ZXN0VGltZSA9IG1vbm90b25pY2FsbHlJbmNyZWFzaW5nVGltZSgpOwpAQCAtMTIxOCw2ICsxMjIx LDI3IEBAIHZvaWQgUmVzb3VyY2VIYW5kbGU6OnJlY2VpdmVkQ2FuY2VsbGF0aW9uKGNvbnN0IEF1 dGhlbnRpY2F0aW9uQ2hhbGxlbmdlJiBjaGFsbGVuCiAgICAgY2xlYXJBdXRoZW50aWNhdGlvbigp OwogfQogCitzdHJ1Y3QgX0FzeW5jUmVzdWx0Q2FsbGJhY2tEYXRhIHsKKyAgICBSZWZQdHI8UmVz b3VyY2VIYW5kbGU+IGhhbmRsZTsKKyAgICBHUmVmUHRyPEdBc3luY1Jlc3VsdD4gYXN5bmNSZXN1 bHQ7Cit9OwordHlwZWRlZiBzdHJ1Y3QgX0FzeW5jUmVzdWx0Q2FsbGJhY2tEYXRhIEFzeW5jUmVz dWx0Q2FsbGJhY2tEYXRhOworCitzdGF0aWMgdm9pZCBzZW5kQXN5bmNSZXN1bHRDYWxsYmFjayhH T2JqZWN0KiBvYmplY3QsIEdBc3luY1Jlc3VsdCogcmVzLCBncG9pbnRlciBkYXRhKQoreworICAg IEFzeW5jUmVzdWx0Q2FsbGJhY2tEYXRhKiBjYWxsYmFja0RhdGEgPSBzdGF0aWNfY2FzdDxBc3lu Y1Jlc3VsdENhbGxiYWNrRGF0YSo+KGRhdGEpOworCisgICAgUmVmUHRyPFJlc291cmNlSGFuZGxl PiBoYW5kbGUgPSBjYWxsYmFja0RhdGEtPmhhbmRsZTsKKyAgICBSZXNvdXJjZUhhbmRsZUludGVy bmFsKiBkID0gaGFuZGxlLT5nZXRJbnRlcm5hbCgpOworICAgIEdSZWZQdHI8R0FzeW5jUmVzdWx0 PiBhc3luY1Jlc3VsdCA9IGFkb3B0R1JlZihjYWxsYmFja0RhdGEtPmFzeW5jUmVzdWx0LmxlYWtS ZWYoKSk7CisKKyAgICBpZiAoZC0+bV9pbnB1dFN0cmVhbSkKKyAgICAgICAgcmVhZENhbGxiYWNr KG9iamVjdCwgYXN5bmNSZXN1bHQuZ2V0KCksIGhhbmRsZS5nZXQoKSk7CisgICAgZWxzZQorICAg ICAgICBzZW5kUmVxdWVzdENhbGxiYWNrKG9iamVjdCwgYXN5bmNSZXN1bHQuZ2V0KCksIGhhbmRs ZS5nZXQoKSk7CisgICAgZGVsZXRlIGNhbGxiYWNrRGF0YTsKK30KKwogc3RhdGljIGJvb2wgd2Fp dGluZ1RvU2VuZFJlcXVlc3QoUmVzb3VyY2VIYW5kbGUqIGhhbmRsZSkKIHsKICAgICAvLyBXZSBu ZWVkIHRvIGNoZWNrIGZvciBkLT5tX3NvdXBSZXF1ZXN0IGJlY2F1c2UgdGhlIHJlcXVlc3QgbWF5 IGhhdmUgcmFpc2VkIGEgZmFpbHVyZQpAQCAtMTI0OCwxMCArMTI3MiwxOSBAQCB2b2lkIFJlc291 cmNlSGFuZGxlOjpwbGF0Zm9ybVNldERlZmVyc0xvYWRpbmcoYm9vbCBkZWZlcnNMb2FkaW5nKQog ICAgIGlmIChkLT5tX2RlZmVycmVkUmVzdWx0KSB7CiAgICAgICAgIEdSZWZQdHI8R0FzeW5jUmVz dWx0PiBhc3luY1Jlc3VsdCA9IGFkb3B0R1JlZihkLT5tX2RlZmVycmVkUmVzdWx0LmxlYWtSZWYo KSk7CiAKKyAgICAgICAgQXN5bmNSZXN1bHRDYWxsYmFja0RhdGEqIGNhbGxiYWNrRGF0YSA9IG5l dyBBc3luY1Jlc3VsdENhbGxiYWNrRGF0YTsKKyAgICAgICAgY2FsbGJhY2tEYXRhLT5oYW5kbGUg PSB0aGlzOworICAgICAgICBjYWxsYmFja0RhdGEtPmFzeW5jUmVzdWx0ID0gYXN5bmNSZXN1bHQ7 CisKKyAgICAgICAgR1NpbXBsZUFzeW5jUmVzdWx0KiBzaW1wbGU7CiAgICAgICAgIGlmIChkLT5t X2lucHV0U3RyZWFtKQotICAgICAgICAgICAgcmVhZENhbGxiYWNrKEdfT0JKRUNUKGQtPm1faW5w dXRTdHJlYW0uZ2V0KCkpLCBhc3luY1Jlc3VsdC5nZXQoKSwgdGhpcyk7CisgICAgICAgICAgICBz aW1wbGUgPSBnX3NpbXBsZV9hc3luY19yZXN1bHRfbmV3KEdfT0JKRUNUKGQtPm1faW5wdXRTdHJl YW0uZ2V0KCkpLAorICAgICAgICAgICAgICAgIHNlbmRBc3luY1Jlc3VsdENhbGxiYWNrLCBjYWxs YmFja0RhdGEsIDApOwogICAgICAgICBlbHNlCi0gICAgICAgICAgICBzZW5kUmVxdWVzdENhbGxi YWNrKEdfT0JKRUNUKGQtPm1fc291cFJlcXVlc3QuZ2V0KCkpLCBhc3luY1Jlc3VsdC5nZXQoKSwg dGhpcyk7CisgICAgICAgICAgICBzaW1wbGUgPSBnX3NpbXBsZV9hc3luY19yZXN1bHRfbmV3KEdf T0JKRUNUKGQtPm1fc291cFJlcXVlc3QuZ2V0KCkpLAorICAgICAgICAgICAgICAgIHNlbmRBc3lu Y1Jlc3VsdENhbGxiYWNrLCBjYWxsYmFja0RhdGEsIDApOworICAgICAgICBnX3NpbXBsZV9hc3lu Y19yZXN1bHRfc2V0X2NoZWNrX2NhbmNlbGxhYmxlKHNpbXBsZSwgZC0+bV9jYW5jZWxsYWJsZS5n ZXQoKSk7CisgICAgICAgIGdfc2ltcGxlX2FzeW5jX3Jlc3VsdF9jb21wbGV0ZV9pbl9pZGxlKHNp bXBsZSk7CiAgICAgfQogfQogCg==