Bug 33258 - [Regression] PNG-in-ICO decoding in public image decoders broken
Summary: [Regression] PNG-in-ICO decoding in public image decoders broken
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Peter Kasting
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-01-06 10:49 PST by Peter Kasting
Modified: 2010-01-06 16:26 PST (History)
4 users (show)

See Also:


Attachments
down.ico (148.83 KB, application/octet-stream)
2010-01-06 10:49 PST, Peter Kasting
no flags Details
the candidate patch (1.31 KB, patch)
2010-01-06 11:31 PST, Yong Li
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Kasting 2010-01-06 10:49:21 PST
Created attachment 45968 [details]
down.ico

http://trac.webkit.org/changeset/52831 broke PNG-in-ICO decoding.  Testcase attached.  Will fix soon.
Comment 1 Yong Li 2010-01-06 10:53:29 PST
sorry it's my bug.

see line 121 in PNGImageDecoder.cpp:

            if ((sizeOnly && decoder->isSizeAvailable()) || m_hasFinishedDecoding)
                break;

just chaning "break" to "return" should fix it.

-Yong
Comment 2 Yong Li 2010-01-06 11:31:39 PST
Created attachment 45973 [details]
the candidate patch
Comment 3 WebKit Review Bot 2010-01-06 11:35:42 PST
Attachment 45973 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
Traceback (most recent call last):
  File "WebKitTools/Scripts/check-webkit-style", line 98, in <module>
    main()
  File "WebKitTools/Scripts/check-webkit-style", line 62, in main
    defaults = style.ArgumentDefaults(style.DEFAULT_OUTPUT_FORMAT,
AttributeError: 'module' object has no attribute 'ArgumentDefaults'
Comment 4 Adam Barth 2010-01-06 11:42:13 PST
Sorry for the spam.  Stylebot is sick.
Comment 5 Eric Seidel (no email) 2010-01-06 14:06:46 PST
Looks like this caused crashes:
http://build.webkit.org/builders/Leopard%20Intel%20Debug%20%28Tests%29/builds/8928
http://build.webkit.org/results/Leopard%20Intel%20Debug%20(Tests)/r52862%20(8928)/results.html
svg/W3C-SVG-1.1/linking-a-01-b.svg

I think we'll need to roll this out. :(
Comment 6 Eric Seidel (no email) 2010-01-06 14:08:06 PST
http://trac.webkit.org/changeset/52862 was the original commit.
Comment 7 Eric Seidel (no email) 2010-01-06 14:08:53 PST
Do we not have a way to test image decoders?  I'm surprised this landed w/o a test case?
Comment 8 Eric Seidel (no email) 2010-01-06 14:14:59 PST
Yong doesn't seem to be around on IRC, so I'm rolling this out in hopes it will fix the crashes on the bot.
Comment 9 Eric Seidel (no email) 2010-01-06 14:26:42 PST
Rolled out in http://trac.webkit.org/changeset/52871.  Peter Kasting notes that we may need to roll out r52831 as well, otherwise this breaks chromium.  This change should have been unrelated.  If it's found to be unrelated, I'll roll it back in.
Comment 10 Eric Seidel (no email) 2010-01-06 14:38:06 PST
I'm rolling this back in after further discussions with Peter and Yong.  I'm testing it on Leopard Debug first to make sure.
Comment 11 Eric Seidel (no email) 2010-01-06 14:43:59 PST
The leopard build is broken at the moment due to http://trac.webkit.org/changeset/52872, so I can't land the re-land yet.
Comment 12 Eric Seidel (no email) 2010-01-06 16:26:55 PST
Committed r52880: <http://trac.webkit.org/changeset/52880>