Last Comment Bug 351943 - Browser crashes when trying to allocate large image
: Browser crashes when trying to allocate large image
: crash, verified1.8.1.5
Product: Core Graveyard
Classification: Graveyard
Component: GFX: OS/2 (show other bugs)
: 1.8 Branch
: x86 OS/2
-- major (vote)
: ---
Assigned To: Peter Weilbacher
Depends on: 290284 353144
  Show dependency treegraph
Reported: 2006-09-09 08:08 PDT by Peter Weilbacher
Modified: 2009-01-22 15:52 PST (History)
5 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---

use new(std::nothrow) (3.51 KB, patch)
2006-09-09 10:21 PDT, Peter Weilbacher
mozilla: review+
Details | Diff | Splinter Review

Description User image Peter Weilbacher 2006-09-09 08:08:59 PDT
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 which gives an image of more than 17k x 17k size:

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 ];
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];
that might be similarly problematic.

Now I am wondering what a good fix might be. Other platforms can use
   if (!mImageBits)
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 (...) {
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 User image knut st. osmundsen 2006-09-09 08:52:53 PDT
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:

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) 
Comment 2 User image Peter Weilbacher 2006-09-09 10:06:46 PDT
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 User image Peter Weilbacher 2006-09-09 10:21:43 PDT
Created attachment 237504 [details] [diff] [review]
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 User image timeless 2006-09-09 15:13:53 PDT
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 User image Peter Weilbacher 2006-09-09 15:38:14 PDT
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 User image Christian :Biesinger (don't email me, ping me on IRC) 2006-09-09 15:51:07 PDT
personally I'd be in favour of requiring a compiler that has a working std::nothrow and use it everywhere :)
Comment 7 User image Mike Kaply [:mkaply] 2006-09-09 18:34:36 PDT
Comment on attachment 237504 [details] [diff] [review]
use new(std::nothrow)

Comment 8 User image Peter Weilbacher 2006-09-13 01:41:54 PDT
I found this in the GCC docs:

  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 User image timeless 2006-09-13 05:47:28 PDT
it's necessary because of the way mozilla works. i'm pretty sure bezilla (mozilla on beos) was using it years ago.
Comment 10 User image Felix Miata 2006-09-20 09:24:21 PDT
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:

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 User image Peter Weilbacher 2006-09-27 03:28:03 PDT
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 User image Peter Weilbacher 2007-06-01 07:15:39 PDT
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 User image Peter Weilbacher 2007-06-10 14:27:45 PDT
Checked in to branch only (where it is relevant).
Comment 14 User image juan becerra [:juanb] 2007-07-16 11:03:27 PDT
Peter, could you help verifying this bug fix on the branch?
Comment 15 User image Peter Weilbacher 2007-07-20 03:33:37 PDT
Verified with Firefox of 20070712 which displays 
   The image "file:///<PATH>/full_jpg.jpg" cannot be displayed,
   because it contains errors.
while Firefox of 20070608 crashes.

Note You need to log in before you can comment on or make changes to this bug.