Closed Bug 989897 Opened 6 years ago Closed 6 years ago

Categories

(Core :: Panning and Zooming, defect)

29 Branch
x86
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31
Tracking Status
firefox30 --- fixed
firefox31 --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: tnikkel, Assigned: tnikkel)

References

Details

(Keywords: perf, Whiteboard: [MemShrink:P2])

Attachments

(1 file)

See bug 957668 comment 87 for str.
I think there are two problems here. The main one is that the root composition bounds aren't calculated correctly. The other one is that the baserect we use for the iframe's display port incorporates the display port for the root scroll frame of the root content document, this means the display port for the iframe essentially gets expanded twice. We should be using what is actually visible, not what is visible in the display port.

The reason the root composition bounds are incorrect is this line
   CSSSize size =
     ParentLayerSize(rootCompositionSize) / (parentResolution * aMetrics.mDevPixelsPerCSSPixel);
This converts from parent layer pixels to CSS pixels, but the CSS pixels are of the parent layer. One CSS pixel in the child layer will take up |resolution| CSS pixels in the parent (where resolution is the resolution of the child). It would make sense to store the root composition bounds in parent layer pixels, except the parent layer in question might be the grand parent or higher. Maybe we need root layer pixels or something.
Is this a child process OOM (browser tab crashed with "well this is embarrassing") or a parent process OOM (phone restart)?
Keywords: perf
Whiteboard: [MemShrink]
this is child process OOM (browser tab crashed with "well this is embarrassing")
The calculation was just wrong.

The key here is that we want to view the root composition size as layer pixels in the current layer (because that is what we want to use it to limit, the actual number of layer pixels in the current layer) and then convert it to CSS pixels. There is no magic conversion to do what we want, we just want plop the root composition size down in any layer and use it as is.
Attachment #8400069 - Flags: review?(bugmail.mozilla)
This is a problem in the code from bug 986413.
Blocks: 986413
Whiteboard: [MemShrink] → [MemShrink:P2]
Comment on attachment 8400069 [details] [diff] [review]
fix root composition size calculation

Review of attachment 8400069 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsDisplayList.cpp
@@ +677,5 @@
>          } else {
> +          LayoutDeviceToParentLayerScale parentResolution(1.0f);
> +          parentResolution.scale =
> +            rootPresShell->GetCumulativeResolution().width
> +            / rootPresShell->GetResolution().width;

Combine this assignment into the parentResolution declaration

LayoutDeviceToParentLayerScale parentResolution(
   blah.width / blah.width);
Attachment #8400069 - Flags: review?(bugmail.mozilla) → review+
Filed bug 991488 for the other problem mentioned in comment 1, the one not addressed by the patch.
https://hg.mozilla.org/mozilla-central/rev/4c74157ac995
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Comment on attachment 8400069 [details] [diff] [review]
fix root composition size calculation

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 957668, bug 986413
User impact if declined: OOM due to asking for too large a displayport on webpages, eg etherpad
Testing completed (on m-c, etc.): been on m-c for several days now
Risk to taking this patch (and alternatives if risky): relatively low, should affect b2g only
String or IDL/UUID changes made by this patch: none
Attachment #8400069 - Flags: approval-mozilla-aurora?
Attachment #8400069 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
OS: Mac OS X → Gonk (Firefox OS)
You need to log in before you can comment on or make changes to this bug.