409006 - Huge alert() window causes application exit() (XError BadAlloc or RenderBadPicture or BadDrawable)

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Mats Palmgren (inactive)]

Huge alert() window causes application exit() (XError BadAlloc or RenderBadPicture or BadDrawable).

STEPS TO REPRODUCE
1. load attached testcase

ACTUAL RESULT

The error was 'RenderBadPicture (invalid Picture parameter)'.
  (Details: serial 8403 error_code 182 request_code 157 minor_code 8)

or

The error was 'BadAlloc (insufficient resources for operation)'.
  (Details: serial 12055 error_code 11 request_code 53 minor_code 0)

or

The error was 'BadDrawable (invalid Pixmap or Window parameter)'.
  (Details: serial 4610 error_code 9 request_code 66 minor_code 0)

Comment 1

[Mats Palmgren (inactive)]

Attachment: alert() with huge height and width

Comment 2

[Mats Palmgren (inactive)]

Attachment: alert() with huge height

Comment 3

[Mats Palmgren (inactive)]

Attachment: alert() with huge width

Comment 4

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

Clamping the window to twice the screen size in either direction
fixes it for me.

Comment 5

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

What if the user has three screens? Before this patch they could have made a window big enough to span all three screens, now they can't.

Comment 6

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2

I couldn't find a way enumerate nsIScreens so I added
  nsIScreen getScreenAt(in unsigned long index);
to nsIScreenManager.idl

and then used that to calculate the union rect for all the screens,
then limit the window to twice the width/height of that union rect.

I did basic testing to verify that it doesn't break Windows and MacOSX
and it fixes the crash for me on Linux.  In all three cases on a
single-monitor system.

Comment 7

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

