Plumbing explicit share groups through context creation

Plumbing explicit share groups through context creation

Requires corresponding Blink changes, which is why the new interfaces are hidden
behind #ifdefs.

BUG=127940
R=kbr@chromium.org, piman@chromium.org, sievers@chromium.org

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

[bajones, 2013-12-10 19:28:25]

The Blink changes will be more involved, but this side of things is reasonably
straightforward, so it should be safe to land first.

[no sievers, 2013-12-10 19:53:34]

https://codereview.chromium.org/101223005/diff/1/content/common/gpu/client/we...
File content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc
(right):

https://codereview.chromium.org/101223005/diff/1/content/common/gpu/client/we...
content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc:1219: if
(!context->MaybeInitializeGL(NULL)) {
This can't be done here but has to happen on the thread where the context is
used. Specifically, it's a problem with the compositor in threaded mode and the
offscreen context provider it creates through this codepath.

It should probably be named better (BindToThread() like in l
ContextProviderCommandBuffer::BindToCurrentThread()).

[jamesr, 2013-12-10 23:44:53]

Are we ready to break webgl out of the share group? what context created through
this path still needs to be in it?

[bajones, 2013-12-11 00:30:41]

On 2013/12/10 23:44:53, jamesr wrote:
> Are we ready to break webgl out of the share group? what context created
through
> this path still needs to be in it?

We still want contexts created by the compositor to be part of the global
implicit share group, but WebGL contexts will be broken out into their own share
groups in the corresponding Blink CL.

[no sievers, 2013-12-11 00:36:26]

https://codereview.chromium.org/101223005/diff/1/content/common/gpu/client/we...
File content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc
(right):

https://codereview.chromium.org/101223005/diff/1/content/common/gpu/client/we...
content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc:413: if
(attributes_.shareResources) {
You might want to check '&& !share_context' here also, because it'd be bad if
you inserted this context here and it is actually not in the global share group
(another context would potentially use it to share with).

I'd also DCHECK() somewhere that neither this context nor share_context have
shareResources set.

https://codereview.chromium.org/101223005/diff/1/content/common/gpu/client/we...
content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc:1219: if
(!context->MaybeInitializeGL(NULL)) {
On 2013/12/10 19:53:34, sievers wrote:
> This can't be done here but has to happen on the thread where the context is
> used. Specifically, it's a problem with the compositor in threaded mode and
the
> offscreen context provider it creates through this codepath.
> 
> It should probably be named better (BindToThread() like in l
> ContextProviderCommandBuffer::BindToCurrentThread()).

Can you maybe just pull this out by calling makeContextCurrent() in
RendererWebKitPlatformSupportImpl::createOffscreenGraphicsContext3D()?

[no sievers, 2013-12-11 00:41:37]

And you want to think about context recovery for contexts that share explicitly.

You have to make sure that when you recreate the contexts that a newly created
contexts does not specify a lost context to share with.
Or is it maybe good enough to fail context creation or makeCurrent() somewhere
if share_context->IsCommandBufferContextLost()?

Otherwise you have to worry about the order. With the global share group we
handle this by removing all contexts in the list when the gpu channel is lost.

[bajones, 2013-12-11 22:56:09]

New patch up. This is a little ugly still, since for the
WebGraphicsContext3DCommandBufferImpl we now, effectively, have two mechanisms
tracking the share groups: The existing method that tracked a global common
share group, and the new method that tracks explicitly set share groups but
doesn't do anything if the context is going to be added to the old method. That
should be able to be consolidated, but maybe that's for another day? I don't
want this patch to get too big.

[piman, 2013-12-12 23:56:39]

https://codereview.chromium.org/101223005/diff/20001/content/common/gpu/clien...
File content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc
(right):

https://codereview.chromium.org/101223005/diff/20001/content/common/gpu/clien...
content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc:395: }
So, there is a race risk here.
After you release the share_group_lock, the share_context could be deleted (on
another thread), and could be the last context in gles2_share_group.
If that's the case, on the service side, we will destroy the corresponding
ContextGroup, and free all remaining texture ids and friends.
However, because you keep the gles2::ShareGroup and will use it in the
GLES2Implementation, you get into the state where the ShareGroup doesn't match
the ContextGroup any more (it could have ids allocated on the client side that
don't exist on the service side), leading to problems later on.

I think you need to keep the lock held between when you grab the context and
both:
- you create the service-side context (InitializeCommandBuffer)
- you create the client-side context (GLES2Implementation).

At that point, the interaction between share_group_'s lock and the
g_all_shared_contexts_lock is starting to become complex.

At the same time, g_all_shared_contexts is really just like another ShareGroup.
Maybe we could combine both? Have a per-GpuChannelHost ShareGroup for
WebGraphicsContext3DCommandBufferImpl that have Attributes::shareResources set
and no explicit ShareGroup, and pick that in the constructor, making the rest of
the code common. WDYT?

https://codereview.chromium.org/101223005/diff/20001/content/common/gpu/clien...
content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc:1235:
WebGraphicsContext3D* share_context) {
nit: pass a WebGraphicsContext3DCommandBufferImpl here, making it explicit to
the caller that it's responsible for ensuring the type correctness.

https://codereview.chromium.org/101223005/diff/20001/content/common/gpu/clien...
File content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.h
(right):

https://codereview.chromium.org/101223005/diff/20001/content/common/gpu/clien...
content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.h:668:
WebGraphicsContext3DCommandBufferImpl* GetAnyContext() {
nit: maybe append "Locked" to these functions to make it explicit that the lock
is expected to be held. It's not obvious at the call site that AddContext and
RemoveContext are different in that respect.

[bajones, 2013-12-13 00:17:20]

https://codereview.chromium.org/101223005/diff/20001/content/common/gpu/clien...
File content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc
(right):

https://codereview.chromium.org/101223005/diff/20001/content/common/gpu/clien...
content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc:395: }
On 2013/12/12 23:56:39, piman wrote:
> So, there is a race risk here.
> After you release the share_group_lock, the share_context could be deleted (on
> another thread), and could be the last context in gles2_share_group.
> If that's the case, on the service side, we will destroy the corresponding
> ContextGroup, and free all remaining texture ids and friends.
> However, because you keep the gles2::ShareGroup and will use it in the
> GLES2Implementation, you get into the state where the ShareGroup doesn't match
> the ContextGroup any more (it could have ids allocated on the client side that
> don't exist on the service side), leading to problems later on.
> 
> I think you need to keep the lock held between when you grab the context and
> both:
> - you create the service-side context (InitializeCommandBuffer)
> - you create the client-side context (GLES2Implementation).
> 
> At that point, the interaction between share_group_'s lock and the
> g_all_shared_contexts_lock is starting to become complex.
> 
> At the same time, g_all_shared_contexts is really just like another
ShareGroup.
> Maybe we could combine both? Have a per-GpuChannelHost ShareGroup for
> WebGraphicsContext3DCommandBufferImpl that have Attributes::shareResources set
> and no explicit ShareGroup, and pick that in the constructor, making the rest
of
> the code common. WDYT?

I like it! I was thinking about replacing g_all_shared_contexts with a
ShareGroup, but hadn't considered attaching it to the GpuChannelHost, which
simplifies object lifetime management significantly.

Thanks for the suggestion!

[bajones, 2014-01-03 21:01:46]

Alright, time for a post-holiday update! Refactored as @piman suggested so that
the WebGraphicsContext3DCommandBufferImpl always receives a ShareGroup, which
unifies much of the code. Still maintaining a map of GpuChannelHosts in order to
preserve the current the dependency tree, but even that's been simplified.

PTAL!

[bajones, 2014-01-07 19:27:01]

Review ping now that piman@ is (supposedly) no longer OOO

[piman, 2014-01-08 01:39:23]

LGTM+nit. Please make sure Daniel sees this too, since he's more familiar with
the per-GpuProcessHost share group part (in case I overlooked something).

https://codereview.chromium.org/101223005/diff/110001/content/common/gpu/clie...
File content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc
(right):

https://codereview.chromium.org/101223005/diff/110001/content/common/gpu/clie...
content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc:53:
WebGraphicsContext3DCommandBufferImpl::ShareGroup* defaultShareGroupForHost(
nit: chrome style - DefaultShareGroupForHost. Maybe GetDefaultShareGroupForHost
?

[Ken Russell (switch to Gerrit), 2014-01-08 02:40:38]

Overall this looks good.

It's been a while since I've looked at this code, so take the below with a grain
of salt, but it seems there's one problem.

https://codereview.chromium.org/101223005/diff/110001/content/common/gpu/clie...
File content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc
(right):

https://codereview.chromium.org/101223005/diff/110001/content/common/gpu/clie...
content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc:53:
WebGraphicsContext3DCommandBufferImpl::ShareGroup* defaultShareGroupForHost(
Since a lock is needed for the share group map, it seems risky and racy to be
returning a raw pointer to the ShareGroup from this function. Since the
WebGraphicsContext3DCommandBufferImpl ref-counts the ShareGroup, it seems
plausible the WGC3D might go away and the ShareGroup be deleted between the time
this function returns and the time the caller ref-counts the ShareGroup. I think
this can be solved by returning a scoped_refptr<WGC3DCBI::ShareGroup*>.

[no sievers, 2014-01-08 14:03:53]

Looks good with a few comments.

https://codereview.chromium.org/101223005/diff/110001/content/common/gpu/clie...
File content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc
(right):

https://codereview.chromium.org/101223005/diff/110001/content/common/gpu/clie...
content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc:53:
WebGraphicsContext3DCommandBufferImpl::ShareGroup* defaultShareGroupForHost(
On 2014/01/08 02:40:39, Ken Russell wrote:
> Since a lock is needed for the share group map, it seems risky and racy to be
> returning a raw pointer to the ShareGroup from this function. Since the
> WebGraphicsContext3DCommandBufferImpl ref-counts the ShareGroup, it seems
> plausible the WGC3D might go away and the ShareGroup be deleted between the
time
> this function returns and the time the caller ref-counts the ShareGroup. I
think
> this can be solved by returning a scoped_refptr<WGC3DCBI::ShareGroup*>.

Yes, good catch. For example:
1) WGC3D is constructed on impl thread and gets raw pointer to default share
group, but does not hold a reference yet
2) OnGpuChannelLost() is called on main thread for another context (in the
group) which clears g_default_share_groups
3) The context from 2) is deleted on the main thread which releases the last
reference to ShareGroup causing it to get deleted

https://codereview.chromium.org/101223005/diff/110001/content/common/gpu/clie...
content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc:252: if
(share_context) {
DCHECK(!attributes_.shareResources);

On the other hand if |share_context| was created with shareResources, that seems
confusing but looks like it would still work as expected, right? (Identical to
creating this context with shareResources == true).

https://codereview.chromium.org/101223005/diff/110001/content/common/gpu/clie...
content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc:253:
share_group_ = share_context->share_group_;
It's possible that host_ != share_context->host_ if the context was lost.
Should we make sure makeCurrent() later will fail consistently in that case by
setting host_ to NULL here?
Or should it create a new share group here if share_context's channel was lost?

With the default share group the recovery is easier here, because it doesn't
matter which context gets recreated first and all new contexts will end up in
the same new share group once the channel was reestablished.

If you explicitly pass in the share group (which might be referencing a lost
channel) it's maybe a bit trickier, because if you create context B by passing
in A, you have to recreate A first.

So it also depends on what the logic for this will be like in Blink.

https://codereview.chromium.org/101223005/diff/110001/content/common/gpu/clie...
content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc:393:
gles2_share_group = share_context->GetImplementation()->share_group();
Can you keep the DCHECK, i.e. DCHECK(!share_context || gles2_share_group.get())?

https://codereview.chromium.org/101223005/diff/110001/content/common/gpu/clie...
content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc:425: 
Can we still release the lock here so it isn't held while we initialize the
gles2 impl like before?

[bajones, 2014-01-08 21:24:36]

https://codereview.chromium.org/101223005/diff/110001/content/common/gpu/clie...
File content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc
(right):

https://codereview.chromium.org/101223005/diff/110001/content/common/gpu/clie...
content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc:53:
WebGraphicsContext3DCommandBufferImpl::ShareGroup* defaultShareGroupForHost(
On 2014/01/08 02:40:39, Ken Russell wrote:
> Since a lock is needed for the share group map, it seems risky and racy to be
> returning a raw pointer to the ShareGroup from this function. Since the
> WebGraphicsContext3DCommandBufferImpl ref-counts the ShareGroup, it seems
> plausible the WGC3D might go away and the ShareGroup be deleted between the
time
> this function returns and the time the caller ref-counts the ShareGroup. I
think
> this can be solved by returning a scoped_refptr<WGC3DCBI::ShareGroup*>.

Good catch. Fixed.

https://codereview.chromium.org/101223005/diff/110001/content/common/gpu/clie...
content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc:252: if
(share_context) {
On 2014/01/08 14:03:54, sievers wrote:
> DCHECK(!attributes_.shareResources);
> 
> On the other hand if |share_context| was created with shareResources, that
seems
> confusing but looks like it would still work as expected, right? (Identical to
> creating this context with shareResources == true).

Yes, that's correct. Slightly confusing, but I think it's well behaved as far as
these things go. (Accidentally sharing with more contexts than intended is
probably better than accidentally sharing with less than expected.)

https://codereview.chromium.org/101223005/diff/110001/content/common/gpu/clie...
content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc:253:
share_group_ = share_context->share_group_;
On 2014/01/08 14:03:54, sievers wrote:
> It's possible that host_ != share_context->host_ if the context was lost.
> Should we make sure makeCurrent() later will fail consistently in that case by
> setting host_ to NULL here?
> Or should it create a new share group here if share_context's channel was
lost?
> 
> With the default share group the recovery is easier here, because it doesn't
> matter which context gets recreated first and all new contexts will end up in
> the same new share group once the channel was reestablished.
> 
> If you explicitly pass in the share group (which might be referencing a lost
> channel) it's maybe a bit trickier, because if you create context B by passing
> in A, you have to recreate A first.
> 
> So it also depends on what the logic for this will be like in Blink.

Hm... thanks for pointing this out. I need to think about it a bit further.

[no sievers, 2014-01-09 13:39:41]

https://codereview.chromium.org/101223005/diff/110001/content/common/gpu/clie...
File content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc
(right):

https://codereview.chromium.org/101223005/diff/110001/content/common/gpu/clie...
content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc:252: if
(share_context) {
On 2014/01/08 21:24:37, bajones wrote:
> On 2014/01/08 14:03:54, sievers wrote:
> > DCHECK(!attributes_.shareResources);
> > 
> > On the other hand if |share_context| was created with shareResources, that
> seems
> > confusing but looks like it would still work as expected, right? (Identical
to
> > creating this context with shareResources == true).
> 
> Yes, that's correct. Slightly confusing, but I think it's well behaved as far
as
> these things go. (Accidentally sharing with more contexts than intended is
> probably better than accidentally sharing with less than expected.)

Sounds good. I'd still add the DCHECK(!attributes_.shareResources) though since
it's different (compared to share_context->shareResources == true) and it would
be illegal to request to be in the global share group and a specific other one
at the same time.

https://codereview.chromium.org/101223005/diff/110001/content/common/gpu/clie...
content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc:253:
share_group_ = share_context->share_group_;
On 2014/01/08 21:24:37, bajones wrote:
> On 2014/01/08 14:03:54, sievers wrote:
> > It's possible that host_ != share_context->host_ if the context was lost.
> > Should we make sure makeCurrent() later will fail consistently in that case
by
> > setting host_ to NULL here?
> > Or should it create a new share group here if share_context's channel was
> lost?
> > 
> > With the default share group the recovery is easier here, because it doesn't
> > matter which context gets recreated first and all new contexts will end up
in
> > the same new share group once the channel was reestablished.
> > 
> > If you explicitly pass in the share group (which might be referencing a lost
> > channel) it's maybe a bit trickier, because if you create context B by
passing
> > in A, you have to recreate A first.
> > 
> > So it also depends on what the logic for this will be like in Blink.
> 
> Hm... thanks for pointing this out. I need to think about it a bit further.

Can we just do this maybe to mimick the behavior of the default share group:
- passing in a share_context whose channel is lost will cause this context to be
created with a fresh ShareGroup
- in Blink we always track the latest context that was created to use as a
share_context

When the channel is lost, we know that all contexts will eventually be recreated
and therefore all will end up using the same new ShareGroup at that point.

[bajones, 2014-01-10 23:30:59]

On 2014/01/09 13:39:41, sievers wrote:
> Can we just do this maybe to mimick the behavior of the default share group:
> - passing in a share_context whose channel is lost will cause this context to
be
> created with a fresh ShareGroup
> - in Blink we always track the latest context that was created to use as a
> share_context
> 
> When the channel is lost, we know that all contexts will eventually be
recreated
> and therefore all will end up using the same new ShareGroup at that point.

Talked this over with kbr@, and at this level in the code it really only makes
sense to treat a lost share_context as an error. The reason being that when a
WebGL rendering context is restored from a lost state the WebGraphicsContext3D
backing it is discarded and a new one is created. As such any attempt to
preserve share groups of lost and restored contexts will need to happen in
blink.

I've added a check for that case in the create function and will return NULL if
we hit it, and I've also added the requested DCHECK.

[Ken Russell (switch to Gerrit), 2014-01-10 23:48:10]

On 2014/01/10 23:30:59, bajones wrote:
> On 2014/01/09 13:39:41, sievers wrote:
> > Can we just do this maybe to mimick the behavior of the default share group:
> > - passing in a share_context whose channel is lost will cause this context
to
> be
> > created with a fresh ShareGroup
> > - in Blink we always track the latest context that was created to use as a
> > share_context
> > 
> > When the channel is lost, we know that all contexts will eventually be
> recreated
> > and therefore all will end up using the same new ShareGroup at that point.
> 
> Talked this over with kbr@, and at this level in the code it really only makes
> sense to treat a lost share_context as an error. The reason being that when a
> WebGL rendering context is restored from a lost state the WebGraphicsContext3D
> backing it is discarded and a new one is created. As such any attempt to
> preserve share groups of lost and restored contexts will need to happen in
> blink.
> 
> I've added a check for that case in the create function and will return NULL
if
> we hit it, and I've also added the requested DCHECK.

I hope this was the right decision and won't result in race conditions. Others,
please think this through carefully. If it would be more robust to do it the
other way, i.e., create a new share group behind the scenes, we should do that.

[piman, 2014-01-11 01:10:18]

On 2014/01/10 23:48:10, Ken Russell wrote:
> On 2014/01/10 23:30:59, bajones wrote:
> > On 2014/01/09 13:39:41, sievers wrote:
> > > Can we just do this maybe to mimick the behavior of the default share
group:
> > > - passing in a share_context whose channel is lost will cause this context
> to
> > be
> > > created with a fresh ShareGroup
> > > - in Blink we always track the latest context that was created to use as a
> > > share_context
> > > 
> > > When the channel is lost, we know that all contexts will eventually be
> > recreated
> > > and therefore all will end up using the same new ShareGroup at that point.
> > 
> > Talked this over with kbr@, and at this level in the code it really only
makes
> > sense to treat a lost share_context as an error. The reason being that when
a
> > WebGL rendering context is restored from a lost state the
WebGraphicsContext3D
> > backing it is discarded and a new one is created. As such any attempt to
> > preserve share groups of lost and restored contexts will need to happen in
> > blink.
> > 
> > I've added a check for that case in the create function and will return NULL
> if
> > we hit it, and I've also added the requested DCHECK.
> 
> I hope this was the right decision and won't result in race conditions.
Others,
> please think this through carefully. If it would be more robust to do it the
> other way, i.e., create a new share group behind the scenes, we should do
that.

It's hard to tell which one is less prone to races while there's no code using
this for now.
But remember that a context can be lost asynchronously at any point, and,
especially if you use multiple threads, it's hard to get a consistent view
across all contexts in the same share group.

I think, as long as the calling code would deal with the failure of the context
creation by itself recreating a share group and "blacklisting" the existing
share group, then you should be ok. Keeping in mind that you can't rely on a
creation failure to cause all future context creation with the same group to
fail (since other contexts in the share group may not know yet that they're
lost).

[bajones, 2014-02-19 21:34:10]

Revisiting this now that the GraphicsContext3D refactor is done. The Blink side
of this change still needs to address some of the issues that have been brought
up, but I believe that the Chrome-side code in this CL should be ready to go in.

kbr@, sievers@: Technically piman@'s stamp is all that I need to commit but I'd
appreciate a LG from both of you before I land this.

[Ken Russell (switch to Gerrit), 2014-02-19 22:38:25]

LGTM as far as I can tell.

It would be much better if more exhaustive tests existed which lost the context
from the service side at unexpected times -- for example, after executing N
commands, and have N vary from 1 to, say, 200, for some simple WebGL example.
These tests should be pixel tests, in order to ensure that the rendering
completed as expected if it was actually supposed to.

It would be great if you'd file a bug about writing such tests if you can think
of useful scenarios to test.

https://codereview.chromium.org/101223005/diff/450001/content/common/gpu/clie...
File content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc
(right):

https://codereview.chromium.org/101223005/diff/450001/content/common/gpu/clie...
content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc:1187:
return NULL;
If this check is necessary, then it seems to me there'll be a race if the
share_context is lost at an unexpected time between now and when this
WebGraphicsContext3DCommandBufferImpl is initialized.

https://codereview.chromium.org/101223005/diff/450001/content/common/gpu/clie...
File content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.h
(right):

https://codereview.chromium.org/101223005/diff/450001/content/common/gpu/clie...
content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.h:83: // in
use the share group's lock should be aquired before calling this
"in use" -> "in use," for clarity

[no sievers, 2014-02-24 19:37:25]

Sorry for the delay. Just one thing...

https://codereview.chromium.org/101223005/diff/510001/content/common/gpu/clie...
File content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc
(right):

https://codereview.chromium.org/101223005/diff/510001/content/common/gpu/clie...
content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc:398:
share_group_->AddContextLocked(this);
It's safer to move this further down until after we created real_gl_.
Otherwise we might fail and return in line 404 and another context trying to
share with this one will crash in line 396 above (GetImplementation() returning
NULL).

https://codereview.chromium.org/101223005/diff/510001/content/common/gpu/clie...
File content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.h
(right):

https://codereview.chromium.org/101223005/diff/510001/content/common/gpu/clie...
content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.h:113:
~ShareGroup();
nit: virtual

[bajones, 2014-02-24 20:22:34]

Done and Done.

[no sievers, 2014-02-24 21:09:34]

lgtm

[bajones, 2014-02-24 21:14:19]

The CQ bit was checked by bajones@chromium.org

[commit-bot: I haz the power, 2014-02-24 21:24:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bajones@chromium.org/101223005/570001

[commit-bot: I haz the power, 2014-02-25 01:29:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bajones@chromium.org/101223005/570001

[commit-bot: I haz the power, 2014-02-25 04:34:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-25 04:34:08]

Try jobs failed on following builders: mac_chromium_rel

[bajones, 2014-02-25 18:19:50]

Committed patchset #10 manually as r253204 (presubmit successful).