1082745 - Avoid reoptimizing optimized SourceSurfaceCairos

Status: VERIFIED FIXED

Description

[Michael Wu [:mwu]]

Attachment: Avoid reoptimizing optimized SourceSurfaceCairos

In some cases, OptimizeSourceSurface will be called on a surface that's already been optimized. This patch makes the function return early in that case.

Comment 1

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8504923 [details] [diff] [review]
Avoid reoptimizing optimized SourceSurfaceCairos

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

::: gfx/2d/DrawTargetCairo.cpp
@@ +1277,5 @@
>  #ifdef CAIRO_HAS_XLIB_SURFACE
> +  if (aSurface->GetType() == SurfaceType::CAIRO &&
> +      cairo_surface_get_type(
> +        static_cast<SourceSurfaceCairo*>(aSurface)->GetSurface()) ==
> +          CAIRO_SURFACE_TYPE_XLIB) {

We could probably make this generic and compare the type of aSurface to that of mSurface.

That'd make it cleaner if we ever implement optimizing for other cairo backends.

Comment 2

[Bas Schouten (:bas.schouten)]

Comment on attachment 8504923 [details] [diff] [review]
Avoid reoptimizing optimized SourceSurfaceCairos

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

::: gfx/2d/DrawTargetCairo.cpp
@@ +1277,5 @@
>  #ifdef CAIRO_HAS_XLIB_SURFACE
> +  if (aSurface->GetType() == SurfaceType::CAIRO &&
> +      cairo_surface_get_type(
> +        static_cast<SourceSurfaceCairo*>(aSurface)->GetSurface()) ==
> +          CAIRO_SURFACE_TYPE_XLIB) {

Agreed.

Comment 3

[Michael Wu [:mwu]]

Attachment: Avoid reoptimizing optimized SourceSurfaceCairos, v2

The check is now generic.

Comment 4

[Michael Wu [:mwu]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f013910bbead

Comment 5

[Michael Wu [:mwu]]

[Tracking Requested - why for this release]:
This fixes severe leaks for certain canvas users (like freeciv) on GTK/X11.

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f013910bbead

Comment 7

[Lawrence Mandel [:lmandel] (use needinfo)]

(In reply to Michael Wu [:mwu] from comment #5)
> [Tracking Requested - why for this release]:
> This fixes severe leaks for certain canvas users (like freeciv) on GTK/X11.

How far back does this issue go?

Comment 8

[Michael Wu [:mwu]]

Goes back to Gecko 32 when bug 994081 landed.

Comment 9

[Lawrence Mandel [:lmandel] (use needinfo)]

Thanks for the bug ref. Once this has been verified on m-c, let's get it uplifted to Beta and Aurora.

Comment 10

[Michael Wu [:mwu]]

To verify, use a Linux system/VM, and load freeciv from the link in bug 1081926 - http://play.freeciv.org/webclient/?action=load&load=tutorial&scenario=true . Watch Xorg in top (or your favorite process monitor) and make sure the memory usage doesn't jump several gigabytes when freeciv loads.

Comment 11

[Nicholas Nethercote [:njn]]

(In reply to Michael Wu [:mwu] from comment #10)
> To verify, use a Linux system/VM, and load freeciv from the link in bug
> 1081926 -
> http://play.freeciv.org/webclient/?action=load&load=tutorial&scenario=true .
> Watch Xorg in top (or your favorite process monitor) and make sure the
> memory usage doesn't jump several gigabytes when freeciv loads.

But note that it doesn't affect all systems. I was unable to reproduce on my Ubuntu 14.04 box.

Comment 12

[Michael Wu [:mwu]]

Xrender needs to be enabled/supported to trigger this bug.

Comment 13

[Michael Wu [:mwu]]

Verified as fixed by user on bug 1081926 comment 22 .

Comment 14

[Michael Wu [:mwu]]

Comment on attachment 8505586 [details] [diff] [review]
Avoid reoptimizing optimized SourceSurfaceCairos, v2

Approval Request Comment
[Feature/regressing bug #]: Regressed by bug 994081.
[User impact if declined]: Using the canvas API in certain ways will cause massive memory usage in the X server on GTK/X11 systems. It can often cause an X server OOM crash.
[Describe test coverage new/current, TBPL]: No tests, but testing this is somewhat difficult since it'll be made ineffective once bug 1081926 is fixed.
[Risks and why]: Low. I can't think of any risks.
[String/UUID change made/needed]: None.

Comment 15

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8505586 [details] [diff] [review]
Avoid reoptimizing optimized SourceSurfaceCairos, v2

Beta+
Aurora+

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/fdd0036a0ae5
https://hg.mozilla.org/releases/mozilla-beta/rev/5e3fc9d8a99b