1280762 - crashes in mozilla::layers::ShmemTextureReadLock::ReadUnlock

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This bug was filed from the Socorro interface and is 
report bp-0135ea2e-6594-44d4-8dcf-1a2a22160618.
=============================================================

There have been a decent number of crashes with signature mozilla::layers::ShmemTextureReadLock::ReadUnlock in the past few weeks.  They started in nightly build 20160603030242, so they appear to likely be a regression from bug 1272600.

It's not a bad topcrash on nightly and aurora, but it seems like there's some risk it could be worse on beta.

So far, all the crashes (32 total) are from D3D11 users on 64-bit Windows (all but 1 on Windows 10; the other 1 on Windows 7).  All crashes are EXCEPTION_ACCESS_VIOLATION_WRITE.

The top of the stack seems to always be:
mozilla::layers::ShmemTextureReadLock::ReadUnlock
mozilla::layers::Compositor::EndFrame
mozilla::layers::CompositorD3D11::EndFrame
mozilla::layers::LayerManagerComposite::Render
mozilla::layers::LayerManagerComposite::UpdateAndRender
mozilla::layers::LayerManagerComposite::EndTransaction
mozilla::layers::CompositorBridgeParent::CompositeToTarget
mozilla::layers::CompositorVsyncScheduler::Composite
nsRunnableMethodImpl<T>::Run

For the full list of crashes, see:
https://crash-stats.mozilla.com/signature/?product=Firefox&date=%3E%3D2016-06-01&signature=mozilla%3A%3Alayers%3A%3AShmemTextureReadLock%3A%3AReadUnlock

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Also, 30 of the 32 crashes have adapter vendor ID 0x1002, adapter device ID 0x6798, adapter subsys ID 32111682, though those crashes are split across 2 driver versions (20 on 16.200.1025.0, 10 on 16.150.2401.1002).  (The two other crashes have different data, and those 2 others include the one Windows 7 crash.)  So it could be a small number of users hitting this.

Comment 2

[Sotaro Ikeda [:sotaro]]

The crash seems same to Bug 1261166 Comment 24. It caused by lifetime of shmem. The lifetime of shmem is bounded to PLayerTransaction ipc now. The lifetime is going to be extended by Bug 1176011 soon. Then the crash seems to be addressed by Bug 1176011.

Comment 3

[Sotaro Ikeda [:sotaro]]

I am not sure if it is easy to uplift bug 1176011 fix to firefox 49.

Comment 4

[Nicolas Silva [:nical]]

(In reply to Sotaro Ikeda [:sotaro] from comment #3)
> I am not sure if it is easy to uplift bug 1176011 fix to firefox 49.

I am looking for a workaround in the mean time.

Comment 5

[Nicolas Silva [:nical]]

Attachment: Ensure textures are ReadUnlocked before the top-level protocol that allocates them is destroyed

This patch makes Compositor hold on to textures rather than locks (to make sure we don't accidentally unlock twice), and forces a ReadUnlock during the destruction of the TextureParent (which always happens before PLayerParent goes away with its shmems).

Comment 6

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8763512 [details] [diff] [review]
Ensure textures are ReadUnlocked before the top-level protocol that allocates them is destroyed

Looks good.

Comment 7

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #2)
> The crash seems same to Bug 1261166 Comment 24. It caused by lifetime of
> shmem. The lifetime of shmem is bounded to PLayerTransaction ipc now. The
> lifetime is going to be extended by Bug 1176011 soon. Then the crash seems
> to be addressed by Bug 1176011.

During checking Bug 1280436, I recognized that the above comment is wrong. It does not address the crash.

A peer of CompositorBridgeChild could be CrossProcessCompositorBridgeParent. Compositor::Destroy() is related to only to CompositorBridgeParent.

Comment 8

[Sotaro Ikeda [:sotaro]]

Then attachment 8763512 [details] [diff] [review] is the only solution to the crash for now.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3d7ba7f672e
ReadUnlock the TextureHost when its IPDL actor dies. r=sotaro

Comment 11

[Carsten Book [:Tomcat]]

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

Comment 12

[Liz Henry (:lizzard) (relman/hg->git project)]

Is this meant as the workaround for 49 (mentioned in comment 4)? Do you want to request uplift? Thanks!

Comment 13

[Nicolas Silva [:nical]]

Comment on attachment 8763512 [details] [diff] [review]
Ensure textures are ReadUnlocked before the top-level protocol that allocates them is destroyed

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: crashes
[Describe test coverage new/current, TreeHerder]: none
[Risks and why]: low: rather simple patch
[String/UUID change made/needed]: none

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8763512 [details] [diff] [review]
Ensure textures are ReadUnlocked before the top-level protocol that allocates them is destroyed

Crash fix for recent regression, taking this for aurora.

Comment 15

[Carsten Book [:Tomcat]]

has problems uplifting to aurora:

grafting 351003:c3d7ba7f672e "Bug 1280762 - ReadUnlock the TextureHost when its IPDL actor dies. r=sotaro"
merging gfx/layers/Compositor.cpp
merging gfx/layers/Compositor.h
merging gfx/layers/composite/TextureHost.cpp
merging gfx/layers/composite/TextureHost.h
warning: conflicts while merging gfx/layers/Compositor.cpp! (edit, then use 'hg resolve --mark')
warning: conflicts while merging gfx/layers/composite/TextureHost.cpp! (edit, then use 'hg resolve --mark')
warning: conflicts while merging gfx/layers/composite/TextureHost.h! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')

Comment 16

[Nicolas Silva [:nical]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3a78197e37b6