Closed
Bug 1266209
Opened 8 years ago
Closed 8 years ago
crash in mozilla::gfx::Path::TransformedCopyToBuilder
Categories
(Core :: Graphics, defect)
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)
1.60 KB,
patch
|
bas.schouten
:
review+
|
Details | Diff | Splinter Review |
1.68 KB,
patch
|
bas.schouten
:
review+
|
Details | Diff | Splinter Review |
2.73 KB,
patch
|
bas.schouten
:
review+
|
Details | Diff | Splinter Review |
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)
Assignee | ||
Comment 3•8 years ago
|
||
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)
Comment 4•8 years ago
|
||
(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)
Assignee | ||
Comment 5•8 years ago
|
||
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)
Updated•8 years ago
|
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)
Updated•8 years ago
|
Attachment #8743978 -
Flags: review?(bas) → review+
Comment 7•8 years ago
|
||
(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
Assignee | ||
Updated•8 years ago
|
Keywords: leave-open
Comment 10•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b0e5199c93c6
Comment 11•8 years ago
|
||
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?
Assignee | ||
Comment 14•8 years ago
|
||
(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)
Assignee | ||
Updated•8 years ago
|
Keywords: steps-wanted,
testcase-wanted
Assignee | ||
Comment 15•8 years ago
|
||
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)
Comment 16•8 years ago
|
||
(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)
Assignee | ||
Comment 17•8 years ago
|
||
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)
Updated•8 years ago
|
Attachment #8749717 -
Flags: review?(bas) → review+
Comment 19•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f89f0e3d9d7a
https://crash-stats.mozilla.com/signature/?build_id=20160512030253&cpu_arch=x86&product=Firefox&release_channel=nightly&platform=Windows&date=%3E%3D2016-05-12&signature=mozilla%3A%3Agfx%3A%3APath%3A%3ATransformedCopyToBuilder shows the crashes in the May 12 build. I'm not sure what information you'd get out of them from the above patch.
Flags: needinfo?(lsalzman)
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)
Assignee | ||
Comment 24•8 years ago
|
||
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.
Assignee | ||
Comment 26•8 years ago
|
||
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)
Assignee | ||
Comment 27•8 years ago
|
||
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)
Updated•8 years ago
|
Attachment #8753150 -
Flags: review?(bas) → review+
Updated•8 years ago
|
Flags: needinfo?(bas)
Comment 29•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a7f37ab1e0c3
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.
Assignee | ||
Comment 31•8 years ago
|
||
(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
status-firefox49:
--- → fixed
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Updated•8 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•