Fixed missing code in EnqueueTextureForDeletion....

Fixed missing code in EnqueueTextureForDeletion.

BUG=38945
TEST=reloaded Pepper 3D plugins repeatedly

NOTE: encountered unrelated problems while testing. These changes
should be stress tested with CA plugins.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=42380

[Ken Russell (switch to Gerrit), 2010-03-23 02:44:16]

Please review. Thanks.

I wasn't able to stress test these changes with the Pepper 3D plugins due to
other unrelated bugs. Could you either try them with CA plugins and verify that
the browser process's RSS stays steady during many page reloads, or let me know
how you test CA plugins? Thanks.

[stuartmorgan, 2010-03-23 15:28:44]

There's no guarantee that the container manager's Draw will be called after the
last container is deleted (in fact it seems likely that it wouldn't) in which
case this would still leak.

Also, it looks like two size changes in a row without an intervening draw would
cause a texture to be destroyed twice, which I assume is bad.

What's the advantage of having the manager handle the texture deletion? Since it
didn't create them, it seems oddly asymmetrical. Wouldn't it make more sense for
each container to have a variable containing its previous texture, make
EnqueueTextureForDeletion (which would need renaming) move texture_ to that
variable, and have the container's Draw and destructor destroy that texture?

Why does the previous texture need the extra lifespan, by the way, given that
the texture is only used in Draw?

[Ken Russell (switch to Gerrit), 2010-03-23 18:52:52]

On 2010/03/23 15:28:44, stuartmorgan wrote:
> There's no guarantee that the container manager's Draw will be called after
the
> last container is deleted (in fact it seems likely that it wouldn't) in which
> case this would still leak.

When the OpenGL context associated with the CAOpenGLLayer drawing these
containers is destroyed, the texture objects will all be reclaimed, so any leak
would only be transient.

> Also, it looks like two size changes in a row without an intervening draw
would
> cause a texture to be destroyed twice, which I assume is bad.

The "if (texture_)" guard in
AcceleratedSurfaceContainerMac::EnqueueTextureForDeletion prevents this.

> What's the advantage of having the manager handle the texture deletion? Since
it
> didn't create them, it seems oddly asymmetrical. Wouldn't it make more sense
for
> each container to have a variable containing its previous texture, make
> EnqueueTextureForDeletion (which would need renaming) move texture_ to that
> variable, and have the container's Draw and destructor destroy that texture?
> 
> Why does the previous texture need the extra lifespan, by the way, given that
> the texture is only used in Draw?

The OpenGL context in which the texture was allocated needs to be current when
the texture is deleted. Unfortunately, on Mac OS X there are essentially three
kinds of OpenGL contexts (AGL, CGL and NSOpenGLContext) so it is not possible
(or, at least, not easy) to save and restore them. For this reason I was
extremely concerned about making the CGLContextObj current at arbitrary points
in time. This limits the potential time for deleting the texture to the draw
operation. The texture can not be deleted in the container's destructor, which
is the reason to make the container manager responsible for the texture
deletion.

[stuartmorgan, 2010-03-23 19:55:34]

LGTM.

I built locally and reloaded a CA plugin over and over, and everything looked
good.

On 2010/03/23 18:52:52, kbr wrote:
> When the OpenGL context associated with the CAOpenGLLayer drawing these
> containers is destroyed, the texture objects will all be reclaimed, so any
leak
> would only be transient.

Ah, I see. Maybe add a comment in the destructor or where the deletion loop is
explaining that, so it doesn't look like a leak to the untrained eye?

> The "if (texture_)" guard in
> AcceleratedSurfaceContainerMac::EnqueueTextureForDeletion prevents this.

Doh, sorry.

> The texture can not be deleted in the container's destructor, which
> is the reason to make the container manager responsible for the texture
> deletion.

That makes sense; sorry for the confusion on my part.

[Ken Russell (switch to Gerrit), 2010-03-23 21:04:43]

On 2010/03/23 19:55:34, stuartmorgan wrote:
> LGTM.
> 
> I built locally and reloaded a CA plugin over and over, and everything looked
> good.

Great. Thanks for testing that.

> On 2010/03/23 18:52:52, kbr wrote:
> > When the OpenGL context associated with the CAOpenGLLayer drawing these
> > containers is destroyed, the texture objects will all be reclaimed, so any
> leak
> > would only be transient.
> 
> Ah, I see. Maybe add a comment in the destructor or where the deletion loop is
> explaining that, so it doesn't look like a leak to the untrained eye?

Added a comment above the deletion loop in
AcceleratedSurfaceContainerManagerMac::Draw.

> > The texture can not be deleted in the container's destructor, which
> > is the reason to make the container manager responsible for the texture
> > deletion.
> 
> That makes sense; sorry for the confusion on my part.

No apologies; it's a good question and perhaps something to be revisited.

[stuartmorgan, 2010-03-23 21:10:37]

LGTM