|Summary:||Chromium bug: Compositing on a lost context causes latch deadlocks|
|Product:||WebKit||Reporter:||John Bates <jbates>|
|Component:||New Bugs||Assignee:||John Bates <jbates>|
|Severity:||Normal||CC:||jamesr, kbr, vangelis, webkit.review.bot|
|Version:||528+ (Nightly build)|
Description John Bates 2011-06-29 19:25:24 PDT
Chromium bug: Compositing on a lost context causes latch deadlocks
Comment 2 John Bates 2011-06-29 19:28:52 PDT
This bug is triggered by opening two tabs in the same renderer that support lost context recovery. Killing the GPU process would recreate the contexts. Then when the background tab is clicked, the first composite occurs on the old compositor context, thereby latching on a lost context and leading to deadlock.
Comment 3 James Robinson 2011-06-29 19:49:39 PDT
Comment on attachment 99211 [details] Patch isCompositorContextLost() is not a synchronous operation, so it seems like this will still be racy if the context is lost and the renderer side hasn't noticed yet - no?
Comment 4 John Bates 2011-06-30 09:44:16 PDT
(In reply to comment #3) > (From update of attachment 99211 [details]) > isCompositorContextLost() is not a synchronous operation, so it seems like this will still be racy if the context is lost and the renderer side hasn't noticed yet - no? This change fixes a condition that was actually not a race. The background tab has already lost its contexts (at GPU process kill time), and later on, it is displayed. Since the first thing we do is composite, we haven't discovered yet that the context was already lost, and we go ahead latching on it. For the cases where the compositor context is lost after this point, I think it's handled already. Here are the cases: - compositor context is lost due to error (memory, etc): GPU process detects error and kills whole GpuChannel, bringing down all child contexts as well. No deadlock because IPCs don't block on closed channels. - GPU crashed/killed: Also no deadlock because IPCs don't block on closed channels.
Comment 5 Kenneth Russell 2011-06-30 14:43:05 PDT
Comment on attachment 99211 [details] Patch Looks fine as long as it's been tested. I am also concerned about what would happen if the GPU process died immediately after the call to isCompositorContextLost, and also about what happens if we skip doing a render at this point, but it seems this is an improvement.
Comment 7 James Robinson 2011-06-30 16:06:01 PDT
Comment on attachment 99382 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=99382&action=review > Source/WebKit/chromium/ChangeLog:9 > + The cause of deadlock was that a setLatch command is added to the lost > + compositor GL context, and a waitLatch would never complete on a child context. > + This change checks whether the compositor context is in error state before > + doing the compositing. nit: in the future, put this after the bug link and a newline. pattern is: Reviewed by ... <newline> bug title bug URL <newline> long desc long desc long desc <newline> per-file stuff
Comment 8 WebKit Review Bot 2011-06-30 16:47:06 PDT
Comment on attachment 99382 [details] Patch Clearing flags on attachment: 99382 Committed r90185: <http://trac.webkit.org/changeset/90185>
Comment 9 WebKit Review Bot 2011-06-30 16:47:10 PDT
All reviewed patches have been landed. Closing bug.