59355 - very large font size causes GDK to exit

Status: VERIFIED FIXED

(Core Graveyard :: GFX, defect, P3)

Description

[David Harkness]

Browser crashes if font size is over 13,000 pt.


body {margin-top: 5px; margin-bottom:0px; margin-left:0px; margin-right:0px;
font-size: 13433pt; font-family: verdana, helvetica, sans-serif; font-weight: bold }

Comment 1

[Boris Zbarsky [:bzbarsky]]

I see this with Linux trunk build 20000110608.

The problem here is that the font is never allocated -- I do not have enough
memory for xfs to properly scale the font.  (I have 384MB physical + 256MB swap,
and that's insufficient).

The error I get:
Gdk-ERROR **: BadAlloc (insufficient resources for operation)
  serial 4502 error_code 11 request_code 45 minor_code 0

And then the Mozilla dies.  Running under gdb confuses gdb -- it still thinks
Mozilla is running but can't kill it because of:
"Error accessing memory address 0x401b98c0: No such process."

Comment 2

[Loco]

Boy, this is a good one:).  Is this even valid?  Tossing over to CSS groupies.

Comment 3

[uamjet602]

Well if we consider fonts this big a stupid thing to do, we should cap them.

Comment 4

[Jacob Kjome]

I just checked with a testcase that I'll post and both Mozilla and IE 5.5 seems 
to render this just fine.  It doesn't use up a whole lot of memory either; 
certainly not "384MB physical + 256MB swap".  I am on Win2k with 128 meg ram 
and 192 meg swap.  Looks like this is a Linux only issue.

Jake

Comment 5

[Jacob Kjome]

Attachment: Tescase showing display of 13433pt text

Comment 6

[Boris Zbarsky [:bzbarsky]]

This is, of course, an X-only (Unix-only, essentially) issue.  In X all fonts
have to be converted to bitmaps before being rendered to the screen.  A set of
bitmaps for a 13000pt font is huge.

On Windows/Mac, the system directly handles vector fonts, so there is no such
problem....

Comment 7

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Giving to erik to see what he wants to do with this.  Hopefully it can be fixed
with appropriate null-checks so we get to the point where we just don't display
anything for the font.  Or perhaps we can guard against fonts that are too big...

Comment 8

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Giving to erik to see what he wants to do with this.  Hopefully it can be fixed
with appropriate null-checks so we get to the point where we just don't display
anything for the font.  Or perhaps we can guard against fonts that are too big...

Comment 9

[Keyser Sose]

Is this still a problem in the latest nightlies?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Still there in CVS build from 2000-12-12.

I would say this is a valid crash bug, the question is what should be done about
it.  As such, confirming.

Comment 11

[Frank Tang]

I search the GTK source tree for
Gdk-ERROR **: BadAlloc (insufficient resources for operation)
  serial 4502 error_code 11 request_code 45 minor_code 0

Here is the function in gtk+-1.2.6/gdk/gdk.c
/*
 *--------------------------------------------------------------
 * gdk_x_error
 *
 *   The X error handling routine.
 *
 * Arguments:
 *   "display" is the X display the error orignated from.
 *   "error" is the XErrorEvent that we are handling.
 *
 * Results:
 *   Either we were expecting some sort of error to occur,
 *   in which case we set the "gdk_error_code" flag, or this
 *   error was unexpected, in which case we will print an
 *   error message and exit. (Since trying to continue will
 *   most likely simply lead to more errors).
 *
 * Side effects:
 *
 *--------------------------------------------------------------
 */

static int
gdk_x_error (Display     *display,
             XErrorEvent *error)
{
  if (error->error_code)
    {
      if (gdk_error_warnings)
        {
          char buf[64];
          
          XGetErrorText (display, error->error_code, buf, 63);

#ifdef G_ENABLE_DEBUG     
          g_error ("%s\n  serial %ld error_code %d request_code %d minor_code %d\
n", 
                   buf, 
                   error->serial, 
                   error->error_code, 
                   error->request_code,
                   error->minor_code);
#else /* !G_ENABLE_DEBUG */
          fprintf (stderr, "Gdk-ERROR **: %s\n  serial %ld error_code %d 
request_code %d minor_code %d\n",
                   buf, 
                   error->serial, 
                   error->error_code, 
                   error->request_code,
                   error->minor_code);

          exit(1);
#endif /* G_ENABLE_DEBUG */
        }
      gdk_error_code = error->error_code;
    }
  
  return 0;
}


Therefore, it is exit(-1) inside gdk but not mozilla. I don't think we can fix 
this bug. We probably could find some way to work around it though. 
I hate crasher but this is a exit, not really a crahser. 
reassign to bstell and cc shanjian. 

Comment 12

[kill this account]

I chatted with Erik about this. 

One question is "how big is too big?" or how big should we allow?

If I was force to answer this I'm inclined to think 4x max screen size (say 
18" * 72.5 * 4 =~ 5K points).

Does this problem happen alot?

Would anyone object if I mark this as "WONTFIX" ?

Comment 13

[Samuel Sieb]

Would it be possible to just limit max point size?  There is always the
possibility of someone creating crashme pages with this.  How much work would it
be to set a limit?

Comment 14

[Frank Tang]

The question is really - what should the limit be ? I suggest we limit the point
size to 500, any objection ? 

Comment 15

[Frank Tang]

blizzard and pavlov- this is a problem causee by an exit inside GTK. what should
we do with this.
Also, see similar issue in 69204
I think that is a different bug but it exit by the same code.

