Destroy SurfaceDisplayOutputSurface's surface in its destructor

Destroy SurfaceDisplayOutputSurface's surface in its destructor

This prevents a leak and avoids hitting a DCHECK.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=283997

[jamesr, 2014-07-17 20:50:32]

I tried to do this, but it failed because destroying the surface will cause the
factory to attempt to return resources which is a virtual call back through this
guy's client, which is already dead by the time we get here.

[jbauman, 2014-07-17 22:06:26]

On 2014/07/17 20:50:32, jamesr wrote:
> I tried to do this, but it failed because destroying the surface will cause
the
> factory to attempt to return resources which is a virtual call back through
this
> guy's client, which is already dead by the time we get here.

Would it make sense to NULL out the client_ and avoid doing the callback in this
case?

[jamesr, 2014-07-17 22:25:37]

The client_ is supposed to outlive the implementation so I think in this case
the client is technically violating the contract - i.e. it's becoming unusable
while the SDOS is still alive.  I think this is true of the OutputSurface
clients in general but it's not a problem in practice.

Would we ever care about the returned resources in the case where we're shutting
down the output surface?  I think not, so maybe we should just null client_
before destroying the surface and then null check in the callback.  That doesn't
feel 100% satisfying but is probably good enough.

[jbauman, 2014-07-17 22:32:29]

PTAL.

On 2014/07/17 22:25:37, jamesr wrote:
> The client_ is supposed to outlive the implementation so I think in this case
> the client is technically violating the contract - i.e. it's becoming unusable
> while the SDOS is still alive.  I think this is true of the OutputSurface
> clients in general but it's not a problem in practice.

Yeah, LayerTreeHostImpl member destruction order seem pretty sensitive.
> 
> Would we ever care about the returned resources in the case where we're
shutting
> down the output surface?  I think not, so maybe we should just null client_
> before destroying the surface and then null check in the callback.  That
doesn't
> feel 100% satisfying but is probably good enough.

We currently have to destroy the ResourceProvider whenever the OutputSurface is
shut down, and that deletes all the textures and returns any delegated resources
to that compositor's child, so there's no real point to returning the resources.

[jamesr, 2014-07-17 22:38:48]

lgtm

[jbauman, 2014-07-17 22:39:54]

The CQ bit was checked by jbauman@chromium.org

[commit-bot: I haz the power, 2014-07-17 22:41:27]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jbauman@chromium.org/400063002/20001

[commit-bot: I haz the power, 2014-07-18 03:39:35]

Change committed as 283997