Looking at the flakiness log https://test-results.appspot.com/dashboards/flakiness_dashboard.html#testType=chrome_public_test_apk&tests=org.chromium.chrome.browser.bookmarks.BookmarkTest%23testSearchBookmarks, it looks as if this has been badly flaky (failing most runs, but occasionally passing) since 2017-5-10.

The immediate cause seems to be the switch to Junit4 (https://codereview.chromium.org/2856333002). A local build of this as it is currently (with Junit4) failed 9 out of 12 runs. After reverting the changes to BookmarkTest.java from that CL the test ran 20 times without failing.

I suspect the actual problem is a problem with BookmarkManager when the activity is shut down; switching from Junit3 to Junit4 changes how activities are created and destroyed.

As sheriff I will land a CL that switches just BookmarkTest back to Junit3 (I don't want to revert the whole of https://codereview.chromium.org/2856333002), but this needs further investigation.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7c66cb42c5d8bb5f5b80a6456ab1d1b0dd259604

commit 7c66cb42c5d8bb5f5b80a6456ab1d1b0dd259604
Author: Anthony Berent <aberent@chromium.org>
Date: Mon May 15 15:04:44 2017

Switch BookmarkTest back to Junit3

Switching BookmarkTest to Junit4 make testSearchBookmarks very flaky on
tablets. As a short term solutions switch it back to Junit3.

This reverts part of https://codereview.chromium.org/2856333002.

BUG= 722303 

Change-Id: I99b93ea2ee9f1e7463740ebf060bda3e38086f46
Reviewed-on: https://chromium-review.googlesource.com/506010
Reviewed-by: Theresa Wellington <twellington@chromium.org>
Cr-Commit-Position: refs/heads/master@{#471765}
[modify] https://crrev.com/7c66cb42c5d8bb5f5b80a6456ab1d1b0dd259604/chrome/android/javatests/src/org/chromium/chrome/browser/bookmarks/BookmarkTest.java

hmm, ya, this should be a JUnit4 problem related to how the activity launch is handled in the activity test rule. I can take a look at this after the majority tests are converted

As +aberent mentioned in comment2 and told me offline, this is likely not JUnit4 issue. Re assigning back to twellington

More specifically, without investigating this in great detail, the lack of checks for an empty stack before popping the stack in BookmarkManager.closeSearchUI() looked suspicious to me. In all other cases BookmarkManager checks for an empty stack before popping it.

If the search view is showing, there should be something below BookmarkUIState#STATE_SEARCHING on the stack. It is invalid to call closeSearchUI() when the UI is not showing. Here's how the flow is supposed to work:

1) User opens bookmarks, state is pushed on stack
2) User starts a search, causing new state to be pushed on stack
3) User takes some action to hide search view, causing #onEndSearch() to be called
4) Search state is popped off stack, new state is set by popping state from #1 off the stack and pushing it again.

If the stack is empty at 408 that indicates there is no state before the search state or the user wasn't searching when the methods relating to ending a search are called, both of which shouldn't happen.

Note that earlier in the stack trace SelectableListLayout#onDetachedFromWindow() checks if mIsSearching before calling hideSearchView().

There's clearly something going wrong, but checking for an empty stack before popping is not likely the correct solution. I think that instead of calling BookmarkDelegate#closeSearchUI() directly, BookmarkTest should go through the proper SelectableList* widget methods to ensure that SelectableListToolbar is left in the correct state OR #closeSearchUI() should explicitly hide the SelectableListToolbar view if it is still showing.

Fortunately I don't think this represents a real user facing problem but rather a discrepancy in the path the test code exercises vs the actual viable in-product paths.

We should be able to call SelectableListToolbar#hideSearchView() instead of delegate#closeSearchUI() on line 260 of the BookmarkTest. 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6a6e35ff95c1cc413a8930d2e728830842e3fa56

commit 6a6e35ff95c1cc413a8930d2e728830842e3fa56
Author: Yoland Yan <yolandyan@chromium.org>
Date: Wed Oct 04 16:35:54 2017

Convert BookmarkTest to JUnit4

This conversion address the search UI unsync issue from crbug/722303.

For more on JUnit4 migration, please check
src/testing/android/docs/junit4.md

Bug: 640116, 722303 
Change-Id: I3f1aeea09e626ca6bdf16ff06bede2afe6387fd3
Reviewed-on: https://chromium-review.googlesource.com/699596
Reviewed-by: Theresa <twellington@chromium.org>
Commit-Queue: Yoland Yan <yolandyan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#506408}
[modify] https://crrev.com/6a6e35ff95c1cc413a8930d2e728830842e3fa56/chrome/android/javatests/src/org/chromium/chrome/browser/bookmarks/BookmarkTest.java