mac: Remove uses of a CoreGraphics private API that causes WindowServer crashes.

mac: Implement a simple animation for CustomConstrainedWindowSheet.

The original animation uses CoreGraphics private APIs. This causes WindowServer
crashes when used by the extension install dialog. This CL adds a simple
animation for the extension install dialog to use.

The code for performing the simple animation came from tapted's CL:
https://codereview.chromium.org/1794023004

BUG=515627, 548824
Committed: https://crrev.com/480c8640a6aaed2cde12711c2985d8c6a4221060
Cr-Commit-Position: refs/heads/master@{#381162}

[erikchen, 2016-03-11 20:52:45]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[erikchen, 2016-03-11 20:53:16]

erikchen@chromium.org changed reviewers:
+ tapted@chromium.org

[erikchen, 2016-03-11 20:53:17]

tapted: Please review.

[commit-bot: I haz the power, 2016-03-11 20:53:29]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1787553003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1787553003/1

[commit-bot: I haz the power, 2016-03-11 21:05:08]

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

[commit-bot: I haz the power, 2016-03-11 21:05:08]

Dry run: Try jobs failed on following builders:
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[erikchen, 2016-03-11 21:13:32]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-11 21:14:08]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1787553003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1787553003/20001

[commit-bot: I haz the power, 2016-03-11 22:05:39]

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

[commit-bot: I haz the power, 2016-03-11 22:05:40]

Dry run: This issue passed the CQ dry run.

[tapted, 2016-03-14 00:03:22]

It's possible this fixes http://crbug.com/436884 as well.

But, this will also impact a lot of dialogs - not just extension install. But
also print preview, http auth, confirm form resubmission, etc. Do those dialogs
evoke a crash on the VM? Should we consider plumbing through an argument from
ExtensionInstallDialogController to CustomConstrainedWindowSheet, so that only
Extension install dialog is affected?

Also the show/hide animation is basically now just a fade in/out. There are much
more "normal" ways to do that. E.g. something like
https://codereview.chromium.org/1794023004

https://codereview.chromium.org/1787553003/diff/20001/ui/base/cocoa/constrain...
File ui/base/cocoa/constrained_window/constrained_window_animation.mm (right):

