New issue
Advanced search Search tips

Issue 864478 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Closed: Aug 31
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 1
Type: Bug
Q3



Sign in to add a comment

App crashes on tapping on UNDO after closing multiple tabs in Multitasking mode.

Project Member Reported by pmadalla@chromium.org, Jul 17

Issue description

App 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

 
Cc: marq@chromium.org
Labels: -Pri-3 M-69 Q2 Pri-1
Owner: edchin@chromium.org
Status: Assigned (was: Untriaged)
Owner: kkhorimoto@chromium.org
Status: Started (was: Assigned)
I'll take a look at this one as well.
Labels: S-Undo-Close-All-Tabs
Status: Assigned (was: Started)
Not actively working on this yet; focusing on Issue 853634 at the moment.
Status: Started (was: Assigned)
Cc: edchin@chromium.org sdefresne@chromium.org rohitrao@chromium.org
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.
Agree that the proposed CL is way too big to merge. 

Are we OK shipping M69 without any fix for this?
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.
Cc: kariahda@chromium.org
Adding Kariah to this bug for her input regarding merging/severity.

Also the fix is here: crrev.com/c/1148596
Labels: -M-69 M-70
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.
Labels: -Q2 Q3
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.
After the crash, are the user's tabs still there, or have they lost them? 
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.
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.
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.
The CL is in the CQ now, and should be landed today.
Great thanks.
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.
Project Member

Comment 20 by bugdroid1@chromium.org, 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

Labels: Merge-Request-70
Status: Fixed (was: Started)
Finally made it through the CQ a day later.
Project Member

Comment 22 by sheriffbot@chromium.org, Sep 1

Labels: -Merge-Request-70 Hotlist-Merge-Approved Merge-Approved-70
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
Project Member

Comment 23 by sheriffbot@chromium.org, 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
Project Member

Comment 24 by bugdroid1@chromium.org, Sep 6

Labels: -merge-approved-70 merge-merged-3538
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

Status: Verified (was: Fixed)
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