1172811 - xul!mozilla::layers::CompositorD3D11::UpdateRenderTarget Failed with large window

Status: RESOLVED FIXED

(Core :: Graphics, defect, P3)

Description

[SkyLined]

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:38.0) Gecko/20100101 Firefox/38.0
Build ID: 20150525141253

Steps to reproduce:

1) Allow popups.
2) Execute this JavaScript:

  window.open("about:blank", "_blank", "left:0").resizeTo(0x1FFF0000,0x1FFF0000);




Actual results:

Int3 in xul!mozilla::layers::CompositorD3D11::HandleError+0x26:

15e8f50c 6aa9b39d xul!mozilla::layers::CompositorD3D11::HandleError(
			HRESULT hr = 0x887a0001, 
			mozilla::layers::CompositorD3D11::Severity aSeverity = 0n1789506461 (No matching enumerant))+0x26 [c:\builds\moz2_slave\rel-m-rel-w32_bld-000000000000\build\gfx\layers\d3d11\compositord3d11.cpp @ 1474]
15e8f518 6aa9bae8 xul!mozilla::layers::CompositorD3D11::Failed(
			HRESULT hr = 0x887a0001, 
			mozilla::layers::CompositorD3D11::Severity aSeverity = 0n1789508328 (No matching enumerant))+0xa [c:\builds\moz2_slave\rel-m-rel-w32_bld-000000000000\build\gfx\layers\d3d11\compositord3d11.cpp @ 1486]
15e8f600 6aa99655 xul!mozilla::layers::CompositorD3D11::UpdateRenderTarget(void)+0x8c [c:\builds\moz2_slave\rel-m-rel-w32_bld-000000000000\build\gfx\layers\d3d11\compositord3d11.cpp @ 1276]
15e8f6a4 6a1c896a xul!mozilla::layers::CompositorD3D11::BeginFrame(
			class nsIntRegion * aInvalidRegion = 0x15e8f838, 
			struct mozilla::gfx::RectTyped<mozilla::gfx::UnknownUnits> * aClipRectIn = 0x00000000, 
			struct mozilla::gfx::RectTyped<mozilla::gfx::UnknownUnits> * aRenderBounds = 0x15e8f730, 
			struct mozilla::gfx::RectTyped<mozilla::gfx::UnknownUnits> * aClipRectOut = 0x15e8f820, 
			struct mozilla::gfx::RectTyped<mozilla::gfx::UnknownUnits> * aRenderBoundsOut = 0x15e8f704)+0x76 [c:\builds\moz2_slave\rel-m-rel-w32_bld-000000000000\build\gfx\layers\d3d11\compositord3d11.cpp @ 1062]
15e8f794 6a271d99 xul!mozilla::SamplerStackFrameRAII::SamplerStackFrameRAII(
			char * aInfo = 0x15e8f81c "???", 
			js::ProfileEntry::Category aCategory = 0n367589600 (No matching enumerant), 
			unsigned int line = 0x24463670)+0x19 [c:\builds\moz2_slave\rel-m-rel-w32_bld-000000000000\build\obj-firefox\dist\include\geckoprofilerimpl.h @ 375]
15e8f850 6a271a64 xul!mozilla::layers::LayerManagerComposite::ApplyOcclusionCulling(
			class mozilla::layers::Layer * aLayer = 0x15e8f8e0, 
			class nsIntRegion * aOpaqueRegion = 0x21bc95a8)+0x2ac [c:\builds\moz2_slave\rel-m-rel-w32_bld-000000000000\build\gfx\layers\composite\layermanagercomposite.cpp @ 243]
15e8f8c4 6a271add xul!mozilla::layers::LayerManagerComposite::EndTransaction(
			<function> * aCallback = <Memory access error>, 
			void * aCallbackData = <Memory access error>, 
			mozilla::layers::LayerManager::EndTransactionFlags aFlags = <Memory access error>)+0x129 [c:\builds\moz2_slave\rel-m-rel-w32_bld-000000000000\build\gfx\layers\composite\layermanagercomposite.cpp @ 312]
15e8f8d4 6a4ba706 xul!mozilla::layers::LayerManagerComposite::EndEmptyTransaction(
			mozilla::layers::LayerManager::EndTransactionFlags aFlags = END_DEFAULT (0n0))+0xf [c:\builds\moz2_slave\rel-m-rel-w32_bld-000000000000\build\gfx\layers\composite\layermanagercomposite.cpp @ 258]
15e8f928 6a4ba025 xul!mozilla::layers::CompositorParent::CompositeToTarget(
			class mozilla::gfx::DrawTarget * aTarget = 0x00000000, 
			struct nsIntRect * aRect = 0x00000000)+0xf5 [c:\builds\moz2_slave\rel-m-rel-w32_bld-000000000000\build\gfx\layers\ipc\compositorparent.cpp @ 986]
