298717 - Crash on multipart/x-mixed-replace: gif,jpeg,jpeg sequence [@ nsJPEGDecoder::OutputScanlines][@ gdk_rgb_convert_0888][@ gdk_rgb_init]

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Paul Marks]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b2) Gecko/20050622 Firefox/1.0+
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b2) Gecko/20050622 Firefox/1.0+

I created this testcase while trying to write a server-push webcam.  It seems
that when I send a gif, followed by a series of jpegs, it causes an instant
crash for Firefox and Mozilla on Linux.

Reproducible: Always

Steps to Reproduce:
1. Use Linux
2. Goto URL

Actual Results:  
Firefox went poof.

Expected Results:  
It should display some images.

The sequence is generated by a Perl script.  Here's the source:
http://web.ics.purdue.edu/~pmarks/mozilla-crash/crash.txt

Comment 1

[Andrew Schultz]

Attachment: stacktrace

Comment 2

[Andrew Schultz]

confirmed with linux suite trunk 2005062305

==> gfx:gtk

Comment 3

[Stuart Parmenter]

This is almost certainly not linux-only.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Over to a real owner...

Comment 5

[timeless]

*** Bug 254912 has been marked as a duplicate of this bug. ***

Comment 6

[timeless]

swift% uname -a
SunOS swift 5.11 snv_49 i86pc i386 i86pc
swift% mdb /usr/lib/firefox/firefox-bin /var/crash/cores/firefox-bin.8467.global.100.1162476665.swift
> $c!c++filt
libc.so.1`_lwp_kill+0x15(1, b)
libc.so.1`raise+0x22(b)
void nsProfileLock::FatalSignalHandler(int)+0xe6(b, 0, 8045184)
libc.so.1`__sighndlr+0xf(b, 0, 8045184, 806aec8)
libc.so.1`call_user_handler+0x2b8(b, 0, 8045184)
libc.so.1`sigacthandler+0xc2(b, 0, 8045184)
libgdk-x11-2.0.so.0.800.20`gdk_rgb_convert_0888+0x86(825b100, 8389018, 40, 0, 40, 20)
libgdk-x11-2.0.so.0.800.20`gdk_draw_rgb_image_core+0x160(825b100, 81b7938, 825b8f0, 0, 0, 40)
libgdk-x11-2.0.so.0.800.20`gdk_draw_rgb_image_dithalign+0x8e(81b7938, 825b8f0, 0, 0, 40, 20)
libgfx_gtk.so`void nsImageGTK::UpdateCachedImage()+0x613(f662998)
libgfx_gtk.so`unsigned nsImageGTK::Draw(nsIRenderingContext&,nsIDrawingSurface*,int,int,int,int,int,int,int,int)+0x6a(f662998, b585e70, db8ea70, 0, 0, 40)
libgfx_gtk.so`unsigned nsRenderingContextImpl::DrawImage(imgIContainer*,const nsRect&,const nsRect&)+0x528(b585e70, fe73758, 8045840, 8045830)
libgfx_gtk.so`unsigned nsRenderingContextGTK::DrawImage(imgIContainer*,const nsRect&,const nsRect&)+0x17a(b585e70, fe73758, 8045840, 8045830)
libgklayout.so`unsigned nsImageFrame::Paint(nsPresContext*,nsIRenderingContext&,const nsRect&,nsFramePaintLayer,unsigned)+0x31b(10522f80, 1030e3d0, b585e70, 8045950, 4, 0)
libgklayout.so`void nsContainerFrame::PaintChild(nsPresContext*,nsIRenderingContext&,const nsRect&,nsIFrame*,nsFramePaintLayer,unsigned)+0x105(10522910, 1030e3d0, b585e70, 8045bc0, 10522f80, 4)
libgklayout.so`void nsBlockFrame::PaintChild(nsPresContext*,nsIRenderingContext&,const nsRect&,nsIFrame*,nsFramePaintLayer,unsigned)+0x3e(10522910, 1030e3d0, b585e70, 8045bc0, 10522f80, 4)
libgklayout.so`void nsBlockFrame::PaintChildren(nsPresContext*,nsIRenderingContext&,const nsRect&,nsFramePaintLayer,unsigned)+0x500(10522910, 1030e3d0, b585e70, 8045bc0, 4, 0)
libgklayout.so`void nsHTMLContainerFrame::PaintDecorationsAndChildren(nsPresContext*,nsIRenderingContext&,const nsRect&,nsFramePaintLayer,int,unsigned)+0x1a5(10522910, 1030e3d0, b585e70, 8045bc0, 4, 1)
libgklayout.so`unsigned nsBlockFrame::Paint(nsPresContext*,nsIRenderingContext&,const nsRect&,nsFramePaintLayer,unsigned)+0x156(10522910, 1030e3d0, b585e70, 8045bc0, 4, 0)
libgklayout.so`void nsContainerFrame::PaintChild(nsPresContext*,nsIRenderingContext&,const nsRect&,nsIFrame*,nsFramePaintLayer,unsigned)+0x105(10522790, 1030e3d0, b585e70, 8045e30, 10522910, 4)
libgklayout.so`void nsBlockFrame::PaintChild(nsPresContext*,nsIRenderingContext&,const nsRect&,nsIFrame*,nsFramePaintLayer,unsigned)+0x3e(10522790, 1030e3d0, b585e70, 8045e30, 10522910, 4)
libgklayout.so`void nsBlockFrame::PaintChildren(nsPresContext*,nsIRenderingContext&,const nsRect&,nsFramePaintLayer,unsigned)+0x500(10522790, 1030e3d0, b585e70, 8045e30, 4, 0)
libgklayout.so`void nsHTMLContainerFrame::PaintDecorationsAndChildren(nsPresContext*,nsIRenderingContext&,const nsRect&,nsFramePaintLayer,int,unsigned)+0x1a5(10522790, 1030e3d0, b585e70, 8045e30, 4, 1)
libgklayout.so`unsigned nsBlockFrame::Paint(nsPresContext*,nsIRenderingContext&,const nsRect&,nsFramePaintLayer,unsigned)+0x156(10522790, 1030e3d0, b585e70, 8045e30, 4, 0)
libgklayout.so`void nsContainerFrame::PaintChild(nsPresContext*,nsIRenderingContext&,const nsRect&,nsIFrame*,nsFramePaintLayer,unsigned)+0x105(106b6f64, 1030e3d0, b585e70, 8046090, 10522790, 4)
libgklayout.so`void nsContainerFrame::PaintChildren(nsPresContext*,nsIRenderingContext&,const nsRect&,nsFramePaintLayer,unsigned)+0x61(106b6f64, 1030e3d0, b585e70, 8046090, 4, 0)
libgklayout.so`unsigned nsHTMLContainerFrame::Paint(nsPresContext*,nsIRenderingContext&,const nsRect&,nsFramePaintLayer,unsigned)+0x78(106b6f64, 1030e3d0, b585e70, 8046090, 4, 0)
libgklayout.so`unsigned CanvasFrame::Paint(nsPresContext*,nsIRenderingContext&,const nsRect&,nsFramePaintLayer,unsigned)+0x98(106b6f64, 1030e3d0, b585e70, 8046090, 4, 0)
libgklayout.so`unsigned PresShell::Paint(nsIView*,nsIRenderingContext&,const nsRect&)+0x1d2(e356ff0, aff1268, b585e70, 8046090)
libgklayout.so`unsigned nsView::Paint(nsIRenderingContext&,const nsRect&,unsigned,int&)+0x63(aff1268, b585e70, 8046090, 0, 80460ac)
libgklayout.so`void nsViewManager::RenderDisplayListElement(DisplayListElement2*,nsIRenderingContext*)+0xbe(b8367c8, c756fa0, b585e70)
libgklayout.so`void nsViewManager::RenderViews(nsView*,nsIRenderingContext&,const nsRegion&,nsIDrawingSurface*,const nsVoidArray&)+0x1ef(b8367c8, db7f740, b585e70, 8046220, db8ea70, 8046270)
libgklayout.so`void nsViewManager::Refresh(nsView*,nsIRenderingContext*,nsIRegion*,unsigned)+0x556(b8367c8, db7f740, b585e70, f0d38d8, 1)
libgklayout.so`unsigned nsViewManager::DispatchEvent(nsGUIEvent*,nsEventStatus*)+0x427(b8367c8, 80464b4, 804645c)
libgklayout.so`nsEventStatus HandleEvent(nsGUIEvent*)+0x8b(80464b4)
libwidget_gtk2.so`unsigned nsCommonWidget::DispatchEvent(nsGUIEvent*,nsEventStatus&)+0x3a(f1ab220, 80464b4, 8046520)
libwidget_gtk2.so`int nsWindow::OnExposeEvent(_GtkWidget*,_GdkEventExpose*)+0x21c(f1ab220, 825b4e8, 8046a0c)
libwidget_gtk2.so`int expose_event_cb(_GtkWidget*,_GdkEventExpose*)+0x4a(825b4e8, 8046a0c, 0)
libgtk-x11-2.0.so.0.800.20`_gtk_marshal_BOOLEAN__BOXED+0x71(8425fc8, 8046620, 2, 80466dc, 804663c, 0)
libgobject-2.0.so.0.1000.2`g_closure_invoke+0x112(8425fc8, 8046620, 2, 80466dc, 804663c)
libgobject-2.0.so.0.1000.2`signal_emit_unlocked_R+0x754(8154658, 0, 825b4e8, 804685c, 80466dc)
libgobject-2.0.so.0.1000.2`g_signal_emit_valist+0x663(825b4e8, 21, 0, 8046950)
libgobject-2.0.so.0.1000.2`g_signal_emit+0x29(825b4e8, 21, 0, 8046a0c, 8046974)
libgtk-x11-2.0.so.0.800.20`gtk_widget_event_internal+0x212(825b4e8, 8046a0c)
libgtk-x11-2.0.so.0.800.20`gtk_widget_send_expose+0x82(825b4e8, 8046a0c)
libgtk-x11-2.0.so.0.800.20`gtk_main_do_event+0x42c(8046a0c, 0)
libgdk-x11-2.0.so.0.800.20`gdk_window_process_updates_internal+0x15b(a2f5220)
libgdk-x11-2.0.so.0.800.20`gdk_window_process_all_updates+0x66(fe563dbc, 8046ab0, fe4fc713, 0, fe563dbc, 8046b38)
libgdk-x11-2.0.so.0.800.20`gdk_window_update_idle+0x26(0)
libglib-2.0.so.0.1000.2`g_idle_dispatch+0x1f(e6535e0, fe59e66c, 0)
libglib-2.0.so.0.1000.2`g_main_dispatch+0x1c8(80c3d10)
libglib-2.0.so.0.1000.2`g_main_context_dispatch+0x85(80c3d10)
libglib-2.0.so.0.1000.2`g_main_context_iterate+0x3d1(80c3d10, 1, 1, 81a1b58)
libglib-2.0.so.0.1000.2`g_main_loop_run+0x1ba(85a0260)
libgtk-x11-2.0.so.0.800.20`gtk_main+0xb3(8046ee8, 8086670, 8169600, 8046c48, fb758bb3, 816faa8)
libwidget_gtk2.so`unsigned nsAppShell::Run()+0x34(816faa8)
libtoolkitcomps.so`unsigned nsAppStartup::Run()+0x2b(8169600)
XRE_main+0x28b9(5, 8046f94, 8086648)
main+0x27(5, 8046f94, 8046fac)
_start+0x7a(5, 804711c, 8047139, 8047143, 8047145, 8047154)

