Track whether frames are in an active document using the frame visibility API

REOPENED
Assigned to

Status

()

REOPENED
3 years ago
a year ago

People

(Reporter: seth, Assigned: seth)

Tracking

(Depends on: 4 bugs, Blocks: 4 bugs)

unspecified
mozilla49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

3 years ago
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.
(Assignee)

Comment 1

3 years ago
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.
(Assignee)

Updated

3 years ago
Blocks: 1263019
(Assignee)

Updated

3 years ago
Summary: Track page visibility using the frame visibility API → Track whether frames are in an active document using the frame visibility API
(Assignee)

Comment 2

3 years ago
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
(Assignee)

Comment 3

3 years ago
Created attachment 8741159 [details] [diff] [review]
Mark frames NONVISIBLE if their pres shell is inactive or frozen.

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+
(Assignee)

Comment 11

2 years ago
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.
(Assignee)

Updated

2 years ago
Depends on: 1269934
(Assignee)

Updated

2 years ago
Depends on: 1269935
(Assignee)

Updated

2 years ago
Depends on: 1269937
(Assignee)

Comment 12

2 years ago
Created attachment 8748449 [details] [diff] [review]
Mark frames NONVISIBLE if their pres shell is inactive or frozen.

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)
(Assignee)

Updated

2 years ago
Attachment #8741159 - Attachment is obsolete: true
(Assignee)

Comment 13

2 years ago
The try job in comment 10 corresponds to this version of the patch.
Attachment #8748449 - Flags: review?(mstange) → review+
(Assignee)

Comment 14

2 years ago
Created attachment 8748988 [details] [diff] [review]
Mark frames NONVISIBLE if their pres shell is inactive or frozen.

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.
(Assignee)

Updated

2 years ago
Attachment #8748449 - Attachment is obsolete: true

Updated

2 years ago
Blocks: 1270389
(Assignee)

Comment 15

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/719d6d5d9d2114dec8bff62d646435ab42cf5491
Bug 1259281 - Mark frames NONVISIBLE if their pres shell is inactive or frozen. r=mstange

Comment 16

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/719d6d5d9d21
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
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?
Flags: needinfo?(tnikkel)
I don't plan to work on this. Whoever is working on intersection observer may want to merge these two things.
Flags: needinfo?(tnikkel)
You need to log in before you can comment on or make changes to this bug.