I've found multiple ways we could fix bug 1122704; so far the focus has been on changes we could make in ImageLib. After more investigation, though, I think the issue in bug 1122704 goes deeper. An (edited) IRC log: [18:10:28] seth: tn: not sure if you saw the discussion in #memshrink, but i put together a quick hack that tests bug 1123976, and i think it's going to be a win [18:12:50] seth: tn: i'm a bit puzzled as to *why* we do better, though, because it looks like we call RequestDiscard on a document's images when the document gets destroyed ... [18:44:13] @tn: seth, are the required RequestDiscard calls even being made? [18:44:24] seth: tn: locally, i see them being made when i close a tab, yeah [18:44:30] seth: tn: that's what is so puzzling .... [18:59:52] seth: tn: actually on techcrunch i can reproduce this even locally ... [20:40:09] seth: tn: alright, i have come to a conclusion about this [20:40:27] seth: tn: the problem is in PresShell::Destroy() [20:41:05] seth: tn: we call ClearVisibleImagesList() *before* we call mDocument->DeleteShell() [20:42:00] seth: tn: ClearVisibleImagesList() calls nsImageLoadingContent::DecrementVisibleCount(), which will generally end up calling nsImageLoadingContent::UntrackImage(), which calls nsDocument::RemoveImage() [20:42:46] seth: tn: that is bad news, because now the image is not in nsDocument::mImageTracker, which means that when we call RequestDiscard() on all of the images in nsDocument::DeleteShell(), we do *not* call RequestDiscard() on most images [20:44:11] seth: tn: the most straightforward fix i see (which is still kinda more verbose than i'd prefer, but such is life), is to route a REQUEST_DISCARD flag from ClearVisibleImagesList() all the way to nsDocument::RemoveImage() ... [21:14:24] seth: tn: in my testing this modification results in all of the banner images and such on techcrunch getting immediately discarded when the tab is closed [21:14:28] seth: tn: before, none of them were I'm still waiting on try and AWSY, so I'm not yet sure how the results of the changes proposed here to compare to the results of the changes in bug 1125462. We may want both patches. I think we may have finally uncovered the fundamental problem, though.

Here's the patch.

Comment on attachment 8554136 [details] [diff] [review] Make sure we request discarding for images in PresShell::Destroy Using a flag for the request discard part would enforce clarity of the code in the future if any one decides to be less diligent with commenting the parameter name then you have in this patch.

(In reply to Timothy Nikkel (:tn) from comment #2) > Using a flag for the request discard part would enforce clarity of the code > in the future if any one decides to be less diligent with commenting the > parameter name then you have in this patch. Yeah, that's a good point. I should probably just move the REQUEST_DISCARD enumeration into the IDL file.

Let's consider this part 1 now.

OK, here's the switch to use an enumeration. It ends up touching a lot of stuff, so I think it's worth making this a separate patch.

Try job here: https://tbpl.mozilla.org/?tree=Try&rev=4f620ba0b9fb

OK, it looks like this change exposed a previously-existing race condition where the observer in browser_bug666317.js can be destroyed before we finish tearing down the document containing the image it's observing, resulting in a null dereference when we try to deliver a DISCARD notification from the RequestDiscard call we're now firing thanks to the changes in this patch. We really need to rethink the ScriptedObserver stuff, but for now this is extremely straightforward to fix - we inline attachDiscardObserver() into step2(), so that a stack reference remains to the observer.

New try job here: https://tbpl.mozilla.org/?tree=Try&rev=46544db174f2

Comment on attachment 8554228 [details] [diff] [review] (Part 2) - Use an enumeration instead of a boolean to request discarding in nsIImageLoadingContent thanks

Thanks for the review! Pushed: remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/fa3e911c2941 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/042523d55ed8

Comment on attachment 8554228 [details] [diff] [review] (Part 2) - Use an enumeration instead of a boolean to request discarding in nsIImageLoadingContent Also if there ever lands without part 1 it would need an iid bump.

Yup, that's true. We'll keep them together.

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/1f0d37966eac - odd bedfellows, but opt Windows hung in one of the various sizes of PNG in image/test/reftest/encoders-lossless/, and Android failed a couple of tests.

OK, investigating the failure here led me to discover bug 1126038. Here's a try job with bug 1126038's patch applied: https://tbpl.mozilla.org/?tree=Try&rev=78533be937b3

Looks like the only failure there is from bug 1126038, and I've fixed it there. Pushed: remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/bbd77af91d82 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/b44e10f83623

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=5959006&repo=mozilla-inbound

OK, I think this is ready to go now. Pushed: remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/71bc716fc021 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/f44673d50e55

https://hg.mozilla.org/mozilla-central/rev/71bc716fc021 https://hg.mozilla.org/mozilla-central/rev/f44673d50e55

Comment on attachment 8554237 [details] [diff] [review] (Part 1) - Make sure we request discarding for images in PresShell::Destroy Approval Request Comment [Feature/regressing bug #]: Part of fix for bug 1125272 and bug 1119938. [User impact if declined]: Already approved. [Describe test coverage new/current, TreeHerder]: In 38 for a long time. [Risks and why]: Low risk; in 38 for a long time. [String/UUID change made/needed]: None.

Pushed to Aurora: remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/63498d169679 remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/7e51c23a4404

Comment on attachment 8554237 [details] [diff] [review] (Part 1) - Make sure we request discarding for images in PresShell::Destroy Part of a collection of ImageLib work that has been tested by QE and verbally approved to land. Adding approval after the fact.