Closed
Bug 1105702
Opened 10 years ago
Closed 10 years ago
HandleError crash in CompositorD3D11::VerifyBufferSize
Categories
(Core :: Graphics, defect)
Tracking
()
RESOLVED
WORKSFORME
People
(Reporter: kairo, Unassigned)
References
(Blocks 1 open bug)
Details
(Keywords: crash)
Crash Data
Attachments
(1 file)
8.34 KB,
patch
|
nical
:
feedback+
|
Details | Diff | Splinter Review |
This bug was filed from the Socorro interface and is
report bp-e949202f-2de1-400b-8650-28d612141127.
=============================================================
This is another crash tipped up by bug 1087270. This is happening at somewhat low volume on Nightly, but low volume on Nightly usually means significant volume by the time things hit beta.
Top frames:
0 xul.dll mozilla::layers::CompositorD3D11::HandleError(long, mozilla::layers::CompositorD3D11::Severity) gfx/layers/d3d11/CompositorD3D11.cpp
1 xul.dll mozilla::layers::CompositorD3D11::VerifyBufferSize() gfx/layers/d3d11/CompositorD3D11.cpp
2 xul.dll mozilla::layers::CompositorD3D11::UpdateRenderTarget() gfx/layers/d3d11/CompositorD3D11.cpp
3 xul.dll mozilla::layers::CompositorD3D11::BeginFrame(nsIntRegion const&, mozilla::gfx::RectTyped<mozilla::gfx::UnknownUnits> const*, mozilla::gfx::RectTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::RectTyped<mozilla::gfx::UnknownUnits>*, mozilla::gfx::RectTyped<mozilla::gfx::UnknownUnits>*) gfx/layers/d3d11/CompositorD3D11.cpp
4 xul.dll mozilla::layers::LayerManagerComposite::Render() gfx/layers/composite/LayerManagerComposite.cpp
Find more reports and stats at https://crash-stats.mozilla.com/report/list?signature=mozilla%3A%3Alayers%3A%3ACompositorD3D11%3A%3AHandleError%28long%2C%20mozilla%3A%3Alayers%3A%3ACompositorD3D11%3A%3ASeverity%29%20%7C%20mozilla%3A%3Alayers%3A%3ACompositorD3D11%3A%3AVerifyBufferSize%28%29
Comment 1•10 years ago
|
||
I'm thinking something like this to get more information from the crashes. If it is useful to know the size, that is. Is it possible that we're running into "you need at least two in the chain" limitation?
Attachment #8530409 -
Flags: feedback?(nical.bugzilla)
Reporter | ||
Comment 2•10 years ago
|
||
Comment on attachment 8530409 [details] [diff] [review]
Get more information from the crashes.
Review of attachment 8530409 [details] [diff] [review]:
-----------------------------------------------------------------
::: gfx/layers/d3d11/CompositorD3D11.cpp
@@ +1139,5 @@
> mContext->Unmap(readTexture, 0);
> }
>
> +bool
> +CompositorD3D11::IsCriticalError(HRESULT hr)
Does that change mean we'll need to replace ::HandleError with ::IsCriticalError in the crash-stats prefix list as well? Probably not, as there's no MOZ_CRASH left in here, but do the locations of the new MOZ_CRASH calls make more sense or do we need more additions to the prefix list?
Comment 3•10 years ago
|
||
Ignore the renaming of the function for now. Some more details that I thought I added before the patch, but it turns out I didn't:
Looks like we're hitting DXGI_ERROR_DEVICE_REMOVED -> DXGI_ERROR_INVALID_CALL return from ResizeBuffers (buffer count 1) and we MOZ_CRASH 'on purpose'. Bad size or doesn't like swap chain length 1?
I'm also unclear about HandleError() function. We purposely assert (MOZ_ASSERT(aSeverity != DebugAssert) if there is a failure, right? A comment mentions that we should be using gfxCriticalError, but we're not because of the off main thread issues - gfxCriticalError works off main thread, except in the E10S situation. We're not really using Severity enum, but I don't know if that's work in progress or just something that was started and then didn't quite get finished.
Comment 4•10 years ago
|
||
(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #2)
> ...
>
> Does that change mean we'll need to replace ::HandleError with
> ::IsCriticalError in the crash-stats prefix list as well? Probably not, as
> there's no MOZ_CRASH left in here, but do the locations of the new MOZ_CRASH
> calls make more sense or do we need more additions to the prefix list?
If we were to take this patch, looks like we can remove HandleError from the prefix list, and would not need to add IsCriticalError.
Comment 5•10 years ago
|
||
Comment on attachment 8530409 [details] [diff] [review]
Get more information from the crashes.
Review of attachment 8530409 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good apart from HRESULT being logged as decimal instead of hexadecimal, see below.
::: gfx/layers/d3d11/CompositorD3D11.cpp
@@ +1163,3 @@
> {
> + if (IsCriticalError(hr)) {
> + gfxCriticalError() << "Failed at " << aOp << " result " << hr;
gfx::hexa(hr)
@@ +1165,5 @@
> + gfxCriticalError() << "Failed at " << aOp << " result " << hr;
> + MOZ_CRASH("Failed with a critical error");
> + return true;
> + } else if (FAILED(hr)) {
> + gfxWarning() << "Failed at " << aOp << " result " << hr;
same thing about the hexadecimal code
Attachment #8530409 -
Flags: feedback?(nical.bugzilla) → feedback+
Comment 6•10 years ago
|
||
(In reply to Nicolas Silva [:nical] from comment #5)
> ...
> > + gfxCriticalError() << "Failed at " << aOp << " result " << hr;
>
> gfx::hexa(hr)
Good point, easy enough to do.
On a related note, Bas, is it against the overall design to have BasicLogger understand HRESULT (Windows only) and get a separate overload for it, especially as it is platform specific? I imagine it is, just checking.
Flags: needinfo?(bas)
Comment 7•10 years ago
|
||
(In reply to Milan Sreckovic [:milan] from comment #3)
> I'm also unclear about HandleError() function. We purposely assert
> (MOZ_ASSERT(aSeverity != DebugAssert) if there is a failure, right? A
> comment mentions that we should be using gfxCriticalError, but we're not
> because of the off main thread issues - gfxCriticalError works off main
> thread, except in the E10S situation.
At the time this was added, gfxCriticalError was only useful when called from the main thread as it wouldn't do anything when called off the main thread.
> We're not really using Severity enum,
> but I don't know if that's work in progress or just something that was
> started and then didn't quite get finished.
The idea is to be able to do things like
hr = IfThisFailsThenWeCantRenderAnythingSoWeAreDoomed();
HandleError(hr, Critical);
But I haven't yet spent the time figuring out which calls are so critical that we'd rather crash than recover from them. It's just sugar and I don't mind if we remove it.
Comment 8•10 years ago
|
||
(In reply to Milan Sreckovic [:milan] from comment #6)
> On a related note, Bas, is it against the overall design to have BasicLogger
> understand HRESULT (Windows only) and get a separate overload for it,
> especially as it is platform specific? I imagine it is, just checking.
IIRC, HRESULT is just an uint64_t so if we have the logger recognize it as an hexadecimal value, it will log all 64bit integers the same way.
Comment 9•10 years ago
|
||
Yeah, that makes sense. It feels a bit smelly to add it to Logging.h anyway... :)
Flags: needinfo?(bas)
Comment 10•10 years ago
|
||
(In reply to Nicolas Silva [:nical] from comment #8)
> IIRC, HRESULT is just an uint64_t
HRESULT is a long [1], which is an uint32_t on Windows.
[1] http://msdn.microsoft.com/en-us/library/windows/desktop/aa383751.aspx
Comment 11•10 years ago
|
||
Sorry, an int32_t.
Comment 12•10 years ago
|
||
(In reply to Nicolas Silva [:nical] from comment #7)
> ...
>
> hr = IfThisFailsThenWeCantRenderAnythingSoWeAreDoomed();
> HandleError(hr, Critical);
Yeah, makes sense. In the case of critical errors, we crash, so we get the information we need from the stack. If we don't crash though, it'd be nice to get some of the information out of it, and if HandleError is a function, and all it takes are those two arguments, then we may lose it. That's the main reason I added the char* into "Failed", just to get us a bit more information. Or if HandlError was a macro, we could pick up __LINE__ or something like that.
Reporter | ||
Comment 13•10 years ago
|
||
This isn't happening in post-36, so I'm closing the bug as WFM.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → WORKSFORME
You need to log in
before you can comment on or make changes to this bug.
Description
•