Fix crash caused by concurrent access to framebuffer_combo_complete_map_.

Fix crash caused by concurrent access to framebuffer_combo_complete_map_.

The current implementation of framebuffer completeness testing uses a single global hash_map as a cache. This leads to races in WebView when GPU and render threads try to access the cache concurrently.

BUG=518858
Committed: https://crrev.com/fc199b47cb5b74c9f2ccf3a6316fc9b7009c6904
Cr-Commit-Position: refs/heads/master@{#344940}

[Tobias Sargeant, 2015-08-10 15:54:07]

tobiasjs@chromium.org changed reviewers:
+ sievers@chromium.org

[Tobias Sargeant, 2015-08-10 15:54:08]

PTAL. Sorry the diff isn't very clean; I wanted to avoid locking twice. I also
don't think it's going to be possible to write a test for this.

[no sievers, 2015-08-10 19:59:25]

Since it's an optimization, can we avoid the lock and just create a map per
thread

[no sievers, 2015-08-10 19:59:37]

On 2015/08/10 19:59:25, sievers wrote:
> Since it's an optimization, can we avoid the lock and just create a map per
> thread

+ '?'

[Tobias Sargeant, 2015-08-10 20:05:11]

On 2015/08/10 19:59:37, sievers wrote:
> On 2015/08/10 19:59:25, sievers wrote:
> > Since it's an optimization, can we avoid the lock and just create a map per
> > thread
> 
> + '?'

A thread local was my first thought, but I got the impression that they were
discouraged. Will rework that way.

[Tobias Sargeant, 2015-08-10 20:12:31]

On 2015/08/10 20:05:11, Tobias Sargeant wrote:
> On 2015/08/10 19:59:37, sievers wrote:
> > On 2015/08/10 19:59:25, sievers wrote:
> > > Since it's an optimization, can we avoid the lock and just create a map
per
> > > thread
> > 
> > + '?'
> 
> A thread local was my first thought, but I got the impression that they were
> discouraged. Will rework that way.

Is testing framebuffer completeness all that slow? Maybe we could just remove
the cache (on Android at least).

[no sievers, 2015-08-10 21:16:46]

On 2015/08/10 20:12:31, Tobias Sargeant wrote:
> On 2015/08/10 20:05:11, Tobias Sargeant wrote:
> > On 2015/08/10 19:59:37, sievers wrote:
> > > On 2015/08/10 19:59:25, sievers wrote:
> > > > Since it's an optimization, can we avoid the lock and just create a map
> per
> > > > thread
> > > 
> > > + '?'
> > 
> > A thread local was my first thought, but I got the impression that they were
> > discouraged. Will rework that way.
> 

You could also pass it in explicitly. We do that for some other things.
GpuChannelManager could pass it through to each context group or so (or have a
getter).


> Is testing framebuffer completeness all that slow? Maybe we could just remove
> the cache (on Android at least).

I don't know, my guess is that it's driver dependent. You'd think that they
could do what we do or something faster.
At the same time I wouldn't be surprised if calling into
CheckFramebufferStatus() ends up being expensive on some drivers :)

If you disable it, you don't need to do it for all of Android. And you don't
even need to do it for all of WebView, but you could instead make sure that only
one thread allows the cache (probably not the RenderThread).

[Tobias Sargeant, 2015-08-14 12:42:47]

How's this? Given there's one FramebufferManager per ContextGroup, so it seems
like a natural place to put this cache.

I also removed (in PS2) the tests that assert that there exists a cache for
GetStatus. It seems like we shouldn't be testing that, and the only thing that
needs to be asserted is that error results are not cached (although I can't say
I can see why the driver might tell us that a FB is incomplete once, and then
say things are fine subsequently, without the attachments changing).

[no sievers, 2015-08-17 18:00:32]

On 2015/08/14 12:42:47, Tobias Sargeant wrote:
> How's this? Given there's one FramebufferManager per ContextGroup, so it seems
> like a natural place to put this cache.
> 

Technically this regresses it because now all the different client groups will
have to figure out valid combinations separately.
It's worse in practice since nowadays we mostly isolate contexts into different
ContextGroups (webgl, render compositor, and browser compositor anyways since
it's in a different process).
Do you think we could stash it into GpuChannelManager? Then it would be more
globally shared.


> I also removed (in PS2) the tests that assert that there exists a cache for
> GetStatus. It seems like we shouldn't be testing that, and the only thing that
> needs to be asserted is that error results are not cached (although I can't
say
> I can see why the driver might tell us that a FB is incomplete once, and then
> say things are fine subsequently, without the attachments changing).

[Tobias Sargeant, 2015-08-17 18:22:20]

On 2015/08/17 18:00:32, sievers wrote:

> Technically this regresses it because now all the different client groups will
> have to figure out valid combinations separately.
> It's worse in practice since nowadays we mostly isolate contexts into
different
> ContextGroups (webgl, render compositor, and browser compositor anyways since
> it's in a different process).
> Do you think we could stash it into GpuChannelManager? Then it would be more
> globally shared.

I'd be happy to change this, but I'm not sure how to plumb it through. Could you
provide me with a pointer, please?

[no sievers, 2015-08-17 19:32:30]

On 2015/08/17 18:22:20, Tobias Sargeant wrote:
> On 2015/08/17 18:00:32, sievers wrote:
> 
> > Technically this regresses it because now all the different client groups
will
> > have to figure out valid combinations separately.
> > It's worse in practice since nowadays we mostly isolate contexts into
> different
> > ContextGroups (webgl, render compositor, and browser compositor anyways
since
> > it's in a different process).
> > Do you think we could stash it into GpuChannelManager? Then it would be more
> > globally shared.
> 
> I'd be happy to change this, but I'm not sure how to plumb it through. Could
you
> provide me with a pointer, please?

You can do the same thing as with the shader translator cache here in
gpu_command_buffer_stub.cc (have the channel manager own the cache and provide a
getter method):

 context_group_ = new gpu::gles2::ContextGroup(
        mailbox_manager,
        new GpuCommandBufferMemoryTracker(channel),
        channel_->gpu_channel_manager()->shader_translator_cache()

For the in-process versions maybe this can go into
InProcessCommandBuffer::Service.

[Tobias Sargeant, 2015-08-18 15:10:11]

> You can do the same thing as with the shader translator cache here in
> gpu_command_buffer_stub.cc (have the channel manager own the cache and provide
a
> getter method):

Ok. Done. It makes the CL a lot bigger though. Could you please take a look and
tell me what you think?

[no sievers, 2015-08-19 00:43:13]

This is nice!

https://codereview.chromium.org/1278333003/diff/120001/gpu/command_buffer/ser...
File gpu/command_buffer/service/framebuffer_completeness_cache.cc (right):

https://codereview.chromium.org/1278333003/diff/120001/gpu/command_buffer/ser...
gpu/command_buffer/service/framebuffer_completeness_cache.cc:1: // Copyright (c)
2015 The Chromium Authors. All rights reserved.
nit: drop '(c)'

https://codereview.chromium.org/1278333003/diff/120001/gpu/command_buffer/ser...
gpu/command_buffer/service/framebuffer_completeness_cache.cc:5: #include
"gpu/command_buffer/service/framebuffer_completeness_cache.h"
nit: newline after #include

https://codereview.chromium.org/1278333003/diff/120001/gpu/command_buffer/ser...
File gpu/command_buffer/service/framebuffer_completeness_cache.h (right):

https://codereview.chromium.org/1278333003/diff/120001/gpu/command_buffer/ser...
gpu/command_buffer/service/framebuffer_completeness_cache.h:1: // Copyright (c)
2015 The Chromium Authors. All rights reserved.
nit: drop '(c)' in new headers

https://codereview.chromium.org/1278333003/diff/120001/gpu/command_buffer/ser...
gpu/command_buffer/service/framebuffer_completeness_cache.h:6: #define
GPU_COMMAND_BUFFER_COMMON_FRAMEBUFFER_COMPLETENESS_CACHE_H_
nit: s/BUFFER_COMMON/BUFFER_SERVICE

https://codereview.chromium.org/1278333003/diff/120001/gpu/command_buffer/ser...
File gpu/command_buffer/service/framebuffer_manager.cc (right):

https://codereview.chromium.org/1278333003/diff/120001/gpu/command_buffer/ser...
gpu/command_buffer/service/framebuffer_manager.cc:496: #if defined(OS_MACOSX)
Doesn't this break unittesting on Mac?
You could move the #ifdef to the construction call site for production code
instead.

https://codereview.chromium.org/1278333003/diff/120001/gpu/command_buffer/ser...
File gpu/command_buffer/service/framebuffer_manager_unittest.cc (left):

https://codereview.chromium.org/1278333003/diff/120001/gpu/command_buffer/ser...
gpu/command_buffer/service/framebuffer_manager_unittest.cc:843:
framebuffer_->GetStatus(texture_manager_.get(), GL_FRAMEBUFFER);
I think all of these GetStatus() calls are important, because they test if the
cache actually works and we end up *not* calling
gl_->CheckFramebufferStatusEXT() again.

But you should be able to change it so that we don't need the if-conditions and
the tests works the same way - with the cache - on all platforms.

Can you double check that the test fails if we don't enable the cache and with
the extra GetStatus calls? It probably comes down to making sure we created the
right StrictMock for gl_.

[Tobias Sargeant, 2015-08-19 11:33:12]

tobiasjs@chromium.org changed reviewers:
+ boliu@chromium.org, sky@chromium.org

[Tobias Sargeant, 2015-08-19 11:33:13]

+boliu for android_webview/
+sky for components/view_manager/gles2/

Daniel: should I add a test that - using the default construction path for
FramebufferManager - the provided cache is ignored on OSX?

https://codereview.chromium.org/1278333003/diff/120001/gpu/command_buffer/ser...
File gpu/command_buffer/service/framebuffer_completeness_cache.cc (right):

https://codereview.chromium.org/1278333003/diff/120001/gpu/command_buffer/ser...
gpu/command_buffer/service/framebuffer_completeness_cache.cc:1: // Copyright (c)
2015 The Chromium Authors. All rights reserved.
On 2015/08/19 00:43:13, sievers wrote:
> nit: drop '(c)'

Done.

https://codereview.chromium.org/1278333003/diff/120001/gpu/command_buffer/ser...
gpu/command_buffer/service/framebuffer_completeness_cache.cc:5: #include
"gpu/command_buffer/service/framebuffer_completeness_cache.h"
On 2015/08/19 00:43:13, sievers wrote:
> nit: newline after #include

Done.

https://codereview.chromium.org/1278333003/diff/120001/gpu/command_buffer/ser...
File gpu/command_buffer/service/framebuffer_completeness_cache.h (right):

https://codereview.chromium.org/1278333003/diff/120001/gpu/command_buffer/ser...
gpu/command_buffer/service/framebuffer_completeness_cache.h:1: // Copyright (c)
2015 The Chromium Authors. All rights reserved.
On 2015/08/19 00:43:13, sievers wrote:
> nit: drop '(c)' in new headers

Done.

https://codereview.chromium.org/1278333003/diff/120001/gpu/command_buffer/ser...
gpu/command_buffer/service/framebuffer_completeness_cache.h:6: #define
GPU_COMMAND_BUFFER_COMMON_FRAMEBUFFER_COMPLETENESS_CACHE_H_
On 2015/08/19 00:43:13, sievers wrote:
> nit: s/BUFFER_COMMON/BUFFER_SERVICE

Done.

https://codereview.chromium.org/1278333003/diff/120001/gpu/command_buffer/ser...
File gpu/command_buffer/service/framebuffer_manager.cc (right):

https://codereview.chromium.org/1278333003/diff/120001/gpu/command_buffer/ser...
gpu/command_buffer/service/framebuffer_manager.cc:496: #if defined(OS_MACOSX)
On 2015/08/19 00:43:13, sievers wrote:
> Doesn't this break unittesting on Mac?
> You could move the #ifdef to the construction call site for production code
> instead.

It would've, if not for the removal of the unittests that asserted the caching
behaviour. As caching wasn't required for correctness (in fact not caching is
required for correctness on OSX - we don't have a test for that) I felt like it
was right to remove the tests.

I've put them back now, and rejigged this code.

https://codereview.chromium.org/1278333003/diff/120001/gpu/command_buffer/ser...
File gpu/command_buffer/service/framebuffer_manager_unittest.cc (left):

https://codereview.chromium.org/1278333003/diff/120001/gpu/command_buffer/ser...
gpu/command_buffer/service/framebuffer_manager_unittest.cc:843:
framebuffer_->GetStatus(texture_manager_.get(), GL_FRAMEBUFFER);
On 2015/08/19 00:43:13, sievers wrote:
> I think all of these GetStatus() calls are important, because they test if the
> cache actually works and we end up *not* calling
> gl_->CheckFramebufferStatusEXT() again.
> 
> But you should be able to change it so that we don't need the if-conditions
and
> the tests works the same way - with the cache - on all platforms.
> 
> Can you double check that the test fails if we don't enable the cache and with
> the extra GetStatus calls? It probably comes down to making sure we created
the
> right StrictMock for gl_.

Done, and tested that it fails if the cache is unset.

[boliu, 2015-08-19 17:36:19]

android_webview lgtm

kinda big, guess won't be merged back to m45 then..

https://codereview.chromium.org/1278333003/diff/140001/android_webview/browse...
File android_webview/browser/deferred_gpu_command_service.cc (right):

https://codereview.chromium.org/1278333003/diff/140001/android_webview/browse...
android_webview/browser/deferred_gpu_command_service.cc:159: if
(!framebuffer_completeness_cache_.get())
style: need braces for if with multi-line body

[Tobias Sargeant, 2015-08-19 17:39:38]

We could merge one of the earlier patchsets, which were a lot smaller...
On 19 Aug 2015 6:36 p.m., <boliu@chromium.org> wrote:

> android_webview lgtm
>
> kinda big, guess won't be merged back to m45 then..
>
>
>
>
https://codereview.chromium.org/1278333003/diff/140001/android_webview/browse...
> File android_webview/browser/deferred_gpu_command_service.cc (right):
>
>
>
https://codereview.chromium.org/1278333003/diff/140001/android_webview/browse...
> android_webview/browser/deferred_gpu_command_service.cc:159: if
> (!framebuffer_completeness_cache_.get())
> style: need braces for if with multi-line body
>
> https://codereview.chromium.org/1278333003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[no sievers, 2015-08-19 23:39:47]

https://codereview.chromium.org/1278333003/diff/140001/gpu/command_buffer/ser...
File gpu/command_buffer/service/framebuffer_completeness_cache.h (right):

https://codereview.chromium.org/1278333003/diff/140001/gpu/command_buffer/ser...
gpu/command_buffer/service/framebuffer_completeness_cache.h:41: #endif  //
GPU_COMMAND_BUFFER_COMMON_FRAMEBUFFER_COMPLETENESS_CACHE_H_
nit: _COMMON_ -> _SERVICE_

https://codereview.chromium.org/1278333003/diff/140001/gpu/command_buffer/ser...
File gpu/command_buffer/service/framebuffer_manager.cc (right):

https://codereview.chromium.org/1278333003/diff/140001/gpu/command_buffer/ser...
gpu/command_buffer/service/framebuffer_manager.cc:262:
framebuffer_combo_complete_cache_ = NULL;
Isn't it easier to pass in NULL on Mac from GpuCommandBufferStub? (I don't think
we have to worry about GPU switching from other call sites.) THen you also don't
need the setter method for testing.

[no sievers, 2015-08-19 23:46:54]

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

[no sievers, 2015-08-19 23:47:00]

https://codereview.chromium.org/1278333003/diff/140001/gpu/command_buffer/ser...
File gpu/command_buffer/service/framebuffer_manager.cc (right):

https://codereview.chromium.org/1278333003/diff/140001/gpu/command_buffer/ser...
gpu/command_buffer/service/framebuffer_manager.cc:262:
framebuffer_combo_complete_cache_ = NULL;
On 2015/08/19 23:39:47, sievers wrote:
> Isn't it easier to pass in NULL on Mac from GpuCommandBufferStub? (I don't
think
> we have to worry about GPU switching from other call sites.) THen you also
don't
> need the setter method for testing.

And you can add a TODO, because I think we can do better and figure out from the
call site if the GPU is actually switchable. I'm not sure if we are even also
missing Optimus switching here. +kbr@

[Ken Russell (switch to Gerrit), 2015-08-20 00:04:10]

Generally looks fine if this has been tested, but a couple of minor points.

https://codereview.chromium.org/1278333003/diff/140001/android_webview/browse...
File android_webview/browser/deferred_gpu_command_service.h (right):

https://codereview.chromium.org/1278333003/diff/140001/android_webview/browse...
android_webview/browser/deferred_gpu_command_service.h:50:
framebuffer_completeness_cache() override;
The second line should be intended here. There are several other places in the
code which need a similar update.

https://codereview.chromium.org/1278333003/diff/140001/gpu/command_buffer/ser...
File gpu/command_buffer/service/framebuffer_completeness_cache.h (right):

https://codereview.chromium.org/1278333003/diff/140001/gpu/command_buffer/ser...
gpu/command_buffer/service/framebuffer_completeness_cache.h:16: // representing
[XXX]
Incomplete comment?

https://codereview.chromium.org/1278333003/diff/140001/gpu/command_buffer/ser...
File gpu/command_buffer/service/framebuffer_manager.cc (right):

https://codereview.chromium.org/1278333003/diff/140001/gpu/command_buffer/ser...
gpu/command_buffer/service/framebuffer_manager.cc:262:
framebuffer_combo_complete_cache_ = NULL;
On 2015/08/19 23:47:00, sievers wrote:
> On 2015/08/19 23:39:47, sievers wrote:
> > Isn't it easier to pass in NULL on Mac from GpuCommandBufferStub? (I don't
> think
> > we have to worry about GPU switching from other call sites.) THen you also
> don't
> > need the setter method for testing.
> 
> And you can add a TODO, because I think we can do better and figure out from
the
> call site if the GPU is actually switchable. I'm not sure if we are even also
> missing Optimus switching here. +kbr@

To the best of my knowledge, Optimus switching doesn't migrate the process from
one GPU to the other -- only dynamically decides at program startup which GPU it
will run on. Therefore I think this general approach is still OK.

https://codereview.chromium.org/1278333003/diff/140001/gpu/command_buffer/ser...
File gpu/command_buffer/service/framebuffer_manager_unittest.cc (left):

https://codereview.chromium.org/1278333003/diff/140001/gpu/command_buffer/ser...
gpu/command_buffer/service/framebuffer_manager_unittest.cc:839:
EXPECT_CALL(*gl_, CheckFramebufferStatusEXT(GL_FRAMEBUFFER))
Why are these CheckFramebufferStatusEXT expectations removed? It looks like it's
still being called from Framebuffer::GetStatus.

[no sievers, 2015-08-20 00:11:29]

https://codereview.chromium.org/1278333003/diff/140001/gpu/command_buffer/ser...
File gpu/command_buffer/service/framebuffer_manager_unittest.cc (left):

https://codereview.chromium.org/1278333003/diff/140001/gpu/command_buffer/ser...
gpu/command_buffer/service/framebuffer_manager_unittest.cc:839:
EXPECT_CALL(*gl_, CheckFramebufferStatusEXT(GL_FRAMEBUFFER))
On 2015/08/20 00:04:10, Ken Russell wrote:
> Why are these CheckFramebufferStatusEXT expectations removed? It looks like
it's
> still being called from Framebuffer::GetStatus.

Actually if you are setting the expectation than you are testing that the cache
*does* not work and calls into the driver.

It was done because of the Mac ifdef. However, we don't need to do this for the
tests - they can work with the cache on Mac - and we can just disable the cache
for Mac in real production builds only.

[Tobias Sargeant, 2015-08-20 11:05:14]

https://codereview.chromium.org/1278333003/diff/140001/android_webview/browse...
File android_webview/browser/deferred_gpu_command_service.cc (right):

https://codereview.chromium.org/1278333003/diff/140001/android_webview/browse...
android_webview/browser/deferred_gpu_command_service.cc:159: if
(!framebuffer_completeness_cache_.get())
On 2015/08/19 17:36:19, boliu wrote:
> style: need braces for if with multi-line body 

Will fix. It was a single line until git cl format. :-/

https://codereview.chromium.org/1278333003/diff/140001/android_webview/browse...
File android_webview/browser/deferred_gpu_command_service.h (right):

https://codereview.chromium.org/1278333003/diff/140001/android_webview/browse...
android_webview/browser/deferred_gpu_command_service.h:50:
framebuffer_completeness_cache() override;
On 2015/08/20 00:04:10, Ken Russell wrote:
> The second line should be intended here. There are several other places in the
> code which need a similar update.

This is clang-format'ed, and it's also consistent with how method definitions
with long return types get formatted.

https://codereview.chromium.org/1278333003/diff/140001/gpu/command_buffer/ser...
File gpu/command_buffer/service/framebuffer_completeness_cache.h (right):

https://codereview.chromium.org/1278333003/diff/140001/gpu/command_buffer/ser...
gpu/command_buffer/service/framebuffer_completeness_cache.h:16: // representing
[XXX]
On 2015/08/20 00:04:10, Ken Russell wrote:
> Incomplete comment?

Yep. Oops.

https://codereview.chromium.org/1278333003/diff/140001/gpu/command_buffer/ser...
gpu/command_buffer/service/framebuffer_completeness_cache.h:41: #endif  //
GPU_COMMAND_BUFFER_COMMON_FRAMEBUFFER_COMPLETENESS_CACHE_H_
On 2015/08/19 23:39:47, sievers wrote:
> nit: _COMMON_ -> _SERVICE_

Done.

https://codereview.chromium.org/1278333003/diff/140001/gpu/command_buffer/ser...
File gpu/command_buffer/service/framebuffer_manager.cc (right):

https://codereview.chromium.org/1278333003/diff/140001/gpu/command_buffer/ser...
gpu/command_buffer/service/framebuffer_manager.cc:262:
framebuffer_combo_complete_cache_ = NULL;
On 2015/08/19 23:39:47, sievers wrote:
> Isn't it easier to pass in NULL on Mac from GpuCommandBufferStub? (I don't
think
> we have to worry about GPU switching from other call sites.) THen you also
don't
> need the setter method for testing.

