251136 - nsRenderingContextGTK uses mSurface after it's been freed

Status: RESOLVED FIXED

(Core Graveyard :: GFX: Gtk, defect)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I found a reproducible crash inside GTK trying to debug another bug. Here's what
valgrind says:

==7945==
==7945== Invalid read of size 4
==7945==    at 0x56D7104: nsDrawingSurfaceGTK::GetDrawable()
(nsDrawingSurfaceGTK.h:104)
==7945==    by 0x56D9892: nsRenderingContextGTK::CreateDrawingSurface(nsRect
const&, unsigned, void*&) (nsRenderingContextGTK.cpp:814)
==7945==    by 0x570B428: nsRenderingContextImpl::AllocateBackbuffer(nsRect
const&, nsRect const&, void*&, int) (nsRenderingContextImpl.cpp:105)
==7945==    by 0x56DAC37: nsRenderingContextGTK::GetBackbuffer(nsRect const&,
nsRect const&, void*&) (nsRenderingContextGTK.cpp:1453)
==7945==    by 0x414D72AD: nsViewManager::Refresh(nsView*, nsIRenderingContext*,
nsIRegion*, unsigned) (nsViewManager.cpp:832)
==7945==    by 0x414D9585: nsViewManager::DispatchEvent(nsGUIEvent*,
nsEventStatus*) (nsViewManager.cpp:1823)
==7945==    by 0x414D1923: HandleEvent(nsGUIEvent*) (nsView.cpp:76)
==7945==    by 0x3661441: nsCommonWidget::DispatchEvent(nsGUIEvent*,
nsEventStatus&) (nsCommonWidget.cpp:218)
==7945==    by 0x3654D6C: nsWindow::OnExposeEvent(_GtkWidget*, _GdkEventExpose*)
(nsWindow.cpp:1200)
==7945==    by 0x3658B23: expose_event_cb(_GtkWidget*, _GdkEventExpose*)
(nsWindow.cpp:3166)
==7945==    Address 0x463A8038 is 12 bytes inside a block of size 116 free'd
==7945==    at 0x86493D: free (vg_replace_malloc.c:231)
==7945==    by 0x80604BE: __builtin_delete (nsAppRunner.cpp:189)
==7945==    by 0x8649E3: operator delete(void*) (vg_replace_malloc.c:253)
==7945==    by 0x56D201A: nsDrawingSurfaceGTK::~nsDrawingSurfaceGTK()
(nsDrawingSurfaceGTK.cpp:115)
==7945==    by 0x56D1B80: nsDrawingSurfaceGTK::Release()
(nsDrawingSurfaceGTK.cpp:43)
==7945==    by 0x56D9924: nsRenderingContextGTK::DestroyDrawingSurface(void*)
(nsRenderingContextGTK.cpp:832)
==7945==    by 0x570B572: nsRenderingContextImpl::DestroyCachedBackbuffer()
(nsRenderingContextImpl.cpp:141)
==7945==    by 0x56DAC4F: nsRenderingContextGTK::ReleaseBackbuffer()
(nsRenderingContextGTK.cpp:1459)
==7945==    by 0x414D762C: nsViewManager::Refresh(nsView*, nsIRenderingContext*,
nsIRegion*, unsigned) (nsViewManager.cpp:914)
==7945==    by 0x414D9585: nsViewManager::DispatchEvent(nsGUIEvent*,
nsEventStatus*) (nsViewManager.cpp:1823)

Usually I think this "just works" because the old data has not yet been
clobbered, but something in my tree is causing that memory to be reused quickly
so I get a crasher here.

The core problem seems to be that mSurface is still pointing to the backbuffer
that we destroyed; we never unselected it from the rendering context.

I have a fix that causes UpdateGC to use the mOffscreenBuffer instead, which I
believe persists for the life of the rendering context. This seems to fix the bug.

The lifetime and ownership rules for drawing surfaces are insane.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

This seems to fix it.

Comment 2

[Roland Mainz]

roc:
> The lifetime and ownership rules for drawing surfaces are insane.

Guess why the Xlib, Xprint and PostScript gfx was switched over to nsCOMPtr
usage ? :)

Comment 3

[Roland Mainz]

roc:
Do you have time to port the same change to the Xlib toolkit, please ?

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

checked in.

Roland: I don't have much time. Can you do it?

Comment 5

[Roland Mainz]

Attachment: Xlib/Xprint-Patch for 2004-07-18-trunk

Comment 6

[Roland Mainz]

Comment on attachment 153551 [details] [diff] [review]
Xlib/Xprint-Patch for 2004-07-18-trunk

Requesting r=/sr=/checkin ...

Comment 7

[Roland Mainz]

roc:
Did you check the patch "in" yet ?

Comment 8

[Roland Mainz]

Comment on attachment 153551 [details] [diff] [review]
Xlib/Xprint-Patch for 2004-07-18-trunk

bz checked the patch in
(http://tinderbox.mozilla.org/bonsai/cvsquery.cgi?module=MozillaTinderboxAll&br
anch=HEAD&cvsroot=/cvsroot&date=explicit&mindate=1090463760&maxdate=1090464000&
who=bzbarsky%25mit.edu) ...

Comment 9

[Christopher Blizzard (:blizzard)]

Marking fixed.