Prevent the opening of a new Tor tab on every renderer initiated navigation. #20125
Conversation
boocmp
force-pushed
the
issues/32706
branch
from
2d6be9f to
b4324c1
Compare
|
looks like some related tests are failing: https://ci.brave.com/job/pr-brave-browser-issues-32706-linux-x64/5/testReport/ |
boocmp
force-pushed
the
issues/32706
branch
from
b2be452 to
9ca8efa
Compare
| friend class WebContentsUserData; | ||
|
|
||
| // Used only for pointer comparasion. | ||
| const raw_ptr<content::WebContents, DanglingUntriaged> |
There was a problem hiding this comment.
do you really need to have Dangling trait here? It should be safe to compare even a dangling pointer. see UnsafelyUnwrapPtrForComparison.
chromium_src/chrome/browser/renderer_context_menu/render_view_context_menu.cc
Show resolved
Hide resolved
| Profile::FromBrowserContext(web_contents->GetBrowserContext()); | ||
| TorProfileManager::SwitchToTorProfile(profile, | ||
| base::BindRepeating(&OnTorProfileCreated, std::move(onion_location))); | ||
| content::WebContents* context, |
There was a problem hiding this comment.
context intentional? why not web_contents?
| @@ -86,7 +89,8 @@ OnionLocationNavigationThrottle::WillProcessResponse() { | |||
| } | |||
| // If user prefers opening it automatically | |||
| if (pref_service_->GetBoolean(prefs::kAutoOnionRedirect)) { | |||
| delegate_->OpenInTorWindow(navigation_handle()->GetWebContents(), url); | |||
| delegate_->OpenInTorWindow(navigation_handle()->GetWebContents(), url, | |||
| false); | |||
There was a problem hiding this comment.
@darkdh @fmarier why do we keep loading the page via https here? Spec says:
a) If the user has enabled automatic Onion-Location redirects
the header is equivalent to a redirect with a Refresh header and a
timeout of 0 seconds [1]. As an example: the header in 2.1 would be
treated like a `Refresh: 0;URL='http://vwc43ag5jyewlfgf.onion'` header.
which means the https loading should stop. In our case we cannot reuse the same tab for .onion URL by adding Refresh header, but I think we should stop the loading here.
I think we can technically do a redirect via throttler(?) which should just close/stop the current tab and open Tor window with the .onion URL.
There was a problem hiding this comment.
I must missed that when implementing it.
Will content::NavigationThrottle::BLOCK_RESPONSE suffice?
There was a problem hiding this comment.
Yes, it will, I can easily add this change.
Resolves brave/brave-browser#32706
Submitter Checklist:
QA/YesorQA/No;release-notes/includeorrelease-notes/exclude;OS/...) to the associated issuenpm run test -- brave_browser_tests,npm run test -- brave_unit_testswikinpm run lint,npm run presubmitwiki,npm run gn_check,npm run tslintgit rebase master(if needed)Reviewer Checklist:
gnAfter-merge Checklist:
changes has landed on
Test Plan: