mac: Fix a history swiper bug where the shield wouldn't disappear.

mac: Fix a history swiper bug where the shield wouldn't disappear.

There were 3 separate problems.
1. Retain bug :(
2. historyOverlay was removing the view from the superview during dealloc,
which is atypical. If the view didn't exist, historyOverlay was actually
creating the view, and sending a callback to the delegate.
3. We were not specifying an NSAnimationContext grouping/duration, which
non-deterministically would use a 0-duration animation, which actually
prevented the animation and callback from triggering. As a result, the
shield would never disappear.

On a side note: unit_tests did not work when run with
--gtest-filter=HistoryOverlay* because we did not specify an NSAnimationContext
with a non-zero duration. 0-duration animation contexts cause animations to not
fire, which meant that the delegate callback never went through.

BUG=330312
TEST=manual

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

[erikchen, 2013-12-27 23:29:13]

Please take a look.

I haven't confirmed that this fixes the problem, but it seems likely. I'm in the
process of preparing a binary for mehmet@ to test.

[Nico, 2013-12-27 23:56:13]

https://codereview.chromium.org/107343008/diff/1/chrome/browser/ui/cocoa/hist...
File chrome/browser/ui/cocoa/history_overlay_controller.mm (left):

https://codereview.chromium.org/107343008/diff/1/chrome/browser/ui/cocoa/hist...
chrome/browser/ui/cocoa/history_overlay_controller.mm:97: }
This was added in https://chromiumcodereview.appspot.com/10913066 / 
https://chromiumcodereview.appspot.com/16632009 . Did you check the TEST= lines
in those two CLs are still working with this gone?

[Nico, 2013-12-28 00:08:30]

https://codereview.chromium.org/107343008/diff/1/chrome/browser/renderer_host...
File
chrome/browser/renderer_host/chrome_render_widget_host_view_mac_history_swiper.mm
(left):

https://codereview.chromium.org/107343008/diff/1/chrome/browser/renderer_host...
chrome/browser/renderer_host/chrome_render_widget_host_view_mac_history_swiper.mm:249:
historyOverlay_ = [historyOverlay retain];
I guess this path isn't covered by tests, else asan should've found this, right?

[erikchen, 2013-12-30 19:20:33]

PTAL

question: When I run the unit tests locally, with my changes reverted,
'DismissClearsAnimations' hangs. Since I've migrated the code over to the other
test, now both of them hang locally. Any idea what's going on?

https://codereview.chromium.org/107343008/diff/1/chrome/browser/renderer_host...
File
chrome/browser/renderer_host/chrome_render_widget_host_view_mac_history_swiper.mm
(left):

https://codereview.chromium.org/107343008/diff/1/chrome/browser/renderer_host...
chrome/browser/renderer_host/chrome_render_widget_host_view_mac_history_swiper.mm:249:
historyOverlay_ = [historyOverlay retain];
On 2013/12/28 00:08:30, Nico wrote:
> I guess this path isn't covered by tests, else asan should've found this,
right?

As far as I can tell, this path is covered by my mac_history_swiper_unit_test.
(There's a partial mock that still forwards of this method to the real object,
and I know that line 242 is called, because it broke unit tests when I added it.

According to llvm, LeakSanitizer is still in experimental mode, so I'm assuming
its stable, but doesn't catch all leaks...

https://codereview.chromium.org/107343008/diff/1/chrome/browser/ui/cocoa/hist...
File chrome/browser/ui/cocoa/history_overlay_controller.mm (left):

https://codereview.chromium.org/107343008/diff/1/chrome/browser/ui/cocoa/hist...
chrome/browser/ui/cocoa/history_overlay_controller.mm:97: }
On 2013/12/27 23:56:13, Nico wrote:
> This was added in https://chromiumcodereview.appspot.com/10913066 / 
> https://chromiumcodereview.appspot.com/16632009 . Did you check the TEST=
lines
> in those two CLs are still working with this gone?

I've manually tested the repro conditions with both magic mouse and track pad,
and the bugs do not reproduce. Unsurprisingly, the unit tests fail.

[Robert Sesek, 2013-12-30 19:23:27]

On 2013/12/30 19:20:33, erikchen1 wrote:
> PTAL
> 
> question: When I run the unit tests locally, with my changes reverted,
> 'DismissClearsAnimations' hangs. Since I've migrated the code over to the
other
> test, now both of them hang locally. Any idea what's going on?

These tests all run the MessagePump (essentially CFRunLoop) while waiting on the
animation to complete. They assign a completion block to then stop the loop once
the animation completes. If it's hanging, -animationDidStop:finished: is not
being called (or the mock is not matching it). C.f. lines 90-98.

[erikchen, 2013-12-30 20:11:49]

On 2013/12/30 19:23:27, rsesek wrote:
> On 2013/12/30 19:20:33, erikchen1 wrote:
> > PTAL
> > 
> > question: When I run the unit tests locally, with my changes reverted,
> > 'DismissClearsAnimations' hangs. Since I've migrated the code over to the
> other
> > test, now both of them hang locally. Any idea what's going on?
> 
> These tests all run the MessagePump (essentially CFRunLoop) while waiting on
the
> animation to complete. They assign a completion block to then stop the loop
once
> the animation completes. If it's hanging, -animationDidStop:finished: is not
> being called (or the mock is not matching it). C.f. lines 90-98.

I'm trying to figure out why the test 'DismissClearsAnimations' hangs on my
local machine on a clean repo, but works on the try-bots. This difference makes
me think that the error is not in code-land but in a compile/run time
configuration problem.

[Robert Sesek, 2013-12-30 21:05:46]

On 2013/12/30 20:11:49, erikchen1 wrote:
> On 2013/12/30 19:23:27, rsesek wrote:
> > On 2013/12/30 19:20:33, erikchen1 wrote:
> > > PTAL
> > > 
> > > question: When I run the unit tests locally, with my changes reverted,
> > > 'DismissClearsAnimations' hangs. Since I've migrated the code over to the
> > other
> > > test, now both of them hang locally. Any idea what's going on?
> > 
> > These tests all run the MessagePump (essentially CFRunLoop) while waiting on
> the
> > animation to complete. They assign a completion block to then stop the loop
> once
> > the animation completes. If it's hanging, -animationDidStop:finished: is not
> > being called (or the mock is not matching it). C.f. lines 90-98.
> 
> I'm trying to figure out why the test 'DismissClearsAnimations' hangs on my
> local machine on a clean repo, but works on the try-bots. This difference
makes
> me think that the error is not in code-land but in a compile/run time
> configuration problem.

The bots may run the tests using a different test runner strategy. I'm not
abreast of all the changes that happened in this area recently, but there are
threads on chromium-dev about it. At the very least, try using the exact
invocation locally that's printed to the stdout of the bots.

[erikchen, 2013-12-30 21:40:21]

On 2013/12/30 21:05:46, rsesek wrote:
> On 2013/12/30 20:11:49, erikchen1 wrote:
> > On 2013/12/30 19:23:27, rsesek wrote:
> > > On 2013/12/30 19:20:33, erikchen1 wrote:
> > > > PTAL
> > > > 
> > > > question: When I run the unit tests locally, with my changes reverted,
> > > > 'DismissClearsAnimations' hangs. Since I've migrated the code over to
the
> > > other
> > > > test, now both of them hang locally. Any idea what's going on?
> > > 
> > > These tests all run the MessagePump (essentially CFRunLoop) while waiting
on
> > the
> > > animation to complete. They assign a completion block to then stop the
loop
> > once
> > > the animation completes. If it's hanging, -animationDidStop:finished: is
not
> > > being called (or the mock is not matching it). C.f. lines 90-98.
> > 
> > I'm trying to figure out why the test 'DismissClearsAnimations' hangs on my
> > local machine on a clean repo, but works on the try-bots. This difference
> makes
> > me think that the error is not in code-land but in a compile/run time
> > configuration problem.
> 
> The bots may run the tests using a different test runner strategy. I'm not
> abreast of all the changes that happened in this area recently, but there are
> threads on chromium-dev about it. At the very least, try using the exact
> invocation locally that's printed to the stdout of the bots.

