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

Issue 688047 link

Starred by 0 users

Issue metadata

Status: Verified
Owner:
Closed: Feb 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 1
Type: Bug-Regression



Sign in to add a comment

MostVisited icon title is incorrectly updated

Project Member Reported by srikanthg@chromium.org, Feb 2 2017

Issue description

App Version: M57.0.2987.20 beta
iOS Version: 10.3, 9.3.5
Device: iPhone7, iPad mini
URL: na

Steps to reproduce:
  1. Launch Google Chrome Beta
  2. (Make sure you don't have any tiles in NTP, Clear browsing data if required)
  3. Visit apple.com 
  4. Open a new new tab and make sure one tile is updated for Apple
  5. Goto back to previous tab, and navigate to bbc.com
  6. Once the page loaded, tap on back arrow twice to reach NTP

Observed results: Apple tile title is displayed as bbc.com, also a new tile created for bbc.com

Expected results: Apple tile title shouldn't be updated.

Number of times you were able to reproduce: 5/5
Bug reproducible after clean install: Yes
Bug reproducible after clearing cache and cookies: Yes
Bug reproducible on Chrome Mobile on Android: NA
Bug reproducible on Safari/Firefox: Firefox: NA, Safari: NA
Bug reproducible on current stable build (App Version, iOS Version): M56 NO
Bug reproducible on the current beta channel build (App Version, iOS Version): M57 Yes

Link to video/image: https://drive.google.com/file/d/0B-xmXLQhjeKucXRvTWFlclNVZXM/view 

 

Comment 1 by treib@chromium.org, Feb 2 2017

Cc: mastiz@chromium.org sfiera@chromium.org
This seems a really bad regression! I had a quick attempt with Clank dev (M57.0.2987.19) and can't repro, although it seems unlikely that it's iOS-specific.

How does History show up in this scenario? Is the mismatch visible there too?


I was able to repro on iOS near HEAD (built Wed at 522533e188). Assuming apple.com for the first site visited and bbc.com for the second, then what I get is

Tile 1
URL http://www.apple.com/ (correct)
Title www.bbc.com (wrong)
Icon None (??? [1])

Tile 2
URL http://www.bbc.com/ (correct)
Title BBC (correct)
Icon BBC icon (correct)

I also tried this with the fully-qualified URLS http://www.apple.com/ and http://www.bbc.com/ (to verify that this is not a redirect bug). This bug still happens in this case, so it's different from crbug/670271 (but may still be related).

I also tried with the following:
1. In tab #1, go to www.apple.com, then www.bbc.com
2. Open tab #2, go to www.chromium.org
3. Back in tab #1, go back
4. Go to new tab
This still reproduces, with the problem still observable in history.

The fact that this shows up in history, and doesn't seem to be reproducible on Android, suggests to me that the bug resides in Chrome iOS's history implementation, which TopSites then passes on to the NTP (I was able to repro with popular sites enabled—the bug seems to be limited to TopSites specifically)


[1] Weirdly, I tried both apple.com and amazon.com for tile #1 but didn't get apple-touch-icons. I *know* both of those sites have them. History shows an icon for apple.com but I think it's the favicon; hard to tell, but I think it's lower-resolution than the BBC and Chromium icons shown next to it in history.
I see a few patches by kkhorimoto@ which could be related. Perhaps try reverting be48752128d7ec4a5bd6d980157deb60dbb29548? I myself don't have a working setup for iOS development.

More likely, we'll need to bisect.
Cc: eugene...@chromium.org
Based on your observations in comment#4, this seems tobe a problem with PendingIndex navigation.
Issue reproduces with Pending Index Navigation is enabled and doesn't repro when disabling PendingIndex navigation.
cc'ing eugenebut@ owner of this feature.

Labels: ReleaseBlock-Stable M-57
Owner: kkhorimoto@chromium.org
Status: Assigned (was: Untriaged)
Cc: -eugene...@chromium.org
Labels: -Pri-2 Pri-1
Owner: eugene...@chromium.org
Status: Started (was: Assigned)
Project Member

Comment 10 by bugdroid1@chromium.org, Feb 7 2017

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

commit 9fc98ecff33175b1bc234f9c6c3f42c07285dc1f
Author: eugenebut <eugenebut@chromium.org>
Date: Tue Feb 07 22:53:33 2017

Extended HistoryUITestCase to test titles.

Also added disabled testHistoryUpdateAfterBackNavigation as a test case
for  crbug.com/688047  and cleaned up WebViewContainingText matcher usage.

BUG= 688047 

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

[modify] https://crrev.com/9fc98ecff33175b1bc234f9c6c3f42c07285dc1f/ios/chrome/browser/ui/history/history_ui_egtest.mm

Project Member

Comment 12 by bugdroid1@chromium.org, Feb 10 2017

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

commit 69378e0417fa4dbcae4dc863e5b46b68d71ac0be
Author: stkhapugin <stkhapugin@chromium.org>
Date: Fri Feb 10 12:10:09 2017

Revert of Fixed title updating for back forward navigation. (patchset #3 id:40001 of https://codereview.chromium.org/2685803002/ )

Reason for revert:
Breaks -[TabHistoryPopupControllerTestCase testTabHistoryMenu] tab_history_popup_controller_egtest.mm downstream, because New Tab in history popup (when long pressed on back) displays "chrome://newtab" instead of "New Tab".

I actually don't know why this test is not ran upstream, but I have to revert, sorry!

Original issue's description:
> Fixed title updating for back forward navigation.
>
> Chrome relies on KVO compliant "title" property to subscribe for
> title updates. If this KVO change happens during then navigation
> it is unclear if title was changes for the previous page, or the
> navigation was committed and title was changed for the new page.
>
> So if there is a navigation in progress WebController should ignore
> KVO change, but it should always attempt to update title when
> navigation is committed.
>
> BUG= 688047 ,  677356 
>
> Review-Url: https://codereview.chromium.org/2685803002
> Cr-Commit-Position: refs/heads/master@{#449527}
> Committed: https://chromium.googlesource.com/chromium/src/+/487e4cfd53bac5944dad49cbce3b939a0c7bf035

TBR=kkhorimoto@chromium.org,eugenebut@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 688047 ,  677356 

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

[modify] https://crrev.com/69378e0417fa4dbcae4dc863e5b46b68d71ac0be/ios/chrome/browser/tabs/tab.mm
[modify] https://crrev.com/69378e0417fa4dbcae4dc863e5b46b68d71ac0be/ios/chrome/browser/tabs/tab_unittest.mm
[modify] https://crrev.com/69378e0417fa4dbcae4dc863e5b46b68d71ac0be/ios/web/web_state/ui/crw_web_controller.mm
[modify] https://crrev.com/69378e0417fa4dbcae4dc863e5b46b68d71ac0be/ios/web/web_state/ui/crw_web_controller_unittest.mm
[modify] https://crrev.com/69378e0417fa4dbcae4dc863e5b46b68d71ac0be/ios/web/web_state/ui/crw_wk_navigation_states.h
[modify] https://crrev.com/69378e0417fa4dbcae4dc863e5b46b68d71ac0be/ios/web/web_state/ui/crw_wk_navigation_states.mm
[modify] https://crrev.com/69378e0417fa4dbcae4dc863e5b46b68d71ac0be/ios/web/web_state/ui/crw_wk_navigation_states_unittest.mm

Project Member

Comment 13 by bugdroid1@chromium.org, Feb 11 2017

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

commit adb03a4e58c33d0adc812a2ddae2180abe8ac148
Author: eugenebut <eugenebut@chromium.org>
Date: Sat Feb 11 02:03:52 2017

Relanding "Fixed title updating for back forward navigation."

Chrome relies on KVO compliant "title" property to subscribe for
title updates. If this KVO change happens during then navigation
it is unclear if title was changes for the previous page, or the
navigation was committed and title was changed for the new page.

So if there is a navigation in progress WebController should ignore
KVO change, but it should always attempt to update title when
navigation is committed.

BUG= 688047 ,  677356 

Review-Url: https://codereview.chromium.org/2685803002
Cr-Commit-Position: refs/heads/master@{#449527}
(cherry picked from commit 487e4cfd53bac5944dad49cbce3b939a0c7bf035)

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

[modify] https://crrev.com/adb03a4e58c33d0adc812a2ddae2180abe8ac148/ios/chrome/browser/tabs/tab.mm
[modify] https://crrev.com/adb03a4e58c33d0adc812a2ddae2180abe8ac148/ios/chrome/browser/tabs/tab_unittest.mm
[modify] https://crrev.com/adb03a4e58c33d0adc812a2ddae2180abe8ac148/ios/web/web_state/ui/crw_web_controller.mm
[modify] https://crrev.com/adb03a4e58c33d0adc812a2ddae2180abe8ac148/ios/web/web_state/ui/crw_web_controller_unittest.mm
[modify] https://crrev.com/adb03a4e58c33d0adc812a2ddae2180abe8ac148/ios/web/web_state/ui/crw_wk_navigation_states.h
[modify] https://crrev.com/adb03a4e58c33d0adc812a2ddae2180abe8ac148/ios/web/web_state/ui/crw_wk_navigation_states.mm
[modify] https://crrev.com/adb03a4e58c33d0adc812a2ddae2180abe8ac148/ios/web/web_state/ui/crw_wk_navigation_states_unittest.mm

Project Member

Comment 14 by bugdroid1@chromium.org, Feb 12 2017

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

commit 6cc15b2c2eb82ac455cd7dbfe912be9914c1c03d
Author: eugenebut <eugenebut@chromium.org>
Date: Sun Feb 12 02:04:08 2017

Revert of Relanding "Fixed title updating for back forward navigation." (patchset #3 id:40001 of https://codereview.chromium.org/2691693002/ )

Reason for revert:
Breaks some other tests this time:
https://uberchromegw.corp.google.com/i/internal.bling.main/builders/iphone9-simulator-x64/builds/9939

Original issue's description:
> Relanding "Fixed title updating for back forward navigation."
>
> Chrome relies on KVO compliant "title" property to subscribe for
> title updates. If this KVO change happens during then navigation
> it is unclear if title was changes for the previous page, or the
> navigation was committed and title was changed for the new page.
>
> So if there is a navigation in progress WebController should ignore
> KVO change, but it should always attempt to update title when
> navigation is committed.
>
> BUG= 688047 ,  677356 
>
> Review-Url: https://codereview.chromium.org/2685803002
> Cr-Commit-Position: refs/heads/master@{#449527}
> (cherry picked from commit 487e4cfd53bac5944dad49cbce3b939a0c7bf035)
>
> Review-Url: https://codereview.chromium.org/2691693002
> Cr-Commit-Position: refs/heads/master@{#449828}
> Committed: https://chromium.googlesource.com/chromium/src/+/adb03a4e58c33d0adc812a2ddae2180abe8ac148

TBR=kkhorimoto@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 688047 ,  677356 

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

[modify] https://crrev.com/6cc15b2c2eb82ac455cd7dbfe912be9914c1c03d/ios/chrome/browser/tabs/tab.mm
[modify] https://crrev.com/6cc15b2c2eb82ac455cd7dbfe912be9914c1c03d/ios/chrome/browser/tabs/tab_unittest.mm
[modify] https://crrev.com/6cc15b2c2eb82ac455cd7dbfe912be9914c1c03d/ios/web/web_state/ui/crw_web_controller.mm
[modify] https://crrev.com/6cc15b2c2eb82ac455cd7dbfe912be9914c1c03d/ios/web/web_state/ui/crw_web_controller_unittest.mm
[modify] https://crrev.com/6cc15b2c2eb82ac455cd7dbfe912be9914c1c03d/ios/web/web_state/ui/crw_wk_navigation_states.h
[modify] https://crrev.com/6cc15b2c2eb82ac455cd7dbfe912be9914c1c03d/ios/web/web_state/ui/crw_wk_navigation_states.mm
[modify] https://crrev.com/6cc15b2c2eb82ac455cd7dbfe912be9914c1c03d/ios/web/web_state/ui/crw_wk_navigation_states_unittest.mm

Project Member

Comment 15 by bugdroid1@chromium.org, Feb 12 2017

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

commit 176138f21cb2df89eb6157bb748b60207038eff4
Author: eugenebut <eugenebut@chromium.org>
Date: Sun Feb 12 04:14:48 2017

Reland of landing "Fixed title updating for back forward navigation." (patchset #1 id:1 of https://codereview.chromium.org/2689083002/ )

Reason for revert:
This revert did not help. Reverting the revert.

Original issue's description:
> Revert of Relanding "Fixed title updating for back forward navigation." (patchset #3 id:40001 of https://codereview.chromium.org/2691693002/ )
>
> Reason for revert:
> Breaks some other tests this time:
> https://uberchromegw.corp.google.com/i/internal.bling.main/builders/iphone9-simulator-x64/builds/9939
>
> Original issue's description:
> > Relanding "Fixed title updating for back forward navigation."
> >
> > Chrome relies on KVO compliant "title" property to subscribe for
> > title updates. If this KVO change happens during then navigation
> > it is unclear if title was changes for the previous page, or the
> > navigation was committed and title was changed for the new page.
> >
> > So if there is a navigation in progress WebController should ignore
> > KVO change, but it should always attempt to update title when
> > navigation is committed.
> >
> > BUG= 688047 ,  677356 
> >
> > Review-Url: https://codereview.chromium.org/2685803002
> > Cr-Commit-Position: refs/heads/master@{#449527}
> > (cherry picked from commit 487e4cfd53bac5944dad49cbce3b939a0c7bf035)
> >
> > Review-Url: https://codereview.chromium.org/2691693002
> > Cr-Commit-Position: refs/heads/master@{#449828}
> > Committed: https://chromium.googlesource.com/chromium/src/+/adb03a4e58c33d0adc812a2ddae2180abe8ac148
>
> TBR=kkhorimoto@chromium.org
> # Skipping CQ checks because original CL landed less than 1 days ago.
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG= 688047 ,  677356 
>
> Review-Url: https://codereview.chromium.org/2689083002
> Cr-Commit-Position: refs/heads/master@{#449881}
> Committed: https://chromium.googlesource.com/chromium/src/+/6cc15b2c2eb82ac455cd7dbfe912be9914c1c03d

TBR=kkhorimoto@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 688047 ,  677356 

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

[modify] https://crrev.com/176138f21cb2df89eb6157bb748b60207038eff4/ios/chrome/browser/tabs/tab.mm
[modify] https://crrev.com/176138f21cb2df89eb6157bb748b60207038eff4/ios/chrome/browser/tabs/tab_unittest.mm
[modify] https://crrev.com/176138f21cb2df89eb6157bb748b60207038eff4/ios/web/web_state/ui/crw_web_controller.mm
[modify] https://crrev.com/176138f21cb2df89eb6157bb748b60207038eff4/ios/web/web_state/ui/crw_web_controller_unittest.mm
[modify] https://crrev.com/176138f21cb2df89eb6157bb748b60207038eff4/ios/web/web_state/ui/crw_wk_navigation_states.h
[modify] https://crrev.com/176138f21cb2df89eb6157bb748b60207038eff4/ios/web/web_state/ui/crw_wk_navigation_states.mm
[modify] https://crrev.com/176138f21cb2df89eb6157bb748b60207038eff4/ios/web/web_state/ui/crw_wk_navigation_states_unittest.mm

Project Member

Comment 16 by bugdroid1@chromium.org, Feb 14 2017

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

commit 3e0da344ce088f22ac29af6084d5fb7b83f3624f
Author: eugenebut <eugenebut@chromium.org>
Date: Tue Feb 14 01:30:51 2017

Enabled testHistoryUpdateAfterBackNavigation.

BUG= 688047 

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

[modify] https://crrev.com/3e0da344ce088f22ac29af6084d5fb7b83f3624f/ios/chrome/browser/ui/history/history_ui_egtest.mm

Labels: Merge-Request-57
Status: Fixed (was: Started)
Project Member

Comment 18 by sheriffbot@chromium.org, Feb 14 2017

Labels: -Merge-Request-57 Hotlist-Merge-Review Merge-Review-57
This bug requires manual review: Less than 2 weeks to go before AppStore submit on M57
Please contact the milestone owner if you have questions.
Owners: amineer@(clank), cmasso@(bling), ketakid@(cros), govind@(desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Verified on chrome canary version 58.0.3012.0 on iPad4 10.2.1,iPhone 7plus 10.2.1, following the comments mentioned in comment 0 observed that the most visited icon title is displayed correctly.
Status: Verified (was: Fixed)
Labels: -Hotlist-Merge-Review -Merge-Review-57 Merge-Approved-57
Project Member

Comment 22 by bugdroid1@chromium.org, Feb 15 2017

Labels: -merge-approved-57 merge-merged-2987
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/55217cf9efff8866160932c1614077d4ed43a2d3

commit 55217cf9efff8866160932c1614077d4ed43a2d3
Author: Eugene But <eugenebut@google.com>
Date: Wed Feb 15 16:30:32 2017

Fixed title updating for back forward navigation.

Chrome relies on KVO compliant "title" property to subscribe for
title updates. If this KVO change happens during then navigation
it is unclear if title was changes for the previous page, or the
navigation was committed and title was changed for the new page.

So if there is a navigation in progress WebController should ignore
KVO change, but it should always attempt to update title when
navigation is committed.

BUG= 688047 ,  677356 

Review-Url: https://codereview.chromium.org/2685803002
Cr-Commit-Position: refs/heads/master@{#449527}
(cherry picked from commit 487e4cfd53bac5944dad49cbce3b939a0c7bf035)

Review-Url: https://codereview.chromium.org/2701463002 .
Cr-Commit-Position: refs/branch-heads/2987@{#521}
Cr-Branched-From: ad51088c0e8776e8dcd963dbe752c4035ba6dab6-refs/heads/master@{#444943}

[modify] https://crrev.com/55217cf9efff8866160932c1614077d4ed43a2d3/ios/chrome/browser/tabs/tab.mm
[modify] https://crrev.com/55217cf9efff8866160932c1614077d4ed43a2d3/ios/chrome/browser/tabs/tab_unittest.mm
[modify] https://crrev.com/55217cf9efff8866160932c1614077d4ed43a2d3/ios/web/web_state/ui/crw_web_controller.mm
[modify] https://crrev.com/55217cf9efff8866160932c1614077d4ed43a2d3/ios/web/web_state/ui/crw_web_controller_unittest.mm
[modify] https://crrev.com/55217cf9efff8866160932c1614077d4ed43a2d3/ios/web/web_state/ui/crw_wk_navigation_states.h
[modify] https://crrev.com/55217cf9efff8866160932c1614077d4ed43a2d3/ios/web/web_state/ui/crw_wk_navigation_states.mm
[modify] https://crrev.com/55217cf9efff8866160932c1614077d4ed43a2d3/ios/web/web_state/ui/crw_wk_navigation_states_unittest.mm

Verified the issue on the build 57.0.2987.72beta tested on iPhone7+(iOS 10).
Most visited icons are updated correctly works fine.
Project Member

Comment 24 by bugdroid1@chromium.org, Feb 23 2017

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

commit 77569373deed80a50d4787cd594f40953d1596a0
Author: eugenebut <eugenebut@chromium.org>
Date: Thu Feb 23 19:54:34 2017

Cherry-picked remaining parts of 'Fixed title updating for back forward navigation.'

Cherry-picked patchset #1 and #2 from cl/2691693002

BUG= 688047 ,  677356 
Review-Url: https://codereview.chromium.org/2685803002
NOTRY=true
NOPRESUBMIT=true

Review-Url: https://codereview.chromium.org/2711123004
Cr-Commit-Position: refs/branch-heads/2987@{#666}
Cr-Branched-From: ad51088c0e8776e8dcd963dbe752c4035ba6dab6-refs/heads/master@{#444943}

[modify] https://crrev.com/77569373deed80a50d4787cd594f40953d1596a0/ios/web/web_state/ui/crw_web_controller.mm

Verified on chrome beta version 57.0.2987.85 on iPhone 7 and iPad4 with iOS 10.2.1 following the steps mentioned in comment #0. Most visited icons are displayed correctly.

Sign in to add a comment