Bug 70666 - BitmapImage::dataChanged() does not reset enough incomplete frames
Summary: BitmapImage::dataChanged() does not reset enough incomplete frames
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Peter Kasting
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-10-21 19:49 PDT by Peter Kasting
Modified: 2011-10-31 18:06 PDT (History)
1 user (show)

See Also:


Attachments
Manual testcase (220 bytes, text/html)
2011-10-21 19:49 PDT, Peter Kasting
no flags Details
patch v1 (3.39 KB, patch)
2011-10-21 19:53 PDT, Peter Kasting
no flags Details | Formatted Diff | Diff
patch v2 (3.38 KB, patch)
2011-10-21 20:01 PDT, Peter Kasting
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Kasting 2011-10-21 19:49:53 PDT
Created attachment 112060 [details]
Manual testcase

BitmapData::dataChanged() assumes (unconditionally!) that the last frame it currently has in |m_frames| is affected when more data arrives, and none of the others are.  This is not true for ICO files, where we may have a variety of different frames (due to different parts of the browser asking for different sizes) but any or all of them may be incompletely decoded and may need to have their cached metadata reset.  Therefore, this needs to be fixed to iterate through |m_frames| and clear all incomplete ones.

Technically, my fix will also save us from unnecessarily clearing the last frame in the list (for any image type) if it was already "complete", although since the underlying decoder often caches this, I doubt that will have any noticeable perf impact.

Attached is a manual testcase that repros the problem for me if opened in an incognito Chrome window -- pressing "play" shows a partial Ars Technica logo.  I specify an incognito window because this way you can close the window and then open a new one to flush the cache, without which you won't reproduce the bug after one try (unless you manually clear the cache).
Comment 1 Peter Kasting 2011-10-21 19:53:34 PDT
Created attachment 112061 [details]
patch v1
Comment 2 WebKit Review Bot 2011-10-21 19:55:49 PDT
Attachment 112061 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/graphics/BitmapImage.cpp:198:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WebCore/platform/graphics/BitmapImage.cpp:199:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WebCore/platform/graphics/BitmapImage.cpp:201:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WebCore/platform/graphics/BitmapImage.cpp:213:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Total errors found: 4 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Peter Kasting 2011-10-21 20:01:36 PDT
Created attachment 112062 [details]
patch v2

Hate that style rule
Comment 4 James Robinson 2011-10-31 17:07:59 PDT
Comment on attachment 112062 [details]
patch v2

R=me
Comment 5 WebKit Review Bot 2011-10-31 18:06:14 PDT
Comment on attachment 112062 [details]
patch v2

Clearing flags on attachment: 112062

Committed r98930: <http://trac.webkit.org/changeset/98930>
Comment 6 WebKit Review Bot 2011-10-31 18:06:21 PDT
All reviewed patches have been landed.  Closing bug.