Timothy Nikkel (:tnikkel) Assignee
	Description • 10 years ago

See bug 957668 comment 87 for str.

	Timothy Nikkel (:tnikkel) Assignee
	Comment 1 • 10 years ago

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.

	Jason Smith [:jsmith]
	Comment 2 • 10 years ago

Is this a child process OOM (browser tab crashed with "well this is embarrassing") or a parent process OOM (phone restart)?

	No-Jun Park [:njpark]
	Comment 3 • 10 years ago

this is child process OOM (browser tab crashed with "well this is embarrassing")

	Timothy Nikkel (:tnikkel) Assignee
	Comment 4 • 10 years ago

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.

	Timothy Nikkel (:tnikkel) Assignee
	Comment 5 • 10 years ago

This is a problem in the code from bug 986413.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 6 • 10 years ago

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);

	Timothy Nikkel (:tnikkel) Assignee
	Comment 7 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/4c74157ac995

	Timothy Nikkel (:tnikkel) Assignee
	Comment 8 • 10 years ago

Filed bug 991488 for the other problem mentioned in comment 1, the one not addressed by the patch.

	Carsten Book [:Tomcat]
	Comment 9 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/4c74157ac995

	Timothy Nikkel (:tnikkel) Assignee
	Comment 10 • 10 years ago

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

	Timothy Nikkel (:tnikkel) Assignee
	Comment 11 • 10 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/eef9ddcce204