njn, should we consider firing a memory pressure notification where I put the comment?

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?

Oh, another question: have you seen this make a difference in practice?

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?

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.

(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.

This propagates the error upwards a bit. I wish there were a way to test this stuff :-/.

(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 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."