Closed Bug 1221056 Opened 4 years ago Closed 4 years ago

Ensure that the RemoveTexture message never races with layers transaction

Categories

(Core :: Graphics: Layers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: nical, Assigned: nical)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Currently when a TextureClient's refcount gets to 0, the texture sends an IPDL message to the host side in order to synchronize the destruction of the TextureClient/Host pair and their shared data. What typically happens is that we have a texture A used as the front buffer of a compositable, and we replace it with texture B in a transaction. when we assign B to the front buffer slot, A's ref count goes to zero and the message is sent before the end of the layers transaction, which can cause the host side to have a "blank flash" between the moment A is destroyed and B arrives to take its place.
We work around this manually in each compositable by keeping the texture alive until the end of the transaction, but since we do it manually we don't succeed in doing it systematically.

This bug is about making sure the texture is kept alive long enough automatically without each compositable having dedicated logic to ensure it.

The way I want to fix this move the TemoveTexture message in the transaction (and create mini-transactions when a texture is destroyed outside of one).
Assignee: nobody → nical.bugzilla
1219529 increases the likelihood of the race being observable, so I want to fix this before I can land bug 1219529.
Blocks: 1219529
I ended up keeping the RemoveTexture message for the out-of-transaction case. It's a bit awkward, but removing it completely required some involved plumbing in the code around transactions, and I wanted this patch to keep a reasonable size. So I decided to split the work. I'll do the transaction cleanup in bug 1221566 and most likely get rid of the PTexture::RemoveTexture message there.
Attachment #8683227 - Flags: review?(sotaro.ikeda.g)
Blocks: 1221566
Comment on attachment 8683227 [details] [diff] [review]
OpDestroy in transaction

Looks good :)
Attachment #8683227 - Flags: review?(sotaro.ikeda.g) → review+
Hm, I thought that IPDL didn't support sending (async) messages from within a synchronous message handler. For instance if we have RecvFoo() and it is a synchronous message, we can't call SendBar() from RecvFoo. But actually, we do just that with RecvUpdate and SendPeningAsyncMessages. Unless I am missing something I could simplify the patch.
Sotaro, wasn't there a limitation like that (maybe the reason why we have SendClearTextureHostSync + SendRemoveTexture, instead of just SendRemoveTextureSync)?
Flags: needinfo?(sotaro.ikeda.g)
IIRC, IPDL does not have a limitation to prevent sending async message during receiving sync message.

And MessageChannel::Send(Message* aMsg) does not have such limitaion.
https://dxr.mozilla.org/mozilla-central/source/ipc/glue/MessageChannel.cpp#544

One quirk about it is that parent sent messages order could be reverted in child side like the following. After child receive a aync message reply from parent, child might receive older message than the reply like the following. This is one of big reason to do quirk to transfer fence handle from parent to child side.

[1] Parent send async message 1
[2] Child send sync Message 3
[3] Parent reply to sync message 3
[4] Child receive the reply to message 3.
[5] Child receive async message 1.
Flags: needinfo?(sotaro.ikeda.g)
https://hg.mozilla.org/mozilla-central/rev/9f7580535214
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Depends on: 1237197
Discussed with nical, he was okay to back out the patch (due to bug 1237197). https://hg.mozilla.org/mozilla-central/rev/ec25e284ca6f
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
See Also: → 1237220
https://hg.mozilla.org/mozilla-central/rev/ed482fa2c0c6
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.