1208234 - crash in mozilla::gfx::SourceSurfaceD2DTarget::GetDataSurface()

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[[:philipp]]

This bug was filed from the Socorro interface and is 
report bp-07ec21c3-c988-4b1b-9f4d-5c8b02150924.
=============================================================
Crashing Thread
Frame 	Module 	Signature 	Source
0 	xul.dll 	mozilla::gfx::SourceSurfaceD2DTarget::GetDataSurface() 	gfx/2d/SourceSurfaceD2DTarget.cpp
1 	xul.dll 	mozilla::gfx::GetCairoSurfaceForSourceSurface(mozilla::gfx::SourceSurface*, bool, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&) 	gfx/2d/DrawTargetCairo.cpp
2 	xul.dll 	mozilla::gfx::GfxPatternToCairoPattern 	gfx/2d/DrawTargetCairo.cpp
3 	xul.dll 	mozilla::gfx::DrawTargetCairo::DrawPattern(mozilla::gfx::Pattern const&, mozilla::gfx::StrokeOptions const&, mozilla::gfx::DrawOptions const&, mozilla::gfx::DrawTargetCairo::DrawPatternType, bool) 	gfx/2d/DrawTargetCairo.cpp

this graphics crash seems to have been introduced in 42.0a1 and subsequent builds - it's on rank #16 in the 42.0b1 crash list at them moment. a broad range of devices from all major gpu vendors are affected.

https://crash-stats.mozilla.com/search/?signature=%3Dmozilla%3A%3Agfx%3A%3ASourceSurfaceD2DTarget%3A%3AGetDataSurface%28%29&date=%3E2015-01-01&_facets=signature&_facets=version&_facets=adapter_vendor_id&_facets=adapter_device_id&_facets=adapter_driver_version&_facets=app_notes&_facets=user_comments&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#facet-version

Comment 1

[u279076]

Based on build IDs it looks like this *may* have started with the July 30, 2015 build which assumes the following pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2ee9895e032c&tochange=62469b20ec84

Comment 2

[Sylvestre Ledru [:Sylvestre] back june 18th]

Tracking as it is an important crash in 42.

Comment 3

[Sylvestre Ledru [:Sylvestre] back june 18th]

Milan, do you know who could help with this? Thanks

Comment 4

[Bas Schouten (:bas.schouten)]

The most supicious bug in the pushlog is bug 1183910.

Comment 5

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

We should be able to just patch the null pointer access, if the stack is to be trusted - David, could this be a result of the "things are getting mixed up with D2D and unaccelerated" you dealt with recently?

Comment 6

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

Attachment: Stop the nullptr dereference. r=bschouten

Comment 7

[David Anderson [:dvander] - inactive, e-mail if emergency]

Yeah, I bet it is exactly that. We used to not clear the ID3D10Device on TDRs, which bug 1183910 changed. We may have gotten a TDR, removed the factory, and fallen back to software. Then if we still had a 2D draw target lying around it might now assume we're still accelerated and still have an ID3D10 device.

Too bad we don't have more stack here, since I bet some cache is not being invalidated on TDRs and that's the real bug. But a null check on the d3d10 should be there anyway.

Comment 8

[Bas Schouten (:bas.schouten)]

Comment on attachment 8671538 [details] [diff] [review]
Stop the nullptr dereference. r=bschouten

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

::: gfx/2d/SourceSurfaceD2DTarget.cpp
@@ +64,5 @@
>    desc.BindFlags = 0;
>    desc.MiscFlags = 0;
>  
> +  if (!Factory::GetDirect3D10Device()) {
> +    gfxCriticalError() << "Invalid D3D10 device in D2D target surface";

nit: No D3D10 device assigned to factory.

Would be a more correct message.

Comment 9

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

checkin-needed without a try run; we're replacing a nullptr dereference, but let me know if you want a run.

Comment 10

[David Anderson [:dvander] - inactive, e-mail if emergency]

There are some more worrisome cases here: in gfx/2d/SourceSurfaceD2DTarget.cpp

Comment 11

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

Good one: https://crash-stats.mozilla.com/report/index/80fe1b41-08fb-46f1-98a0-7a5fd2151008 suggests we lost D3D11 compositor

Comment 12

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

Leave open after we land the patch above, just to see what happens to the crashes.

Comment 13

[Pulsebot]

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

Comment 14

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

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

Comment 15

[Bas Schouten (:bas.schouten)]

Comment on attachment 8671538 [details] [diff] [review]
Stop the nullptr dereference. r=bschouten

Approval Request Comment
[Feature/regressing bug #]: Suspected bug 1183910
[User impact if declined]: Crash
[Describe test coverage new/current, TreeHerder]: None, code not used anymore on trunk
[Risks and why]: Low, nullpointer check, may change crash into some artifacting
[String/UUID change made/needed]: None

Comment 16

[Carsten Book [:Tomcat]]

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

Comment 17

[Sylvestre Ledru [:Sylvestre] back june 18th]

Comment on attachment 8671538 [details] [diff] [review]
Stop the nullptr dereference. r=bschouten

Fix a crash, taking it. Should be in 38 beta 6.

Comment 18

[Sylvestre Ledru [:Sylvestre] back june 18th]

In the previous comment s/38/42/
Looks like I was traumatized by the 38 cycle :p

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/96980e16feb0

Comment 20

[Carsten Book [:Tomcat]]

 https://hg.mozilla.org/releases/mozilla-aurora/rev/2c0e8f1cdb03

Comment 21

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

The crashes with this particular signature look appears to have stopped - was there a spike elsewhere?

Comment 22

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

(In reply to Milan Sreckovic [:milan] from comment #21)
> The crashes with this particular signature look appears to have stopped -
> was there a spike elsewhere?

Perhaps in OOMs?

Comment 23

[u279076]

We're still seeing reports of this crash but it's at really low volume and all on unsupported Firefox versions (14 reports last week, 100% in Firefox Beta 42). For this reason I'm closing this bug as fixed.

Comment 24

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.