New issue
Advanced search Search tips

Issue 913322 link

Starred by 2 users

Issue metadata

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



Sign in to add a comment

Omnibox freezes and shows incorrect url

Project Member Reported by subhashi...@chromium.org, Dec 10

Issue description

App Version: 72.0.3626.8 beta
iOS Version: 12.1 beta 3
Devices: iPhone 8 Plus

Steps to reproduce:
1. Launch iOS Chrome
2. Navigate to any webpage ex. Facebook.com
3. Scroll the page up

Observed Result:
At Step 2: Shows a white or gray bar appears below omnibox
At Step 3: Omnibox get struck or freezes 
 
Note: 
1. Tapping on omnibox once it freezes,  tab closes.
2. Opening a new tab and navigating to any webpage  - Omnibox gets struck with incorrect url
3. Issue is not consistently reproducible. It is very intermittent. 
 
Expected Result:
Omnibox should not freeze

Number of times you were able to reproduce: 1/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 the current stable build :M71, No
Bug reproducible on the current beta channel build : M72, No

Type-bug-regression? Yes

Link to Image/Video:
https://drive.google.com/open?id=1qQrT4KdtvXxyx0CUH8wMU6ANvLpsDn8W


 
Cc: gambard@chromium.org justincohen@chromium.org
Labels: -Pri-2 M-72 Pri-1
Owner: stkhapugin@chromium.org
Status: Assigned (was: Untriaged)
This is new in M72.
Cc: stkhapugin@chromium.org
Components: UI>Browser>FullScreen
Owner: kkhorimoto@chromium.org
Seems like a fullscreen bug
Cc: kkhorimoto@chromium.org
 Issue 916001  has been merged into this issue.
Labels: ReleaseBlock-Stable
Labels: -Restrict-View-Google
Status: Started (was: Assigned)
Taking a look at this now.  Removing RVG since  Issue 916001  was merged into this and non-Google Chromium users are commenting on that bug.
kkhorimoto: Any update here? This seems to be high visibility.
Will be looping back to this tomorrow.
I was banging my head against this trying to get it to repro all day yesterday, but have had no luck.

A couple questions for subhashinik@:
- Can you do a bisect on when this started to occur?
- Does this still repro on M73?
- If it doesn't repro on M73, would it be possible to bisect when it was fixed?
- Does this only repro on 8 Plus iOS12 devices?
- Does this only repro from NTP->web navigations?
- Does the URL have to be typed, or do link navigations via bookmarks work?
- Does this occur with or without the smooth scrolling experiment enabled?
Cc: subhashi...@chromium.org
I can't repro using latest beta 72.0.3626.51.
I just ran into this issue on M73.0.3668.0 canary with iOS 12.1.2 GM, on iPhone7 plus device. I don't have any special flags enabled, but no consistent repro steps. So far was able to repro only once. I will post more steps if I am able to repro consistently.
Could able to reproduce on latest chrome beta version 72.0.3626.58

Steps to repro
1 Freshly install chrome and force quit the app.
2 Preload the URL
(With a very slight push send the omnibox to hidden state)
3.  The push should only hide the omnibox (the webpage should not scroll up)--> Release the finger off the screen with the push immediately
4.  Immediately before the omnibox is hidden completely, scroll it down with a very slight pull (so that only the omnibox is unhidden)
5.  When the URL is slightly grey, tap in the omnibox.

Note: Steps 3, 4, 5 should be performed with some pace.

Please check the videos:

Video 1: https://drive.google.com/file/d/1J-HyBlOZnn2ZfOU0dZAFd-7gQM82rvO1/view?usp=sharing

Video 2: https://drive.google.com/file/d/13p2qIMuaRtmLFdXY_VZhQq7CrWxLCxra/view?usp=sharing

From 2:35 mins
Video 3: https://drive.google.com/file/d/1zc8GSgBnP7j95QkFz0euilCqZvBve3Dx/view?usp=sharing
Could also able to reproduce the issue on latest chrome canary Version 73.0.3671.0.

Device: iPhone XR, iPhone 8 plus
iOS: 12.1.2, 12.1.3 beta 4

vbarigela: Looks like you can repro the issue with some consistency. Can you answer kkhorimoto's questions in comment 8 ?
I have few answers so far:
- Can you do a bisect on when this started to occur?
  We don't have concrete 5/5 steps to repro, so bisecting would be hard at this point. Still working on consistent repro steps.
- Does this still repro on M73?
  Yes, as of this mornings available M73 canary its still reproducible. 
- If it doesn't repro on M73, would it be possible to bisect when it was fixed? 
   N/A
- Does this only repro on 8 Plus iOS12 devices?
  Happens on various devicecs, iPhone7Plus, iPhoneX, XR as well.
- Does this only repro from NTP->web navigations?
  Mostly but no always. I saw this once tapping on a link to an article in the same webpage.