Moving it to the ContextGroup constructor instead. That should give the same
code cleanup effect, but also intercepts other call paths in
components/view_manager/gles2/command_buffer_driver.cc and
components/view_manager/gles2/command_buffer_local.cc. I'm unsure whether these
are used on OSX.

Also, the latter of those two files was missed initially, so it looks like no
trybots compile it.

https://codereview.chromium.org/1278333003/diff/140001/gpu/command_buffer/ser...
gpu/command_buffer/service/framebuffer_manager.cc:262:
framebuffer_combo_complete_cache_ = NULL;
On 2015/08/19 23:47:00, sievers wrote:
> On 2015/08/19 23:39:47, sievers wrote:
> > Isn't it easier to pass in NULL on Mac from GpuCommandBufferStub? (I don't
> think
> > we have to worry about GPU switching from other call sites.) THen you also
> don't
> > need the setter method for testing.
> 
> And you can add a TODO, because I think we can do better and figure out from
the
> call site if the GPU is actually switchable. I'm not sure if we are even also
> missing Optimus switching here. +kbr@

Done.

https://codereview.chromium.org/1278333003/diff/140001/gpu/command_buffer/ser...
File gpu/command_buffer/service/framebuffer_manager_unittest.cc (left):

https://codereview.chromium.org/1278333003/diff/140001/gpu/command_buffer/ser...
gpu/command_buffer/service/framebuffer_manager_unittest.cc:839:
EXPECT_CALL(*gl_, CheckFramebufferStatusEXT(GL_FRAMEBUFFER))
On 2015/08/20 00:11:29, sievers wrote:
> On 2015/08/20 00:04:10, Ken Russell wrote:
> > Why are these CheckFramebufferStatusEXT expectations removed? It looks like
> it's
> > still being called from Framebuffer::GetStatus.
> 
> Actually if you are setting the expectation than you are testing that the
cache
> *does* not work and calls into the driver.
> 
> It was done because of the Mac ifdef. However, we don't need to do this for
the
> tests - they can work with the cache on Mac - and we can just disable the
cache
> for Mac in real production builds only.

