Bug 63681 – Chromium bug: Compositing on a lost context causes latch deadlocks

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[John Bates]

Chromium bug: Compositing on a lost context causes latch deadlocks

[John Bates]

Created attachment 99211 [details]
Patch

[John Bates]

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.

[James Robinson]

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?

[John Bates]

(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.

[Kenneth Russell]

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.

[John Bates]

Created attachment 99382 [details]
Patch

[James Robinson]

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

[WebKit Review Bot]

Comment on attachment 99382 [details]
Patch

Clearing flags on attachment: 99382

Committed r90185: <http://trac.webkit.org/changeset/90185>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.