App freezes on opening a new tab in clear browsing data pop in recent tabs. |
|||||||||||||||||
Issue descriptionApp Version: 70.0.3501.0 Canary iOS Version: iOS 11.4,12 Device: iPhone Pre-Condition: 1- Have some data stored in history. Steps to reproduce: 1. Launch Chrome 2. Tap on Tab Switcher. 3. Scroll to Recent tabs. 4. Tap on Show Full history 5. Tap on Clear browsing data > Tap on Clear browsing data 6. Send the app to background. 7. Launch chrome from Todays view or 3D force touch. 8. Tap on Done Observed results: App freezes user gets stucks in History screen Expected results: App should not freeze 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 Dolphin/Safari/Firefox: Safari : NA Bug reproducible on the current beta channel build : Yes in M69 Bug reproducible on the current beta channel build : Yes in M70 Link to video/image: https://drive.google.com/file/d/17Ux9TiEkpNVpRT5Bgl2jYkitf3a7VNYB/view?usp=sharing
,
Jul 25
,
Jul 25
,
Jul 25
@3 Once you've investigated a bit, could you please add a short summary of the issue to this bug? What's going wrong that's causing the app to freeze? How does the Settings CBD ui avoid this same issue?
,
Jul 26
This bug only surfaces from tabgrid. From BVC, history and CBD are dismissed effectively by calling `[self dismissViewControllerAnimated:NO completion:nil];`, which dismisses all child view controllers. From Tabgrid, HistoryCoordinator is just told to stop. So the crux of the issue is HistoryCoordinator calling `[self.historyNavigationController dismissViewControllerAnimated:YES completion:completionHandler];`, which only dismisses CBD since it is the only child view controller. Still trying to figure out why that results in History freezing.
,
Jul 26
,
Jul 27
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6b08da93c186a16cdf6e2bf59044af69c96c7d94 commit 6b08da93c186a16cdf6e2bf59044af69c96c7d94 Author: Chris Lu <thegreenfrog@chromium.org> Date: Fri Jul 27 05:23:52 2018 [ios] Dismiss Clear Browsing Data before Dismissing History Navigation Controller If Clear Browsing Data is presented when History attempts to dismiss itself, nothing will happen. Need to dismiss Clear Browsing Data First. Video: https://drive.google.com/open?id=1KUKsgv8OSQc_1301ff2XMgK2_hi6dd1- Bug: 867329 Change-Id: I1cd5f242ef410267ed5d4ab0e006884a3288eafe Reviewed-on: https://chromium-review.googlesource.com/1150620 Commit-Queue: Chris Lu <thegreenfrog@chromium.org> Reviewed-by: Sergio Collazos <sczs@chromium.org> Cr-Commit-Position: refs/heads/master@{#578544} [modify] https://crrev.com/6b08da93c186a16cdf6e2bf59044af69c96c7d94/ios/chrome/browser/ui/history/history_clear_browsing_data_coordinator.h [modify] https://crrev.com/6b08da93c186a16cdf6e2bf59044af69c96c7d94/ios/chrome/browser/ui/history/history_clear_browsing_data_coordinator.mm [modify] https://crrev.com/6b08da93c186a16cdf6e2bf59044af69c96c7d94/ios/chrome/browser/ui/history/history_coordinator.mm
,
Jul 27
,
Jul 27
,
Jul 28
Your change meets the bar and is auto-approved for M69. Please go ahead and merge the CL to branch 3497 manually. Please contact milestone owner if you have questions. Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jul 30
Let's get canary verification please.
,
Jul 30
Then this is approved to merge.
,
Jul 30
Canary verified on 7/30
,
Jul 30
Approved to merge!
,
Jul 30
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7b31a9fcbe95e1e4e215496bad858be20771af10 commit 7b31a9fcbe95e1e4e215496bad858be20771af10 Author: sczs <sczs@chromium.org> Date: Mon Jul 30 18:40:48 2018 [ios] Dismiss Clear Browsing Data before Dismissing History Navigation Controller If Clear Browsing Data is presented when History attempts to dismiss itself, nothing will happen. Need to dismiss Clear Browsing Data First. Video: https://drive.google.com/open?id=1KUKsgv8OSQc_1301ff2XMgK2_hi6dd1- Bug: 867329 TBR=thegreenfrog@chromium.org (cherry picked from commit 6b08da93c186a16cdf6e2bf59044af69c96c7d94) Change-Id: I1cd5f242ef410267ed5d4ab0e006884a3288eafe Reviewed-on: https://chromium-review.googlesource.com/1150620 Commit-Queue: Chris Lu <thegreenfrog@chromium.org> Reviewed-by: Sergio Collazos <sczs@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#578544} Reviewed-on: https://chromium-review.googlesource.com/1155384 Cr-Commit-Position: refs/branch-heads/3497@{#224} Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753} [modify] https://crrev.com/7b31a9fcbe95e1e4e215496bad858be20771af10/ios/chrome/browser/ui/history/history_clear_browsing_data_coordinator.h [modify] https://crrev.com/7b31a9fcbe95e1e4e215496bad858be20771af10/ios/chrome/browser/ui/history/history_clear_browsing_data_coordinator.mm [modify] https://crrev.com/7b31a9fcbe95e1e4e215496bad858be20771af10/ios/chrome/browser/ui/history/history_coordinator.mm
,
Jul 31
This has been merged. Removing merge TBD label.
,
Jul 31
Verified on Canary 70.0.3508.0 on iPhone X iOS 11.4 . This bug is still reproducible.
,
Aug 1
Ah yes! Sorry I misread the bug and only half-fixed it for when we have ClearBrowsingData open, not the dialog also :)
,
Aug 2
To further elaborate on the issue, two logic paths are triggered for open tabs action, in the order in which they are executed:
- (void)application:(UIApplication*)application
performActionForShortcutItem:(UIApplicationShortcutItem*)shortcutItem
completionHandler:(void (^)(BOOL succeeded))completionHandler
- (void)applicationDidBecomeActive:(UIApplication*)application
This became an issue because in the first round, historyClearBrowsingDataCoordinator is nulled before it completely finishes dismissing all child view controllers above history. Therefore, the second round immediately calls the callback block (which happens before the 1st round can get to the callback), which does this:
[self.historyNavigationController dismissViewControllerAnimated:YES
completion:completionHandler];
self.historyNavigationController = nil;
since there are still child view controllers, -dismissViewControllerAnimated does not dismiss History. Thus, we're still left with History and the second time around the navController has been nulled, so no dismiss happens.
,
Aug 2
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/450b0990fb9685ed1c2df8c941904ce909d82f6e commit 450b0990fb9685ed1c2df8c941904ce909d82f6e Author: Chris Lu <thegreenfrog@chromium.org> Date: Thu Aug 02 22:18:27 2018 [ios] Fix Unwinding of History and ClearBrowsingData when Clear Browsing Data dialog is open - stops all action sheet coordinators in ClearBrowsingDataTableViewController before dismissing itself. - sets historyClearBrowsingDataCoordinator to nil after finishing unwinding ClearBrowsingData and History to prevent race condition. Video: https://drive.google.com/open?id=1wDymlQtfStNWBY9mI9GYqlig8qkipBDF Bug: 867329 Change-Id: I7c0825b02ed7d3c6122f0a5ae37c99181fc6d182 Reviewed-on: https://chromium-review.googlesource.com/1159683 Commit-Queue: Chris Lu <thegreenfrog@chromium.org> Reviewed-by: Sergio Collazos <sczs@chromium.org> Cr-Commit-Position: refs/heads/master@{#580348} [modify] https://crrev.com/450b0990fb9685ed1c2df8c941904ce909d82f6e/ios/chrome/browser/ui/history/history_clear_browsing_data_coordinator.mm [modify] https://crrev.com/450b0990fb9685ed1c2df8c941904ce909d82f6e/ios/chrome/browser/ui/history/history_coordinator.mm [modify] https://crrev.com/450b0990fb9685ed1c2df8c941904ce909d82f6e/ios/chrome/browser/ui/settings/clear_browsing_data_table_view_controller.h [modify] https://crrev.com/450b0990fb9685ed1c2df8c941904ce909d82f6e/ios/chrome/browser/ui/settings/clear_browsing_data_table_view_controller.mm
,
Aug 3
verified on Canary on 8/3
,
Aug 3
This bug requires manual review: Less than 28 days to go before AppStore submit on M69 Please contact the milestone owner if you have questions. Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Aug 3
Removed merge-merged label so we can follow this second CL merge. Approved.
,
Aug 3
,
Aug 3
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/26aef0d0afc889f66c5b024da141080353621bdb commit 26aef0d0afc889f66c5b024da141080353621bdb Author: Chris Lu <thegreenfrog@chromium.org> Date: Fri Aug 03 20:44:14 2018 [ios] Fix Unwinding of History and ClearBrowsingData when Clear Browsing Data dialog is open - stops all action sheet coordinators in ClearBrowsingDataTableViewController before dismissing itself. - sets historyClearBrowsingDataCoordinator to nil after finishing unwinding ClearBrowsingData and History to prevent race condition. Video: https://drive.google.com/open?id=1wDymlQtfStNWBY9mI9GYqlig8qkipBDF TBR=thegreenfrog@chromium.org (cherry picked from commit 450b0990fb9685ed1c2df8c941904ce909d82f6e) Bug: 867329 Change-Id: I7c0825b02ed7d3c6122f0a5ae37c99181fc6d182 Reviewed-on: https://chromium-review.googlesource.com/1159683 Commit-Queue: Chris Lu <thegreenfrog@chromium.org> Reviewed-by: Sergio Collazos <sczs@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#580348} Reviewed-on: https://chromium-review.googlesource.com/1162633 Cr-Commit-Position: refs/branch-heads/3497@{#391} Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753} [modify] https://crrev.com/26aef0d0afc889f66c5b024da141080353621bdb/ios/chrome/browser/ui/history/history_clear_browsing_data_coordinator.mm [modify] https://crrev.com/26aef0d0afc889f66c5b024da141080353621bdb/ios/chrome/browser/ui/history/history_coordinator.mm [modify] https://crrev.com/26aef0d0afc889f66c5b024da141080353621bdb/ios/chrome/browser/ui/settings/clear_browsing_data_table_view_controller.h [modify] https://crrev.com/26aef0d0afc889f66c5b024da141080353621bdb/ios/chrome/browser/ui/settings/clear_browsing_data_table_view_controller.mm
,
Aug 7
Verified on chrome canary version 70.0.3515.0 on iPhone 8 plus with iOS 12 beta 6, iPhone X with iOS 11.4.1, following steps mentioned in comment #0. No freezing observed. Looks good.
,
Aug 8
Verified on M69.0.3497.31 beta iOS: 10.3.3, 12.0 beta#6 iPhone7Plus, iPhoneX |
|||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||
Comment 1 by marq@chromium.org
, Jul 25Labels: -Pri-2 -MS-Tab-Grid -S-Recent-Tabs-In-Grid S-Clear-Browsing-History MS-History Q2 M-69 Pri-1