Closed Bug 1266209 Opened 8 years ago Closed 8 years ago

crash in mozilla::gfx::Path::TransformedCopyToBuilder

Categories

(Core :: Graphics, defect)

48 Branch
Unspecified
Windows
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox47 --- unaffected
firefox48 + wontfix
firefox49 --- fixed

People

(Reporter: u279076, Assigned: lsalzman)

Details

(5 keywords, Whiteboard: [gfx-noted])

Crash Data

Attachments

(3 files, 1 obsolete file)

This bug was filed from the Socorro interface and is 
report bp-0893a1d8-eebe-4f7f-813b-7f58e2160419.
=============================================================
0 	xul.dll 	mozilla::gfx::Path::TransformedCopyToBuilder(mozilla::gfx::Matrix const&) 	obj-firefox/dist/include/mozilla/gfx/2D.h
1 	xul.dll 	gfxContext::EnsurePathBuilder() 	gfx/thebes/gfxContext.cpp
2 	xul.dll 	GlyphBufferAzure::Flush(bool) 	gfx/thebes/gfxFont.cpp
3 	xul.dll 	gfxFont::DrawGlyphs(gfxShapedText*, unsigned int, unsigned int, gfxPoint*, TextRunDrawParams const&, FontDrawParams const&) 	gfx/thebes/gfxFont.cpp
4 	xul.dll 	gfxFont::Draw(gfxTextRun*, unsigned int, unsigned int, gfxPoint*, TextRunDrawParams const&, unsigned short) 	gfx/thebes/gfxFont.cpp
5 	xul.dll 	gfxTextRun::DrawGlyphs(gfxFont*, gfxTextRun::Range, gfxPoint*, gfxTextRun::PropertyProvider*, gfxTextRun::Range, TextRunDrawParams&, unsigned short) 	gfx/thebes/gfxTextRun.cpp
6 	xul.dll 	gfxTextRun::Draw(gfxTextRun::Range, gfxPoint, gfxTextRun::DrawParams const&) 	gfx/thebes/gfxTextRun.cpp
7 	xul.dll 	DrawTextRun 	layout/generic/nsTextFrame.cpp
8 	xul.dll 	nsTextFrame::DrawTextRun(gfxTextRun::Range, gfxPoint const&, nsTextFrame::DrawTextRunParams const&) 	layout/generic/nsTextFrame.cpp
9 	xul.dll 	nsTextFrame::DrawText(gfxTextRun::Range, gfxPoint const&, nsTextFrame::DrawTextParams const&) 	layout/generic/nsTextFrame.cpp
10 	xul.dll 	nsTextFrame::PaintText(nsTextFrame::PaintTextParams const&, nsCharClipDisplayItem const&, float) 	layout/generic/nsTextFrame.cpp
11 	xul.dll 	nsDisplayText::Paint(nsDisplayListBuilder*, nsRenderingContext*) 	layout/generic/nsTextFrame.cpp
12 	xul.dll 	mozilla::FrameLayerBuilder::PaintItems(nsTArray<mozilla::FrameLayerBuilder::ClippedDisplayItem>&, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&, gfxContext*, nsRenderingContext*, nsDisplayListBuilder*, nsPresContext*, mozilla::gfx::IntPointTyped<mozilla::gfx::UnknownUnits> const&, float, float, int) 	layout/base/FrameLayerBuilder.cpp
13 	xul.dll 	mozilla::FrameLayerBuilder::DrawPaintedLayer(mozilla::layers::PaintedLayer*, gfxContext*, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::layers::DrawRegionClip, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, void*) 	layout/base/FrameLayerBuilder.cpp
14 	xul.dll 	mozilla::layers::BasicPaintedLayer::PaintThebes(gfxContext*, mozilla::layers::Layer*, void (*)(mozilla::layers::PaintedLayer*, gfxContext*, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::layers::DrawRegionClip, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, void*), void*) 	gfx/layers/basic/BasicPaintedLayer.cpp
15 	xul.dll 	mozilla::layers::BasicLayerManager::PaintSelfOrChildren(mozilla::layers::PaintLayerContext&, gfxContext*) 	gfx/layers/basic/BasicLayerManager.cpp
16 	xul.dll 	mozilla::layers::BasicLayerManager::PaintLayer(gfxContext*, mozilla::layers::Layer*, void (*)(mozilla::layers::PaintedLayer*, gfxContext*, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::layers::DrawRegionClip, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, void*), void*) 	gfx/layers/basic/BasicLayerManager.cpp
17 	xul.dll 	mozilla::layers::BasicLayerManager::PaintSelfOrChildren(mozilla::layers::PaintLayerContext&, gfxContext*) 	gfx/layers/basic/BasicLayerManager.cpp
18 	xul.dll 	mozilla::layers::BasicLayerManager::PaintLayer(gfxContext*, mozilla::layers::Layer*, void (*)(mozilla::layers::PaintedLayer*, gfxContext*, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::layers::DrawRegionClip, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, void*), void*) 	gfx/layers/basic/BasicLayerManager.cpp
19 	xul.dll 	mozilla::layers::BasicLayerManager::PaintSelfOrChildren(mozilla::layers::PaintLayerContext&, gfxContext*) 	gfx/layers/basic/BasicLayerManager.cpp
20 	xul.dll 	mozilla::layers::BasicLayerManager::PaintLayer(gfxContext*, mozilla::layers::Layer*, void (*)(mozilla::layers::PaintedLayer*, gfxContext*, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::layers::DrawRegionClip, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, void*), void*) 	gfx/layers/basic/BasicLayerManager.cpp
21 	xul.dll 	mozilla::layers::BasicLayerManager::EndTransactionInternal(void (*)(mozilla::layers::PaintedLayer*, gfxContext*, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::layers::DrawRegionClip, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, void*), void*, mozilla::layers::LayerManager::EndTransactionFlags) 	gfx/layers/basic/BasicLayerManager.cpp
22 	xul.dll 	nsDisplayList::PaintRoot(nsDisplayListBuilder*, nsRenderingContext*, unsigned int) 	layout/base/nsDisplayList.cpp
23 	xul.dll 	ClipBackgroundByText 	layout/base/nsDisplayList.cpp
24 	xul.dll 	nsDisplayBackgroundImage::PaintInternal(nsDisplayListBuilder*, nsRenderingContext*, nsRect const&, nsRect*) 	layout/base/nsDisplayList.cpp
25 	xul.dll 	nsDisplayBackgroundImage::Paint(nsDisplayListBuilder*, nsRenderingContext*) 	layout/base/nsDisplayList.cpp
26 	xul.dll 	mozilla::FrameLayerBuilder::PaintItems(nsTArray<mozilla::FrameLayerBuilder::ClippedDisplayItem>&, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&, gfxContext*, nsRenderingContext*, nsDisplayListBuilder*, nsPresContext*, mozilla::gfx::IntPointTyped<mozilla::gfx::UnknownUnits> const&, float, float, int) 	layout/base/FrameLayerBuilder.cpp
27 	xul.dll 	mozilla::FrameLayerBuilder::DrawPaintedLayer(mozilla::layers::PaintedLayer*, gfxContext*, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::layers::DrawRegionClip, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, void*) 	layout/base/FrameLayerBuilder.cpp
28 	xul.dll 	mozilla::layers::ClientPaintedLayer::PaintThebes() 	gfx/layers/client/ClientPaintedLayer.cpp
29 	xul.dll 	mozilla::layers::ClientPaintedLayer::RenderLayerWithReadback(mozilla::layers::ReadbackProcessor*) 	gfx/layers/client/ClientPaintedLayer.cpp
30 	xul.dll 	mozilla::layers::ClientContainerLayer::RenderLayer() 	gfx/layers/client/ClientContainerLayer.h
31 	xul.dll 	mozilla::layers::ClientContainerLayer::RenderLayer() 	gfx/layers/client/ClientContainerLayer.h
32 	xul.dll 	mozilla::layers::ClientLayerManager::EndTransactionInternal(void (*)(mozilla::layers::PaintedLayer*, gfxContext*, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::layers::DrawRegionClip, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, void*), void*, mozilla::layers::LayerManager::EndTransactionFlags) 	gfx/layers/client/ClientLayerManager.cpp
33 	xul.dll 	mozilla::layers::ClientLayerManager::EndTransaction(void (*)(mozilla::layers::PaintedLayer*, gfxContext*, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::layers::DrawRegionClip, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, void*), void*, mozilla::layers::LayerManager::EndTransactionFlags) 	gfx/layers/client/ClientLayerManager.cpp
34 	xul.dll 	nsDisplayList::PaintRoot(nsDisplayListBuilder*, nsRenderingContext*, unsigned int) 	layout/base/nsDisplayList.cpp
35 	xul.dll 	nsLayoutUtils::PaintFrame(nsRenderingContext*, nsIFrame*, nsRegion const&, unsigned int, unsigned int) 	layout/base/nsLayoutUtils.cpp
36 	xul.dll 	PresShell::Paint(nsView*, nsRegion const&, unsigned int) 	layout/base/nsPresShell.cpp
37 	xul.dll 	nsViewManager::ProcessPendingUpdatesPaint(nsIWidget*) 	view/nsViewManager.cpp
38 	xul.dll 	nsViewManager::ProcessPendingUpdatesForView(nsView*, bool) 	view/nsViewManager.cpp
39 	xul.dll 	nsViewManager::ProcessPendingUpdates() 	view/nsViewManager.cpp
40 	xul.dll 	nsRefreshDriver::Tick(__int64, mozilla::TimeStamp) 	layout/base/nsRefreshDriver.cpp
41 	xul.dll 	mozilla::RefreshDriverTimer::TickDriver(nsRefreshDriver*, __int64, mozilla::TimeStamp) 	layout/base/nsRefreshDriver.cpp
42 	xul.dll 	mozilla::RefreshDriverTimer::TickRefreshDrivers(__int64, mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&) 	layout/base/nsRefreshDriver.cpp
43 	xul.dll 	mozilla::RefreshDriverTimer::Tick(__int64, mozilla::TimeStamp) 	layout/base/nsRefreshDriver.cpp
44 	xul.dll 	mozilla::VsyncRefreshDriverTimer::RunRefreshDrivers(mozilla::TimeStamp) 	layout/base/nsRefreshDriver.cpp
45 	xul.dll 	mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver(mozilla::TimeStamp) 	layout/base/nsRefreshDriver.cpp
46 	xul.dll 	mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::NotifyVsync(mozilla::TimeStamp) 	layout/base/nsRefreshDriver.cpp
47 	xul.dll 	mozilla::layout::VsyncChild::RecvNotify(mozilla::TimeStamp const&) 	layout/ipc/VsyncChild.cpp
48 	xul.dll 	mozilla::layout::PVsyncChild::OnMessageReceived(IPC::Message const&) 	obj-firefox/ipc/ipdl/PVsyncChild.cpp
49 	xul.dll 	mozilla::ipc::PBackgroundChild::OnMessageReceived(IPC::Message const&) 	obj-firefox/ipc/ipdl/PBackgroundChild.cpp
50 	xul.dll 	mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&) 	ipc/glue/MessageChannel.cpp
51 	xul.dll 	mozilla::ipc::MessageChannel::DispatchMessageW(IPC::Message const&) 	ipc/glue/MessageChannel.cpp
52 	xul.dll 	mozilla::ipc::MessageChannel::OnMaybeDequeueOne() 	ipc/glue/MessageChannel.cpp
53 	xul.dll 	RunnableMethod<mozilla::ipc::MessageChannel, void ( mozilla::ipc::MessageChannel::*)(void), mozilla::Tuple<> >::Run() 	ipc/chromium/src/base/task.h
54 	xul.dll 	MessageLoop::RunTask(Task*) 	ipc/chromium/src/base/message_loop.cc
55 	xul.dll 	MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask const&) 	ipc/chromium/src/base/message_loop.cc
56 	xul.dll 	MessageLoop::DoWork() 	ipc/chromium/src/base/message_loop.cc
57 	xul.dll 	mozilla::ipc::DoWorkRunnable::Run() 	ipc/glue/MessagePump.cpp
58 	xul.dll 	nsThread::ProcessNextEvent(bool, bool*) 	xpcom/threads/nsThread.cpp
59 	xul.dll 	NS_ProcessNextEvent(nsIThread*, bool) 	xpcom/glue/nsThreadUtils.cpp
60 	xul.dll 	mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) 	ipc/glue/MessagePump.cpp
61 	xul.dll 	mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) 	ipc/glue/MessagePump.cpp
62 	xul.dll 	MessageLoop::RunHandler() 	ipc/chromium/src/base/message_loop.cc
63 	xul.dll 	MessageLoop::Run() 	ipc/chromium/src/base/message_loop.cc
64 	xul.dll 	nsBaseAppShell::Run() 	widget/nsBaseAppShell.cpp
65 	xul.dll 	nsAppShell::Run() 	widget/windows/nsAppShell.cpp
66 	xul.dll 	XRE_RunAppShell 	toolkit/xre/nsEmbedFunctions.cpp
67 	xul.dll 	mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) 	ipc/glue/MessagePump.cpp
68 	xul.dll 	MessageLoop::RunHandler() 	ipc/chromium/src/base/message_loop.cc
69 	xul.dll 	MessageLoop::Run() 	ipc/chromium/src/base/message_loop.cc
70 	xul.dll 	XRE_InitChildProcess 	toolkit/xre/nsEmbedFunctions.cpp
71 	plugin-container.exe 	wmain 	toolkit/xre/nsWindowsWMain.cpp
72 	plugin-container.exe 	__scrt_common_main_seh 	f:/dd/vctools/crt/vcstartup/src/startup/exe_common.inl:264
Ø 73 	kernel32.dll 	kernel32.dll@0x15a4c 	
Ø 74 	ntdll.dll 	ntdll.dll@0x2b830 	
Ø 75 	kernel32.dll 	kernel32.dll@0x9b82f 	
Ø 76 	kernel32.dll 	kernel32.dll@0x9b82f 	
=============================================================
More reports: https://crash-stats.mozilla.com/report/list?signature=mozilla%3A%3Agfx%3A%3APath%3A%3ATransformedCopyToBuilder#tab-reports
[Tracking Requested - why for this release]:
This is a new crash starting with 20160418030305.

Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=1f16d3da9280e40ada252acf8110b91ee1edbb08&tochange=ae7413abfa4d3954a6a4ce7c1613a7100f367f9a
Milan, do you know who can look in to this?
Flags: needinfo?(milan)
The overt symptom here seems to be that gfxContext::EnsurePathBuilder calls path->TransformedCopyToBuilder where path is null.