https://codereview.chromium.org/1787553003/diff/20001/ui/base/cocoa/constrain...
ui/base/cocoa/constrained_window/constrained_window_animation.mm:48: CGError
CGSSetWindowTransform(const CGSConnection cid,
You said CGSSetWindowWarp caused the crashes, but the logs had
 > ERROR: CGSSetWindowTransformAtPlacement() returned 268435459

Is that weird/something to worry about?

(Did you try out the `pulse` on the VM with the extension install dialog open,
by mashing clicks on the webcontents in the background?)

[erikchen, 2016-03-14 20:56:40]

On 2016/03/14 00:03:22, tapted wrote:
> It's possible this fixes http://crbug.com/436884 as well.
> 
> But, this will also impact a lot of dialogs - not just extension install. But
> also print preview, http auth, confirm form resubmission, etc. Do those
dialogs
> evoke a crash on the VM? Should we consider plumbing through an argument from
> ExtensionInstallDialogController to CustomConstrainedWindowSheet, so that only
> Extension install dialog is affected?

I tried the print preview dialog, and it didn't crash the vm window server. Not
sure what's different about extension install window.

> 
> Also the show/hide animation is basically now just a fade in/out. There are
much
> more "normal" ways to do that. E.g. something like
> https://codereview.chromium.org/1794023004
> 
>
https://codereview.chromium.org/1787553003/diff/20001/ui/base/cocoa/constrain...
> File ui/base/cocoa/constrained_window/constrained_window_animation.mm (right):
> 
>
https://codereview.chromium.org/1787553003/diff/20001/ui/base/cocoa/constrain...
> ui/base/cocoa/constrained_window/constrained_window_animation.mm:48: CGError
> CGSSetWindowTransform(const CGSConnection cid,
> You said CGSSetWindowWarp caused the crashes, but the logs had
>  > ERROR: CGSSetWindowTransformAtPlacement() returned 268435459
> 
> Is that weird/something to worry about?
> 
> (Did you try out the `pulse` on the VM with the extension install dialog open,
> by mashing clicks on the webcontents in the background?)

I tried this out, and this didn't cause a VM window server crash.


What's the plan here? We should either land this CL, or a similar one to fix the
existing crash. Note that the CL you linked has test failures. I'm inclined to
remove all uses of private CGS APIs, but it sounds like you would prefer to keep
as many of them in as possible? The problem is that we don't fundamentally know
"why" these functions are causing crashes, and we have a non-deterministic,
no-repro wild case of this, which worries me a lot.

[tapted, 2016-03-14 22:56:33]

On 2016/03/14 20:56:40, erikchen wrote:
> On 2016/03/14 00:03:22, tapted wrote:
> > It's possible this fixes http://crbug.com/436884 as well.
> > 
> > But, this will also impact a lot of dialogs - not just extension install.
But
> > also print preview, http auth, confirm form resubmission, etc. Do those
> dialogs
> > evoke a crash on the VM? Should we consider plumbing through an argument
from
> > ExtensionInstallDialogController to CustomConstrainedWindowSheet, so that
only
> > Extension install dialog is affected?
> 
> I tried the print preview dialog, and it didn't crash the vm window server.
Not
> sure what's different about extension install window.

I put a theory up on the bug -- the extension install dialog has an NSScrollView
which does interesting things with Layers since linking to the 10.10SDK. Linking
to 10.10 actually caused a regression in the show animation for extension
install dialogs. The fix for that -- https://codereview.chromium.org/1362983005
-- might also be contributing to these crashes (it's possible reverting that
could make the window server crashes go away).

> 
> > 
> > Also the show/hide animation is basically now just a fade in/out. There are
> much
> > more "normal" ways to do that. E.g. something like
> > https://codereview.chromium.org/1794023004
> > 
> >
>
https://codereview.chromium.org/1787553003/diff/20001/ui/base/cocoa/constrain...
> > File ui/base/cocoa/constrained_window/constrained_window_animation.mm
(right):
> > 
> >
>
https://codereview.chromium.org/1787553003/diff/20001/ui/base/cocoa/constrain...
> > ui/base/cocoa/constrained_window/constrained_window_animation.mm:48: CGError
> > CGSSetWindowTransform(const CGSConnection cid,
> > You said CGSSetWindowWarp caused the crashes, but the logs had
> >  > ERROR: CGSSetWindowTransformAtPlacement() returned 268435459
> > 
> > Is that weird/something to worry about?
> > 
> > (Did you try out the `pulse` on the VM with the extension install dialog
open,
> > by mashing clicks on the webcontents in the background?)
> 
> I tried this out, and this didn't cause a VM window server crash.
> 
> 
> What's the plan here? We should either land this CL, or a similar one to fix
the
> existing crash. Note that the CL you linked has test failures. I'm inclined to
> remove all uses of private CGS APIs, but it sounds like you would prefer to
keep
> as many of them in as possible? The problem is that we don't fundamentally
know
> "why" these functions are causing crashes, and we have a non-deterministic,
> no-repro wild case of this, which worries me a lot.


I agree we should try to avoid these private APIs, but I think we need a plan to
restore the UX. Fade in/out for these hasn't gone through ui review, and I think
it feels kinda weird. These window server deaths are affecting a relatively
small number of users, so removing the animation entirely feels like an
unnecessary regression.

So, my preference would be to just avoid the animation for extension install
dialogs and monitor the situation. If there's a whiff of more crashes, then we
can escalate quickly. I think we should add checking for the return values of
CGSSetWindowWarp/CGSSetWindowAlpha/etc. -- perhaps with a CHECK so we can
(maybe?) get a crash dump before the window server dies -- that could give us
greater confidence that these are "safe".

Then, using toolkit-views for the extension install dialog is probably only a
milestone or two away. These tab-modal dialogs in toolkit-views currently also
use these CGS APIs, but without an NSScrollView: they look a lot more like print
preview does in that regard. But I think there's also a simple change that could
just animate the ui::Layer holding the install dialog (after adding an
appropriate shadow) instead of asking the window server to morph things.


WDYT of that approach? There is greater risk, but I think disabling just for
extension install dialogs could give us more insight into what might be causing
these crashes, as well as prevent a more widespread regression in the UX. Then
we can work on phasing out the private APIs without UX regressions.

[tapted, 2016-03-14 23:02:54]

On 2016/03/14 20:56:40, erikchen wrote:
> Note that the CL you linked has test failures.

I think these are all the same check:

  EXPECT_EQ(1.0, [sheet_window_ alphaValue]);

the CL moves the show animation from a main-thread-blocking (i.e. while (now() <
t) { update(); draw(); sleep(); }) to an asynchronous CAAnimation to fade-in the
window, so `alphaValue` will update asynchronously. Blocking animations are
kinda gross. The show animation doesn't really need to be blocking, but the hide
animation has lifetime issues if its not.

[erikchen, 2016-03-14 23:37:20]

On 2016/03/14 22:56:33, tapted wrote:
> On 2016/03/14 20:56:40, erikchen wrote:
> > On 2016/03/14 00:03:22, tapted wrote:
> > > It's possible this fixes http://crbug.com/436884 as well.
> > > 
> > > But, this will also impact a lot of dialogs - not just extension install.
> But
> > > also print preview, http auth, confirm form resubmission, etc. Do those
> > dialogs
> > > evoke a crash on the VM? Should we consider plumbing through an argument
> from
> > > ExtensionInstallDialogController to CustomConstrainedWindowSheet, so that
> only
> > > Extension install dialog is affected?
> > 
> > I tried the print preview dialog, and it didn't crash the vm window server.
> Not
> > sure what's different about extension install window.
> 
> I put a theory up on the bug -- the extension install dialog has an
NSScrollView
> which does interesting things with Layers since linking to the 10.10SDK.
Linking
> to 10.10 actually caused a regression in the show animation for extension
> install dialogs. The fix for that --
https://codereview.chromium.org/1362983005
> -- might also be contributing to these crashes (it's possible reverting that
> could make the window server crashes go away).
> 
> > 
> > > 
> > > Also the show/hide animation is basically now just a fade in/out. There
are
> > much
> > > more "normal" ways to do that. E.g. something like
> > > https://codereview.chromium.org/1794023004
> > > 
> > >
> >
>
https://codereview.chromium.org/1787553003/diff/20001/ui/base/cocoa/constrain...
> > > File ui/base/cocoa/constrained_window/constrained_window_animation.mm
> (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1787553003/diff/20001/ui/base/cocoa/constrain...
> > > ui/base/cocoa/constrained_window/constrained_window_animation.mm:48:
CGError
> > > CGSSetWindowTransform(const CGSConnection cid,
> > > You said CGSSetWindowWarp caused the crashes, but the logs had
> > >  > ERROR: CGSSetWindowTransformAtPlacement() returned 268435459
> > > 
> > > Is that weird/something to worry about?
> > > 
> > > (Did you try out the `pulse` on the VM with the extension install dialog
> open,
> > > by mashing clicks on the webcontents in the background?)
> > 
> > I tried this out, and this didn't cause a VM window server crash.
> > 
> > 
> > What's the plan here? We should either land this CL, or a similar one to fix
> the
> > existing crash. Note that the CL you linked has test failures. I'm inclined
to
> > remove all uses of private CGS APIs, but it sounds like you would prefer to
> keep
> > as many of them in as possible? The problem is that we don't fundamentally
> know
> > "why" these functions are causing crashes, and we have a non-deterministic,
> > no-repro wild case of this, which worries me a lot.
> 
> 
> I agree we should try to avoid these private APIs, but I think we need a plan
to
> restore the UX. Fade in/out for these hasn't gone through ui review, and I
think
> it feels kinda weird. These window server deaths are affecting a relatively
> small number of users, so removing the animation entirely feels like an
> unnecessary regression.
> 
> So, my preference would be to just avoid the animation for extension install
> dialogs and monitor the situation. If there's a whiff of more crashes, then we
> can escalate quickly. I think we should add checking for the return values of
> CGSSetWindowWarp/CGSSetWindowAlpha/etc. -- perhaps with a CHECK so we can
> (maybe?) get a crash dump before the window server dies -- that could give us
> greater confidence that these are "safe".
> 
> Then, using toolkit-views for the extension install dialog is probably only a
> milestone or two away. These tab-modal dialogs in toolkit-views currently also
> use these CGS APIs, but without an NSScrollView: they look a lot more like
print
> preview does in that regard. But I think there's also a simple change that
could
> just animate the ui::Layer holding the install dialog (after adding an
> appropriate shadow) instead of asking the window server to morph things.
> 
> 
> WDYT of that approach? There is greater risk, but I think disabling just for
> extension install dialogs could give us more insight into what might be
causing
> these crashes, as well as prevent a more widespread regression in the UX. Then
> we can work on phasing out the private APIs without UX regressions.

sgtm

[erikchen, 2016-03-15 00:20:03]

Description was changed from

==========
mac: Remove uses of a CoreGraphics private API that causes WindowServer crashes.

Remove all uses of CGSSetWindowWarp, which deterministically crashes OS X VMs,
and likely causes crashes on real machines as well.

BUG=515627, 548824
==========

to

==========
mac: Implement a simple animation for CustomConstrainedWindowSheet.

The original animation uses CoreGraphics private APIs. This causes WindowServer
crashes when used by the extension install dialog. This CL adds a simple
animation for the extension install dialog to use.

The code for performing the simple animation came from tapted's CL:
https://codereview.chromium.org/1794023004

BUG=515627, 548824
==========

[erikchen, 2016-03-15 00:22:47]

tapted: PTAL

[erikchen, 2016-03-15 00:22:52]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-15 00:23:06]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1787553003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1787553003/40001

[tapted, 2016-03-15 00:39:40]

lgtm

we could also use an @property, but I'm not overly enamoured about them -I'll
leave it up to you :)

https://codereview.chromium.org/1787553003/diff/40001/chrome/browser/ui/cocoa...
File
chrome/browser/ui/cocoa/constrained_window/constrained_window_custom_sheet.mm
(right):

https://codereview.chromium.org/1787553003/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/constrained_window/constrained_window_custom_sheet.mm:40:
[[NSAnimationContext currentContext] setDuration:kAnimationDuration];
I think we need a

[NSAnimationContext beginGrouping];

before this

and a 

[NSAnimationContext endGrouping];

after the setAlphaValue.

I guess the duration might "leak" out to other animations otherwise.

https://codereview.chromium.org/1787553003/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller.mm
(right):

https://codereview.chromium.org/1787553003/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller.mm:63:
[sheet setUseSimpleAnimations:YES];
nit: I guess it's somewhat redundant, but we should say the extension install
dialog is special, so perhaps a comment here like

  // The extension install dialog can cause window server crashes when using the
  // complex animations provided by private APIs, so use simple animations.
  // See http://crbug.com/548824.

[erikchen, 2016-03-15 00:52:25]

https://codereview.chromium.org/1787553003/diff/40001/chrome/browser/ui/cocoa...
File
chrome/browser/ui/cocoa/constrained_window/constrained_window_custom_sheet.mm
(right):

https://codereview.chromium.org/1787553003/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/constrained_window/constrained_window_custom_sheet.mm:40:
[[NSAnimationContext currentContext] setDuration:kAnimationDuration];
On 2016/03/15 00:39:40, tapted wrote:
> I think we need a
> 
> [NSAnimationContext beginGrouping];
> 
> before this
> 
> and a 
> 
> [NSAnimationContext endGrouping];
> 
> after the setAlphaValue.
> 
> I guess the duration might "leak" out to other animations otherwise.

Done.

https://codereview.chromium.org/1787553003/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller.mm
(right):

https://codereview.chromium.org/1787553003/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller.mm:63:
[sheet setUseSimpleAnimations:YES];
On 2016/03/15 00:39:40, tapted wrote:
> nit: I guess it's somewhat redundant, but we should say the extension install
> dialog is special, so perhaps a comment here like
> 
>   // The extension install dialog can cause window server crashes when using
the
>   // complex animations provided by private APIs, so use simple animations.
>   // See http://crbug.com/548824.

Done.

[erikchen, 2016-03-15 00:52:29]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2016-03-15 00:52:30]

The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org
Link to the patchset: https://codereview.chromium.org/1787553003/#ps60001
(title: "Comments from tapted.")

[commit-bot: I haz the power, 2016-03-15 00:52:38]

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

[commit-bot: I haz the power, 2016-03-15 01:27:18]

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

[commit-bot: I haz the power, 2016-03-15 01:27:19]

Try jobs failed on following builders:
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[erikchen, 2016-03-15 01:43:43]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2016-03-15 01:43:43]

The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org
Link to the patchset: https://codereview.chromium.org/1787553003/#ps80001
(title: "Add semicolons.")

[commit-bot: I haz the power, 2016-03-15 01:44:04]

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

[commit-bot: I haz the power, 2016-03-15 02:57:03]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-03-15 02:58:09]

Description was changed from

==========
mac: Implement a simple animation for CustomConstrainedWindowSheet.

The original animation uses CoreGraphics private APIs. This causes WindowServer
crashes when used by the extension install dialog. This CL adds a simple
animation for the extension install dialog to use.

The code for performing the simple animation came from tapted's CL:
https://codereview.chromium.org/1794023004

BUG=515627, 548824
==========

to

==========
mac: Implement a simple animation for CustomConstrainedWindowSheet.

The original animation uses CoreGraphics private APIs. This causes WindowServer
crashes when used by the extension install dialog. This CL adds a simple
animation for the extension install dialog to use.

The code for performing the simple animation came from tapted's CL:
https://codereview.chromium.org/1794023004

BUG=515627, 548824

Committed: https://crrev.com/480c8640a6aaed2cde12711c2985d8c6a4221060
Cr-Commit-Position: refs/heads/master@{#381162}
==========

[commit-bot: I haz the power, 2016-03-15 02:58:10]

Patchset 5 (id:??) landed as
https://crrev.com/480c8640a6aaed2cde12711c2985d8c6a4221060
Cr-Commit-Position: refs/heads/master@{#381162}