616994 - Image dragging (probably) causes big space leak

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect, P1)

Description

[Julian Seward [:jseward]]

Go to http://www.cad-comic.com/cad/20030614

Pick up image with mouse (all 6 panes are 1 big image), drag, release.
Dragged image slides back to middle of original and disappears.
(what's this functionality for?  I never figured it out.)

Repeat another 16 times.

Memcheck leak check then reports as below.  "possibly lost" means
interior pointers to the blocks were found at process exit, but no
pointers to the start thereof.  So at a bare minimum, Fx should have
freed them after each drag, but it didn't.

If you drag N times you'll see N blocks leaked.

9,504,768 bytes in 17 blocks are possibly lost in loss record 5,559 of 5,559
   at 0x4C27878: malloc (vg_replace_malloc.c:236)
   by 0x9373263: gdk_pixbuf_new (gdk-pixbuf.c:269)
   by 0x620B3BC: nsImageToPixbuf::ImgSurfaceToPixbuf(gfxImageSurface*,
   by 0x620B77C: nsImageToPixbuf::SurfaceToPixbuf(gfxASurface*, int, i
   by 0x621DA31: nsDragService::InvokeDragSession(nsIDOMNode*, nsISupp
   by 0x6226C87: nsBaseDragService::InvokeDragSessionWithImage(nsIDOMN
   by 0x5A63266: nsEventStateManager::DoDefaultDragStart(nsPresContext
   by 0x5A6A995: nsEventStateManager::GenerateDragGesture(nsPresContex
   by 0x5A6B0F4: nsEventStateManager::PreHandleEvent(nsPresContext*, n
   by 0x57F8C40: PresShell::HandleEventInternal(nsEvent*, nsIView*, ns
   by 0x57F9E50: PresShell::HandlePositionedEvent(nsIView*, nsIFrame*,
   by 0x57FD766: PresShell::HandleEvent(nsIView*, nsGUIEvent*, int, ns

Comment 1

[Julian Seward [:jseward]]

16 bit display depth (a vnc server), I should probably add.

Comment 2

[Boris Zbarsky [:bzbarsky]]

> (what's this functionality for?  I never figured it out.)

The sliding back is just standard "drop on a target that didn't accept it" feedback.  If you drag to your desktop, or url bar, or another window/tab then we'll do a more useful drop.

Looking at InvokeDragSession, this seems to be the relevant code:


257           if (!SetAlphaPixmap(surface, context, offsetX, offsetY, dragRect)) {
258             GdkPixbuf* dragPixbuf =
259               nsImageToPixbuf::SurfaceToPixbuf(surface, dragRect.width, dragRect.height);
260             if (dragPixbuf)
261               gtk_drag_set_icon_pixbuf(context, dragPixbuf, offsetX, offsetY);

I believe that if clause should look like this:

  if (dragPixbuf) {
    gtk_drag_set_icon_pixbuf(context, dragPixbuf, offsetX, offsetY);
    g_object_unref(dragPixbuf);
  }

(and that we should document the static nsImageToPixbuf methods as requiring a caller unref).  Does making that change fix the leak?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Don't leak the buffer we create for the drag image.

OK, I verified that I can see the leak with |top| with a vanilla build, but not after applying this patch.

Comment 4

[Julian Seward [:jseward]]

Verified here too.

Is the first hunk in the patch relevant?  It looks like it
accidently crept in from somewhere else.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Without the bogus layout change

Uh... yes.  That hunk shouldn't be there!

Comment 6

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/25cd87d5c081