Closed Bug 1123080 Opened 5 years ago Closed 5 years ago

crash in _cairo_d2d_get_temp_rt called from CanvasRenderingContext2D::DrawImage, almost exclusively on Intel cards

Categories

(Core :: Canvas: 2D, defect, critical)

x86
Windows NT
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox36 --- wontfix
firefox37 + fixed
firefox38 --- fixed
firefox39 --- fixed

People

(Reporter: kairo, Assigned: nical)

Details

(Keywords: crash, Whiteboard: [gfx-noted])

Crash Data

Attachments

(2 files, 2 obsolete files)

This bug was filed from the Socorro interface and is 
report bp-04a19bdd-a9cf-4284-9e13-4f4a62150111.
=============================================================

Top frames:
0 	gkmedias.dll 	_cairo_d2d_get_temp_rt 	gfx/cairo/cairo/src/cairo-d2d-surface.cpp
1 	gkmedias.dll 	_cairo_d2d_paint 	gfx/cairo/cairo/src/cairo-d2d-surface.cpp
2 	gkmedias.dll 	_cairo_surface_paint 	gfx/cairo/cairo/src/cairo-surface.c
3 	gkmedias.dll 	_cairo_gstate_paint 	gfx/cairo/cairo/src/cairo-gstate.c
4 	gkmedias.dll 	_moz_cairo_paint 	gfx/cairo/cairo/src/cairo.c
5 	gkmedias.dll 	_moz_cairo_paint_with_alpha 	gfx/cairo/cairo/src/cairo.c
6 	xul.dll 	mozilla::gfx::DrawTargetCairo::DrawSurface(mozilla::gfx::SourceSurface*, mozilla::gfx::RectTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::RectTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::DrawSurfaceOptions const&, mozilla::gfx::DrawOptions const&) 	gfx/2d/DrawTargetCairo.cpp
7 	xul.dll 	mozilla::dom::CanvasRenderingContext2D::DrawImage(mozilla::dom::HTMLImageElementOrHTMLCanvasElementOrHTMLVideoElement const&, double, double, double, double, double, double, double, double, unsigned char, mozilla::ErrorResult&) 	dom/canvas/CanvasRenderingContext2D.cpp
8 	xul.dll 	mozilla::dom::CanvasRenderingContext2D::DrawImage(mozilla::dom::HTMLImageElementOrHTMLCanvasElementOrHTMLVideoElement const&, double, double, mozilla::ErrorResult&) 	dom/canvas/CanvasRenderingContext2D.h
9 	xul.dll 	mozilla::dom::CanvasRenderingContext2DBinding::drawImage 	obj-firefox/dom/bindings/CanvasRenderingContext2DBinding.cpp
10 	xul.dll 	mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) 	dom/bindings/BindingUtils.cpp
[...]

This is being called from JITed code apparently.

This is happening on all kinds of Firefox versions, almost exclusively on Intel graphics but on different adapters and drivers from 8.15.10.2291 through 9.17.10.3347, all addresses are 0x0 so it might be a null deref of some kind.

Find more reports and stats at https://crash-stats.mozilla.com/report/list?signature=_cairo_d2d_get_temp_rt and/or https://crash-stats.mozilla.com/signature/?signature=_cairo_d2d_get_temp_rt
Intel released new drivers this month, if anyone is using the Intel products below, try updating your drivers.

https://communities.intel.com/thread/59216

10.18.10.4061 for Ivy Bridge & Baytrail.

https://communities.intel.com/thread/59216

