Closed
Bug 1221056
Opened 9 years ago
Closed 9 years ago
Ensure that the RemoveTexture message never races with layers transaction
Categories
(Core :: Graphics: Layers, defect)
Core
Graphics: Layers
Tracking
()
RESOLVED
FIXED
mozilla46
Tracking | Status | |
---|---|---|
firefox46 | --- | fixed |
People
(Reporter: nical, Assigned: nical)
References
Details
Attachments
(1 file)
(deleted),
patch
|
sotaro
:
review+
|
Details | Diff | Splinter Review |
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 | ||
Updated•9 years ago
|
Assignee: nobody → nical.bugzilla
Assignee | ||
Comment 1•9 years ago
|
||
1219529 increases the likelihood of the race being observable, so I want to fix this before I can land bug 1219529.
Blocks: 1219529
Assignee | ||
Comment 2•9 years ago
|
||
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)
Comment 3•9 years ago
|
||
Comment on attachment 8683227 [details] [diff] [review]
OpDestroy in transaction
Looks good :)
Attachment #8683227 -
Flags: review?(sotaro.ikeda.g) → review+
Assignee | ||
Comment 4•9 years ago
|
||
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)
Comment 5•9 years ago
|
||
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)
Comment 7•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox46:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Depends on: 1237197
Updated•9 years ago
|
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 → ---
Updated•9 years ago
|
Comment 10•9 years ago
|
||
bugherder |
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•