- Does the URL have to be typed, or do link navigations via bookmarks work?
  You can type the urls, or tap on MostVisited tiles or tap NTP content suggestions.
- Does this occur with or without the smooth scrolling experiment enabled?
  Haven't checked on this setting yet. It should be in default state.

From a device where I reproduced below is the list of variations and it doesn't show anything related to fullscreen experiments.
73ebcf26-ca7d8d80
b1dd1b0c-3f4a17df
16e0dd70-3f4a17df
2b6ab552-ca7d8d80
b7e2524c-1181467f
411c711e-3f4a17df
6025934e-3f4a17df
a582a1b8-ad75ce17
2a565533-f23d1dea
fd1d44dd-f23d1dea
68ccc549-f23d1dea
2d80427c-f23d1dea
587904d4-ca7d8d80
6c785e29-3f4a17df
fc57f990-f23d1dea
9b4c4257-ce5577ae
9504402-3f4a17df
f79cb77b-3d47f4f4
e7b45bb7-377be55a
fc7fb3f6-27616b7a
26464629-3f4a17df
51af0496-f23d1dea
51b9b54d-f23d1dea
d840bbac-1410f10
1fcbb124-d411cf1
494d8760-52325d43
3ac60855-486e2a9c
f296190c-cd4ab62e
4442aae2-6bdfffe7
ed1d377-e1cc0f14
75f0f0a0-a5822863
e2b18481-7564fb06
e7e71889-e1cc0f14
d0ed99bc-d0ed99bc
This is reproduced on M71 as well. M71.0.3578.89 stable.
- Does this occur with or without the smooth scrolling experiment enabled?
Looks like this only happening when smooth scrolling is enabled from chrome://flags.
Thanks for all the information!  From the videos and steps above, it looks like this isn't related to navigation.  It seems that this is happening when the fullscreen adjustment animation that occurs at the end of a scroll event is cancelled due to a new scroll.  It may be that the animator is not being cancelled properly.  I'm going to investigate if this is easier with a longer animation duration (might be easier to reproduce the timing that way).
Okay, made some progress here thanks to the input from testers.  I can repro this bug when I slow the animations down so that I can more easily time the repro steps.  This is happening when the scroll end adjustment animation to hide the toolbars is interrupted by an overscroll action.  When the overscroll action is kicked off, it sets its OverscrollState to STARTED_PULLING, adds the OverscrollActionsView to the toolbar (using a snapshot of the current toolbar), and creates a ScopedFullscreenDisabler.   When the disabler is created, we receive two |-scrollViewDidScroll:| events back-to-back:

1) A scroll event where the viewport insets are set to the current FullscreenAnimator value.  The animator is stopped when fullscreen is disabled.
2) A scroll event when the BVC responds to the FullscreenControllerObserver::FullscreenEnabledStateChanged() call and updates the viewport insets to the value corresponding with progress == 1.0.

These two |-scrollViewDidScroll:| events interact with each other such that the resulting OverscrollState is NO_PULL_STARTED and the OverscrollActionsView is removed from the superview.  This means that the logic after creating the disabler, which assumes that the state is still STARTED_PULLING, is incorrect.  Moreover, since |-setOverscrollState:| is idempotent, the OverscrollActionView is not cleaned up when the state is reset to NO_PULL_STARTED after the overscroll gesture.

If you open a new tab, a new OverscrollActionsController is created, which manages its own OverscrollActionsView.  However, since the same PrimaryToolbarView is shared between the tabs, the old action view with the snapshotted URL remains visible.

I tried a couple things out today, but they introduced some unintended animation side effects; I'll continue investigating and using my findings to create a fix tomorrow.
Okay, fix is here: crrev.com/c/1414330
Project Member

Comment 20 by bugdroid1@chromium.org, Jan 16 (6 days ago)

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

commit cdcad69f5500551ddf133b715a333ea82863d176
Author: Kurt Horimoto <kkhorimoto@chromium.org>
Date: Wed Jan 16 22:08:39 2019

[iOS] Fix stale OverscrollActionsView bug.

The referenced bug occured due to the interaction between the overscroll
bounce implementation and BVC's update of WKScrollView.contentInset when
fullscreen is disabled.  In certain timing conditions where a drag event
ends in the same runloop as fullscreen is disabled, the bounce state can
be set up such that the start and finish contentInset are the same.
This means that the |-scrollViewDidScroll| callback is never called, and
the OverscrollActionView is never removed because the overscrollState is
not reset to NO_PULL_STARTED.

This problem was exacerbated by the interaction between the scroll end
fullscreen adjustment animations and the overscrollState.  When a pull
event is started while the toolbars are being animated to the collaped
state, the overscrollState is updated as such:
1) NO_PULL_STARTED => STARTED_PULLING: happens when the scroll begins
2) STARTED_PULLING => NO_PULL_STARTED: happens when BVC updates the
   viewport insets as the result of the ScopedFullscreenDisabler being
   created for the overscroll event.

