172325 2017-05-18 17:42:26 -0700 [REGRESSION](r216901): Delete ImageDecoder if BitmapImage::destroyDecodedData() was called to destroy all the decoded frames 2017-05-18 21:09:50 -0700 1 1 1 Unclassified WebKit Images WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 sabouhallawa sabouhallawa commit-queue simon.fraser thorton webkit-bug-importer oldest_to_newest 1310364 0 sabouhallawa 2017-05-18 17:42:26 -0700 In the change <http://trac.webkit.org/changeset/216901>, the function BitmapImage::destroyDecodedData() was changed such that if destroyAll was true but BitmapImage::destroyDecodedData() returns false, destroyAll was set to false which would lead to not calling ImageSource::clear(). ImageSource::clear() deletes the current ImageDecoder and creates a new one if the Image::data() is not null. Not calling ImageSource::clear() from BitmapImage::destroyDecodedData() when the passed destroyAll is true can cause the following problems: 1) CachedImage::didReplaceSharedBufferContents() calls m_image->destroyDecodedData(true) when the data SharedBuffer is switched and it assumes the current ImageDecoder will be deleted and a new one will be created with the new ShareBuffer. 2) For large images, the ImageDecoder may keep large buffers for raster data. Under memory pressure, the MemoryCache will request all the images to release their decoded frames. Because of https://bugs.webkit.org/show_bug.cgi?id=170640, we can't delete the current decoded frame. But deleting the ImageDecoder itself will release the raster data which will not be needed as long the current decoded frame is still cached. However for animated images, it is okay not to call ImageSource::clear(). Animating an image happens after receiving all its data. So problem (1) is not a concern here. But deleting the ImageDecoder while animating an image will cause the animation to jitter because the new ImageDecoder has to decode all the frames from 0..currentFrame to be able to decode the nextFrame if it's equal to (currentFrame + 1). 1310383 1 310589 sabouhallawa 2017-05-18 18:37:05 -0700 Created attachment 310589 Patch 1310390 2 310589 simon.fraser 2017-05-18 18:49:46 -0700 Comment on attachment 310589 Patch r=me but this code is becoming really hard to reason about. 1310428 3 sabouhallawa 2017-05-18 20:29:30 -0700 <rdar://problem/32243153> 1310443 4 310589 commit-queue 2017-05-18 21:09:48 -0700 Comment on attachment 310589 Patch Clearing flags on attachment: 310589 Committed r217096: <http://trac.webkit.org/changeset/217096> 1310444 5 commit-queue 2017-05-18 21:09:50 -0700 All reviewed patches have been landed. Closing bug. 310589 2017-05-18 18:37:05 -0700 2017-05-18 21:09:48 -0700 Patch bug-172325-20170518183726.patch text/plain 2491 sabouhallawa SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDIxNzA3OCkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDI2IEBACisyMDE3LTA1LTE4ICBTYWlkIEFi b3UtSGFsbGF3YSAgPHNhYm91aGFsbGF3YUBhcHBsZS5jb20+CisKKyAgICAgICAgW1JFR1JFU1NJ T05dKHIyMTY5MDEpOiBEZWxldGUgSW1hZ2VEZWNvZGVyIGlmIEJpdG1hcEltYWdlOjpkZXN0cm95 RGVjb2RlZERhdGEoKSB3YXMgY2FsbGVkIHRvIGRlc3Ryb3kgYWxsIHRoZSBkZWNvZGVkIGZyYW1l cworICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTcyMzI1 CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgV2hlbiBj YWxsaW5nIEJpdG1hcEltYWdlOjpkZXN0cm95RGVjb2RlZERhdGEoKSB3aXRoIGRlc3Ryb3lBbGwg PSB0cnVlLCB0aGUKKyAgICAgICAgY3VycmVudCBJbWFnZURlY29kZXIgaGFzIHRvIGJlIGRlbGV0 ZWQgcmVnYXJkbGVzcyB0aGUgY3VycmVudCBmcmFtZSBuZWVkcworICAgICAgICB0byBiZSBjYWNo ZWQgb3Igbm90LiBUaGlzIGlzIHRydWUgZXhjZXB0IHdoZW4gdGhlIGltYWdlIGlzIGFuaW1hdGlu Zy4KKyAgICAgICAgQ3JlYXRpbmcgYSBuZXcgSW1hZ2VEZWNvZGVyIGZvciB0aGUgYW5pbWF0ZWQg aW1hZ2Ugd2lsbCBsZWFkIHRvIGRlY29kaW5nCisgICAgICAgIGFsbCB0aGUgZnJhbWVzIGZyb20g ZnJhbWUtemVybyB0aWxsIHRoZSBjdXJyZW50IGZyYW1lLgorCisgICAgICAgIERlbGV0aW5nIHRo ZSBjdXJyZW50IEltYWdlRGVjb2RlciBoYXMgdGhlIGJlbmVmaXQgb2YgcmVsZWFzaW5nIGl0cyBy YXN0ZXIKKyAgICAgICAgZGF0YS4gV2UgYWxzbyBtdXN0IGRlbGV0ZSB0aGUgY3VycmVudCBJbWFn ZURlY29kZXIgd2hlbiB0aGUgQ2FjaGVkSW1hZ2UKKyAgICAgICAgc3dpdGNoZWQgaXRzIGRhdGEg U2hhcmVkQnVmZmVyLgorCisgICAgICAgIFRoZSBmaXggaXMgcmV0dXJuIHRoZSBjb25kaXRpb24g aW4gQml0bWFwSW1hZ2U6OmRlc3Ryb3lEZWNvZGVkRGF0YSgpIHRvIAorICAgICAgICBiZSBhcyBp dCB3YXMgYmVmb3JlIHIyMTY5MDEuCisKKyAgICAgICAgKiBwbGF0Zm9ybS9ncmFwaGljcy9CaXRt YXBJbWFnZS5jcHA6CisgICAgICAgIChXZWJDb3JlOjpCaXRtYXBJbWFnZTo6ZGVzdHJveURlY29k ZWREYXRhKToKKwogMjAxNy0wNS0xOCAgQW5keSBFc3RlcyAgPGFlc3Rlc0BhcHBsZS5jb20+CiAK ICAgICAgICAgRU5BQkxFKEFQUExFX1BBWV9ERUxFR0FURSkgc2hvdWxkIGJlIE5PIG9uIG1hY09T IFNpZXJyYSBhbmQgZWFybGllcgpJbmRleDogU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhp Y3MvQml0bWFwSW1hZ2UuY3BwCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XZWJDb3JlL3BsYXRmb3Jt L2dyYXBoaWNzL0JpdG1hcEltYWdlLmNwcAkocmV2aXNpb24gMjE3MDc4KQorKysgU291cmNlL1dl YkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvQml0bWFwSW1hZ2UuY3BwCSh3b3JraW5nIGNvcHkpCkBA IC03OSwxNyArNzksMTYgQEAgdm9pZCBCaXRtYXBJbWFnZTo6ZGVzdHJveURlY29kZWREYXRhKGJv bwogCiAgICAgaWYgKCFkZXN0cm95QWxsKQogICAgICAgICBtX3NvdXJjZS5kZXN0cm95RGVjb2Rl ZERhdGFCZWZvcmVGcmFtZShtX2N1cnJlbnRGcmFtZSk7Ci0gICAgZWxzZSBpZiAoIWNhbkRlc3Ry b3lEZWNvZGVkRGF0YSgpKSB7CisgICAgZWxzZSBpZiAoIWNhbkRlc3Ryb3lEZWNvZGVkRGF0YSgp KQogICAgICAgICBtX3NvdXJjZS5kZXN0cm95QWxsRGVjb2RlZERhdGFFeGNsdWRlRnJhbWUobV9j dXJyZW50RnJhbWUpOwotICAgICAgICBkZXN0cm95QWxsID0gZmFsc2U7Ci0gICAgfSBlbHNlIHsK KyAgICBlbHNlIHsKICAgICAgICAgbV9zb3VyY2UuZGVzdHJveUFsbERlY29kZWREYXRhKCk7CiAg ICAgICAgIG1fY3VycmVudEZyYW1lRGVjb2RpbmdTdGF0dXMgPSBJbWFnZUZyYW1lOjpEZWNvZGlu Z1N0YXR1czo6SW52YWxpZDsKICAgICB9CiAKICAgICAvLyBUaGVyZSdzIG5vIG5lZWQgdG8gdGhy b3cgYXdheSB0aGUgZGVjb2RlciB1bmxlc3Mgd2UncmUgZXhwbGljaXRseSBhc2tlZAogICAgIC8v IHRvIGRlc3Ryb3kgYWxsIG9mIHRoZSBmcmFtZXMuCi0gICAgaWYgKCFkZXN0cm95QWxsKQorICAg IGlmICghZGVzdHJveUFsbCB8fCBtX3NvdXJjZS5oYXNBc3luY0RlY29kaW5nUXVldWUoKSkKICAg ICAgICAgbV9zb3VyY2UuY2xlYXJGcmFtZUJ1ZmZlckNhY2hlKG1fY3VycmVudEZyYW1lKTsKICAg ICBlbHNlCiAgICAgICAgIG1fc291cmNlLmNsZWFyKGRhdGEoKSk7Cg==