Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(643)

Issue 2889003002: Change NavigationEntry to use virtual URL in error pages for blocked navigations (Closed)

Created:
3 years, 7 months ago by nasko
Modified:
3 years, 7 months ago
Reviewers:
Charlie Reis, jam, Devlin
CC:
chromium-reviews, jam, nasko+codewatch_chromium.org, darin-cc_chromium.org, creis+watch_chromium.org, alexmos
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Change NavigationEntry to use virtual URL in error pages for blocked navigations Error pages today commit with the original URL in the NavigationEntry. This means that on history navigations/reload, the original URL will be used. However, when navigations are blocked, we shouldn't be allowing the blocked URL to be set as the NavigationEntry URL. There has been a long standing desire to change this aspect of error pages and this CL is a small step in that direction. The goal of this CL is to classify error pages due to blocked navigations and change the URL of the NavigationEntry to be a safe one - "about:blank". To preserve the existing UX, though, the originally blocked URL is set as the virtual URL on the NavigationEntry. This allows history navigations/reload to end up at about:blank, which is much safer behavior than allowing the load to the blocked URL. BUG=723796 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2889003002 Cr-Commit-Position: refs/heads/master@{#474535} Committed: https://chromium.googlesource.com/chromium/src/+/5ff4eeaefbc41f51d02887e0c42bb6e72ba292a0

Patch Set 1 #

Patch Set 2 : Speculative implementation of blocked navigations. #

Patch Set 3 : Fix failing PlzNavigate test, added more error codes. #

Patch Set 4 : Add a test for reload. #

Total comments: 22

Patch Set 5 : Address review comments. #

Patch Set 6 : Remove some error codes and add a TODO. #

Total comments: 1

Patch Set 7 : Remove reload test since it isn't accessible cross-origin. #

Total comments: 3

Patch Set 8 : Address review comments. #

Patch Set 9 : Add back reload test (PlzNavigate only) and address review comments. #

Total comments: 2

Patch Set 10 : Rebase. #

Patch Set 11 : Improve the extensions check. #

Patch Set 12 : rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+206 lines, -4 lines) Patch
M chrome/browser/extensions/navigation_observer.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +10 lines, -1 line 0 comments Download
M content/browser/browser_side_navigation_browsertest.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +41 lines, -0 lines 0 comments Download
M content/browser/frame_host/data_url_navigation_browsertest.cc View 1 2 3 4 1 chunk +64 lines, -0 lines 0 comments Download
M content/browser/frame_host/navigation_controller_impl.cc View 1 2 3 4 5 6 7 2 chunks +38 lines, -0 lines 0 comments Download
M content/browser/frame_host/navigation_controller_impl_browsertest.cc View 1 2 3 4 5 6 7 1 chunk +43 lines, -0 lines 0 comments Download
M content/browser/frame_host/navigation_handle_impl_browsertest.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +5 lines, -3 lines 0 comments Download
M content/test/data/data_url_navigations.html View 1 chunk +5 lines, -0 lines 0 comments Download

Messages

Total messages: 54 (37 generated)
nasko
Hey Charlie, Can you review this CL for me? It implements the solution we discussed ...
3 years, 7 months ago (2017-05-18 21:19:30 UTC) #11
Charlie Reis
Thanks! I'm very excited to get better protection against blocked navigations. A few thoughts and ...
3 years, 7 months ago (2017-05-18 22:41:46 UTC) #12
nasko
https://codereview.chromium.org/2889003002/diff/60001/content/browser/frame_host/data_url_navigation_browsertest.cc File content/browser/frame_host/data_url_navigation_browsertest.cc (right): https://codereview.chromium.org/2889003002/diff/60001/content/browser/frame_host/data_url_navigation_browsertest.cc#newcode960 content/browser/frame_host/data_url_navigation_browsertest.cc:960: // The window.open() should have resulted in an error ...
3 years, 7 months ago (2017-05-18 22:55:11 UTC) #13
Charlie Reis
Great. Next step is investigating the test and chatting with net folks. If we keep ...
3 years, 7 months ago (2017-05-18 23:15:51 UTC) #18
nasko
https://codereview.chromium.org/2889003002/diff/60001/content/browser/frame_host/navigation_controller_impl.cc File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/2889003002/diff/60001/content/browser/frame_host/navigation_controller_impl.cc#newcode908 content/browser/frame_host/navigation_controller_impl.cc:908: return false; On 2017/05/18 23:15:51, Charlie Reis wrote: > ...
3 years, 7 months ago (2017-05-19 18:30:52 UTC) #23
nasko
Hey Charlie, Can you review this CL for sign off? It should be ready to ...
3 years, 7 months ago (2017-05-24 16:01:26 UTC) #30
Charlie Reis
Great! Basically looks good, but I have a concern about the removed reload test. If ...
3 years, 7 months ago (2017-05-24 16:42:11 UTC) #31
nasko
Added the reload test in PlzNavigate mode only, as reloads there behave much more sanely. ...
3 years, 7 months ago (2017-05-24 20:46:29 UTC) #34
nasko
Hey Devlin, Would you mind stamping chrome/browser/extensions/navigation_observer.cc for OWNERS, since we discussed this fix offline? ...
3 years, 7 months ago (2017-05-24 20:55:56 UTC) #36
Devlin
chrome/browser/extensions/navigation_observer.cc lgtm!
3 years, 7 months ago (2017-05-24 21:15:50 UTC) #39
Charlie Reis
The extension change makes me wonder whether other code will break when we alter GetURL() ...
3 years, 7 months ago (2017-05-24 21:50:52 UTC) #40
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2889003002/220001
3 years, 7 months ago (2017-05-24 22:16:23 UTC) #44
nasko
https://codereview.chromium.org/2889003002/diff/160001/chrome/browser/extensions/navigation_observer.cc File chrome/browser/extensions/navigation_observer.cc (right): https://codereview.chromium.org/2889003002/diff/160001/chrome/browser/extensions/navigation_observer.cc#newcode76 chrome/browser/extensions/navigation_observer.cc:76: const GURL& url = nav_entry->GetVirtualURL().is_empty() On 2017/05/24 21:50:52, Charlie ...
3 years, 7 months ago (2017-05-24 22:16:42 UTC) #45
Charlie Reis
Thanks, LGTM
3 years, 7 months ago (2017-05-24 22:20:45 UTC) #46
commit-bot: I haz the power
Failed to apply patch for content/browser/browser_side_navigation_browsertest.cc: While running git apply --index -3 -p1; error: patch ...
3 years, 7 months ago (2017-05-25 00:23:47 UTC) #48
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2889003002/240001
3 years, 7 months ago (2017-05-25 00:55:48 UTC) #51
commit-bot: I haz the power
3 years, 7 months ago (2017-05-25 02:51:31 UTC) #54
Message was sent while issue was closed.
Committed patchset #12 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/5ff4eeaefbc41f51d02887e0c42b...

Powered by Google App Engine
This is Rietveld 408576698