987054 - Skia Canvas leak

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Jesse Ruderman]

Attachment: testcase

1. Set:
     user_pref("gfx.canvas.azure.backends", "skia");
2. Run a debug build with XPCOM_MEM_LEAK_LOG=2
3. Load the testcase
4. Quit

>== BloatView: ALL (cumulative) LEAK STATISTICS, default process 3643
>
>     |<----------------Class--------------->|<-----Bytes------>|<----------------Objects---------------->|<--------------References-------------->|
>                                              Per-Inst   Leaked    Total      Rem      Mean       StdDev     Total      Rem      Mean       StdDev
>   0 TOTAL                                          40      288   535813        4 ( 5500.67 +/-  8438.68)  1213051        7 ( 3796.25 +/-  6964.68)
> 101 DataSourceSurfaceSkia                         120      120        1        1 (    1.00 +/-     0.00)        8        2 (    2.50 +/-     0.94)
> 102 DataSourceSurfaceWrapper                       24       48        3        2 (    1.00 +/-     0.82)        6        2 (    1.50 +/-     0.85)
> 118 DrawTargetSkia                                120      120        1        1 (    1.00 +/-     0.00)        9        3 (    2.27 +/-     1.28)

Seems to be a regression from about 2 days ago.

Comment 1

[Andreas Gal :gal]

We need to do this on our automation.

Comment 2

[Jeff Muizelaar [:jrmuizel]]

Perhaps caused by the Skia update?

Comment 3

[Milan Sreckovic [:milan] (needinfo for best results)]

George, let's verify if this is from Skia update, if so, please fix "in a day" or back it out.

Comment 4

[George Wright (:gw280) (needinfo me!)]

Investigating today.

Comment 5

[Milan Sreckovic [:milan] (needinfo for best results)]

I don't know how much the fix to bug 985773 changes the timing of destruction, but it may be worth taking the patch when testing this?  Probably wouldn't make any difference...

Comment 6

[George Wright (:gw280) (needinfo me!)]

(In reply to Milan Sreckovic [:milan] from comment #5)
> I don't know how much the fix to bug 985773 changes the timing of
> destruction, but it may be worth taking the patch when testing this? 
> Probably wouldn't make any difference...

I don't think it'll matter because this isn't accelerated canvas, and that only affects GL-backed stuff, but I will check anyway.

Comment 7

[George Wright (:gw280) (needinfo me!)]

Attachment: 0001-Bug-987054-If-we-can-t-create-a-valid-DrawTarget-in-.patch

It looks like what's happening here is that CopyableCanvasLayer is creating a wrapping gfxImageSurface on the DrawTarget snapshot, but in the case where width|height > 32768, cairo refuses to create it, and the gfxASurface is marked invalid. This means that the DrawTarget is still manually reffed in gfxPlatform.cpp:933, but the balancing deref from the SetData is never called, as SetData bails early as the gfxASurface is invalid.

Not sure what the best thing to do here is to handle the failure of creation, but we definitely should be checking CairoStatus() to error out.

Opinions, Matt?

Comment 8

[George Wright (:gw280) (needinfo me!)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8a910903da40

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8a910903da40