Closed Bug 1026580 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Graphics: Layers, defect)

x86_64
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: sotaro, Assigned: sotaro)

References

Details

Attachments

(1 file, 2 obsolete files)

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: nobody → sotaro.ikeda.g
Add NS_WARNING.
Attachment #8441467 - Attachment is obsolete: true
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?
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.
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+
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
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
See Also: → 1174592
You need to log in before you can comment on or make changes to this bug.