39786 2010-05-26 14:55:49 -0700 Public GIF decoder fails to mark some images as "done" 2010-05-26 15:26:55 -0700 1 1 1 Unclassified WebKit Images 528+ (Nightly build) PC All RESOLVED FIXED http://threeframes.net/ P2 Normal --- 1 pkasting pkasting abarth hyatt oldest_to_newest 230937 0 pkasting 2010-05-26 14:55:49 -0700 The open-source GIF decoder has a bug that results in it never marking some GIFs as "done". The problematic code is effectively: for (...; bytes remaining > bytes to consume in current state; ...) { switch (state) { ... case x: if (consuming bytes led to a finished GIF) state = gif_done; break; ... case gif_done: clientptr->gifComplete(); return true; // "success" } } return false; // "need more bytes to continue" The reason this is a problem is that when we switch the state to gif_done, we don't reset the "bytes to consume" value to 0. So we go to run through the loop again and find that there are no more bytes in the file, but we want to consume some bytes. Thus we exit the loop and return false, without ever marking the GIF as "done". The effect of this is that we erroneously mark these GIFs as having failed, and in some cases fail to properly set things like the repeat count. Try running Chrome 6.0.408.1 on the URL given in this bug's URL field and note that while the GIFs on the page should all animate indefinitely, most don't until you refresh or reload the page. The fix here is that instead of setting the state directly, we should use a macro that is used to do all the other state changes, which simultaneously sets the new state and the new "bytes to consume" value (which in this case should be 0). Patch coming shortly. 230942 1 57165 pkasting 2010-05-26 15:01:04 -0700 Created attachment 57165 patch v1 This patch fixes both places that failed to use GETN() to change state: one by using the macro, and the other by eliminating it entirely. (WebKit uses this code differently than Mozilla did, and marking corrupt GIFs as "failed" won't prevent their display.) 230944 2 57165 abarth 2010-05-26 15:03:46 -0700 Comment on attachment 57165 patch v1 I have no idea what this does, but I believe you. It's too bad we don't have more than one image expert. 230945 3 abarth 2010-05-26 15:04:13 -0700 Maybe Hyatt has some idea who might know this code better? 230947 4 pkasting 2010-05-26 15:06:59 -0700 (In reply to comment #3) > Maybe Hyatt has some idea who might know this code better? Sadly, I think hyatt and I are the only people to have worked on this, and it was years ago for him. I probably ought to just gut GIFImageReader.cpp someday and rewrite it from scratch. It's been a never-ending source of problems. 230962 5 pkasting 2010-05-26 15:26:55 -0700 Fixed in r60255. 57165 2010-05-26 15:01:04 -0700 2010-05-26 15:03:45 -0700 patch v1 gif_patch text/plain 2476 pkasting SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA2MDI1MikKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTcgQEAKKzIwMTAtMDUtMjYgIFBldGVyIEthc3RpbmcgIDxwa2FzdGluZ0Bnb29n bGUuY29tPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAg IGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0zOTc4NgorICAgICAgICBQ cm9wZXJseSByZXNldCB8Ynl0ZXNfdG9fY29uc3VtZXwgd2hlbiByZWFjaGluZyB0aGUgImdpZl9k b25lIiBzdGF0ZSBpbgorICAgICAgICB0aGUgb3Blbi1zb3VyY2UgR0lGIGRlY29kZXIuCisgICAg ICAgIAorICAgICAgICBObyB0ZXN0cywgc2luY2UgdGhlcmUncyBubyB0ZXN0IGhhcm5lc3Mgc3Vw cG9ydCBmb3IgY2hlY2tpbmcgdGhlCisgICAgICAgIGludGVybmFsIEltYWdlRGVjb2RlciBzdGF0 ZSB2YWx1ZXMuCisKKyAgICAgICAgKiBwbGF0Zm9ybS9pbWFnZS1kZWNvZGVycy9naWYvR0lGSW1h Z2VSZWFkZXIuY3BwOgorICAgICAgICAoR0lGSW1hZ2VSZWFkZXI6OnJlYWQpOiBVc2UgYSBtYWNy byB0byBwZXJmb3JtIHRoZSBzdGF0ZSBjaGFuZ2UsIGxpa2Ugd2UgZG8gZXZlcnl3aGVyZSBlbHNl IGluIHRoZSBmaWxlLiAgQWxzbyBjb3JyZWN0bHkgcmV0dXJuICJmYWlsdXJlIiBmb3IgY2VydGFp biBjb3JydXB0IEdJRnMsIHNpbmNlIHRoYXQgZG9lc24ndCBwcmV2ZW50IHRoZWlyIGRpc3BsYXkg KGR1ZSB0byBXZWJLaXQncyBkaWZmZXJlbnQgdXNlIG9mIHRoaXMgY29kZSBjb21wYXJlZCB0byBN b3ppbGxhKS4KKwogMjAxMC0wNS0yNiAgQWxleGV5IFByb3NrdXJ5YWtvdiAgPGFwQGFwcGxlLmNv bT4KIAogICAgICAgICBNYWMgMzIgYml0IGJ1aWxkIGZpeC4KSW5kZXg6IFdlYkNvcmUvcGxhdGZv cm0vaW1hZ2UtZGVjb2RlcnMvZ2lmL0dJRkltYWdlUmVhZGVyLmNwcAo9PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBX ZWJDb3JlL3BsYXRmb3JtL2ltYWdlLWRlY29kZXJzL2dpZi9HSUZJbWFnZVJlYWRlci5jcHAJKHJl dmlzaW9uIDU5OTM5KQorKysgV2ViQ29yZS9wbGF0Zm9ybS9pbWFnZS1kZWNvZGVycy9naWYvR0lG SW1hZ2VSZWFkZXIuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC01NTcsNyArNTU3LDcgQEAgYm9vbCBH SUZJbWFnZVJlYWRlcjo6cmVhZChjb25zdCB1bnNpZ25lZAogICAgIGNhc2UgZ2lmX2ltYWdlX3N0 YXJ0OgogICAgIHsKICAgICAgIGlmICgqcSA9PSAnOycpIHsgLyogdGVybWluYXRvciAqLwotICAg ICAgICBzdGF0ZSA9IGdpZl9kb25lOworICAgICAgICBHRVROKDAsIGdpZl9kb25lKTsKICAgICAg ICAgYnJlYWs7CiAgICAgICB9CiAKQEAgLTU3MSwyMSArNTcxLDEwIEBAIGJvb2wgR0lGSW1hZ2VS ZWFkZXI6OnJlYWQoY29uc3QgdW5zaWduZWQKICAgICAgICAqIGJldHdlZW4gYmxvY2tzLiBUaGUg R0lGODdhIHNwZWMgdGVsbHMgdXMgdG8ga2VlcCByZWFkaW5nCiAgICAgICAgKiB1bnRpbCB3ZSBm aW5kIGFuIGltYWdlIHNlcGFyYXRvciwgYnV0IEdJRjg5YSBzYXlzIHN1Y2gKICAgICAgICAqIGEg ZmlsZSBpcyBjb3JydXB0LiBXZSBmb2xsb3cgR0lGODlhIGFuZCBiYWlsIG91dC4gKi8KLSAgICAg IGlmICgqcSAhPSAnLCcpIHsKLSAgICAgICAgaWYgKGltYWdlc19kZWNvZGVkID4gMCkgewotICAg ICAgICAgIC8qIFRoZSBmaWxlIGlzIGNvcnJ1cHQsIGJ1dCBvbmUgb3IgbW9yZSBpbWFnZXMgaGF2 ZQotICAgICAgICAgICAqIGJlZW4gZGVjb2RlZCBjb3JyZWN0bHkuIEluIHRoaXMgY2FzZSwgd2Ug cHJvY2VlZAotICAgICAgICAgICAqIGFzIGlmIHRoZSBmaWxlIHdlcmUgY29ycmVjdGx5IHRlcm1p bmF0ZWQgYW5kIHNldAotICAgICAgICAgICAqIHRoZSBzdGF0ZSB0byBnaWZfZG9uZSwgc28gdGhl IEdJRiB3aWxsIGRpc3BsYXkuCi0gICAgICAgICAgICovCi0gICAgICAgICAgc3RhdGUgPSBnaWZf ZG9uZTsKLSAgICAgICAgfSBlbHNlIHsKLSAgICAgICAgICAvKiBObyBpbWFnZXMgZGVjb2RlZCwg dGhlcmUgaXMgbm90aGluZyB0byBkaXNwbGF5LiAqLwotICAgICAgICAgIHJldHVybiBjbGllbnRw dHIgPyBjbGllbnRwdHItPnNldEZhaWxlZCgpIDogZmFsc2U7Ci0gICAgICAgIH0KLSAgICAgICAg YnJlYWs7Ci0gICAgICB9IGVsc2UKLSAgICAgICAgR0VUTig5LCBnaWZfaW1hZ2VfaGVhZGVyKTsK KyAgICAgIGlmICgqcSAhPSAnLCcpCisgICAgICAgIHJldHVybiBjbGllbnRwdHIgPyBjbGllbnRw dHItPnNldEZhaWxlZCgpIDogZmFsc2U7CisKKyAgICAgIEdFVE4oOSwgZ2lmX2ltYWdlX2hlYWRl cik7CiAgICAgfQogICAgIGJyZWFrOwogCg==