Using the per-revision bisect providing the bisect results,
Good build: 55.0.2873.0 (Revision: 421052).
Bad build: 55.0.2875.0 (Revision: 421703).

You are probably looking for a change made after 421223 (known good), but no later than 421224 (first known bad).
CHANGELOG URL:
  https://chromium.googlesource.com/chromium/src/+log/5b1519e7304f1a6d2f1c3e50c2d8a933e21f2806..092d3be76da7c2aef6a84e2791b5818a06cb4689

@jam -- Could you please look into the issue, pardon me if it has nothing to do with your changes and if possible please assign it to concern owner.
Adding RB Label as this is a recent Regression, please remove if not required.

Thank You.

A friendly reminder that M55 Stable is launch is coming soon! Your bug is labelled as Stable ReleaseBlock, pls make sure to land the fix and get it merged into the release branch by November 25th, 5:00 PM PST in order to make into the desktop Stable final build cut. Thank you!

Assigning to bauerb@chromium.org (CL reviewer - https://chromium.googlesource.com/chromium/src/+log/5b1519e7304f1a6d2f1c3e50c2d8a933e21f2806..092d3be76da7c2aef6a84e2791b5818a06cb4689) as jam@ is OOO for this week and next week.

Okay, if we want this to be fixed quickly, someone who is more familiar with navigation code (ie. PlzNavigate) will have to take a look at this.

All I can come up with is that there is some race condition between starting the navigation (which clears geolocation indicators) and updating the UI, and the behavior is changed by switching from DidStartProvisionalLoadForFrame() to DidStartNavigation().

It should also be noted that the behavior mentioned above (starting a navigation clears geolocation indicators) has been around since 2010 (https://codereview.chromium.org/3305004), but it doesn't automatically trigger a UI update. If that were to happen reliably, the geolocation icon would *always* disappear and appear again.

Thanks for looking at this. I've looked at jam@ CL and see one potential issue: he now clears cookie on an in-page navigation which was not done before. I think this may be the issue here. DidStartNavigation happens at the same time as DidStartProvisionalLoad, so this shouldn't be an issue. I'll see if a patch can fix this.

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

commit d796bf464ffe66b896c8def42b1f916f62da9928
Author: clamy <clamy@chromium.org>
Date: Wed Nov 23 18:35:11 2016

Don't update TabSpecificContentSettings on same-page navigation

This CL ensures that the TabSpecificContentSettings are not updated when
navigating same-page. This fixes an issue where the location tracking
icon would disappear when accepting to send location info to Google
search. This is because Google search would do a fragment navigation
when location tracking was accepted, which would fire DidStartNavigation
causing the location information to be reset. Previously,
DidStartProvisionalLoad would not be called for a fragment navigation,
so the icon would not be reset.

For the record, TabSpecificContentSettings was made to use DidStartNavigation
in https://codereview.chromium.org/2374443003.

BUG= 667256 

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

[modify] https://crrev.com/d796bf464ffe66b896c8def42b1f916f62da9928/chrome/browser/content_settings/tab_specific_content_settings.cc

Before we approve merge to M55, could you please make sure this change is well baked/verified in Canary and safe to merge to M55? Please note that M55 merge has to happen latest before 4:00 PM PT, Monday (11/28) in order to make into the desktop final build cut.

A friendly reminder that M55 Stable is launch is coming soon! Your bug is labelled as Stable ReleaseBlock, pls make sure to land the fix and get it merged into the release branch latest by November 28th, 4:00 PM PT in order to make into the desktop Stable final build cut (sooner the better). Thank you!

Your change meets the bar and is auto-approved for M55 (branch: 2883)

Tested the issue on Latest Canary# 57.0.2931.0 on Windows and Mac and is working as intended.
Location icon is displayed properly when Allow is clicked on bubble.
Attaching screencast for reference.

The Build# 57.0.2931.0 is not available for Linux. Will update the issue if a new build for Linux is available.

Adding TE-Verified-57.0.2931.0 labels as verified on Windows and Mac.
Thank You.

Deleted: 667256.mov 1.2 MB

667256.mov 1.2 MB Download

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

commit fadbabf762310ae5d44221c37929dbf4807f5fe9
Author: clamy <clamy@chromium.org>
Date: Fri Nov 25 09:55:42 2016

Don't update TabSpecificContentSettings on same-page navigation

This CL ensures that the TabSpecificContentSettings are not updated when
navigating same-page. This fixes an issue where the location tracking
icon would disappear when accepting to send location info to Google
search. This is because Google search would do a fragment navigation
when location tracking was accepted, which would fire DidStartNavigation
causing the location information to be reset. Previously,
DidStartProvisionalLoad would not be called for a fragment navigation,
so the icon would not be reset.

For the record, TabSpecificContentSettings was made to use DidStartNavigation
in https://codereview.chromium.org/2374443003.

BUG= 667256 

Review-Url: https://codereview.chromium.org/2524053002
Cr-Commit-Position: refs/heads/master@{#434208}
(cherry picked from commit d796bf464ffe66b896c8def42b1f916f62da9928)

Review URL: https://codereview.chromium.org/2534563002 .

Cr-Commit-Position: refs/branch-heads/2883@{#662}
Cr-Branched-From: 614d31daee2f61b0180df403a8ad43f20b9f6dd7-refs/heads/master@{#423768}

[modify] https://crrev.com/fadbabf762310ae5d44221c37929dbf4807f5fe9/chrome/browser/content_settings/tab_specific_content_settings.cc

Tested this issue on Ubuntu 14.04 and Windows-10 using chrome latest M55-55.0.2883.70 by following steps mentioned in the original comment. Observed ‘Location’ icon does not stay in omnibox after clicking on 'Allow' in permission bubble.

clamy@ Could you please confirm is this issue is merged to latest M55? Attaching scree-cast for reference.

Thanks!

Deleted: 667256.mp4 2.1 MB

	667256.mp4 2.1 MB View Download

I see my change in the log when checking out M55-55.0.2883.70. Note that there were issues in the code prior to the patch: a failed navigation would make the icon disappear. This may be what's happening?

 clamy@ please correct me if I am wrong, I feel that the issue is not completely fixed. based on the statement and what I am seeing from Comment#16 verification. 

I have tested this on Windows 7 and 10 with Chrome version 55.0.2883.70 I see similar behavior but other thing what I observed that if I refresh the page I see the location icon in Omnibox. later if I try to change the location default to use global default Ask consecutive selects works fine i.e., I see the location icon in Omnibox.

Also want to check about "a failed navigation would make the icon disappear. This may be what's happening?" what does failed navigation means in this case is it the focus on the tab or on the bubble,sorry I am little unclear on that.

Reopening the bug based on comment #18.

I don't have a lot of insight here other than to confirm I can reproduce on 55.0.2883.70 on Windows too.

I cannot reproduce at head.

clamy's fix is in that version for sure: https://storage.googleapis.com/chromium-find-releases-static/d79.html#d796bf464ffe66b896c8def42b1f916f62da9928

A revert seems like the way to go here as bauerb suggested offline, but I'm not sure how easy/clean of a revert that will be now.

Still looking... 

I tried reverting d796bf464ffe66b896c8def42b1f916f62da9928 out of a local build and that does break it at head. So it's confusing why it does not help on 55.

Also fwiw, I don't see any failed navigation in devtools when any of this happens, but I'm not sure if that's capturing everything or not.

The best I can offer to do now is to try to build the 55 branch, but that will take a while to sync and build, so it's likely clamy or bauerb will be back online (and more knowledgeable) before I get much of anywhere in debugging.

I'll start that syncing now anyway...

+nasko@: did we change the meaning of NavigationHandle::IsSamePage in M56?

I remember that we had IsSamePage and IsSynchronous, and that there was a patch to merge those. I'm wondering if it's possible than when merging to M55 I should have been using the old function instead. That may explain why it's fixed on HEAD but not on M55.

Other than this function definition issue, I don't see why we would have such a discrepancy. If it's not explained by us not correctly classifying the fragment navigation as same-page in the NavigationHandle, then we should probably revert jam's patch on M55. I'll have a look at it tomorrow morning EMEA time if it wasn't done by then.

(I failed at my first attempt to get/build 55 using releases/55.0..., trying again with branches/2883/...)

The meaning of IsSamePage should not have changed, as it was more or less analogous to IsSynchronous. However, it my CL did touch this area, so it is quite possible to be related.

git find-releases 2e9de94b320e23d920de86114648cb5e851a2fd5
commit 2e9de94b320e23d920de86114648cb5e851a2fd5 was:
  initially in 56.0.2903.0
No merges found.

It does look like it isn't in M55, so it could be the root cause for why the patch behaves differently.

[Following the go/ChromeReleaseBranches instructions infuriatingly resulted in a failed gclient sync (twice, once using the release tag, and once using the branch). So I don't know how to build 55.]

After much confusion, mmoss has narrowed the 55 build down to some ACL problems that are being investigated at the moment.

Per other offline discussion, nasko felt that either merging nasko's CL or reverting jam@s seem reasonable approaches to getting 55 out the door, but we need to get 55 building in order to be able to be confident in either (be able to run browser_tests locally first, e.g.)

ACLs for 55 are fixed, and 55 is now syncable. I am still pulling and then will start a build, but probably won't get anywhere on investigating the actual fix now tonight.

I'm investigating today. If I can't get to a simple fix in the afternoon, I'll try to reverse John's patch.

Ok so the issue in M55 is that NavigationHandle::IsSamePage does not return the same value as on head. For example:
1) I am on https://www.google.fr/webhp?sourceid=chrome-instant&ion=1&espv=2&ie=UTF-8.
2) I navigate to https://www.google.fr/webhp?sourceid=chrome-instant&ion=1&espv=2&ie=UTF-8#q=darty. The NavigationHandle I instantiate has IsSamePage to false, but IsSynchronous to true.
3) I navigate again to https://www.google.fr/webhp?sourceid=chrome-instant&ion=1&espv=2&ie=UTF-8#q=darty&dlnr=1, and again IsSamePage returns false, but IsSynchronous returns true. This is the navigation which causes the location tracking icon to become unset.

I'll investigate more to see why this is happening, and if it was fixed by Nasko's patch or the changes I made to not delete Navigationhandles on synchronous navigations.

And the reason is very simple: at that time, we were not providing is_same_page as a parameter to the creation of the NavigationHandle, so the NavigationHandle doesn't know it is same-page when DidStartNavigation is fired (it happens during the constructor of the NavigationHandle). In fact, also filtering NavigationHandles with IsSynchronousNavigation set to true does solve the bug.

I'll upload a CL with that fix and a CL with the revert so that we can decide what to land.

In any case, I'd like to reiterate that the underlying code seems broken to me. Resetting this settings at the start of the navigation is wrong: we have absolutely no guarantee that it will be successful, and a 204/205 response or a download will leave you on the same page with settings cleared since there is no code to bring it back. Changes to the settings should be done at commit time, when we know for sure the page is being replaced.

Thanks, Camille! FWIW, I agree that the current behavior is broken. 

So the CL with the fix is at https://codereview.chromium.org/2536423003/ and the revert at https://codereview.chromium.org/2543613002/. Depending on what people prefer I may land either.

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

commit 32ddf24e3b852f14d2fc6d927060e5b9f5b2db31
Author: clamy <clamy@chromium.org>
Date: Wed Nov 30 14:57:50 2016

Fix issue with location tracking icon disappearing on M55

This CL is a followup to https://codereview.chromium.org/2534563002/
which merged https://codereview.chromium.org/2524053002/ into M55. The
CL on trunk fixed the issue on trunk, but was not enough on M55 beacuse
at that time we also need to filter out NavigationHandles with
IsSynchronousNavigation set to true. On M55,
NavigationHandle::IsSamePage was set later in time, so we can't use it
as we're doing on trunk.

BUG= 667256 

Publish DEPS for Chromium 55.0.2883.70

R=bauerb@chromium.org, nasko@chromium.org

Review URL: https://codereview.chromium.org/2536423003 .

Cr-Commit-Position: refs/branch-heads/2883@{#692}
Cr-Branched-From: 614d31daee2f61b0180df403a8ad43f20b9f6dd7-refs/heads/master@{#423768}

[modify] https://crrev.com/32ddf24e3b852f14d2fc6d927060e5b9f5b2db31/chrome/browser/content_settings/tab_specific_content_settings.cc

Thank you everyone.
Just triggered M55 build (55.0.2883.72) with this fix in. Prudhvi, please verify once M55 binaries are available. Thank you.

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

commit 57f99bd2aed3c3b5ea06f44ae7b8c42ff687481f
Author: clamy <clamy@chromium.org>
Date: Wed Nov 30 18:25:11 2016

Add comment about same-page navs to WCO::DidStart/FinishNavigation

This CL updates the description of
WebContentsObserver::DidStartNavigation and
WebContentsObserver::DidFinishNavigation to remind implementers that
they will also be called on same-page navigations. Note that this is the
main difference between WebContentsObserver::DidStartNavigation and
WebContentsObserver::DidStartProvisionalLoad in non-PlzNavigate
behavior.

BUG= 667256 

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

[modify] https://crrev.com/57f99bd2aed3c3b5ea06f44ae7b8c42ff687481f/content/public/browser/web_contents_observer.h

Verified the fix on Chrome version 55.0.2883.72 on Windows 7,10, Mac and Linux :

Test 1 :
Steps Followed :
1. Launch Chrome and visit "https://googlechrome.github.io/samples/permissions/" and  Click on [Geolocation]
2. Selected "allow" from pop-up bubble "googlechrome.github.io"

Expected/Observed behavior : 
Location icon should appear in Omnibox

Test2 :
Steps followed
1. Launch Chrome and visit "https://googlechrome.github.io/samples/permissions/" and  Click on [Geolocation]
2. Selected "allow" from pop-up bubble "googlechrome.github.io"
3. Location icon should appear in Omnibox
4. Open devtools-->js console and execute "window.history.pushState({}, ""); "
5. Still see the location icon in Omnibox
6. Refresh the tab location icon disappered
7. Clicking on geo location again 
8. There was no pop-up bubble but location icon should appeared in omnibox 
9. refresh the page location icon disappers

Expected/Observed :
On every reload the location icon should disapper, it only shows when the page asks for location

Verified the issue on Ubuntu 14.04, Windows-10 and Mac OS 10.12 using chrome latest M55-55.0.2883.75 by following steps mentioned in the comment #0 & #39. Observed the ‘Location’ icon stay in omnibox after clicking on 'Allow' in permission bubble as expected with out disappearing. Hence adding TE-Verified label.

The call to navigation_handle->IsSamePage() in TabSpecificContentSettings::DidStartNavigation, added in https://codereview.chromium.org/2534563002/, triggers the DCHECK in NavigationHandleImpl::IsSamePage, which is still there in 55 branch.
Why didn't you delete this call in https://codereview.chromium.org/2536423003/ ?

This is broken again on Version 56.0.2924.28 beta (64-bit) on Linux.

Steps to reproduce:
1. Launch Chrome and visit "https://googlechrome.github.io/samples/permissions/" and  Click on [Geolocation]
2. Selected "allow" from pop-up bubble "googlechrome.github.io"
3. Location icon should appear in Omnibox
4. Open devtools-->js console and execute "window.history.pushState({}, ""); "
5. Still see the location icon in Omnibox
6. Observe that the location icon is gone from the omnibox even though it should not.

This is fixed in Tip of Tree on Linux, so there is a CL that needs to be merged back.

The bisect points to 
You are probably looking for a change made after 434200 (known bad), but no later than 434208 (first known good).
CHANGELOG URL:  https://chromium.googlesource.com/chromium/src/+log/c1fa4049b9bf140f14ae31b2bd6bdac5b81df4ad..d796bf464ffe66b896c8def42b1f916f62da9928
which points to the CL from comment 38.

According to this https://storage.googleapis.com/chromium-find-releases-static/57f.html#57f99bd2aed3c3b5ea06f44ae7b8c42ff687481f the fix only went into M57.

I think this is a regression for which the fix needs to be merged back into M56.

[Automated comment] DEPS changes referenced in bugdroid comments, needs manual review.

Sorry, the CL fixing this is in comment #8. It looks like the change made it into M55 and M58. Assigning to clamy@.

And I mean to say "It looks like the change made it into M55 and M57."

[Automated comment] DEPS changes referenced in bugdroid comments, needs manual review.

Approved for merge into M56

Your change has been approved for M56. Beta RC cut is scheduled at 3.00 PM today 01/03, please merged the CL ASAP.

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

commit 663c7917c9b2c91ade7de77568508012d24f36ce
Author: Dominic Battre <battre@chromium.org>
Date: Wed Jan 04 11:49:31 2017

Don't update TabSpecificContentSettings on same-page navigation

This CL ensures that the TabSpecificContentSettings are not updated when
navigating same-page. This fixes an issue where the location tracking
icon would disappear when accepting to send location info to Google
search. This is because Google search would do a fragment navigation
when location tracking was accepted, which would fire DidStartNavigation
causing the location information to be reset. Previously,
DidStartProvisionalLoad would not be called for a fragment navigation,
so the icon would not be reset.

For the record, TabSpecificContentSettings was made to use DidStartNavigation
in https://codereview.chromium.org/2374443003.

BUG= 667256 

Review-Url: https://codereview.chromium.org/2524053002
Cr-Commit-Position: refs/heads/master@{#434208}
(cherry picked from commit d796bf464ffe66b896c8def42b1f916f62da9928)

Review URL: https://codereview.chromium.org/2534563002 .

Cr-Commit-Position: refs/branch-heads/2883@{#662}
Cr-Branched-From: 614d31daee2f61b0180df403a8ad43f20b9f6dd7-refs/heads/master@{#423768}
(cherry picked from commit fadbabf762310ae5d44221c37929dbf4807f5fe9)

Review-Url: https://codereview.chromium.org/2614673002 .
Cr-Commit-Position: refs/branch-heads/2924@{#664}
Cr-Branched-From: 3a87aecc31cd1ffe751dd72c04e5a96a1fc8108a-refs/heads/master@{#433059}

[modify] https://crrev.com/663c7917c9b2c91ade7de77568508012d24f36ce/chrome/browser/content_settings/tab_specific_content_settings.cc

I am not sure whether I made the RC cut and hope that there will be another one otherwise, because I think it is important that this gets fixed. Please ping me in case of time critical situations (3:00 PM was midnight here) and I saw your message only on the next day in central european time.

Verified the issue on Ubuntu 14.04, Windows-10 and Mac OS 10.12 using chrome latest M56-56.0.2924.59 by following steps mentioned in the comment #0 & #42. Observed the ‘Location’ icon stay in omnibox after clicking on 'Allow' in permission bubble as expected with out disappearing. Hence adding TE-Verified label.

Thanks!

Deleted: 667256.png 132 KB

	667256.png 132 KB View Download