Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix loading brave crash url doesn't cause renderer crash #2229

Merged
merged 2 commits into from
Apr 24, 2019
Merged

Fix loading brave crash url doesn't cause renderer crash #2229

merged 2 commits into from
Apr 24, 2019

Conversation

simonhong
Copy link
Member

@simonhong simonhong commented Apr 16, 2019 •
edited
Loading

Send converted debug url to renderer instead of user typed url.
Also convert to chrome scheme more earlier when browser initiated navigation starts.
With this earlier converting, IsRendererDebugURL() can check with converted url.

Fix brave/brave-browser#4111

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Verified that these changes build without errors on
    • Windows
    • macOS
    • Linux
  • Verified that these changes pass automated tests (npm test brave_unit_tests && npm test brave_browser_tests && npm run test-security) on
    • Windows
    • macOS
    • Linux
  • Verified that all lint errors/warnings are resolved (npm run lint)
  • Ran git rebase master (if needed).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.
  • Request a security/privacy review as needed.
  • Add appropriate QA labels (QA/Yes or QA/No) to include the closed issue in milestone

Test Plan:

npm test brave_browser_tests --filter=BraveSchemeLoadBrowserTest.CrashURLTest

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source

@simonhong simonhong added this to the 0.65.x - Nightly milestone Apr 16, 2019
@simonhong simonhong requested a review from bridiver April 16, 2019 03:44
@simonhong simonhong self-assigned this Apr 16, 2019
@simonhong simonhong force-pushed the fix_brave_crash_url branch 4 times, most recently from b66ef31 to fc2dcfb Compare April 16, 2019 09:57
@kjozwiak
Copy link
Member

@bsclifton @rebron the above fixes the brave://crash issue that was found by @LaurenWags. When this gets reviewed/merged into nightly, probably worth uplifting this into 0.63.x.

bridiver
bridiver reviewed Apr 16, 2019
View reviewed changes
patches/content-public-common-url_utils.cc.patch Outdated
if (url.SchemeIs(url::kJavaScriptScheme))
return true;

+#if defined(BRAVE_CHROMIUM_BUILD)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused, this is not the change we talked about

bridiver
bridiver reviewed Apr 16, 2019
View reviewed changes
chromium_src/content/public/common/url_utils.cc Outdated
const char kBraveUIShorthangURL[] = "brave://shorthang/";

bool IsBraveRendererDebugURL(const GURL& url) {
if (url == kBraveUICheckCrashURL || url == kBraveUIBadCastCrashURL ||
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is the change we want anyway, but why would we do all this when we could just convert chrome -> brave and call IsRendererDebugURL?

bridiver
bridiver requested changes Apr 16, 2019
View reviewed changes
Copy link
Collaborator

@bridiver bridiver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see comments

bridiver
bridiver reviewed Apr 16, 2019
View reviewed changes
chromium_src/content/public/common/url_utils_browsertest.cc Outdated
@@ -0,0 +1,40 @@
/* Copyright (c) 2019 The Brave Authors. All rights reserved.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not a test of url_utils, it's a test of the navigation controller

@simonhong simonhong force-pushed the fix_brave_crash_url branch 3 times, most recently from 53fa1ee to ef0c479 Compare April 16, 2019 20:52
bridiver
bridiver previously approved these changes Apr 17, 2019
View reviewed changes
@simonhong
Copy link
Member Author

Will merge when buildbot completes its build

@simonhong simonhong force-pushed the fix_brave_crash_url branch 4 times, most recently from 4b1ebaa to 46e6647 Compare April 18, 2019 23:14
@simonhong
Copy link
Member Author

simonhong commented Apr 19, 2019 •
edited
Loading

@bridiver Can you approve again? To make buildbot green, I rebased and still can't get all green because of not related with this PR(signing issue). (https://staging.ci.brave.com/job/brave-browser-build-pr/job/fix_brave_crash_url/8/execution/node/410/log/)
I think this can be merged.

@simonhong
Fix loading brave crash url doesn't cause renderer crash
4b071c0 
Send converted debug url to renderer.
@simonhong
Convert brave sheme more earlier when browser initiated navigation st…
f24b048 
…arts

With this, IsRendererDebugURL() can check with converted url.
@simonhong simonhong requested a review from bridiver April 24, 2019 03:15
@simonhong simonhong merged commit eaba810 into master Apr 24, 2019
@simonhong simonhong deleted the fix_brave_crash_url branch April 24, 2019 17:53
brave-builds pushed a commit that referenced this pull request Apr 24, 2019
@brave-browser-releases
Uplift of #2229 (squashed) to dev
cd0ef16
brave-builds pushed a commit that referenced this pull request Apr 24, 2019
@brave-browser-releases
Uplift of #2229 (squashed) to beta
3efb554
This was referenced Apr 24, 2019
Merged
Merged
bsclifton pushed a commit that referenced this pull request Apr 26, 2019
@brave-browser-releases @bsclifton
Uplift of #2229 (squashed) to beta
e534e34
bbondy added a commit that referenced this pull request May 5, 2019
@bbondy
Remove call to AdjustNavigateParamsForURLBraveImpl because it doesn't…
ec3c006 
… exist

Because it was only added from
#2229 which is only in 0.64.x

Auditors: @bsclifton

Related to ab02110
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bridiver bridiver bridiver approved these changes

Assignees

@simonhong simonhong

Labels
None yet
Projects
None yet
Milestone
0.66.x - Release
Development

Successfully merging this pull request may close these issues.

brave://crash does not crash tab
3 participants
@simonhong @kjozwiak @bridiver