We should only lock CSS images (background-image, border-image, etc) when they're visible. After bug 1157546, this can be implemented by locking CSS images in nsIFrame::OnBecameVisible() and unlocking them in nsIFrame::OnBecameNonvisible(). We'll also need to remove the existing code that locks them forever and throws away the key.

checking in on this since it blocks a performance regression!

(In reply to Joel Maher (:jmaher) from comment #1) > checking in on this since it blocks a performance regression! Patches coming right up. Unfortunately I had to rewrite this one a few times to get an implementation I was really happy with.

Here's the patch! Again, there's only one part, but I've separated the patch in two for easier reviewing. Part 1a takes care of removing the old tracking code for CSS images that's no longer needed given the changes in part 1b. In particular, the style structs that contain images no longer need to support TrackImage() and UntrackImage() calls, and we can just delete the code that calls those methods in most cases, as in part 1b we start relying on the image visibility mechanism instead.

This part (the final part) actually implements visibility tracking for CSS images. It turns out to be pretty simple: - In ImageLoader, AssociateRequestToFrame() now registers the image with the document (locking it) if the frame is visible. DisassociateRequestFromFrame() similarly unregisters the image if the frame is visible. - We also need to consider dynamic changes in visibility, so ImageLoader gains two new methods: RegisterImagesForFrame() and UnregisterImagesForFrame(). These methods are called from nsIFrame::OnBecameVisible() and nsIFrame::OnBecameNonvisible() respectively. That's really about it! Since AssociateRequestToFrame() and DisassociateRequestFromFrame() are already called in the appropriate places (e.g. when a CSS background-image is added or removed for a given frame), these changes are pretty much all we need to make things work. Easy! (Though I have to admit, it took me a couple of passes to realize things could be this simple!)

Looks like there are a few failures in that try job: image/test/mochitest/test_background_image_anim.html (in oth) layout/base/tests/browser_onbeforeunload_only_after_interaction_in_frame.js (in bc3) layout/reftests/bugs/1114526-1.html (in R1 or R2) Surprisingly few considering what a fundamental change this is. I'll investigate those and upload an updated version of the patch.

^^^ This updated try job includes bug 1223747, bug 1223751, and bug 1223753 , which should fix all of the failures above. It's worth noting that these were all existing issues that were just exposed by accurately tracking visibility of CSS images.

A minor rebase was required because of changes in the blocking bugs.

Updated this patch to use IsVisible() when possible.

Based upon the try results above, it looks like this entire patch stack is now ready to land module review.

Comment on attachment 8687052 [details] [diff] [review] (Part 1b) - Register CSS images with the document when their associated frame is visible. >@@ -88,16 +91,47 @@ ImageLoader::AssociateRequestToFrame(imgIRequest* aRequest, >+ if (aFrame->IsVisible()) { >+ nsIDocument* doc = aFrame->PresContext()->Document(); >+ if (doc) { >+ doc->AddImage(aRequest); >+ } >+ } >+ } >+} You can count me as the first victim of the naming of IsVisible, because as I would reading this I was thinking "why is the css style 'visibility: visible' all we need to check here??".

Comment on attachment 8687052 [details] [diff] [review] (Part 1b) - Register CSS images with the document when their associated frame is visible. Are you planning to respond to UnlockedDraw notifications? Because I think you need to (for animation purposes at least).

Jet, is there someone who can pick this work back up again or should we deprioritize it?

tn: is this something we should still try to get landed? If yes, can you help summarize the remaining pieces needed? Thx!

It doesn't seem like it's that high of priority. It doesn't cause us problems in common cases most of the time. The work remaining is similar to the two patches that are posted in this bug, although they have surely bitrot.

P2 per comment 18.