This means that the logic in |-setOverscrollState:| after creating the
disabler (where the OverscrollActionView is added) is incorrect since
it assumes that the state is still STARTED_PULLING.  Moreover, since
|-setOverscrollState:| is idempotent, setting the state back to
NO_PULL_STARTED does not successfully clean up the
OverscrollActionView.

This CL fixes this issue by:
- Adding |_ignoreScrollForDisabledFullscreen|, which allows us to
  ignore the STARTED_PULLING => NO_PULL_STARTED transition that
  occurs when disabling fullscreen.
- Explicitly calling |-clear| if the bounce animation doesn't have any
  work to do.  This allows the OverscrollActionView to be removed as
  overscrollState is set back to NO_PULL_STARTED.

Bug:  913322 
Change-Id: I11c0e4ad8d3725a41783c3ffbb6c49e5a4d9f4c9
Reviewed-on: https://chromium-review.googlesource.com/c/1414330
Commit-Queue: Kurt Horimoto <kkhorimoto@chromium.org>
Reviewed-by: Eugene But <eugenebut@chromium.org>
Reviewed-by: Gauthier Ambard <gambard@chromium.org>
Cr-Commit-Position: refs/heads/master@{#623391}
[modify] https://crrev.com/cdcad69f5500551ddf133b715a333ea82863d176/ios/chrome/browser/ui/overscroll_actions/overscroll_actions_controller.mm

Comment 21 by kkhorimoto@chromium.org, Jan 16 (6 days ago)

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

Comment 22 by sheriffbot@chromium.org, Jan 16 (6 days ago)

Labels: -Merge-Request-72 Merge-Review-72 Hotlist-Merge-Review
This bug requires manual review: Less than 9 days to go before AppStore submit on M72
Please contact the milestone owner if you have questions.
Owners: govind@(Android), kariahda@(iOS), djmm@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 23 by srikanthg@chromium.org, Jan 17 (5 days ago)

Status: Verified (was: Fixed)
Verified the fix on iPhoneX, iPhone7 plus
iOS: 12.1.2, 12.1.3
Build: M73.0.3674.0 canary

I will also let HYD team verify the fix once merged onto M72.

Comment 24 by kariahda@chromium.org, Jan 18 (4 days ago)

Approved. Please merge asap.
Project Member

Comment 25 by cr-audit...@appspot.gserviceaccount.com, Jan 18 (4 days ago)

Labels: CommitLog-Audit-Violation Merge-Without-Approval
Here's a summary of the rules that were executed: 
 - OnlyMergeApprovedChange: Rule Failed -- Revision 945d296fb405a5fe45bfd4612bd92491a1a05c52 was merged to refs/branch-heads/3626 branch with no merge approval from a TPM! 
Please explain why this change was merged to the branch!
 - AcknowledgeMerge: Notification Required -- 
Project Member

Comment 26 by cr-audit...@appspot.gserviceaccount.com, Jan 18 (4 days ago)

Labels: Merge-Merged-72-3626
The following revision refers to this bug: 
https://chromium.googlesource.com/chromium/src.git/+/945d296fb405a5fe45bfd4612bd92491a1a05c52

Commit: 945d296fb405a5fe45bfd4612bd92491a1a05c52
Author: kkhorimoto@chromium.org
Commiter: kkhorimoto@chromium.org
Date: 2019-01-18 19:36:54 +0000 UTC

[iOS] Fix stale OverscrollActionsView bug.

The referenced bug occured due to the interaction between the overscroll
bounce implementation and BVC's update of WKScrollView.contentInset when
fullscreen is disabled.  In certain timing conditions where a drag event
ends in the same runloop as fullscreen is disabled, the bounce state can
be set up such that the start and finish contentInset are the same.
This means that the |-scrollViewDidScroll| callback is never called, and
the OverscrollActionView is never removed because the overscrollState is
not reset to NO_PULL_STARTED.

This problem was exacerbated by the interaction between the scroll end
fullscreen adjustment animations and the overscrollState.  When a pull
event is started while the toolbars are being animated to the collaped
state, the overscrollState is updated as such:
1) NO_PULL_STARTED => STARTED_PULLING: happens when the scroll begins
2) STARTED_PULLING => NO_PULL_STARTED: happens when BVC updates the
   viewport insets as the result of the ScopedFullscreenDisabler being
   created for the overscroll event.

This means that the logic in |-setOverscrollState:| after creating the
disabler (where the OverscrollActionView is added) is incorrect since
it assumes that the state is still STARTED_PULLING.  Moreover, since
|-setOverscrollState:| is idempotent, setting the state back to
NO_PULL_STARTED does not successfully clean up the
OverscrollActionView.

