During running "WindowTest" app. pipe connection of the app is broken. STR: * Run the test app at bug 964386 attachment 8366455 [details] on master b2g device. Logcat log has the following log. > 04-23 17:15:37.809 183 507 I Gecko : [Parent 183] WARNING: pipe error: Bad file number: file ../../../gecko/ipc/chromium/src/chrome/common/ipc_channel_posix.cc, line 709 > 04-23 17:15:37.809 183 507 I Gecko : [Parent 183] WARNING: pipe error: Bad file number: file ../../../gecko/ipc/chromium/src/chrome/common/ipc_channel_posix.cc, line 709 > 04-23 17:15:37.809 183 507 I Gecko : [Parent 183] WARNING: pipe error: Bad file number: file ../../../gecko/ipc/chromium/src/chrome/common/ipc_channel_posix.cc, line 709

The problem seems to be related too much file descriptors especially around code of Fence and gralloc. But each process does not open a lot of file descriptors.

I checked the file descriptor by using the following commands. The file descriptor number and "sync" seem not have a problem. - | adb shell ls /proc/<b2g_pid>/fd | wc -w | - | adb shell lsof | grep "sync" |

I tested on master nexus-4 and master nexus-5. On nexus-5, the problem seems to happen more easily.

When tiling is disabled, the problem seems not happens.

Nominate to 1.4+. When Bug 998504 is fixed on b2g v1.4, this problem becomes to happen on b2g v1.4

But the WindowTest app does not OOM? What about the fd count for that process?

I already confirmed that SendCompositorRecycle() in TextureParent::CompositorRecycle() causes the problem. By commenting out the following, I confirmed that the problem does not happen. >> mozilla::unused << SendCompositorRecycle(handle); http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/TextureHost.cpp#643

"pipe error: Bad file number" seems to happen that android::Fence is destroyed in b2g process side, before the Fence is delivered to client side.

Before "pipe error: Bad file number" happened, a lot of the following logout are present in the logcat. They were not reached to Child side. > [PTextureParent] Sending Msg_CompositorRecycle([TODO])

By enabling IPC message log, I faced the b2g process crash because of PTexture protocol's problem. I am going to create a bug for it.

