1026580 - Remove an infinite loop possibility from AsyncTransactionTracker::WaitComplete()

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Sotaro Ikeda [:sotaro]]

AsyncTransactionTracker::WaitComplete() could wait infinity if a transaction is not complete. In current normal use case, the transaction should be completed soon. But current code have a risk of falling into infinite loop.

Comment 1

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Remove an infinite loop possibility from AsyncTransactionTracker::WaitComplete()

Comment 2

[Sotaro Ikeda [:sotaro]]

Attachment: patch v2 - Remove an infinite loop possibility from AsyncTransactionTracker::WaitComplete()

Add NS_WARNING.

Comment 3

[Nicolas Silva [:nical]]

(In reply to Sotaro Ikeda [:sotaro] from comment #0)
> AsyncTransactionTracker::WaitComplete() could wait infinity if a transaction
> is not complete. In current normal use case, the transaction should be
> completed soon. But current code have a risk of falling into infinite loop.

Can you elaborate on the case where we may end up never completing the transaction? Also please add the explanation in a comment in the patch so that we don't end up forgetting and filing a bug like "Possible spurious wake ups in AsyncTransactionTracker".

Comment 4

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

Comment on attachment 8441482 [details] [diff] [review]
patch v2 - Remove an infinite loop possibility from AsyncTransactionTracker::WaitComplete()

Review of attachment 8441482 [details] [diff] [review]:
-----------------------------------------------------------------

Before the change, we could wait twice, for 10 seconds each team - after the change, we wait once for 10 seconds.  When do we hit the infinite loop?

Comment 5

[Sotaro Ikeda [:sotaro]]

The infinite loop happens when a transaction does not complete by a bug. It should not happens, but the gecko need to protect from this risk.

Comment 6

[Sotaro Ikeda [:sotaro]]

To complete the transaction, compositor side need to respond quickly. On b2g, this should not happen. On other platform, it might happen. For example, on android, there is a bug about like that.

Comment 7

[Nicolas Silva [:nical]]

Comment on attachment 8441482 [details] [diff] [review]
patch v2 - Remove an infinite loop possibility from AsyncTransactionTracker::WaitComplete()

Review of attachment 8441482 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/ipc/AsyncTransactionTracker.cpp
@@ +31,5 @@
>  {
>    MOZ_ASSERT(!InImageBridgeChildThread());
>  
>    MonitorAutoLock mon(mCompletedMonitor);
> +  if (!mCompleted) {

Please add a comment explaining that we don't use a loop here because in rare cases, the transaction may not complete, that it would cause a deadlock and that timing out this way is a lesser evil.

Comment 8

[Sotaro Ikeda [:sotaro]]

Attachment: patch v3 - Remove an infinite loop possibility from AsyncTransactionTracker::WaitComplete()

Update logout. Carry "r=nical".

Comment 9

[Sotaro Ikeda [:sotaro]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2342853a754f

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2342853a754f