Reland Window Control Animations for TouchView

Reland Window Control Animations for TouchView

The original change contained a use-after-free bug. This was caused by window crossfade animation during maximizing, combined with input event processing. The crossfade would take the current layer and delete it. However the next input event to the window tree would attempt to access the now deleted layer.

Original Code Review: https://codereview.chromium.org/271913002/
Revert Review: https://codereview.chromium.org/309973002/

Revert "Revert of Animate window control changes in TouchView (https://codereview.chromium.org/271913002/)"
This reverts commit 8863218a5e97c4914a5a03df59e9d5d17f53a2cb.

TBR=jamescook@chromium.org
TEST=FrameCaptionButtonContainerViewTest.AnimationUpdatesLayerTree
BUG=363717
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=276782

[jonross, 2014-06-04 21:07:40]

This is a fix for the memory leak found by ASan.

I ran both interactive_ui_tests and browser_tests that had showed the leak. They
no longer report the leak.

The leak was reproduced just using chrome. The leak no longer reproduces there.

[flackr, 2014-06-04 22:41:43]

https://codereview.chromium.org/316693002/diff/40001/ash/frame/caption_button...
File ash/frame/caption_buttons/frame_caption_button_container_view.cc (right):

https://codereview.chromium.org/316693002/diff/40001/ash/frame/caption_button...
ash/frame/caption_buttons/frame_caption_button_container_view.cc:423:
close_button_->GetWidget()->UpdateRootLayers();
I think it'd make more sense for FrameCaptionButton to be a LayerOwnerDelegate
and set themselves up to paint to layers. Then you wouldn't need to check which
layer this was for here.

https://codereview.chromium.org/316693002/diff/40001/ash/frame/caption_button...
File ash/frame/caption_buttons/frame_caption_button_container_view.h (right):

https://codereview.chromium.org/316693002/diff/40001/ash/frame/caption_button...
ash/frame/caption_buttons/frame_caption_button_container_view.h:18: class Layer;
If this is only needed for the override, you can rely on having Layer defined by
including layer_owner_delegate.h.

https://codereview.chromium.org/316693002/diff/40001/ash/frame/caption_button...
File ash/frame/caption_buttons/frame_caption_button_container_view_unittest.cc
(right):

https://codereview.chromium.org/316693002/diff/40001/ash/frame/caption_button...
ash/frame/caption_buttons/frame_caption_button_container_view_unittest.cc:208:
TEST_F(FrameCaptionButtonContainerViewTest, MaximizeAnimationMemoryLeak) {
nit: s/MaximizeAnimationMemoryLeak/AnimationUpdatesLayerTree

https://codereview.chromium.org/316693002/diff/40001/ash/frame/caption_button...
ash/frame/caption_buttons/frame_caption_button_container_view_unittest.cc:228:
EXPECT_TRUE(widget_can_maximize->IsMaximized());
After this, using original_layer has been destroyed right? It would be worth
documenting that it is now an invalid pointer and that dereferencing it could
cause a segfault.

[jonross, 2014-06-05 15:16:39]

https://codereview.chromium.org/316693002/diff/40001/ash/frame/caption_button...
File ash/frame/caption_buttons/frame_caption_button_container_view.cc (right):

https://codereview.chromium.org/316693002/diff/40001/ash/frame/caption_button...
ash/frame/caption_buttons/frame_caption_button_container_view.cc:423:
close_button_->GetWidget()->UpdateRootLayers();
On 2014/06/04 22:41:44, flackr wrote:
> I think it'd make more sense for FrameCaptionButton to be a LayerOwnerDelegate
> and set themselves up to paint to layers. Then you wouldn't need to check
which
> layer this was for here.

Done.

https://codereview.chromium.org/316693002/diff/40001/ash/frame/caption_button...
File ash/frame/caption_buttons/frame_caption_button_container_view.h (right):

https://codereview.chromium.org/316693002/diff/40001/ash/frame/caption_button...
ash/frame/caption_buttons/frame_caption_button_container_view.h:18: class Layer;
On 2014/06/04 22:41:44, flackr wrote:
> If this is only needed for the override, you can rely on having Layer defined
by
> including layer_owner_delegate.h.

Done.

https://codereview.chromium.org/316693002/diff/40001/ash/frame/caption_button...
File ash/frame/caption_buttons/frame_caption_button_container_view_unittest.cc
(right):

https://codereview.chromium.org/316693002/diff/40001/ash/frame/caption_button...
ash/frame/caption_buttons/frame_caption_button_container_view_unittest.cc:208:
TEST_F(FrameCaptionButtonContainerViewTest, MaximizeAnimationMemoryLeak) {
On 2014/06/04 22:41:44, flackr wrote:
> nit: s/MaximizeAnimationMemoryLeak/AnimationUpdatesLayerTree

Done.

https://codereview.chromium.org/316693002/diff/40001/ash/frame/caption_button...
ash/frame/caption_buttons/frame_caption_button_container_view_unittest.cc:228:
EXPECT_TRUE(widget_can_maximize->IsMaximized());
On 2014/06/04 22:41:44, flackr wrote:
> After this, using original_layer has been destroyed right? It would be worth
> documenting that it is now an invalid pointer and that dereferencing it could
> cause a segfault.

Done.

[flackr, 2014-06-05 15:24:38]

nit: Update your TEST= line to name the exact tests which verify the CL

Otherwise LGTM

[jonross, 2014-06-05 15:33:18]

Hi Oshima,

This is a reland of the window control animations. The first patch is the
original work which you have reviewed.

There are changes to the files in ash/frame/caption_buttons/ which address a
memory leak.

James, there have been no changes to the files which you have reviewed
previously. Those being the files under chrome/browser/ui/views/frame/

[James Cook, 2014-06-05 16:49:13]

c/b/ui/views/frame still LGTM

[oshima, 2014-06-06 17:01:24]

+ sky@ for question about RecreateLayer behavior in views.

https://codereview.chromium.org/316693002/diff/60001/ash/frame/caption_button...
File ash/frame/caption_buttons/frame_caption_button.cc (right):

https://codereview.chromium.org/316693002/diff/60001/ash/frame/caption_button...
ash/frame/caption_buttons/frame_caption_button.cc:186: }
I wonder if this should be the default behavior in views::View. sky@, what is
your opinion?

[sky, 2014-06-06 19:45:15]

https://codereview.chromium.org/316693002/diff/60001/ash/frame/caption_button...
File ash/frame/caption_buttons/frame_caption_button.cc (right):

https://codereview.chromium.org/316693002/diff/60001/ash/frame/caption_button...
ash/frame/caption_buttons/frame_caption_button.cc:186: }
On 2014/06/06 17:01:24, oshima wrote:
> I wonder if this should be the default behavior in views::View. sky@, what is
> your opinion?

