Closed Bug 453836 Opened 13 years ago Closed 13 years ago

Fennec does not handle OOM while images decoding.

Categories

(Core :: ImageLib, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: romaxa, Assigned: dougt)

References

()

Details

(Keywords: mobile)

Attachments

(1 file)

Open URL and wait.

EXPECTED OUTCOME:
Low memory notification dialog shown

ACTUAL OUTCOME:
Fennec killed by OOM killer.
Assignee: nobody → doug.turner
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.
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?
> 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.
Attached patch patch v.1Splinter Review
changes ImageUpdated() to return a nsresult.
decoders pass any failure code up and out to cancel the image load.
Works good for me.
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;
>+      }
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.
The other stuff is fine. The rv thing I was honestly wondering about; the return value cast I don't feel terribly strongly about.
does that me r=joeydrew!
check in, young man!
78d19b676e11

Thanks joe!
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Component: General → ImageLib
Product: Fennec → Core
QA Contact: general → imagelib
Keywords: mobile
Depends on: 490163
You need to log in before you can comment on or make changes to this bug.