Subframes' displayport base rects are clipped to their scrollport (in ScrollFrameHelper::DecideScrollableLayer). This is fine when they have small scrollports. However, sometimes scrollports can be very large. For example, https://wiki.openstreetmap.org/wiki/FR:%C3%89l%C3%A9ments_cartographiques on fennec. On a narrow phone screen the tables become scrollable subframes which scroll horizontally. Vertically, however, the tables are displayed in full, meaning their scrollport is many screens tall. This results in the table layers having very large display ports - much larger than that of the page's main layer. Should a frame's base displayport size be restricted even further to the size of the parent frame's base displayport? Or something else?

Yeah, it should be restricted to the "root composition size". There used to be code that does this, added in bug 986413. The relevant hunks seem to be missing in m-c, not sure when it got taken out... I'll dig around a bit.

Oh, actually it's still there, it just got refactored. So the call to aFrameMetrics.CalculateBoundedCompositedSizeInCssPixels in CalculatePendingDisplayPort should be clamping the displayport to the root composition size, although perhaps that code path isn't always getting the correct root composition size?

In CalculatePendingDisplayPort we use the bounded composited size when calculating the display port size. Then we use the display port size along with the bounded composited size (again) to calculate the margins. Instead, I think we should be using the non-bounded composited size when calculating the margins. This will give the negative margins we talked about on irc, and a sensible final display port size. Sound correct? I'll whip up a patch if so.

That sounds reasonable to me. tnikkel, what do you think? As far as i can tell, the displayport base (to which the margins are applied on the layout side) is not getting bounded, so this should work.

The other option would be to bound the display port base to the root composition size? I don't really know this code, but intuitively that would make more sense to me if it's possible.

(In reply to Jamie Nicol [:jnicol] from comment #5) > The other option would be to bound the display port base to the root > composition size? I don't really know this code, but intuitively that would > make more sense to me if it's possible. I agree. I think we could do this fairly easily by bounding the display port base just after reading it in order to combine it with the margins [1]. We can calculate the root composition at that size by calling nsLayoutUtils::CalculateBasicFrameMetrics [2] and getting the root composition size from the resulting FrameMetrics. [1] https://dxr.mozilla.org/mozilla-central/rev/d6d81655dd9e146c300a64c0fcaeb04ca3300a19/layout/base/nsLayoutUtils.cpp#891 [2] https://dxr.mozilla.org/mozilla-central/rev/d6d81655dd9e146c300a64c0fcaeb04ca3300a19/layout/base/nsLayoutUtils.h#2654

The displayport base (for subframes) is the intersection of the dirty rect and the scroll port. If the document doesn't have any other display ports then the dirty rect would be less than or equal to the size of the root scroll frame (basically the root composition size). But we do have display ports, so the dirty rect takes into account those as well (ie it contains what is rendered of the subframe in the displayport of the parent scroll frame, not what is visible after the parent scroll frames clips). I would consider this a bug as in the design the displayport base was intended to be what is visible, not what is rendered. So fixing that bug would be ideal, instead of hacking around it in other places.

As for effectively doing that perhaps this is a good approach: 1) get the composition bounds of the root scroll frame of the root content doc (or root doc if there isn't a root content doc) 2) transform those bounds to our local scroll frame using something like nsLayoutUtil::TransformFrameRectToAncestor (we'd need from ancestor descendant instead though) 3) only do this if there is a displayport set to avoid the overhead when its not needed

