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)
Core
Graphics: ImageLib
Tracking
()
RESOLVED
FIXED
mozilla40
People
(Reporter: seth, Assigned: seth)
References
Details
Attachments
(1 file)
2.84 KB,
patch
|
tnikkel
:
review+
|
Details | Diff | Splinter Review |
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.
Comment 1•9 years ago
|
||
As discussed on IRC, we would like to see this bug fixed for 38.
status-firefox38:
--- → affected
status-firefox39:
--- → affected
status-firefox40:
--- → affected
tracking-firefox38:
--- → +
Assignee | ||
Comment 6•9 years ago
|
||
(In reply to Sylvestre Ledru [:sylvestre] from comment #5) > Seth, any news on this? Thanks Working on it.
Flags: needinfo?(seth)
Assignee | ||
Comment 7•9 years ago
|
||
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)
Assignee | ||
Comment 8•9 years ago
|
||
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)
Assignee | ||
Comment 9•9 years ago
|
||
(Well, even if you do *enter a low memory situation with respect to the SurfaceCache*. A true OOM, of course, is unrecoverable.)
Assignee | ||
Comment 10•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f9551532128
Updated•9 years ago
|
Attachment #8601864 -
Flags: review?(tnikkel) → review+
Assignee | ||
Comment 11•9 years ago
|
||
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.
Comment 12•9 years ago
|
||
(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)
Assignee | ||
Comment 13•9 years ago
|
||
(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.
Assignee | ||
Comment 14•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f399b4d1dc1
Assignee | ||
Comment 15•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/914258721d1f
Assignee | ||
Comment 16•9 years ago
|
||
^ 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
Comment 18•9 years ago
|
||
Seth, if you think it is still relevant, could you fill an uplift request to 39? Thanks
Assignee | ||
Comment 19•9 years ago
|
||
(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)
Comment 20•9 years ago
|
||
ok, thanks!
You need to log in
before you can comment on or make changes to this bug.
Description
•