1237375 - When swap chain ResizeBuffers fails because there are still lingering references to the back buffer/render target view

Status: RESOLVED WORKSFORME

(Core :: Graphics, defect, P3)

Description

[Milan Sreckovic [:milan] (needinfo for best results)]

As a follow up to bug 1229533, we left a notification if there are lingering references that would stop us from resizing the buffer.  We're hitting those:

https://crash-stats.mozilla.com/report/index/4fe1eb13-257b-48bc-a74a-210292151231
https://crash-stats.mozilla.com/report/index/c6054079-6720-47af-9788-aadd62160101
https://crash-stats.mozilla.com/report/index/80b39ca0-a19a-414c-9181-63b742160101
https://crash-stats.mozilla.com/report/index/4c536e18-79de-49a6-aa8f-d30db2151231
https://crash-stats.mozilla.com/report/index/8aad2ab2-2e71-4d7d-833b-7ed062160102
etc.

Comment 1

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Part 1. Show the value of ref count when > 0, for extra info. r=bas

Not a fix, just some more data.

Comment 2

[Bas Schouten (:bas.schouten)]

Attachment: MozReview Request: Bug 1237375: Add some more extensive debugging information. r=milan

Review commit: https://reviewboard.mozilla.org/r/29961/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/29961/

Comment 3

[Bas Schouten (:bas.schouten)]

