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

RESOLVED FIXED in mozilla33

Status

()

Core
Graphics: Layers
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: sotaro, Assigned: sotaro)

Tracking

unspecified
mozilla33
x86_64
Windows 7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

4 years ago
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.
(Assignee)

Updated

4 years ago
Assignee: nobody → sotaro.ikeda.g
(Assignee)

Comment 1

4 years ago
Created attachment 8441467 [details] [diff] [review]
patch -  Remove an infinite loop possibility from AsyncTransactionTracker::WaitComplete()
(Assignee)

Comment 2

4 years ago
Created attachment 8441482 [details] [diff] [review]
patch v2 - Remove an infinite loop possibility from AsyncTransactionTracker::WaitComplete()

Add NS_WARNING.
Attachment #8441467 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8441482 - Flags: review?(nical.bugzilla)
(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 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?
(Assignee)

Comment 5

4 years ago
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.
(Assignee)

Comment 6

4 years ago
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 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.
Attachment #8441482 - Flags: review?(nical.bugzilla) → review+
(Assignee)

Comment 8

4 years ago
Created attachment 8443943 [details] [diff] [review]
patch v3 - Remove an infinite loop possibility from AsyncTransactionTracker::WaitComplete()

Update logout. Carry "r=nical".
Attachment #8441482 - Attachment is obsolete: true
Attachment #8443943 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/2342853a754f
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
See Also: → bug 1174592
You need to log in before you can comment on or make changes to this bug.