863958 - ICO decoder assumes you can never have a write after you get the size [@mozilla::image::ImageMetadata::SetSize]

Status: RESOLVED FIXED

(Core :: ImageLib, defect, critical)

Description

[Christoph Diehl [:posidron]]

Attachment: testcase

Tested with m-i changeset: 129367:9ff5c0134bbf

Comment 1

[Christoph Diehl [:posidron]]

Attachment: callstack

Comment 2

[Joe Drew (not getting mail)]

Attachment: Don't call SetSize if we already have the size

In this case the ICO decoder's logic was just plain wrong; it assumed it could call SetSize as willy-nilly as it liked, which isn't the case. If we happened to have a sufficiently-large ICO, we'd get this assertion failure.

https://tbpl.mozilla.org/?tree=Try&rev=9663d8686bc0

Comment 3

[Joe Drew (not getting mail)]

Attachment: crashtest

This is Christoph's testcase.

Comment 4

[Joe Drew (not getting mail)]

Another try at try, since we had infra problems that gave us lots of false timeouts: https://tbpl.mozilla.org/?tree=Try&rev=d3f8ea4f8696

Comment 5

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 740386 [details] [diff] [review]
Don't call SetSize if we already have the size

Review of attachment 740386 [details] [diff] [review]:
-----------------------------------------------------------------

Excellent.

Comment 6

[Seth Fowler [:seth] [:s2h]]

Ironically this patch has almost identical code to the patch for bug 858818, which didn't get checked in because I was having trouble producing a working test case. I'm glad that we've got one in this bug. I'll dupe that bug to this one.

Comment 8

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 740387 [details] [diff] [review]
crashtest

Review of attachment 740387 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. I'm a bit puzzled as to why I couldn't reproduce this in the .png I generated for the other bug. I had a much bigger one (more than 10 times the size!) but it still didn't work. Strange. At any rate, it's great that we have a test case for this now!

Comment 9

[Joe Drew (not getting mail)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8658d3607e1a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/506e1fb34b30

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8658d3607e1a
https://hg.mozilla.org/mozilla-central/rev/506e1fb34b30

Comment 11

[Joe Drew (not getting mail)]

Comment on attachment 740386 [details] [diff] [review]
Don't call SetSize if we already have the size

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 716140
User impact if declined: Assertion failures in debug builds
Testing completed (on m-c, etc.): On m-c for a couple of days
Risk to taking this patch (and alternatives if risky): Not risky.
String or IDL/UUID changes made by this patch: none

Comment 12

[Alex Keybl [:akeybl]]

Comment on attachment 740386 [details] [diff] [review]
Don't call SetSize if we already have the size

In support of developers, approving for uplift to Aurora.

Comment 13

[Joe Drew (not getting mail)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9b664bf6ac2a