10.18.10.4080 for Haswell & Broadwell.
(In reply to GMA from comment #1)
> Intel released new drivers this month, if anyone is using the Intel products
> below, try updating your drivers.
> 
> https://communities.intel.com/thread/59216
> 
> 10.18.10.4061 for Ivy Bridge & Baytrail.
> 
> https://communities.intel.com/thread/59216
> 
> 10.18.10.4080 for Haswell & Broadwell.

As I said above, I do not not see any 10.* drivers in the crash reports at all, I only see versions up to 9.17.10.3347 there.
I need to look at this.
Flags: needinfo?(bas)
Whiteboard: [gfx-noted]
I don't understand how we're getting a D2D cairo surface here. Still investigating.
Flags: needinfo?(bas)
Bas are you still looking at this?

[Tracking Requested - why for this release]:
The 36 release is affected, about as much as 35 (1%) so I don't think this is worth tracking for a dot release. But it would be nice to get this for 37. Status of 38 is unknown, not enough data.
Flags: needinfo?(bas)
Not really actively looking, but I still think it's very bad that this happens!
Flags: needinfo?(bas)
I'm going to track this crash based on David's request. However, we have a shorter 37 Beta cycle. If this doesn't get an owner soon, this is unlikely to be fixed in 37.
(In reply to Bas Schouten (:bas.schouten) from comment #4)
> I don't understand how we're getting a D2D cairo surface here. Still
> investigating.

Looks like we fail to create the D2D DrawTarget for whatever reason here and go into the fallback target path here:
https://hg.mozilla.org/mozilla-central/file/3da36cdb4198/gfx/thebes/gfxPlatform.cpp#l1176

Then we use CreateDrawTargetForSurface here:
https://hg.mozilla.org/mozilla-central/file/3da36cdb4198/gfx/thebes/gfxPlatform.cpp#l1160

And the Surface we passed was created here:
https://hg.mozilla.org/mozilla-central/file/3da36cdb4198/gfx/thebes/gfxWindowsPlatform.cpp#l781


I guess the logic in CreateOffscreenSurface dates from when we used D2D through thebes.
We can probably simplify this now and remove the gfxD2DSurface stuff (what about the gdi surface?).
Actually, do we even need the gfxD2DSurface class at all in the code base these days?
Flags: needinfo?(bas)
Assignee: nobody → nical.bugzilla
question answered in a meeting: we should not need gfxD2DSurface anymore.
Flags: needinfo?(bas)
Attached patch don't use gfxD2DSurfaces (obsolete) — Splinter Review
I'll remove gfxD2DSurface entirely in a followup patch but this one is small and simple to uplift.
Attachment #8575361 - Flags: review?(bas)
Comment on attachment 8575361 [details] [diff] [review]
don't use gfxD2DSurfaces

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

::: gfx/thebes/gfxWindowsPlatform.cpp
@@ -771,5 @@
>      nsRefPtr<gfxASurface> surf = nullptr;
>  
>  #ifdef CAIRO_HAS_WIN32_SURFACE
>      if (mRenderMode == RENDER_GDI)
>          surf = new gfxWindowsSurface(ThebesIntSize(size),

Should we remove this one too?
Attachment #8575366 - Flags: review?(bas)
Comment on attachment 8575366 [details] [diff] [review]
Remove gfxD2DSurface entirely

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

The Plugin code appears to still be relying on thebes wrapped D2D surfaces: https://dxr.mozilla.org/mozilla-central/source/dom/plugins/ipc/PluginInstanceChild.cpp#2725
See try failures: https://hg.mozilla.org/try/rev/482b907fcbbd
Attachment #8575366 - Flags: review?(bas)
This path is less likely to interact in bad ways with other parts of the code like plugins.
Attachment #8575361 - Attachment is obsolete: true
Attachment #8575361 - Flags: review?(bas)
Attachment #8575902 - Flags: review?(bas)
Attachment #8575902 - Flags: review?(bas) → review+
Comment on attachment 8575902 [details] [diff] [review]
use cairo's image backend for canvas fallback

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: crashes in cairo's d2d backend
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: low risk, very simple patch
[String/UUID change made/needed]:
Attachment #8575902 - Flags: approval-mozilla-beta?
Attachment #8575902 - Flags: approval-mozilla-aurora?
I agree we should uplift, and like the fix, but the patch is not in fact simple, even though the lines of code change is small :)
(In reply to Milan Sreckovic [:milan] from comment #17)
> I agree we should uplift, and like the fix, but the patch is not in fact
> simple, even though the lines of code change is small :)

Agreed it's simple as "simple to back out" more than "doesn't have implications", but since it is focused on canvas2d fallback, the area of code that this can break is pretty small compared to the a lot of the things that we have been uplifting lately.
We have a couple of choices with this bug:

1. We can uplift the bug for Beta 5 and verify with crash data that this crash is indeed fixed. This is the easier but riskier approach.
2. We can uplift to Aurora tomorrow (after this has successfully stuck on Nightly), where we do have some crash volume, test for a few days until gtb for Beta 6 on Monday to see if the crash volume drops, and then uplift to Beta.

Milan - How concerned are you about the potential fallout from this patch? Would you prefer to go direct to Beta or take the Aurora step first?
Flags: needinfo?(milan)
Let's go with #1, based on what Nical is saying. We'll also get a Betabreakers test for canvas 2d.
Flags: needinfo?(milan)
Not sure why we started hitting it, but Nical, I'd just change https://dxr.mozilla.org/mozilla-central/source/gfx/2d/DrawTargetCairo.cpp#1525 to have the CriticalLog::DefaultOptions(Factory::ReasonableSurfaceSize(aSize)) as an argument to gfxCriticalError that caused this crash.
nical - Are you going to have another patch ready for Beta 5 gtb today? (Need the patch in a few hours.)
Flags: needinfo?(nical.bugzilla)
(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #23)
> nical - Are you going to have another patch ready for Beta 5 gtb today?
> (Need the patch in a few hours.)

Yes, I have one and I was waiting for a green try push https://treeherder.mozilla.org/#/jobs?repo=try&revision=187f0c801cff but win7 tests are taking forever.
Flags: needinfo?(nical.bugzilla)
Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: crashes
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: rather low, affects the fallback path of canvas2d only and makes it use cairo's image backend which we understand, test and control more than cairo's d2d backend.
[String/UUID change made/needed]:
Attachment #8575902 - Attachment is obsolete: true
Attachment #8575902 - Flags: approval-mozilla-beta?
Attachment #8575902 - Flags: approval-mozilla-aurora?
Attachment #8576733 - Flags: approval-mozilla-beta?
Attachment #8576733 - Flags: approval-mozilla-aurora?
Comment on attachment 8576733 [details] [diff] [review]
use cairo's image backend for canvas fallback on windows carrying r=Bas

OK. Let's try this again. Beta+ Aurora+
Attachment #8576733 - Flags: approval-mozilla-beta?
Attachment #8576733 - Flags: approval-mozilla-beta+
Attachment #8576733 - Flags: approval-mozilla-aurora?
Attachment #8576733 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/a8bf12896244
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.