1016440 - crash in gfxContext::gfxContext(mozilla::gfx::DrawTarget*)

Status: VERIFIED FIXED

(Core :: Graphics, defect)

Description

[u279076]

+++ This bug was initially created as a clone of Bug #952721 +++

This bug was filed from the Socorro interface and is 
report
bp-b1c59df7-f542-4b54-8a35-0bc382140526
=============================================================
0 	xul.dll 	gfxContext::gfxContext(mozilla::gfx::DrawTarget *,mozilla::gfx::PointTyped<mozilla::gfx::UnknownUnits> const &) 	gfx/thebes/gfxContext.cpp
1 	xul.dll 	mozilla::image::ClippedImage::GetFrameInternal(nsIntSize const &,mozilla::SVGImageContext const *,unsigned int,unsigned int) 	image/src/ClippedImage.cpp
2 	xul.dll 	mozilla::image::ClippedImage::Draw(gfxContext *,GraphicsFilter,gfxMatrix const &,gfxRect const &,nsIntRect const &,nsIntSize const &,mozilla::SVGImageContext const *,unsigned int,unsigned int) 	image/src/ClippedImage.cpp
3 	xul.dll 	DrawImageInternal 	layout/base/nsLayoutUtils.cpp
4 	xul.dll 	nsLayoutUtils::DrawImage(nsRenderingContext *,imgIContainer *,GraphicsFilter,nsRect const &,nsRect const &,nsPoint const &,nsRect const &,unsigned int) 	layout/base/nsLayoutUtils.cpp
5 	xul.dll 	nsImageRenderer::DrawBorderImageComponent(nsPresContext *,nsRenderingContext &,nsRect const &,nsRect const &,mozilla::gfx::IntRectTyped<mozilla::CSSPixel> const &,unsigned char,unsigned char,nsSize const &,unsigned char) 	layout/base/nsCSSRendering.cpp
6 	xul.dll 	DrawBorderImage 	layout/base/nsCSSRendering.cpp
7 	xul.dll 	nsCSSRendering::PaintBorderWithStyleBorder(nsPresContext *,nsRenderingContext &,nsIFrame *,nsRect const &,nsRect const &,nsStyleBorder const &,nsStyleContext *,int) 	layout/base/nsCSSRendering.cpp
8 	xul.dll 	nsCSSRendering::PaintBorder(nsPresContext *,nsRenderingContext &,nsIFrame *,nsRect const &,nsRect const &,nsStyleContext *,int) 	layout/base/nsCSSRendering.cpp
9 	xul.dll 	nsDisplayBorder::Paint(nsDisplayListBuilder *,nsRenderingContext *) 	layout/base/nsDisplayList.cpp
10 	xul.dll 	mozilla::FrameLayerBuilder::PaintItems(nsTArray<mozilla::FrameLayerBuilder::ClippedDisplayItem> &,nsIntRect const &,gfxContext *,nsRenderingContext *,nsDisplayListBuilder *,nsPresContext *,nsIntPoint const &,float,float,int) 	layout/base/FrameLayerBuilder.cpp
11 	xul.dll 	mozilla::FrameLayerBuilder::DrawThebesLayer(mozilla::layers::ThebesLayer *,gfxContext *,nsIntRegion const &,mozilla::layers::DrawRegionClip,nsIntRegion const &,void *) 	layout/base/FrameLayerBuilder.cpp
12 	xul.dll 	mozilla::layers::ThebesLayerD3D9::DrawRegion(nsIntRegion &,mozilla::layers::SurfaceMode,nsTArray<mozilla::layers::ReadbackProcessor::Update> const &) 	gfx/layers/d3d9/ThebesLayerD3D9.cpp
13 	xul.dll 	mozilla::layers::ThebesLayerD3D9::RenderThebesLayer(mozilla::layers::ReadbackProcessor *) 	gfx/layers/d3d9/ThebesLayerD3D9.cpp
14 	xul.dll 	mozilla::layers::ContainerLayerD3D9::RenderLayer() 	gfx/layers/d3d9/ContainerLayerD3D9.cpp
15 	xul.dll 	mozilla::layers::ContainerLayerD3D9::RenderLayer() 	gfx/layers/d3d9/ContainerLayerD3D9.cpp
16 	xul.dll 	mozilla::layers::LayerManagerD3D9::Render() 	gfx/layers/d3d9/LayerManagerD3D9.cpp
17 	xul.dll 	mozilla::layers::LayerManagerD3D9::EndTransaction(void (*)(mozilla::layers::ThebesLayer *,gfxContext *,nsIntRegion const &,mozilla::layers::DrawRegionClip,nsIntRegion const &,void *),void *,mozilla::layers::LayerManager::EndTransactionFlags) 	gfx/layers/d3d9/LayerManagerD3D9.cpp
18 	xul.dll 	nsDisplayList::PaintForFrame(nsDisplayListBuilder *,nsRenderingContext *,nsIFrame *,unsigned int) 	layout/base/nsDisplayList.cpp
19 	xul.dll 	nsLayoutUtils::PaintFrame(nsRenderingContext *,nsIFrame *,nsRegion const &,unsigned int,unsigned int) 	layout/base/nsLayoutUtils.cpp
20 	xul.dll 	PresShell::Paint(nsView *,nsRegion const &,unsigned int) 	layout/base/nsPresShell.cpp
21 	xul.dll 	nsViewManager::ProcessPendingUpdatesPaint(nsIWidget *) 	view/src/nsViewManager.cpp
22 	xul.dll 	nsViewManager::ProcessPendingUpdatesForView(nsView *,bool) 	view/src/nsViewManager.cpp
23 	xul.dll 	nsViewManager::ProcessPendingUpdates() 	view/src/nsViewManager.cpp
24 	xul.dll 	nsRefreshDriver::Tick(__int64,mozilla::TimeStamp) 	layout/base/nsRefreshDriver.cpp
25 	xul.dll 	mozilla::RefreshDriverTimer::Tick() 	layout/base/nsRefreshDriver.cpp
26 	xul.dll 	nsTimerImpl::Fire() 	xpcom/threads/nsTimerImpl.cpp
27 	xul.dll 	nsTimerEvent::Run() 	xpcom/threads/nsTimerImpl.cpp
28 	xul.dll 	nsThread::ProcessNextEvent(bool,bool *) 	xpcom/threads/nsThread.cpp
29 	xul.dll 	NS_ProcessNextEvent(nsIThread *,bool) 	xpcom/glue/nsThreadUtils.cpp
30 	xul.dll 	mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate *) 	ipc/glue/MessagePump.cpp
31 	xul.dll 	_SEH_epilog4 	
32 	xul.dll 	MessageLoop::Run() 	ipc/chromium/src/base/message_loop.cc
33 	xul.dll 	nsBaseAppShell::Run() 	widget/xpwidgets/nsBaseAppShell.cpp
34 	xul.dll 	nsAppShell::Run() 	widget/windows/nsAppShell.cpp
35 	nss3.dll 	nss3.dll@0x7970 	
36 	xul.dll 	NS_InitXPCOM2 	xpcom/build/nsXPComInit.cpp
37 	mozalloc.dll 	moz_free 	memory/mozalloc/mozalloc.cpp
38 	xul.dll 	nsComponentManagerImpl::AddRef() 	xpcom/components/nsComponentManager.cpp
39 	xul.dll 	nsComponentManagerImpl::QueryInterface(nsID const &,void * *) 	xpcom/components/nsComponentManager.cpp
40 	xul.dll 	nsCOMPtr_base::assign_from_qi(nsQueryInterface,nsID const &) 	xpcom/glue/nsCOMPtr.cpp
41 	xul.dll 	ScopedXPCOMStartup::Initialize() 	toolkit/xre/nsAppRunner.cpp
42 	xul.dll 	XREMain::XRE_main(int,char * * const,nsXREAppData const *) 	toolkit/xre/nsAppRunner.cpp
43 	xul.dll 	XRE_main 	toolkit/xre/nsAppRunner.cpp
44 	firefox.exe 	do_main 	browser/app/nsBrowserApp.cpp
45 	firefox.exe 	NS_internal_main(int,char * *) 	browser/app/nsBrowserApp.cpp
46 	firefox.exe 	wmain 	toolkit/xre/nsWindowsWMain.cpp
47 	firefox.exe 	__tmainCRTStartup 	f:/dd/vctools/crt_bld/self_x86/crt/src/crtexe.c:552
48 	kernel32.dll 	kernel32.dll@0x1919f 	
49 	ntdll.dll 	ntdll.dll@0x4a8cb 	
50 	ntdll.dll 	ntdll.dll@0x4a8a1 	
=============================================================