+    if (screenManager) {

Just exit early if !screenManager.

+static void
+SetSafeWindowSize(nsRect& aBounds, PRInt32 aWidth, PRInt32 aHeight)

Why not call it GetSafeWindowSize and just return an nsSize (and take an nsSize parameter)? Then use mBounds.SizeTo(...)

+        screenInfo = _XnrmQueryScreens(GDK_DISPLAY(), &numScreens);
+        mNumScreens = PR_MAX(0, numScreens);

When is numScreens negative?

+  nsCOMPtr<nsIScreen> screen;
+  rv = mCachedScreenArray->GetElementAt(aIndex, getter_AddRefs(screen));
+  *aScreen = screen.get();
+  NS_IF_ADDREF(*aScreen);

Why don't you just pass aScreen directly to GetElementAt?

+  nsCOMPtr<nsIScreen> screen;
+  rv = mCachedScreenArray->GetElementAt(0, getter_AddRefs(screen));
   *aPrimaryScreen = screen.get();
   NS_IF_ADDREF(*aPrimaryScreen);

Same here

+  struct CountMonitorsData* data =
+    reinterpret_cast<struct CountMonitorsData*>(ioParam);

Can't this be a static_cast instead?

Grrr, all this screen code could use a good cleanup...

Comment 8

[Karl Tomlinson (:karlt)]

I don't see this error with the testcases here (probably because my Xserver is using XAA), but this looks like it could be a dupe of bug 402204, which may be INVALID as it's not a bug in Mozilla.  See bug 402204 comment 23.

Comment 9

[Mats Palmgren (inactive)]

I haven't tested bug 402204 but it looks more like bug 424333.
Anyway, I disagree that either of these bugs are invalid just because
the error occurs in non-mozilla code -- we should do our best to
avoid crashing when we can, especially in cases like these.

Comment 10

[Mats Palmgren (inactive)]

Attachment: Patch rev. 3

(In reply to comment #7)
static_cast did not compile since the type is LPARAM, other than that
your comments were addressed.  I switched to
nsCOMArray<nsIScreen> mCachedScreenArray; to simplify the code a bit.

Comment 11

[Karl Tomlinson (:karlt)]

Sorry, this is not a dupe of bug 402204, assuming that's the "pixmaps of width 8192 or greater" issue at http://cgit.freedesktop.org/xorg/xserver/commit/?id=bc2d516f16d94c805b4dfa8e5b9eef40ff0cbe98 , as that is only large widths, not large heights.

I did manage to reproduce a couple of times in a Gentoo build with attachment 293848 [details] and XAA, but not yet with a debug build or while I have gdb attached to the distro build.

I don't seem to be getting useful info from XineramaQueryScreens.  I have one X Screen with two monitors but XineramaQueryScreens only gives 1 output device with the dimensions of only one of the monitors.  Maybe that's another X bug, but kwin seems to know the dimensions, so maybe it uses Xrandr instead.  (Migrating to Xrandr is beyond this bug I think.)

If we want to constrain the window size, isn't what we want here, the size of the X Screen, not the size of the output device?  With a virtual screen, don't we want the size of that virtual Screen, rather than the size of an output device that may be displaying a portion of the Screen?  (I don't think one window can't span more than one X Screen.)

I would have thought that it was the window manager's role to sensibly constrain the requested window size and kwin seems to do that, but I haven't yet worked out why a large window or pixmap is being created.

Comment 12

[Karl Tomlinson (:karlt)]

(In reply to comment #11)
> I would have thought that it was the window manager's role to sensibly
> constrain the requested window size

Scratch that.  Expecting the window manager to do anything with non-toplevel windows is unreasonable.

So the app constraining window sizes based on the Screen size does seem the best option.

Comment 13

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

(In reply to comment #11)
> I don't seem to be getting useful info from XineramaQueryScreens.  I have one X
> Screen with two monitors but XineramaQueryScreens only gives 1 output device
> with the dimensions of only one of the monitors.  Maybe that's another X bug,
> but kwin seems to know the dimensions, so maybe it uses Xrandr instead. 
> (Migrating to Xrandr is beyond this bug I think.)
> 
> If we want to constrain the window size, isn't what we want here, the size of
> the X Screen, not the size of the output device?  With a virtual screen, don't
> we want the size of that virtual Screen, rather than the size of an output
> device that may be displaying a portion of the Screen?  (I don't think one
> window can't span more than one X Screen.)

This sounds reasonable to me, what do you think Mats?

Comment 14

[Mats Palmgren (inactive)]

(In reply to comment #11)
> I don't seem to be getting useful info from XineramaQueryScreens.

Did you check _XnrmIsActive(display) return non-zero before using it?

(In reply to comment #13)
> This sounds reasonable to me, what do you think Mats?

Sure.

Comment 15

[Mats Palmgren (inactive)]

Attachment: crashtest.diff

Comment 16

[Mats Palmgren (inactive)]

Attachment: Patch rev. 4

Comment 17

[Karl Tomlinson (:karlt)]

(In reply to comment #14)
> (In reply to comment #11)
> > I don't seem to be getting useful info from XineramaQueryScreens.
> 
> Did you check _XnrmIsActive(display) return non-zero before using it?

Yes, _XnrmIsActive(GDK_DISPLAY()) did return non-zero, thanks.

It seems that the server (including xrandr) had just forgotten that it was running that monitor (which would also explain why menus were popping up on the other monitor).

xdpyinfo -ext XINERAMA:
XINERAMA version 1.1 opcode: 153
  head #0: 1680x1050 @ 0,0

xrandr:
Screen 0: minimum 320 x 200, current 1680 x 1850, maximum 1680 x 1850
VGA_1 disconnected
PANEL connected
   1280x800       60.0 +
DVI-D_1 connected 1680x1050+0+0 474mm x 296mm
   1680x1050      60.0*+   59.9

Running "xrandr --output PANEL --auto --pos 200x1050" refreshed the driver's
info:

XINERAMA version 1.1 opcode: 153
  head #0: 1280x800 @ 200,1050
  head #1: 1680x1050 @ 0,0

Screen 0: minimum 320 x 200, current 1680 x 1850, maximum 1680 x 1850
VGA_1 disconnected
PANEL connected 1280x800+200+1050 331mm x 207mm
   1280x800       60.0*+
DVI-D_1 connected 1680x1050+0+0 474mm x 296mm
   1680x1050      60.0*+   59.9

(In reply to comment #16)
> Created an attachment (id=327402) [details]
> Patch rev. 4

Thank you for this.  This also has the advantage of actually getting the
_current_ Screen size.  When XineramaIsActive, nsScreen* just describe the
situation at the time when nsScreenManagerGtk::EnsureInit is first invoked.

Comment 18

[Mats Palmgren (inactive)]

Comment on attachment 327401 [details] [diff] [review]
crashtest.diff

This crash test didn't work since it never reached onload due to
the alert(), I suppose.

Comment 19

[Mats Palmgren (inactive)]

Attachment: crashtest2.diff

Second attempt at making a crash test.  This passed locally but caused a
later crash test (403296-1.xhtml) to fail loading on Tinderboxes.
Don't know why.

Comment 20

[Mats Palmgren (inactive)]

http://hg.mozilla.org/mozilla-central/index.cgi/rev/af0281bfafdb

-> FIXED

Comment 21

[Mats Palmgren (inactive)]

Comment on attachment 327402 [details] [diff] [review]
Patch rev. 4

Low risk "crash" fix.

Comment 22

[Samuel Sidler (old account; do not CC)]

Mats, any work on getting a crash test landed? Prefer to see that landed before taking this in 1.9.0.x...

Comment 23

[Sylvain Pasche]

This introduces a rather bad regression on Google Reader (bug 451636). IMHO it shouldn't land on branch until this is fixed.

Comment 24

[Samuel Sidler (old account; do not CC)]

Comment on attachment 327402 [details] [diff] [review]
Patch rev. 4

Not going to take for 1.9.0.2. Please re-nom for 1.9.0.3 after there is a crash test landed and the regression is fixed.

Comment 25

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Hrm, I don't understand -- why are we constraining sizes to 2x screen dimensions, instead of to X 16-bit coordinate limits?  The test cases all blow well past the signed 16-bit range, and that's what presumably causes the bug, no?

Comment 26

[Mats Palmgren (inactive)]

The 2x screen dimensions came from bug 394859.  It seemed like a good limit
in this case too, although I only meant it for top level windows.
We can make it 32767 and hope it works for all X servers, until proven
otherwise.  I'll fix it in bug 451636.

Comment 27

[Bob Clary [:bc] (inactive)]

gfx/thebes/crashtests/409006-1.html
gfx/thebes/crashtests/409006-2.html
gfx/thebes/crashtests/409006-3.html
http://hg.mozilla.org/mozilla-central/rev/c323a64b048b

Comment 28

[Bob Clary [:bc] (inactive)]

crap, these were backed out in http://hg.mozilla.org/mozilla-central/rev/dc1e02b89c94. Shouldn't we disable them rather than completely remove them from the tree? sorry for the spam.