While testing the fix for https://bugs.webkit.org/show_bug.cgi?id=176089, I realized that there was something weird going on with GIFImageDecoder::clearFrameBufferCache(). Despite I was being called, it wasn't really freeing any buffer. Due to this, all the decoded frames will stay in the cache and consume tons of memory instead of respecting the limit of 30MB. The fix to properly delete frames from the cache is trivial, but doing so triggers an unexpected issue: clearFrameBufferCache() receives a clearBeforeFrame parameter, searches backwards for the frame required to render clearBeforeFrame (clearing unneeded frames in between), and then clears all the buffers before the required one. This is because it expects that the next frame rendered will be clearBeforeFrame, and in order to accelerate that we keep the required frame decoded. The problem comes when, after that, we request to decode a former frame that was cleared from the cache. We can't start decoding that frame directly because it may depend on a former frame. So we would need to find that former frame and decode it first. This causes that the fix for https://bugs.webkit.org/show_bug.cgi?id=176089 doesn't work once clearFrameBufferCache is working properly, because it would start decoding the required frame. The proper fix for this would be, besides fixing cleatFrameBufferCache(), implementing a function to find the frame that is required to render the requested frame, and start decoding from there. We could just decode always from frame 0, and it would work, but it would be very inefficient.
Created attachment 322809 [details] Patch
Attachment 322809 [details] did not pass style-queue: ERROR: Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] Total errors found: 2 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 322809 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=322809&action=review > Source/WebCore/platform/image-decoders/gif/GIFImageDecoder.cpp:110 > + for (size_t i = frameIndex; i > 0; i--) { we normally do --i in for loops. > Source/WebCore/platform/image-decoders/gif/GIFImageDecoder.cpp:128 > + const GIFFrameContext* frameContext = m_reader->frameContext(i - 1); const auto*. This can return nullptr, if we know for sure we should never get nullptr in this particular case, then add an assert, otherwise, handle the null case here, I guess continuing the loop. > Source/WebCore/platform/image-decoders/gif/GIFImageDecoder.cpp:133 > + if (frameRect.contains(IntRect(IntPoint(), size()))) I think you can do something like if (frameRect.contains({ { }, size() } }))) > Source/WebCore/platform/image-decoders/gif/GIFImageDecoder.cpp:149 > + size_t startFrame = findFirstRequiredFrameToDecode(index); > + for (size_t i = startFrame; i <= index; i++) This could probably be a single line for (auto i = findFirstRequiredFrameToDecode(index); i <= index; ++i) > Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:289 > + return frame < m_frames.size() ? m_frames[frame].get() : 0; 0 -> nullptr
Comment on attachment 322809 [details] Patch Attachment 322809 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/4765738 New failing tests: workers/wasm-long-compile.html
Created attachment 322813 [details] Archive of layout-test-results from ews114 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 322809 [details] Patch Attachment 322809 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/4765781 New failing tests: accessibility/ios-simulator/video-elements-ios.html
Created attachment 322818 [details] Archive of layout-test-results from ews122 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews122 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6
Created attachment 322823 [details] Patch
Attachment 322823 [details] did not pass style-queue: ERROR: Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] Total errors found: 2 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 322823 [details] Patch Clearing flags on attachment: 322823 Committed r222910: <http://trac.webkit.org/changeset/222910>
All reviewed patches have been landed. Closing bug.