Looks like this is caused because the code relies on the new interstitial being shown in order to get rid of the old one (i.e. there is no explicit call to destroy the old one).

Adding a manual call to DontProceed() on the old one gets rid of the bug, but causes the previous page to be flashed on the screen before the interstitial comes up again, seems that manually calling OnDontProceed() (without running the rest of the code that's in DontProceed()) would solve the problem.

This bug also doesn't seem to be specific to reloads, it also shows if you navigate from a safe browsing interstitial to another website that would show one. For example if you are seeing the interstitial for http://testsafebrowsing.appspot.com/s/phishing.html and try to go to http://testsafebrowsing.appspot.com/s/malware.html the icon on the second interstitial will be the gray i.

A bit more on the root cause of this, it seems to be happening because the add to the WhitelistUrlSet gets called from DisplayBlockingPage, which gets called from the url_checker, meanwhile the call to remove from the whitelist happens in OnBlockingPageDone, which gets called from OnDontProceed, which in the case of a reload, or a move from interstitial to interstitial happens in the Show method of the new interstitial. Since the url_checker runs before the call to show(), that causes the wrong order between add/remove to whitelist.

Possible solutions (that don't modify calls for other types of interstitials):
1) Change where the calls to add/remove from whitelist happen.
2) Manually call onDontProceed in the specific case where the page is currently a safe browsing interstitial and a new one is being loaded.
3) Special case the remove from whitelist call, so it doesn't run if the new site is also triggering a safe browsing interstitial (Simplest solution, but sounds hacky)

Tricky! Thanks for looking into this. Here's another idea which I haven't fully thought through yet but might work: I wonder if what we really want to do is associate pending URLs with the blocking page that added them to the pending whitelist URL set.

So the |pending_| map in WhitelistUrlSet would actually be a map from a URL to a list of <BaseBlockingPage*, SBThreatType> pairs. InsertPending and RemovePending would take a BaseBlockingPage* as arguments and would insert/remove from the list instead of removing the URL entirely.

That way, when we hide a blocking page due to a refresh or a navigation to another interstitial, we would remove the pending URL corresponding to that particular blocking page, leaving intact the pending URL that the new interstitial added.

Do you think that would work?

Started working on that fix (keeping a pointer to the BaseBlockingPage), however in most cases where add/remove is called we don't have an instance of the BaseBlockingPage, so it would require some larger changes to the code in order to save a pointer to the blocking page object (and I couldn't figure out how to do it for ChromePasswordProtectionService, since it doesn't seem it uses a blocking page). 

What I thought was to keep a counter of how many times a website has been added (i.e. the map is now from url to pair<SBThreatType, int>), and when removing, the count gets reduced by one, and if it gets to 0 the site is removed from the map. The SBThreatType just gets updated to the last added on every add, since that would be the valid one (since everytime the count reaches 2, we know the older threat type is now stale).

Does this make sense? I am running the tests in trybot now, and plan to send it for review if it passes.

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

commit d54f696d8df0053e47442be48219fa4ebb77e5d7
Author: Carlos IL <carlosil@chromium.org>
Date: Thu Sep 21 02:40:51 2017

WhitelistURLSet now uses count for adding/removing

WhitelistURLSet keeps a count of how many times a URL has been added to
the pending whitelist, and when removing, it decreases the count by one
, onlyremoving a URL if the count reaches zero. This solves a problem
where upon reloading a safe browsing interstitial, the website would be
re-added and removed from the whitelist in the wrong order, causing it
to not be in the whitelist when it was supposed to.

Bug:  666172 
Change-Id: Ic870153381685890b5c3bc99ad2133837ffe34d7
Reviewed-on: https://chromium-review.googlesource.com/671672
Commit-Queue: Carlos IL <carlosil@chromium.org>
Reviewed-by: Jialiu Lin <jialiul@chromium.org>
Cr-Commit-Position: refs/heads/master@{#503335}
[modify] https://crrev.com/d54f696d8df0053e47442be48219fa4ebb77e5d7/chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc
[modify] https://crrev.com/d54f696d8df0053e47442be48219fa4ebb77e5d7/components/safe_browsing/base_ui_manager.cc