After https://trac.webkit.org/changeset/149435 WebProcess crahes on http://gifland.us . At least with MiniBrowser of Qt and Nix. With r149434 everything works fine, but with r149435 WebProcess crashes in 5-10 seconds after the start. The bug is still valid on ToT. (QtWebKit - r150020 , WebKitNix - last merge: r149902) It needs more investigation.
Can't reproduce any badness on applemac port with ~5 minutes of clicking around on gifland.us. Do you have a backtrace for the crash?
no crash here either even with frame flattening on (though the iframe on the page has fixed dimensions, so shouldn't make any difference)
Created attachment 201693 [details] GDB backtrace for gifland.us Here is the GDB backtrace for gifland.us with QtWebKit on r150061. I checked and I got same backtrace on r149435. Additionally I checked r149434 too and I got same backtrace on gifland.us as mentioned in the original bug.
It seems this bug is related to Coordinated Graphics somehow.
(In reply to comment #4) > It seems this bug is related to Coordinated Graphics somehow. It actually seems related to image decoding...
(In reply to comment #5) > (In reply to comment #4) > > It seems this bug is related to Coordinated Graphics somehow. > It actually seems related to image decoding... The assertion hits in image decoder ... but it is somehow related to Coordinated Graphics, because there are no assertion with WebKit1 based QtTestBrowser and no assertion on Mac. And is related to r149435 (and r149287), because there were no assertion before r149435.
I started debugging it, so assign to me. The root of the problem is that ImageFrame::setSize() is called again after the gif is restarted with the frame 0. But it shouldn't ... (The assertion hit with WebKit1 and WebKit2 too. But in release mode only WebKit2 crashes. So it seems it is unrelated to Coordinated Graphics.)
149435 is innocent, it only increased the incidence of the crash. And I managed to reproduce the crash with WK1 and WK2 too. And then I managed to bisect the real culprit: https://trac.webkit.org/changeset/143936
I found some strange thing during debugging this bug. Let's see this page: http://gifland.us/198988 The crash happens when the animation is restarted, but not always. Here is an example simplified trace: ... - GIFImageDecoder::initFrameBuffer(frameIndex=26) - ImageSource::clear(destroyAll=false, clearBeforeFrame=25) - GIFImageDecoder::clearFrameBufferCache() cleared: 1 frame - GIFImageDecoder::initFrameBuffer(frameIndex=27) - ImageSource::clear(destroyAll=false, clearBeforeFrame=26) - GIFImageDecoder::clearFrameBufferCache() cleared: 1 frame - GIFImageDecoder::initFrameBuffer(frameIndex=0) -->>>> ImageFrame::setSize(): ASSERT(!width() && !height()); - BOOOOM! - ImageSource::clear(destroyAll=false, clearBeforeFrame=27) - GIFImageDecoder::clearFrameBufferCache() cleared: 2 frames - GIFImageDecoder::initFrameBuffer(frameIndex=1) -->>>> ASSERT(prevBuffer->status() == ImageFrame::FrameComplete); - BOOOOM! Segmentation fault (core dumped) - see the attached backtrace (2nd attachment) --> It is caused because the empty frame 0 was copied previously.
Created attachment 202818 [details] 2nd backtrace
cc Peter as the author of https://bugs.webkit.org/show_bug.cgi?id=23743 and Dave as the reviewer. Maybe you have any idea how is it possible that the frame 0 is deleted too early.
(In reply to comment #11) > cc Peter as the author of https://bugs.webkit.org/show_bug.cgi?id=23743 > and Dave as the reviewer. Maybe you have any idea how is it possible that > the frame 0 is deleted too early. See https://code.google.com/p/chromium/issues/detail?id=240113#c37 and onward. In short, I bet you're seeing the same problem we are, and the cause is detailed in comment 39 on that bug -- http://trac.webkit.org/changeset/113486 made a bunch of incorrect changes to BitmapImage.cpp. We were planning to fix and upstream, but I don't know how long that will take. It probably wouldn't be hard for you to fix; if you do so I can give you an informal review.
(In reply to comment #12) > See https://code.google.com/p/chromium/issues/detail?id=240113#c37 and onward. In short, I bet you're seeing the same problem we are, and the cause is detailed in comment 39 on that bug -- http://trac.webkit.org/changeset/113486 made a bunch of incorrect changes to BitmapImage.cpp. > > We were planning to fix and upstream, but I don't know how long that will take. It probably wouldn't be hard for you to fix; if you do so I can give you an informal review. Thanks, it seems it is a same bug. I checked https://src.chromium.org/viewvc/blink?revision=149883&view=revision and it it fixed the bug for me.
Created attachment 203569 [details] Patch
Comment on attachment 203569 [details] Patch Attachment 203569 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/694370
Comment on attachment 203569 [details] Patch Attachment 203569 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/694362
Comment on attachment 203569 [details] Patch Attachment 203569 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/693579
Comment on attachment 203569 [details] Patch I try to fix the Mac build.
Created attachment 203577 [details] with speculative Mac buildfix
Comment on attachment 203577 [details] with speculative Mac buildfix View in context: https://bugs.webkit.org/attachment.cgi?id=203577&action=review Somehow I thought there would need to be other precursor patches upstreamed. In any case, this direct translation seems pretty OK to me. > Source/WebCore/platform/graphics/ImageSource.cpp:167 > + const float duration = m_decoder->frameDurationAtIndex(index) / 1000.0f; Nit: Extra space
Created attachment 203792 [details] proposed patch Extra space removed. :) Fortunately other patches wasn't needed. It was the first imagedecoder commit with behavioural change after the fork.
Comment on attachment 203792 [details] proposed patch Attachment 203792 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/662588 New failing tests: inspector/geolocation-success.html
Created attachment 203848 [details] Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-10 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.3
Created attachment 204328 [details] Patch I think the EWS reults are unrelated to this bug. Let's try it again.
ping for review?
Comment on attachment 204328 [details] Patch LGTM
Comment on attachment 204328 [details] Patch Clearing flags on attachment: 204328 Committed r151957: <http://trac.webkit.org/changeset/151957>
All reviewed patches have been landed. Closing bug.
*** Bug 93764 has been marked as a duplicate of this bug. ***
Re-opened since this is blocked by bug 119267
(In reply to comment #30) > Re-opened since this is blocked by bug 119267 Reverted, because it revealed a bug on Mac I can't fixed ( bug119267 ): http://trac.webkit.org/changeset/153475 just for a future note: Once if we can fix it properly on a way makes CG and other platforms happy too, we should integrate r152531 to this patch too. And we should fix bug119218 before we remove explicit image decoding from ImageSource::frameIsCompleteAtIndex() and ImageSourceframeDurationAtIndex()
There's no information on that bug or this about what the "bug you can't fix" actually is. (Not that I'm planning on working on this, but whoever does will need to know...)
(In reply to comment #32) > There's no information on that bug or this about what the "bug you can't fix" actually is. Oh, sorry I referenced to wrong bug. Here is the Mac bug with detailed information: https://bugs.webkit.org/show_bug.cgi?id=119267 > (Not that I'm planning on working on this, but whoever does will need to know...) :) I'm not expect you work on this, but any hint or idea would be very welcome. I think (at least hope) if we could fix bug119218 somehow, we could simple commit r151957 and r152531 again.
(In reply to comment #33) > (In reply to comment #32) > > There's no information on that bug or this about what the "bug you can't fix" actually is. > Oh, sorry I referenced to wrong bug. Here is the Mac bug with detailed > information: https://bugs.webkit.org/show_bug.cgi?id=119267 That's the same bug you linked to before. It doesn't have detailed information, unless I'm really really blind.
(In reply to comment #33) > (In reply to comment #32) > > There's no information on that bug or this about what the "bug you can't fix" actually is. ... Ah, it isn't my day, I meant https://bugs.webkit.org/show_bug.cgi?id=119016
If the problem on CG is that it requires a decode to answer frameIsCompleteAtIndex() then can you do the decode in ImageSource:: frameIsCompleteAtIndex() in ImageSourceCG.cpp? ImageSource::frameIsCompleteAtIndex() is to answer if the frame is fully loaded whether the frame is decoded or not. WebKit's native and Blink can answer this call without decoding. If CG's decoder doesn't have this capability then the fix should be in ImageSourceCG.cpp, no?
Maybe you should look at the patch I had posted rather long time ago in bug 93764, which tried to solve this same issue in a quite naive way. I've got my own builds of Mac WebKit (using CoreGraphics) running with that patch of mine applied since then (and a little bit longer even). I didn't encounter any issues with my approach and it hugely improves performance of animated GIFs in my case, such that sites with lots of animated GIFs can now finally be viewed at all on OS X 10.5 . Note that there were important internal changes made concerning frame caching in the CoreGraphics image decoder after 10.5 and I did never test this against later versions of OS X.
(In reply to comment #36) > If the problem on CG is that it requires a decode to answer frameIsCompleteAtIndex() then can you do the decode in ImageSource:: frameIsCompleteAtIndex() in ImageSourceCG.cpp? The idea is good, I like it. Let me try it tomorrow. > ImageSource::frameIsCompleteAtIndex() is to answer if the frame is fully loaded whether the frame is decoded or not. WebKit's native and Blink can answer this call without decoding. If CG's decoder doesn't have this capability then the fix should be in ImageSourceCG.cpp, no? I absolutely agree, I said exactly the same thing in bug119016, but I haven't managed to convince Geoffrey about it and r151957 was considered the guilty. But let me try your suggestion.
Created attachment 207816 [details] Patch
Comment on attachment 207816 [details] Patch It is the same patch to the original: http://trac.webkit.org/changeset/151957 with these additional fixes/changes: - Integrated http://trac.webkit.org/changeset/152531 to make EFL and Cairo happy. - Change CG implementation of ImageSource::frameIsCompleteAtIndex() to explicitly decode image to be able determine if a frame is completely loaded or not. Previously it worked because BitMapImage::frameIsCompleteAtIndex() explicitly triggered image decoding, before calling ImageSourceCG's ImageSource::frameIsCompleteAtIndex(). - Unfortunately I couldn't make ImageSource::frameIsCompleteAtIndex const, because now the CG's implementation calls the non-const createFrameAtIndex().
cc Geoffrey, Mark and Tim from Apple side. Could you accept this change to make CG and non-CG platforms happy too?
(In reply to comment #40) > (From update of attachment 207816 [details]) > - Change CG implementation of ImageSource::frameIsCompleteAtIndex() to explicitly decode image > to be able determine if a frame is completely loaded or not. Previously it worked because > BitMapImage::frameIsCompleteAtIndex() explicitly triggered image decoding, before calling > ImageSourceCG's ImageSource::frameIsCompleteAtIndex(). Based on my brief reading of the patch this would seem to end up allocating many temporary CGImageRef's due to ImageSource::createFrameAtIndex being called much more frequently. For instance, it'll now be called twice for every call of BitmapImage::cacheFrame (once within frameIsCompleteAtIndex where the result is just discarded, and then again within BitmapImage::cacheFrame itself where the result is actually used). I don't think it's desirable to be adding calls to CGImageSourceCreateImageAtIndex where we then just ignore the result.
(In reply to comment #42) > Based on my brief reading of the patch this would seem to end up allocating many temporary CGImageRef's due to ImageSource::createFrameAtIndex being called much more frequently. For instance, it'll now be called twice for every call of BitmapImage::cacheFrame (once within frameIsCompleteAtIndex where the result is just discarded, and then again within BitmapImage::cacheFrame itself where the result is actually used). I don't think it's desirable to be adding calls to CGImageSourceCreateImageAtIndex where we then just ignore the result. You're right that BitmapImage::cacheFrame would trigger two calls to CGImageSourceCreateImageAtIndex. Have you got any idea, how can we avoid it? My first proposal in the other bug, which would have left CG implementation untouched - https://bugs.webkit.org/attachment.cgi?id=207713&action=review - was rejected, because you don't want pepper the code with ifdefs. I have one more idea, it might work: What if CG's ImageSource::frameIsCompleteAtIndex() didn't call createFrameAtIndex unconditionally? But only if it is necessary, when the return value is false. And after the createFrameAtIndex it could return with the recalculated return value. I'm going to try it locally a little bit later today.
Comment on attachment 207816 [details] Patch removing r? until checking better fixes for CG
Set unassigned, I can't work on it until back from holiday. Feel free to pick it up if anybody is interested in this bug.
*** Bug 115846 has been marked as a duplicate of this bug. ***