I did some digging into gfx memory usage, and on amazon.com we use 650MB or so in GL textures on a Nexus 6P (which has a 2k screen). This is pretty unreasonable. Layer dump below. Botond thinks it is due to this bug. Note the gigantic subframe region bounds. 01-26 17:29:36.378 8345 8407 I Gecko : LayerManager (0xcab462e0) 01-26 17:29:36.378 8345 8407 I Gecko : ContainerLayerComposite (0xc743f400) [shadow-visible=< (x=0, y=0, w=1440, h=2140); >] [visible=< (x=0, y=0, w=1440, h=2140); >] [componentAlpha] [metrics0={ [cb=(x=0.000000, y=0.000000, w=1440.000000, h=2140.000000)] [sr=(x=0.000000, y=0.000000, w=408.000000, h=606.333313)] [s=(0,0)] [dp=(x=0.000000, y=0.000000, w=408.000000, h=606.333313)] [cdp=(x=0.000000, y=0.000000, w=408.000000, h=606.333313)] [color=rgba(0, 0, 0, 0.000000)] [scrollId=2] [z=3.53] }] 01-26 17:29:36.378 8345 8407 I Gecko : PaintedLayerComposite (0xbf0f3c00) [not visible] { hitregion=< (x=0, y=0, w=1440, h=2140); >} [opaqueContent] 01-26 17:29:36.378 8345 8407 I Gecko : ContainerLayerComposite (0xc7440000) [shadow-clip=(x=0, y=0, w=1440, h=2140)] [shadow-visible=< (x=0, y=0, w=1440, h=2140); >] [clip=(x=0, y=0, w=1440, h=2140)] [visible=< (x=0, y=0, w=1440, h=2140); >] [componentAlpha] [metrics0={ [cb=(x=0.000000, y=0.000000, w=1440.000000, h=2140.000000)] [sr=(x=0.000000, y=0.000000, w=408.000000, h=2980.600098)] [s=(0,0)] [dp=(x=0.000000, y=0.000000, w=408.000000, h=2980.600098)] [cdp=(x=0.000000, y=0.000000, w=408.000000, h=1885.866699)] [color=rgba(255, 255, 255, 1.000000)] [scrollId=4] [scrollParent=2] [rcd] [z=3.53] }] [force-dtc] 01-26 17:29:36.378 8345 8407 I Gecko : PaintedLayerComposite (0xbf0ef800) [not visible] { hitregion=< (x=0, y=0, w=1440, h=2140); >} [opaqueContent] 01-26 17:29:36.378 8345 8407 I Gecko : PaintedLayerComposite (0xc3284800) [shadow-clip=(x=0, y=0, w=0, h=0)] [clip=(x=0, y=0, w=0, h=0)] [not visible] 01-26 17:29:36.378 8345 8407 I Gecko : ColorLayerComposite (0xc3284c00) [bounds=(x=0, y=0, w=1440, h=10520)] [visible=< (x=0, y=0, w=1440, h=10520); >] { hitregion=< (x=0, y=0, w=1440, h=10520); > dispatchtocontentregion=< (x=0, y=0, w=1440, h=10520); >} [opaqueContent] [color=rgba(255, 255, 255, 1.000000)] [bounds=(x=0, y=0, w=1440, h=10520)] 01-26 17:29:36.378 8345 8407 I Gecko : PaintedLayerComposite (0xc9406000) [shadow-clip=(x=0, y=0, w=1440, h=10520)] [shadow-visible=< (x=0, y=0, w=1440, h=2140); >] [bounds=(x=-5, y=0, w=1450, h=10520)] [visible=< (x=0, y=0, w=1440, h=4402); (x=0, y=4402, w=21, h=671); (x=699, y=4402, w=44, h=671); (x=1420, y=4402, w=20, h=671); (x=0, y=5073, w=1440, h=3); (x=0, y=5076, w=21, h=670); (x=699, y=5076, w=44, h=670); (x=1420, y=5076, w=20, h=670); (x=0, y=5746, w=1440, h=4774); >] { hitregion=< (x=0, y=0, w=1440, h=10520); > dispatchtocontentregion=< (x=0, y=0, w=1440, h=10520); >} [opaqueContent] [metrics0={ [cb=(x=0.000000, y=0.000000, w=1440.000000, h=10519.764648)] [sr=(x=0.000000, y=0.000000, w=408.000000, h=2980.600098)] [s=(0,0)] [dp=(x=0.000000, y=0.000000, w=408.000000, h=2980.600098)] [cdp=(x=0.000000, y=0.000000, w=408.000000, h=2980.600098)] [color=rgba(0, 0, 0, 0.000000)] [scrollId=5] [scrollParent=4] [clip=(x=0, y=0, w=1440, h=10520)] [z=3.53] }] [valid=< (x=0, y=0, w=1440, h=4402); (x=0, y=4402, w=21, h=671); (x=699, y=4402, w=44 01-26 17:29:36.378 8345 8407 I Gecko : TiledContentHost (0xc4c6eb60) 01-26 17:29:36.378 8345 8407 I Gecko : ContainerLayerComposite (0xc7f28c00) [shadow-clip=(x=0, y=480, w=1440, h=441)] [shadow-transform=[ 1 0; 0 1; 0 480; ]] [shadow-visible=< (x=0, y=0, w=1440, h=441); >] [clip=(x=0, y=480, w=1440, h=441)] [transform=[ 1 0; 0 1; 0 480; ]] [visible=< (x=0, y=0, w=1440, h=441); >] [backfaceHidden] [metrics0={ [cb=(x=0.000000, y=0.000000, w=1440.000000, h=10519.764648)] [sr=(x=0.000000, y=0.000000, w=408.000000, h=2980.600098)] [s=(0,0)] [dp=(x=0.000000, y=0.000000, w=408.000000, h=2980.600098)] [cdp=(x=0.000000, y=0.000000, w=408.000000, h=2980.600098)] [color=rgba(0, 0, 0, 0.000000)] [scrollId=5] [scrollParent=4] [clip=(x=0, y=0, w=1440, h=10520)] [z=3.53] }] 01-26 17:29:36.378 8345 8407 I Gecko : PaintedLayerComposite (0xbf0ec800) [not visible] { hitregion=< (x=0, y=0, w=1440, h=442); > dispatchtocontentregion=< (x=0, y=0, w=1440, h=442); >} [opaqueContent] [backfaceHidden] 01-26 17:29:36.378 8345 8407 I Gecko : ContainerLayerComposite (0xc7fbbc00) [shadow-clip=(x=0, y=0, w=1440, h=441)] [shadow-visible=< (x=0, y=0, w=1440, h=441); >] [clip=(x=0, y=0, w=1440, h=441)] [visible=< (x=0, y=0, w=1440, h=442); >] [backfaceHidden] 01-26 17:29:36.378 8345 8407 I Gecko : ContainerLayerComposite (0xc85e8000) [shadow-visible=< (x=0, y=0, w=1440, h=441); >] [visible=< (x=0, y=0, w=1440, h=442); >] [backfaceHidden] 01-26 17:29:36.378 8345 8407 I Gecko : PaintedLayerComposite (0xbf0ec400) [not visible] { hitregion=< (x=0, y=0, w=8640, h=442); > dispatchtocontentregion=< (x=0, y=0, w=8640, h=442); >} [opaqueContent] [backfaceHidden] 01-26 17:29:36.378 8345 8407 I Gecko : PaintedLayerComposite (0xc85e9800) [shadow-clip=(x=0, y=0, w=0, h=0)] [clip=(x=0, y=0, w=0, h=0)] [not visible] 01-26 17:29:36.378 8345 8407 I Gecko : ImageLayerComposite (0xc9401800) [shadow-clip=(x=0, y=0, w=1440, h=441)] [shadow-transform=[ 1.17647 0; 0 1.17647; -10.5882 0; ]] [shadow-visible=< (x=8, y=0, w=1225, h=375); >] [clip=(x=0, y=0, w=1440, h=441)] [transform=[ 1.17647 0; 0 1.17647; -10.5882 0; ]] [bounds=(x=-11, y=0, w=1462, h=442)] [visible=< (x=8, y=0, w=1225, h=375); >] { hitregion=< (x=8, y=0, w=1225, h=376); > dispatchtocontentregion=< (x=8, y=0, w=1225, h=376); >} [opaqueContent] 01-26 17:29:36.378 8345 8407 I Gecko : ImageHost (0xc4ca6e70) 01-26 17:29:36.378 8345 8407 I Gecko : MemoryTextureHost (0xceca1500) [size=(w=1242, h=375)] [format=SurfaceFormat::B8G8R8A8] [flags=NoFlags] [picture-rect=(x=0, y=0, w=1242, h=375)] 01-26 17:29:36.378 8345 8407 I Gecko : PaintedLayerComposite (0xc3284000) [shadow-clip=(x=0, y=0, w=1440, h=10520)] [bounds=(x=7, y=4402, w=1427, h=1344)] [visible=< (x=7, y=4402, w=706, h=671); (x=729, y=4402, w=705, h=671); (x=7, y=5076, w=706, h=670); (x=729, y=5076, w=705, h=670); >] { hitregion=< (x=7, y=4402, w=706, h=671); (x=728, y=4402, w=707, h=671); (x=7, y=5075, w=706, h=672); (x=728, y=5075, w=707, h=672); > dispatchtocontentregion=< (x=7, y=4402, w=706, h=671); (x=728, y=4402, w=707, h=671); (x=7, y=5075, w=706, h=672); (x=728, y=5075, w=707, h=672); >} [backfaceHidden] [metrics0={ [cb=(x=0.000000, y=0.000000, w=1440.000000, h=10519.764648)] [sr=(x=0.000000, y=0.000000, w=408.000000, h=2980.600098)] [s=(0,0)] [dp=(x=0.000000, y=0.000000, w=408.000000, h=2980.600098)] [cdp=(x=0.000000, y=0.000000, w=408.000000, h=2980.600098)] [color=rgba(0, 0, 0, 0.000000)] [scrollId=5] [scrollParent=4] [clip=(x=0, y=0, w=1440, h=10520)] [z=3.53] }] [valid=< (x=7, y=4402, w=706, h=671); (x=729, y=4402, w=705, h=671); (x=7, y=5076, w=706, h=670 01-26 17:29:36.378 8345 8407 I Gecko : TiledContentHost (0xc3390b70) 01-26 17:29:36.378 8345 8407 I Gecko : PaintedLayerComposite (0xc323a400) [shadow-clip=(x=0, y=0, w=1440, h=10520)] [bounds=(x=41, y=4909, w=1305, h=803)] [visible=< (x=42, y=4910, w=1108, h=65); (x=42, y=4975, w=1112, h=64); (x=42, y=5584, w=1039, h=64); (x=42, y=5648, w=1304, h=64); >] [componentAlpha] [metrics0={ [cb=(x=0.000000, y=0.000000, w=1440.000000, h=10519.764648)] [sr=(x=0.000000, y=0.000000, w=408.000000, h=2980.600098)] [s=(0,0)] [dp=(x=0.000000, y=0.000000, w=408.000000, h=2980.600098)] [cdp=(x=0.000000, y=0.000000, w=408.000000, h=2980.600098)] [color=rgba(0, 0, 0, 0.000000)] [scrollId=5] [scrollParent=4] [clip=(x=0, y=0, w=1440, h=10520)] [z=3.53] }] [valid=< (x=42, y=4910, w=1108, h=65); (x=42, y=4975, w=1112, h=64); (x=42, y=5584, w=1039, h=64); (x=42, y=5648, w=1304, h=64); >] 01-26 17:29:36.379 8345 8407 I Gecko : TiledContentHost (0xc3390c60)

I think I've done what you suggested in comment 8. I've verified that it does as intended - memory usage is down and the apz minimap looks sensible.

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=71f86c7b22cd Ludovic, could you check whether you can crash with that build? (https://archive.mozilla.org/pub/mobile/try-builds/jnicol@mozilla.com-71f86c7b22cd3e94b4139791768e5f21ff5506db/try-android-api-11/fennec-47.0a1.en-US.android-arm.apk)

(In reply to Jamie Nicol [:jnicol] from comment #11) > Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=71f86c7b22cd > > Ludovic, could you check whether you can crash with that build? > (https://archive.mozilla.org/pub/mobile/try-builds/jnicol@mozilla.com- > 71f86c7b22cd3e94b4139791768e5f21ff5506db/try-android-api-11/fennec-47.0a1.en- > US.android-arm.apk) not crashing.

Comment on attachment 8712618 [details] [diff] [review] Patch v1 >Previously they were restricted only to the scrollport of the subframe, >which could lead to very large displayports in cases where the >scrollport was larger than the root composition bounds. This explanation is misleading because it makes it seem like the only restriction on the displayport base is the scrollport. I'd suggest something like this: "Previously displayport bases were computed as the intersection of the scrollport with the dirtyrect. However the dirtyrect covers what is rendered, and with displayports what we render can be much larger than what is visible. With displayport bases intended to represent what was visible, this was a problem. By restricting them to the root composition size this makes them more closely match what is visible. To do this more properly we'd want to intersect the dirtyrect with the scroll clip of every ancestor scroll frame, not just the root composition bounds." >+ if (rootPresContext) { >+ const nsIPresShell* const rootPresShell = rootPresContext->PresShell(); >+ nsIFrame* const rootScrollFrame = rootPresShell->GetRootScrollFrame(); >+ if (rootScrollFrame) { If there is no root scroll frame (XUL documents) then you should use the root frame.

Use the root frame if there is no root scroll frame, and updated commit message.

Try run on patch v2: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a23f8648060&selectedJob=16347264 few unrelated intermittents but it looks good. Requesting checkin of patch v2 please.

Kats, do we need to uplift this to 46? (Note to self: if we do, then we also need to remember to uplift the performance regression fix in 1246443, once it's landed)

I don't see a strong reason to uplift to 46 at the moment. This patch helps a lot for Fennec but so far we haven't run into specific cases that would be helped by it on desktop. So if it rides on 47 that should be fine, because APZ is only turned on for desktop in 46 and will only get enabled for Fennec in 47 (or later). Unless something comes up for desktop that this fixes, I'm inclined to just leave it out of 46 and let it ride.