1061696 - crash in IUnknown::QueryInterface<IDXGISurface>(IDXGISurface**)

Status: VERIFIED FIXED

(Core :: Graphics, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

This bug was filed from the Socorro interface and is 
report bp-20bd23c1-d46a-4a45-9fe7-28f402140901.
=============================================================

This is the 5th top crasher for users of builds attempting D3D11 layers.

Comment 1

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

It's the #8 topcrash for Firefox 34.0a2 users right now, with 109/5958 crashes in the last 7 days. More reports: https://crash-stats.mozilla.com/report/list?product=Firefox&range_value=7&range_unit=days&date=2014-09-09&signature=IUnknown%3A%3AQueryInterface%3CIDXGISurface%3E%28IDXGISurface**%29&version=Firefox%3A34.0a2#tab-reports


Crashing thread from https://crash-stats.mozilla.com/report/index/e0d981c0-24d8-4593-8bcc-0bb9d2140905:

 0 	xul.dll 	IUnknown::QueryInterface<IDXGISurface>(IDXGISurface**) 	c:/program files (x86)/windows kits/8.0/include/um/Unknwnbase.h:131
1 	xul.dll 	mozilla::gfx::AutoSaveRestoreClippedOut::Save() 	gfx/2d/DrawTargetD2D.cpp
2 	xul.dll 	mozilla::gfx::DrawTargetD2D::ClearRect(mozilla::gfx::RectTyped<mozilla::gfx::UnknownUnits> const&) 	gfx/2d/DrawTargetD2D.cpp
3 	xul.dll 	mozilla::layers::RotatedBuffer::DrawBufferQuadrant(mozilla::gfx::DrawTarget*, mozilla::layers::RotatedBuffer::XSide, mozilla::layers::RotatedBuffer::YSide, mozilla::layers::RotatedBuffer::ContextSource, float, mozilla::gfx::CompositionOp, mozilla::gfx::SourceSurface*, mozilla::gfx::Matrix const*)

Comment 2

[Robert Kaiser]

[Tracking Requested - why for this release]:
This is also one of our top issues on 33 beta and it's one of the GFX issues we need to solve in this cycle. It's the #4 crash on 33.0b right now.

Comment 3

[Nicolas Silva [:nical]]

Bas, it be fairly easy to have release builds early return in AutoSaveRestoreClippedOut::Save if allocating the temporary texture failed, instead of crashing. we'd risk having slightly broken rendering but I assume that it's better than crashing. Does this sound good to you?

Comment 4

[Bas Schouten (:bas.schouten)]

(In reply to Nicolas Silva [:nical] from comment #3)
> Bas, it be fairly easy to have release builds early return in
> AutoSaveRestoreClippedOut::Save if allocating the temporary texture failed,
> instead of crashing. we'd risk having slightly broken rendering but I assume
> that it's better than crashing. Does this sound good to you?

We sort of could.. but we're in a really bad situation if this happens anyway. The good thing is this code is going away with D2D 1.1, is there some correlation with driver versions/devices, etc.? Fwiw, this should only be happening to people with D2D, not for people with just D3D11, so this is even worse for them.

Comment 5

[Nicolas Silva [:nical]]

(In reply to Bas Schouten (:bas.schouten) from comment #4)
> (In reply to Nicolas Silva [:nical] from comment #3)
> > Bas, it be fairly easy to have release builds early return in
> > AutoSaveRestoreClippedOut::Save if allocating the temporary texture failed,
> > instead of crashing. we'd risk having slightly broken rendering but I assume
> > that it's better than crashing. Does this sound good to you?
> 
> We sort of could.. but we're in a really bad situation if this happens
> anyway. 

It's part of this big family of "failed to allocate a D3D texture" bugs that are causing crashes in many different places since we enabled OMTC, so either there is one thing we are doing badly and I hope to find out soon, or it's just that we effectlively use more texture memory than we used too (double buffering, etc.) and there is very little we can do except try to recover from them...

> The good thing is this code is going away with D2D 1.1,

Man, this is good news. It's a sad little code path, I hope we don't run into it too often already.

Is D2D1.1 going to be uplifted, though? If this is a top crasher we need something to fix it (or paper over it) in the mean time.

Comment 6

[Nicolas Silva [:nical]]

Attachment: Don't crash release builds when failing to allocate a surface in AutoRestoreClippedOut::save

Not ideal but if this code is going away thanks to D2D1.1 soon-ish, it's better than crashing in the mean time.

Comment 7

[Nicolas Silva [:nical]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fb65a6ab2fd6

Comment 8

[Ryan VanderMeulen [:RyanVM]]

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

Comment 9

[Sylvestre Ledru [:Sylvestre]]

Nical, could you fill an uplift request for aurora & beta? Merci!

Comment 10

[Nicolas Silva [:nical]]

Comment on attachment 8487871 [details] [diff] [review]
Don't crash release builds when failing to allocate a surface in AutoRestoreClippedOut::save

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: Crashes on Windows.
[Describe test coverage new/current, TBPL]: baking in central. No specific tests because the issue only happens with some gpu drivers.
[Risks and why]: very low, trivial patch.
[String/UUID change made/needed]:

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/de1eed9ed4c2
https://hg.mozilla.org/releases/mozilla-beta/rev/ff9cef7b2f9d

Comment 13

[Tracy Walker [:tracy]]

Crash stats indicate this is fixed across branches.

Comment 14

[Bogdan Maris, Desktop Test Engineering]

Marking as verified since it was verified on target milestone build.