571507 - Crash [@ mozilla::layers::ContainerLayerD3D9::RenderLayer() ]

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[bogas04]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a6pre) Gecko/20100611 Firefox 3.6.3
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a6pre) Gecko/20100611 Firefox 3.6.3

more info @ http://crash-stats.mozilla.com/report/index/bp-54bcb8dd-db0b-4f1c-910b-053cd2100611

I really don't know what it is , but it crashed on me when i clicked the aero frame after viewing WebM video , first i thought it was WebM problem , but then seeing about:crashes i saw this D3D9 Layers thing. Sorry im not novice at all this , but there is really no related bug , so filing this

Reproducible: Always

Steps to Reproduce:
Clicking aero frame with maybe Direct2D enabled
Actual Results:  
Crash (Not always)

Expected Results:  
No crashes

Comment 1

[[not reading bugmail]]

The crash stat shows it dropped off here:

http://hg.mozilla.org/mozilla-central/annotate/b84d0be52070/gfx/layers/d3d9/ContainerLayerD3D9.cpp#l163

Comment 2

[bogas04]

So does this mean its fixed?

Comment 3

[Scoobidiver (away)]

This crash signature is back since b6pre/20100906 build.The crash daily rate is about 1-7 crashes/day.It happens with every kind of graphic cards.For me, clicking on aero frames does not crash FF.Signature	mozilla::layers::ContainerLayerD3D9::RenderLayer()UUID	62c52c3e-7116-4ba1-aad2-7b6c12100920Time 	2010-09-20 00:12:39.91841Uptime	533Last Crash	239062 seconds (2.8 days) before submissionInstall Age	533 seconds (8.9 minutes) since version was first installed.Product	FirefoxVersion	4.0b7preBuild ID	20100919042023Branch	2.0OS	Windows NTOS Version	6.1.7600CPU	x86CPU Info	GenuineIntel family 6 model 15 stepping 6Crash Reason	EXCEPTION_ACCESS_VIOLATION_READCrash Address	0x0User Comments	App Notes 	AdapterVendorID: 8086, AdapterDeviceID: 29a2Crashing ThreadFrame 	Module 	Signature [Expand] 	Source0 	xul.dll 	mozilla::layers::ContainerLayerD3D9::RenderLayer 	gfx/layers/d3d9/ContainerLayerD3D9.cpp:1591 	xul.dll 	mozilla::layers::ContainerLayerD3D9::RenderLayer 	gfx/layers/d3d9/ContainerLayerD3D9.cpp:2212 	xul.dll 	mozilla::layers::ContainerLayerD3D9::RenderLayer 	gfx/layers/d3d9/ContainerLayerD3D9.cpp:2213 	xul.dll 	mozilla::layers::LayerManagerD3D9::Render 	gfx/layers/d3d9/LayerManagerD3D9.cpp:2914 	xul.dll 	mozilla::layers::LayerManagerD3D9::EndTransaction 	gfx/layers/d3d9/LayerManagerD3D9.cpp:1595 	xul.dll 	nsDisplayList::PaintForFrame 	layout/base/nsDisplayList.cpp:4526 	xul.dll 	nsLayoutUtils::PaintFrame 	layout/base/nsLayoutUtils.cpp:14297 	xul.dll 	PresShell::Paint 	layout/base/nsPresShell.cpp:60898 	xul.dll 	nsViewManager::RenderViews 	view/src/nsViewManager.cpp:4479 	xul.dll 	nsViewManager::Refresh 	view/src/nsViewManager.cpp:41310 	xul.dll 	nsViewManager::DispatchEvent 	view/src/nsViewManager.cpp:91311 	xul.dll 	AttachedHandleEvent 	view/src/nsView.cpp:19312 	xul.dll 	nsWindow::DispatchEvent 	widget/src/windows/nsWindow.cpp:355613 	xul.dll 	nsWindow::DispatchWindowEvent 	widget/src/windows/nsWindow.cpp:358714 		@0x4330e3f

Comment 4

[Scoobidiver (away)]

Sorry for comment 3 formatting, but Bugzilla removes end of line of the current comment when product and component are also changed.

This crash signature is back since b6pre/20100906 build.
The crash daily rate is about 1-7 crashes/day.
It happens with every kind of graphic cards.
For me, clicking on aero frames does not crash FF.

Signature   mozilla::layers::ContainerLayerD3D9::RenderLayer()
UUID     62c52c3e-7116-4ba1-aad2-7b6c12100920
Time     2010-09-20 00:12:39.91841
Uptime   533
Last Crash    239062 seconds (2.8 days) before submission
Install Age    533 seconds (8.9 minutes) since version was first installed.
Product   Firefox
Version    4.0b7pre
Build ID    20100919042023
Branch    2.0
OS    Windows NT
OS Version    6.1.7600
CPU    x86
CPU Info    GenuineIntel family 6 model 15 stepping 6
Crash Reason    EXCEPTION_ACCESS_VIOLATION_READ
Crash Address  0x0
AdapterVendorID: 8086, AdapterDeviceID: 29a2

