164864 2016-11-17 05:31:02 -0800 REGRESSION(r208511): ImageDecoders: Crash decoding GIF images since r208511 2016-11-18 08:41:54 -0800 1 1 1 Unclassified WebKit Platform WebKit Local Build Unspecified Unspecified RESOLVED FIXED Gtk P2 Normal --- 1 cgarcia webkit-unassigned bugs-noreply clopez sabouhallawa simon.fraser oldest_to_newest 1252276 0 cgarcia 2016-11-17 05:31:02 -0800 This happens sometimes since r208511 because the same decoder is decoded by more than one thread at the same time and the decoders are not thread-safe. Several methods in ImageDecoder need to decode partially the image, so it's possible that one method calls frameBufferAtIndex at the same times as createFrameImageAtIndex that now can be called from the image decoder thread. 1252277 1 295048 cgarcia 2016-11-17 05:33:20 -0800 Created attachment 295048 Patch 1252291 2 clopez 2016-11-17 07:52:41 -0800 *** Bug 164866 has been marked as a duplicate of this bug. *** 1252296 3 295048 simon.fraser 2016-11-17 08:19:38 -0800 Comment on attachment 295048 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=295048&action=review > Source/WebCore/ChangeLog:8 > + This happens sometimes since r208511 because the same decoder is decoded by more than one thread at the same same decoder is decoded by -> same decoder is used by > Source/WebCore/platform/image-decoders/ImageDecoder.h:218 > + Lock m_lock; Would be nice to name this so that it indicates what it protects. 1252297 4 cgarcia 2016-11-17 08:24:34 -0800 (In reply to comment #3) > Comment on attachment 295048 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=295048&action=review Thanks for the review. > > Source/WebCore/ChangeLog:8 > > + This happens sometimes since r208511 because the same decoder is decoded by more than one thread at the same > > same decoder is decoded by -> same decoder is used by Oops. > > Source/WebCore/platform/image-decoders/ImageDecoder.h:218 > > + Lock m_lock; > > Would be nice to name this so that it indicates what it protects. Actually I named this m_decoderLock at first, but sounded redundant to me because the class is ImageDecoder. Maybe m_decodeLock, since we are protecting the decode() method in the end. 1252299 5 simon.fraser 2016-11-17 08:35:51 -0800 (In reply to comment #4) > Actually I named this m_decoderLock at first, but sounded redundant to me > because the class is ImageDecoder. Maybe m_decodeLock, since we are > protecting the decode() method in the end. Locks don't protect code, they protect data. m_lock is probably OK here. What needs to happen to make the decoders thread safe? 1252305 6 295048 sabouhallawa 2016-11-17 09:28:42 -0800 Comment on attachment 295048 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=295048&action=review Would it be better to rename the existing virtual frameBufferAtIndex() functions to be internalFrameBufferAtIndex() and create a non virtual function called frameBufferAtIndex() in imageDecoder class which does the locking and call internalFrameBufferAtIndex()? > Source/WebCore/platform/image-decoders/ImageDecoder.cpp:174 > + Locker<Lock> locker(m_lock); You can use LockHolder instead of Locker<Lock>. 1252606 7 cgarcia 2016-11-17 22:24:22 -0800 (In reply to comment #5) > (In reply to comment #4) > > Actually I named this m_decoderLock at first, but sounded redundant to me > > because the class is ImageDecoder. Maybe m_decodeLock, since we are > > protecting the decode() method in the end. > > Locks don't protect code, they protect data. m_lock is probably OK here. Good point. > What needs to happen to make the decoders thread safe? You mean making every decoder thread safe instead of protecting them from ImageDecoder? 1252607 8 cgarcia 2016-11-17 22:29:42 -0800 (In reply to comment #6) > Comment on attachment 295048 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=295048&action=review > > Would it be better to rename the existing virtual frameBufferAtIndex() > functions to be internalFrameBufferAtIndex() and create a non virtual > function called frameBufferAtIndex() in imageDecoder class which does the > locking and call internalFrameBufferAtIndex()? I thought about that but I also want to make sure the returned ImageFrame is not modified by another thread, that's why don't release the lock until the end of the function. > > Source/WebCore/platform/image-decoders/ImageDecoder.cpp:174 > > + Locker<Lock> locker(m_lock); > > You can use LockHolder instead of Locker<Lock>. Sure! 1252617 9 cgarcia 2016-11-17 23:25:57 -0800 Committed r208881: <http://trac.webkit.org/changeset/208881> 1252684 10 sabouhallawa 2016-11-18 08:41:54 -0800 (In reply to comment #8) > (In reply to comment #6) > > Comment on attachment 295048 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=295048&action=review > > > > Would it be better to rename the existing virtual frameBufferAtIndex() > > functions to be internalFrameBufferAtIndex() and create a non virtual > > function called frameBufferAtIndex() in imageDecoder class which does the > > locking and call internalFrameBufferAtIndex()? > > I thought about that but I also want to make sure the returned ImageFrame is > not modified by another thread, that's why don't release the lock until the > end of the function. > But the only function that changes the ImageFrame or the ImageDecoder:: m_frameBufferCache is ImageDecoder::frameBufferAtIndex(). It does that via calling ImageDecoder::decode() which calls ImageReader::decode(). The ImageReader::decode() is the one that is responsible for caching the ImageFrames in ImageDecoder:: m_frameBufferCache. There is another calls to ImageDecoder::decode() from ImageDecoder::isSizeAvailable() but this calls decodes the size of the image frame only. It does not cache any ImageFrame. So I think it's cleaner to lock m_frameBufferCache for writing in the one place that changes it instead of locking every place that makes a call to the same function. 295048 2016-11-17 05:33:20 -0800 2016-11-17 08:19:38 -0800 Patch wcore-gif-crash.diff text/plain 3267 cgarcia ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZwppbmRleCAwZmYxY2NmLi5lYzEyMGZjIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29y ZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMjEg QEAKKzIwMTYtMTEtMTcgIENhcmxvcyBHYXJjaWEgQ2FtcG9zICA8Y2dhcmNpYUBpZ2FsaWEuY29t PgorCisgICAgICAgIFJFR1JFU1NJT04ocjIwODUxMSk6IEltYWdlRGVjb2RlcnM6IENyYXNoIGRl Y29kaW5nIEdJRiBpbWFnZXMgc2luY2UgcjIwODUxMQorICAgICAgICBodHRwczovL2J1Z3Mud2Vi a2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTY0ODY0CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9C T0RZIChPT1BTISkuCisKKyAgICAgICAgVGhpcyBoYXBwZW5zIHNvbWV0aW1lcyBzaW5jZSByMjA4 NTExIGJlY2F1c2UgdGhlIHNhbWUgZGVjb2RlciBpcyBkZWNvZGVkIGJ5IG1vcmUgdGhhbiBvbmUg dGhyZWFkIGF0IHRoZSBzYW1lCisgICAgICAgIHRpbWUgYW5kIHRoZSBkZWNvZGVycyBhcmUgbm90 IHRocmVhZC1zYWZlLiBTZXZlcmFsIG1ldGhvZHMgaW4gSW1hZ2VEZWNvZGVyIG5lZWQgdG8gZGVj b2RlIHBhcnRpYWxseSB0aGUgaW1hZ2UsCisgICAgICAgIHNvIGl0J3MgcG9zc2libGUgdGhhdCBv bmUgbWV0aG9kIGNhbGxzIGZyYW1lQnVmZmVyQXRJbmRleCBhdCB0aGUgc2FtZSB0aW1lcyBhcyBj cmVhdGVGcmFtZUltYWdlQXRJbmRleCB0aGF0IG5vdworICAgICAgICBjYW4gYmUgY2FsbGVkIGZy b20gdGhlIGltYWdlIGRlY29kZXIgdGhyZWFkLiBVc2UgYSBMb2NrIGluIEltYWdlRGVjb2RlciB0 byBwcm90ZWN0IGNhbGxzIHRvIGZyYW1lQnVmZmVyQXRJbmRleC4KKworICAgICAgICAqIHBsYXRm b3JtL2ltYWdlLWRlY29kZXJzL0ltYWdlRGVjb2Rlci5jcHA6CisgICAgICAgIChXZWJDb3JlOjpJ bWFnZURlY29kZXI6OmZyYW1lSXNDb21wbGV0ZUF0SW5kZXgpOgorICAgICAgICAoV2ViQ29yZTo6 SW1hZ2VEZWNvZGVyOjpmcmFtZUR1cmF0aW9uQXRJbmRleCk6CisgICAgICAgIChXZWJDb3JlOjpJ bWFnZURlY29kZXI6OmNyZWF0ZUZyYW1lSW1hZ2VBdEluZGV4KToKKyAgICAgICAgKiBwbGF0Zm9y bS9pbWFnZS1kZWNvZGVycy9JbWFnZURlY29kZXIuaDoKKwogMjAxNi0xMS0xNyAgWXVzdWtlIFN1 enVraSAgPHV0YXRhbmUudGVhQGdtYWlsLmNvbT4KIAogICAgICAgICBVbnJldmlld2VkLCBhdHRl bXB0IHRvIGZpeCBsaW5rIGVycm9yIGFmdGVyIHIyMDg4NDEgcGFydDIKZGlmZiAtLWdpdCBhL1Nv dXJjZS9XZWJDb3JlL3BsYXRmb3JtL2ltYWdlLWRlY29kZXJzL0ltYWdlRGVjb2Rlci5jcHAgYi9T b3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9pbWFnZS1kZWNvZGVycy9JbWFnZURlY29kZXIuY3BwCmlu ZGV4IGJhYmRhODkuLjYyOTBkOGYgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3Jt L2ltYWdlLWRlY29kZXJzL0ltYWdlRGVjb2Rlci5jcHAKKysrIGIvU291cmNlL1dlYkNvcmUvcGxh dGZvcm0vaW1hZ2UtZGVjb2RlcnMvSW1hZ2VEZWNvZGVyLmNwcApAQCAtMzUsNiArMzUsNyBAQAog CiAjaW5jbHVkZSA8YWxnb3JpdGhtPgogI2luY2x1ZGUgPGNtYXRoPgorI2luY2x1ZGUgPHd0Zi9M b2NrZXIuaD4KIAogdXNpbmcgbmFtZXNwYWNlIHN0ZDsKIApAQCAtMTcwLDYgKzE3MSw3IEBAIHRl bXBsYXRlIDxNYXRjaFR5cGUgdHlwZT4gaW50IGdldFNjYWxlZFZhbHVlKGNvbnN0IFZlY3Rvcjxp bnQ+JiBzY2FsZWRWYWx1ZXMsIGluCiAKIGJvb2wgSW1hZ2VEZWNvZGVyOjpmcmFtZUlzQ29tcGxl dGVBdEluZGV4KHNpemVfdCBpbmRleCkKIHsKKyAgICBMb2NrZXI8TG9jaz4gbG9ja2VyKG1fbG9j ayk7CiAgICAgSW1hZ2VGcmFtZSogYnVmZmVyID0gZnJhbWVCdWZmZXJBdEluZGV4KGluZGV4KTsK ICAgICByZXR1cm4gYnVmZmVyICYmIGJ1ZmZlci0+aXNDb21wbGV0ZSgpOwogfQpAQCAtMTkzLDYg KzE5NSw3IEBAIHVuc2lnbmVkIEltYWdlRGVjb2Rlcjo6ZnJhbWVCeXRlc0F0SW5kZXgoc2l6ZV90 IGluZGV4KSBjb25zdAogCiBmbG9hdCBJbWFnZURlY29kZXI6OmZyYW1lRHVyYXRpb25BdEluZGV4 KHNpemVfdCBpbmRleCkKIHsKKyAgICBMb2NrZXI8TG9jaz4gbG9ja2VyKG1fbG9jayk7CiAgICAg SW1hZ2VGcmFtZSogYnVmZmVyID0gZnJhbWVCdWZmZXJBdEluZGV4KGluZGV4KTsKICAgICBpZiAo IWJ1ZmZlciB8fCBidWZmZXItPmlzRW1wdHkoKSkKICAgICAgICAgcmV0dXJuIDA7CkBAIC0yMTMs NiArMjE2LDcgQEAgTmF0aXZlSW1hZ2VQdHIgSW1hZ2VEZWNvZGVyOjpjcmVhdGVGcmFtZUltYWdl QXRJbmRleChzaXplX3QgaW5kZXgsIFN1YnNhbXBsaW5nTGUKICAgICBpZiAoc2l6ZSgpLmlzRW1w dHkoKSkKICAgICAgICAgcmV0dXJuIG51bGxwdHI7CiAKKyAgICBMb2NrZXI8TG9jaz4gbG9ja2Vy KG1fbG9jayk7CiAgICAgSW1hZ2VGcmFtZSogYnVmZmVyID0gZnJhbWVCdWZmZXJBdEluZGV4KGlu ZGV4KTsKICAgICBpZiAoIWJ1ZmZlciB8fCBidWZmZXItPmlzRW1wdHkoKSB8fCAhYnVmZmVyLT5o YXNCYWNraW5nU3RvcmUoKSkKICAgICAgICAgcmV0dXJuIG51bGxwdHI7CmRpZmYgLS1naXQgYS9T b3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9pbWFnZS1kZWNvZGVycy9JbWFnZURlY29kZXIuaCBiL1Nv dXJjZS9XZWJDb3JlL3BsYXRmb3JtL2ltYWdlLWRlY29kZXJzL0ltYWdlRGVjb2Rlci5oCmluZGV4 IDVmZDRmMWIuLmE5NmIzNTggMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2lt YWdlLWRlY29kZXJzL0ltYWdlRGVjb2Rlci5oCisrKyBiL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3Jt L2ltYWdlLWRlY29kZXJzL0ltYWdlRGVjb2Rlci5oCkBAIC0zNCw2ICszNCw3IEBACiAjaW5jbHVk ZSAiUGxhdGZvcm1TY3JlZW4uaCIKICNpbmNsdWRlICJTaGFyZWRCdWZmZXIuaCIKICNpbmNsdWRl IDx3dGYvQXNzZXJ0aW9ucy5oPgorI2luY2x1ZGUgPHd0Zi9Mb2NrLmg+CiAjaW5jbHVkZSA8d3Rm L09wdGlvbmFsLmg+CiAjaW5jbHVkZSA8d3RmL1JlZlB0ci5oPgogI2luY2x1ZGUgPHd0Zi9WZWN0 b3IuaD4KQEAgLTIxNCw2ICsyMTUsNyBAQCBuYW1lc3BhY2UgV2ViQ29yZSB7CiAjZW5kaWYKICAg ICAgICAgYm9vbCBtX2lzQWxsRGF0YVJlY2VpdmVkIHsgZmFsc2UgfTsKICAgICAgICAgYm9vbCBt X2ZhaWxlZCB7IGZhbHNlIH07CisgICAgICAgIExvY2sgbV9sb2NrOwogICAgIH07CiAKIH0gLy8g bmFtZXNwYWNlIFdlYkNvcmUK