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 app is quitted right after executing installed app #213

Merged
merged 2 commits into from
Jul 3, 2018
Merged

Fix app is quitted right after executing installed app #213

merged 2 commits into from
Jul 3, 2018

Conversation

simonhong
Copy link
Member

@simonhong simonhong commented Jul 2, 2018 •
edited
Loading

local string's buffer is used after it is freed.

Close brave/brave-browser#467

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).
  • 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.

Test Plan:

Execute installed app from dmg

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions

@simonhong
Fix app is quitted right after executing installed app
4a6216a 
local string's buffer is used after it is freed.
@simonhong simonhong self-assigned this Jul 2, 2018
@simonhong simonhong requested a review from darkdh July 2, 2018 12:16
@darkdh
Copy link
Member

darkdh commented Jul 2, 2018

strdup is called at the end of ProductDirNameForBundle so why making early return here

bridiver
bridiver reviewed Jul 2, 2018
View reviewed changes
patches/chrome-common-chrome_paths_mac.mm.patch Outdated
@@ -21,7 +21,7 @@ index cca3bbb41ae25484c3e87c9aba09a8b07741e2b2..9e2676c79f09a565b5047af20bb82d44
+ product_dir_name = "BraveSoftware/Brave-Browser";
Copy link
Collaborator

Choose a reason for hiding this comment

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

why are we doing this at all? It can be set with CrProductDirName

Copy link
Collaborator

Choose a reason for hiding this comment

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

this also isn't consistent with chrome_paths_linux which has an official_build flag (although I'm not sure why that isn't part of GetChannelSuffixForDataDir)

bridiver
bridiver requested changes Jul 2, 2018
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.

ideally we should use CrProductDir, but in any case this should use a method override for GetDefaultUserDataDirectory and not patch

bridiver referenced this pull request Jul 2, 2018
@simonhong
Use brave-style user dir on linux
6c0e4c4 
Brave support four different user dir depends on channel.
* Brave-Browser for stable release
* Brave-Browser-Beta for beta release
* Brave-Browser-Dev for dev release
* Brave-Development for unofficial build

Canary channel isn't supported on linux.

Channel suffixes are determined from $CHROME_VERSION_EXTRA, which is
passed by the launch wrapper script.
For more detail, see src/docs/user_data_dir.md#Linux
@simonhong
Copy link
Member Author

strdup is called at the end of ProductDirNameForBundle so why making early return here

I assume the problem is name's buffer after const std::string name is destroyed.

Anyway, I'll try to use CrProductDir instead of patching.

@bridiver
Copy link
Collaborator

bridiver commented Jul 2, 2018

@simonhong that's what we do in muon. I think we would probably add it to the existing plist fixup script

@simonhong
Copy link
Member Author

@bridiver Yep, we can do that easily our own tweak script :)

@simonhong
Use CrProductDirName for fetching default user dir on MacOS
bb991b2 
In brave/build/config.gni brave_product_dir_name is made and it is
passed to tweak_info_plist.py
So, we don't need to modify chrome_paths_mac.mm anymore.
bridiver
bridiver approved these changes Jul 3, 2018
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.

++

@simonhong
Copy link
Member Author

Yay! a lot of code and patch file are deleted.
Thanks for nice review!

@simonhong simonhong merged commit 98e9688 into master Jul 3, 2018
darkdh
darkdh reviewed Jul 3, 2018
View reviewed changes
Copy link
Member

@darkdh darkdh left a comment

Choose a reason for hiding this comment

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

++

@simonhong simonhong deleted the fix_crash branch August 19, 2018 23:23
@Apetree100122 Apetree100122 mentioned this pull request Sep 15, 2023
Open
@MarkBuffalo MarkBuffalo mentioned this pull request Jan 1, 2024
Open
@kingjay66 kingjay66 mentioned this pull request Jan 1, 2024
Open
@MarkBuffalo MarkBuffalo mentioned this pull request Mar 15, 2024
Open
@kingjay66 kingjay66 mentioned this pull request Mar 19, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@darkdh darkdh darkdh left review comments

@bridiver bridiver bridiver approved these changes

Assignees

@simonhong simonhong

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@simonhong @darkdh @bridiver