Crashing Thread
Frame     Module     Signature [Expand]     Source
0    xul.dll     mozilla::layers::ContainerLayerD3D9::RenderLayer    
gfx/layers/d3d9/ContainerLayerD3D9.cpp:159
1     xul.dll    
mozilla::layers::ContainerLayerD3D9::RenderLayer    
gfx/layers/d3d9/ContainerLayerD3D9.cpp:221
2     xul.dll    
mozilla::layers::ContainerLayerD3D9::RenderLayer    
gfx/layers/d3d9/ContainerLayerD3D9.cpp:221
3     xul.dll    
mozilla::layers::LayerManagerD3D9::Render    
gfx/layers/d3d9/LayerManagerD3D9.cpp:291
4   xul.dll    
mozilla::layers::LayerManagerD3D9::EndTransaction    
gfx/layers/d3d9/LayerManagerD3D9.cpp:159
5     xul.dll    
nsDisplayList::PaintForFrame     layout/base/nsDisplayList.cpp:452
6     xul.dll
    nsLayoutUtils::PaintFrame     layout/base/nsLayoutUtils.cpp:1429
7    xul.dll     PresShell::Paint     layout/base/nsPresShell.cpp:6089
8     xul.dll 
   nsViewManager::RenderViews     view/src/nsViewManager.cpp:447
9     xul.dll  
  nsViewManager::Refresh     view/src/nsViewManager.cpp:413
10     xul.dll    
nsViewManager::DispatchEvent     view/src/nsViewManager.cpp:913
11     xul.dll  
  AttachedHandleEvent     view/src/nsView.cpp:193
12     xul.dll    
nsWindow::DispatchEvent     widget/src/windows/nsWindow.cpp:3556
13     xul.dll 
   nsWindow::DispatchWindowEvent     widget/src/windows/nsWindow.cpp:3587
14     @0x4330e3f

Comment 5

[Kelsey Gilbert [:jgilbert]]

Current location on MXR of "renderTexture->GetSurfaceLevel(0, getter_AddRefs(renderSurface));" is http://mxr.mozilla.org/mozilla-central/source/gfx/layers/d3d9/ContainerLayerD3D9.cpp#188

I don't know much about d3d, but could it be an instance where the texture fails to be created properly, then has GetSurfaceLevel called on it?

Comment 6

[Kelsey Gilbert [:jgilbert]]

Attachment: Adds checks for failure after CreateTexture(2D) in D3D9/10 layers code

Try: https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=d8114437557b

Comment 7

[Kelsey Gilbert [:jgilbert]]

Attachment: Adds checks for failure after CreateTexture(2D) in D3D9/10 layers code

Messed up an indent.

Comment 8

[Seth Spitzer]

Jeff / Bas:

Do you think we also need a check for failures here?

http://mxr.mozilla.org/mozilla-central/source/gfx/layers/d3d10/LayerManagerD3D10.cpp#599

579 void
580 LayerManagerD3D10::UpdateRenderTarget()
581 {
582   if (mRTView) {
583     return;
584   }
585 
586   HRESULT hr;
587 
588   nsRefPtr<ID3D10Texture2D> backBuf;
589   
590   if (mSwapChain) {
591     hr = mSwapChain->GetBuffer(0, __uuidof(ID3D10Texture2D), (void**)backBuf.StartAssignment());
592     if (FAILED(hr)) {
593       return;
594     }
595   } else {
596     backBuf = mBackBuffer;
597   }
598   
599   mDevice->CreateRenderTargetView(backBuf, NULL, getter_AddRefs(mRTView));
600 }


http://mxr.mozilla.org/mozilla-central/source/gfx/layers/d3d10/ImageLayerD3D10.cpp#490

490   mDevice->CreateTexture2D(&texDesc, NULL, getter_AddRefs(surfTexture));

Thanks,

-Seth

Comment 9

[Bas Schouten (:bas.schouten)]

Comment on attachment 565695 [details] [diff] [review]
Adds checks for failure after CreateTexture(2D) in D3D9/10 layers code

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

You're using nsDependentCString in D3D10, and NS_LITERAL_STRING in D3D9, any particular reason? I never quite understand our string logic but I think the latter is more appropriate.

