Lindsay, could you please ask QA to check if this is a regression. Thanks!

If this is not a regression between M54 and M55, is it possible to check whether it regressed when we switched from UIWebView to WKWebView (M48 to M49 IIRC).

The oldest i can install is M50 version, and the behavior is same. Builds M48 or 49 can't be installed now.
We never have a testcase that these should be remembered for 7days. I always make sure to see the interstitial after force quit assuming it should only be remembered per session.

M46 was UIWebView only
M47 has both (not worth testing)
M48 was WKWebView only for clean install

I have filed a bug for the builds
https://bugs.chromium.org/p/chromium/issues/detail?id=685801

Hi Vinutha,
Can you please check builds 54 and 53 today and let us know the behavior in those 2 milestones?
Thanks,

The bug is reproducible on build 53.0.2785.97 dev and 54.0.2840.98 dev in iPhone6+ iOS 10.2.1
Video: https://drive.google.com/open?id=0B6GVWQnhaMClMmdvMWtSLUJ0bTQ 

This has regressed when we shipped WKWebView in M48.

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

commit a8475d7892c2333e06372af29d1648e550d72ed8
Author: eugenebut <eugenebut@chromium.org>
Date: Mon Jan 30 15:51:27 2017

Remember user decision for bad ssl certs.

Call registerAllowedCertificate:forHost:status: when user accepted bad
SSL cert in the same way as Request Tracker did:
https://codereview.chromium.org/2648763002/patch/1/10002

BUG=685745

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

[modify] https://crrev.com/a8475d7892c2333e06372af29d1648e550d72ed8/ios/web/web_state/ui/crw_web_controller.mm

[Auto-generated comment by a script] We noticed that this issue is targeted for M-57; it appears the fix may have landed after branch point, meaning a merge might be required. Please confirm if a merge is required here - if so add Merge-Request-57 label, otherwise remove Merge-TBD label. Thanks.

Your change meets the bar and is auto-approved for M57. Please go ahead and merge the CL to branch 2987 manually. Please contact milestone owner if you have questions.
Owners: amineer@(clank), cmasso@(bling), ketakid@(cros), govind@(desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

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

commit ce62a87e5eb602484b4abbf117af31cec3296fcb
Author: Eugene But <eugenebut@google.com>
Date: Tue Jan 31 00:26:43 2017

Remember user decision for bad ssl certs.

Call registerAllowedCertificate:forHost:status: when user accepted bad
SSL cert in the same way as Request Tracker did:
https://codereview.chromium.org/2648763002/patch/1/10002

BUG=685745

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

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

[modify] https://crrev.com/ce62a87e5eb602484b4abbf117af31cec3296fcb/ios/web/web_state/ui/crw_web_controller.mm

Verified on build 58.0.3004.0 Canary in iPhone6+ iOS 10.2.1 and iPad Pro iOS 10.1.1 User decision for bad ssl certs is remembered.

> Call registerAllowedCertificate:forHost:status: when user accepted bad
> SSL cert in the same way as Request Tracker did:

I don't know much about this, but will it inherit the 7 day behaviour?

I don't know if this inherits the 7 day behaviour. I just plugged in the old code and I don't know if iOS actually ever supported 7 days policy or had something else.

Need clarification: Eugene, is this only remember if the ssl page is in foreground and you force quit the app? Steps from comment#0 are fixed. But a slight modification of steps still shows the problem.

Steps to reproduce: 
1.) Load badssl.com and tap on Expired link
2.) Proceed with interstitial (Advanced -> PROCEED TO EXPIRED.BADSSL.COM)
    Close this tab. Background the app.
3.) Quite Chrome (double tap on home button, flick the app from the list)
4.) Launch Chrome again (this has to be a cold launch)

Navigate to badssl.com and tap on Expired link again.

Interstitial page is displayed again.

Can you conform if this is intended or ssl preference should be remembered even in this scenario.

Comment #21 describes the bug. Assigning to Kurt, who implemented session storage for cert_policy_cache.

This seems to be occurring because closing the app by flicking up from the app switcher is killing the application process before we get a chance to finish serializing the session (along with its certificate policy cache).  I've put some breakpoints in these spots in the code, and it does take a noticeable amount of time for the session to be properly serialized (~3 second delay on iPhone 8 Simulator).  Upon backgrounding the app, we do this without scheduling as a background task, although there is still a very slight delay between tapping the home button to background the app and the actual code being executed (mostly due to the backgrounding animation, I think).  When entering the app switcher, the application state switches from "Active" => "Inactive", but the app is not registered as "Backgrounded".  The repro steps above work because the swipe-up that kills the application process occurs before the background task finishes saving the interstitial decision.

I don't think that this needs to be RBS, as the policy will be saved properly in any case where the user doesn't force-quit the app within 3s of an interstitial decision.  Moreover, reproducing this bug makes the app *more* secure rather than less secure.

One possible solution would be to immediately save the session history from TabModel's |-willResignActive:| instead of |-applicationDidEnterBackground:|, as that would also capture when the user enters the app switcher.  Assigning to Sylvain since he wrote TabModel's serialization hooks.  Is there a reason why we are only saving upon backgrounding?  Is it for performance reasons, or just because we weren't aware that entering the app switcher doesn't trigger a backgrounding notification?

Please note the behavior is same even after waiting for more then 2mins.
Based on my new steps from comment#21, After backgrounding the app I waited for more than 2mins, browsed other apps. Then after 2mins opened App Switcher and then force quit from there. Then Launch Chrome, Navigate to badssl.com tap expired.badssl.com and I see Security Interstitial page.

Okay interesting, it was repro'ing for me consistently when I quit the app quickly.  I'll investigate with pause in the repro steps.

Actually, the repro steps in #21 seems WAI.  We currently persist the interstitial decisions made for each Tab in the list upon backgrounding.  Since the Tab was closed before backgrounding, the certificate decision is not persisted.

Although WAI, this is not really the optimal solution for remembering certificate policy decisions, and is mostly done this way because it is easy to implement.  lgarron@, how are interstitial decisions persisted in content//?  Do you store this decision per hostname for a certain amount of time?

Saving the session is scheduled when one of the following event occurs:
- a new navigation is committed,
- the active tab changes,
- the app is backgrounded.

Is it possible that no navigation happen after the interstitial is presented (maybe because the navigation already completed in the background)? If so, then no session save will be scheduled and killing the app would cause the override to be forgotten.

Regarding scheduling during backgrounding vs presenting the app switcher, it is because saving session is expensive so we want to avoid unnecessary work. A better fix would be to schedule a save when user override the interstitial.

We're already scheduling a session save event after the interstitial's Proceed() is called, since it commits the navigation.  It's deferred, so it's still possible to force quit the app before it's saved, but I think that's an issue we can live with.

The steps in comment #21 will still repro the bug, though, because we only save the cert policy decisions for the WebStates that exist in the session.  Closing the Tab in which we've proceeded past the interstitial will prevent that decision from being serialized.  To fix this, we'd need a serialization mechanism in addition to the CRWSessionStorages currently used, since those are specific to a WebState.  We'd want another storage mechanism at the BrowserState level to handle these policy decisions that should be shared across WebStates.

This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue.

Sorry for the inconvenience if the bug really should have been left as Available.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Lowering priority per comment #28