Workaround to prevent crashes when destroying CGL contexts

Workaround to prevent crashes when destroying CGL contexts

Observation 1: It was discovered in crbug.com/410402 that deleting
the GL textures bound to IOSurfaces before releasing the CGL context
from the CAOpenGLLayer can result in crashes.

Observation 2: It was discovered in crbug.com/411782 that not deleting
the GL textures bounds to IOSurfaces before destroying the CGL context
can result in crashes.

As way speculative workaround for this, retain the CGL context when the
texture is created (when it is known to be valid), and the post a task to
- make that CGL context current
- delete the GL texture
- make no CGL context current
- release the CGL context
- release the IOSurface used by the GL texture

The theory here is that by doing this in a posted task, having a fresh stack
and having manually retained the CGL context when it was known-valid
will work around the crash in Observation 1, and manually destroying the
texture while its IOSurface is still retained and before the CGL context is
destroyed will work around the crash in Observation 2.

BUG=411782
Committed: https://crrev.com/282049cc52cd09cca07b8bd838f448174c7a87f5
Cr-Commit-Position: refs/heads/master@{#294275}

[ccameron, 2014-09-09 21:23:44]

ccameron@chromium.org changed reviewers:
+ kbr@chromium.org

[ccameron, 2014-09-09 21:26:59]

I tested the scenarios that were failing from "Observation 1", but without any
repro for the "Observation 2" crash, that part of the fix is still something of
a shot in the dark. That said, I do like how this explicitly tracks which
IOSurface is bound, rather than keeping around the "is dirty" state.

[Ken Russell (switch to Gerrit), 2014-09-10 01:04:08]

https://codereview.chromium.org/558803002/diff/1/content/browser/compositor/i...
File content/browser/compositor/io_surface_layer_mac.mm (right):

https://codereview.chromium.org/558803002/diff/1/content/browser/compositor/i...
content/browser/compositor/io_surface_layer_mac.mm:54: // Finally release down
the IOSurface.
"release the IOSurface"

https://codereview.chromium.org/558803002/diff/1/content/browser/compositor/i...
content/browser/compositor/io_surface_layer_mac.mm:488:
CFRetain(io_surface_texture_io_surface_);
I'm dubious that these calls to CGLRetainContext and CFRetain are balanced
correctly. Since the Scoped[CF]TypeRefs are bound as pass-by-value, base::Bind()
should be creating a copy of the Scoped[CF]TypeRef objects, and thereby calling
CGLRetainContext/CFRetain in the copy constructor. The reset() calls in
DestroyTexture should then call CGLReleaseContext / CFRelease. Can you please
double-check this perhaps with the OpenGL Driver Monitor and make sure we aren't
leaking IOSurfaces (or check the reference counts of the objects manually in
DestroyTexture)?

[ccameron, 2014-09-10 01:30:03]

No idea what I was thinking with that. I'm re-running the tests with that fixed
-- does this lg if they pass?

https://codereview.chromium.org/558803002/diff/1/content/browser/compositor/i...
File content/browser/compositor/io_surface_layer_mac.mm (right):

https://codereview.chromium.org/558803002/diff/1/content/browser/compositor/i...
content/browser/compositor/io_surface_layer_mac.mm:54: // Finally release down
the IOSurface.
On 2014/09/10 01:04:08, Ken Russell wrote:
> "release the IOSurface"

Done.

https://codereview.chromium.org/558803002/diff/1/content/browser/compositor/i...
content/browser/compositor/io_surface_layer_mac.mm:488:
CFRetain(io_surface_texture_io_surface_);
On 2014/09/10 01:04:08, Ken Russell wrote:
> I'm dubious that these calls to CGLRetainContext and CFRetain are balanced
> correctly. Since the Scoped[CF]TypeRefs are bound as pass-by-value,
base::Bind()
> should be creating a copy of the Scoped[CF]TypeRef objects, and thereby
calling
> CGLRetainContext/CFRetain in the copy constructor. The reset() calls in
> DestroyTexture should then call CGLReleaseContext / CFRelease. Can you please
> double-check this perhaps with the OpenGL Driver Monitor and make sure we
aren't
> leaking IOSurfaces (or check the reference counts of the objects manually in
> DestroyTexture)?

They're completely and embarrassingly wrong. Fixed.

[Ken Russell (switch to Gerrit), 2014-09-10 04:06:39]

Have you attached the OpenGL Driver Monitor to the browser process and made sure
that the OpenGL context and IOSurface count don't increase over time as windows
are opened and closed?

This will look good to me once the issue below is fixed.

https://codereview.chromium.org/558803002/diff/20001/content/browser/composit...
File content/browser/compositor/io_surface_layer_mac.mm (right):

https://codereview.chromium.org/558803002/diff/20001/content/browser/composit...
content/browser/compositor/io_surface_layer_mac.mm:356:
io_surface_texture_context_.reset(CGLRetainContext(glContext));
Sorry I didn't notice this the first time through, but the CGLRetainContext call
here looks wrong too. io_surface_texture_context_ will call CGLRetainContext on
the incoming context itself.

[ccameron, 2014-09-10 07:53:26]

Thanks! I gave this a run with the GL driver monitor, and this doesn't have
leaks.

https://codereview.chromium.org/558803002/diff/20001/content/browser/composit...
File content/browser/compositor/io_surface_layer_mac.mm (right):

https://codereview.chromium.org/558803002/diff/20001/content/browser/composit...
content/browser/compositor/io_surface_layer_mac.mm:356:
io_surface_texture_context_.reset(CGLRetainContext(glContext));
On 2014/09/10 04:06:38, Ken Russell wrote:
> Sorry I didn't notice this the first time through, but the CGLRetainContext
call
> here looks wrong too. io_surface_texture_context_ will call CGLRetainContext
on
> the incoming context itself.

This one is correct -- there is no implicit retain when initializing a
CFTypeRef. I verified with the OpenGL driver monitor -- we get +1 context per
window, then -1 as we close windows.

[Ken Russell (switch to Gerrit), 2014-09-10 22:24:52]

LGTM with one more comment.

https://codereview.chromium.org/558803002/diff/20001/content/browser/composit...
File content/browser/compositor/io_surface_layer_mac.mm (right):

https://codereview.chromium.org/558803002/diff/20001/content/browser/composit...
content/browser/compositor/io_surface_layer_mac.mm:356:
io_surface_texture_context_.reset(CGLRetainContext(glContext));
On 2014/09/10 07:53:26, ccameron1 wrote:
> On 2014/09/10 04:06:38, Ken Russell wrote:
> > Sorry I didn't notice this the first time through, but the CGLRetainContext
> call
> > here looks wrong too. io_surface_texture_context_ will call CGLRetainContext
> on
> > the incoming context itself.
> 
> This one is correct -- there is no implicit retain when initializing a
> CFTypeRef. I verified with the OpenGL driver monitor -- we get +1 context per
> window, then -1 as we close windows.

OK, sorry about my confusion, but it looks like you could pass
base::scoped_policy::RETAIN here and let the traits object do the work, which
would be more obvious.

[ccameron, 2014-09-10 22:49:14]

Excellent idea -- thanks, that makes it much more clear.

[ccameron, 2014-09-10 22:49:19]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2014-09-10 22:51:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/558803002/40001

[commit-bot: I haz the power, 2014-09-11 00:50:39]

Committed patchset #3 (id:40001) as 85cbb6b258c493bd15a9d77bb52bfbfcfa4005c4

[commit-bot: I haz the power, 2014-09-11 00:53:41]

Patchset 3 (id:??) landed as
https://crrev.com/282049cc52cd09cca07b8bd838f448174c7a87f5
Cr-Commit-Position: refs/heads/master@{#294275}