453836 - Fennec does not handle OOM while images decoding.

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Oleg Romashin (:romaxa)]

Open URL and wait.

EXPECTED OUTCOME:
Low memory notification dialog shown

ACTUAL OUTCOME:
Fennec killed by OOM killer.

Comment 1

[Oleg Romashin (:romaxa)]

I think we should insert low mem checking at some place.... I'm not sure which places is better:

1) nsThebesImage - ImageUpdated/or create new function.
2) Insert mem check directly in image decoders ProcessData-jpeg/ReadDataOut-png....

Also we should try to free some memory allocated by image surface, otherwise we we almost dead.

Comment 2

[Doug Turner (:dougt)]

right, ImageUpdated might be the right place.  I have tried putting OOM checks both after the allocation of the gfxSurface and after OptimizeImage.  Neither check is hit before you crash.

I added a OOM check in ImageUpdated(), and we do hit it.  However, i am unsure how to cancel the image load in this method.  stuart, any ideas?

Comment 3

[Oleg Romashin (:romaxa)]

> how to cancel the image load in this method.
I think we should add return value to this method (instead void), check it on image/decoders side and interrupt decoding.

Comment 4

[Doug Turner (:dougt)]

Attachment: patch v.1

changes ImageUpdated() to return a nsresult.
decoders pass any failure code up and out to cancel the image load.

Comment 5

[Oleg Romashin (:romaxa)]

Works good for me.

Comment 6

[Joe Drew (not getting mail)]

Comment on attachment 337562 [details] [diff] [review]
patch v.1


>--- a/modules/libpr0n/decoders/bmp/nsICODecoder.cpp
>+++ b/modules/libpr0n/decoders/bmp/nsICODecoder.cpp
>@@ -110,9 +110,10 @@ NS_IMETHODIMP nsICODecoder::Close()
>   // This should be a mFrame function, so that we don't have to query for interface...
>   nsIntRect r(0, 0, mDirEntry.mWidth, mDirEntry.mHeight);
>   nsCOMPtr<nsIImage> img(do_GetInterface(mFrame));
>-  if (img)
>-    img->ImageUpdated(nsnull, nsImageUpdateFlags_kBitsChanged, &r);
>-
>+  nsresult rv = NS_ERROR_FAILURE;

This should be NS_OK, since the code used to return NS_OK if there was no img.

>+  if (img) 
>+    rv = img->ImageUpdated(nsnull, nsImageUpdateFlags_kBitsChanged, &r);
>+    
>-void
>+nsresult
> nsGIFDecoder2::FlushImageData()
> {
>+  nsresult rv = NS_OK;
>+;

Stray semicolon here.

>   switch (mCurrentPass - mLastFlushedPass) {
>     case 0:  // same pass
>       if (mCurrentRow - mLastFlushedRow)
>-        FlushImageData(mLastFlushedRow + 1, mCurrentRow - mLastFlushedRow);
>+        rv = FlushImageData(mLastFlushedRow + 1, mCurrentRow - mLastFlushedRow);
>       break;
>   
>     case 1:  // one pass on - need to handle bottom & top rects
>-      FlushImageData(0, mCurrentRow + 1);
>-      FlushImageData(mLastFlushedRow + 1, mGIFStruct.height - (mLastFlushedRow + 1));
>+      rv = FlushImageData(0, mCurrentRow + 1);
>+      rv |= FlushImageData(mLastFlushedRow + 1, mGIFStruct.height - (mLastFlushedRow + 1));

Is bitwise OR the correct thing here?

> //******************************************************************************
>@@ -370,7 +377,7 @@ void nsGIFDecoder2::EndImageFrame()
>   // First flush all pending image data 
>   if (!mGIFStruct.images_decoded) {
>     // Only need to flush first frame
>-    FlushImageData();
>+    (void) FlushImageData();

Ick. Do you feel strongly about casting away the return value?

>+nsresult
>+nsJPEGDecoder::OutputScanlines(PRBool* suspend)
> {
>+  *suspend = PR_FALSE;
>+
>   const PRUint32 top = mInfo.output_scanline;
>-  PRBool rv = PR_TRUE;
>+  nsresult rv = NS_OK;;

Stray semicolon.

>diff --git a/modules/libpr0n/decoders/png/nsPNGDecoder.cpp b/modules/libpr0n/decoders/png/nsPNGDecoder.cpp
>--- a/modules/libpr0n/decoders/png/nsPNGDecoder.cpp
>+++ b/modules/libpr0n/decoders/png/nsPNGDecoder.cpp
>@@ -189,7 +189,11 @@ void nsPNGDecoder::EndImageFrame()
> 
>     nsIntRect r(0, 0, width, height);
>     nsCOMPtr<nsIImage> img(do_GetInterface(mFrame));
>-    img->ImageUpdated(nsnull, nsImageUpdateFlags_kBitsChanged, &r);
>+    nsresult rv = img->ImageUpdated(nsnull, nsImageUpdateFlags_kBitsChanged, &r);
>+    if (NS_FAILED(rv)) {
>+      mError = PR_TRUE;
>+      // allow the call out to the observers.
>+    }

You don't really need the nsresult rv there -- you could just do NS_FAILED(img->imageUpdated...). Similarly below.

>@@ -816,7 +820,11 @@ row_callback(png_structp png_ptr, png_by
>       // Only do incremental image display for the first frame
>       nsIntRect r(0, row_num, width, 1);
>       nsCOMPtr<nsIImage> img(do_GetInterface(decoder->mFrame));
>-      img->ImageUpdated(nsnull, nsImageUpdateFlags_kBitsChanged, &r);
>+      nsresult rv = img->ImageUpdated(nsnull, nsImageUpdateFlags_kBitsChanged, &r);
>+      if (NS_FAILED(rv)) {
>+        decoder->mError = PR_TRUE;  /* bail */
>+        return;
>+      }

Comment 7

[Doug Turner (:dougt)]

Thanks for the feedback:

the bitwise OR is so that I do not have to have an if/return block. if FlushImageData(...) ever return two different error codes, FlushImageData() would return the wrong thing.  However, in this case it would still be an error code.  If this doesn't bother you, it doesn't bother me.  fwiw, this pattern is used a bit :

http://mxr.mozilla.org/mozilla-central/search?string=rv+|=


casting away the return value, is an indication to me (and maybe others?) that i really don't care about the return code in this case.


I fixed the other stuff.  let me know what you want me to do about the above; i don't care much either way.

Comment 8

[Joe Drew (not getting mail)]

The other stuff is fine. The rv thing I was honestly wondering about; the return value cast I don't feel terribly strongly about.

Comment 9

[Doug Turner (:dougt)]

does that me r=joeydrew!

Comment 10

[Joe Drew (not getting mail)]

check in, young man!

Comment 11

[Doug Turner (:dougt)]

78d19b676e11

Thanks joe!