New issue
Advanced search Search tips

Issue 677193 link

Starred by 1 user

Issue metadata

Status: Verified
Owner: ----
Closed: Feb 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 2
Type: Bug



Sign in to add a comment

Go forward does not work for interstitials that are in the middle of navigation stack

Project Member Reported by eugene...@chromium.org, Dec 27 2016

Issue description

App Version (from "Chrome Settings > About Chrome"): 47
iOS Version: iOS 10.2
Device: iPhone SE

Steps to reproduce: 
1.) Turn on Airplane Mode
2.) Go to expired.badssl.com
3.) Go to google.com
4.) Go Back
5.) Go Back
6.) Turn off Airplane Mode
7.) Go Forward
8.) Go Forward

Observed behavior: 
NTP is loaded (app went back)

Expected behavior: 
google.com is loaded (app should go back)

 

Comment 1 Deleted

This is different from  crbug.com/677190  and has a different root cause. 

 crbug.com/677190  caused by 2 different issues (one is GetIndexForOffset, another is same as root cause for this bug). 

Project Member

Comment 3 by bugdroid1@chromium.org, Feb 14 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b2517f141263ee8913024a3a96cd909423ad7214

commit b2517f141263ee8913024a3a96cd909423ad7214
Author: eugenebut <eugenebut@chromium.org>
Date: Tue Feb 14 03:21:43 2017

Fixed navigation for displayed SSL interstitials.

Navigation did not work for interstitials for 2 reasons which are fixed
in this CL:

1.) |WebInterstitialImpl::DontProceed| should not reload.
Reloading stops pending navigation and reloads last committed item,
which is not always correct target item. On other platforms
InterstitialPageImpl::DontProceed does not reload.

2.) |clearTransientContentView| removes pending item, so it should be
called before pending navigation is initiated. So next
|clearTransientContentView| call will be no-op.

This CL changes the behavior, so when user taps "BACK TO SAFETY" the page
is not reloaded. This is fine, but in order to update omnibox URL, tab
should subscribe to webStateDidDismissInterstitial: callback and update
the toolbar.

BUG= 677193 , 677190 , 688009 

Review-Url: https://codereview.chromium.org/2687343003
Cr-Commit-Position: refs/heads/master@{#450220}

[modify] https://crrev.com/b2517f141263ee8913024a3a96cd909423ad7214/ios/chrome/browser/tabs/tab.mm
[modify] https://crrev.com/b2517f141263ee8913024a3a96cd909423ad7214/ios/web/interstitials/web_interstitial_impl.mm
[modify] https://crrev.com/b2517f141263ee8913024a3a96cd909423ad7214/ios/web/web_state/ui/crw_web_controller.mm

Status: Fixed (was: Available)
Verified this on M58.0.3013.0 canary

After step#7 from the original steps to reproduce, i see expired.badssl.com is loaded and security interstitial is displayed. But the expected behavior states that google.com should be loaded.

@eugenebut can you confirm what is the expected behavior here?
I remember that forward button did not work correctly in M47 when current page was SSL interstitial. I think |Step 4| is redundant in STR, but I don't have the old build to confirm that. 
Project Member

Comment 7 by bugdroid1@chromium.org, Feb 16 2017

Labels: merge-merged-2987
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9674693c37dc34ad55e2f21f19050ed2938d6d1c

commit 9674693c37dc34ad55e2f21f19050ed2938d6d1c
Author: eugenebut <eugenebut@chromium.org>
Date: Thu Feb 16 01:36:27 2017

Fixed navigation for displayed SSL interstitials.

Navigation did not work for interstitials for 2 reasons which are fixed
in this CL:

1.) |WebInterstitialImpl::DontProceed| should not reload.
Reloading stops pending navigation and reloads last committed item,
which is not always correct target item. On other platforms
InterstitialPageImpl::DontProceed does not reload.

2.) |clearTransientContentView| removes pending item, so it should be
called before pending navigation is initiated. So next
|clearTransientContentView| call will be no-op.

This CL changes the behavior, so when user taps "BACK TO SAFETY" the page
is not reloaded. This is fine, but in order to update omnibox URL, tab
should subscribe to webStateDidDismissInterstitial: callback and update
the toolbar.

BUG= 677193 , 677190 , 688009 
TBR=kkhorimoto@chromium.org
NOTRY=true
NOPRESUBMIT=true

Review-Url: https://codereview.chromium.org/2687343003
Cr-Commit-Position: refs/heads/master@{#450220}
(cherry picked from commit b2517f141263ee8913024a3a96cd909423ad7214)

Review-Url: https://codereview.chromium.org/2694623013
Cr-Commit-Position: refs/branch-heads/2987@{#537}
Cr-Branched-From: ad51088c0e8776e8dcd963dbe752c4035ba6dab6-refs/heads/master@{#444943}

[modify] https://crrev.com/9674693c37dc34ad55e2f21f19050ed2938d6d1c/ios/chrome/browser/tabs/tab.mm
[modify] https://crrev.com/9674693c37dc34ad55e2f21f19050ed2938d6d1c/ios/web/interstitials/web_interstitial_impl.mm
[modify] https://crrev.com/9674693c37dc34ad55e2f21f19050ed2938d6d1c/ios/web/web_state/ui/crw_web_controller.mm

Description: Show this description
Status: Verified (was: Fixed)
Verified as per the bug #1 after taking to Eugene.Looks good. Tested on M58.0.3019.0 Canary on iPhone 6+ iOS 10.2 , iPad Pro iOS 10.1.1
Verified on chrome beta version 57.0.2987.72 on iPhone 7 plus with iOS 10.2.1, iPad Air 10.2.1 following steps mentioned in comment #0.  google.com is displayed at step #8.  Looks good.

Sign in to add a comment