New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 809488 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Feb 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Bug-Regression

Blocked on:
issue 788901

Blocking:
issue 784904
issue 814080



Sign in to add a comment

NavigationControllerBrowserTest.RaceCrossOriginNavigationAndSameDocumentHistoryNavigation in content_browsertests failing on multiple builders

Project Member Reported by sheriff-...@appspot.gserviceaccount.com, Feb 6 2018

Issue description

Filed by sheriff-o-matic@appspot.gserviceaccount.com on behalf of gsennton@google.com

NavigationControllerBrowserTest.RaceCrossOriginNavigationAndSameDocumentHistoryNavigation in content_browsertests failing on multiple builders

Builders failed on: 
- KitKat Tablet Tester: 
  https://build.chromium.org/p/chromium.android/builders/KitKat%20Tablet%20Tester
- Marshmallow 64 bit Tester: 
  https://build.chromium.org/p/chromium.android/builders/Marshmallow%2064%20bit%20Tester

First failing build on Marshmallow tester:
https://uberchromegw.corp.google.com/i/chromium.android/builders/Marshmallow%2064%20bit%20Tester/builds/19377



 
Cc: clamy@chromium.org blundell@chromium.org
There are only 3 CLs in the blamelist, and I think only 2 of them touch this kind of code:

clamy@'s CL:
https://chromium.googlesource.com/chromium/src/+/af4bf2d9fdaa6ca56ac275c2a53b85f9cf51c394

blundell@'s CL:
https://chromium.googlesource.com/chromium/src/+/de1ec68cd038d5893641b01723e27732abca22b6
Cc: -clamy@chromium.org -blundell@chromium.org
Labels: OS-Android
Owner: clamy@chromium.org
Oh nvm, Clamy@'s CL touches the actual test, assigning to clamy@

Comment 3 by clamy@chromium.org, Feb 6 2018

Yes that's likely related. I will disable the test on Android while I investigate.

Comment 4 by kbr@chromium.org, Feb 7 2018

Blockedon: 788901
Blocking: 784904
Components: UI>Browser>Navigation Tests>Flaky Internals>Sandbox>SiteIsolation
Labels: -Pri-2 Sheriff-Chromium Pri-1 Type-Bug-Regression
This is P1 if not P0; it is causing many CLs to be rejected from the CQ or retried due the high failure rate. Only one example:
https://ci.chromium.org/buildbot/tryserver.chromium.android/android_n5x_swarming_rel/356477

I think the test should be turned off by the sheriffs until it's stabilized again.

Project Member

Comment 5 by bugdroid1@chromium.org, Feb 7 2018

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

commit 15cad144a25c5f8593c17b1cfe38e91ec315a397
Author: clamy <clamy@chromium.org>
Date: Wed Feb 07 10:56:58 2018

Disable one NavigationController test on Android

This CL disables
NavigationControllerBrowserTest.RaceCrossOriginNavigationAndSameDocumentHistoryNavigation
on Android while we investigate why it is flaky on this platform.

BUG= 809488 

Change-Id: Ib7cdc2fce5c817e466123a4e476c378b119a29ed
Reviewed-on: https://chromium-review.googlesource.com/904663
Reviewed-by: Gustav Sennton <gsennton@chromium.org>
Commit-Queue: Camille Lamy <clamy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#534979}
[modify] https://crrev.com/15cad144a25c5f8593c17b1cfe38e91ec315a397/content/browser/frame_host/navigation_controller_impl_browsertest.cc

Labels: -Sheriff-Chromium

Comment 7 by clamy@chromium.org, Feb 7 2018

So it seems that half of the time, we don't get the fallback for the same-document navigations to trigger. I'm investigating why.
Project Member

Comment 8 by bugdroid1@chromium.org, Feb 7 2018

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

commit e3c894eb0b22d2a85e7e46f6f24cb80961c8245d
Author: Andrey Kosyakov <caseq@chromium.org>
Date: Wed Feb 07 22:39:38 2018

Disable NavigationControllerBrowserTest.RaceCrossOriginNavigationAndSameDocumentHistoryNavigation

It's a flaky timeout on most platforms.

TBR: clamy
NOTRY: true
Bug:  809488 
Change-Id: Idfa6896f55622c5b79b7ffc81fe008241991daa3
Reviewed-on: https://chromium-review.googlesource.com/907646
Reviewed-by: Andrey Kosyakov <caseq@chromium.org>
Commit-Queue: Andrey Kosyakov <caseq@chromium.org>
Cr-Commit-Position: refs/heads/master@{#535167}
[modify] https://crrev.com/e3c894eb0b22d2a85e7e46f6f24cb80961c8245d/content/browser/frame_host/navigation_controller_impl_browsertest.cc

Comment 9 by clamy@chromium.org, Feb 8 2018

So looking at the test, I think we do have a race condition that leads to the deletion of the same-document NavigationRequest, preventing the fallback to take effect. I think this is a real issue, and will work on a CL to address it. Basically, we should be storing NavigationRequests for cross-document and same-document separately, and not delete them when one another starts or commits. This matches what happens in the renderer process.

Comment 10 by nasko@chromium.org, Feb 22 2018

Blocking: 814080
Project Member

Comment 11 by bugdroid1@chromium.org, Feb 23 2018

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

commit fc3850d936fa5922f720404cc8611462f8b32a1d
Author: clamy <clamy@chromium.org>
Date: Fri Feb 23 14:35:28 2018

Fix for same-document navigation fallback

This CL fixes an issue where the fallback to cross-document navigation
for a same-document navigation racing with a commit didn't work. The
commit would delete the same-document NavigationRequest, preventing it
from being restarted cross-document. In this CL, we store cross-document
and same-document NavigationRequests separately, as they do not cancel
each other. In particular, this means that a cross-document navigation
commit will not erase a same-document NavigationRequest and vice-versa.
This more accurately matches what happens in Blink's FrameLoader.

BUG= 809488 

Change-Id: I4ff91e938f18d6e6bed20e0a05a926569e970924
Reviewed-on: https://chromium-review.googlesource.com/925423
Commit-Queue: Camille Lamy <clamy@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#538777}
[modify] https://crrev.com/fc3850d936fa5922f720404cc8611462f8b32a1d/content/browser/frame_host/navigation_controller_impl_browsertest.cc
[modify] https://crrev.com/fc3850d936fa5922f720404cc8611462f8b32a1d/content/browser/frame_host/render_frame_host_impl.cc
[modify] https://crrev.com/fc3850d936fa5922f720404cc8611462f8b32a1d/content/browser/frame_host/render_frame_host_impl.h
[modify] https://crrev.com/fc3850d936fa5922f720404cc8611462f8b32a1d/content/browser/frame_host/render_frame_host_manager.cc
[modify] https://crrev.com/fc3850d936fa5922f720404cc8611462f8b32a1d/content/browser/web_contents/web_contents_impl.cc
[modify] https://crrev.com/fc3850d936fa5922f720404cc8611462f8b32a1d/content/public/test/navigation_simulator.cc

Comment 12 by clamy@chromium.org, Feb 23 2018

Status: Fixed (was: Available)

Sign in to add a comment