Acknowledged.

https://codereview.chromium.org/1278333003/diff/140001/gpu/command_buffer/ser...
gpu/command_buffer/service/framebuffer_manager_unittest.cc:839:
EXPECT_CALL(*gl_, CheckFramebufferStatusEXT(GL_FRAMEBUFFER))
On 2015/08/20 00:04:10, Ken Russell wrote:
> Why are these CheckFramebufferStatusEXT expectations removed? It looks like
it's
> still being called from Framebuffer::GetStatus.

Acknowledged.

[no sievers, 2015-08-20 21:05:31]

lgtm

[boliu, 2015-08-21 23:51:07]

The CQ bit was checked by boliu@chromium.org

[boliu, 2015-08-21 23:51:07]

The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org
Link to the patchset: https://codereview.chromium.org/1278333003/#ps160001
(title: "Address comments.")

[boliu, 2015-08-21 23:51:32]

Think this is ready to land, right?

[commit-bot: I haz the power, 2015-08-21 23:52:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1278333003/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1278333003/160001

[commit-bot: I haz the power, 2015-08-22 00:04:02]

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

[commit-bot: I haz the power, 2015-08-22 00:04:03]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[boliu, 2015-08-22 00:06:10]

boliu@chromium.org changed reviewers:
+ msw@chromium.org

[boliu, 2015-08-22 00:06:10]

+msw for components/view_manager

[msw, 2015-08-22 00:20:40]

On 2015/08/22 00:06:10, boliu wrote:
> +msw for components/view_manager

components/view_manager rubber stamp lgtm

[boliu, 2015-08-22 00:22:51]

The CQ bit was checked by boliu@chromium.org

[commit-bot: I haz the power, 2015-08-22 00:23:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1278333003/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1278333003/160001

[commit-bot: I haz the power, 2015-08-22 00:39:13]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-08-22 00:40:18]

Patchset 9 (id:??) landed as
https://crrev.com/fc199b47cb5b74c9f2ccf3a6316fc9b7009c6904
Cr-Commit-Position: refs/heads/master@{#344940}