Last Comment Bug 660580 - Don't use infallible allocators for storing image source data in RasterImage
: Don't use infallible allocators for storing image source data in RasterImage
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: ImageLib (show other bugs)
: unspecified
: All All
: -- normal with 4 votes (vote)
: mozilla7
Assigned To: Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
:
Mentors:
Depends on:
Blocks: 660577
  Show dependency treegraph
 
Reported: 2011-05-29 21:19 PDT by Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
Modified: 2011-06-08 20:53 PDT (History)
20 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Use a FallibleTArray (2.52 KB, patch)
2011-05-29 21:55 PDT, Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
n.nethercote: feedback+
Details | Diff | Splinter Review
Patch (5.95 KB, patch)
2011-05-30 10:39 PDT, Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
joe: review+
Details | Diff | Splinter Review

Description Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-05-29 21:19:01 PDT

    
Comment 1 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-05-29 21:55:03 PDT
Created attachment 536004 [details] [diff] [review]
Use a FallibleTArray

njn, should we consider firing a memory pressure notification where I put the comment?
Comment 2 Nicholas Nethercote [:njn] 2011-05-29 22:07:46 PDT
Comment on attachment 536004 [details] [diff] [review]
Use a FallibleTArray

Review of attachment 536004 [details] [diff] [review]:
-----------------------------------------------------------------

