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

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?

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?

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.

(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 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

(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.

(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.

(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.

Yeah, that makes sense. It feels a bit smelly to add it to Logging.h anyway... :)

(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

Sorry, an int32_t.

(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.

This isn't happening in post-36, so I'm closing the bug as WFM.