1292774 - Intermittent dom/media/tests/mochitest/test_peerConnection_captureStream_canvas_webgl.html | application crashed [@ mozilla::gfx::Log<1,mozilla::gfx::CriticalLogger>::WriteLog(std::basic_string<char,std::char_traits<char>,std::allocator<char> > const &)]

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Treeherder Bug Filer]

Filed by: phil [at] philringnalda.com

https://treeherder.mozilla.org/logviewer.html#?job_id=4532526&repo=mozilla-central

http://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-central-win64-debug/1470035603/mozilla-central_win8_64-debug_test-mochitest-media-bm126-tests1-windows-build10.txt.gz

Comment 1

[Nils Ohlmeier [:drno]]

Looks like WebRTC test waiting for a red pixel to show up severely upset the Gfx folks in this case. Crashes here:
https://dxr.mozilla.org/mozilla-central/rev/6b65dd49d4f045c0a9753ce60bdb4b7b4aaedcf8/gfx/2d/Logging.h#510

Therefore I'm guessing this is actually something the Gfx folks should have a look at.

Comment 2

[Nicolas Silva [:nical]]

The failures comes from a D3D11 compositor still in use after a device reset. It's not clear to me when exactly we change the DeviceManager's D3D11 device and whether we are suppose to make sure to replace the compositor beforehand. Could be that we are about to reset the compositor in the next frame, still have a few commands to run on the old one but have already registered a new device in the DeviceManager? Otherwise it means we have a compositor with an outdated device lingering, which should have been replaced.

ni? Milan, you added this assertion.

Comment 3

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

David and Jerry understand these restarts the best these days, but this is usually bad news.

Comment 4

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

If this is new, I wouldn't be surprised if it was tickled by patches on bug 1167235, mostly because of the "trying to lock a TextureHost without a D3D device" message that precedes the assert, and that bug changing some of the timing around locking...

Comment 5

[Jerry Shih[:jerry] (UTC+8) (inactive)]

 02:33:00     INFO -  [GFX1-]: D3D11 swap chain preset failed 0x887a0005
 02:33:00     INFO -  [GFX1]: Out of sync D3D11 devices in HandleError, 0
 02:33:01     INFO -  Assertion failure: [GFX1]: Out of sync D3D11 devices in HandleError, 0, at c:\builds\moz2_slave\m-cen-w64-d-000000000000000000\build\src\obj-firefox\dist\include\mozilla/gfx/Logging.h:509
 02:33:13     INFO -  #01: mozilla::gfx::Log<1,mozilla::gfx::CriticalLogger>::Flush() [obj-firefox/dist/include/mozilla/gfx/Logging.h:278]
 02:33:13     INFO -  #02: mozilla::layers::CompositorD3D11::HandleError(long,mozilla::layers::CompositorD3D11::Severity) [gfx/layers/d3d11/CompositorD3D11.cpp:1713]
 02:33:13     INFO -  #03: mozilla::layers::CompositorD3D11::EndFrame() [gfx/layers/d3d11/CompositorD3D11.cpp:1314]

We should update the HandleError(). The d3dswapchain->Present() call could return device_remove/reset error. I think the assert is not necessary.

Comment 6

[Jerry Shih[:jerry] (UTC+8) (inactive)]

(In reply to Jerry Shih[:jerry] (UTC+8) from comment #5)
>  02:33:00     INFO -  [GFX1-]: D3D11 swap chain preset failed 0x887a0005

0x887a0005 => DXGI_ERROR_DEVICE_REMOVED

https://msdn.microsoft.com/zh-tw/library/windows/desktop/bb509553(v=vs.85).aspx

Comment 7

[Jerry Shih[:jerry] (UTC+8) (inactive)]

The crash is at compositor thread with:

  if (mDevice && DeviceManagerD3D11::Get()->GetCompositorDevice() != mDevice) {
    gfxCriticalError() << "Out of sync D3D11 devices in HandleError, " << (int)mVerifyBuffersFailed;
  }


-----
From refresh driver at main thread, the reset flow paths are:
nsRefreshDriver::Tick()
  gfxPlatform::GetPlatform()->SchedulePaintIfDeviceReset()
    InvalidateWindowForDeviceReset() // call OnPaint for each window
      nsWindow::OnPaint()
        gfxWindowsPlatform::GetPlatform()->UpdateRenderMode();  // re-create d3d and d2d device
        nsBaseWidget::OnRenderingDeviceReset()
          CompositorBridgeParent::ResetCompositor() // schedule ResetCompositorTask, and wait it done.


Even though the CompositorBridgeParent::ResetCompositor() call is synchronous(ResetCompositor() will update the d3d device in compositor), gecko still can do the composition during UpdateRenderMode().
Then the "DeviceManagerD3D11::Get()->GetCompositorDevice()" might be different from mDevice.

Comment 8

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: turn to use gfxCriticalNote for mismatched d3d11 device in CompositorD3D11::HandleError(). v1

David, what do you think for comment 7?

Comment 9

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to Jerry Shih[:jerry] (UTC+8) from comment #8)
> Created attachment 8779235 [details] [diff] [review]
> turn to use gfxCriticalNote for mismatched d3d11 device in
> CompositorD3D11::HandleError(). v1
> 
> David, what do you think for comment 7?

I think we have to change calls like this to be gfxCriticalNotes instead, and make sure our code can handle the undesirable case instead of asserting.

I'm not sure this message is worth preserving though. It can always race because we revoke the device without locking the compositor. I don't think we want to change that. So I think it'd be okay to just delete the whole check, but if you'd rather just loosen the assert that's fine too.

Comment 10

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Please land the attachment 8779235 [details] [diff] [review] to m-c.
Just a trivial change, so there is no try.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d1392c28c8d
Turn to use gfxCriticalNote for mismatched d3d11 device in CompositorD3D11::HandleError(). r=dvander

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1d1392c28c8d

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Can you please request approval to land this on Aurora as well?

Comment 14

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8779235 [details] [diff] [review]
turn to use gfxCriticalNote for mismatched d3d11 device in CompositorD3D11::HandleError(). v1

Approval Request Comment
[Feature/regressing bug #]:
Hit assert like:
https://treeherder.mozilla.org/logviewer.html#?job_id=4532526&repo=mozilla-central#L13228

[User impact if declined]:
Before this patch, gecko will just hit assert if it is under driver-reset/remove status. This patch try to remove the assert and just show it as a gfx error message in crash report.

[Describe test coverage new/current, TreeHerder]:
It's already in ff51, and I don't see another crash-report for this assert in ff51.

[Risks and why]:
Low.

For comment 9, it's easy to hit this assertion during driver remove/reset. And gecko will handle the driver remove/reset in other path. Maybe we could just remove this assertion, but I still like to preserve this information. So, this patch try to use gfxCriticalNote instead of gfxCriticalError. gfxCriticalNote will just show the message in crash-report without assert.

[String/UUID change made/needed]:
none

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8779235 [details] [diff] [review]
turn to use gfxCriticalNote for mismatched d3d11 device in CompositorD3D11::HandleError(). v1

Fixes an intermittent failure, has been on Nightly for over a month, Aurora50+

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d2dfb7d2fe1a