15e8f960 6a4b9f5f xul!mozilla::layers::CompositorParent::CompositeCallback(
			class mozilla::TimeStamp aScheduleTime = class mozilla::TimeStamp)+0x43 [c:\builds\moz2_slave\rel-m-rel-w32_bld-000000000000\build\gfx\layers\ipc\compositorparent.cpp @ 901]
15e8f988 6a32eb5b xul!RunnableMethod<mozilla::layers::CompositorParent,void (void)+0x22 [c:\builds\moz2_slave\rel-m-rel-w32_bld-000000000000\build\ipc\chromium\src\base\task.h @ 311]
15e8f9bc 6a32dd6c xul!MessageLoop::DoWork(void)+0xd4 [c:\builds\moz2_slave\rel-m-rel-w32_bld-000000000000\build\ipc\chromium\src\base\message_loop.cc @ 447]
15e8f9ec 6a32c268 xul!base::MessagePumpForUI::DoRunLoop(void)+0x49 [c:\builds\moz2_slave\rel-m-rel-w32_bld-000000000000\build\ipc\chromium\src\base\message_pump_win.cc @ 217]
15e8fa0c 6a32c370 xul!base::MessagePumpWin::RunWithDispatcher(
			class base::MessagePump::Delegate * delegate = 0x15e8fa9c, 
			class base::MessagePumpWin::Dispatcher * dispatcher = 0x15e8fa50)+0x31 [c:\builds\moz2_slave\rel-m-rel-w32_bld-000000000000\build\ipc\chromium\src\base\message_pump_win.cc @ 56]
15e8fa18 6a32efb3 xul!base::MessagePumpWin::Run(
			class base::MessagePump::Delegate * delegate = 0x15e8fa9c)+0xa [c:\builds\moz2_slave\rel-m-rel-w32_bld-000000000000\build\ipc\chromium\src\base\message_pump_win.h @ 78]
15e8fa50 6a32edc7 xul!MessageLoop::RunHandler(void)+0x20 [c:\builds\moz2_slave\rel-m-rel-w32_bld-000000000000\build\ipc\chromium\src\base\message_loop.cc @ 227]
15e8fa70 6a7693b2 xul!MessageLoop::Run(void)+0x19 [c:\builds\moz2_slave\rel-m-rel-w32_bld-000000000000\build\ipc\chromium\src\base\message_loop.cc @ 201]
15e8fb70 77c9ad1f xul!base::Thread::ThreadMain+0x2b044e [c:\builds\moz2_slave\rel-m-rel-w32_bld-000000000000\build\ipc\chromium\src\base\thread.cc @ 173]
15e8fbb8 77c9acea ntdll_77c40000!__RtlUserThreadStart+0x2f
15e8fbc8 00000000 ntdll_77c40000!_RtlUserThreadStart+0x1b




Expected results:

No crash.

Comment 2

[Bas Schouten (:bas.schouten)]

Veeery unlikely to be a dupe, but I'll fix this one and we'll see.

Comment 3

[Bas Schouten (:bas.schouten)]

This JS code -does- crash for me, but with a totally different stack.

Comment 4

[Bas Schouten (:bas.schouten)]

Attachment: Do not even attempt to create swap chains larger than the maximum texture size

This seems like an adequate fix, we could somehow make attempts to draw these insane size windows partially but I don't think that's worth it.

Comment 5

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8621102 [details] [diff] [review]
Do not even attempt to create swap chains larger than the maximum texture size

