Closed Bug 1529892 Opened 1 year ago Closed 1 year ago

Fixed position elements can easily cause OOM when zoomed in far

Categories

(Core :: Graphics: Layers, defect)

65 Branch
Unspecified
Android
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox66 --- wontfix
firefox67 + fixed
firefox68 --- fixed

People

(Reporter: jnicol, Assigned: botond)

References

()

Details

Attachments

(6 files, 2 obsolete files)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
3.14 KB, patch
Details | Diff | Splinter Review

I noticed this on duck duck go maps. Due to bug 1525570 attempting to zoom in on the map actually zooms the viewport. The page has a large fixed position element for the information panel, which then becomes incredibly large. Because its layer is non-scrollable, we use a SingleTiledContentClient for it. SingleTiledContentClients aren't clipped by the critical display port, so we allocate the full size of the layer and it ends badly.

I think for non-scrollable layers we should still use a MultiTiledContentClient once their size becomes greater than either the screen size or critical display port size. Not sure yet how we get that information to the place where we choose the ContentClient.

Attachment #9046098 - Attachment is obsolete: true

Hey Jamie, could you give one of the Android builds from this Try push a whirl?

https://treeherder.mozilla.org/#/jobs?repo=try&tier=1&revision=fea05df9ee4fbbcdcbe6fc96eb4d86741f6f9031

They are failing an Android test but I'm pretty sure that's an issue with the test rather than an issue with the fix.

Flags: needinfo?(jnicol)

Great, this seems to work! It prevents the crash when zoomed fully in to an image on twitter. The checkerboarding, however, is very apparent when you scroll quickly whilst zoomed in. I definitely prefer checkerboarding to crashing, but I wonder if tweaking the margins might improve it somewhat.

Flags: needinfo?(jnicol)

(In reply to Jamie Nicol [:jnicol] from comment #4)

Great, this seems to work! It prevents the crash when zoomed fully in to an image on twitter. The checkerboarding, however, is very apparent when you scroll quickly whilst zoomed in. I definitely prefer checkerboarding to crashing, but I wonder if tweaking the margins might improve it somewhat.

Does it checkerboard more than scrolled (that is, not position:fixed) content zoomed in and scrolled at a similar speed? We are sizing the displayport the same way in both cases (except I guess that we may not have a low-resolution displayport for fixed content).

FWIW, I did try this on Twitter, but for me, Twitter doesn't let me zoom into images fully (like, anywhere near 10x) in the first place. It looks to me like their image view intercepts touch events and performs its own zooming, with a rubber-banding effect after a small amount of zoom (perhaps 2x or 3x). Not sure if you're doing something different perhaps.

(In reply to Botond Ballo [:botond] from comment #6)

FWIW, I did try this on Twitter, but for me, Twitter doesn't let me zoom into images fully (like, anywhere near 10x) in the first place. It looks to me like their image view intercepts touch events and performs its own zooming, with a rubber-banding effect after a small amount of zoom (perhaps 2x or 3x). Not sure if you're doing something different perhaps.

Yes, I needed to enable "Always enable zoom" in Settings -> Accessibility, as was suggested on bug 1532386 by someone. I assume the reporter of that bug must have had that enabled because I couldn't reproduce without it.

I am not sure if it is checkerboarding more or not. I will have a further play around with this tomorrow and see what I can see.

(In reply to Jamie Nicol [:jnicol] from comment #7)

Yes, I needed to enable "Always enable zoom" in Settings -> Accessibility, as was suggested on bug 1532386 by someone. I assume the reporter of that bug must have had that enabled because I couldn't reproduce without it.

Ah, got it. So yeah, I tried it that way, and I do see checkerboarding when scrolling fast, but there clearly is a displayport (if you scroll more slowly you can tell we're async scrolling but not checkerboarding). I think it checkerboards about the same as scrolled content, though scrolled content appears to checkerboard less due to low-resolution painting of a larger area. We could consider doing low-resolution painting for fixed content too; I think it would require using a tiled layer for them?

(In reply to Botond Ballo [:botond] from comment #3)

https://treeherder.mozilla.org/#/jobs?repo=try&tier=1&revision=fea05df9ee4fbbcdcbe6fc96eb4d86741f6f9031

They are failing an Android test but I'm pretty sure that's an issue with the test rather than an issue with the fix.

This is looking better:

https://treeherder.mozilla.org/#/jobs?repo=try&tier=1&revision=7dd70d10d75672d71e287b7611ffea7a1dcebad4

The fixes a latent bug with WebRender where we would clear it after reading it
in ComputeScrollMetadata, but WR would sometimes call ComputeScrollMetadata a
second time for the same scroll frame in the same transaction, resulting in
the update sometimes not making it into the transaction.

Depends on D28775

Attachment #9057406 - Attachment is obsolete: true
Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2e7759e843ac
Use zooming pref rather than platform ifdef in OutOfFlowDisplayData::ComputeVisibleRectForFrame(). r=kats
https://hg.mozilla.org/integration/autoland/rev/dfe53274deb8
Extend APZTestData with free-form additional data not grouped by paint or scroll frame. r=kats,Ehsan
https://hg.mozilla.org/integration/autoland/rev/eef11daea095
Add a mochitest to check that the rendered area for a fixed-position element matches our expectations. r=kats
https://hg.mozilla.org/integration/autoland/rev/c63d6ed08651
Limit the area of a fixed-position element that's painted to the displayport. r=kats
https://hg.mozilla.org/integration/autoland/rev/094b212a3cbf
Move the clearing of a pending visual scroll update to the end of the paint. r=kats
Assignee: jnicol → botond
Duplicate of this bug: 1532386

[Tracking Requested - why for this release]:

Carrying over tracking flag from bug 1532386, which these patches fixed.

This is just the fix (the fourth patch) rebased onto beta.

I plan to request uplift just for this patch; the other patches are all related to the test, including adding new test infrastructure which doesn't seem appropriate to uplift at this stage.

Comment on attachment 9062004 [details] [diff] [review]
Fix rebased onto beta

Beta/Release Uplift Approval Request

  • User impact if declined: On Android, when zooming in on pages with position: fixed elements, the browser can try to render a texture that's too large to fit into memory, and OOM as a result.

The OOM has been observed on Twitter's picture view, albeit only with Settings -> Accessibility -> "Always enable zoom" checked (which is not the default). However, it can potentially be triggered on other sites (which don't impose a maximum zoom level) even with default settings.

  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Medium
  • Why is the change risky/not risky? (and alternatives if risky): This is a small, targeted fix. The code it touches is Android-only and specific to the handling of position: fixed elements; nonetheless, it is display-list building code which can be tricky to reason about. The patch has had ~4 days to bake on Nightly so far.

Given the above factors, and where we are in the cycle, I would classify an uplift as medium risk. It does fix a bug with severe symptoms (OOM; see bug 1532386), so I am suggesting to uplift anyways.

  • String changes made/needed: none
Attachment #9062004 - Flags: approval-mozilla-beta?
Comment on attachment 9062004 [details] [diff] [review]
Fix rebased onto beta

As it fixes bug 1532386 and the try build is green, I am taking the uplift in 67 beta 16, thanks.
Attachment #9062004 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
See Also: → 1549292
You need to log in before you can comment on or make changes to this bug.