Issue metadata
Sign in to add a comment
|
Background opened tabs are not restored correctly on cold start. |
||||||||||||||||||||||
Issue descriptionApp Version: 66.0.3359.0 canary iOS Version: 11.2.6, 10.3.3 Device: iPhones URL: any Steps to reproduce: 1. Launch Google Chrome 2. Navigate to techmeme.com 3. Long tap on any link and select "Open link in a new tab" 4. Background the app and Force quit 5. Launch the app and navigate to the tab opened in step#3 Observed results: Observe that blank page is displayed. Expected results: Tab contents should be loaded correctly. Good Version: 65.0.3305.0 Canary #948835e Bad Version: 65.0.3306.0 Canary #7b77ca9 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 Safari/Firefox: Firefox: NA, Safari: NA Bug reproducible on current stable build (App Version, iOS Version): M65 Yes, Working fine in M64 Bug reproducible on the current beta channel build (App Version, iOS Version): M66 Yes Link to Video: https://drive.google.com/file/d/15v3KvGfsRSOZfc8E2y2xgRYwC76dRoHM/view
,
Mar 8 2018
Sylvain, can you take a look?
,
Mar 9 2018
From https://omahaproxy.appspot.com/: 65.0.3305.0 => 1e63251be5feaebd51e75475d20c7570b18929d9 65.0.3306.0 => 7193ef9f4e3d088055cf4109f09e2b53a5db8831 Those revision are on the branch, so we need to translate them to the first CL on master. This turn out to be parent commit for both since they are just the CL changing the revision number. So after the fix we have: 65.0.3305.0 => 533b299c8b27b6f00387f2c3dfb018c175868271 65.0.3306.0 => 4cec1c6f7ab05203d702cb1fc6b43dfef5063872 Let's see whether there were any changes to iOS in that range. $ git log --oneline 533b299c8b27b6f00387f2c3dfb018c175868271...4cec1c6f7ab05203d702cb1fc6b43dfef5063872 -- ios 3a449ceed137 [iOS] Only disable BVC rotation during transition animations. 4d1f696c4766 [iOS] Fix fullscreen for replaced scroll views. 44835133e5da Use PagePlaceholderTabHelper to present overlay. d7d7ac5f4290 [iOS] Enable EarlGrey tests 4731b2779159 Improve SnapshotGenerateDelegate API. f2ee0665d9d0 Use Profile's pref to cache position in the iOS bookmarks. 82e828b536e2 Remove Tab -useGreyImageCache property. Now, let's try to see whether we can reproduce the bug on local builds of 65.0.3306.0 and have a working app when building 65.0.3305.0. If this is the case, then we will be able to use "git bisect" to pin-point the regression. $ git checkout 533b299c8b27b6f00387f2c3dfb018c175868271 && gclient sync && ninja -C ... $ git checkout 4cec1c6f7ab05203d702cb1fc6b43dfef5063872 && gclient sync && ninja -C ... After confirming that issue is reproducible with locally build 4cec1c6f7ab05203d702cb1fc6b43dfef5063872 and do not reproduce with locally build 533b299c8b27b6f00387f2c3dfb018c175868271, we can start git bisect. $ git bisect start $ git bisect good 533b299c8b27b6f00387f2c3dfb018c175868271 $ git bisect bad 4cec1c6f7ab05203d702cb1fc6b43dfef5063872 Bisecting: 29 revisions left to test after this (roughly 5 steps) [2370d808c46d420802846ff53825e39c14b6b9eb] Remove unused code after /deep/ and ::shadow removal $ git bisect good Bisecting: 14 revisions left to test after this (roughly 4 steps) [a9fd9eb2ade18dbd028703a7f6bcb862923c444a] Accept failures in external/wpt/websockets/opening-handshake/005.html?wss $ git bisect bad Bisecting: 7 revisions left to test after this (roughly 3 steps) [7fa64f29c448b7a7dabb3587c2829846b8222f24] Roll src/third_party/skia/ 7557bbbe1..68c8156cd (1 commit) $ git bisect bad Bisecting: 3 revisions left to test after this (roughly 2 steps) [ed6f5c83b64a0e7792de626e64e99239d34f4ca2] Update V8 to version 6.5.105.1 (cherry-pick). $ git bisect bad Bisecting: 0 revisions left to test after this (roughly 1 step) [0c5fc82ad739d803fcb8ca79071767903843f465] Zip Archiver: Handle wrong passwords input for Traditional PKZIP. $ git bisect bad Bisecting: 0 revisions left to test after this (roughly 0 steps) [44835133e5dae4bbc458a7f75a0caa453791342a] Use PagePlaceholderTabHelper to present overlay. $ git bisect bad 44835133e5dae4bbc458a7f75a0caa453791342a is the first bad commit commit 44835133e5dae4bbc458a7f75a0caa453791342a Author: Sylvain Defresne <sdefresne@chromium.org> Date: Wed Dec 27 11:38:36 2017 +0100 Use PagePlaceholderTabHelper to present overlay. USe PagePlaceholderTabHelper to present the grey image overlay instead of the CRWWebController implementation. Bug: 616244, 620939 Change-Id: I60589825448892ff25edd37b6cdf681b3c253892 Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs Reviewed-on: https://chromium-review.googlesource.com/836989 Commit-Queue: Sylvain Defresne <sdefresne@chromium.org> Reviewed-by: Eugene But <eugenebut@chromium.org> Cr-Commit-Position: refs/heads/master@{#526219} :040000 040000 ae8075f6cafe07cd0ad118e63daffa9c438dd95e 90a88014554a8680a84f89b8726cb152b7c36b57 M ios
,
Mar 9 2018
Looking at the CL that introduced the regression, the most likely source of failure is due to the removal to -setOverlayPreviewMode: calls. They likely did put the WebState/WebController in a state that forced a reload when made visible that is no longer happening.
,
Mar 9 2018
I was wrong, the -setOverlayPreviewMode: methods don't change anything. The problem comes from changes to -canTakeSnapshotForWebState:.
This method diff is the following:
- (BOOL)canTakeSnapshotForWebState:(web::WebState*)webState {
DCHECK_EQ(_webStateImpl, webState);
- return webState->CanTakeSnapshot();
+ return !PagePlaceholderTabHelper::FromWebState(webState)
+ ->displaying_placeholder();
}
The WebStateImpl::CanTakeSnapshot() checks whether CRWWebController _containerView is nil or not, while PagePlaceholderTabHelper::displaying_placeholder() checks whether a placeholder has been requested. If this method returns YES, then WebStateImpl::GetView() gets invoked. This should not change anything, except that it relies on an apparently invalid assumption on how WebState API works.
The code assumes that the following two sequences are equivalent (assuming that a load request has been sent previously on the WebState, and that the CRWWebController _containerView is nil, which is the case after restoring a WebState from cold start in the background):
webState->NavigationManager()->LoadIfNecessary();
webState->GetView();
and
webState->GetView();
webState->NavigationManager()->LoadIfNecessary();
However, they are not. If GetView() is called first, then _containerView will be constructed but the navigation won't start. Then LoadIfNecessary() won't do anything as the implementation does nothing if _containerView is not nil. I think this is a bug in WebState.
A possible fix is to force a placeholder overlay on all background webstate when restoring a session. This should fix -canTakeSnapshotForWebState: to return NO and prevent the call of GetView(), but I think this is a workaround, so I'll open a bug to fix the WebState API.
,
Mar 9 2018
I see that on PixelBook 66.0.3359.10 (Official Build) dev (64-bit). Just resizing a little bit displays the page again.
,
Mar 9 2018
,
Mar 9 2018
jmeurin: this bug is iOS specific. It is probably best to file a separate bug for PixelBook as the fix is going to be different from the fix for iOS (the bug is in platform specific code).
,
Mar 10 2018
,
Mar 12 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/eebda1ae9ca8ab4d2d85a5141060a1bb4db70dd1 commit eebda1ae9ca8ab4d2d85a5141060a1bb4db70dd1 Author: Sylvain Defresne <sdefresne@chromium.org> Date: Mon Mar 12 14:07:09 2018 Fix loading background Tab after cold start If a place holder is scheduled to be displayed, do not allow capturing a snapshot. This prevents calling WebState::GetView before WebState::NavigationManager::LoadIfNecessary and thus prevents bumping into issue crbug.com/820484 . Bug: 820111 Change-Id: I9d326a0fdccd7b6ea22b1b5477e79b7b7b82a586 Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs Reviewed-on: https://chromium-review.googlesource.com/956188 Reviewed-by: Rohit Rao <rohitrao@chromium.org> Commit-Queue: Sylvain Defresne <sdefresne@chromium.org> Cr-Commit-Position: refs/heads/master@{#542473} [modify] https://crrev.com/eebda1ae9ca8ab4d2d85a5141060a1bb4db70dd1/ios/chrome/browser/ui/browser_view_controller.mm
,
Mar 12 2018
,
Mar 12 2018
This bug requires manual review: Request affecting a post-stable build Please contact the milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Mar 13 2018
Your change meets the bar and is auto-approved for M66. Please go ahead and merge the CL to branch 3359 manually. Please contact milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), josafat@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Mar 13 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/40eb3584498b9f28c5f1c1de3e57f3cb7f6b5737 commit 40eb3584498b9f28c5f1c1de3e57f3cb7f6b5737 Author: Sylvain Defresne <sdefresne@chromium.org> Date: Tue Mar 13 14:36:45 2018 Fix loading background Tab after cold start If a place holder is scheduled to be displayed, do not allow capturing a snapshot. This prevents calling WebState::GetView before WebState::NavigationManager::LoadIfNecessary and thus prevents bumping into issue crbug.com/820484 . Bug: 820111 Change-Id: I9d326a0fdccd7b6ea22b1b5477e79b7b7b82a586 Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs Reviewed-on: https://chromium-review.googlesource.com/956188 Reviewed-by: Rohit Rao <rohitrao@chromium.org> Commit-Queue: Sylvain Defresne <sdefresne@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#542473}(cherry picked from commit eebda1ae9ca8ab4d2d85a5141060a1bb4db70dd1) Reviewed-on: https://chromium-review.googlesource.com/960601 Reviewed-by: Sylvain Defresne <sdefresne@chromium.org> Cr-Commit-Position: refs/branch-heads/3359@{#195} Cr-Branched-From: 66afc5e5d10127546cc4b98b9117aff588b5e66b-refs/heads/master@{#540276} [modify] https://crrev.com/40eb3584498b9f28c5f1c1de3e57f3cb7f6b5737/ios/chrome/browser/ui/browser_view_controller.mm
,
Mar 13 2018
,
Mar 14 2018
Verified on chrome beta version 66.0.3359.30 on iPhone 6s plus 10.3.3 and iPhone 7 11.2.6, following steps mentioned in comment #0. Tab contents are loaded. Looks good.
,
Mar 19 2018
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
,
Mar 19 2018
,
Mar 20 2018
Verified the issue on the build 67.0.3376.0 canary tested in iPhone7+(11.2.6). Background opened tabs are restored properly, works fine. |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by eugene...@chromium.org
, Mar 8 2018Components: -Mobile>WebView>Glue UI>Browser>Sessions