Review of attachment 8621102 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/d3d11/CompositorD3D11.cpp
@@ +1182,5 @@
>  CompositorD3D11::VerifyBufferSize()
>  {
> +  if (mSize.width > GetMaxTextureSize() ||
> +      mSize.height > GetMaxTextureSize()) {
> +    return false;

What does returning false do here? i.e. What does the user see?

Comment 6

[Bas Schouten (:bas.schouten)]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #5)
> Comment on attachment 8621102 [details] [diff] [review]
> Do not even attempt to create swap chains larger than the maximum texture
> size
> 
> Review of attachment 8621102 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/d3d11/CompositorD3D11.cpp
> @@ +1182,5 @@
> >  CompositorD3D11::VerifyBufferSize()
> >  {
> > +  if (mSize.width > GetMaxTextureSize() ||
> > +      mSize.height > GetMaxTextureSize()) {
> > +    return false;
> 
> What does returning false do here? i.e. What does the user see?

In this window.. nothing.. if the window is bigger than 8K (or 16K on modern cards) that's probably fine though, right now we crash. We could try and draw it partially, as I mentioned, but I think that's a dodgy solution.

Comment 7

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8621102 [details] [diff] [review]
Do not even attempt to create swap chains larger than the maximum texture size

Review of attachment 8621102 [details] [diff] [review]:
-----------------------------------------------------------------

Shouldn't we just be restricting the window size to the max texture size using WM_GETMINMAXINFO?

Comment 8

[Bas Schouten (:bas.schouten)]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #7)
> Comment on attachment 8621102 [details] [diff] [review]
> Do not even attempt to create swap chains larger than the maximum texture
> size
> 
> Review of attachment 8621102 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Shouldn't we just be restricting the window size to the max texture size
> using WM_GETMINMAXINFO?

I'm not against that, but I still think this is a good idea.

Comment 9

[Jeff Muizelaar [:jrmuizel]]

(In reply to Bas Schouten (:bas.schouten) from comment #8)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #7)
> > Comment on attachment 8621102 [details] [diff] [review]
> > Do not even attempt to create swap chains larger than the maximum texture
> > size
> > 
> > Review of attachment 8621102 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Shouldn't we just be restricting the window size to the max texture size
> > using WM_GETMINMAXINFO?
> 
> I'm not against that, but I still think this is a good idea.

Can we do the other bit first?

Comment 10

[Bas Schouten (:bas.schouten)]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #9)
> (In reply to Bas Schouten (:bas.schouten) from comment #8)
> > (In reply to Jeff Muizelaar [:jrmuizel] from comment #7)
> > > Comment on attachment 8621102 [details] [diff] [review]
> > > Do not even attempt to create swap chains larger than the maximum texture
> > > size
> > > 
> > > Review of attachment 8621102 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > Shouldn't we just be restricting the window size to the max texture size
> > > using WM_GETMINMAXINFO?
> > 
> > I'm not against that, but I still think this is a good idea.
> 
> Can we do the other bit first?

I don't think we should personally, that seems like a more involved thing to do in widget land with more potentially hard to predict consequences. A change like that should ride the trains in my opinion. This crash simply fixes a crash.

Comment 11

[Jeff Muizelaar [:jrmuizel]]

(In reply to Bas Schouten (:bas.schouten) from comment #10)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #9)
> > (In reply to Bas Schouten (:bas.schouten) from comment #8)
> > > (In reply to Jeff Muizelaar [:jrmuizel] from comment #7)
> > > > Comment on attachment 8621102 [details] [diff] [review]
> > > > Do not even attempt to create swap chains larger than the maximum texture
> > > > size
> > > > 
> > > > Review of attachment 8621102 [details] [diff] [review]:
> > > > -----------------------------------------------------------------
> > > > 
> > > > Shouldn't we just be restricting the window size to the max texture size
> > > > using WM_GETMINMAXINFO?
> > > 
> > > I'm not against that, but I still think this is a good idea.
> > 
> > Can we do the other bit first?
> 
> I don't think we should personally, that seems like a more involved thing to
> do in widget land with more potentially hard to predict consequences. A
> change like that should ride the trains in my opinion. This crash simply
> fixes a crash.

But isn't Firefox still unusable with this patch?

Comment 12

[Bas Schouten (:bas.schouten)]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #11)
> (In reply to Bas Schouten (:bas.schouten) from comment #10)
> > (In reply to Jeff Muizelaar [:jrmuizel] from comment #9)
> > > (In reply to Bas Schouten (:bas.schouten) from comment #8)
> > > > (In reply to Jeff Muizelaar [:jrmuizel] from comment #7)
> > > > > Comment on attachment 8621102 [details] [diff] [review]
> > > > > Do not even attempt to create swap chains larger than the maximum texture
> > > > > size
> > > > > 
> > > > > Review of attachment 8621102 [details] [diff] [review]:
> > > > > -----------------------------------------------------------------
> > > > > 
> > > > > Shouldn't we just be restricting the window size to the max texture size
> > > > > using WM_GETMINMAXINFO?
> > > > 
> > > > I'm not against that, but I still think this is a good idea.
> > > 
> > > Can we do the other bit first?
> > 
> > I don't think we should personally, that seems like a more involved thing to
> > do in widget land with more potentially hard to predict consequences. A
> > change like that should ride the trains in my opinion. This crash simply
> > fixes a crash.
> 
> But isn't Firefox still unusable with this patch?

One window that's 4 times as big as your screen will be unusable, it's unlikely you were going to use that window in a particularly productive way anyway :).

Comment 13

[u279076]

Looks like this patch has been pending review for over a year. Is this bug still valid?

Comment 14

[SkyLined]

FWIW: it no longer reproduces for me.