Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2510003002:
If replacing a safebrowsing interstitial, don't call DontProceed on the old one. (Closed)
DescriptionIf replacing a safebrowsing interstitial, don't call DontProceed on the old one.
Calling DontProceed directly has some side effects like destroying the pending NavigationEntry.
There is already common code in InterstitialPageImpl::Show to handle closing an existing interstitial when showing a new one, and it properly handles the pending NavigationEntry.
BUG=655794
Committed: https://crrev.com/3294450834f7ae8978994a8d2f7f9dd1df3178f2
Cr-Commit-Position: refs/heads/master@{#435445}
Patch Set 1 #Patch Set 2 : set TODO url for crbug.com/666172 #
Total comments: 12
Patch Set 3 : rebase #Patch Set 4 : review changes #
Messages
Total messages: 29 (19 generated)
Description was changed from ========== If replacing a safebrowsing interstitial, don't DontProceed the old one. Calling DontProceed has some side effects like destroying the pending NavigationEntry. Showing the new interstitial will automatically hide the old one anyway. BUG=655794 ========== to ========== If replacing a safebrowsing interstitial, don't DontProceed the old one. There is already common code in InterstitialPageImpl::Show to handle closing an existing interstitial when showing a new one, and it already has code to properly handle the pending NavigationEntry. Calling DontProceed directly has some side effects like destroying the pending NavigationEntry. There is already common code in InterstitialPageImpl::Show to handle closing an existing interstitial when showing a new one, and it poperly handles the pending NavigationEntry. BUG=655794 ==========
Description was changed from ========== If replacing a safebrowsing interstitial, don't DontProceed the old one. There is already common code in InterstitialPageImpl::Show to handle closing an existing interstitial when showing a new one, and it already has code to properly handle the pending NavigationEntry. Calling DontProceed directly has some side effects like destroying the pending NavigationEntry. There is already common code in InterstitialPageImpl::Show to handle closing an existing interstitial when showing a new one, and it poperly handles the pending NavigationEntry. BUG=655794 ========== to ========== If replacing a safebrowsing interstitial, don't DontProceed the old one. Calling DontProceed directly has some side effects like destroying the pending NavigationEntry. There is already common code in InterstitialPageImpl::Show to handle closing an existing interstitial when showing a new one, and it poperly handles the pending NavigationEntry. BUG=655794 ==========
Description was changed from ========== If replacing a safebrowsing interstitial, don't DontProceed the old one. Calling DontProceed directly has some side effects like destroying the pending NavigationEntry. There is already common code in InterstitialPageImpl::Show to handle closing an existing interstitial when showing a new one, and it poperly handles the pending NavigationEntry. BUG=655794 ========== to ========== If replacing a safebrowsing interstitial, don't call DontProceed on the old one. Calling DontProceed directly has some side effects like destroying the pending NavigationEntry. There is already common code in InterstitialPageImpl::Show to handle closing an existing interstitial when showing a new one, and it poperly handles the pending NavigationEntry. BUG=655794 ==========
Description was changed from ========== If replacing a safebrowsing interstitial, don't call DontProceed on the old one. Calling DontProceed directly has some side effects like destroying the pending NavigationEntry. There is already common code in InterstitialPageImpl::Show to handle closing an existing interstitial when showing a new one, and it poperly handles the pending NavigationEntry. BUG=655794 ========== to ========== If replacing a safebrowsing interstitial, don't call DontProceed on the old one. Calling DontProceed directly has some side effects like destroying the pending NavigationEntry. There is already common code in InterstitialPageImpl::Show to handle closing an existing interstitial when showing a new one, and it properly handles the pending NavigationEntry. BUG=655794 ==========
mattm@chromium.org changed reviewers: + nparker@chromium.org
The CQ bit was checked by mattm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
nparker@chromium.org changed reviewers: + meacer@chromium.org
Thanks, Matt. +meacer for review also. I mostly have questions. https://codereview.chromium.org/2510003002/diff/20001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right): https://codereview.chromium.org/2510003002/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:461: // interstitial, showing the new one will automatically hide the old one. What causes the old one to get hidden? And do we want a new one to show at all? It seems weird to have one show, then have another replace it. https://codereview.chromium.org/2510003002/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:471: // This is an interstitial for a page's resource, let's queue it. note: felt mentioned that we no longer queue interstitials and show a sequence of them, that this is effectively dead code. I haven't looked through this recently to see what parts of it are dead.
https://codereview.chromium.org/2510003002/diff/20001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right): https://codereview.chromium.org/2510003002/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:461: // interstitial, showing the new one will automatically hide the old one. On 2016/11/17 20:08:24, Nathan Parker wrote: > What causes the old one to get hidden? https://cs.chromium.org/chromium/src/content/browser/frame_host/interstitial_... > And do we want a new one to show at all? > It seems weird to have one show, then have another replace it. I guess for the reload case, but this is the same path that gets hit if you see a warning, type a different url into the omnibox, and then get a warning for that page. Seems easier to just always display the new one than to add extra code to try to figure out if it's the same as the old one. https://codereview.chromium.org/2510003002/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:471: // This is an interstitial for a page's resource, let's queue it. On 2016/11/17 20:08:24, Nathan Parker wrote: > note: felt mentioned that we no longer queue interstitials and show a sequence > of them, that this is effectively dead code. I haven't looked through this > recently to see what parts of it are dead. That's doesn't seem to be entirely true. I think what was actually killed was the multi-threat interstitial. This code is still active. So it will actually queue up the resources and show a second interstitial, it'll just be another regular interstitial instead of a multi-threat interstitial. (I just verified this.) I don't know if this was the intended result or if this should be killed too...
On 2016/11/17 20:08:25, Nathan Parker wrote: > Thanks, Matt. > > +meacer for review also. ping meacer
Sorry for the delay. Lgtm with nits. https://codereview.chromium.org/2510003002/diff/20001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc (right): https://codereview.chromium.org/2510003002/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:925: GURL url = SetupWarningAndNavigate(); nit: const? https://codereview.chromium.org/2510003002/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:936: NavigationController& controller = tab->GetController(); const https://codereview.chromium.org/2510003002/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:938: EXPECT_EQ(controller.GetVisibleEntry()->GetURL(), url); nit: The order of params seems reversed here. url is the expected value, so it should come first. Same below. https://codereview.chromium.org/2510003002/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:949: // ExpectSecurityIndicatorDowngrade(tab, 0u); Is this intentionally commented out? If so, you might want to point that out. E.g. // Call |ExpectSecurityIndicatorDowngrade(tab, 0u);| here once the bug is fixed.
The CQ bit was checked by mattm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks. Nathan did you have any further comments? https://codereview.chromium.org/2510003002/diff/20001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc (right): https://codereview.chromium.org/2510003002/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:925: GURL url = SetupWarningAndNavigate(); On 2016/11/30 00:04:09, Mustafa Emre Acer wrote: > nit: const? Done. https://codereview.chromium.org/2510003002/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:936: NavigationController& controller = tab->GetController(); On 2016/11/30 00:04:09, Mustafa Emre Acer wrote: > const Done. https://codereview.chromium.org/2510003002/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:938: EXPECT_EQ(controller.GetVisibleEntry()->GetURL(), url); On 2016/11/30 00:04:09, Mustafa Emre Acer wrote: > nit: The order of params seems reversed here. url is the expected value, so it > should come first. Same below. Done. https://codereview.chromium.org/2510003002/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:949: // ExpectSecurityIndicatorDowngrade(tab, 0u); On 2016/11/30 00:04:09, Mustafa Emre Acer wrote: > Is this intentionally commented out? If so, you might want to point that out. > E.g. > > // Call |ExpectSecurityIndicatorDowngrade(tab, 0u);| here once the bug is fixed. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
lgtm
The CQ bit was checked by mattm@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from meacer@chromium.org Link to the patchset: https://codereview.chromium.org/2510003002/#ps60001 (title: "review changes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1480542303834120,
"parent_rev": "235711bd070d22eb737b70e58bdaee3a5c759320", "commit_rev":
"3fe6549586cf0c1bcfb1d7030ff56f3a0741dd21"}
Message was sent while issue was closed.
Description was changed from ========== If replacing a safebrowsing interstitial, don't call DontProceed on the old one. Calling DontProceed directly has some side effects like destroying the pending NavigationEntry. There is already common code in InterstitialPageImpl::Show to handle closing an existing interstitial when showing a new one, and it properly handles the pending NavigationEntry. BUG=655794 ========== to ========== If replacing a safebrowsing interstitial, don't call DontProceed on the old one. Calling DontProceed directly has some side effects like destroying the pending NavigationEntry. There is already common code in InterstitialPageImpl::Show to handle closing an existing interstitial when showing a new one, and it properly handles the pending NavigationEntry. BUG=655794 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== If replacing a safebrowsing interstitial, don't call DontProceed on the old one. Calling DontProceed directly has some side effects like destroying the pending NavigationEntry. There is already common code in InterstitialPageImpl::Show to handle closing an existing interstitial when showing a new one, and it properly handles the pending NavigationEntry. BUG=655794 ========== to ========== If replacing a safebrowsing interstitial, don't call DontProceed on the old one. Calling DontProceed directly has some side effects like destroying the pending NavigationEntry. There is already common code in InterstitialPageImpl::Show to handle closing an existing interstitial when showing a new one, and it properly handles the pending NavigationEntry. BUG=655794 Committed: https://crrev.com/3294450834f7ae8978994a8d2f7f9dd1df3178f2 Cr-Commit-Position: refs/heads/master@{#435445} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/3294450834f7ae8978994a8d2f7f9dd1df3178f2 Cr-Commit-Position: refs/heads/master@{#435445} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
