Found via bughunter and reproduced with a windows 7 debug build. Steps to reproduce: Load http://adkelectricinc.com/ Assertion failure: mTargetSize.width <= aOriginalSize.width (Created a downscale r, but width is larger), at c:/Users/mozilla/debug-builds/mozilla-central/image/ Downscaler.cpp:68 #01: mozilla::image::nsGIFDecoder2::BeginImageFrame (c:\users\mozilla\debug-buil ds\mozilla-central\image\decoders\nsgifdecoder2.cpp:283) #02: mozilla::image::nsGIFDecoder2::WriteInternal (c:\users\mozilla\debug-builds \mozilla-central\image\decoders\nsgifdecoder2.cpp:1106) #03: mozilla::image::Decoder::Write (c:\users\mozilla\debug-builds\mozilla-centr al\image\decoder.cpp:186) #04: mozilla::image::Decoder::Decode (c:\users\mozilla\debug-builds\mozilla-cent ral\image\decoder.cpp:128) #05: mozilla::image::DecodePool::Decode (c:\users\mozilla\debug-builds\mozilla-c entral\image\decodepool.cpp:455) #06: mozilla::image::DecodePoolWorker::Run (c:\users\mozilla\debug-builds\mozill a-central\image\decodepool.cpp:292) #07: nsThread::ProcessNextEvent (c:\users\mozilla\debug-builds\mozilla-central\x pcom\threads\nsthread.cpp:960) #08: NS_ProcessNextEvent (c:\users\mozilla\debug-builds\mozilla-central\xpcom\gl ue\nsthreadutils.cpp:277) #09: mozilla::ipc::MessagePumpForNonMainThreads::Run (c:\users\mozilla\debug-bui lds\mozilla-central\ipc\glue\messagepump.cpp:355) #10: MessageLoop::RunInternal (c:\users\mozilla\debug-builds\mozilla-central\ipc \chromium\src\base\message_loop.cc:234) #11: MessageLoop::RunHandler (c:\users\mozilla\debug-builds\mozilla-central\ipc\ chromium\src\base\message_loop.cc:228) #12: MessageLoop::Run (c:\users\mozilla\debug-builds\mozilla-central\ipc\chromiu m\src\base\message_loop.cc:202) #13: nsThread::ThreadFunc (c:\users\mozilla\debug-builds\mozilla-central\xpcom\t hreads\nsthread.cpp:384) #14: _PR_NativeRunThread (c:\users\mozilla\debug-builds\mozilla-central\nsprpub\ pr\src\threads\combined\pruthr.c:397) #15: pr_root (c:\users\mozilla\debug-builds\mozilla-central\nsprpub\pr\src\md\wi ndows\w95thred.c:90) #16: _get_flsindex[MSVCR120 +0x2c01d] #17: _get_flsindex[MSVCR120 +0x2c001] #18: BaseThreadInitThunk[kernel32 +0x4ee1c] #19: RtlInitializeExceptionChain[ntdll +0x637eb] #20: RtlInitializeExceptionChain[ntdll +0x637be]

seth: could you take a look at that ? Thanks!

I can't reproduce this. =( Carsten, I assume you reproduced this at default page zoom? Can you check in about:config what the value of "layout.css.devPixelsPerPx" is? If it's -1, can you try setting it to one of these settings and let me know if it reproduces? "1.0", "1.25", "1.5", "2.0" Trying to figure out what's different about your settings vs. mine.

(FWIW, I can't reproduce at any of those settings.)

I reproduced on a win 7 32bit vm with bp-157ef1ab-8421-4343-8749-f26062150924 [@ convolveVertically_SSE2<T>(short const*, int, unsigned char* const*, int, unsigned char*) ] bp-d043d3b8-2318-4330-b4c0-0cd4f2150924 [@ jemalloc_crash | moz_xmalloc | std::vector<T>::_Reallocate(unsigned int) ] locally with osx 10.9.5 bp-e004a50b-aa87-4858-aaf2-a70b02150924 [@ nsBlockFrame::ReflowDirtyLines(nsBlockReflowState&) ] my local setting for layout.css.devPixelsPerPx is -1. changing it to: 1 : bp-e5c6f320-9d51-41be-a156-45f592150924 [@ nsViewManager::IncrementDisableRefreshCount() ] 1.25: bp-d84cd0a4-baf4-4460-8fed-635262150924 [@ jemalloc_crash | je_malloc_usable_size | libsystem_malloc.dylib@0x10f23 ] 1.5: No crash 2.0: No Crash s-s since I don't like the variety of stacks.

I've attached asan errors for the original url and for another url which has also reproduced the original assertion. They show AddressSanitizer: heap-use-after-free | AddressSanitizer: heap-buffer-overflow I haven't included the second url since it is NSFW, but if you need it please contact me (bc) via irc.

Here's the GIF in question. I can reproduce reliably by viewing this GIF and zooming out. (Often I'll crash without having to zoom out, which I think is due to downscaling for the favicon.)

With hex addresses we see 0x5a mLinesInBuffer = 0xa5a5a5a5 mPrevInvalidatedLine = 0xa5a5a5a5 mCurrentOutLine = 0xa5a5a5a5 mCurrentInLine = 0xa5a5a5a5

