931331 - Skia: Double free with fillText on huge canvas

Status: RESOLVED WORKSFORME

(Core :: Graphics, defect)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox)

With:
  user_pref("layers.use-deprecated-textures", true);
  user_pref("gfx.canvas.azure.backends", "skia");

the testcase causes a double-free.

Nightly: bp-7e72bff0-3a71-4293-8474-55edf2131026

Comment 1

[Jesse Ruderman]

Attachment: stacks (ASan)

Comment 2

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

Not a default configuration, so not a priority, but let's try to get ahead of it.

Comment 3

[Al Billings [:abillings - ex-MoCo]]

Is this pref'd on for any releases or platforms?

Comment 4

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

This preference is on by default on Android and B2G.  The crash, the stack and the bug are in specific code OS X.  So, you're right, let's verify that this is not a problem on Android or B2G.  Can we do ASAN builds for those platforms?

Comment 5

[Al Billings [:abillings - ex-MoCo]]

I don't think we can make Android ASAN builds (or B2G).

Comment 6

[Matt Wobensmith [:mwobensmith][:matt:]]

Can't reproduce on Android Fennec 25.0.

Still working to test on B2G.

Neither of which are ASan builds, of course.

Comment 7

[Daniel Veditz [:dveditz]]

The original crash didn't look like an ASan build, pretty sure they don't report to socorro. mwobensmith did not crash on Android (non-asan) but has not yet tried b2g.

Are we planning on using Skia on Mac in the future? If not and this ends up only being a problem on Mac with non-default prefs and nowhere else then we can lower the severity. Even though we are crashing deep in the OS it's during a release in Firefox, and it's possible we've abused the OS object in some way.

Comment 8

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

We are planning on using Skia on the Mac, though not with "deprecated textures".  So that we don't lose this one, let's keep the severity high for a bit longer?

Comment 9

[Al Billings [:abillings - ex-MoCo]]

Critsmash triage is making this a sec-high because it isn't on everywhere yet.

Comment 10

[Andrew McCreight [:mccr8]]

George, is Skia still disabled for OSX?  Is this something somebody should look at in the near term?

Comment 11

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

Skia is still non-default.  Bug 932958 would make the switch.  On the roadmap, but not scheduled yet.

Comment 12

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

Yes, we'd like to be able to turn it on. When I'm back from travelling I will look at whether this is still reproducible with the current version of Skia in-tree.

Comment 13

[Sylvestre Ledru [:Sylvestre]]

George, I guess you are not planning to turn it on for 29. Right?

Comment 14

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

No, we're not going to make this the default in 29, or 30 or 31, for that matter.

Comment 15

[Sylvestre Ledru [:Sylvestre]]

OK. Thanks. So, updating the tracking flags + status accordingly.
Please set the tracking flag back on the release in which Skia is going to be enabled for Mac OS X (and obviously, if that bug is not yet fixed).

Comment 16

[Al Billings [:abillings - ex-MoCo]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #15)
> OK. Thanks. So, updating the tracking flags + status accordingly.
> Please set the tracking flag back on the release in which Skia is going to
> be enabled for Mac OS X (and obviously, if that bug is not yet fixed).

Why explicitly minus this for releases? Disabled or not, it is a high rated security issue and we should take a fix if it is available.

Comment 17

[Sylvestre Ledru [:Sylvestre]]

That meant, to me, that we won't block the release of 29 because of this bug. But I will be happy to uplift any patch.

Comment 18

[Al Billings [:abillings - ex-MoCo]]

A minus on tracking is an explicit decision to *not* take a patch on a release.

Comment 19

[Sylvestre Ledru [:Sylvestre]]

No, it is not ;) We accept uplift requests even if they are not tracked.

Comment 20

[Al Billings [:abillings - ex-MoCo]]

Actually it is, based on six years of experience. We always will accept requests but an explicit minus means we've turned it away. Perhaps you're new here?

Comment 21

[Al Billings [:abillings - ex-MoCo]]

(That was meant with a smiley but, seriously, don't minus things unless you're explicitly rejecting them.)

Comment 22

[Lukas Blakk [:lsblakk] use ?needinfo]

(In reply to Al Billings [:abillings] from comment #20)
> Actually it is, based on six years of experience. We always will accept
> requests but an explicit minus means we've turned it away. Perhaps you're
> new here?

It's not Al, we take uplift nominations all the time on bugs that are not tracked.  We can discuss this further off-bug if you like.

Comment 23

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

Ignoring the deprecated textures part, and using Steven Michaud's ASAN build from 2014/06/23 (http://people.mozilla.org/~stmichaud/bmo/firefox-asan-howto.txt), no crash on this test case on 10.9.  We do have a newer Skia than when this first showed up.  Jesse, do you still see this in 33?

Comment 24

[Jesse Ruderman]

WFM on trunk

Comment 25

[James Willcox (:snorp) ([e-mail]) (he/him)]

We don't use deprecated textures on Mac, do we? Do those even exist anymore?