New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 820111 link

Starred by 0 users

Issue metadata

Status: Verified
Owner:
Closed: Mar 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 1
Type: Bug-Regression

Blocked on:
issue 820484



Sign in to add a comment

Background opened tabs are not restored correctly on cold start.

Project Member Reported by srikanthg@chromium.org, Mar 8 2018

Issue description

App 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
 
Cc: -eugene...@chromium.org kkhorimoto@chromium.org sdefresne@chromium.org danyao@chromium.org
Components: -Mobile>WebView>Glue UI>Browser>Sessions
Owner: sdefresne@chromium.org
Status: Assigned (was: Untriaged)
Sylvain, can you take a look?
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

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.
Cc: eugene...@chromium.org
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.

Comment 6 by jmeurin@google.com, 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.
Blockedon: 820484
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).
Project Member

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

Labels: M-65 M-66 Merge-Request-66 Merge-Request-65
Status: Fixed (was: Assigned)
Project Member

Comment 12 by sheriffbot@chromium.org, Mar 12 2018

Labels: -Merge-Request-65 Merge-Review-65 Hotlist-Merge-Review
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
Project Member

Comment 13 by sheriffbot@chromium.org, Mar 13 2018

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

Comment 14 by bugdroid1@chromium.org, Mar 13 2018

Labels: -merge-approved-66 merge-merged-3359
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

Comment 15 by cmasso@google.com, Mar 13 2018

Labels: -Hotlist-Merge-Review -Hotlist-Merge-Approved Merge-Approved-66
Status: Verified (was: Fixed)
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.
Project Member

Comment 17 by sheriffbot@chromium.org, Mar 19 2018

Cc: cmasso@google.com
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
Labels: -Merge-Review-65 -Merge-Approved-66
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