FWIW, those addresses are 0x84848484 for me (not 0xa5a5a5a5). Here's a similar gdb session from me, putting a breakpoint at Downscaler::BeginFrame just before the assertion has tripped.

Comment on attachment 8665723 [details] p/x *this just before assertion-failure, in dholbert's gdb log (non-jemalloc) Oh, but this is in a --disable-jemalloc build -- that probably explains my lack of 0xa5a5a5a5.

seems affect aurora and trunk so far and seems also more widespread like with multiple sites

[Tracking Requested - why for this release]: cf #comment 11.

Bob, Daniel, thanks so much for your investigations here. Your help has been invaluable! (In reply to Daniel Holbert [:dholbert] from comment #6) > Here's the GIF in question. I can reproduce reliably by viewing this GIF and > zooming out. (Often I'll crash without having to zoom out, which I think > is due to downscaling for the favicon.) For some reason I cannot reproduce when the GIF is embedded in a page, but I can reproduce viewing it directly if I zoom out as far as possible. So I've finally been able to look at the crash directly. I've confirmed that this is *yet another* dupe of bug 1207378. (Though I won't mark it as a dupe until the fix lands due to this being a security bug.) I've already pushed the patch to inbound, so this should be fixed on trunk as soon as inbound gets merged. I'll request uplift to Aurora at that point too.

Bob can we rerun the urls that hit this crash ? to confirm its fixed or not ?

I started a re-test on Sunday for all of the urls seen since 8/10 (When Firefox 40 was released) but that was before this landed on mozilla-central. Damn, I thought this landed earlier. The re-test will take another couple of days. I'll redo after they finish.

Not fixed on Nightly or Aurora. Example http://electoralsearch.in/

confirmed only the line number seems have changed from /Downscaler.cpp:69 where it was 68 before

I somehow missed that this was still happening. Will reinvestigate tomorrow.

Any news here Seth? Does this still affect 43 or is it only happening on nightly/aurora channels?

Too late for 43.

[Tracking Requested - why for this release]:

still reproduces on http://electoralsearch.in/ - stack attached

The problem at that URL is with our .ico handling. ICOs can have a number of BMPs inside them with different sizes. We render the smallest one that's larger than the target size (if such a BMP exists -- otherwise we just choose the largest). If we have a BMP larger than the target size, we create a downscaler. The heuristic for choosing which BMP to use was slightly wrong, so we could sometimes choose a smaller BMP than our target size (on one axis). This leads to the downscaler failing this assertion. This patch simply fixes that heuristic.

We are in RC mode, it's too late and this is now a wontfix for Fx44.

Tracking because it seems we are close to have a proper fix.

Comment on attachment 8706316 [details] [diff] [review] 1207958.patch Sorry for the delay.

Comment on attachment 8706316 [details] [diff] [review] 1207958.patch Approval Request Comment [Feature/regressing bug #]: ICO decoding. [User impact if declined]: Some ICO images can cause crashes. [Describe test coverage new/current, TreeHerder]: wfm. [Risks and why]: Very, very low. [String/UUID change made/needed]: None.

Comment on attachment 8706316 [details] [diff] [review] 1207958.patch Fix a crash, taking it.

al, dan: do we need also a security rating here ?

Yes, this shouldn't have been checked in without sec-approval+ being given. We need the sec-approval? flag set on the patch and the security questions answered. https://wiki.mozilla.org/Security/Bug_Approval_Process

Now that this patch has landed in public do we need to chemspill or re-spin the RC for it? Is the minor match change patch obscure enough that we can wait another 6 weeks? Really needed the sec-approval questions answered on this one.

If this one is easily exploitable and chemspill-worthy, I don't see any choice other than doing another RC 44.0-build3 with this fix. Is it chemspill-worthy? So far I haven't felt the need to do a build3 yet so this issue will be the only driving force behind another RC build. Please let me know which way to proceed.

Note: for posterity & automated-testcase purposes down the line, here's a testcase that reproduces the electoralsearch.in fatal assertion in debug builds that don't have the patch. (Unlike testcase 1, this testcase (and the electoralsearch.in site) doesn't crash opt builds.)

Dan, Al: I really do not want to do a build3 unless it is a must. Could you please let me know what your stand on this issue is? Is it a chemspill-worthy issue or not? Please keep in mind we go live on Tuesday.

I can't answer that question without Edwin or Milan answering the sec-approval questions.

Comment on attachment 8706316 [details] [diff] [review] 1207958.patch [Security approval request comment] How easily could an exploit be constructed based on the patch? Not easily. Looks like a run-of-the-mill bugfix. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No. Which older supported branches are affected by this flaw? 43+ If not all supported branches, which bug introduced the flaw? bug 1207378 Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Trivial. Applies cleanly on Aurora. How likely is this patch to cause regressions; how much testing does it need? Very unlikely.

OK, we don't need to stop-ship/re-spin Fx 44.

(In reply to Daniel Veditz [:dveditz] from comment #42) > OK, we don't need to stop-ship/re-spin Fx 44. Thanks Dan and Edwin.

I was able to reproduce this issue on Firefox 44.0a1 (2015-09-24) using Windows 10 64-bit. Verified fixed on 46.0a2 (2016-03-02), Firefox 45 RC (20160301003640) and Firefox 45.0 ESR (20160302195223) under Windows 10 64-bit, Ubuntu 12.04 32-bit and Mac OS X 10.11.