::: gfx/layers/d3d10/ContainerLayerD3D10.cpp
@@ +212,5 @@
> +    if (FAILED(hr)) {
> +      LayerManagerD3D10::ReportFailure(nsDependentCString("Failed to create render target view for immediate ContainerLayerD3D10::RenderLayer"),
> +                                       hr);
> +      return;
> +    }

I don't think we need to check the result of CreateRenderTarget view. If the texture is valid this will succeed.

::: gfx/layers/d3d9/ImageLayerD3D9.cpp
@@ +459,5 @@
> +                                  getter_AddRefs(tmpCbTexture), NULL);
> +    if (!FAILED(hr))
> +      hr = aDevice->CreateTexture(mData.mCbCrSize.width, mData.mCbCrSize.height,
> +                                  1, 0, D3DFMT_L8, D3DPOOL_SYSTEMMEM,
> +                                  getter_AddRefs(tmpCrTexture), NULL);

nit: I know it's ugly here, but { } should still apply :) Especially in this multi-line case.

Comment 10

[Kelsey Gilbert [:jgilbert]]

(In reply to Bas Schouten (:bas) from comment #9)
> Comment on attachment 565695 [details] [diff] [review] [diff] [details] [review]
> Adds checks for failure after CreateTexture(2D) in D3D9/10 layers code
> 
> Review of attachment 565695 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> You're using nsDependentCString in D3D10, and NS_LITERAL_STRING in D3D9, any
> particular reason? I never quite understand our string logic but I think the
> latter is more appropriate.

I don't have a good grip on it either, unfortunately. I think I did it because nearby code was written as such, but looking at it again, I'm not really seeing that. I'll upload one with just NS_LITERAL_STRINGs.
> 
> ::: gfx/layers/d3d10/ContainerLayerD3D10.cpp
> @@ +212,5 @@
> > +    if (FAILED(hr)) {
> > +      LayerManagerD3D10::ReportFailure(nsDependentCString("Failed to create render target view for immediate ContainerLayerD3D10::RenderLayer"),
> > +                                       hr);
> > +      return;
> > +    }
> 
> I don't think we need to check the result of CreateRenderTarget view. If the
> texture is valid this will succeed.

Ok, will remove.

> ::: gfx/layers/d3d9/ImageLayerD3D9.cpp
> @@ +459,5 @@
> > +                                  getter_AddRefs(tmpCbTexture), NULL);
> > +    if (!FAILED(hr))
> > +      hr = aDevice->CreateTexture(mData.mCbCrSize.width, mData.mCbCrSize.height,
> > +                                  1, 0, D3DFMT_L8, D3DPOOL_SYSTEMMEM,
> > +                                  getter_AddRefs(tmpCrTexture), NULL);
> 
> nit: I know it's ugly here, but { } should still apply :) Especially in this
> multi-line case.

True, will fix.

Comment 11

[Kelsey Gilbert [:jgilbert]]

Attachment: Adds checks for failure after CreateTexture(2D) in D3D9/10 layers code

Update reflecting review comments, and fixed a bug with a cluster of missing HRESULT assignments, and added a check as per Seth's comment above.

Comment 12

[Bas Schouten (:bas.schouten)]

Comment on attachment 570909 [details] [diff] [review]
Adds checks for failure after CreateTexture(2D) in D3D9/10 layers code

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

Looks good.

::: gfx/layers/d3d10/ImageLayerD3D10.cpp
@@ +409,5 @@
>    dataCr.pSysMem = mData.mCrChannel;
>    dataCr.SysMemPitch = mData.mCbCrStride;
>  
> +  HRESULT hr = mDevice->CreateTexture2D(&descY, &dataY, getter_AddRefs(mYTexture));
> +  if (!FAILED(hr))  hr = mDevice->CreateTexture2D(&descCbCr, &dataCb, getter_AddRefs(mCbTexture));

I really prefer { }, but if noone else objects feel free to leave this :)

Comment 13

[Kelsey Gilbert [:jgilbert]]

Attachment: Adds checks for failure after CreateTexture(2D) in D3D9/10 layers code

Added brackets as per most recent review, and fixed where I accidentally a return value.

Comment 14

[Kelsey Gilbert [:jgilbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a56d922453c7

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a56d922453c7

Comment 16

[Joe Drew (not getting mail)]

(In reply to Bas Schouten (:bas) from comment #12)
> > +  HRESULT hr = mDevice->CreateTexture2D(&descY, &dataY, getter_AddRefs(mYTexture));
> > +  if (!FAILED(hr))  hr = mDevice->CreateTexture2D(&descCbCr, &dataCb, getter_AddRefs(mCbTexture));
> 
> I really prefer { }, but if noone else objects feel free to leave this :)

Body of the if on the same line as the if itself is always wrong and bad! (You can't set a breakpoint inside the if!)