Closed Bug 1155332 Opened 9 years ago Closed 9 years ago

If we don't have enough memory to fully decode an image, discard what we've decoded so far immediately

Categories

(Core :: Graphics: ImageLib, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox38 + wontfix
firefox39 + wontfix
firefox40 --- fixed

People

(Reporter: seth, Assigned: seth)

References

Details

Attachments

(1 file)

See bug 1150089 for more discussion. Especially with animated images, by the time we realize we've run out of memory, we have already  almost completely filled up the SurfaceCache, which makes Firefox behave very badly because no images can load. This state continues until the image expires out of the cache, which takes quite a while. We should discard the image's data immediately in this situation.
As discussed on IRC, we would like to see this bug fixed for 38.
Depends on: 1155429
No longer depends on: 1155429
Seth, any news on this? Thanks
Flags: needinfo?(seth)
(In reply to Sylvestre Ledru [:sylvestre] from comment #5)
> Seth, any news on this? Thanks

Working on it.
Flags: needinfo?(seth)
I'm about to post a patch for this, but I think it's significantly higher risk than the patch in bug 1161859, and the patch in bug 1161859 _directly_ fixes the problem reported in bug 1150089. I think we should track bug 1161859 for 38, and keep the patch in this bug 39+. Sound OK?
Flags: needinfo?(sledru)
Here's the patch. It consists of two pieces:

- In Decoder, we detect when SurfaceCache::Insert fails due to low memory, and
  we treat that as an actual error. (Before, we just marked the decoder as
  aborted, so DoError() didn't get called.) This makes this kind of failure
  "sticky", but I've come around to the view that that's actually desireable,
  since it helps the system recover.

- In RasterImage::DoError(), we tear down everything the image owns immediately.

This way, even if you do OOM, nothing bad happens to your browser session -
recovery is immediate and total.
Attachment #8601864 - Flags: review?(tnikkel)
(Well, even if you do *enter a low memory situation with respect to the SurfaceCache*. A true OOM, of course, is unrecoverable.)
Blocks: 1116359
Attachment #8601864 - Flags: review?(tnikkel) → review+
Thanks for the quick review!

Unfortunately, it looks like this broke /2dcontext/drawing-images-to-the-canvas/2d.drawImage.broken.html.

I'll investigate tomorrow and get this landed.
(In reply to Seth Fowler [:seth] from comment #7)
> I think we should track bug 1161859 for 38, and keep the patch in this bug 39+. Sound OK?
Well, we built 38 RC1. So, it is a bit late. We might take it 38.0.5 or 38.1.0 esr.
For 39, we can probably take this one.
Flags: needinfo?(sledru)
(In reply to Sylvestre Ledru [:sylvestre] from comment #12)
> (In reply to Seth Fowler [:seth] from comment #7)
> > I think we should track bug 1161859 for 38, and keep the patch in this bug 39+. Sound OK?
> Well, we built 38 RC1. So, it is a bit late. We might take it 38.0.5 or
> 38.1.0 esr.
> For 39, we can probably take this one.

Roger. I've requested uplift to 38.1 in the other bug.
Depends on: 1162282
^ Pushed as the try job in comment 14 indicates that bug 1162282 fixed the test failure.
https://hg.mozilla.org/mozilla-central/rev/914258721d1f
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Depends on: 1165009
Seth, if you think it is still relevant, could you fill an uplift request to 39? Thanks
Flags: needinfo?(seth)
(In reply to Sylvestre Ledru [:sylvestre] from comment #18)
> Seth, if you think it is still relevant, could you fill an uplift request to
> 39? Thanks

I don't feel comfortable uplifting this. It turned out to be a riskier patch that I originally hoped it would be, and it's already resulted in a serious regression (bug 1165009). Let's let this ride the trains as normal. Please untrack for 39. Bug 1161859 is already in 39 anyway, and that fixes the primary issue that this was intended to address.
Flags: needinfo?(seth)
Depends on: 1234023
Depends on: 1237709
No longer depends on: 1234023
You need to log in before you can comment on or make changes to this bug.