Closed Bug 1130195 Opened 9 years ago Closed 8 years ago

crash in mozilla::gfx::DrawTargetD2D1::FinalizeDrawing(mozilla::gfx::CompositionOp, mozilla::gfx::Pattern const&)

Categories

(Core :: Graphics, defect)

Product:
Core
Component:
Graphics
Version:
36 Branch
Platform:
x86
Windows NT
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla43
Tracking Flags:
Tracking Status
firefox38 --- affected
firefox39 --- wontfix
firefox40 --- affected
firefox41 --- affected
firefox42 --- fixed
firefox43 --- fixed

People

(Reporter: alex_mayorga, Assigned: milan)

Details

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

Crash Data

Attachments

(2 files, 3 obsolete files)

Get some more info in the crash case. Carry r=bschouten
5.56 KB, patch
milan
: review+
milan
: checkin+
Details | Diff | Splinter Review
Don't immediately crash if we can't create bitmap. Carry r=bas
1.31 KB, patch
milan
: review+
Sylvestre
: approval-mozilla-aurora+
Details | Diff | Splinter Review 
This bug was filed from the Socorro interface and is 
report bp-c39707b9-ce2c-4753-9663-33abd2150205.
=============================================================
Component: General → Graphics
Product: Firefox → Core
(In reply to Benoit Girard (:BenWa) from comment #1)
> non sensitive urls:
> http://www.wralsportsfan.com/snow-no-unc-duke-game-will-be-played/14455312/
> (crash in canvas)
> http://www.moneycontrol.com/bestportfolio/wealth-management-tool/
> investments#port_top
> https://www.youtube.com/watch?v=CGk8x9VIfSo
> about:blank
> 
> :bas should we be checking the result here?
> http://hg.mozilla.org/releases/mozilla-beta/annotate/e3c1a6cbe4d3/gfx/2d/
> DrawTargetD2D1.cpp#l968

Yes, if we fail we probably still want to crash for now, but we'd like to have the return value in the gfxCriticalError log to get a better understanding of this.
Flags: needinfo?(bas)
To Milan who's been leading the gfxCriticalError push. Presumably this fail if the size is too large. I'm not sure how directly this is exposed to web content but IMO we should handle this failure better than a gfxCriticalError but it's probably a good start.
Flags: needinfo?(milan)
Assignee: nobody → milan
Flags: needinfo?(milan)
Comment on attachment 8569390 [details] [diff] [review]
Get some more info in the crash case. r=bschouten

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

::: gfx/2d/DrawTargetD2D1.cpp
@@ +965,5 @@
>      }
>  
>      RefPtr<ID2D1Bitmap> tmpBitmap;
> +    HRESULT hr = mDC->CreateBitmap(D2DIntSize(mSize), D2D1::BitmapProperties(D2DPixelFormat(mFormat)), byRef(tmpBitmap));
> +  if (FAILED(hr)) {

nit: indent
Attachment #8569390 - Flags: review?(bas) → review+
Fix the white space.
Attachment #8569390 - Attachment is obsolete: true
Attachment #8569482 - Flags: review+
No nightly crashes yet.  Bas, there is one 38 crash I looked at - (https://crash-stats.mozilla.com/report/index/a0046d11-e7d4-47a9-aed1-d115b2150221), what do you make out of:
D3D11-WARP? D3D11-WARP- D2D? D2D1.1? D2D1.1+ D2D+ DWrite? DWrite+ in the App Notes?

Does it mean we ended up in Basic + D2D1.1 (the "untested and slow") somehow?
Flags: needinfo?(bas)
Or things just got truncated and didn't show up in the App Notes...
While responding to this needinfo :)  Error D2DERR_RECREATE_TARGET shows up in few of these crashes, with reasonably sized bitmaps (https://crash-stats.mozilla.com/report/index/cf575a26-ce76-4391-966e-720182150223)
(In reply to Milan Sreckovic [:milan] from comment #10)
> No nightly crashes yet.  Bas, there is one 38 crash I looked at -
> (https://crash-stats.mozilla.com/report/index/a0046d11-e7d4-47a9-aed1-
> d115b2150221), what do you make out of:
> D3D11-WARP? D3D11-WARP- D2D? D2D1.1? D2D1.1+ D2D+ DWrite? DWrite+ in the App
> Notes?
> 
> Does it mean we ended up in Basic + D2D1.1 (the "untested and slow") somehow?

I don't know what to make of it, I'd at least expect a D3D11- in there :s.

(In reply to Milan Sreckovic [:milan] from comment #12)
> While responding to this needinfo :)  Error D2DERR_RECREATE_TARGET shows up
> in few of these crashes, with reasonably sized bitmaps
> (https://crash-stats.mozilla.com/report/index/cf575a26-ce76-4391-966e-
> 720182150223)

This is a symptom of a device reset.
Flags: needinfo?(bas)
Perhaps too early to tell, but something else may have fixed this problem in 39, as I'm not seeing any recent 39 crash reports.  Very few in 38, although there are some from March.
Bas, what do you think could have done it?  Whatever it was, it may be worth uplifting, though I don't see how we'd easily guess - it would need to have been something very early in 39.

In the meantime, should we leave this as is, replace it with MOZ_CRASH(), or wallpaper over it, by doing an early return?
Flags: needinfo?(bas)
We wouldn't be running into people's profiles with D2D1.1 "off" because of the changes to preferences we made along the way?
FWIW another crash from earlier today...

Report ID 	Date Submitted
bp-61470222-c43c-4c4a-9fb9-0c3f92150424
	24/04/2015	11:13 a.m.
So, there are some of these crashes in 40 as well.    This particular one https://crash-stats.mozilla.com/report/index/b1d83bb9-a636-4245-9504-149562150422, for example, fails to create a 30000x926 bitmap in DrawTargetD2D1::OptimizeSourceSurface (invalid argument), and eventually crashes failing to create a 2284x1093 bitmap in DrawTargetD2D1::FinalizeDrawing (out of memory)
Flags: needinfo?(bas)
Bas, do we still want to keep crashing in this scenario (comment 18)?
Flags: needinfo?(bas)
(In reply to Milan Sreckovic [:milan] from comment #19)
> Bas, do we still want to keep crashing in this scenario (comment 18)?

That sort of sounds like it might a 'real' OOM situation. In that case it would be good to crash. We could try and look at specifically the error code.
Flags: needinfo?(bas)
(In reply to Bas Schouten (:bas.schouten) from comment #20)
> (In reply to Milan Sreckovic [:milan] from comment #19)
> > Bas, do we still want to keep crashing in this scenario (comment 18)?
> 
> That sort of sounds like it might a 'real' OOM situation. In that case it
> would be good to crash. We could try and look at specifically the error code.

Meaning - only crash if we do get the OOM error code?  I'll make a patch for that, you can take a look.
Note that if this is an actual OOM, it would be great if we could annotate the allocation size, so that 1) we have that data on crashes and 2) our OOM crash detection can find those.
Comment on attachment 8598819 [details] [diff] [review]
Only purposely crash if OOM error code. Verbose (and likely unnecessary) nullptr checks. r=bschouten

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

The nullchecks should be useless, but who cares.

::: gfx/2d/DrawTargetD2D1.cpp
@@ +244,5 @@
>    }
>  
> +  if (!mTempBitmap) {
> +    gfxCriticalError() << "Invalid D3D1 tempBitmap1";
> +    PopClip();

I think you meant to add a return here.. otherwise that PopClip is not only useless, it's harmful since there's another popclip lower in this function and the pushes/pops will go unbalanced.
Attachment #8598819 - Flags: review?(bas) → review+
(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #23)
> Note that if this is an actual OOM, it would be great if we could annotate
> the allocation size, so that 1) we have that data on crashes and 2) our OOM
> crash detection can find those.

How do I do that?
Flags: needinfo?(kairo)
dmajor should know the details, forwarding to him.
Flags: needinfo?(kairo) → needinfo?(dmajor)
NS_ABORT_OOM(sizeInBytes);
Flags: needinfo?(dmajor)
Comment on attachment 8598819 [details] [diff] [review]
Only purposely crash if OOM error code. Verbose (and likely unnecessary) nullptr checks. r=bschouten

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

::: gfx/2d/DrawTargetD2D1.cpp
@@ +990,5 @@
> +            // For now, crash if OOM (by not returning and dereferencing
> +            // tmpBitmap), otherwise return and keep going.
> +            // E_OUTOFMEMORY is 0x8007000e.
> +            if (hr != E_OUTOFMEMORY) {
> +              return;

In the return scenario, should we do mDC->Flush() first?
Bas ^^
Flags: needinfo?(bas)
Let's see how this works out, but it's without the extra Flush from comment 28.
Comment on attachment 8598819 [details] [diff] [review]
Only purposely crash if OOM error code. Verbose (and likely unnecessary) nullptr checks. r=bschouten

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

::: gfx/2d/DrawTargetD2D1.cpp
@@ +990,5 @@
> +            // For now, crash if OOM (by not returning and dereferencing
> +            // tmpBitmap), otherwise return and keep going.
> +            // E_OUTOFMEMORY is 0x8007000e.
> +            if (hr != E_OUTOFMEMORY) {
> +              return;

It shouldn't matter at all.
Still a problem on 38.0b9

bp-3aca2f17-996c-447a-9ff7-c51352150505
	05/05/2015	01:00 p.m.
status-firefox38: --- → affected
(In reply to alex_mayorga from comment #32)
> Still a problem on 38.0b9
> 
> bp-3aca2f17-996c-447a-9ff7-c51352150505
> 	05/05/2015	01:00 p.m.

It "looks" to me that the crash occurs on accessing tmpBitmap that should have been created by a CreateBitmap call, which probably failed, leaving tmpBitmap null. Provided that's the issue, just checking the result and bailing out on failure would get rid of the crash, at least.
(In reply to Lee Salzman [:eihrul] from comment #33)
Yes, this is an OOM situation. Alex's machine is constrained on address space and thus sends us a lot of OOM reports. I believe the crux of the discussion so far has been whether to treat this as a "small OOM" (just set the OOM flag and crash; the process is so low on memory that fixing this would merely lead to a crash somewhere else) versus a "large OOM" (handle it gracefully because we might survive).

There's been discussion about how we could annotate the crash reports to decide the question, but really from the fact that Alex hits this repeatedly, my gut says it's a large OOM. Let's skip the annotation churn and just bail gracefully.
¡Hola!

This is in fact my significant other's laptop that was for unknown reasons to me setup with Windows 7 32-bit =(

Another crash FWIW...

bp-ef22dfb0-2637-46ba-b01c-8a7542150605
	05/06/2015	12:36 p.m.


Crashing Thread
Frame 	Module 	Signature 	Source
0 	xul.dll 	mozilla::gfx::DrawTargetD2D1::FinalizeDrawing(mozilla::gfx::CompositionOp, mozilla::gfx::Pattern const&) 	gfx/2d/DrawTargetD2D1.cpp
1 	xul.dll 	mozilla::gfx::DrawTargetD2D1::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/DrawTargetD2D1.cpp
2 	xul.dll 	gfxContext::Paint(double) 	gfx/thebes/gfxContext.cpp
3 	xul.dll 	mozilla::layers::PaintWithMask(gfxContext*, float, mozilla::layers::Layer*) 	gfx/layers/basic/BasicLayersImpl.cpp
4 	xul.dll 	mozilla::layers::BasicPaintedLayer::PaintThebes(gfxContext*, mozilla::layers::Layer*, void (*)(mozilla::layers::PaintedLayer*, gfxContext*, nsIntRegion const&, mozilla::layers::DrawRegionClip, nsIntRegion const&, void*), void*) 	gfx/layers/basic/BasicPaintedLayer.cpp
status-firefox39: --- → affected
Some of these actually report D2DERR_RECREATE_TARGET, rather than out of memory, but most are out of memory.  Some seem to small to be the large OOM, but let's see what happens if we do postpone the immediate crash.  We do everywhere else, this is one holdout where we proceed and crash.
This is a minimal subset of the reviewed patch.
Attachment #8598819 - Attachment is obsolete: true
Attachment #8646569 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/b4f66a22aba6
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
A random note; whether related to this or a number of other bugs, we do pass empty or negative sized rectangles to D2D1 functions.  I'll investigate this further, but something like that may very well not be properly handled by some drivers.
As expected, this got rid of this crash on nightly, though it still exists on other releases.  Anthony, can you figure out if there perhaps new (or spiking) graphics crashes in the past week, to see if this crash just moved to a different signature?
Flags: needinfo?(bas) → needinfo?(anthony.s.hughes)
I was unable to find any new or spiking signatures which might be related to this. I checked the explosiveness reports for Firefox 39, 40, 41, and 42 but nothing stands out. If this did move somewhere else it's not in any significant volume.
Flags: needinfo?(anthony.s.hughes)
Comment on attachment 8646569 [details] [diff] [review]
Don't immediately crash if we can't create bitmap.  Carry r=bas

Approval Request Comment
If we have large enough volume of crashes in Dev edition, it may be worth uplifting this.  I imagine we would, this has been around for a while.
Attachment #8646569 - Flags: approval-mozilla-aurora?
Comment on attachment 8646569 [details] [diff] [review]
Don't immediately crash if we can't create bitmap.  Carry r=bas

Fix a crash, taking it.
Attachment #8646569 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
No 42+ crashes from what I can tell since this change.
You need to log in before you can comment on or make changes to this bug.