112977 2013-03-21 16:30:11 -0700 Crash in WebCore::MediaPlayer::cachedResourceLoader + 4 2013-03-21 22:54:17 -0700 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 jer.noble jer.noble eric.carlson feature-media-reviews jonlee webkit.review.bot oldest_to_newest 860852 0 jer.noble 2013-03-21 16:30:11 -0700 Crash in WebCore::MediaPlayer::cachedResourceLoader + 4 860889 1 194388 jer.noble 2013-03-21 16:56:39 -0700 Created attachment 194388 Patch 860890 2 jer.noble 2013-03-21 16:57:28 -0700 <rdar://problem/13435161> 861088 3 194388 ggaren 2013-03-21 22:01:34 -0700 Comment on attachment 194388 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=194388&action=review I don't think this can be right. > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:238 > + [m_loaderDelegate.get() setCallback:0]; If there is a race condition between A and B, by definition there is a race condition between A and every unsynchronized subset of B. Therefore, you can't solve a race condition between AVAssetResourceLoader and ~MediaPlayerPrivateAVFoundationObjC() by adding a line of code to ~MediaPlayerPrivateAVFoundationObjC(), unless that line of code locks a mutex. Also, setting m_loaderDelegate's callback to 0 right before deallocating it is dubious. This is only meaningful if you rely on the value stored to that memory after that memory is freed. 861099 4 194388 ggaren 2013-03-21 22:18:21 -0700 Comment on attachment 194388 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=194388&action=review > Source/WebCore/ChangeLog:13 > + been deleted, and the delegate's m_callback pointer is not pointing at freed memory. Typo: should be "now". > Source/WebCore/ChangeLog:17 > + m_callback ivar, to avoid calling into freed memory for already in-process delegate callbacks. Instead of saying "already in-process" here, let's say "already queued". If the callbacks were truly in-process, this fix wouldn't work. >> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:238 >> + [m_loaderDelegate.get() setCallback:0]; > > If there is a race condition between A and B, by definition there is a race condition between A and every unsynchronized subset of B. Therefore, you can't solve a race condition between AVAssetResourceLoader and ~MediaPlayerPrivateAVFoundationObjC() by adding a line of code to ~MediaPlayerPrivateAVFoundationObjC(), unless that line of code locks a mutex. > > Also, setting m_loaderDelegate's callback to 0 right before deallocating it is dubious. This is only meaningful if you rely on the value stored to that memory after that memory is freed. I take it back: This works because the delegate queue is the main queue we're running on, and that acts as synchronization. 861103 5 jer.noble 2013-03-21 22:27:21 -0700 (In reply to comment #3) > (From update of attachment 194388 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=194388&action=review > > I don't think this can be right. > > > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:238 > > + [m_loaderDelegate.get() setCallback:0]; > > If there is a race condition between A and B, by definition there is a race condition between A and every unsynchronized subset of B. Therefore, you can't solve a race condition between AVAssetResourceLoader and ~MediaPlayerPrivateAVFoundationObjC() by adding a line of code to ~MediaPlayerPrivateAVFoundationObjC(), unless that line of code locks a mutex. The synchronization happens due to AVAssetResourceLoader calling dispatch_sync() to the main thread. m_loaderDelegate is retained in CoreMedia's thread, and not released until that dispatch_sync() returns. So while m_loaderDelegate's retain count is modified from both threads, m_loaderDelegate->m_callback is only accessed from the main thread (from within the dispatch_sync), which is why it is safe to modify its value without a lock. Previously, MediaPlayerPrivateAVFoundationObjC was destroyed in one runloop, and the object pointed to by m_loaderDelegate was accessed (by CoreMedia) in the next runloop. With this change, that will still happen, but m_loaderDelegate will no longer have a stale pointer to a (destroyed) MediaPlayerPrivateAVFoundationObjC. > Also, setting m_loaderDelegate's callback to 0 right before deallocating it is dubious. This is only meaningful if you rely on the value stored to that memory after that memory is freed. Not really. Your statement would be correct if AVAssetResourceLoader did not retain m_loaderDelegate. Since it does retain m_loaderDelegate, it will be accessing valid memory when it calls into -resourceLoader: shouldWaitForLoadingOfRequestedResource:. 861104 6 jer.noble 2013-03-21 22:30:02 -0700 (In reply to comment #4) > (From update of attachment 194388 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=194388&action=review > > I take it back: This works because the delegate queue is the main queue we're running on, and that acts as synchronization. Whew! > Typo: should be "now". Fixed. > > Source/WebCore/ChangeLog:17 > > + m_callback ivar, to avoid calling into freed memory for already in-process delegate callbacks. > > instead of saying "already in-process" here, let's say "already queued". If the callbacks were truly in- process, this fix wouldn't work. Sure thing. Thanks! 861119 7 jer.noble 2013-03-21 22:54:17 -0700 Committed r146563: <http://trac.webkit.org/changeset/146563> 194388 2013-03-21 16:56:39 -0700 2013-03-21 22:18:20 -0700 Patch bug-112977-20130321165227.patch text/plain 4052 jer.noble U3VidmVyc2lvbiBSZXZpc2lvbjogMTQ2NDc2CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggZmRkNWU2NmIyYjM4ODEz ZTJmYzMxMWQxNmE1YjU4NzU0MjM3YzdkYi4uZTM0ZGUxNTk0MDJlN2RhMTI0ZDQzM2Q0NGExNTkx YWJjNWJhNGU2NyAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDI3IEBACisyMDEzLTAzLTIxICBKZXIg Tm9ibGUgIDxqZXIubm9ibGVAYXBwbGUuY29tPgorCisgICAgICAgIENyYXNoIGluIFdlYkNvcmU6 Ok1lZGlhUGxheWVyOjpjYWNoZWRSZXNvdXJjZUxvYWRlciArIDQKKyAgICAgICAgaHR0cHM6Ly9i dWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTExMjk3NworCisgICAgICAgIFJldmlld2Vk IGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFNwZWN1bGF0aXZlIGZpeCBmb3IgYSBOVUxM LWRlcmVmZXJlbmNlIGNyYXNoLiBNZWRpYVBsYXllclByaXZhdGVBVkZvdW5kYXRpb25PYmpDIGlz IHRoZQorICAgICAgICBzb2xlIG93bmVyIG9mIGEgV2ViQ29yZUFWRkxvYWRlckRlbGVnYXRlIGlu c3RhbmNlLiBJdCByZWxlYXNlcyB0aGlzIGluc3RhbmNlIGluIGl0cyBkZXN0cnVjdG9yLAorICAg ICAgICBidXQgaXQgaXMgcG9zc2libGUgdGhhdCwgb24gYW5vdGhlciB0aHJlYWQsIHRoZSBBVkFz c2V0UmVzb3VyY2VMb2FkZXIgaGFzIGFscmVhZHkgYmVndW4KKyAgICAgICAgdXNpbmcgdGhlIGRl bGVnYXRlIGFuZCBpbiBzbyBkb2luZyBoYXMgcmV0YWluZWQgaXQuIEJ5IHRoZSB0aW1lIHRoZSBk ZWxlZ2F0ZSBtZXRob2QgaXMgZmlyZWQKKyAgICAgICAgb24gdGhlIG1haW4gdGhyZWFkLCB0aGUg TWVkaWFQbGF5ZXJQcml2YXRlQVZGb3VuZGF0aW9uT2JqQyBvd25lciBvZiB0aGUgZGVsZWdhdGUg aGFzIGFscmVhZHkKKyAgICAgICAgYmVlbiBkZWxldGVkLCBhbmQgdGhlIGRlbGVnYXRlJ3MgbV9j YWxsYmFjayBwb2ludGVyIGlzIG5vdCBwb2ludGluZyBhdCBmcmVlZCBtZW1vcnkuCisKKyAgICAg ICAgSW4gYWRkaXRpb24gdG8gY2FsbGluZyAtW0FWQXNzZXRSZXNvdXJjZUxvYWRlciBzZXREZWxl Z2F0ZTpxdWV1ZTpdIHRvIGF2b2lkIGFueSBub3QteWV0LXN0YXJ0ZWQKKyAgICAgICAgZGVsZWdh dGUgY2FsbGJhY2tzLCBNZWRpYVBsYXllclByaXZhdGVBVkZvdW5kYXRpb25PYmpDIHNob3VsZCBj bGVhciB0aGUgV2ViQ29yZUFWRkxvYWRlckRlbGVnYXRlCisgICAgICAgIG1fY2FsbGJhY2sgaXZh ciwgdG8gYXZvaWQgY2FsbGluZyBpbnRvIGZyZWVkIG1lbW9yeSBmb3IgYWxyZWFkeSBpbi1wcm9j ZXNzIGRlbGVnYXRlIGNhbGxiYWNrcy4KKworICAgICAgICAqIHBsYXRmb3JtL2dyYXBoaWNzL2F2 Zm91bmRhdGlvbi9vYmpjL01lZGlhUGxheWVyUHJpdmF0ZUFWRm91bmRhdGlvbk9iakMubW06Cisg ICAgICAgIChXZWJDb3JlOjpNZWRpYVBsYXllclByaXZhdGVBVkZvdW5kYXRpb25PYmpDOjp+TWVk aWFQbGF5ZXJQcml2YXRlQVZGb3VuZGF0aW9uT2JqQyk6IENsZWFyIHRoZSBtX2xvYWRlckRlbGVn YXRlJ3MgY2FsbGJhY2sgcG9pbnRlci4KKyAgICAgICAgKC1bV2ViQ29yZUFWRkxvYWRlckRlbGVn YXRlIHJlc291cmNlTG9hZGVyOnNob3VsZFdhaXRGb3JMb2FkaW5nT2ZSZXF1ZXN0ZWRSZXNvdXJj ZTpdKTogQ2hlY2sgdGhlIHZhbHVlIG9mIG1fY2FsbGJhY2sgYmVmb3JlIGNvbnRpbnVpbmcuCisg ICAgICAgICgtW1dlYkNvcmVBVkZMb2FkZXJEZWxlZ2F0ZSByZXNvdXJjZUxvYWRlcjpkaWRDYW5j ZWxMb2FkaW5nUmVxdWVzdDpdKTogRGl0dG8uCisgICAgICAgICgtW1dlYkNvcmVBVkZMb2FkZXJE ZWxlZ2F0ZSBzZXRDYWxsYmFjazpdKTogQWRkZWQgc2ltcGxlIHNldHRlci4KKwogMjAxMy0wMy0x OCAgQW5kcmV5IEtvc3lha292ICA8Y2FzZXFAY2hyb21pdW0ub3JnPgogCiAgICAgICAgIFdlYiBJ bnNwZWN0b3I6IFBhaW50IHJlY3RhbmdsZXMgc2hvd24gb24gaG92ZXIgb3ZlciBUaW1lbGluZSdz IHBhaW50IHJlY29yZHMgb2NjYXNpb25hbGx5IGhhdmUgd3Jvbmcgb2Zmc2V0cwpkaWZmIC0tZ2l0 IGEvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvYXZmb3VuZGF0aW9uL29iamMvTWVk aWFQbGF5ZXJQcml2YXRlQVZGb3VuZGF0aW9uT2JqQy5tbSBiL1NvdXJjZS9XZWJDb3JlL3BsYXRm b3JtL2dyYXBoaWNzL2F2Zm91bmRhdGlvbi9vYmpjL01lZGlhUGxheWVyUHJpdmF0ZUFWRm91bmRh dGlvbk9iakMubW0KaW5kZXggNGIzZGM5ZjA1NTRmYWFiMjZiMzUzYjU3Y2M0ZmEyNGQ3MjE3MTZl OC4uY2I1YzIxMDM5MGQwZWI5NTQwOWI3ZGY3MjQxMzA3YmYzZDA1MzFkNiAxMDA2NDQKLS0tIGEv U291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvYXZmb3VuZGF0aW9uL29iamMvTWVkaWFQ bGF5ZXJQcml2YXRlQVZGb3VuZGF0aW9uT2JqQy5tbQorKysgYi9Tb3VyY2UvV2ViQ29yZS9wbGF0 Zm9ybS9ncmFwaGljcy9hdmZvdW5kYXRpb24vb2JqYy9NZWRpYVBsYXllclByaXZhdGVBVkZvdW5k YXRpb25PYmpDLm1tCkBAIC0xNzEsNiArMTcxLDcgQEAgZW51bSBNZWRpYVBsYXllckFWRm91bmRh dGlvbk9ic2VydmF0aW9uQ29udGV4dCB7CiB9CiAtIChpZClpbml0V2l0aENhbGxiYWNrOihNZWRp YVBsYXllclByaXZhdGVBVkZvdW5kYXRpb25PYmpDKiljYWxsYmFjazsKIC0gKEJPT0wpcmVzb3Vy Y2VMb2FkZXI6KEFWQXNzZXRSZXNvdXJjZUxvYWRlciAqKXJlc291cmNlTG9hZGVyIHNob3VsZFdh aXRGb3JMb2FkaW5nT2ZSZXF1ZXN0ZWRSZXNvdXJjZTooQVZBc3NldFJlc291cmNlTG9hZGluZ1Jl cXVlc3QgKilsb2FkaW5nUmVxdWVzdDsKKy0gKHZvaWQpc2V0Q2FsbGJhY2s6KE1lZGlhUGxheWVy UHJpdmF0ZUFWRm91bmRhdGlvbk9iakMqKWNhbGxiYWNrOwogQGVuZAogI2VuZGlmCiAKQEAgLTIz NCw2ICsyMzUsNyBAQCBNZWRpYVBsYXllclByaXZhdGVBVkZvdW5kYXRpb25PYmpDOjp+TWVkaWFQ bGF5ZXJQcml2YXRlQVZGb3VuZGF0aW9uT2JqQygpCiAgICAgcGxheWVyVG9Qcml2YXRlTWFwKCku cmVtb3ZlKHBsYXllcigpKTsKICNlbmRpZgogI2lmIF9fTUFDX09TX1hfVkVSU0lPTl9NSU5fUkVR VUlSRUQgPj0gMTA5MAorICAgIFttX2xvYWRlckRlbGVnYXRlLmdldCgpIHNldENhbGxiYWNrOjBd OwogICAgIFtbbV9hdkFzc2V0LmdldCgpIHJlc291cmNlTG9hZGVyXSBzZXREZWxlZ2F0ZTpuaWwg cXVldWU6MF07CiAjZW5kaWYKICAgICBjYW5jZWxMb2FkKCk7CkBAIC0xNTMxLDE0ICsxNTMzLDI1 IEBAIE5TQXJyYXkqIGl0ZW1LVk9Qcm9wZXJ0aWVzKCkKIC0gKEJPT0wpcmVzb3VyY2VMb2FkZXI6 KEFWQXNzZXRSZXNvdXJjZUxvYWRlciAqKXJlc291cmNlTG9hZGVyIHNob3VsZFdhaXRGb3JMb2Fk aW5nT2ZSZXF1ZXN0ZWRSZXNvdXJjZTooQVZBc3NldFJlc291cmNlTG9hZGluZ1JlcXVlc3QgKils b2FkaW5nUmVxdWVzdAogewogICAgIFVOVVNFRF9QQVJBTShyZXNvdXJjZUxvYWRlcik7CisgICAg aWYgKCFtX2NhbGxiYWNrKQorICAgICAgICByZXR1cm4gTk87CisKICAgICByZXR1cm4gbV9jYWxs YmFjay0+c2hvdWxkV2FpdEZvckxvYWRpbmdPZlJlc291cmNlKGxvYWRpbmdSZXF1ZXN0KTsKIH0K IAogLSAodm9pZClyZXNvdXJjZUxvYWRlcjooQVZBc3NldFJlc291cmNlTG9hZGVyICopcmVzb3Vy Y2VMb2FkZXIgZGlkQ2FuY2VsTG9hZGluZ1JlcXVlc3Q6KEFWQXNzZXRSZXNvdXJjZUxvYWRpbmdS ZXF1ZXN0ICopbG9hZGluZ1JlcXVlc3QKIHsKICAgICBVTlVTRURfUEFSQU0ocmVzb3VyY2VMb2Fk ZXIpOworICAgIGlmICghbV9jYWxsYmFjaykKKyAgICAgICAgcmV0dXJuOworCiAgICAgcmV0dXJu IG1fY2FsbGJhY2stPmRpZENhbmNlbExvYWRpbmdSZXF1ZXN0KGxvYWRpbmdSZXF1ZXN0KTsKIH0K KworLSAodm9pZClzZXRDYWxsYmFjazooTWVkaWFQbGF5ZXJQcml2YXRlQVZGb3VuZGF0aW9uT2Jq QyopY2FsbGJhY2sKK3sKKyAgICBtX2NhbGxiYWNrID0gY2FsbGJhY2s7Cit9CiBAZW5kCiAjZW5k aWYKIAo=