App crashes on tapping on UNDO after closing multiple tabs in Multitasking mode. |
|||||||||||||
Issue descriptionApp Version: 69.0.3493.0 Canary iOS Version: 10.3.3,iOS 11 Device: iPad Mini2, iPad Air2 Steps to reproduce: 1. Launch Chrome slide over mode 2. Open Many tabs say more than 90. 3. Tap on tabswithcer icon. 4. Tap on Close all. 5. Tap on Undo Observed results: App crashes on tapping on Undo. Expected results: App should not crash. Number of times you were able to reproduce: 3/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 : NA New UI in M69 Bug reproducible on the current beta channel build : Yes in M69 Link to video/image: https://drive.google.com/file/d/1RaA6DhZa9X-Re0erfCgwGC0UPn5tulNp/view?usp=sharing
,
Jul 20
I'll take a look at this one as well.
,
Jul 20
,
Jul 23
Not actively working on this yet; focusing on Issue 853634 at the moment.
,
Jul 24
,
Jul 24
I'm able to reproduce this, but only on device. It seems to be OOM caused by TabModelWebUsageEnabledObserver calling LoadIfNecessary() on every WebState that's being restored in the undo operation. My proposed solution is to move this functionality to a KeyedService. That way, we can disable the auto-loading behavior when restoring many WebStates at once (i.e. undo close all tabs, crash restoration), then reenable to kick off the initial load for WebStates added one at a time. After talking to Rohit about this, we think that this will be a bit too much code for an edge case, so we likely don't want to merge this to M69. cc. Sylvain for input on the proposed solution cc. Ed since this will also help with your snapshotting issue.
,
Jul 25
Agree that the proposed CL is way too big to merge. Are we OK shipping M69 without any fix for this?
,
Jul 25
I think that this is enough of an edge case that we can live without shipping the fix in M69. I've tried on newer devices and I can successfully undo a 99+ page deletion without crashing. This seems more likely to happen on older iPads and in multitasking mode, where we have even less memory to use. However, my testing was restoring large numbers of NTPs; the number might be smaller for loading actual web pages.
,
Jul 25
Adding Kariah to this bug for her input regarding merging/severity. Also the fix is here: crrev.com/c/1148596
,
Jul 25
Updating the milestone to M70. Kariah, feel free to retarget for M69 if you feel this is important enough to merge a new KeyedService over.
,
Jul 25
,
Jul 25
I agree that CL should not be merged to M69. I'm okay letting this wait for M70 considering it only happens on older devices and the user has to have 90+ tabs open.
,
Jul 30
After the crash, are the user's tabs still there, or have they lost them?
,
Jul 30
The bug is that we trigger a page load for every tab when restoring a session (either via Undo Close All Tabs or via the crash restore infobar). This ends up OOMing the app if you have too many tabs. Since we are unable to restore a session without crashing, the tabs are lost permanently. We should only trigger a page load for the active tab, but making that change is hard. I don't think it's something we can take in M69.
,
Aug 22
We de-prioritized the review for crrev.com/c/1148596 because we didn't merge it to M69. I've updated a new patch that resolves merge conflicts; we should get this landed soon since it's a larger fix.
,
Aug 30
Hi Kurt - where are we on the CL? Did you want to have others besides marq review before landing? This is a 500+ line change so it would be great if this could land before branch later today if ready.
,
Aug 30
The CL is in the CQ now, and should be landed today.
,
Aug 30
Great thanks.
,
Aug 30
Still trying to CQ that CL, but it's running into some flakiness in the CQ. I've created crrev.com/c/1197762 to address the flake, but won't be able to land that until I get a reviewer.
,
Aug 31
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/070e40b005e5aa433eab82f2918852f2bdf40396 commit 070e40b005e5aa433eab82f2918852f2bdf40396 Author: Kurt Horimoto <kkhorimoto@chromium.org> Date: Fri Aug 31 20:50:00 2018 [iOS] Move TabModel webUsageEnabled setting to a KeyedService. This allows for the optional disabling of the default LoadIfNecessary() behavior that previously occurred in TabModelWebUsageEnabledObserver. This can be used when restoring a large number of Tabs to prevent the creation of many WKWebViews at once. Bug: 864478 Cq-Include-Trybots: luci.chromium.try:ios-simulator-cronet;luci.chromium.try:ios-simulator-full-configs Change-Id: Ie80722aa178e260e6c6841de8e4cad8bb1e7ada4 Reviewed-on: https://chromium-review.googlesource.com/1148596 Commit-Queue: Kurt Horimoto <kkhorimoto@chromium.org> Reviewed-by: Mark Cogan <marq@chromium.org> Cr-Commit-Position: refs/heads/master@{#588159} [modify] https://crrev.com/070e40b005e5aa433eab82f2918852f2bdf40396/ios/chrome/browser/browser_state/BUILD.gn [modify] https://crrev.com/070e40b005e5aa433eab82f2918852f2bdf40396/ios/chrome/browser/browser_state/browser_state_keyed_service_factories.mm [modify] https://crrev.com/070e40b005e5aa433eab82f2918852f2bdf40396/ios/chrome/browser/tabs/BUILD.gn [modify] https://crrev.com/070e40b005e5aa433eab82f2918852f2bdf40396/ios/chrome/browser/tabs/tab_model.h [modify] https://crrev.com/070e40b005e5aa433eab82f2918852f2bdf40396/ios/chrome/browser/tabs/tab_model.mm [modify] https://crrev.com/070e40b005e5aa433eab82f2918852f2bdf40396/ios/chrome/browser/tabs/tab_model_unittest.mm [delete] https://crrev.com/28e953d7d5bd94c1ab319b185eba612f7456df90/ios/chrome/browser/tabs/tab_model_web_usage_enabled_observer.h [delete] https://crrev.com/28e953d7d5bd94c1ab319b185eba612f7456df90/ios/chrome/browser/tabs/tab_model_web_usage_enabled_observer.mm [modify] https://crrev.com/070e40b005e5aa433eab82f2918852f2bdf40396/ios/chrome/browser/ui/BUILD.gn [modify] https://crrev.com/070e40b005e5aa433eab82f2918852f2bdf40396/ios/chrome/browser/ui/browser_view_controller.mm [modify] https://crrev.com/070e40b005e5aa433eab82f2918852f2bdf40396/ios/chrome/browser/ui/browser_view_controller_unittest.mm [modify] https://crrev.com/070e40b005e5aa433eab82f2918852f2bdf40396/ios/chrome/browser/ui/tab_grid/BUILD.gn [modify] https://crrev.com/070e40b005e5aa433eab82f2918852f2bdf40396/ios/chrome/browser/ui/tab_grid/tab_grid_mediator.mm [add] https://crrev.com/070e40b005e5aa433eab82f2918852f2bdf40396/ios/chrome/browser/web_state_list/web_usage_enabler/BUILD.gn [add] https://crrev.com/070e40b005e5aa433eab82f2918852f2bdf40396/ios/chrome/browser/web_state_list/web_usage_enabler/web_state_list_web_usage_enabler.h [add] https://crrev.com/070e40b005e5aa433eab82f2918852f2bdf40396/ios/chrome/browser/web_state_list/web_usage_enabler/web_state_list_web_usage_enabler_factory.h [add] https://crrev.com/070e40b005e5aa433eab82f2918852f2bdf40396/ios/chrome/browser/web_state_list/web_usage_enabler/web_state_list_web_usage_enabler_factory.mm [add] https://crrev.com/070e40b005e5aa433eab82f2918852f2bdf40396/ios/chrome/browser/web_state_list/web_usage_enabler/web_state_list_web_usage_enabler_impl.h [add] https://crrev.com/070e40b005e5aa433eab82f2918852f2bdf40396/ios/chrome/browser/web_state_list/web_usage_enabler/web_state_list_web_usage_enabler_impl.mm [add] https://crrev.com/070e40b005e5aa433eab82f2918852f2bdf40396/ios/chrome/browser/web_state_list/web_usage_enabler/web_state_list_web_usage_enabler_impl_unittest.mm [modify] https://crrev.com/070e40b005e5aa433eab82f2918852f2bdf40396/ios/chrome/test/BUILD.gn [modify] https://crrev.com/070e40b005e5aa433eab82f2918852f2bdf40396/ios/chrome/test/app/BUILD.gn [modify] https://crrev.com/070e40b005e5aa433eab82f2918852f2bdf40396/ios/chrome/test/app/tab_test_util.mm
,
Aug 31
Finally made it through the CQ a day later.
,
Sep 1
Your change meets the bar and is auto-approved for M70. Please go ahead and merge the CL to branch 3538 manually. Please contact milestone owner if you have questions. Owners: benmason@(Android), kariahda@(iOS), geohsu@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Sep 5
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible! If all merges have been completed, please remove any remaining Merge-Approved labels from this issue. Thanks for your time! To disable nags, add the Disable-Nags label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Sep 6
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/62e1dd9e122417a58abf1a6c7ce38210cc2db1a8 commit 62e1dd9e122417a58abf1a6c7ce38210cc2db1a8 Author: Kurt Horimoto <kkhorimoto@chromium.org> Date: Thu Sep 06 22:58:15 2018 [iOS] Move TabModel webUsageEnabled setting to a KeyedService. This allows for the optional disabling of the default LoadIfNecessary() behavior that previously occurred in TabModelWebUsageEnabledObserver. This can be used when restoring a large number of Tabs to prevent the creation of many WKWebViews at once. Bug: 864478 Cq-Include-Trybots: luci.chromium.try:ios-simulator-cronet;luci.chromium.try:ios-simulator-full-configs Change-Id: Ie80722aa178e260e6c6841de8e4cad8bb1e7ada4 Reviewed-on: https://chromium-review.googlesource.com/1148596 Commit-Queue: Kurt Horimoto <kkhorimoto@chromium.org> Reviewed-by: Mark Cogan <marq@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#588159}(cherry picked from commit 070e40b005e5aa433eab82f2918852f2bdf40396) Reviewed-on: https://chromium-review.googlesource.com/1211671 Reviewed-by: Kurt Horimoto <kkhorimoto@chromium.org> Cr-Commit-Position: refs/branch-heads/3538@{#107} Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811} [modify] https://crrev.com/62e1dd9e122417a58abf1a6c7ce38210cc2db1a8/ios/chrome/browser/browser_state/BUILD.gn [modify] https://crrev.com/62e1dd9e122417a58abf1a6c7ce38210cc2db1a8/ios/chrome/browser/browser_state/browser_state_keyed_service_factories.mm [modify] https://crrev.com/62e1dd9e122417a58abf1a6c7ce38210cc2db1a8/ios/chrome/browser/tabs/BUILD.gn [modify] https://crrev.com/62e1dd9e122417a58abf1a6c7ce38210cc2db1a8/ios/chrome/browser/tabs/tab_model.h [modify] https://crrev.com/62e1dd9e122417a58abf1a6c7ce38210cc2db1a8/ios/chrome/browser/tabs/tab_model.mm [modify] https://crrev.com/62e1dd9e122417a58abf1a6c7ce38210cc2db1a8/ios/chrome/browser/tabs/tab_model_unittest.mm [delete] https://crrev.com/8c17628ca6d5768403e5527075391be33c8b7a1d/ios/chrome/browser/tabs/tab_model_web_usage_enabled_observer.h [delete] https://crrev.com/8c17628ca6d5768403e5527075391be33c8b7a1d/ios/chrome/browser/tabs/tab_model_web_usage_enabled_observer.mm [modify] https://crrev.com/62e1dd9e122417a58abf1a6c7ce38210cc2db1a8/ios/chrome/browser/ui/BUILD.gn [modify] https://crrev.com/62e1dd9e122417a58abf1a6c7ce38210cc2db1a8/ios/chrome/browser/ui/browser_view_controller.mm [modify] https://crrev.com/62e1dd9e122417a58abf1a6c7ce38210cc2db1a8/ios/chrome/browser/ui/browser_view_controller_unittest.mm [modify] https://crrev.com/62e1dd9e122417a58abf1a6c7ce38210cc2db1a8/ios/chrome/browser/ui/tab_grid/BUILD.gn [modify] https://crrev.com/62e1dd9e122417a58abf1a6c7ce38210cc2db1a8/ios/chrome/browser/ui/tab_grid/tab_grid_mediator.mm [add] https://crrev.com/62e1dd9e122417a58abf1a6c7ce38210cc2db1a8/ios/chrome/browser/web_state_list/web_usage_enabler/BUILD.gn [add] https://crrev.com/62e1dd9e122417a58abf1a6c7ce38210cc2db1a8/ios/chrome/browser/web_state_list/web_usage_enabler/web_state_list_web_usage_enabler.h [add] https://crrev.com/62e1dd9e122417a58abf1a6c7ce38210cc2db1a8/ios/chrome/browser/web_state_list/web_usage_enabler/web_state_list_web_usage_enabler_factory.h [add] https://crrev.com/62e1dd9e122417a58abf1a6c7ce38210cc2db1a8/ios/chrome/browser/web_state_list/web_usage_enabler/web_state_list_web_usage_enabler_factory.mm [add] https://crrev.com/62e1dd9e122417a58abf1a6c7ce38210cc2db1a8/ios/chrome/browser/web_state_list/web_usage_enabler/web_state_list_web_usage_enabler_impl.h [add] https://crrev.com/62e1dd9e122417a58abf1a6c7ce38210cc2db1a8/ios/chrome/browser/web_state_list/web_usage_enabler/web_state_list_web_usage_enabler_impl.mm [add] https://crrev.com/62e1dd9e122417a58abf1a6c7ce38210cc2db1a8/ios/chrome/browser/web_state_list/web_usage_enabler/web_state_list_web_usage_enabler_impl_unittest.mm [modify] https://crrev.com/62e1dd9e122417a58abf1a6c7ce38210cc2db1a8/ios/chrome/test/BUILD.gn [modify] https://crrev.com/62e1dd9e122417a58abf1a6c7ce38210cc2db1a8/ios/chrome/test/app/BUILD.gn [modify] https://crrev.com/62e1dd9e122417a58abf1a6c7ce38210cc2db1a8/ios/chrome/test/app/tab_test_util.mm
,
Sep 12
Verified in 70.0.3538.13 Beta in iPad Air(iOS 11.4.1), iPad Air(iOS 12 Beta 12) Followed the steps mentioned in comment#0, the app doesn't crash on tapping UNDO button Link to video: https://drive.google.com/file/d/1AN72OdGUFMaNX_baR3zh59oHU-_vmQK5/view?usp=sharing |
|||||||||||||
►
Sign in to add a comment |
|||||||||||||
Comment 1 by marq@chromium.org
, Jul 17Labels: -Pri-3 M-69 Q2 Pri-1
Owner: edchin@chromium.org
Status: Assigned (was: Untriaged)