It seems that PathBuilderD2D::Finish(), which is supposed to be constructing said path, is returning null here, which it seems can only happen because the call to ID2D1GeometrySink::Close() contained therein fails.

This is a case where I am not sure what the best way to deal with it is, because many callers seem to want EnsurePathBuilder to always ensure a path builder is constructed. Merely asserting here would just move the crash, but still let it crash... Presumably we want to do better here, but PathD2D seems to be the ugly duckling here and the only PathBuilder implementation that behaves this way with letting Finish fail.

Bas, thoughts?
Flags: needinfo?(bas)
(In reply to Lee Salzman [:lsalzman] from comment #3)
> The overt symptom here seems to be that gfxContext::EnsurePathBuilder calls
> path->TransformedCopyToBuilder where path is null.
> 
> It seems that PathBuilderD2D::Finish(), which is supposed to be constructing
> said path, is returning null here, which it seems can only happen because
> the call to ID2D1GeometrySink::Close() contained therein fails.
> 
> This is a case where I am not sure what the best way to deal with it is,
> because many callers seem to want EnsurePathBuilder to always ensure a path
> builder is constructed. Merely asserting here would just move the crash, but
> still let it crash... Presumably we want to do better here, but PathD2D
> seems to be the ugly duckling here and the only PathBuilder implementation
> that behaves this way with letting Finish fail.
> 
> Bas, thoughts?

At the very least we should log the reason (D2D) Finish is failing with a gfxCriticalError. I think Finish faling is somewhat reasonable, if only because one might want it to be fallible for memory reasons on large paths. But it would be good to get an idea of why in this case, since the reports indicate plenty of memory still available.
Flags: needinfo?(bas)
This patch for now just adds a mousetrap to catch this particular bug. It is designed to at least log the reason Finish fails with a gfxCriticalNote, while doing the actual gfxCriticalError where we are currently crashing anyway because of the null dereference.

There are other places where PathBuilderD2D::Finish is called (even in the D2D1 code) and expected to be infallible, but this patch does not seek to address those right now. Ideally we want to find the reason PathBuilderD2D::Finish fails and address that.

This does not fix the issue, but at least will give us more information on how to fix it eventually.
Attachment #8743978 - Flags: review?(bas)
Flags: needinfo?(milan)
There is BasicLayerManager and no mention of D3D11+ in the app notes - Bas, is this a false alarm, or are we somehow in D2D without D3D11?
Flags: needinfo?(bas)
Attachment #8743978 - Flags: review?(bas) → review+
(In reply to Milan Sreckovic [:milan] from comment #6)
> There is BasicLayerManager and no mention of D3D11+ in the app notes - Bas,
> is this a false alarm, or are we somehow in D2D without D3D11?

Yeah.. I don't really have an explanation for this. One thing we can be sure of is that if you have a BasicLayerManager you won't get any of your content rendering occurring with D2D 1.1.
Flags: needinfo?(bas)
(In reply to Anthony Hughes (:ashughes) [GFX][QA][Mentor] from comment #1)
> [Tracking Requested - why for this release]:
> This is a new crash starting with 20160418030305.

I agree with this.  I'd also consider it a topcrash in nightly.

> Pushlog:
> https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=1f16d3da9280e40ada252acf8110b91ee1edbb08&tochange=ae7413abfa4d3954a6a4ce7c1613a7100f367f9a

I think the corresponding regression range should actually be:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=1da1937a9e03&tochange=1f16d3da9280
which doesn't make much sense.

A query for the crashes in nightly is:
https://crash-stats.mozilla.com/signature/?product=Firefox&release_channel=nightly&platform=Windows&date=%3E%3D2016-04-01&signature=mozilla%3A%3Agfx%3A%3APath%3A%3ATransformedCopyToBuilder
Keywords: topcrash
Keywords: leave-open
Nightly 20160429030215 has 5 occurrences of this crash from 2 users. The GraphicsCriticalError field says:

> |[0][GFX1-]: Failed to close PathSink. Code: 0x88990001 (t=32010.8) |[1][GFX1]: gfxContext::EnsurePathBuilder failed in PathBuilder::Finish (t=32010.8) 
> 
> |[C0][GFX1-]: Failed to close PathSink. Code: 0x88990001 (t=18.0147) |[C1][GFX1]: gfxContext::EnsurePathBuilder failed in PathBuilder::Finish (t=18.0147) 
> 
> |[C0][GFX1-]: Failed to close PathSink. Code: 0x88990001 (t=38.962) |[C1][GFX1]: gfxContext::EnsurePathBuilder failed in PathBuilder::Finish (t=38.962) 
> 
> |[C0][GFX1-]: Failed to close PathSink. Code: 0x88990001 (t=849.046) |[C1][GFX1]: gfxContext::EnsurePathBuilder failed in PathBuilder::Finish (t=849.046) 
> 
> |[C0][GFX1-]: Failed to close PathSink. Code: 0x88990001 (t=292.575) |[C1][GFX1]: gfxContext::EnsurePathBuilder failed in PathBuilder::Finish (t=292.575)
Flags: needinfo?(lsalzman)
(In reply to Bas Schouten (:bas.schouten) from comment #7)
> (In reply to Milan Sreckovic [:milan] from comment #6)
> > There is BasicLayerManager and no mention of D3D11+ in the app notes - Bas,
> > is this a false alarm, or are we somehow in D2D without D3D11?
> 
> Yeah.. I don't really have an explanation for this. One thing we can be sure
> of is that if you have a BasicLayerManager you won't get any of your content
> rendering occurring with D2D 1.1.

Likely bug 1268628.



(In reply to Nicholas Nethercote [:njn] from comment #11)
> Nightly 20160429030215 has 5 occurrences of this crash from 2 users. The
> GraphicsCriticalError field says:
> 
> > |[0][GFX1-]: Failed to close PathSink. Code: 0x88990001 (t=32010.8) |[1][GFX1]: gfxContext::EnsurePathBuilder failed in PathBuilder::Finish (t=32010.8) 

This is D2DERR_WRONG_STATE - "The object was not in the correct state to process the method"
(In reply to Milan Sreckovic [:milan] from comment #12)
>...
> This is D2DERR_WRONG_STATE - "The object was not in the correct state to
> process the method"

And given that it was Close() that was called, this applies: "This method returns D2DERR_WRONG_STATE if it has already been called on the command list. If an error occurred on the device context during population, the method returns that error. Otherwise, the method returns S_OK."

So, chances are we are calling Close() twice?
(In reply to Milan Sreckovic [:milan] from comment #13)
> (In reply to Milan Sreckovic [:milan] from comment #12)
> >...
> > This is D2DERR_WRONG_STATE - "The object was not in the correct state to
> > process the method"
> 
> And given that it was Close() that was called, this applies: "This method
> returns D2DERR_WRONG_STATE if it has already been called on the command
> list. If an error occurred on the device context during population, the
> method returns that error. Otherwise, the method returns S_OK."
> 
> So, chances are we are calling Close() twice?

The Close in this case is from ID2D1SimpliedGeometrySink (which doesn't use that wording): https://msdn.microsoft.com/en-us/library/windows/desktop/dd316932%28v=vs.85%29.aspx

The documentation for that is kind of vague, but it seems that calling Close() twice there can also potentially do it. Potentially also calling Close() on a geometry sink that is not in the open state, or on one that has BeginFigure called on it but not EndFigure, though none of these are directly and unambiguously stated. Though with regard to any of those three possibilities, I didn't see any obvious cases where that might occur, but it would require more digging to really verify.
Flags: needinfo?(lsalzman)
It seems like the only way to know the ID2D1SimplifiedGeometrySink is in an error state is to call Close on it. By that time, it's too late as that is what is getting us into this mess in the first place. Bas, ideas?
Flags: needinfo?(bas)
(In reply to Lee Salzman [:lsalzman] from comment #15)
> It seems like the only way to know the ID2D1SimplifiedGeometrySink is in an
> error state is to call Close on it. By that time, it's too late as that is
> what is getting us into this mess in the first place. Bas, ideas?

I will try to look at the code tomorrow and philosophize about what could get this sink in an invalid state.
Flags: needinfo?(bas)
Yet another attempt at prying some more information out of this inscrutable bug to see if we can get further. Simplify is fallible, so I am wondering if it is in part responsible. This gives a warning to attempt to find that out.
Attachment #8749717 - Flags: review?(bas)
Attachment #8749717 - Flags: review?(bas) → review+
Seems like we're calling TransformedCopyToBuilder on a nullptr path, so we crash making a virtual function call?
(In reply to Milan Sreckovic [:milan] from comment #21)
> Seems like we're calling TransformedCopyToBuilder on a nullptr path, so we
> crash making a virtual function call?

And PathD2D::Finish is expected to sometimes fail, so we can't just assert and continue to crash :)
The error above suggests that we have unbalanced mSink->BeginFigure() and mSink->EndFigure() calls.  We need to add more information to track this and find out what's going on.  In the meantime, since this amounts to a MOZ_CRASH, may as well make it one, or even better, a gfxDevCrash().

That does mean we have to take care of the fallible scenario, perhaps only temporarily.  It may turn out that there is a limitation in Direct2D that's not letting us do what we want to do (although I still think we're somewhere unbalancing the Begin/EndFigure and that we can fix that).  In case it doesn't, we'll need to do with the failure, and may as well start now.  Unless we can get enough data in the next day or few worth of crashes.

Bas, is there a reason Finish() isn't considered fallible already, given that we actually have the code in there that makes it fail?
Flags: needinfo?(bas)
I've been poring over the code, even looking inside the Wine source code for answers, and I don't think we're going to get anything conclusive here without an STR.

It seems unlikely that we're messing up BeginFigure/EndFigure, as we have code to track that which at least superficially seems correct on a read-through.

Given that the crashes are happening inside glyph path building, where we are mostly using GetGlyphRunOutline from D2D, we would not even be using BeginFigure/EndFigure directly here.

The Wine source code indicates that, at least in its implementation, BeginFigure can put the path sink in an error state if it fails to add room for a new figure. So that's one possibility, though D2D itself could be doing other wildly different things.

Another possibility is that the path geometry error state is coming from somewhere outside the text rendering code, and that the text rendering code is just what happens to unfortunately trigger the crash after the fact. The crash stacks don't really give us enough information to determine this, though.

One other possibility that seems feasible from reading the Wine code: GetGlyphRunOutline calls BeginFigure on a MoveTo, so if there is already a path builder set in our gfxContext with a Figure active, it could then put the path sink into an error state when GetGlyphRunOutline does its thing. This one might be worth putting a trap in place for somehow.

Some possible wallpapering options for the crash if desired: 1) we could make PathBuilderD2D::Finish return an empty path, 2) we could make EnsurePathBuilder create a path builder for a new empty path if Finish fails, 3) we could allow EnsurePathBuilder to be fallible in gfxContext so that all callers need to deal with this.
Flags: needinfo?(lsalzman)
We don't want the users to suffer while we're trying to figure things out, so let's try the trap you mention.  We should also put in a gfxDevCrash in EnsurePathBuilder (which will change the signature of the crash, but we can deal with that), and then deal with the failure, probably by doing #2 you suggest.
Another reasonable stab in the dark: examination of the Wine code revealed that GetGlyphRunOutline expects no figure to be active at the time it is called. It seems possible that a path builder is left around in just such a state somewhere in the code. We just don't know if or where yet...

This patch just tries to answer the question of if. Should it prove true, we could feasibly use some sort of __FILE__/__LINE__ hack to find out where, or just closing out the existing figures to nullify it before copying glyphs.
Attachment #8753149 - Flags: review?(bas)
Oops. Checked the hresult of the wrong call. Fixed in this version.
Attachment #8753149 - Attachment is obsolete: true
Attachment #8753149 - Flags: review?(bas)
Attachment #8753150 - Flags: review?(bas)
Attachment #8753150 - Flags: review?(bas) → review+
Flags: needinfo?(bas)
I'm not seeing any recent crashes in Nightly 49, and ~50 crashes in Aurora 48.  It may be worth uplifting some of the diagnostics in this bug (should the warning in the second patch be replaced with a critical note?) to get more data.  Unless the bug volume has dropped down enough to just ride 49.
(In reply to Milan Sreckovic [:milan] from comment #30)
> I'm not seeing any recent crashes in Nightly 49, and ~50 crashes in Aurora
> 48.  It may be worth uplifting some of the diagnostics in this bug (should
> the warning in the second patch be replaced with a critical note?) to get
> more data.  Unless the bug volume has dropped down enough to just ride 49.

I wonder if this bug is related bogus device status like from a device reset, or using D2D where we shouldn't be as hypothesized in c6, since that got a lot of attention in the last release or two. If that's the case, then the diagnostics here probably are not going to turn up anything, since they didn't turn up the problem yet.

It is still possible that whatever "fixed" it and the issue are separate but dependent bugs, but that seems unlikely.

I am inclined to just let things ride with this while keeping a watch on it, hoping that we managed to unintentionally fix it. If not, we can re-investigate.
We are seeing a small number of crashes in 48, none in 49 or 50, but since we don't really know if it was in fact the PathD2D.cpp patch on this bug that fixed it, we will leave this riding 49 and not fix it in 48.  Unless the numbers really blow up.
Assignee: nobody → lsalzman
Status: NEW → RESOLVED
Closed: 8 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.