This CL fixes this issue by:
- Adding |_ignoreScrollForDisabledFullscreen|, which allows us to
  ignore the STARTED_PULLING => NO_PULL_STARTED transition that
  occurs when disabling fullscreen.
- Explicitly calling |-clear| if the bounce animation doesn't have any
  work to do.  This allows the OverscrollActionView to be removed as
  overscrollState is set back to NO_PULL_STARTED.

Bug:  913322 
Change-Id: I11c0e4ad8d3725a41783c3ffbb6c49e5a4d9f4c9
Reviewed-on: https://chromium-review.googlesource.com/c/1414330
Commit-Queue: Kurt Horimoto <kkhorimoto@chromium.org>
Reviewed-by: Eugene But <eugenebut@chromium.org>
Reviewed-by: Gauthier Ambard <gambard@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#623391}(cherry picked from commit cdcad69f5500551ddf133b715a333ea82863d176)
Reviewed-on: https://chromium-review.googlesource.com/c/1423017
Reviewed-by: Kurt Horimoto <kkhorimoto@chromium.org>
Cr-Commit-Position: refs/branch-heads/3626@{#731}
Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}
Project Member

Comment 27 by bugdroid1@chromium.org, Jan 18 (4 days ago)

Labels: merge-merged-3626
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/945d296fb405a5fe45bfd4612bd92491a1a05c52

commit 945d296fb405a5fe45bfd4612bd92491a1a05c52
Author: Kurt Horimoto <kkhorimoto@chromium.org>
Date: Fri Jan 18 19:36:54 2019

[iOS] Fix stale OverscrollActionsView bug.

The referenced bug occured due to the interaction between the overscroll
bounce implementation and BVC's update of WKScrollView.contentInset when
fullscreen is disabled.  In certain timing conditions where a drag event
ends in the same runloop as fullscreen is disabled, the bounce state can
be set up such that the start and finish contentInset are the same.
This means that the |-scrollViewDidScroll| callback is never called, and
the OverscrollActionView is never removed because the overscrollState is
not reset to NO_PULL_STARTED.

This problem was exacerbated by the interaction between the scroll end
fullscreen adjustment animations and the overscrollState.  When a pull
event is started while the toolbars are being animated to the collaped
state, the overscrollState is updated as such:
1) NO_PULL_STARTED => STARTED_PULLING: happens when the scroll begins
2) STARTED_PULLING => NO_PULL_STARTED: happens when BVC updates the
   viewport insets as the result of the ScopedFullscreenDisabler being
   created for the overscroll event.

This means that the logic in |-setOverscrollState:| after creating the
disabler (where the OverscrollActionView is added) is incorrect since
it assumes that the state is still STARTED_PULLING.  Moreover, since
|-setOverscrollState:| is idempotent, setting the state back to
NO_PULL_STARTED does not successfully clean up the
OverscrollActionView.

This CL fixes this issue by:
- Adding |_ignoreScrollForDisabledFullscreen|, which allows us to
  ignore the STARTED_PULLING => NO_PULL_STARTED transition that
  occurs when disabling fullscreen.
- Explicitly calling |-clear| if the bounce animation doesn't have any
  work to do.  This allows the OverscrollActionView to be removed as
  overscrollState is set back to NO_PULL_STARTED.

Bug:  913322 
Change-Id: I11c0e4ad8d3725a41783c3ffbb6c49e5a4d9f4c9
Reviewed-on: https://chromium-review.googlesource.com/c/1414330
Commit-Queue: Kurt Horimoto <kkhorimoto@chromium.org>
Reviewed-by: Eugene But <eugenebut@chromium.org>
Reviewed-by: Gauthier Ambard <gambard@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#623391}(cherry picked from commit cdcad69f5500551ddf133b715a333ea82863d176)
Reviewed-on: https://chromium-review.googlesource.com/c/1423017
Reviewed-by: Kurt Horimoto <kkhorimoto@chromium.org>
Cr-Commit-Position: refs/branch-heads/3626@{#731}
Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}
[modify] https://crrev.com/945d296fb405a5fe45bfd4612bd92491a1a05c52/ios/chrome/browser/ui/overscroll_actions/overscroll_actions_controller.mm

Comment 28 by kariahda@chromium.org, Jan 18 (4 days ago)

Labels: -Hotlist-Merge-Review -Merge-Without-Approval -Merge-Review-72

Comment 29 by subhashi...@chromium.org, Yesterday (46 hours ago)

Verified in:

App Version: 72.0.3626.69 beta
iOS Versions: 12.1.2, 12.1.3 beta 4
Devices: iPhone XR, iPhone 8 Plus

Omnibox is not freezing on sending the omnibox to hidden state with a slight push. 

Sign in to add a comment