All users were logged out of Bugzilla on October 13th, 2018

Crash when resizing transparent window using xul:resizer [@ nsWindow::ResizeTransparencyBitmap]

RESOLVED FIXED

Status

--
critical
RESOLVED FIXED
12 years ago
8 years ago

People

(Reporter: skrulx, Assigned: skrulx)

Tracking

({crash, testcase})

Trunk
x86
Linux
crash, testcase

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(7 attachments, 2 obsolete attachments)

(Assignee)

Description

12 years ago
User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.8.0.7) Gecko/20060921 Ubuntu/dapper-security Firefox/1.5.0.7
Build Identifier: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.8.0.7) Gecko/20060921 Ubuntu/dapper-security Firefox/1.5.0.7

Using xulrunner built from the branch tag SONGBIRD_0_2A_20060830_BRANCH, I get a crash (usually something like "*** glibc detected *** free(): invalid next size (normal)") whenever I resize a transparent window using a xul:resizer.  This does not happen when using the normal resizers on the window frame.

Attached is a stack trace as well as a xul window that demonstrates the problem.

(note that this is a real pain on gnome as once the app crashes the pointer gets stuck in the "drag" state and I have to switch to a virtual console and manually kill the xulrunner process.  To make this easier, I started running the test case in vncserver)

Reproducible: Always

Steps to Reproduce:
1. Launch xulrunner with the attached xul file as the defaultChromeURI
2. Grab one of the red resizers and resize the window
3. Observe the crash
(Assignee)

Comment 1

12 years ago
Created attachment 241835 [details]
stack trace of crash
(Assignee)

Comment 2

12 years ago
Created attachment 241837 [details]
xul window test case
(Assignee)

Comment 3

12 years ago
I forgot to mention that I think the UpdateMaskBits() static function in nsWindow.cpp is causing the memory corruption.  Adding a "return" to the top of this function (so it does not execute) causes the crash to go away.  However, I could not see anything obviously wrong with this function.
(Assignee)

Comment 5