(In reply to Bas Schouten (:bas.schouten) from comment #2)
> Created attachment 8705394 [details]
> MozReview Request: Bug 1237375: Add some more extensive debugging
> information. r=milan
> 
> Review commit: https://reviewboard.mozilla.org/r/29961/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/29961/

This checks something more which shouldn't really ever happen.. but I still want to make sure it doesn't...

Comment 4

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8705394 [details]
MozReview Request: Bug 1237375: Add some more extensive debugging information. r=milan

Bas, can you also land my patch?

Comment 5

[Bas Schouten (:bas.schouten)]

(In reply to Milan Sreckovic [:milan] from comment #4)
> Comment on attachment 8705394 [details]
> MozReview Request: Bug 1237375: Add some more extensive debugging
> information. r=milan
> 
> Bas, can you also land my patch?

This patch essentially includes your patch :).

Comment 6

[Bas Schouten (:bas.schouten)]

(In reply to Milan Sreckovic [:milan] from comment #4)
> Comment on attachment 8705394 [details]
> MozReview Request: Bug 1237375: Add some more extensive debugging
> information. r=milan
> 
> Bas, can you also land my patch?

Psst.. this doesn't r+ it in reviewboard ;-)

Comment 7

[Milan Sreckovic [:milan] (needinfo for best results)]

https://reviewboard.mozilla.org/r/29961/#review26821

Comment 8

[Milan Sreckovic [:milan] (needinfo for best results)]

I was forgetting the "publish" step.  Should be OK now.

Comment 9

[Pulsebot]

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

Comment 10

[Carsten Book [:Tomcat]]

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

Comment 11

[Milan Sreckovic [:milan] (needinfo for best results)]

Didn't mean to close this after the diagnostic patches, sorry for not tagging it as leave-open.

Comment 12

[u279076]

It's been several months, have the diagnostic patches gotten us any closer to a solution yet?

Comment 13

[Milan Sreckovic [:milan] (needinfo for best results)]

Confirming the cause, perhaps more than getting us closer to a solution.  The volume of crashes is very small (22)

A recent example: https://crash-stats.mozilla.com/report/index/19943013-f065-46ac-9c05-c2be02160807 has us with the "Unexpecting lingering references to backbuffer! RefCnt: 3" message, as the first error message of the run (so, no earlier messages about lost devices, etc.)  Note that there are no messages about mRTView or mSRV not destroyed on the final release in this (and similar) cases.

There is a single release on 47: https://crash-stats.mozilla.com/report/index/d05420ae-e2e4-494d-a362-197342160812 which is a different stack, but it does have the message "mRTView not destroyed on final release! RefCnt: 1" (and no others.)  Which means that it is (or was) possible to get into that situation.  We can probably ignore this until it happens again.

Comment 14

[Milan Sreckovic [:milan] (needinfo for best results)]

For the first crash in the comment above:

  hr = mSwapChain->GetDesc(&swapDesc); <--- this succeeds
  ...

  ID3D11RenderTargetView* view = nullptr;
  mContext->OMSetRenderTargets(1, &view, nullptr);

  if (mDefaultRT) { <--- mDefaultRT is not null
    RefPtr<ID3D11RenderTargetView> rtView = mDefaultRT->mRTView;
    RefPtr<ID3D11ShaderResourceView> srView = mDefaultRT->mSRV;

    if (mCurrentRT == mDefaultRT) {
      mCurrentRT = nullptr;
    }
    MOZ_ASSERT(mDefaultRT->hasOneRef());
    mDefaultRT = nullptr;

    RefPtr<ID3D11Resource> resource;
    rtView->GetResource(getter_AddRefs(resource)); <--- this is where we set "resource"

    ULONG newRefCnt = rtView.forget().take()->Release(); <--- this returns 0

    if (srView) {
      newRefCnt = srView.forget().take()->Release(); <--- if this ran, it returned 0
    }

    newRefCnt = resource.forget().take()->Release(); <--- this returns 3
    ...


Bas, what does this mean?

Comment 15

[Milan Sreckovic [:milan] (needinfo for best results)]

This one: https://crash-stats.mozilla.com/report/index/a0778c7a-2db1-4ec5-93e9-70cd72160808 has the reference count (that we expect to be zero) at 8 (the report suggests some kind of OOM, but is missing information.)

Comment 16

[Bas Schouten (:bas.schouten)]

(In reply to Milan Sreckovic [:milan] from comment #14)
> For the first crash in the comment above:
> 
>   hr = mSwapChain->GetDesc(&swapDesc); <--- this succeeds
>   ...
> 
>   ID3D11RenderTargetView* view = nullptr;
>   mContext->OMSetRenderTargets(1, &view, nullptr);
> 
>   if (mDefaultRT) { <--- mDefaultRT is not null
>     RefPtr<ID3D11RenderTargetView> rtView = mDefaultRT->mRTView;
>     RefPtr<ID3D11ShaderResourceView> srView = mDefaultRT->mSRV;
> 
>     if (mCurrentRT == mDefaultRT) {
>       mCurrentRT = nullptr;
>     }
>     MOZ_ASSERT(mDefaultRT->hasOneRef());
>     mDefaultRT = nullptr;
> 
>     RefPtr<ID3D11Resource> resource;
>     rtView->GetResource(getter_AddRefs(resource)); <--- this is where we set
> "resource"
> 
>     ULONG newRefCnt = rtView.forget().take()->Release(); <--- this returns 0
> 
>     if (srView) {
>       newRefCnt = srView.forget().take()->Release(); <--- if this ran, it
> returned 0
>     }
> 
>     newRefCnt = resource.forget().take()->Release(); <--- this returns 3
>     ...
> 
> 
> Bas, what does this mean?

It essentially means 'something' directly grabbed a reference to the backbuffer. I don't -think- we ever do this on purpose.

We grab a reference to the backbuffer here: https://dxr.mozilla.org/mozilla-central/source/gfx/layers/d3d11/CompositorD3D11.cpp#1492 and use a smartpointer.

That is handed over to CompositingRenderTarget and this creates rtView and srView for it:

https://dxr.mozilla.org/mozilla-central/source/gfx/layers/d3d11/TextureD3D11.cpp#1095

After that we no longer have any pointer in our code to the backbuffer directly as far as I can tell.

Our debug output suggests we are successfully destroying all those. In other words whatever reference to the backbuffer still exists must be something D3D grabbed internally somehow. This could be something we're doing or a bug on their side. One thing we could try is making sure all devices are flushed at this point? I don't see how that would help but who knows..

Comment 17

[Bas Schouten (:bas.schouten)]

I should probably note in addition to that, the only way for us to get the buffer pointer back out of the RTView is the GetResource call. As far as I can tell we only ever call that here: https://dxr.mozilla.org/mozilla-central/source/gfx/layers/d3d11/CompositorD3D11.cpp#1434

Comment 18

[Milan Sreckovic [:milan] (needinfo for best results)]

This one: https://crash-stats.mozilla.com/report/index/76a80aae-5460-4b83-92fa-5cd612161012 hits the "mRTView not destroyed on final release" with the reference count 1 multiple times before crashing in the driver (call to DXGID3D9TextureData::~DXGID3D9TextureData)

Comment 19

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:bas.schouten, maybe it's time to close this bug?

Comment 20

[Bas Schouten (:bas.schouten)]

We don't use a lot of this code by default anymore with advanced layers anyway.