351943 - Browser crashes when trying to allocate large image

Status: VERIFIED FIXED

(Core Graveyard :: GFX: OS/2, defect)

Description

[Peter Weilbacher]

SeaMonkey and Firefox on OS/2 crash when trying to allocate the memory for really large images. A testcase is to use the CVS Graph link for client.mk which gives an image of more than 17k x 17k size:
http://bonsai.mozilla.org/cvsgraph.cgi?file=mozilla/client.mk&rev=1.293

While most of the time it takes the debugger with it, I managed to find out that the bad allocation happens in nsImageOS2::Init(). It seems to be caused by the the line
    mImageBits = new PRUint8 [ aHeight * mRowBytes ];
(http://lxr.mozilla.org/seamonkey/source/gfx/src/os2/nsImageOS2.cpp#143)
When the debugger does not crash it puts me into a middle of some disassembly that doesn't give anything reasonable for the call stack of the last few calls  (even when using libc061.dbg).

There is another line near the end of the method
    mAlphaBits = new PRUint8 [ aHeight * mARowBytes];
(http://lxr.mozilla.org/seamonkey/source/gfx/src/os2/nsImageOS2.cpp#188)
that might be similarly problematic.

Now I am wondering what a good fix might be. Other platforms can use
   if (!mImageBits)
     return NS_ERROR_OUT_OF_MEMORY;
but that doesn't work on OS/2 for some reason, the "new"-allocation never returns. I am not sure if this is actually a problem in the C-Library (this was a GCC 3.3.5 build using the libc061.dll) or with the code in Mozilla...

Another version that I tried is
   try {
     mImageBits = new PRUint8 [ aHeight * mRowBytes ];
   } catch (...) {
     return NS_ERROR_OUT_OF_MEMORY;
   }
but then the compiler says:
   m:/mozilla/gfx/src/os2/nsImageOS2.cpp:148: error: exception handling
      disabled, use -fexceptions to enable
But when I add -fexceptions I get the same message?!

If none of this works I guess we could make some hack to test the size of the memory available to the process before the "new" line.

Comment 1

[knut st. osmundsen]

new will throw an bad_alloc exception if malloc() fails, it won't ever return NULL. You can actually step the new call, it has sufficient symbols for you to see what's going on.

The reason why try {} catch (...) {} doesn't work, could be related to this problem: http://svn.netlabs.org/libc/ticket/100

Other than try..catch there are two other ways of make new not throw. You can set a handler which will be called when malloc fails. Or you can use new(std::nothrow), which means it will return NULL if malloc fails. I suggest you try out the latter option as the former is a global thing:
 
#include <new>
...
mImageBits = new(std::nothrow) PRUint8 [ aHeight * mRowBytes ];
if (!mImageBits) 
    return NS_ERROR_OUT_OF_MEMORY;

Comment 2

[Peter Weilbacher]

So, it's a C++ standard that new will throw that exception if malloc fails? I wonder where else in the sources new is handled wrongly then. I guess VACPP did it differently.
try {} catch (...) {} may work, but I couldn't test because it doesn't _compile_.

Comment 3

[Peter Weilbacher]

Attachment: use new(std::nothrow)

OK, this indeed seems to work to prevent the crash. For large images, the "cannot be displayed, because it contains errors" message gets displayed. I think this is good enough.
(The patch also fixes the two build warnings by using casts.)

Comment 4

[timeless]

Comment on attachment 237504 [details] [diff] [review]
use new(std::nothrow)

please don't do this.

there are zillions of places where new use new. either fix them all with a wrappers (as every other platform needs). or make the code not use new.

spot fixes of new like this do not solve anything.

note that vc6 doesn't throw which is why mozilla on windows used to be fine.

on vc7(.1) if you used <new.h> instead of <new> you got the old non throwing behavior.

for vc8 you always get the new behavior unless you're very careful with your decorations (and we aren't, so we do crash)

Comment 5

[Peter Weilbacher]

Well, it certainly fixes the one reprodicible crash that I have seen in many months. And that there are not many other crashes like this since the change to GCC (2 years ago) seems to indicate that this isn't a big problem. Better spot fixes as none. :-)

But if you point me to the place where other platforms make this change globally I would of course prefer to copy that for OS/2, too.

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

personally I'd be in favour of requiring a compiler that has a working std::nothrow and use it everywhere :)

Comment 7

[Mike Kaply [:mkaply]]

Comment on attachment 237504 [details] [diff] [review]
use new(std::nothrow)

r=mkaply

Comment 8

[Peter Weilbacher]

I found this in the GCC docs:

  -fcheck-new
  Check that the pointer returned by operator new is non-null before
  attempting to modify the storage allocated. This check is normally
  unnecessary because the C++ standard specifies that operator new will
  only return 0 if it is declared throw(), in which case the compiler
  will always check the return value even without this option. In all
  other cases, when operator new has a non-empty exception specification,
  memory exhaustion is signalled by throwing std::bad_alloc. See also
  new (nothrow).

Before I try this compiler flag: does this work with OS/2's GCC 3.3.5 (and possibly also 3.2.2) and do I understand correctly that it should solve the problem? Any drawbacks that I should know about? Possible performance impact?

Comment 9

[timeless]

it's necessary because of the way mozilla works. i'm pretty sure bezilla (mozilla on beos) was using it years ago.

Comment 10

[Felix Miata]

How large is large? Is there any reason to believe this problem does not also exist in 1.8 branch nightlies?

My last crash minutes ago happened as a result of attempting to open this in a new tab: http://mrmazda.no-ip.com/SS/desktop-a-865-1792x1344x144.png

My uptime on SM 2006091809 1.1b was 45.5 hours of heavy CZ, Mailnews and browser usage. RAM recovered on crash was 285M. Just before crash I discovered a recurring image painting problem was back - images that won't paint fully or at all unless the browser window is less than the image size - that occurs only after considerable uptime and usage. IIRC, this is the same reproduction pattern as my last crash, and they produce no popuplog.os2 entries.

Comment 11

[Peter Weilbacher]

mrmazda: large is probably >512 MB or so (the OS/2 per process limit), but could be a lot less if the browser already uses lots of RAM before trying to allocate for the image... The bug definitely exists in trunk and both branches.

Bug resolution depends on what will be decided from the discussion in bug 353144 (and/or possibly bug 290284).

Comment 12

[Peter Weilbacher]

Hmm, bug 353144 was resolved and the follow-up discussion in the newsgroups didn't go anywhere. Any global fix will be too late for branch and on trunk gfx/src/os2/nsImageOS2.cpp is irrelevant now.
I guess I will just check the available patch (attachment 237504 [details] [diff] [review]) in and so fix the most pressing RAM allocation problem on OS/2.

Comment 13

[Peter Weilbacher]

Checked in to branch only (where it is relevant).

Comment 14

[juan becerra [:juanb]]

Peter, could you help verifying this bug fix on the 1.8.1.5 branch?

Comment 15

[Peter Weilbacher]

Verified with Firefox 2.0.0.5pre of 20070712 which displays 
   The image "file:///<PATH>/full_jpg.jpg" cannot be displayed,
   because it contains errors.
while Firefox 2.0.0.5pre of 20070608 crashes.