12 years ago
Is there more that I can provide then the first attachment? (https://bugzilla.mozilla.org/attachment.cgi?id=241835)

Updated

12 years ago
Severity: normal → critical
Keywords: crash, testcase
Summary: Crash @ nsWindow::ResizeTransparencyBitmap when resizing transparent window using xul:resizer → Crash when resizing transparent window using xul:resizer [@ nsWindow::ResizeTransparencyBitmap]
hum

How about a valgrind log? I'm greedy :-)
(Assignee)

Comment 7

12 years ago
Created attachment 241985 [details]
valgrind log (after window was opened)

Ok, here is a valgrind log collected by simply running the test application with "xulrunner/xulrunner -g -d valgrind application.ini".  I chopped off the beginning of the log to only include what was output after the test window had opened.  The "Invalid read of size 1" errors would start showing up after I began to resize the window using the xul:resizer and would crash after a few moves of the mouse (it does not crash immediately).  Note that I don't get any valgrind output when resizing the window using the window manager's window frame.

Let me know if the full valgrind output is useful or if you want me to run valgrind with any particular options.
You're not getting any assertion failures, right?
It looks like not.

Add
  printf("ChangedMaskBits mask=%d,%d rect=%d,%d,%d,%d\n",
         aMaskWidth, aMaskHeight, aRect.x, aRect.y, aRect.width, aRect.height);
to the start of ChangedMaskBits and
  printf("Allocating mask %d,%d -> %d\n", mBounds.width, mBounds.height, size);
before
  mTransparencyBitmap = new gchar[size];
and
  printf("RE-Allocating mask %d,%d -> %d\n", aNewWidth, aNewHeight, newSize);
before
  gchar* newBits = new gchar[newSize];

and then do the valgrind thing again... Thanks!!!
(Assignee)

Comment 10

12 years ago
Created attachment 242004 [details]
valgrind log with printfs using right resizer

Ok I added the printfs and ran it under valgrind twice.  This run I was resizing using the right resizer.  It crashed pretty quickly as you can see.
(Assignee)

Comment 11

12 years ago
Created attachment 242007 [details]
valgrind log with printfs using left resizer

Here is the second log with printfs using the left resizer to resize the window.  This took a lot longer to crash but eventually did.

Note that there is something strange about resizing a window with a xul:resizer.  The window tends to drift a bit when you are resizing it and winds up in a different location from when you started, not just a different size.  This does not happen when resizing with the window manager's window frame.  I don't know if this is related to this bug or not, or could just be some bad math in the resizer impl.
Looks like the mBounds.width changed without resizing the transparency bitmap ... I'll need to look closer to figure out how that happened
okay I found the problem. Do you want to take a stab at patching it? Here's what you have to do... mBounds.width/height get set in nsCommonWidget::Resize before we call NativeResize. So when NativeResize calls ResizeTransparencyBitmap, we exit early because the new size is the same as the current size.

The most robust solution is to add new members mTransparencyBitmapWidth and mTransparencyBitmapHeight to nsWindow. Track the width and height used to create the transparency bitmap in there. ResizeTransparencyBitmap should use those values for the bitmap's old width and height, instead of mBounds.width/height. Make sense?
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 14

12 years ago
Created attachment 242095 [details] [diff] [review]
Track transparency bitmap size independently of widget bounds

Here is a patch that I think accomplishes what you described.  It does stop the crashing, although valgrind is not 100% happy.  I'll attach the new valgrind log after this.
(Assignee)

Comment 15

12 years ago
Created attachment 242096 [details]
valgrind log with patch applied
ResizeTransparencyBitmap uses mBounds.width/mBounds.height as the old size of the bitmap, which is wrong. It needs to use mTransparencyBitmapWidth/Height.
(Assignee)

Comment 17

12 years ago
Created attachment 242212 [details] [diff] [review]
Missed a few...

Ok, I fixed the stuff I missed in ResizeTransparencyBitmap() and I also modified ApplyTransparencyBitmap() to use the transparency dimensions rather than the mBounds (is this correct?)

After these changes, I am still getting some valgrind output the first time I focus the window:

==4792== Syscall param writev(vector[...]) points to uninitialised byte(s)
==4792==    at 0x75EC8DB: writev (in /lib/libc-2.3.6.so)
==4792==    by 0x6B033FB: (within /usr/lib/libX11.so.6.2.0)
==4792==    by 0x6B08256: _XSend (in /usr/lib/libX11.so.6.2.0)
==4792==    by 0x6AF8849: XPutImage (in /usr/lib/libX11.so.6.2.0)
==4792==    by 0x6AE05D6: XCreateBitmapFromData (in /usr/lib/libX11.so.6.2.0)
==4792==    by 0x5B35144: gdk_bitmap_create_from_data (in /usr/lib/libgdk-x11-2.0.so.0.800.20)
==4792==    by 0xC39DFCE: nsWindow::ApplyTransparencyBitmap() (nsWindow.cpp:2968)
==4792==    by 0xC39E035: nsWindow::NativeShow(int) (nsWindow.cpp:2759)
==4792==    by 0xC3AE8C3: nsCommonWidget::Show(int) (nsCommonWidget.cpp:262)
==4792==    by 0xE682A3C: nsXULWindow::SetVisibility(int) (nsXULWindow.cpp:771)
==4792==    by 0xE676705: nsChromeTreeOwner::SetVisibility(int) (nsChromeTreeOwner.cpp:431)
==4792==    by 0xF11E572: nsGlobalWindow::Focus() (nsGlobalWindow.cpp:3566)
==4792==  Address 0x11004255 is 101 bytes inside a block of size 12,688 alloc'd
==4792==    at 0x4A19A16: malloc (vg_replace_malloc.c:149)
==4792==    by 0x6B0918F: _XAllocScratch (in /usr/lib/libX11.so.6.2.0)
==4792==    by 0x6AF8033: (within /usr/lib/libX11.so.6.2.0)
==4792==    by 0x6AF8849: XPutImage (in /usr/lib/libX11.so.6.2.0)
==4792==    by 0x6AE05D6: XCreateBitmapFromData (in /usr/lib/libX11.so.6.2.0)
==4792==    by 0x5B35144: gdk_bitmap_create_from_data (in /usr/lib/libgdk-x11-2.0.so.0.800.20)
==4792==    by 0xC39DFCE: nsWindow::ApplyTransparencyBitmap() (nsWindow.cpp:2968)
==4792==    by 0xC39E035: nsWindow::NativeShow(int) (nsWindow.cpp:2759)
==4792==    by 0xC3AE8C3: nsCommonWidget::Show(int) (nsCommonWidget.cpp:262)
==4792==    by 0xE682A3C: nsXULWindow::SetVisibility(int) (nsXULWindow.cpp:771)
==4792==    by 0xE676705: nsChromeTreeOwner::SetVisibility(int) (nsChromeTreeOwner.cpp:431)
==4792==    by 0xF11E572: nsGlobalWindow::Focus() (nsGlobalWindow.cpp:3566)

This only happens the very first time I focus the window and does not happen with subsequent focuses.  I added some debug output to make sure that the transparency dimensions was the same as the mBounds when ApplyTransparencyBitmap() is called, and it is.

Also, there seems to be a repainting problem.  When I make the window wider (using either the native resizer or the xul:resizer) the newly exposed area is transparent.  If I force a repaint by putting some other window above the test window and then refocusing the test window, the transparent area goes away and gets painted properly.
Attachment #242095 - Attachment is obsolete: true
Not sure about that valgrind warning. It may be an internal xlib issue.

This patch looks good. The repaint issue is likely a separate problem. Does it make a difference how much you resize at once? Is the problem only if you resize in the X direction or does it also occur when you resize vertically? ResizeTransparencyBitmap might need to fill in the ends of rows to make them opaque...
Comment on attachment 242212 [details] [diff] [review]
Missed a few...

We should get this checked in. You could get someone to check it in for you from Mozilla IRC (irc.mozilla.org #developers)
Attachment #242212 - Flags: superreview+
Attachment #242212 - Flags: review+
Assignee: nobody → skrulx
Whiteboard: [checkin needed]
(Assignee)

Comment 20

12 years ago
Thanks for walking me through this, Robert.  I'll try to get the patch committed today.

As for the repaint issue, it does not seem to matter how much I resize the window and it fails to repaint when I resize both horizontally and vertically.  I'll attach a screen shot.  In this case, the window should be entirely black with the red resizer handles on either side.

(Assignee)

Comment 21

12 years ago
Created attachment 242410 [details]
repaint issue screenshot
This was checked in by timeless earlier today.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed]
Thanks Steve.

At the ends of the two nsWindow::NativeResize methods, try adding a call to ApplyTransparencyBitmap.

(Assignee)

Comment 24

12 years ago
I tried this and it had the effect of fixing the repaint problem only when resizing the window using the xul:resizer and not the native window manager's resizer.

I tried adding it at the end of ResizeTransparencyBitmap(), which worked for both cases, but I don't know if it is optimal.  It also worked when I added it to OnSizeAllocate(), which might be a little better.
I'm surprised it worked at the end of ResizeTransparencyBitmap, because we haven't resized the native widget there (in some cases, anyway)

Let's go with OnSizeAllocate. I'll gladly review another patch :-)

Comment 26

12 years ago
Comment on attachment 242212 [details] [diff] [review]
Missed a few...

mozilla/widget/src/gtk2/nsWindow.cpp 	1.187
mozilla/widget/src/gtk2/nsWindow.h 	1.66
Attachment #242212 - Attachment is obsolete: true

Comment 27

12 years ago
i don't like resolving bugs when i don't know if they're fixed. if you want to reopen this, please do, otherwise please indicate where the new bug is for comment 25.
Product: Core → Core Graveyard
Crash Signature: [@ nsWindow::ResizeTransparencyBitmap]
You need to log in before you can comment on or make changes to this bug.