::: modules/libpr0n/src/imgRequest.cpp
@@ +1126,5 @@
>              sizeHint = PR_MIN(sizeHint, 20000000); /* Bound by something reasonable */
>              RasterImage* rasterImage = static_cast<RasterImage*>(mImage.get());
> +            rv = rasterImage->SetSourceSizeHint(sizeHint);
> +            if (NS_FAILED(rv)) {
> +              // XXXkhuey should we fire a low-memory notification here?

Maybe?  My understanding is that such an event will trigger a JS GC+CC and some other stuff, the details of which I don't know.  If we did that, should we then try SetSourceSizeHint again?

@@ +1128,5 @@
> +            rv = rasterImage->SetSourceSizeHint(sizeHint);
> +            if (NS_FAILED(rv)) {
> +              // XXXkhuey should we fire a low-memory notification here?
> +              NS_WARNING("Failed to preallocate memory for image, likely hitting OOM!");
> +            }

I've never looked at this code before, so I have some stupid questions:

- (Less important) Why does it potentially test |if (imageType == imgIContainer::TYPE_RASTER)| three times in quick succession?

- (More important) If the SetSourceSizeHint() call fails, what happens next?  Does the image not get loaded?  Is there a chance we'll be able to recover in a graceful way?  Maybe that depends on whether the low-memory-notification is fired earlier?  Will we already have swapped the machine half to death before we reach this point?

I guess a pertinent question is:  what would have Firefox 3.6 done in this case?
Comment 3 Nicholas Nethercote [:njn] 2011-05-29 22:10:51 PDT
Oh, another question: have you seen this make a difference in practice?
Comment 4 Bob Clary [:bc:] 2011-05-29 22:23:50 PDT
What signatures would this fix? I see *tons* of oom aborts in crash testing though not necessarily obviously related to images. I thought the oom abort was by design. Should I be filing bugs on them?
Comment 5 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-05-29 22:28:17 PDT
In an ideal world, any allocation site where the size is controlled by content should be fallible, particularly if it can used to allocate a large amount of memory.

If there are oom aborts you're hitting at high volumes, lets file bugs on them and see what we can do.

This fix will fix crashes that look like the top few frames of https://crash-stats.mozilla.com/report/index/ed79b265-e3c0-4133-a301-e5ec82110523.
Comment 6 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-05-29 22:32:53 PDT
(In reply to comment #2)
> Comment on attachment 536004 [details] [diff] [review] [review]
> Use a FallibleTArray
> 
> Review of attachment 536004 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: modules/libpr0n/src/imgRequest.cpp
> @@ +1126,5 @@
> >              sizeHint = PR_MIN(sizeHint, 20000000); /* Bound by something reasonable */
> >              RasterImage* rasterImage = static_cast<RasterImage*>(mImage.get());
> > +            rv = rasterImage->SetSourceSizeHint(sizeHint);
> > +            if (NS_FAILED(rv)) {
> > +              // XXXkhuey should we fire a low-memory notification here?
> 
> Maybe?  My understanding is that such an event will trigger a JS GC+CC and
> some other stuff, the details of which I don't know.  If we did that, should
> we then try SetSourceSizeHint again?

Maybe?  I don't know if we could.  nsMemoryImpl.cpp looks like it runs the memory flushing off a runnable, so we won't get the chance.

> 
> @@ +1128,5 @@
> > +            rv = rasterImage->SetSourceSizeHint(sizeHint);
> > +            if (NS_FAILED(rv)) {
> > +              // XXXkhuey should we fire a low-memory notification here?
> > +              NS_WARNING("Failed to preallocate memory for image, likely hitting OOM!");
> > +            }
> 
> I've never looked at this code before, so I have some stupid questions:
> 
> - (Less important) Why does it potentially test |if (imageType ==
> imgIContainer::TYPE_RASTER)| three times in quick succession?

No idea.

> - (More important) If the SetSourceSizeHint() call fails, what happens next?
> Does the image not get loaded?  Is there a chance we'll be able to recover
> in a graceful way?  Maybe that depends on whether the
> low-memory-notification is fired earlier?  Will we already have swapped the
> machine half to death before we reach this point?

SetSourceSizeHint preallocates an array to receive the image data.  If it fails, that array gets constructed N KB at a time (don't know what N is off hand) so there are more allocation calls involved.  If SetSourceSizeHint fails and then when we try to construct that array we're still out of memory we'll abort loading the image with NS_ERROR_OUT_OF_MEMORY. (http://hg.mozilla.org/mozilla-central/annotate/8d752162e810/modules/libpr0n/src/RasterImage.cpp#l1256).  We then promptly fail to propagate the exception.

> I guess a pertinent question is:  what would have Firefox 3.6 done in this
> case?

I'm not sure ... this code was basically rewritten for 4.
Comment 7 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-05-30 10:39:19 PDT
Created attachment 536123 [details] [diff] [review]
Patch

This propagates the error upwards a bit.  I wish there were a way to test this stuff :-/.
Comment 8 Daniel Holbert [:dholbert] 2011-05-31 10:52:00 PDT
(In reply to comment #6)
> > - (Less important) Why does it potentially test |if (imageType ==
> > imgIContainer::TYPE_RASTER)| three times in quick succession?
> 
> No idea.

The first two of those three checks could definitely be collapsed together. (They're currently separate because they were added in different bugs & do logically different things -- the first is a "if (raster) { trigger some raster-specific hints }", whereas the second is a "Tell the image that it's now got some data, whether it's raster or vector.")

The third check can't be collapsed. It's up one is up one level of indentation from the other two, which means it's triggered on every call to OnDataAvailable.  (whereas the first two are only triggered on the first call to OnDataAvailable)
Comment 9 Joe Drew (not getting mail) 2011-06-08 11:54:29 PDT
Comment on attachment 536123 [details] [diff] [review]
Patch

Review of attachment 536123 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good. Note, however, that the biggest allocations in imagelib should already be fallible as of bug 578357.

::: modules/libpr0n/src/RasterImage.cpp
@@ +2767,5 @@
>    RasterImage* image = static_cast<RasterImage*>(aClosure);
>  
>    // Copy the source data. We squelch the return value here, because returning
>    // an error means that ReadSegments stops reading data, violating our
> +  // invariant that we read everything we get, unless we hit OOM.

Reword the second sentence here: "Unless we hit OOM, we squelch..."

::: modules/libpr0n/src/imgRequest.cpp
@@ +1126,5 @@
>              sizeHint = PR_MIN(sizeHint, 20000000); /* Bound by something reasonable */
>              RasterImage* rasterImage = static_cast<RasterImage*>(mImage.get());
> +            rv = rasterImage->SetSourceSizeHint(sizeHint);
> +            if (NS_FAILED(rv)) {
> +              // XXXkhuey should we fire a low-memory notification here?

Only if the return value is NS_ERROR_OUT_OF_MEMORY ;)

@@ +1149,4 @@
>    }
>  
>    if (imageType == imgIContainer::TYPE_RASTER) {
>      // WriteToRasterImage always consumes everything it gets

"... if it doesn't run out of memory."

::: modules/libpr0n/src/imgTools.cpp
@@ +112,3 @@
>  
>    // Send the source data to the Image. WriteToRasterImage always
>    // consumes everything it gets.

"... if it doesn't run out of memory."
Comment 10 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-06-08 20:53:05 PDT
http://hg.mozilla.org/mozilla-central/rev/0417dfeeb440

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