Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1207943002:
Trigger DidChangeVisisbleSSLState() after OverrideEntry() (Closed)
|
Created:
5 years, 6 months ago by estark Modified:
5 years, 5 months ago CC:
chromium-reviews, jam, nasko+codewatch_chromium.org, creis+watch_chromium.org, darin-cc_chromium.org, felt Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionTrigger DidChangeVisisbleSSLState() after OverrideEntry()
While SSLBlockingPage::OverrideEntry() changes the visible SSL status of
the page, WebContentsImpl::DidChangeVisibleSSLState() never gets
called. This breaks the devtools security panel on interstitials and has
caused other bugs in the past (e.g. crbug.com/474795).
BUG=503112
Committed: https://crrev.com/c95446759f5c9eff845f36c830b1ef87b768c3fa
Cr-Commit-Position: refs/heads/master@{#336683}
Patch Set 1 #
Total comments: 11
Patch Set 2 : nasko comments #Patch Set 3 : rebase #Patch Set 4 : test tweak #Patch Set 5 : test fixes #
Total comments: 1
Messages
Total messages: 25 (9 generated)
estark@chromium.org changed reviewers: + nasko@chromium.org
nasko, could you PTAL? The problem this is solving is that WebContentsImpl::VisibleSSLStateChanged() is not getting called when an interstitial is shown, even though the visible SSL state changes. This means that the devtools security panel doesn't get the current lock icon on an interstitial (VisibleSSLStateChanged() is what triggers the lock icon being sent to devtools). felt pointed out that this issue has come up before, in crbug.com/474795, and no one seemed to know why VisibleSSLStateChanged() should not get called when an interstitial is shown. https://codereview.chromium.org/1207943002/diff/1/content/browser/frame_host/... File content/browser/frame_host/interstitial_page_impl.cc (right): https://codereview.chromium.org/1207943002/diff/1/content/browser/frame_host/... content/browser/frame_host/interstitial_page_impl.cc:247: static_cast<WebContentsImpl*>(web_contents_)->DidChangeVisibleSSLState(); To avoid unnecessary repainting, we could return an argument from OverrideEntry() indicating whether the visible SSL state has been changed, and only call DidChangeVisibleSSLState() when the argument is true. Not sure if that's necessary (i.e. how much we care about the extra repaints).
https://codereview.chromium.org/1207943002/diff/1/chrome/browser/ui/browser_b... File chrome/browser/ui/browser_browsertest.cc (right): https://codereview.chromium.org/1207943002/diff/1/chrome/browser/ui/browser_b... chrome/browser/ui/browser_browsertest.cc:2871: EXPECT_EQ(content::SECURITY_STYLE_AUTHENTICATION_BROKEN, We expect to have an interstitial page being displayed at this time, correct? If so, let's add an EXPECT to ensure that's the case. Also, some comments on what is expected and most importantly why, are very useful when you come back a year from now looking at the code :). https://codereview.chromium.org/1207943002/diff/1/chrome/browser/ui/browser_b... chrome/browser/ui/browser_browsertest.cc:2877: nit: Why the extra empty line here? https://codereview.chromium.org/1207943002/diff/1/chrome/browser/ui/browser_b... chrome/browser/ui/browser_browsertest.cc:2879: observer.latest_security_style()); Do we have a test that ensures that if the user types a different URL or navigates back after the interstitial is shown (but not proceeded through), the SSL indicators are properly updated? https://codereview.chromium.org/1207943002/diff/1/content/browser/frame_host/... File content/browser/frame_host/interstitial_page_impl.cc (right): https://codereview.chromium.org/1207943002/diff/1/content/browser/frame_host/... content/browser/frame_host/interstitial_page_impl.cc:247: static_cast<WebContentsImpl*>(web_contents_)->DidChangeVisibleSSLState(); On 2015/06/25 00:35:47, estark wrote: > To avoid unnecessary repainting, we could return an argument from > OverrideEntry() indicating whether the visible SSL state has been changed, and > only call DidChangeVisibleSSLState() when the argument is true. Not sure if > that's necessary (i.e. how much we care about the extra repaints). In general, we do care about repaints, as we are trying to have a smooth UX on all platforms, regardless of how powerful they are. However, looking at the code in the current delegate, it is totally unconditional, so this will always have to be called. I'd leave it up to you to decide whether to change it to return a boolean or not.
Thanks nasko. https://codereview.chromium.org/1207943002/diff/1/chrome/browser/ui/browser_b... File chrome/browser/ui/browser_browsertest.cc (right): https://codereview.chromium.org/1207943002/diff/1/chrome/browser/ui/browser_b... chrome/browser/ui/browser_browsertest.cc:2871: EXPECT_EQ(content::SECURITY_STYLE_AUTHENTICATION_BROKEN, On 2015/06/25 12:59:32, nasko (paris) wrote: > We expect to have an interstitial page being displayed at this time, correct? If > so, let's add an EXPECT to ensure that's the case. > Also, some comments on what is expected and most importantly why, are very > useful when you come back a year from now looking at the code :). Done. https://codereview.chromium.org/1207943002/diff/1/chrome/browser/ui/browser_b... chrome/browser/ui/browser_browsertest.cc:2877: On 2015/06/25 12:59:32, nasko (paris) wrote: > nit: Why the extra empty line here? Done. https://codereview.chromium.org/1207943002/diff/1/chrome/browser/ui/browser_b... chrome/browser/ui/browser_browsertest.cc:2879: observer.latest_security_style()); On 2015/06/25 12:59:32, nasko (paris) wrote: > Do we have a test that ensures that if the user types a different URL or > navigates back after the interstitial is shown (but not proceeded through), the > SSL indicators are properly updated? Going back (as in "clicking the back button") doesn't seem to actually work for interstitials. In a new tab, https://expired.badssl.com -> https://badssl.com -> click Back takes you back to the new tab page. It's already filed (crbug.com/311500) and I assume this is one of the things that'll be fixed by the interstitial refactor. In the meantime, I added a test that navigates away from the interstitial to a different URL and then navigates back to the interstitial by navigating to the bad https url again. https://codereview.chromium.org/1207943002/diff/1/content/browser/frame_host/... File content/browser/frame_host/interstitial_page_impl.cc (right): https://codereview.chromium.org/1207943002/diff/1/content/browser/frame_host/... content/browser/frame_host/interstitial_page_impl.cc:247: static_cast<WebContentsImpl*>(web_contents_)->DidChangeVisibleSSLState(); On 2015/06/25 12:59:32, nasko (paris) wrote: > On 2015/06/25 00:35:47, estark wrote: > > To avoid unnecessary repainting, we could return an argument from > > OverrideEntry() indicating whether the visible SSL state has been changed, and > > only call DidChangeVisibleSSLState() when the argument is true. Not sure if > > that's necessary (i.e. how much we care about the extra repaints). > > In general, we do care about repaints, as we are trying to have a smooth UX on > all platforms, regardless of how powerful they are. However, looking at the code > in the current delegate, it is totally unconditional, so this will always have > to be called. I'd leave it up to you to decide whether to change it to return a > boolean or not. Hmm, I think I'll leave it as is. I was thinking that we need not fire the event on interstitials that use delegates other than SSLBlockingPage (e.g. malware interstitial, which I imagine uses the default OverrideEntry() implementation that does nothing). But after thinking about it more, I think we do always want to fire the event. The malware interstitial (e.g. https://ianfette.org) shows up as the neutral http:// page icon, and so we want observers to get notified of that neutral icon. So I guess what I'm saying is that, if we didn't fire VisibleSSLStateChanged() unconditionally here, we'd have a bug where devtools security panel shows the lock icon of the previous page when you're on the malware interstitial. FWIW, it looks like the normal navigation path also repaints the toolbar twice -- once by calling SSLManager::DidCommitProvisionalLoad() which calls DiChangeVisibleSSLState(), and once by calling NotifyNavigationStateChanged() -- so we're not making interstitial repaints any worse than normal navigations.
Thanks nasko. https://codereview.chromium.org/1207943002/diff/1/chrome/browser/ui/browser_b... File chrome/browser/ui/browser_browsertest.cc (right): https://codereview.chromium.org/1207943002/diff/1/chrome/browser/ui/browser_b... chrome/browser/ui/browser_browsertest.cc:2871: EXPECT_EQ(content::SECURITY_STYLE_AUTHENTICATION_BROKEN, On 2015/06/25 12:59:32, nasko (paris) wrote: > We expect to have an interstitial page being displayed at this time, correct? If > so, let's add an EXPECT to ensure that's the case. > Also, some comments on what is expected and most importantly why, are very > useful when you come back a year from now looking at the code :). Done. https://codereview.chromium.org/1207943002/diff/1/chrome/browser/ui/browser_b... chrome/browser/ui/browser_browsertest.cc:2877: On 2015/06/25 12:59:32, nasko (paris) wrote: > nit: Why the extra empty line here? Done. https://codereview.chromium.org/1207943002/diff/1/chrome/browser/ui/browser_b... chrome/browser/ui/browser_browsertest.cc:2879: observer.latest_security_style()); On 2015/06/25 12:59:32, nasko (paris) wrote: > Do we have a test that ensures that if the user types a different URL or > navigates back after the interstitial is shown (but not proceeded through), the > SSL indicators are properly updated? Going back (as in "clicking the back button") doesn't seem to actually work for interstitials. In a new tab, https://expired.badssl.com -> https://badssl.com -> click Back takes you back to the new tab page. It's already filed (crbug.com/311500) and I assume this is one of the things that'll be fixed by the interstitial refactor. In the meantime, I added a test that navigates away from the interstitial to a different URL and then navigates back to the interstitial by navigating to the bad https url again. https://codereview.chromium.org/1207943002/diff/1/content/browser/frame_host/... File content/browser/frame_host/interstitial_page_impl.cc (right): https://codereview.chromium.org/1207943002/diff/1/content/browser/frame_host/... content/browser/frame_host/interstitial_page_impl.cc:247: static_cast<WebContentsImpl*>(web_contents_)->DidChangeVisibleSSLState(); On 2015/06/25 12:59:32, nasko (paris) wrote: > On 2015/06/25 00:35:47, estark wrote: > > To avoid unnecessary repainting, we could return an argument from > > OverrideEntry() indicating whether the visible SSL state has been changed, and > > only call DidChangeVisibleSSLState() when the argument is true. Not sure if > > that's necessary (i.e. how much we care about the extra repaints). > > In general, we do care about repaints, as we are trying to have a smooth UX on > all platforms, regardless of how powerful they are. However, looking at the code > in the current delegate, it is totally unconditional, so this will always have > to be called. I'd leave it up to you to decide whether to change it to return a > boolean or not. Hmm, I think I'll leave it as is. I was thinking that we need not fire the event on interstitials that use delegates other than SSLBlockingPage (e.g. malware interstitial, which I imagine uses the default OverrideEntry() implementation that does nothing). But after thinking about it more, I think we do always want to fire the event. The malware interstitial (e.g. https://ianfette.org) shows up as the neutral http:// page icon, and so we want observers to get notified of that neutral icon. So I guess what I'm saying is that, if we didn't fire VisibleSSLStateChanged() unconditionally here, we'd have a bug where devtools security panel shows the lock icon of the previous page when you're on the malware interstitial. FWIW, it looks like the normal navigation path also repaints the toolbar twice -- once by calling SSLManager::DidCommitProvisionalLoad() which calls DiChangeVisibleSSLState(), and once by calling NotifyNavigationStateChanged() -- so we're not making interstitial repaints any worse than normal navigations.
LGTM https://codereview.chromium.org/1207943002/diff/1/chrome/browser/ui/browser_b... File chrome/browser/ui/browser_browsertest.cc (right): https://codereview.chromium.org/1207943002/diff/1/chrome/browser/ui/browser_b... chrome/browser/ui/browser_browsertest.cc:2879: observer.latest_security_style()); On 2015/06/25 14:54:44, estark wrote: > On 2015/06/25 12:59:32, nasko (paris) wrote: > > Do we have a test that ensures that if the user types a different URL or > > navigates back after the interstitial is shown (but not proceeded through), > the > > SSL indicators are properly updated? > > Going back (as in "clicking the back button") doesn't seem to actually work for > interstitials. In a new tab, https://expired.badssl.com -> https://badssl.com -> > click Back takes you back to the new tab page. It's already filed > (crbug.com/311500) and I assume this is one of the things that'll be fixed by > the interstitial refactor. > > In the meantime, I added a test that navigates away from the interstitial to a > different URL and then navigates back to the interstitial by navigating to the > bad https url again. I meant this one indeed. Thanks for adding it! https://codereview.chromium.org/1207943002/diff/1/content/browser/frame_host/... File content/browser/frame_host/interstitial_page_impl.cc (right): https://codereview.chromium.org/1207943002/diff/1/content/browser/frame_host/... content/browser/frame_host/interstitial_page_impl.cc:247: static_cast<WebContentsImpl*>(web_contents_)->DidChangeVisibleSSLState(); On 2015/06/25 14:54:45, estark wrote: > On 2015/06/25 12:59:32, nasko (paris) wrote: > > On 2015/06/25 00:35:47, estark wrote: > > > To avoid unnecessary repainting, we could return an argument from > > > OverrideEntry() indicating whether the visible SSL state has been changed, > and > > > only call DidChangeVisibleSSLState() when the argument is true. Not sure if > > > that's necessary (i.e. how much we care about the extra repaints). > > > > In general, we do care about repaints, as we are trying to have a smooth UX on > > all platforms, regardless of how powerful they are. However, looking at the > code > > in the current delegate, it is totally unconditional, so this will always have > > to be called. I'd leave it up to you to decide whether to change it to return > a > > boolean or not. > > Hmm, I think I'll leave it as is. I was thinking that we need not fire the event > on interstitials that use delegates other than SSLBlockingPage (e.g. malware > interstitial, which I imagine uses the default OverrideEntry() implementation > that does nothing). But after thinking about it more, I think we do always want > to fire the event. The malware interstitial (e.g. https://ianfette.org) shows up > as the neutral http:// page icon, and so we want observers to get notified of > that neutral icon. So I guess what I'm saying is that, if we didn't fire > VisibleSSLStateChanged() unconditionally here, we'd have a bug where devtools > security panel shows the lock icon of the previous page when you're on the > malware interstitial. > > FWIW, it looks like the normal navigation path also repaints the toolbar twice > -- once by calling SSLManager::DidCommitProvisionalLoad() which calls > DiChangeVisibleSSLState(), and once by calling NotifyNavigationStateChanged() -- > so we're not making interstitial repaints any worse than normal navigations. Thanks for digging out the details and for the thorough explanation!
The CQ bit was checked by nasko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1207943002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Exceeded global retry quota
The CQ bit was checked by felt@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nasko@chromium.org Link to the patchset: https://codereview.chromium.org/1207943002/#ps60001 (title: "test tweak")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1207943002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
estark@chromium.org changed reviewers: + msw@chromium.org
msw, could you please review //chrome/browser/ui?
lgtm with an optional nit https://codereview.chromium.org/1207943002/diff/80001/chrome/browser/ui/brows... File chrome/browser/ui/browser_browsertest.cc (right): https://codereview.chromium.org/1207943002/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_browsertest.cc:374: latest_explanations_.warning_explanations.clear(); nit: maybe just do latest_explanations_ = SecurityStyleExplanations();
On 2015/06/29 at 20:02:08, msw wrote: > lgtm with an optional nit > > https://codereview.chromium.org/1207943002/diff/80001/chrome/browser/ui/brows... > File chrome/browser/ui/browser_browsertest.cc (right): > > https://codereview.chromium.org/1207943002/diff/80001/chrome/browser/ui/brows... > chrome/browser/ui/browser_browsertest.cc:374: latest_explanations_.warning_explanations.clear(); > nit: maybe just do latest_explanations_ = SecurityStyleExplanations(); msw@: Emily is OOO until next week, but it would be useful for me to be able to work with this fix on trunk/tip of tree. Would you be alright with leaving latest_explanations_ for now and letting me check the CQ bit?
On 2015/06/29 21:35:57, lgarron wrote: > On 2015/06/29 at 20:02:08, msw wrote: > > lgtm with an optional nit > > > > > https://codereview.chromium.org/1207943002/diff/80001/chrome/browser/ui/brows... > > File chrome/browser/ui/browser_browsertest.cc (right): > > > > > https://codereview.chromium.org/1207943002/diff/80001/chrome/browser/ui/brows... > > chrome/browser/ui/browser_browsertest.cc:374: > latest_explanations_.warning_explanations.clear(); > > nit: maybe just do latest_explanations_ = SecurityStyleExplanations(); > > msw@: Emily is OOO until next week, but it would be useful for me to be able to > work with this fix on trunk/tip of tree. > Would you be alright with leaving latest_explanations_ for now and letting me > check the CQ bit? Yeah, go ahead.
The CQ bit was checked by estark@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nasko@chromium.org Link to the patchset: https://codereview.chromium.org/1207943002/#ps80001 (title: "test fixes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1207943002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/c95446759f5c9eff845f36c830b1ef87b768c3fa Cr-Commit-Position: refs/heads/master@{#336683} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