Firefox 30 has 42 reports:
https://crash-stats.mozilla.com/search/?signature=~gfxContext%3A%3AgfxContext&product=Firefox&version=30.0&build_id=%3E20140506&_facets=build_id&_facets=version&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform

Firefox 31 has 3 reports:
https://crash-stats.mozilla.com/search/?signature=~gfxContext%3A%3AgfxContext&product=Firefox&version=31.0a2&build_id=%3E20140506&_facets=build_id&_facets=version&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform

Firefox 32 has 15 reports:
https://crash-stats.mozilla.com/search/?signature=~gfxContext%3A%3AgfxContext&product=Firefox&version=32.0a1&build_id=%3E20140502&_facets=build_id&_facets=version&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform

Comment 1

[Lukas Blakk [:lsblakk] use ?needinfo]

This bug is a clone of bug 952721 for 'lingering' crashes.  Without this being a topcrasher, it's not something we consider a tracking issue.  The original bug was reproducible.  Is this reproducible on Windows?

Comment 2

[u279076]

(In reply to Lukas Blakk [:lsblakk] from comment #1)
> Is this reproducible on Windows?

QA does not have steps to reproduce and volume is fairly low on Windows so I agree with untracking. I just wanted to nominate it so that this could be marked as such.

Comment 3

[(Away)]

+Seth per bug 952721 comment 78.
These are coming from ClippedImage.

Comment 4

[Robert Longson [:longsonr]]

Attachment: stop crashing

Comment 5

[Jonathan Watt [:jwatt]]

If CreateOffscreenContentDrawTarget returns nullptr, it either means that the requested size was very large or else we're out of [possibly graphics?] memory. I've previously noticed that data wrapping draw targets can be much larger than native draw targets, so one alternative would be to create one of those on failure. I don't think we should though, partly because performance of mixing gpu-based and system memory based draw targets/source surfaces is horrendous. I'd rather take Robert's patch and hope that the resulting misrendering will be reported to us so that we can investigate exactly what's going on when these things fail.

Comment 6

[Jonathan Watt [:jwatt]]

Comment on attachment 8430582 [details] [diff] [review]
stop crashing

r+, but can you make these all NS_ENSURE_TRUE, please? I'd like these to be as noisy as possible. Thanks, Robert!

Comment 7

[Daniel Holbert [:dholbert]]

(In reply to Jonathan Watt [:jwatt] from comment #6)
> Comment on attachment 8430582 [details] [diff] [review]
> stop crashing
> 
> r+, but can you make these all NS_ENSURE_TRUE, please? I'd like these to be
> as noisy as possible. Thanks, Robert!

Note that one of them already has a NS_WARNING (and matches local style, which uses NS_WARNING + return for error-handling). I think what you really want is a useful NS_WARNING in all of the explicit "return" clauses added here, yes?

(Propagating NS_ENSURE_TRUE into code that doesn't already use it is probably not-good, given that bsmedberg has advocated against using NS_ENSURE_* moving forward.)

Comment 8

[Jonathan Watt [:jwatt]]

I'd like something stronger than NS_WARNING. MOZ_ASSERT would be fine by me.

Comment 9

[Robert Longson [:longsonr]]

That's going to leave us with runtime crashes in debug builds still is that really what we want?

We really just want to ignore the operation and continue if we're short of graphics memory rather than aborting because the web page is too big/complex, in which case we need to stick with a warning or NS_ASSERTION

Comment 10

[Jonathan Watt [:jwatt]]

For general users we definitely do want to ignore the failure and continue and misrender. People running debug builds are presumably invested in improving our implementation though, so I'm not too worried about crashing them (potentially me) in exchange for a better chance of figuring out what's causing this. Failing these asserts for genuine OOM reasons should be relatively rare, so if anyone fails them it will most probably be because they've stumbled across a page that causes us to try and create giant DTs that the Moz2D backend rejects as too big for the GPU. That we want to hear about.

Comment 11

[Jonathan Watt [:jwatt]]

Robert, Daniel and I talked this over on IRC and we'd be happiest if you used:

  if (!thing) { NS_ERROR("..."); return; }

at all checking points. With that you can go ahead and check in, unless you're unhappy with that.

Comment 12

[Robert Longson [:longsonr]]

https://tbpl.mozilla.org/?tree=Try&rev=f16e2d0cece7

Comment 13

[Robert Longson [:longsonr]]

https://tbpl.mozilla.org/?tree=Try&rev=11a4ff3c1aa5

Comment 14

[Robert Longson [:longsonr]]

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

Comment 15

[Phil Ringnalda (:philor)]

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

Comment 16

[Bogdan Maris, Desktop Test Engineering [OUT until 12 Jan]]

Firefox 32 beta, 1 crash:
https://crash-stats.mozilla.com/query/?product=Firefox&version=Firefox%3A32.0b&range_value=1&range_unit=weeks&date=08%2F20%2F2014+13%3A00%3A00&query_search=signature&query_type=contains&query=gfxContext%3A%3AgfxContext%28mozilla%3A%3Agfx%3A%3ADrawTarget*%2C+mozilla%3A%3Agfx%3A%3APointTyped%3Cmozilla%3A%3Agfx%3A%3AUnknownUnits%3E+const%26%29&reason=&release_channels=&build_id=&process_type=any&hang_type=any

Aurora, 2 crashes:
https://crash-stats.mozilla.com/query/?product=Firefox&version=Firefox%3A33.0a2&range_value=1&range_unit=weeks&date=08%2F20%2F2014+13%3A00%3A00&query_search=signature&query_type=contains&query=gfxContext%3A%3AgfxContext%28mozilla%3A%3Agfx%3A%3ADrawTarget*%2C+mozilla%3A%3Agfx%3A%3APointTyped%3Cmozilla%3A%3Agfx%3A%3AUnknownUnits%3E+const%26%29&reason=&release_channels=&build_id=&process_type=any&hang_type=any

Nightly, 3 crashes:
https://crash-stats.mozilla.com/query/?product=Firefox&version=Firefox%3A34.0a1&range_value=1&range_unit=weeks&date=08%2F20%2F2014+13%3A00%3A00&query_search=signature&query_type=contains&query=gfxContext%3A%3AgfxContext%28mozilla%3A%3Agfx%3A%3ADrawTarget*%2C+mozilla%3A%3Agfx%3A%3APointTyped%3Cmozilla%3A%3Agfx%3A%3AUnknownUnits%3E+const%26%29&reason=&release_channels=&build_id=&process_type=any&hang_type=any

There are the results of Socorro for the past one week with this signature, is this enough to consider this as verified?

Comment 17

[Robert Longson [:longsonr]]

Looks good to me.

Comment 18

[Bogdan Maris, Desktop Test Engineering [OUT until 12 Jan]]

(In reply to Robert Longson from comment #17)
> Looks good to me.

Thanks. Closing as verified.