The test fails on the line "ASSERT_TRUE(chrome::CanGoForward(browser()));". It checks that we should wait for two LOAD_STOP events, one for each navigation.

vasilii@: could your CLs in build 7738 (https://build.chromium.org/p/chromium.mac/builders/Mac10.10%20Tests/builds/7738) affect on LoginPromptBrowserTest? The test checks navigation/load events. 

None of 2 CLs can affect the test. They don't trigger the Credential Manager API.

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

commit 470a0791c72272cf6f17abb41ba4127c15687e30
Author: samli <samli@chromium.org>
Date: Fri Aug 12 01:39:27 2016

Sheriff: Disable LoginPromptBrowserTest.TestCancelAuth

Flaky on Mac

NOTRY=true
TBR=kolos
BUG= 636875 

Review-Url: https://codereview.chromium.org/2239023003
Cr-Commit-Position: refs/heads/master@{#411508}

[modify] https://crrev.com/470a0791c72272cf6f17abb41ba4127c15687e30/chrome/browser/ui/login/login_handler_browsertest.cc

Looking at the pre-revert blame https://chromium.googlesource.com/chromium/src/+blame/b7235e71f06373f2cdcd166ab4297a2e1cb0e809/chrome/browser/ui/login/login_handler_browsertest.cc, the test is quite old, was moved by asanka@ in 2011 in https://codereview.chromium.org/8316022 and existed even before. (Cc-ing asanka just in case they would like to provide any advice; if not, feel feel free to un-Cc. Thanks!)

While I agree we should break up the test into smaller pieces, I don't think the failed assert with about going forward is caused by that. Reading the code I'm not sure why this is expected to work:
(1) Navigate to auth_page_3 and go back.
(2) Navigate to auth_page and then press forward during navigation.
What I would expect is that after navigating to auth_page, the "next forward" entry would be reset.

I'll have a deeper look.

Also, attaching the logs from the referenced build.

Deleted: Log File contents.html 515 KB

Log File contents.html 515 KB View Download

So far I split the test locally, and got the forward part flaking even on Linux, hinting at platform independence. So I will upload the split, keeping the disabled part disabled on all platforms, and will have a look at that specifically in the next step.

The splitting CL is https://codereview.chromium.org/2244643002/.

Looking at how manual browsing in Chromium works (testing with http://httpbin.org/basic-auth/u/p), I see that my expectations stated in #6 are off and that the test seems to model a legal flow. Will still look into whether there is something one needs to wait for until the navigation controller has up-to-date info.

Logging my debugging results so far:

The flaky test does 2 navigations + one back navigation (to enable a forward navigation afterwards). In all cases, the navigation controller has 3 entries, with 0 being some default, and 1 and 2 being the 2 extra navigations. I was observing the controller at two points: just after the back navigation was confirmed to have stopped loading (by WindowedLoadStopObserver), and just after auth was needed (confirmed by WindowedAuthNeededObserver).

When the test passed, I saw this:
back navigation stopped loading: last entry committed = 1, no pending entry
auth was needed:                 last entry committed = 1, no pending entry

When the test failed, I saw this:
back navigation stopped loading: last entry committed = 2, pending entry = 1
auth was needed:                 last entry committed = 2, no pending entry

I am not sure whether "stopped loading" implies "there should be no pending entry". If it does, then already the first event in the failing situation is wrong. But the second entry is definitely wrong, as it looks like no "back" happened.

Further debugging confirms that the entry 2 in the failing case after "auth needed" is indeed the no-auth page which the back button should get rid of, not the auth page which triggered the auth challenge. So likely the GoBack can fail, and I'll have a look, why.

More notes:
During the back navigation, success is reported, pending is kept 1, last committed 2. Some time after that, pending is cleared (to -1), but committed stays 2. These changes seem to happen in response to Blink signalising load. I have not found the exact spot from where the pending index is cleared and the committed kept at 2, but I continue looking. The served pages all come with HTTP 404, but I'm not sure that's relevant (as that's the same in passing and failing runs).

Re-reading my comments so far, the race seems to be this:

The test waits until GoBack stops loading, but not until NavigationControllerImpl::RendererDidNavigate is called. Starting the load for the auth page seems to interrupt the back-navigation, so that  NavigationControllerImpl::RendererDidNavigate is never called if it is not called before navigation to the auth page. That keeps the current entry at 2, making going forward impossible.

I am still not sure I understand the expected order of these events, and I do not know so far how to wait until the NavigationControllerImpl::RendererDidNavigate moment. Looking further.


Also, WindowedNavigationObserver uses deprecated notifications and should be migrated to WebContentsObserver. I'll consider that, although it is likely not the culprit here.

Alright, investigating navigation events was interesting, but I got lost. During the investigation I noticed some code depending on whether the page load was a success or error, so I tried replacing the HTTP 404 - throwing non-existing test pages with real ones, and the flake disappeared (meaning: with non-existent ones, I was able to see failures it multiple times in 10 runs, with existing ones them 100 out of 100 runs passed).

Therefore I am going to upload a fix migrating the tests to existing URLs and move on.

The CL with the fix is at https://codereview.chromium.org/2246513002/. Together with https://codereview.chromium.org/2244643002/ they now both await review.

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

commit 9544b8426ac4cc3aafdca87a56426de3974f9f04
Author: vabr <vabr@chromium.org>
Date: Fri Aug 12 20:08:02 2016

Split LoginPromptBrowserTest.TestCancelAuth

The TestCancelAuth test contains 4 different test scenarios. This CL makes them separate test cases.

The CL also changes the chrome::[Can]Go* calls to calling the navigation controller for them directly (that's what the static version did anyway) and does minor readability improvements.

BUG= 636875 
R=kolos@chromium.org

Review-Url: https://codereview.chromium.org/2244643002
Cr-Commit-Position: refs/heads/master@{#411751}

[modify] https://crrev.com/9544b8426ac4cc3aafdca87a56426de3974f9f04/chrome/browser/ui/login/login_handler_browsertest.cc

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

commit 006b645010996ba6ce80fd0511a0f30b54da088e
Author: vabr <vabr@chromium.org>
Date: Fri Aug 12 20:13:38 2016

Enable LoginPromptBrowserTest.TestCancelAuth_OnForward

The test was flaky because navigation back was sometimes interrupted by
navigation to a new site before the back-navigation entry could transition from
"pending" to "committed".

The associated bug has some more details, but in the end, making sure that none
of the URLs the test navigated to returned a HTTP 404 fixed the failures.

BUG= 636875 
R=meacer@chromium.org

Review-Url: https://codereview.chromium.org/2246513002
Cr-Commit-Position: refs/heads/master@{#411752}

[modify] https://crrev.com/006b645010996ba6ce80fd0511a0f30b54da088e/chrome/browser/ui/login/login_handler_browsertest.cc