988713 - Race condition when destroying TextureClient & TextureChild.

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[John Lin [:jhlin][:jolin]]

Scenario 1: TextureClient::Finalize() and TextureChild::ActorDestroy() running simultaneously.
  ActorDestroy():
    if (mTextureClient) { <-- true
  Finalize():
    actor->mTextureClient = nullptr;
  ActorDestroy():
    mTextureClient->mActor = nullptr; <-- null mTextureClient

Scenrio 2: TextureClient holds the last ref. to TextureChild.
  ActorDestroy():
     mTextureClient->mActor = nullptr; <-- ref count 1 ==> 0. ~TextureChild()
     mWaitForRecycle = nullptr; <-- invalid mWaitForRecycle

Comment 1

[Sotaro Ikeda [:sotaro]]

John, can you explain actual STRs by user to cause this problem?

Comment 2

[u459114]

Apply the patch in bug 911046, crash happened in H264 video call.

Comment 3

[Peter Chang[:pchang]]

(In reply to John Lin[:jolin][:jhlin] from comment #0)
> Scenario 1: TextureClient::Finalize() and TextureChild::ActorDestroy()
> running simultaneously.
>   ActorDestroy():
>     if (mTextureClient) { <-- true
>   Finalize():
>     actor->mTextureClient = nullptr;
>   ActorDestroy():
>     mTextureClient->mActor = nullptr; <-- null mTextureClient
> 
> Scenrio 2: TextureClient holds the last ref. to TextureChild.
>   ActorDestroy():
>      mTextureClient->mActor = nullptr; <-- ref count 1 ==> 0. ~TextureChild()
>      mWaitForRecycle = nullptr; <-- invalid mWaitForRecycle

I'm trying to check scenario 2. 
At the same time, I would like to redefine the lifecycle of tclient and tchild to avoid crash with multithread condition.

Now the tclient and tchild have the reference cycles in their definition. And it is possible that TextureClient::Finalize and TextureChild::ActorDestroy invoked by different threads.
Inside these two functions, they are trying to break the reference cycles.

From the behavior point of view, the tclient's lifecycle should be longer than tchild.

The plan is to add the condition wait inside TextureClient::Finalize[1] to wait the tchild(mActor) and mWaitForRecycle(reference of tclient) destroyed[2]. After TextureClient::Finalize finish, we could make sure tchild is destroyed.

[1] http://dxr.mozilla.org/mozilla-central/source/gfx/layers/client/TextureClient.cpp#510
[2] http://dxr.mozilla.org/mozilla-central/source/gfx/layers/client/TextureClient.cpp#191
AtomicRefCountedWithFinalize::Release->
TextureClient::Finalize->
ShadowLayerForwarder::RemoveTexture->
TextureClient::ForceRemove()->
mActor->SendRemoveTexture()
...cross IPC...
TextureParent::RecvRemoveTexture() or TextureHost::SendDeleteIPDLActor->
PTextureParent::Send__delete__(actor)
...cross IPC...
TextureChild OnMessageReceived with Msg___delete____ID
->(actor)->DestroySubtree(Deletion)
->ActorDestroy(Deletion)
->TextureChild::ActorDestroy

Comment 4

[Peter Chang[:pchang]]

(In reply to peter chang[:pchang][:peter] from comment #3)
> (In reply to John Lin[:jolin][:jhlin] from comment #0)

> The plan is to add the condition wait inside TextureClient::Finalize[1] to
> wait the tchild(mActor) and mWaitForRecycle(reference of tclient)
> destroyed[2]. After TextureClient::Finalize finish, we could make sure
> tchild is destroyed.
This is not a good idea because of performance concern.

Correct some info from comment 2.

Tclient and tchild are reference recycle only if mWaitForRecycle has value. For video playback case, mWaitForRecycle is always nullptr.

Besides mWaitForRecycle, tchild owns the raw pointer of tclient. That caused the race condition problem when TextureClient::Finalize and TextureChild::ActorDestroy were invoked by different threads.

Tclient holds RefPtr of TextureChild and is going to destroy itself after TextureClient::Finalize. Under this condition, we don't need to free mActor[1] because ~TextureClient() will do.

I will attach my WIP patch and the verification result later.

[1]http://dxr.mozilla.org/mozilla-central/source/gfx/layers/client/TextureClient.cpp#189

Comment 5

[Peter Chang[:pchang]]

Attachment: 0001-Bug-988713.patch

Attached the WIP patch and try to prepare environment for verification.

Comment 6

[Sotaro Ikeda [:sotaro]]

attachment 8398404 [details] [diff] [review] seems not strong enough to timing dependent defect. It seems better to fix the problem by more timing robust way. Most good way seems to use thread safe weak pointer. But it is not provided now :-(

Comment 7

[Sotaro Ikeda [:sotaro]]

If we want to fix the problem without using thread safe weak pointer, we need to think about how to fix the problem more wider area.

Comment 8

[Nicolas Silva [:nical]]

(In reply to peter chang[:pchang][:peter] from comment #3)
> From the behavior point of view, the tclient's lifecycle should be longer
> than tchild.

In the general case this is not true: TextureClient::Finalize() is what causes TextureChild to send the asyncRemoveTexture message, and in response to that the parent side sends __delete__ which will trigger ActorDestroy on the child side.
So in the general case TextureChild is supposed to outlive TextureClient.

Here, we seem to be in a case where ActorDestroy is called before/during Finalize. This must mean that the parent protocol (ImageBridgeChild, or LayerTransactionChild) is being destroyed before the texture client. This looks somewhat similar to bug 966284. Bug 966284 probably won't fix the race entirely if several threads are involved but it's best to know about it to not step on each other's toes.

Comment 9

[Nicolas Silva [:nical]]

Note that the mWaitForRecycle stuff is a recent addition that came in with "simple tiling" which is off by default, so if it causes thread safety issues it should be easier to change this logic than rethink completely TextureClient's shutdown. This was introduced as a way to implement texture recycling, and we have another way to implement said recycling in the other tiling implementation so we'll have to decide and which way we want to stick with and make sure it's thread safe.

Comment 10

[Sotaro Ikeda [:sotaro]]

"ActorDestroy called before/during Finalize" does not happen in normal use case. Therefore it seems better to analyze why it happens.

Comment 11

[Sotaro Ikeda [:sotaro]]

There is not actual STR to reproduce the crash. Can we have an actual STR to reproduce the crash?

Comment 12

[Sotaro Ikeda [:sotaro]]

Nical, in current gecko's PTexture implementation Send__delete__() is always sent from parent side. Is there a reason to do it from parent side. I feel Send__delete__() from child side seems better.

Comment 13

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #12)
> Is there a reason to do it from parent side. 

Correction:
 Is there a reason to do it from parent side?

Comment 14

[Nicolas Silva [:nical]]

(In reply to Sotaro Ikeda [:sotaro] from comment #13)
>  Is there a reason to do it from parent side?

Yes, if we want to be able to send messages from the compositor to the parent to the child, the only way to make sure that they don't cross (while keeping shutdown asynchronous) is to have the side that decides when to destroy the texture (the child side) send a message (RemoveTexture) which triggers the other side to send __delete__.

This way each side knows if it can send a message. If we don't have this kind of hand-shake we can't have the parent side send messages to the child.

Comment 15

[Sotaro Ikeda [:sotaro]]

Thanks! I understand the reason.

Comment 16

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #6)
> attachment 8398404 [details] [diff] [review] seems not strong enough to
> timing dependent defect. It seems better to fix the problem by more timing
> robust way. Most good way seems to use thread safe weak pointer. But it is
> not provided now :-(

Another way to fix the problem is "always call TextureClient::Finalize()'s implementation in TexureClient's IPC thread". It serializes ActorDestroy() and Finalize(). Almost all TextureClient::Finalize()s are called in IPC thread. Therefore it should not add much delay.

Comment 17

[Peter Chang[:pchang]]

(In reply to Sotaro Ikeda [:sotaro] from comment #11)
> There is not actual STR to reproduce the crash. Can we have an actual STR to
> reproduce the crash?

Please see comment 2 and you can refer below link.
https://bugzilla.mozilla.org/show_bug.cgi?id=911046#c6

But I did see the race condition between TextureClient::Finalize() and TextureChild::ActorDestroy().

Comment 18

[Peter Chang[:pchang]]

The following logs are reproduced by video playback.
TexureClient::Finalize() was called by Media StateMachine thread.

app_534   534   379   91688  30656 ffffffff 00000000 S /system/b2g/plugin-container
app_534   540   534   91688  30656 ffffffff 00000000 S ImageBridgeChil
app_534   601   534   91688  30656 ffffffff 00000000 S Binder Thread #
app_534   619   534   91688  30656 ffffffff 00000000 S Media S~hine #1

03-28 11:29:47.119   534   540 I Gecko   : TexureClient::Finalize called this 0x40302ef0 texture child 0x431fb080
03-28 11:29:47.119   534   540 I Gecko   : TexureClient::ForceRemove this 0x40302ef0 flag 0x02000000 texturechild 0x431fb080
03-28 11:29:47.139   534   540 I Gecko   : TexureClient::Finalize called this 0x40302e80 texture child 0x431fb0c0
03-28 11:29:47.139   534   540 I Gecko   : TexureClient::ForceRemove this 0x40302e80 flag 0x02000000 texturechild 0x431fb0c0
03-28 11:29:47.199   534   619 I Gecko   : TexureClient::Finalize called this 0x40302ef0 texture child 0x431fb080
03-28 11:29:47.199   534   540 I Gecko   : TexureClient::ForceRemove this 0x40302ef0 flag 0x02000000 texturechild 0x431fb080
03-28 11:29:47.199   534   540 I Gecko   : TextureChild::ActorDestroy this 0x431fb080 texture client 0x40302ef0
03-28 11:29:47.229   534   540 I Gecko   : TexureClient::Finalize called this 0x40302e80 texture child 0x431fb300

Comment 19

[Peter Chang[:pchang]]

(In reply to Sotaro Ikeda [:sotaro] from comment #16)
> (In reply to Sotaro Ikeda [:sotaro] from comment #6)
> > attachment 8398404 [details] [diff] [review] seems not strong enough to
> > timing dependent defect. It seems better to fix the problem by more timing
> > robust way. Most good way seems to use thread safe weak pointer. But it is
> > not provided now :-(
> 
> Another way to fix the problem is "always call TextureClient::Finalize()'s
> implementation in TexureClient's IPC thread". It serializes ActorDestroy()
> and Finalize(). Almost all TextureClient::Finalize()s are called in IPC
> thread. Therefore it should not add much delay.

Another idea is to move [1] to line 514. Then we could avoid the race condition caused by actor's raw pointer.

http://dxr.mozilla.org/mozilla-central/source/gfx/layers/client/TextureClient.cpp#519

Comment 20

[John Lin [:jhlin][:jolin]]

(In reply to Sotaro Ikeda [:sotaro] from comment #11)
> There is not actual STR to reproduce the crash. Can we have an actual STR to
> reproduce the crash?

When working on bug 911046 I met this problem after apply the patches there.
My test env. is a desktop peer (set up as described in bug 911046 comment 6) and a Nexus 5 with latest m-c Gecko + my patches installed, then connect both to local nightly-gupshup (https://github.com/jesup/nightly-gupshup) server and start a conversation. Usually the device will crash after several minutes.

Comment 21

[Peter Chang[:pchang]]

Attachment: another approach

Comment 22

[Peter Chang[:pchang]]

Attachment: fix race condition between TextureClient::Finalize() and TextureChild::ActorDestroy()

With this patch, I could run the STR from comment 20 for 1 hr without crash.

And from below log, thread 1995 may crash at ActorDestroy[1] if textureclient got destroyed.

[1]http://dxr.mozilla.org/mozilla-central/source/gfx/layers/client/TextureClient.cpp#189

02:35:03.480  1989  2511 I Gecko : TextureClient::Finalize() before RemoveTexture this 0xb1a23ac0
02:35:03.480  1989  2511 I Gecko : TextureClient::Finalize() after RemoveTexture this 0xb1a23ac0
02:35:03.480  1989  1995 I Gecko : TextureChild::ActorDestroy this 0xb011c400
02:35:03.480  1989  2511 I Gecko : ~TextureClient this 0xb1a23ac0
02:35:03.480  1989  2511 I Gecko : ~TextureChild this 0xb011c400

Comment 23

[Nicolas Silva [:nical]]

Sotaro pointed out to me that even with the patch, we can still have a race condition. We should take an hour during the workweek to discuss about TextureClient's shutdown sequence and fixing this in a more bullet-proof way. We can still take this patch but I am marking the bug "leave open" so that we don't close it before we have a solid solution.

Comment 24

[Sotaro Ikeda [:sotaro]]

My point is that attachment 8399258 [details] [diff] [review] is not atomic operation in multi threaded environment, so it could cause the very very rare race condition. If a phone has multiple cpus, this risk becomes higher than one cpu.

Comment 25

[Nicolas Silva [:nical]]

Actually we looked at it more closely with Sotaro and Peter and this patch appears to fix all the race scenarios of this bug. Let's land it!

Comment 26

[Peter Chang[:pchang]]

try server link
https://tbpl.mozilla.org/?tree=Try&rev=038d66beec7c

Comment 27

[Peter Chang[:pchang]]

All tests passed except gaia integration testing from B2G Desktop Linux x64 Opt.
Checking...

https://tbpl.mozilla.org/?tree=Try&rev=038d66beec7c&jobname=b2g_ubuntu64_vm%20try%20opt%20test%20gaia-integration

Comment 28

[Peter Chang[:pchang]]

re-run new try server with latest inbound and gaia integration pass.
https://tbpl.mozilla.org/?tree=Try&rev=48db580e7b41

Comment 29

[Peter Chang[:pchang]]

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

Comment 30

[Carsten Book [:Tomcat]]

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

Comment 31

[Sotaro Ikeda [:sotaro]]

Nominate to "1.4+", because this bug blocks Bug 993736.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/567e247059e0