Bug 58818

Summary: [GTK] fast/block/float/overhanging-tall-block.html crashes in the bots
Product: WebKit Reporter: Alejandro G. Castro <alex>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, eric, mitz, mrobinson, scaroo, simon.fraser, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Attachments:
Description Flags
Patch
none
Patch fixing typo in the ChangeLog none

Description Alejandro G. Castro 2011-04-18 13:25:16 PDT
This is the error we get in the logs:

The program 'DumpRenderTree' received an X Window System error.
This probably reflects a bug in the program.
The error was 'BadAlloc (insufficient resources for operation)'.
  (Details: serial 102741 error_code 11 request_code 53 minor_code 0)
  (Note to programmers: normally, X errors are reported asynchronously;
   that is, you will receive the error a while after causing it.
   To debug your program, run it with the --sync command line
   option to change this behavior. You can then get a meaningful
   backtrace from your debugger if you break on the gdk_x_error() function.)
LEAK: 2494 WebCoreNode
LEAK: 1 Range
LEAK: 73 CachedResource
LEAK: 9 Frame
LEAK: 9 Page
LEAK: 1160 RenderObject

Simon, any clue about what could cause the problem? I'm going to skip the test for the moment.
Comment 1 Alejandro G. Castro 2011-04-18 13:29:52 PDT
Skipped http://trac.webkit.org/changeset/84175
Comment 2 Simon Fraser (smfr) 2011-04-18 13:47:08 PDT
Why would this just crash GTK? Got a crash log?
Comment 3 Alejandro G. Castro 2011-04-19 01:32:30 PDT
(In reply to comment #2)
> Why would this just crash GTK? Got a crash log?

No idea, it is really weird, amazari is checking the problem, unfortunately for some reason the crash does not produce the crash log, and it can not be reproduced locally. We are going to try to run the test in the bot manually to see what we get.
Comment 4 Martin Robinson 2011-04-19 11:09:05 PDT
Created attachment 90219 [details]
Patch
Comment 5 Alejandro G. Castro 2011-04-20 05:34:02 PDT
Comment on attachment 90219 [details]
Patch


Looks goods! Just a couple of comments.

View in context: https://bugs.webkit.org/attachment.cgi?id=90219&action=review

> Source/WebCore/ChangeLog:8
> +        Prevent allocating scratch buffers larger than the target GdkDarawable

GdkDrawable

> Source/WebCore/platform/gtk/WidgetRenderingContext.cpp:93
> +    // We never want to create a scratch buffer larger than the size of our target GdkDrawable.
> +    // This prevents giant pixmap allocations for very large widgets in smaller views.
> +    int maxWidth = 0, maxHeight = 0;
> +    gdk_drawable_get_size(graphicsContext->gdkWindow(), &maxWidth, &maxHeight);
> +    m_targetRect.setSize(m_targetRect.size().shrunkTo(IntSize(maxWidth, maxHeight)));
> +
>      // Widgets sometimes need to draw outside their boundaries for things such as
>      // exterior focus. We want to allocate a some extra pixels in our surface for this.
> -    m_extraSpace = IntSize(15, 15);
> +    static int extraSpace = 15;
> +    m_targetRect.inflate(extraSpace);

Should we inflate first and later shrunkTo?
Comment 6 Martin Robinson 2011-04-20 12:13:44 PDT
(In reply to comment #5)
> (From update of attachment 90219 [details])
> 
> Looks goods! Just a couple of comments.
> 
> View in context: https://bugs.webkit.org/attachment.cgi?id=90219&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        Prevent allocating scratch buffers larger than the target GdkDarawable
> 
> GdkDrawable

Fixed!

> Should we inflate first and later shrunkTo?

inflate() modifies the offset of the target rectangle. If we inflate first and then shrink, there would have to be more code which detected if the offset needs to be corrected after shrunkTo. It seemed simpler to inflate later, since there isn't much harm in having a buffer that is 30 pixels bigger in each direction. We're really just trying to prevent having buffer that is much bigger than the target.
Comment 7 Martin Robinson 2011-04-20 12:22:43 PDT
Created attachment 90385 [details]
Patch fixing typo in the ChangeLog
Comment 8 Eric Seidel (no email) 2011-04-26 16:16:38 PDT
Comment on attachment 90385 [details]
Patch fixing typo in the ChangeLog

Seems reasonable to me.  if you need a more gtk-ish review, you'll have to ask someone else though. :)
Comment 9 Martin Robinson 2011-04-26 16:32:12 PDT
Committed r84978: <http://trac.webkit.org/changeset/84978>
Comment 10 WebKit Review Bot 2011-04-26 17:23:47 PDT
http://trac.webkit.org/changeset/84978 might have broken SnowLeopard Intel Release (Tests)
The following tests are not passing:
accessibility/anchor-linked-anonymous-block-crash.html
accessibility/aria-activedescendant-crash.html
accessibility/aria-checkbox-checked.html
accessibility/aria-checkbox-text.html
accessibility/aria-combobox.html
accessibility/aria-controls-with-tabs.html
accessibility/aria-describedby-on-input.html
accessibility/aria-disabled.html
accessibility/aria-help.html
accessibility/aria-hidden-update.html
accessibility/aria-hidden-with-elements.html
accessibility/aria-hidden.html
accessibility/aria-label.html
accessibility/aria-labelledby-on-input.html
accessibility/aria-labelledby-overrides-label.html
accessibility/aria-labelledby-stay-within.html
accessibility/aria-link-supports-press.html
accessibility/aria-list-and-listitem.html
accessibility/aria-option-role.html
accessibility/aria-presentational-role.html