I agree. Please separate the Views change into its own patch and add test
coverage.

[oshima, 2014-06-06 20:56:51]

by the way, can you update the issue description? I believe it was
use-after-free, not a memory leak.

[jonross, 2014-06-11 16:40:50]

On 2014/06/06 20:56:51, oshima wrote:
> by the way, can you update the issue description? I believe it was
> use-after-free, not a memory leak.

sky@: I will work on updating View in its own patch.
oshima@: Description updated. Would you be able to review the changes made under
ash/frame ?

[oshima, 2014-06-11 18:34:56]

On 2014/06/11 16:40:50, jonross wrote:
> On 2014/06/06 20:56:51, oshima wrote:
> > by the way, can you update the issue description? I believe it was
> > use-after-free, not a memory leak.
> 
> sky@: I will work on updating View in its own patch.
> oshima@: Description updated. Would you be able to review the changes made
under
> ash/frame ?

You still have layer owner code in frame. Do you want to land this first and
then remove it later?

[jonross, 2014-06-11 20:36:51]

On 2014/06/11 18:34:56, oshima wrote:
> On 2014/06/11 16:40:50, jonross wrote:
> > On 2014/06/06 20:56:51, oshima wrote:
> > > by the way, can you update the issue description? I believe it was
> > > use-after-free, not a memory leak.
> > 
> > sky@: I will work on updating View in its own patch.
> > oshima@: Description updated. Would you be able to review the changes made
> under
> > ash/frame ?
> 
> You still have layer owner code in frame. Do you want to land this first and
> then remove it later?

Yes, I'd rather get this change landed before making a change to View itself.
After that lands I am fine going back and removing this.

[oshima, 2014-06-11 20:58:57]

On 2014/06/11 20:36:51, jonross wrote:
> On 2014/06/11 18:34:56, oshima wrote:
> > On 2014/06/11 16:40:50, jonross wrote:
> > > On 2014/06/06 20:56:51, oshima wrote:
> > > > by the way, can you update the issue description? I believe it was
> > > > use-after-free, not a memory leak.
> > > 
> > > sky@: I will work on updating View in its own patch.
> > > oshima@: Description updated. Would you be able to review the changes made
> > under
> > > ash/frame ?
> > 
> > You still have layer owner code in frame. Do you want to land this first and
> > then remove it later?
> 
> Yes, I'd rather get this change landed before making a change to View itself.
> After that lands I am fine going back and removing this.

Please make sure to remove them as it'll be unnecessary. lgtm

[jonross, 2014-06-11 21:03:44]

The CQ bit was checked by jonross@chromium.org

[commit-bot: I haz the power, 2014-06-11 21:04:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jonross@chromium.org/316693002/60001

[commit-bot: I haz the power, 2014-06-11 21:31:02]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/8...)
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/19...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/15642)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/18779)

[jonross, 2014-06-11 21:32:10]

The CQ bit was unchecked by jonross@chromium.org

[commit-bot: I haz the power, 2014-06-11 21:34:05]

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

[commit-bot: I haz the power, 2014-06-11 21:34:05]

Try jobs failed on following builders:
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/15666)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/18800)

[jonross, 2014-06-12 15:14:51]

The CQ bit was checked by jonross@chromium.org

[commit-bot: I haz the power, 2014-06-12 15:18:01]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jonross@chromium.org/316693002/80001

[commit-bot: I haz the power, 2014-06-12 20:06:06]

Change committed as 276782