I've already tried running the tests with all the relevant flags from the
testing bots. As far as I can tell, the flags pertain to sharding and
multi-threading, neither of which are particularly relevant to this bug. Given
that the tests still pass on the testing-bots, I'm not going to worry too much
about this. My best guess is that there's been a behavioral change in 10.9 vs
10.7/10.6 to CFRunLoop. If you want me to fix the test so that it runs locally,
I would just rewrite the logic to use a more standard form of flushing the
CFRunLoop (rather than calling Quit() from within the run loop.

[Robert Sesek, 2013-12-30 21:51:32]

On 2013/12/30 21:40:21, erikchen1 wrote:
> On 2013/12/30 21:05:46, rsesek wrote:
> > On 2013/12/30 20:11:49, erikchen1 wrote:
> > > On 2013/12/30 19:23:27, rsesek wrote:
> > > > On 2013/12/30 19:20:33, erikchen1 wrote:
> > > > > PTAL
> > > > > 
> > > > > question: When I run the unit tests locally, with my changes reverted,
> > > > > 'DismissClearsAnimations' hangs. Since I've migrated the code over to
> the
> > > > other
> > > > > test, now both of them hang locally. Any idea what's going on?
> > > > 
> > > > These tests all run the MessagePump (essentially CFRunLoop) while
waiting
> on
> > > the
> > > > animation to complete. They assign a completion block to then stop the
> loop
> > > once
> > > > the animation completes. If it's hanging, -animationDidStop:finished: is
> not
> > > > being called (or the mock is not matching it). C.f. lines 90-98.
> > > 
> > > I'm trying to figure out why the test 'DismissClearsAnimations' hangs on
my
> > > local machine on a clean repo, but works on the try-bots. This difference
> > makes
> > > me think that the error is not in code-land but in a compile/run time
> > > configuration problem.
> > 
> > The bots may run the tests using a different test runner strategy. I'm not
> > abreast of all the changes that happened in this area recently, but there
are
> > threads on chromium-dev about it. At the very least, try using the exact
> > invocation locally that's printed to the stdout of the bots.
> 
> I've already tried running the tests with all the relevant flags from the
> testing bots. As far as I can tell, the flags pertain to sharding and
> multi-threading, neither of which are particularly relevant to this bug. Given

I wouldn't assume that the sharding and threading do not affect this, since test
parallelization and nondeterministic ordering definitely affect the test runs.

> that the tests still pass on the testing-bots, I'm not going to worry too much
> about this. My best guess is that there's been a behavioral change in 10.9 vs
> 10.7/10.6 to CFRunLoop. If you want me to fix the test so that it runs
locally,
> I would just rewrite the logic to use a more standard form of flushing the
> CFRunLoop (rather than calling Quit() from within the run loop.

Do the other tests that use this pattern pass, though (you haven't mentioned
otherwise, so I'm guessing yes)? I'd be interesting to know why that is the case
if so. What's a "more standard form of flushing the CFRunLoop"? It's completely
safe to call CFRunLoopStop() on the running loop (which is what Quit() does).

[erikchen, 2013-12-30 22:22:41]

On 2013/12/30 21:51:32, rsesek wrote:
> On 2013/12/30 21:40:21, erikchen1 wrote:
> > On 2013/12/30 21:05:46, rsesek wrote:
> > > On 2013/12/30 20:11:49, erikchen1 wrote:
> > > > On 2013/12/30 19:23:27, rsesek wrote:
> > > > > On 2013/12/30 19:20:33, erikchen1 wrote:
> > > > > > PTAL
> > > > > > 
> > > > > > question: When I run the unit tests locally, with my changes
reverted,
> > > > > > 'DismissClearsAnimations' hangs. Since I've migrated the code over
to
> > the
> > > > > other
> > > > > > test, now both of them hang locally. Any idea what's going on?
> > > > > 
> > > > > These tests all run the MessagePump (essentially CFRunLoop) while
> waiting
> > on
> > > > the
> > > > > animation to complete. They assign a completion block to then stop the
> > loop
> > > > once
> > > > > the animation completes. If it's hanging, -animationDidStop:finished:
is
> > not
> > > > > being called (or the mock is not matching it). C.f. lines 90-98.
> > > > 
> > > > I'm trying to figure out why the test 'DismissClearsAnimations' hangs on
> my
> > > > local machine on a clean repo, but works on the try-bots. This
difference
> > > makes
> > > > me think that the error is not in code-land but in a compile/run time
> > > > configuration problem.
> > > 
> > > The bots may run the tests using a different test runner strategy. I'm not
> > > abreast of all the changes that happened in this area recently, but there
> are
> > > threads on chromium-dev about it. At the very least, try using the exact
> > > invocation locally that's printed to the stdout of the bots.
> > 
> > I've already tried running the tests with all the relevant flags from the
> > testing bots. As far as I can tell, the flags pertain to sharding and
> > multi-threading, neither of which are particularly relevant to this bug.
Given
> 
> I wouldn't assume that the sharding and threading do not affect this, since
test
> parallelization and nondeterministic ordering definitely affect the test runs.
You are right.
I was running my tests with a --gtest_filter. It is possible that testing state
from previous tests was leaking into this test, which allowed it to succeed,
though that's pretty scary in and of itself.
> 
> > that the tests still pass on the testing-bots, I'm not going to worry too
much
> > about this. My best guess is that there's been a behavioral change in 10.9
vs
> > 10.7/10.6 to CFRunLoop. If you want me to fix the test so that it runs
> locally,
> > I would just rewrite the logic to use a more standard form of flushing the
> > CFRunLoop (rather than calling Quit() from within the run loop.
> 
> Do the other tests that use this pattern pass, though (you haven't mentioned
> otherwise, so I'm guessing yes)? I'd be interesting to know why that is the
case
> if so. What's a "more standard form of flushing the CFRunLoop"? It's
completely
> safe to call CFRunLoopStop() on the running loop (which is what Quit() does).
A search in our code base for 'new base::MessagePumpNSRunLoop' shows that there
are exactly 4 matches. The other 3 are used in different ways from the one that
we're discussing. The pattern we're looking at right now is unique.

The way I've seen and used CFRunLoop in testing environments is with the api
'CFRunLoopRunInMode'.
while (waitForConditionToHappen) {
  // Let the current run loop run for a small number of seconds.
  CFRunLoopRunInMode(mode, 0.001, NO);
}

I'd have to take a more careful look at how we use run loops in our testing
code, since it looks like we're making an NSRunLoop, and then using CFRunLoop
apis.

[Robert Sesek, 2013-12-30 22:43:56]

On 2013/12/30 22:22:41, erikchen1 wrote:
> On 2013/12/30 21:51:32, rsesek wrote:
> > On 2013/12/30 21:40:21, erikchen1 wrote:
> > > On 2013/12/30 21:05:46, rsesek wrote:
> > > > On 2013/12/30 20:11:49, erikchen1 wrote:
> > > > > On 2013/12/30 19:23:27, rsesek wrote:
> > > > > > On 2013/12/30 19:20:33, erikchen1 wrote:
> > > > > > > PTAL
> > > > > > > 
> > > > > > > question: When I run the unit tests locally, with my changes
> reverted,
> > > > > > > 'DismissClearsAnimations' hangs. Since I've migrated the code over
> to
> > > the
> > > > > > other
> > > > > > > test, now both of them hang locally. Any idea what's going on?
> > > > > > 
> > > > > > These tests all run the MessagePump (essentially CFRunLoop) while
> > waiting
> > > on
> > > > > the
> > > > > > animation to complete. They assign a completion block to then stop
the
> > > loop
> > > > > once
> > > > > > the animation completes. If it's hanging,
-animationDidStop:finished:
> is
> > > not
> > > > > > being called (or the mock is not matching it). C.f. lines 90-98.
> > > > > 
> > > > > I'm trying to figure out why the test 'DismissClearsAnimations' hangs
on
> > my
> > > > > local machine on a clean repo, but works on the try-bots. This
> difference
> > > > makes
> > > > > me think that the error is not in code-land but in a compile/run time
> > > > > configuration problem.
> > > > 
> > > > The bots may run the tests using a different test runner strategy. I'm
not
> > > > abreast of all the changes that happened in this area recently, but
there
> > are
> > > > threads on chromium-dev about it. At the very least, try using the exact
> > > > invocation locally that's printed to the stdout of the bots.
> > > 
> > > I've already tried running the tests with all the relevant flags from the
> > > testing bots. As far as I can tell, the flags pertain to sharding and
> > > multi-threading, neither of which are particularly relevant to this bug.
> Given
> > 
> > I wouldn't assume that the sharding and threading do not affect this, since
> test
> > parallelization and nondeterministic ordering definitely affect the test
runs.
> You are right.
> I was running my tests with a --gtest_filter. It is possible that testing
state
> from previous tests was leaking into this test, which allowed it to succeed,
> though that's pretty scary in and of itself.
> > 
> > > that the tests still pass on the testing-bots, I'm not going to worry too
> much
> > > about this. My best guess is that there's been a behavioral change in 10.9
> vs
> > > 10.7/10.6 to CFRunLoop. If you want me to fix the test so that it runs
> > locally,
> > > I would just rewrite the logic to use a more standard form of flushing the
> > > CFRunLoop (rather than calling Quit() from within the run loop.
> > 
> > Do the other tests that use this pattern pass, though (you haven't mentioned
> > otherwise, so I'm guessing yes)? I'd be interesting to know why that is the
> case
> > if so. What's a "more standard form of flushing the CFRunLoop"? It's
> completely
> > safe to call CFRunLoopStop() on the running loop (which is what Quit()
does).
> A search in our code base for 'new base::MessagePumpNSRunLoop' shows that
there
> are exactly 4 matches. The other 3 are used in different ways from the one
that
> we're discussing. The pattern we're looking at right now is unique.

It's unique in that it's executing Quit() from a block, but all of the results
from codesearch run the loop, wait for an asynchronous event, and then call
Quit() from the running loop. They all seem quite similar to me.

> The way I've seen and used CFRunLoop in testing environments is with the api
> 'CFRunLoopRunInMode'.
> while (waitForConditionToHappen) {
>   // Let the current run loop run for a small number of seconds.
>   CFRunLoopRunInMode(mode, 0.001, NO);
> }

This is functionally equivalent to what is being done now (in terms of how the
run loop is activated) except that it also invokes a busy-wait loop. I don't
think this preferable.

> I'd have to take a more careful look at how we use run loops in our testing
> code, since it looks like we're making an NSRunLoop, and then using CFRunLoop
> apis.

Yes, this is true and could be problematic. The issue is that NSRunLoop provides
no programmatic stopping function and IIRC CFRunLoop alone cannot run
animations. In the test environment, this doesn't seem to cause issues though.

[erikchen, 2013-12-31 00:36:07]

Found the problem - we were missing an animation context, so we were in fact
leaking state from the tests that ran previously...

[erikchen, 2014-01-02 17:35:13]

On 2013/12/31 00:36:07, erikchen1 wrote:
> Found the problem - we were missing an animation context, so we were in fact
> leaking state from the tests that ran previously...

nico, PTAL

[Robert Sesek, 2014-01-02 18:30:36]

LGTM

[Nico, 2014-01-02 18:39:12]

On Thu, Jan 2, 2014 at 9:35 AM, <erikchen@chromium.org> wrote:

> On 2013/12/31 00:36:07, erikchen1 wrote:
>
>> Found the problem - we were missing an animation context, so we were in
>> fact
>> leaking state from the tests that ran previously...
>>
>
> nico, PTAL
>

Looks like rsesek reviewed this already.


>
> https://codereview.chromium.org/107343008/
>

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

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

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erikchen@chromium.org/107343008/210001

[commit-bot: I haz the power, 2014-01-02 19:45:48]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Nico, 2014-01-02 20:02:42]

chrome/browser/renderer_host lgtm

sorry, thought this was fully in c/b/ui/cocoa :-/

[commit-bot: I haz the power, 2014-01-02 20:03:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erikchen@chromium.org/107343008/210001

[commit-bot: I haz the power, 2014-01-02 22:14:02]

Retried try job too often on win for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2014-01-02 22:25:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erikchen@chromium.org/107343008/210001

[commit-bot: I haz the power, 2014-01-03 00:04:00]

Retried try job too often on win for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2014-01-03 00:15:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erikchen@chromium.org/107343008/600001

[commit-bot: I haz the power, 2014-01-03 00:46:58]

Retried try job too often on linux_rel for step(s) chromedriver2_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2014-01-03 16:51:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erikchen@chromium.org/107343008/600001

[commit-bot: I haz the power, 2014-01-03 17:36:50]

Retried try job too often on linux_rel for step(s) chromedriver2_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2014-01-03 18:46:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erikchen@chromium.org/107343008/600001

[commit-bot: I haz the power, 2014-01-03 19:43:36]

Change committed as 242911