New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Do not commit subframe navigations for ipfs #14313
Conversation
01e5662
to
8e373d0
Compare
| // account as well. | ||
| // If the request was canceled by the user, do not show an error page. | ||
| if (net::ERR_ABORTED == net_error_ || | ||
| + BRAVE_ONIPFSIPNS_MAYBE_CANCEL_NAVIGATION |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can't we do that via NavigationThrottle ?
86adb1b
to
acf4234
Compare
714c02e
to
f3d0858
Compare
| @@ -965,6 +966,8 @@ BraveContentBrowserClient::CreateThrottlesForNavigation( | |||
| #endif | |||
|
|
|||
| #if BUILDFLAG(ENABLE_IPFS) | |||
| throttles.push_back( | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't you want to push_front it?
| const IpfsSubframeNavigationThrottle&) = delete; | ||
|
|
||
| // content::NavigationThrottle implementation: | ||
| content::NavigationThrottle::ThrottleCheckResult WillFailRequest() override; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add comments pls
|
|
||
| namespace ipfs { | ||
|
|
||
| class IpfsSubframeNavigationThrottle : public content::NavigationThrottle { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
class-level comment
|
/build linux |
Verified
|
| Brave | 1.44.21 Chromium: 104.0.5112.81 (Official Build) nightly (x86_64) |
|---|---|
| Revision | 5b7b76419d50f583022568b6764b630f6ddc9208-refs/branch-heads/5112@{#1309} |
| OS | macOS Version 13.0 (Build 22A5311f) |
Reproduced the crash using 1.42.86, and the privately-provided minimized testcase.
Worked with @cypt4 and used the minimal testcases from both this issue and these others, collected from HackerOne in brave/brave-browser#24093.
Confirmed I was able to open the following HTML testcases in both Private and regular windows, without crashes.
| minimized testcase | testcase 1 | testcase 2 | testcase 3 | testcase 4 | testcase 5 | testcase 6 |
|---|---|---|---|---|---|---|
![]() |
![]() |
![]() |
![]() |
![]() |
![]() |
![]() |
|
@cypt4 @spylogsster can we get an uplift for this started? Am assuming we'll want it at least in 1.43.x, if not also 1.42.x, via a hotfix/maintenance release? |







Fixes brave/brave-browser#24211
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_tests,npm run lint,npm run gn_check,npm run tslintgit rebase master(if needed)Reviewer Checklist:
gnAfter-merge Checklist:
changes has landed on