Seth Fowler [:seth] [:s2h] Reporter
	Description • 11 years ago

So I can't help but notice that we make calculations similar to the following all over the place in RasterImage:

      aImg->mDecodeRequest->mBytesToDecode = aImg->mSourceData.Length() - aImg->mBytesDecoded;

However, when we aren't storing source data, mBytesDecoded just keeps growing, while mSourceData.Length() stays constant (I believe store just enough data in there to get us through the size decode phase and then never write anything else in.) This is _ultra bad_. mBytesToDecode is unsigned, for example, and ends up wrapping around to a very large number once mBytesDecoded gets larger than mSourceData.Length(), which it rapidly does. There are many other calculations which clearly don't work correctly either.

Frankly I'm a bit puzzled as to how we're able to limp along despite this problem when we aren't storing source data!

	Seth Fowler [:seth] [:s2h] Reporter
	Comment 1 • 11 years ago

I made an attempt to fix this naively by adding an mSourceBytes field which records the true number of bytes and switching all of the calculations over to use that, but that caused a number of issues that I wasn't able to quickly resolve. Joe, I think you might be able to fix this problem faster than I can.

	Seth Fowler [:seth] [:s2h] Reporter
	Comment 2 • 11 years ago

Bug 856602 provides a good test case.

	Joe Drew (not getting mail) Assignee
	Comment 3 • 11 years ago

Turns out this should actually not happen in practice; I removed some code previously that I should've kept and adapted. Patch coming up.

	Joe Drew (not getting mail) Assignee
	Comment 4 • 11 years ago

I'm crossing my fingers that this fixes other crashes, like bug 861552. I definitely was able to reproduce assertion failures in some cases when trying to reproduce that bug, and can't any more.

	Joe Drew (not getting mail) Assignee
	Comment 5 • 11 years ago

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=d5055a412f69

	Seth Fowler [:seth] [:s2h] Reporter
	Comment 6 • 11 years ago

Comment on attachment 737618 [details] [diff] [review]
shutdown the decoder when data finished if we're not storing source data

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

Looks good. The first couple of times I tried this with http://www.shioda-dcom.co.jp/shiocame/ the mJPEG stopped refreshing with a broken image, which led me to do a bunch of investigation, but at this point I *think* there's no problem and it was just a server-side issue.

	Joe Drew (not getting mail) Assignee
	Comment 7 • 11 years ago

Could also have been bug 791258.

	Joe Drew (not getting mail) Assignee
	Comment 8 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/baec6e6659a8

If all goes well, I'll nominate this for aurora too.

	Ryan VanderMeulen [:RyanVM]
	Comment 9 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/baec6e6659a8

	Seth Fowler [:seth] [:s2h] Reporter
	Comment 10 • 11 years ago

This appeared to resolve a crash noted in bug 861595, although I can't really make this bug block that one since the main topic of bug 861595 is different.

	Joe Drew (not getting mail) Assignee
	Comment 11 • 11 years ago

Comment on attachment 737618 [details] [diff] [review]
shutdown the decoder when data finished if we're not storing source data

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 716140
User impact if declined: Crashes
Testing completed (on m-c, etc.): On m-c for 10 days.
Risk to taking this patch (and alternatives if risky): Not terribly risky. 
String or IDL/UUID changes made by this patch: None

	Joe Drew (not getting mail) Assignee
	Comment 12 • 11 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/491f071eeeb6