Avoid reoptimizing optimized SourceSurfaceCairos

VERIFIED FIXED in Firefox 34

Status

()

defect
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: mwu, Assigned: mwu)

Tracking

unspecified
mozilla36
All
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox33 wontfix, firefox34+ fixed, firefox35+ fixed, firefox36 fixed, firefox-esr31 unaffected, b2g-v1.4 unaffected, b2g-v2.0 unaffected, b2g-v2.0M unaffected, b2g-v2.1 unaffected, b2g-v2.2 unaffected)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(1 attachment, 1 obsolete attachment)

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.
Attachment #8504923 - Flags: review?(bas)
Whiteboard: [MemShrink]
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.
Whiteboard: [MemShrink] → [MemShrink:P2]
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.
Attachment #8504923 - Flags: review?(bas) → review+
The check is now generic.
Attachment #8504923 - Attachment is obsolete: true
Hardware: x86_64 → All
[Tracking Requested - why for this release]:
This fixes severe leaks for certain canvas users (like freeciv) on GTK/X11.
https://hg.mozilla.org/mozilla-central/rev/f013910bbead
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
(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?
Flags: needinfo?(mwu)
Goes back to Gecko 32 when bug 994081 landed.
Flags: needinfo?(mwu)
Thanks for the bug ref. Once this has been verified on m-c, let's get it uplifted to Beta and Aurora.
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.
Keywords: verifyme
(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.
Xrender needs to be enabled/supported to trigger this bug.
Verified as fixed by user on bug 1081926 comment 22 .
Status: RESOLVED → VERIFIED
Keywords: verifyme
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.
Attachment #8505586 - Flags: approval-mozilla-beta?
Attachment #8505586 - Flags: approval-mozilla-aurora?
Comment on attachment 8505586 [details] [diff] [review]
Avoid reoptimizing optimized SourceSurfaceCairos, v2

Beta+
Aurora+
Attachment #8505586 - Flags: approval-mozilla-beta?
Attachment #8505586 - Flags: approval-mozilla-beta+
Attachment #8505586 - Flags: approval-mozilla-aurora?
Attachment #8505586 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.