Bug 116041

Summary: Checking if frame is complete and access duration doesn't need a decode
Product: WebKit Reporter: Csaba Osztrogonác <ossy>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: REOPENED ---    
Severity: Critical CC: abecsi, buildbot, commit-queue, dino, d-r, ggaren, hausmann, hclam, hyatt, jturcotte, kbalazs, kling, koivisto, kondapallykalyan, mrowe, noam, ossy, pkasting, rniwa, rtakacs, senorblanco, thorton, tobias.netzel, zalan, zherczeg
Priority: P1 Keywords: BlinkMergeCandidate
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 118464, 119016, 119267    
Bug Blocks: 79668, 109662    
Attachments:
Description Flags
GDB backtrace for gifland.us
none
2nd backtrace
none
Patch
none
with speculative Mac buildfix
none
proposed patch
none
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2
none
Patch
none
Patch none

Description Csaba Osztrogonác 2013-05-13 09:50:20 PDT
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.
Comment 1 Andreas Kling 2013-05-13 10:32:08 PDT
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?
Comment 2 zalan 2013-05-13 11:42:47 PDT
no crash here either even with frame flattening on (though the iframe on the page has fixed dimensions, so shouldn't make any difference)
Comment 3 Csaba Osztrogonác 2013-05-14 02:33:12 PDT
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.
Comment 4 Csaba Osztrogonác 2013-05-14 02:37:16 PDT
It seems this bug is related to Coordinated Graphics somehow.
Comment 5 Noam Rosenthal 2013-05-14 12:43:12 PDT
(In reply to comment #4)
> It seems this bug is related to Coordinated Graphics somehow.
It actually seems related to image decoding...
Comment 6 Csaba Osztrogonác 2013-05-15 03:52:56 PDT
(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.
Comment 7 Csaba Osztrogonác 2013-05-15 06:06:00 PDT
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.)
Comment 8 Csaba Osztrogonác 2013-05-22 00:39:20 PDT
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
Comment 9 Csaba Osztrogonác 2013-05-24 07:26:14 PDT
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.
Comment 10 Csaba Osztrogonác 2013-05-24 07:27:10 PDT
Created attachment 202818 [details]
2nd backtrace
Comment 11 Csaba Osztrogonác 2013-05-27 03:19:47 PDT
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.
Comment 12 Peter Kasting 2013-05-27 13:01:57 PDT
(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.
Comment 13 Csaba Osztrogonác 2013-06-03 03:43:11 PDT
(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.
Comment 14 Csaba Osztrogonác 2013-06-03 03:50:07 PDT
Created attachment 203569 [details]
Patch
Comment 15 Build Bot 2013-06-03 04:10:49 PDT
Comment on attachment 203569 [details]
Patch

Attachment 203569 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/694370
Comment 16 Build Bot 2013-06-03 04:13:07 PDT
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 17 Build Bot 2013-06-03 04:25:22 PDT
Comment on attachment 203569 [details]
Patch

Attachment 203569 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/693579
Comment 18 Csaba Osztrogonác 2013-06-03 04:28:47 PDT
Comment on attachment 203569 [details]
Patch

I try to fix the Mac build.
Comment 19 Csaba Osztrogonác 2013-06-03 05:02:38 PDT
Created attachment 203577 [details]
with speculative Mac buildfix
Comment 20 Peter Kasting 2013-06-03 12:08:09 PDT
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
Comment 21 Csaba Osztrogonác 2013-06-05 03:47:44 PDT
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 22 Build Bot 2013-06-05 07:36:58 PDT
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
Comment 23 Build Bot 2013-06-05 07:37:01 PDT
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
Comment 24 Csaba Osztrogonác 2013-06-11 01:46:43 PDT
Created attachment 204328 [details]
Patch

I think the EWS reults are unrelated to this bug. Let's try it again.
Comment 25 Csaba Osztrogonác 2013-06-24 06:46:20 PDT
ping for review?
Comment 26 Allan Sandfeld Jensen 2013-06-24 07:23:19 PDT
Comment on attachment 204328 [details]
Patch

LGTM
Comment 27 WebKit Commit Bot 2013-06-25 01:45:10 PDT
Comment on attachment 204328 [details]
Patch

Clearing flags on attachment: 204328

Committed r151957: <http://trac.webkit.org/changeset/151957>
Comment 28 WebKit Commit Bot 2013-06-25 01:45:15 PDT
All reviewed patches have been landed.  Closing bug.
Comment 29 Tobias Netzel 2013-06-25 12:48:02 PDT
*** Bug 93764 has been marked as a duplicate of this bug. ***
Comment 30 WebKit Commit Bot 2013-07-30 09:28:51 PDT
Re-opened since this is blocked by bug 119267
Comment 31 Csaba Osztrogonác 2013-07-30 09:48:19 PDT
(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()
Comment 32 Peter Kasting 2013-07-30 10:01:31 PDT
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...)
Comment 33 Csaba Osztrogonác 2013-07-30 10:06:45 PDT
(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.
Comment 34 Peter Kasting 2013-07-30 10:08:34 PDT
(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.
Comment 35 Csaba Osztrogonác 2013-07-30 10:09:06 PDT
(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
Comment 36 Hin-Chung Lam 2013-07-30 12:05:59 PDT
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?
Comment 37 Tobias Netzel 2013-07-30 12:24:33 PDT
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.
Comment 38 Csaba Osztrogonác 2013-07-30 13:45:43 PDT
(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.
Comment 39 Csaba Osztrogonác 2013-07-31 01:07:17 PDT
Created attachment 207816 [details]
Patch
Comment 40 Csaba Osztrogonác 2013-07-31 01:14:32 PDT
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().
Comment 41 Csaba Osztrogonác 2013-07-31 01:17:05 PDT
cc Geoffrey, Mark and Tim from Apple side. Could you accept 
this change to make CG and non-CG platforms happy too?
Comment 42 Mark Rowe (bdash) 2013-07-31 01:38:05 PDT
(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.
Comment 43 Csaba Osztrogonác 2013-07-31 03:21:38 PDT
(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 44 Csaba Osztrogonác 2013-07-31 06:32:46 PDT
Comment on attachment 207816 [details]
Patch

removing r? until checking better fixes for CG
Comment 45 Csaba Osztrogonác 2013-08-02 09:13:59 PDT
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.
Comment 46 Csaba Osztrogonác 2013-10-30 09:16:52 PDT
*** Bug 115846 has been marked as a duplicate of this bug. ***