Open Bug 1259281 Opened 5 years ago Updated 4 years ago
Track whether frames are in an active document using the frame visibility API
The frame visibility API should expose whether the page containing the frame is visible. This will let us get rid of a lot of existing code (e.g. nsPresShell::UpdateImageLockingState() and the related tracking code in nsDocument and nsImageLoadingContent) and implement the same features more cleanly. Once this is done, we will be able to distinguish the following states: (1) Frame is in a visible page and is visible within that page. (2) Frame is in a visible page and is nonvisible within that page. (3) Frame is in a nonvisible page and is visible within that page. (4) Frame is in a nonvisible page and is nonvisible within that page. Distinguishing these states would be very useful for image memory management in particular. I'm not yet certain, but it's probably enough to use a single bit from the existing frame visibility counter to implement this.
I should note that "page visibility" is actually bad terminology here - that sounds like we're talking about the DOM page visibility API. I'm actually referring to the notion of an "active" document (or more precisely, a document that is both active and thawed) - see nsPresShell::UpdateImageLockingState() for the test that's in use currently.
Summary: Track page visibility using the frame visibility API → Track whether frames are in an active document using the frame visibility API
After actually implementing this, I changed my mind about precisely what should be implemented in this bug. I think for now, all we should do is mark frames in an inactive or frozen pres shell NONVISIBLE. The reason is that our current behavior is actually to not do anything different for frames in an inactive or frozen pres shell than we do for frames where are outside the approximately visible region. Since we don't actually have any different code we'd want to run in the different states in comment 0 right now, there's no particular urgency to implement these additional states. However, these additional states come with a cost: if we support them, we need to store the set of all frames for which we're tracking visibility on the pres shell, because we have to have a way to e.g. tell a nonvisible frame that it's now in an inactive document, and a nonvisible frame won't appear in the current visible frames list. There *are* new behaviors that we might want to implement in the future for these new states, such as being more aggressive about freeing image memory when a tab is backgrounded. However, because it's unclear whether we actually want to do these things, I think that the additional memory cost of the set of all visible frames should be part of the calculus for whether we want to implement those behaviors. That means that we should add those additional visibility states in the bug that implements those behaviors, not in this bug.
Depends on: 1261554
Here's the patch. The changes are mostly to comments except in the pres shell code, where we mark all currently-visible frames (in either sense) as nonvisible when the pres shell becomes inactive or frozen, and restore them to visibility when the pres shell becomes active and nonfrozen. This is pretty simple. We do have to be careful that we don't increment or decremement visible counts for frames that are forced to be visible by code outside of the pres shell, though, so there's a little song and dance to make sure that's taken care of.
Attachment #8741159 - Flags: review?(mstange)
Comment on attachment 8741159 [details] [diff] [review] Mark frames NONVISIBLE if their pres shell is inactive or frozen. Review of attachment 8741159 [details] [diff] [review]: ----------------------------------------------------------------- Seems like it's worth putting "mIsActive && !mFrozen" into a utility method on the pres shell. Maybe VisibleForFrameVisibilityPurposes()?
Attachment #8741159 - Flags: review?(mstange) → review+
As you might have guessed from all these try jobs, there were some bugs in the patch above. I've fixed them, but in the process I've ended up totally rewriting the patch. It's much cleaner now. It also puts us on the path of gradually factoring the visibility tracking code out of PresShell itself, which I think is a very good thing. Patch incoming shortly.
Here's the new version of the patch. The vast majority of the patch moves the visible frames sets into a VisibleFramesContainer type that handles the bookkeeping of adding frames to the appropriate set. Beyond further centralizing share frame visibility logic and moving more code out of PresShell itself, VisibleFramesContainer has an additional feature: it can "suppress" visibility tracking, which simply means that we continue to keep track of the visible frames sets, but we don't actually increment or decrement visibility counters. To be more precise, the invariants we maintain are these: (1) When visibility tracking is unsuppressed (the normal state), any frame in one of the visible frames sets has had the appropriate visibility counter incremented. (2) When visibility tracking is suppressed, none of the frames in the visible frames sets has had the corresponding visibility counter incremented. That means that when we transition from unsuppressed to suppressed, we need to decrement all the counters, and when we transition from suppress to unsuppressed, we need to increment them. Once we have this feature, we can trivially implement the behavior we want in this bug just by calling SuppressVisibility() when the pres shell stops being active and UnsuppressVisibility() when it becomes active again. This patch also includes that, change, which is pretty small compared to the additional of VisibleFramesContainer. Sorry for rewriting this after requesting reviewing. After debugging the issues that showed up on try I ended up becoming uncomfortable with the old design, which felt really fragile to me. This is a lot cleaner and safer.
Attachment #8748449 - Flags: review?(mstange)
Attachment #8741159 - Attachment is obsolete: true
The try job in comment 10 corresponds to this version of the patch.
Attachment #8748449 - Flags: review?(mstange) → review+
Thanks for the review! This updated version is rebased and has two tiny changes: - The MOZ_CRASH() calls are moved after the switch statements instead of being placed in a default case, which lets the compiler give us exhaustiveness checking for the switch cases. - I deleted a leftover declaration for a method that got removed in this patch.
Attachment #8748449 - Attachment is obsolete: true
https://hg.mozilla.org/integration/mozilla-inbound/rev/719d6d5d9d2114dec8bff62d646435ab42cf5491 Bug 1259281 - Mark frames NONVISIBLE if their pres shell is inactive or frozen. r=mstange
Depends on: 1272357
I backed this bug out on inbound, I expect the backout to get merged to mozilla-central. I plan to also request uplift to backout on aurora (so this bug would not be landed in any version of Firefox). Bug 1284350 tracks the back out.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Timothy, do you have any interest in following up on this or know of someone who does?
I don't plan to work on this. Whoever is working on intersection observer may want to merge these two things.
You need to log in before you can comment on or make changes to this bug.