Fix Fullscreen Animation Crash

Fix Fullscreen Animation Crash

Ensure that the fullscreen transition object is handled properly when
the BrowserWindowController is deallocated.

BUG=598285
Committed: https://crrev.com/a3b084b28b3ddc741c8f74a6e6e6dc790a58cf75
Cr-Commit-Position: refs/heads/master@{#384985}

[spqchan, 2016-04-01 19:56:22]

Description was changed from

==========
Fix Fullscreen Animation Crash

Ensure that the fullscreen transition object is handled properly when
the BrowserWindowController is deallocated properly.

BUG=598285
==========

to

==========
Fix Fullscreen Animation Crash

Ensure that the fullscreen transition object is handled properly when
the BrowserWindowController is deallocated.

BUG=598285
==========

[spqchan, 2016-04-01 19:56:22]

spqchan@chromium.org changed reviewers:
+ rsesek@chromium.org

[spqchan, 2016-04-01 19:59:04]

PTAL

[Robert Sesek, 2016-04-01 20:04:05]

https://codereview.chromium.org/1850313002/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser_window_controller.mm (right):

https://codereview.chromium.org/1850313002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller.mm:465:
fullscreenTransition_.reset();
resetCustomAppKitFullscreenVariables does this too. But does this stop the
animation? Doesn't the CAAnimation hold a reference to the Transition object,
meaning that the Transition won't actually be dealloc'd here (until the
animation ends)?

[spqchan, 2016-04-04 16:36:16]

PTAL

https://codereview.chromium.org/1850313002/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser_window_controller.mm (right):

https://codereview.chromium.org/1850313002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller.mm:465:
fullscreenTransition_.reset();
On 2016/04/01 20:04:05, Robert Sesek wrote:
> resetCustomAppKitFullscreenVariables does this too. But does this stop the
> animation? Doesn't the CAAnimation hold a reference to the Transition object,
> meaning that the Transition won't actually be dealloc'd here (until the
> animation ends)?

That's a good point, I removed all the animations from the layers before
reseting it.

I've been trying simulating the bug by killing the browser process during the
fullscreen transition and etc, but it doesn't seem like the most reliable way to
do it. Is there are good way for me to simulate this? Thanks!

[Robert Sesek, 2016-04-04 18:13:50]

https://codereview.chromium.org/1850313002/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm (right):

https://codereview.chromium.org/1850313002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:262: controller_
= nullptr;
nullptr -> nil

https://codereview.chromium.org/1850313002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:537: if
(!isEnteringFullscreen_ && controller_)
Don't need the && controller_ since it's an ObjC object and messaging nil is a
noop.

[spqchan, 2016-04-04 18:40:35]

PTAL

https://codereview.chromium.org/1850313002/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm (right):

https://codereview.chromium.org/1850313002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:262: controller_
= nullptr;
On 2016/04/04 18:13:50, Robert Sesek wrote:
> nullptr -> nil

Done.

https://codereview.chromium.org/1850313002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_fullscreen_transition.mm:537: if
(!isEnteringFullscreen_ && controller_)
On 2016/04/04 18:13:50, Robert Sesek wrote:
> Don't need the && controller_ since it's an ObjC object and messaging nil is a
> noop.

Done.

[Robert Sesek, 2016-04-04 18:42:21]

LGTM

[spqchan, 2016-04-04 19:05:21]

The CQ bit was checked by spqchan@chromium.org

[commit-bot: I haz the power, 2016-04-04 19:05:49]

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

[commit-bot: I haz the power, 2016-04-04 19:48:16]

Description was changed from

==========
Fix Fullscreen Animation Crash

Ensure that the fullscreen transition object is handled properly when
the BrowserWindowController is deallocated.

BUG=598285
==========

to

==========
Fix Fullscreen Animation Crash

Ensure that the fullscreen transition object is handled properly when
the BrowserWindowController is deallocated.

BUG=598285
==========

[commit-bot: I haz the power, 2016-04-04 19:48:17]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-04-04 19:49:23]

Description was changed from

==========
Fix Fullscreen Animation Crash

Ensure that the fullscreen transition object is handled properly when
the BrowserWindowController is deallocated.

BUG=598285
==========

to

==========
Fix Fullscreen Animation Crash

Ensure that the fullscreen transition object is handled properly when
the BrowserWindowController is deallocated.

BUG=598285

Committed: https://crrev.com/a3b084b28b3ddc741c8f74a6e6e6dc790a58cf75
Cr-Commit-Position: refs/heads/master@{#384985}
==========

[commit-bot: I haz the power, 2016-04-04 19:49:24]

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