1331274 - Crash in mozilla::gfx::DrawTargetD2D1::Fill

Status: RESOLVED FIXED

(Core :: Graphics, defect, P2)

Description

[[:philipp]]

This bug was filed from the Socorro interface and is 
report bp-3fe1309f-a10c-4281-bccf-ec8912170115.
=============================================================
Crashing Thread (0)
Frame 	Module 	Signature 	Source
0 	xul.dll 	mozilla::gfx::DrawTargetD2D1::Fill(mozilla::gfx::Path const*, mozilla::gfx::Pattern const&, mozilla::gfx::DrawOptions const&) 	gfx/2d/DrawTargetD2D1.cpp:503
1 	xul.dll 	mozilla::dom::CanvasRenderingContext2D::Fill(mozilla::dom::CanvasWindingRule const&) 	dom/canvas/CanvasRenderingContext2D.cpp:3107
2 	xul.dll 	mozilla::dom::CanvasRenderingContext2DBinding::fill 	obj-firefox/dom/bindings/CanvasRenderingContext2DBinding.cpp:3348
3 	xul.dll 	mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) 	dom/bindings/BindingUtils.cpp:2812
4 		@0x1dfc8937 	
5 		@0x3deb45cf 	
6 		@0x181fbff0

crashes with this signature are rising on the beta channel since 51.0b12. they are affecting windows 7 & upwards and are accounting for around 0.1% of crashes on beta now: https://crash-stats.mozilla.com/signature/?release_channel=beta&signature=mozilla%3A%3Agfx%3A%3ADrawTargetD2D1%3A%3AFill&date=>%3D2016-12-15T20%3A31%3A50.000Z#graphs

this would be the changelog between 51.0b11 and beta 12: https://hg.mozilla.org/releases/mozilla-beta/pushloghtml?fromchange=FIREFOX_51_0b11_RELEASE&tochange=FIREFOX_51_0b12_RELEASE

Comment 1

[Gerry Chang [:gchang]]

Hi Peter,
Can you help take a look at this one? Thanks.

Comment 2

[Peter Chang[:pchang]]

Kevin, could you take a look to see what else we can do here? I think this one is related to bug 1318283.

Comment 3

[Kevin Chen[:kechen] (UTC + 8)]

After a brief investigation, we get null pointer for aPath in[1], which is weird since we've checked the validation of the pointer in[2]. It might be dereferenced by other thread somewhere.

Also the crash volume increased in beta channel after firefox 51.0b12 which indicates the root cause might be the fixes between 51.0b11 and 51.0b12.


[1] https://hg.mozilla.org/releases/mozilla-beta/annotate/09142d07fd73/gfx/2d/DrawTargetD2D1.cpp#l503
[2] https://hg.mozilla.org/releases/mozilla-beta/annotate/09142d07fd73/dom/canvas/CanvasRenderingContext2D.cpp#l3097
[3] https://hg.mozilla.org/releases/mozilla-beta/pushloghtml?fromchange=0a17d39220700e742bf37a960967480b2f8159f1&tochange=9ddd4fee07842e72ba49f1583ec5f596f6e60e72

Comment 4

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

NeedToCalculateBounds() may cause ClearTarget() to get called, which resets the path.  Bug 1318283 is sort of related, and a patch on bug 1329796 may also be necessary, but it may be enough here to check for valid aPath in the call.

Comment 5

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

Oh, and I believe this is going up because bug 1318283 patch removed an earlier crash.

Comment 6

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

If you end up modifying DrawTargetD2D1, please also make the equivalent change in DrawTargetSkia.

Comment 7

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

Attachment: Bug 1331274: aPath coming into Fill method may not be valid.

Review commit: https://reviewboard.mozilla.org/r/104966/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/104966/

Comment 8

[Kevin Chen[:kechen] (UTC + 8)]

Comment on attachment 8827221 [details]
Bug 1331274: aPath coming into Fill method may not be valid.

https://reviewboard.mozilla.org/r/104966/#review105872

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b7dfb6794455
aPath coming into Fill method may not be valid. r=kechen

Comment 10

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

Comment on attachment 8827221 [details]
Bug 1331274: aPath coming into Fill method may not be valid.

Approval Request Comment
[Feature/Bug causing the regression]: Most likely 1298552
[User impact if declined]: High volume crash in beta, aurora
[Is the change risky?]: Null pointer check, low risk
[Why is the change risky/not risky?]: The callers are fine with the early return.

Comment 11

[Julien Cristau [:jcristau]]

Comment on attachment 8827221 [details]
Bug 1331274: aPath coming into Fill method may not be valid.

crash fix for aurora52

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/595fc6dd4397

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

Not a high volume crash from this one signature, is it that it's showing up in others or we think it might be related to bug 1318283 ? We could still get this into 51 RC2 but it's very last minute.

Comment 15

[Liz Henry (:lizzard) (relman/hg->git project)]

Can you also request uplift to m-r?

Comment 16

[Peter Chang[:pchang]]

Comment on attachment 8827221 [details]
Bug 1331274: aPath coming into Fill method may not be valid.

Approval Request Comment
[Feature/Bug causing the regression]: Most likely 1298552
[User impact if declined]: High volume crash in beta, aurora
[Is the change risky?]: Null pointer check, low risk
[Why is the change risky/not risky?]: The callers are fine with the early return.

Comment 17

[Gerry Chang [:gchang]]

Comment on attachment 8827221 [details]
Bug 1331274: aPath coming into Fill method may not be valid.

Fix a crash. I would like to take this in 51 RC2.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/dd4b8065a9a9

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/dd4b8065a9a9