The reporter of this bug thinks it's a good idea to advertise on slashdot that we haven't fixed this bug. I don't appreciate that.

Comment 7

[Daniel Veditz [:dveditz]]

Realistically I don't see a fix appearing in time for the next releases, but we'd like to take a patch when we've got one.

Comment 8

[Daniel Veditz [:dveditz]]

We're not ever going to block for this crash on the 1.8.0 branch, but if this ever gets fixed you can ask for approval on a branch-merged patch.

Comment 9

[Damon Sicore (:damons)]

We'll try to get this in beta 1; however, there's a chance this one might not make it until B2.  Per Stuart.

Comment 10

[Andrew Smith]

In case it's not already known - opening http://web.ics.purdue.edu/~pmarks/mozilla-crash/crash.pl doesn't crash. (that's the data alone, without the html code)

Does anyone know or suspect what's causing this?

Comment 11

[Andrew Smith]

And I was going to add that after going to http://web.ics.purdue.edu/~pmarks/mozilla-crash/crash.pl going to http://web.ics.purdue.edu/~pmarks/mozilla-crash/ also doesn't crash.

Comment 12

[timeless]

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/modules/libpr0n/decoders/jpeg/nsJPEGDecoder.cpp&mark=487&rev=1.72#487

Incident ID: 34871520
Stack Signature	nsJPEGDecoder::OutputScanlines() aabf39d4
Product ID	CaminoTrunk
Build ID	2007061722
Trigger Time	2007-08-12 03:13:00.0
Platform	MacOSX
Operating System	Darwin 7.9.0
Module	Camino + (001d9fa8)
URL visited	
User Comments	testing a known fatal (gtk) bug on osx
Since Last Crash	1886 sec
Total Uptime	3329793 sec
Trigger Reason	SIGBUS: Bus Error: (signal 10)
Source File, Line No.	/builds/tinderbox/CmTrunk/Darwin_8.6.0_Depend/build/ppc/modules/libpr0n/decoders/jpeg//builds/tinderbox/CmTrunk/Darwin_8.6.0_Depend/mozilla/modules/libpr0n/decoders/jpeg/nsJPEGDecoder.cpp, line 487
Stack Trace 	
nsJPEGDecoder::OutputScanlines()  [mozilla/modules/libpr0n/decoders/jpeg/nsJPEGDecoder.cpp, line 487]
nsJPEGDecoder::OutputScanlines()  [mozilla/modules/libpr0n/decoders/jpeg/nsJPEGDecoder.cpp, line 477]
imgRequest::OnDataAvailable()  [mozilla/modules/libpr0n/src/imgRequest.cpp, line 912]
nsMultiMixedConv::SendData()  [mozilla/netwerk/streamconv/converters/nsMultiMixedConv.cpp, line 840]
nsMultiMixedConv::OnDataAvailable()  [mozilla/netwerk/streamconv/converters/nsMultiMixedConv.cpp, line 524]
nsStreamListenerTee::OnDataAvailable()  [mozilla/netwerk/base/src/nsStreamListenerTee.cpp, line 840]
nsHttpChannel::OnDataAvailable()

This crash is probably easier to deal w/, OTOH fixing one might not fix others.

The obvious guess is that we don't switch decoders when the content type changes.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Please don't nuke assignees, ok?

Comment 14

[timeless]

that was bugzilla being "helpful" [automatic js], in fact, there's no easy way to get what i want which is to fix the QA contact as i move. (newer versions of bugzilla allow for this.) I should notice such nukes when i review bugmail....

Comment 15

[Andrew Smith]

Attachment: potential solution

The crash happens because in imgContainer::DecodingComplete() the gfxIImageFrame is optimised, and then nsJPEGDecoder::OutputScanlines() was trying to access its data.

There are many ways to fix it but the solution with likely the least impact on the rest of the tree is to not reuse the container. Stuart, what do you think?

Comment 16

[Alfred Kayser]

Another 'solution' would be to do SetMutable(TRUE) on re-used images, however that is not implemented.
SetMutable(FALSE) -> image->Optimize(): remove the image surface, replacing it by the OS platform image.
SetMutable(TRUE) -> NOP
To fix this, the SetMutable(TRUE) should call 'LockImagePixels' to recreate the image buffer again.
However this is costly (allocation of image buffer, and then conversion of OS surface data to the image buffer), possible just as costly (or more) than just killing the existing image and creating a new one?

Comment 17

[Andrew Smith]

I don't know more about multipart/x-mixed-replace than what I learned from reading for a few minutes on some webpage.

From what I did learn it seems that the old frames are permanently discarded, so doing any work on the image that's just about to be discarded seems to be a waste.

Comment 18

[Stuart Parmenter]

imho, the jpeg decoder should just call LockImageData() on the frame before it accesses the data and calls Unlock after.

Comment 19

[Stuart Parmenter]

Comment on attachment 276678 [details] [diff] [review]
potential solution

this patch will make it so that we don't draw over the same frame which makes animations via multipart mixed really suck

Comment 20

[Andrew Smith]

Attachment: using fixed SetMutable(false)

If I just call mFrame->LockImageData() in OutputScanlines() I get an assertion fired: trying to get data on an immutable frame: 'mMutable', file gfxImageFrame.cpp, line 276

This patch makes SetMutable(false) do something useful (LockImageData()), and I call SetMutable() if it is a multipart. These 6 lines in OutputScanlines() are almost the same as the ones at the bottom of the file.

I'm not entirely sure it's ok to leave the frame in whatever state LockImageData() puts it in, but it seems to work.

Comment 21

[Andrew Smith]

Attachment: using fixed SetMutable(false)

grr i forgot to rerun diff

Comment 22

[Stuart Parmenter]

Comment on attachment 276865 [details] [diff] [review]
 using fixed SetMutable(false)

Unlock releases the image surface if there is an optimized surface so not calling that could result in 2x the amount of memory being used for an image.  That is not good.

That said, Unlock looks like it should draw its contents in to mOptSurface if it is around which it doesn't currently do since otherwise mOptSurface will be stale.

Comment 23

[Andrew Smith]

Attachment: using LockImageData()/UnlockImageData()

stuart: is this it?

Tested on linux only, doesn't seem lilely to break on other platforms.

Comment 24

[Stuart Parmenter]

Comment on attachment 277429 [details] [diff] [review]
using LockImageData()/UnlockImageData()

this looks good.

Comment 25

[Alfred Kayser]

In UnlockImagePixels, replace:
  if (mImageSurface) {
with
  if (mImageSurface && mOptSurface) {
Otherwise SetSource could get a null pointer.

this now becomes:
     if (mImageSurface && mOptSurface) {
+        nsRefPtr<gfxContext> context = new gfxContext(mOptSurface);
+        context->SetOperator(gfxContext::OPERATOR_SOURCE);
+        context->SetSource(mImageSurface);
+        context->Paint();
         // Don't need the pixel data anymore
         mImageSurface = nsnull;
     }

Also doing Lock/Unlock for every call to OutputScanlines() can be costly, but it the most simple to force image updating.

Comment 26

[Stuart Parmenter]

they will basically be no-ops for unoptimized images so this will only really slow down OutputScanlines for multipart/x-mixed-replaced images.

Comment 27

[Andrew Smith]

Attachment: same with extra null check

Comment 28

[timeless]

are you sure you don't need approval first?

Comment 29

[Andrew Smith]

I have heard that "blockers already plus'd are auto-approved" means that bugs with blocking1.9+ don't need aproval, but I haven't confirmed that.

Comment 30

[:Gavin Sharp [email: [e-mail]]]

Comment 29 is correct per the tree rules at the top of http://tinderbox.mozilla.org/showbuilds.cgi?tree=Firefox .

Comment 31

[Phil Ringnalda (:philor)]

gfx/src/shared/gfxImageFrame.cpp 1.40
gfx/src/thebes/nsThebesImage.cpp 1.50
modules/libpr0n/decoders/jpeg/nsJPEGDecoder.cpp 1.76