Comment 16

[Håkan Waara]

I don't crash on the testcase.

I believe there was a bug where someone lowered the font-size limit or the root
problem is simply fixed. Does anyone else crash on the testcase?

If not, then please go ahead and mark this WORKSFORME.

Comment 17

[kill this account]

per Håkan suggestion marking it WORKSFORME

Comment 18

[Håkan Waara]

Verified worksforme.

Comment 19

[kill this account]

changed title: this problem is a library (GDK) exiting not a crash

Comment 20

[kill this account]

Subject: Re: Bug 69204 - Mozilla 0.8 crash (GDK error)
   Date: 19 Mar 2001 20:03:03 -0500
   From: Owen Taylor <[e-mail]>

[e-mail] (Brian Stell) writes:

> Owen,
> 
> Are you familiar with gdk.c? If not, perhaps you could 
> direct me to anyone else?
> 
> There are several bugs where Mozilla exits in gdk_x_error().
> 
> http://bugzilla.mozilla.org/show_bug.cgi?id=59355
> http://bugzilla.mozilla.org/show_bug.cgi?id=69204
> 
> In general moz tries to continue running even when there
> is a problem where something is not possible. Although the
> desired operation is not possible the user may want to
> try something else. Also, if the user was doing any editing 
> (email/html) then moz does not have a chance to save before 
> exit.
> 
> I note that in the debug path it does not actually exit.

Not true. Always exits one way or the other unless you have a error
trap pushed. The difference is on the debug path, it calls
g_error() which calls abort().
 
> Is there any way for Mozilla to not exit (and not crash)?

Well, if you do gdk_error_trap_push() at the start of your program,
then all subsequent errors are caught _and silently ignored_.

However, ignoring errors is not what this intended for. If you get an
X error is it is 99.9% an application error.


You are supposed to use this around small chunks of code that
do a potentially dangerous operation, such as getting a property
on a window you don't own.

 gdk_error_trap_push ();
 
 [ Dangerous operation ] 

 if (gdk_error_trap_pop ())
   /* clean up */

continuing past an X error usually violates assumptions that GTK+ is
making. In the normal mode of operation of X, if I make a call like
XQueryColor, it will either succeed or die trying. So, GTK+ doesn't
(as a rule) check to see if the result of calls that only fail 
ruing error cleanup. So, just ignoring errors is a bad idea:

 - It prevents you from finding out and debugging real bugs

 - It may violate assumptions that GTK+ is making.

However, a at least one of these two errors you are seeing is genuinely
something that might make sense to try to catch and clean up after.

Since gdk_font_load does check the result of XLoadQueryFont
and return NULL, it may work for you to do gdk_error_trap_push()
around your calls

See XSetErrorHandler for more information on how X errors work 
and what they are.

Regards,
                                        Owen

Comment 21

[kill this account]

Now that we have a possible solution I am reopening this.

Comment 22

[kill this account]

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

Comment 23

[Asa Dotzler [:asa]]

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

Comment 24

[kill this account]

Attachment: patch bracketing gdk_font_load() with gdk_error_trap_push/gdk_error_trap_pop

Comment 25

[kill this account]

I've put in the patch recommened by Owen Taylor.
I've spot checked that the browser appears normal after this change.

However, since I never saw the crash on my system I cannot verify this fixes 
the bug.

Can someone review/verify this?

Thanks

Comment 26

[Christopher Blizzard (:blizzard)]

r=blizzard

Comment 27

[Brendan Eich [:brendan]]

sr=[e-mail], and I'm curious: is it always a BadAlloc error that we
are suppressing with this patch, and does it have no other dire effects?

/be

Comment 28

[kill this account]

> is it always a BadAlloc error that we are suppressing with this patch, 
> and does it have no other dire effects?

I don't think so. Here is the gdk_font_load from gdkfont.c

    GdkFont*
    gdk_font_load (const gchar *font_name)
    {
      GdkFont *font;
      GdkFontPrivate *private;
      XFontStruct *xfont;

      g_return_val_if_fail (font_name != NULL, NULL);

      font = gdk_font_hash_lookup (GDK_FONT_FONT, font_name);
      if (font)
        return font;

      xfont = XLoadQueryFont (gdk_display, font_name);
      if (xfont == NULL)
        return NULL;

      font = gdk_font_lookup (xfont->fid);
      if (font != NULL)
        {
          private = (GdkFontPrivate *) font;
          if (xfont != private->xfont)
        XFreeFont (gdk_display, xfont);

          gdk_font_ref (font);
        }
      else
        {
          private = g_new (GdkFontPrivate, 1);
          private->xdisplay = gdk_display;
          private->xfont = xfont;
          private->ref_count = 1;
          private->names = NULL;

          font = (GdkFont*) private;
          font->type = GDK_FONT_FONT;
          font->ascent =  xfont->ascent;
          font->descent = xfont->descent;

          gdk_xid_table_insert (&xfont->fid, font);
        }

      gdk_font_hash_insert (GDK_FONT_FONT, font, font_name);

      return font;
    }

Comment 29

[kill this account]

checked in

Comment 30

[kill this account]

I cannot reproduce this bug (too much virtual memory!)

Can someone verify that this fixes the problem?

Comment 31

[Kamil Toman]

I can't reproduce the bug any more. Seems ok for me, now.

Comment 32

[kill this account]

based on the comment by [e-mail] I am marking verified.
Please change this if not appropriate.