Closed Bug 1171371 Opened 5 years ago Closed 4 years ago

On memory-pressure, remove any stale images from the visible images list

Categories

(Core :: Layout: Images, Video, and HTML Frames, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: seth, Assigned: seth)

References

Details

Attachments

(1 file, 2 obsolete files)

On low memory B2G devices, memory-pressure is not an uncommon situation. We should remove stale images from the visible images list when we enter memory-pressure, because that will allow us to discard those images and hopefully ease the memory pressure.

(When I say "stale images" here, I mean images which are in the visible images list, but which are no longer visible.)
Here's the patch, implementing along the lines of our discussion on IRC.
Attachment #8615195 - Flags: review?(tnikkel)
Attachment #8615195 - Flags: review?(tnikkel) → review+
I think you need to bump the NS_IPRESSHELL_IID.
This version of the attach bumps NS_IPRESSHELL_IID.
Attachment #8615195 - Attachment is obsolete: true
Thanks for catching that Mats.
OK, we just needed an additional null check for mPresContext.

I have to say I was a little surprised at this, as it looks like other code
assumes that mPresContext is non-null, but I confirmed with dbaron that it *can*
be non-null in some situations - for example, during page teardown.
Attachment #8615455 - Attachment is obsolete: true
Comment on attachment 8615728 [details] [diff] [review]
On memory-pressure, remove any stale images from the visible images list

[Approval Request Comment]
Bug caused by (feature/regressing bug #): This patch helps us recover from memory pressure with image locking enabled. We enabled image locking on B2G in bug 1148696.
User impact if declined: Images may fail to decode due to low memory, causing blank thumbnails in the gallery which may persist across reboots (making this effectively a data loss issue). This patch is one of several that together fix bug 1166136, which is nominated for blocking 2.2, and bug 1164164, which is a 2.2 blocker. Additionally, this patch should help us recover from memory pressure more easily, no matter what the cause is.
Testing completed: Extensively tested locally. On mozilla-inbound and anticipated to land on mozilla-central as of now.
Risk to taking this patch (and alternatives if risky): This is a pretty low risk patch.
String or UUID changes made by this patch: None.
Attachment #8615728 - Flags: approval-mozilla-b2g37?
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #7)
> Backed out for Android testAboutPage crashes.

What he said. https://treeherder.mozilla.org/logviewer.html#?job_id=10453971&repo=mozilla-inbound, backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/ca87dfad14cf
Comment on attachment 8615728 [details] [diff] [review]
On memory-pressure, remove any stale images from the visible images list

Hmm, frustrating. Not really sure what's going on here, and unfortunately we're out of time to debug it.
Attachment #8615728 - Flags: approval-mozilla-b2g37?
See Also: → 1172427
I rebased and pushed to try
https://treeherder.mozilla.org/#/jobs?repo=try&revision=809473283162

Looks to be green now.

Seth, any reason I shouldn't land this now?
Flags: needinfo?(seth)
Flags: needinfo?(seth)
https://hg.mozilla.org/mozilla-central/rev/9bf8c3d6ec19
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Product: Core → Core Graveyard
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.