This first appeared on 20150318. Then spiked in volume on 20150320. No crashes with builds since. This bug was filed from the Socorro interface and is report bp-103f0e9b-5ff9-4a6b-a4f3-106522150320. ============================================================= Frame Module Signature Source 0 xul.dll skia::ConvolutionFilter1D::FilterForValue(int, int*, int*) gfx/2d/convolver.h 1 xul.dll mozilla::image::Downscaler::CommitRow() image/src/Downscaler.cpp 2 xul.dll mozilla::image::nsJPEGDecoder::OutputScanlines(bool*) image/decoders/nsJPEGDecoder.cpp 3 xul.dll mozilla::image::nsJPEGDecoder::WriteInternal(char const*, unsigned int) image/decoders/nsJPEGDecoder.cpp 4 xul.dll mozilla::image::Decoder::Write(char const*, unsigned int) image/src/Decoder.cpp 5 xul.dll mozilla::image::Decoder::Decode() image/src/Decoder.cpp 6 xul.dll mozilla::image::DecodePool::Decode(mozilla::image::Decoder*) image/src/DecodePool.cpp 7 xul.dll mozilla::image::DecodeWorker::Run() image/src/DecodePool.cpp 8 xul.dll nsThreadPool::Run() xpcom/threads/nsThreadPool.cpp 9 xul.dll nsThread::ProcessNextEvent(bool, bool*) xpcom/threads/nsThread.cpp 10 xul.dll NS_ProcessNextEvent(nsIThread*, bool) xpcom/glue/nsThreadUtils.cpp 11 xul.dll mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) ipc/glue/MessagePump.cpp 12 xul.dll MessageLoop::RunHandler() ipc/chromium/src/base/message_loop.cc 13 xul.dll MessageLoop::Run() ipc/chromium/src/base/message_loop.cc 14 xul.dll nsThread::ThreadFunc(void*) xpcom/threads/nsThread.cpp 15 nss3.dll _PR_NativeRunThread nsprpub/pr/src/threads/combined/pruthr.c 16 nss3.dll pr_root nsprpub/pr/src/md/windows/w95thred.c 17 msvcr120.dll _callthreadstartex f:\dd\vctools\crt\crtw32\startup\threadex.c:376 18 msvcr120.dll msvcr120.dll@0x2c000 19 kernel32.dll BaseThreadInitThunk 20 ntdll.dll __RtlUserThreadStart 21 ntdll.dll _RtlUserThreadStart

I expect this is caused by decode on downscale.

Investigating this.

OK, I found it. This code is not super intuitive, which made it harder to see the problem. In part 1 I just want to fix a few of those issues. More specifically: - There was some misleading formatting. - FilterForValue() is sometimes called to actually retrieve a filter, and sometimes purely to get the filter's offset and length. That's pretty misleading to the reader, so I wrapped the callsites that just wanted to grab the offset and length in a helper method. - I added assertions that FilterForValue()'s first argument is in the correct range everywhere that it's called.

This patch actually fixes the bug. The issue is pretty straightforward: when we're at the end of our output buffer, DownscaleInputLine() sets mCurrentOutLine equal to mTargetSize.height(). We also check for that in the loop condition in CommitRow(). But, after DownscaleInputLine() runs but *before* we check that loop condition, we call GetFilterOffsetAndLength() and pass in mCurrentOutLine as the position argument. The problem is that when we're at the end of our output buffer, mCurrentOutLine no longer points to a valid position. That can lead to a crash inside Skia code. The fix is straightforward: we check whether we've reached the end of the output buffer between DownscaleInputLine() and the call to GetFilterOffsetAndLength(). Rather than check the same thing again in the loop condition, I've turned that into an assertion, since the only time the difference is important is for the first time through the loop, when mCurrentOutLine should definitely be less than mTargetSize.height.

Thanks for the quick review!

https://hg.mozilla.org/integration/mozilla-inbound/rev/b0f92f800921 https://hg.mozilla.org/integration/mozilla-inbound/rev/4f0f41035aa3

https://hg.mozilla.org/mozilla-central/rev/b0f92f800921 https://hg.mozilla.org/mozilla-central/rev/4f0f41035aa3

Nominating for ESR per bug 1224200 comment 20

Comment on attachment 8590039 [details] [diff] [review] (Part 2) - Fix an off-by-one error in image::Downscaler [Approval Request Comment] If this is not a sec:{high,crit} bug, please state case for ESR consideration: needed so another uplift make sense User impact if declined: crashes Fix Landed on Version: m-c 44 Risk to taking this patch (and alternatives if risky): pretty safe String or UUID changes made by this patch: none See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment on attachment 8590035 [details] [diff] [review] (Part 1) - Add assertions and fix style issues in image::Downscaler Same answers as other patch.

If this fix landed in m-c last in April 2015 (comment 9), it should already be in ESR38 since 38.1.0 was released in July 2015. Timothy is that not the case?

Looking at the file in mxr on esr38 it does not have this patch http://mxr.mozilla.org/mozilla-esr38/source/image/src/Downscaler.cpp The same is true of my local checkout of esr38.

Never mind, I'm wrong, it would have had to be in m-r in order to land in 38.1.0 in July. this is why having lunch and a lot of coffee is good for a person...

Fixing the flags back to what they should be.

Comment on attachment 8590035 [details] [diff] [review] (Part 1) - Add assertions and fix style issues in image::Downscaler Please uplift to esr38, both patches. Needed to support another sec rated crash fix.

Comment on attachment 8590039 [details] [diff] [review] (Part 2) - Fix an off-by-one error in image::Downscaler Uplift to esr38 for the 38.6.0esr release. Needed to support another crash fix.

https://hg.mozilla.org/releases/mozilla-esr38/rev/9d14787bd10e https://hg.mozilla.org/releases/mozilla-esr38/rev/8bfaa27698ca