Bug 63681 - Chromium bug: Compositing on a lost context causes latch deadlocks
Summary: Chromium bug: Compositing on a lost context causes latch deadlocks
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: John Bates
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-06-29 19:25 PDT by John Bates
Modified: 2011-06-30 16:47 PDT (History)
4 users (show)

See Also:


Attachments
Patch (1.53 KB, patch)
2011-06-29 19:26 PDT, John Bates
no flags Details | Formatted Diff | Diff
Patch (1.81 KB, patch)
2011-06-30 15:51 PDT, John Bates
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description John Bates 2011-06-29 19:25:24 PDT
Chromium bug: Compositing on a lost context causes latch deadlocks
Comment 1 John Bates 2011-06-29 19:26:04 PDT
Created attachment 99211 [details]
Patch
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 6 John Bates 2011-06-30 15:51:09 PDT
Created attachment 99382 [details]
Patch
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.