(In reply to Sotaro Ikeda [:sotaro] from comment #10) > By enabling IPC message log, I faced the b2g process crash because of > PTexture protocol's problem. I am going to create a bug for it. Bug 1000660 is created for it.

(In reply to Sotaro Ikeda [:sotaro] from comment #8) > "pipe error: Bad file number" seems to happen that android::Fence is > destroyed in b2g process side, before the Fence is delivered to client side. android::Fence is kernel reference counted object, it needs to be keep alive in b2g process until the Fence is delivered to client side.

(In reply to Sotaro Ikeda [:sotaro] from comment #12) > > android::Fence is kernel reference counted object, it needs to be keep alive > in b2g process until the Fence is delivered to client side. In current gecko, previous Fence is hold by TextureHostOGL.

(In reply to Ben Kelly [:bkelly] from comment #6) > But the WindowTest app does not OOM? What about the fd count for that > process? The app is not OOM. The following is b2g-procrank. Just pipe connection is broken. The fd count was 110. -------------------------------- APPLICATION PID Vss Rss Pss Uss cmdline b2g 184 268320K 90436K 74620K 67592K /system/b2g/b2g WindowTest 1168 112464K 43844K 31232K 28736K /system/b2g/plugin-container Homescreen 865 81072K 34188K 18600K 13836K /system/b2g/plugin-container Settings 1075 80476K 29692K 15791K 12660K /system/b2g/plugin-container Built-in Keyboa 966 66152K 21456K 8978K 6508K /system/b2g/plugin-container (Preallocated a 1344 59968K 18804K 7799K 5720K /system/b2g/plugin-container (Nuwa) 836 53776K 19584K 6678K 2348K /system/b2g/plugin-container

I understand the cause of the problem. There is a case that Fence's file descriptor is still in IPC transter between TextureParent and TextureChild even after TextureParent is deleted. This happens because IPC message is async. Fence object is kept alive by TextureHost. But IPC's fence delivery is still in progress even after TextureHost is deleted.

The patch defer Fence objects release until IPC delivery complete. If IPC connection is removed during delivery, defer Fence objects release some enough time for IPC message delivery complete.

attachment 8412960 [details] [diff] [review] borrows some code from 984434.

(In reply to Sotaro Ikeda [:sotaro] from comment #17) > attachment 8412960 [details] [diff] [review] borrows some code from 984434. Correction: Bug 984434.

(In reply to Sotaro Ikeda [:sotaro] from comment #17) > attachment 8412960 [details] [diff] [review] borrows some code from 984434. The patch seems to fix the problem for the time being.

(In reply to Sotaro Ikeda [:sotaro] from comment #19) > (In reply to Sotaro Ikeda [:sotaro] from comment #17) > > attachment 8412960 [details] [diff] [review] borrows some code from 984434. > > The patch seems to fix the problem for the time being. Are we saying bug 998504 has been fixed with this patch? Please confirm.

(In reply to Preeti Raghunath(:Preeti) from comment #20) > (In reply to Sotaro Ikeda [:sotaro] from comment #19) > > (In reply to Sotaro Ikeda [:sotaro] from comment #17) > > > attachment 8412960 [details] [diff] [review] borrows some code from 984434. > > > > The patch seems to fix the problem for the time being. > > Are we saying bug 998504 has been fixed with this patch? Please confirm. Yes. But the patch is not committable patch. Need to update it.

Some clean up.

attachment 8414487 [details] [diff] [review] has the following concerns. - [1] To create AsyncTransactionTracker' serial ID, global mutex is used. Use the mutex because, gecko does not support 64bit atomic operation - [2] To track Fence delivery, uses a lot of IPC message - [3] Use timeout to defer releasing Fence objects, if there are in transit Fence objects when TextureParent is deleted.

Comment on attachment 8414487 [details] [diff] [review] patch v2 - Defer Fence release until IPC delivery complete nical, can I have a feedback to the patch?

> - [2] To track Fence delivery, uses a lot of IPC message It might be better to defer [2] to a new bug.

un-bitrot.

Add missing files

Fix nits.

Comment on attachment 8414729 [details] [diff] [review] patch v5 - Defer Fence release until IPC delivery complete Review of attachment 8414729 [details] [diff] [review]: ----------------------------------------------------------------- I am a little bit worried about the volume ipc messages this is adding (especially if we have a lot of tiles on the screen), but I don't know exactly how much overhead it will be. I have an alternative proposition: You want to receive a notification from the child side after it has received the fences so as to know that you can destroy them safely. The tricky thing is that PTexture is "short lived" in that we tend to create and destroy them often, so we need to be very careful about this notification not racing with PTexture deletion as well. But maybe we don't actually need to send this notification at the PTexture level. We could send a message from parent to child at the end of the composition, the child would send a reply and the deletion of fence objects could wait for this reply (a bit like this patch, but instead of doing it for each Texture, do it once per CompositableTransactionParent). It might make things simpler because PCompositable and PImageBridge have synchronous shutdowns so it's easier to not race with that (compared to PTexture that is almost fully async), and it would reduce the volume of IPC messages (1 extra message per channel instead of one per texture). Also I think that if you do it this way you won't have to do the PostDelayedTask hack. How do you feel about this? I'll set feedback- because I would prefer something along the lines of what I proposed above, but you can still convince me that we need your solution in the short term. ::: gfx/layers/client/TextureClient.cpp @@ +105,5 @@ > + FenceHandle fence = op.fence(); > + if (mTextureClient) { > + mTextureClient->SetReleaseFenceHandle(fence); > + } > + if (IPCOpen()) { If the child receives this after it sent SendRemove, the ChildAsyncMessage will reach a dead parent actor. If mTextureClient is null you are sure that SendRemove already happened but you could still get into trouble if ForceRemove was called manually on the TextureClient. So my suggestion: turn this into if (IPCOpen() && mTextureClient) and null out TextureChild::mTextureClient in ForceRemove.

(In reply to Nicolas Silva [:nical] from comment #29) > Comment on attachment 8414729 [details] [diff] [review] > patch v5 - Defer Fence release until IPC delivery complete > > Review of attachment 8414729 [details] [diff] [review]: > ----------------------------------------------------------------- > > I am a little bit worried about the volume ipc messages this is adding > (especially if we have a lot of tiles on the screen), but I don't know > exactly how much overhead it will be. > I have an alternative proposition: > You want to receive a notification from the child side after it has received > the fences so as to know that you can destroy them safely. The tricky thing > is that PTexture is "short lived" in that we tend to create and destroy them > often, so we need to be very careful about this notification not racing with > PTexture deletion as well. But maybe we don't actually need to send this > notification at the PTexture level. We could send a message from parent to > child at the end of the composition, the child would send a reply and the > deletion of fence objects could wait for this reply (a bit like this patch, > but instead of doing it for each Texture, do it once per > CompositableTransactionParent). > It might make things simpler because PCompositable and PImageBridge have > synchronous shutdowns so it's easier to not race with that (compared to > PTexture that is almost fully async), and it would reduce the volume of IPC > messages (1 extra message per channel instead of one per texture). Also I > think that if you do it this way you won't have to do the PostDelayedTask > hack. > How do you feel about this? Yeah, I agree. I also thought same thing. But if we want to move both sending Fence and receiving the send result on CompositableTransactionParent level, the change could becomes very larger for b2g v1.4. So, I commented as in Comment 25. If we limit the change only to "receiving the send result on CompositableTransactionParent level" part, the change might be minimum.

(In reply to Sotaro Ikeda [:sotaro] from comment #30) > (In reply to Nicolas Silva [:nical] from comment #29) > > Comment on attachment 8414729 [details] [diff] [review] > > patch v5 - Defer Fence release until IPC delivery complete > > > > Review of attachment 8414729 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > I am a little bit worried about the volume ipc messages this is adding > > (especially if we have a lot of tiles on the screen), but I don't know > > exactly how much overhead it will be. > > I have an alternative proposition: > > You want to receive a notification from the child side after it has received > > the fences so as to know that you can destroy them safely. The tricky thing > > is that PTexture is "short lived" in that we tend to create and destroy them > > often, so we need to be very careful about this notification not racing with > > PTexture deletion as well. But maybe we don't actually need to send this > > notification at the PTexture level. We could send a message from parent to > > child at the end of the composition, the child would send a reply and the > > deletion of fence objects could wait for this reply (a bit like this patch, > > but instead of doing it for each Texture, do it once per > > CompositableTransactionParent). > > It might make things simpler because PCompositable and PImageBridge have > > synchronous shutdowns so it's easier to not race with that (compared to > > PTexture that is almost fully async), and it would reduce the volume of IPC > > messages (1 extra message per channel instead of one per texture). Also I > > think that if you do it this way you won't have to do the PostDelayedTask > > hack. > > How do you feel about this? > > Yeah, I agree. I also thought same thing. But if we want to move both > sending Fence and receiving the send result on CompositableTransactionParent > level, the change could becomes very larger for b2g v1.4. So, I commented as > in Comment 25. If we limit the change only to "receiving the send result on > CompositableTransactionParent level" part, the change might be minimum. Ah yes sorry I wasn't clear. I meant to only move the "ok you are good to remove the fences" notification to the CompositableTransactionParent level for now. Sending the fence object to the TextureClient would still remain at the Texture level, so that you can fix this bug without having to rewrite too much of the existing code.

During debugging, I found strange symptom. LayerTransactionChild, ShadowLayerForwarder, LayerTransactionParent are increased for each window.open() and never destructed.

ShadowLayerForwarder leak seeem to be injected by Bug 933082. But LayerTransactionChild and LayerTransactionParent leaks seems to be present since Compositor were enabled. LayerTransactionChild and LayerTransactionParent shutdowns are triggered from RenderFrameChild and RenderFrameParent. But this pass can be used only when LayerTransactionChild/LayerTransactionParent are generated via RenderFrameChild/RenderFrameParent. They create LayerTransactionChild/LayerTransactionParent only when CompositorChild is not present. http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp#2434

I am going to create a bug for Comment 33.

Apply the comment.

Update nits.

Comment on attachment 8416000 [details] [diff] [review] patch part 1 - Add AsyncTransactionTracker Review of attachment 8416000 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/ipc/AsyncTransactionTracker.h @@ +102,5 @@ > + void ClearAllAsyncTransactionTrackers(); > + > + void DestroyAsyncTransactionTrackersHolder(); > + > +protected: nit: you don't need to write protected twice

Current patches has a dependency to Bug 1004191. It seem better to remove it. Bug 1004191 seems not easy to fix soon.

By only the current patch the following case is not handled correctly. On child side, ShadowLayerForwarder is destroyed without sending response to some FenceHandles deliveries. - AsyncTransactionTrackers sent by parent since last CompositableParentManager::ReceiveCompositableUpdate(). Bug 1004191 covers this use case by killing LayerTransactionParent/LayerTransactionChild. To remove the dependency, ShadowLayerForwarder or ClientLayerManager needs to send pending responses before their destruction.

The current patches store the pending transaction response Ids in CompositableForwarder. This seems not a good place to store them. The transaction lifetimes are tide to IPC protocol. It's better to store them on IPC object. ImageBridggeChild is IPC object and derived from CompositableForwarder. ShadowLayerForwarder is derived from CompositableForwarder. But it is not IPC object and it's lifetime is shorter than LayerTransactionChild.

(In reply to Sotaro Ikeda [:sotaro] from comment #45) > The current patches store the pending transaction response Ids in > CompositableForwarder. This seems not a good place to store them. The > transaction lifetimes are tide to IPC protocol. It's better to store them on > IPC object. I re-think about it. we do not need to change this. ShadowLayerForwarder is not destroyed until all TextureClients are destroyed.

> > I re-think about it. we do not need to change this. ShadowLayerForwarder is > not destroyed until all TextureClients are destroyed. Hmm, there is still a possibility that TOpDeliverFence message is received after ShadowLayerForwarder if we do not expect LayerTransactionChild shutdown.

> ::: gfx/layers/ipc/AsyncTransactionTracker.h > @@ +102,5 @@ > > + void ClearAllAsyncTransactionTrackers(); > > + > > + void DestroyAsyncTransactionTrackersHolder(); > > + > > +protected: > > nit: you don't need to write protected twice It is just my preference to make clear the border beween functions and variables. It seems not common in gecko. I am going to update it in next patch.

Apply the comment. Carry r=nical.

Remove dependency to bug 1004191.

Remove dependency to bug 1004191.

Fix nits.

Comment on attachment 8416681 [details] [diff] [review] patch part 4 v2 - Change to IPDL nical, can you review the patch again. After the review, I added relatively big change.

Remove dependency to bug 1004191.

From Bug 1004191 Comment 31, attachment 8416681 [details] [diff] [review] seem to have a problem to ShadowLayerForwarder::~ShadowLayerForwarder(). Need to update it.

Fix comment 56.

Comment on attachment 8416681 [details] [diff] [review] patch part 4 v2 - Change to IPDL Review of attachment 8416681 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the comment below addressed ::: gfx/layers/ipc/LayersMessages.ipdlh @@ +440,5 @@ > > +union AsyncMessage { > + OpDeliverFence; > + OpReplyDeliverFence; > +}; Please add two separate unions for child vs parent async messages, since the content of the messages are different.

Comment on attachment 8417020 [details] [diff] [review] patch part 3 v4 - Change around IPC code Review of attachment 8417020 [details] [diff] [review]: ----------------------------------------------------------------- looks good to me ::: gfx/layers/ipc/ShadowLayers.cpp @@ +702,3 @@ > } > > +void ShadowLayerForwarder::StopHandlePendingAsyncMessge() nit: I was confused by the name of this method. Not sure about a better name. Maybe you could just put the two calls directly in ShadowLayerForwarder's destructor since it is the only user of this method. Actually, maybe it's not that bad of a name, up to you. But since it is a short method and only called once I feel like it's just more readable to place its two lines directly in the caller. Not a big deal anyway.

(In reply to Nicolas Silva [:nical] from comment #60) > Comment on attachment 8417020 [details] [diff] [review] > patch part 3 v4 - Change around IPC code > > Review of attachment 8417020 [details] [diff] [review]: > ----------------------------------------------------------------- > > looks good to me > > ::: gfx/layers/ipc/ShadowLayers.cpp > @@ +702,3 @@ > > } > > > > +void ShadowLayerForwarder::StopHandlePendingAsyncMessge() > > nit: I was confused by the name of this method. Not sure about a better > name. Maybe you could just put the two calls directly in > ShadowLayerForwarder's destructor since it is the only user of this method. From Bug 1004191 Comment 31, we can not put the StopHandlePendingAsyncMessge() call to ShadowLayerForwarder's destructor. There is a risk that LayerTransactionChild is already closed, if the destructor is triggered by CompositorChild::Destroy().

I am going to add a comment about comment 61.

Apply the comments. Carry "r=nical".

Un-bitrot. Carry "r=nical".

Un-bitrot. Carry "r=nical".

Fix build failure on some platforms and add pointer checks. Carry "r=nical".

https://tbpl.mozilla.org/?tree=Try&rev=06c4ce8f7392

Fix build failures on some platforms. Carry "r=nical".

(In reply to Sotaro Ikeda [:sotaro] from comment #71) > https://tbpl.mozilla.org/?tree=Try&rev=fd97b23bc34e Mutex leak is detected. It seems AsyncTransactionTracker's global mutex.

Fix Mutex leak.

I am going to create a patch for b2g v1.4.

Patch for b2g v1.4.

As far as I know this is an issue with fences which weren't introduced until JB. Since tarako is based on ICS it should be unaffected.

Crash observed on: Device: msm8226 Firmware: AU_LABEL Moz BuildID: 20140505000201 B2G Version: 1.4 Gecko Version: 30.0 Gaia: http://git.mozilla.org/?p=releases/gaia.git;a=commit;h=fccb15d6940db51615545574877a62d69406b1c2 Gecko: http://git.mozilla.org/?p=releases/gecko.git;a=commit;h=3ed3bbf1941e608bc9630942c7063a8b818f36bc

(In reply to caf-b2g-bot (PoC: ggrisco) from comment #82) > Crash observed on: > > Device: msm8226 > Firmware: AU_LABEL > Moz BuildID: 20140505000201 > B2G Version: 1.4 > Gecko Version: 30.0 > Gaia: > http://git.mozilla.org/?p=releases/gaia.git;a=commit; > h=fccb15d6940db51615545574877a62d69406b1c2 > Gecko: > http://git.mozilla.org/?p=releases/gecko.git;a=commit; > h=3ed3bbf1941e608bc9630942c7063a8b818f36bc The above liks's gecko does not include this bug's fix.