Issue metadata
Sign in to add a comment
|
Omnibox freezes and shows incorrect url |
||||||||||||||||||||||
Issue descriptionApp 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
,
Dec 21
Seems like a fullscreen bug
,
Dec 21
,
Dec 26
,
Dec 28
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.
,
Jan 7
kkhorimoto: Any update here? This seems to be high visibility.
,
Jan 10
Will be looping back to this tomorrow.
,
Jan 11
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?
,
Jan 11
,
Jan 11
I can't repro using latest beta 72.0.3626.51.
,
Jan 12
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.
,
Jan 14
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
,
Jan 14
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
,
Jan 14
vbarigela: Looks like you can repro the issue with some consistency. Can you answer kkhorimoto's questions in comment 8 ?
,
Jan 14
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
,
Jan 14
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.
,
Jan 14
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).
,
Jan 15
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.
,
Jan 16
Okay, fix is here: crrev.com/c/1414330
,
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
,
Jan 16
(6 days ago)
,
Jan 16
(6 days ago)
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
,
Jan 17
(5 days ago)
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.
,
Jan 18
(4 days ago)
Approved. Please merge asap.
,
Jan 18
(4 days ago)
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 --
,
Jan 18
(4 days ago)
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}
,
Jan 18
(4 days ago)
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
,
Jan 18
(4 days ago)
,
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 |
|||||||||||||||||||||||
Comment 1 by pkl@chromium.org
, Dec 10Labels: -Pri-2 M-72 Pri-1
Owner: stkhapugin@chromium.org
Status: Assigned (was: Untriaged)