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.

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

fix: Expose missing Add/RemoveExtraParameter methods to macOS node child processes #15790

Merged
merged 2 commits into from Jul 29, 2019

Conversation

@lepinay
Copy link
Contributor

lepinay commented Nov 21, 2018

Description of Change

This will expose the missing methods on crashReporter when running in a child node process (fork/spawn) on mac:
AddExtraParameter
RemoveExtraParameter

Checklist

Release Notes

Notes: Fixed crashReporter addExtraParameter / removeExtraParameter methods undefined in macOS node child processes

@lepinay lepinay requested a review from Nov 21, 2018
@miniak miniak changed the title fix: Expose missing Add/RemoveExtraParameter methods to MACOSX node child processes fix: Expose missing Add/RemoveExtraParameter methods to macOS node child processes Jan 5, 2019
spec/fixtures/api/crash.html Outdated Show resolved Hide resolved
@lepinay

This comment has been minimized.

Copy link
Contributor Author

lepinay commented Jan 15, 2019

Hi @miniak , just bumping this thread, change was reverted based on review, let me know if anything else is needed.
Cheers

@miniak
miniak approved these changes Jan 15, 2019
atom/app/node_main.cc Outdated Show resolved Hide resolved
@miniak

This comment has been minimized.

Copy link
Contributor

miniak commented Apr 18, 2019

@lepinay can you please rebase on top of latest master?

@lepinay lepinay force-pushed the lepinay:AddExtraMacFixRebased branch from d76f92f to c9af770 Apr 19, 2019
@lepinay

This comment has been minimized.

Copy link
Contributor Author

lepinay commented Apr 19, 2019

Hi @miniak, rebase to master is done

@emmkimme

This comment has been minimized.

Copy link
Contributor

emmkimme commented May 9, 2019

Hi @miniak
this MR is on-hold since Nov. 2018, could it be possible to merge it please
Thanks

@miniak

This comment has been minimized.

Copy link
Contributor

miniak commented May 22, 2019

@emmkimme please rebase again
@codebytere can you help merging the PR?

@lepinay lepinay force-pushed the lepinay:AddExtraMacFixRebased branch 2 times, most recently from de59650 to a2d4f34 May 22, 2019
@lepinay

This comment has been minimized.

Copy link
Contributor Author

lepinay commented May 22, 2019

Hi, @miniak @codebytere rebase is done.

@miniak

This comment has been minimized.

Copy link
Contributor

miniak commented Jun 16, 2019

@lepinay needs rebasing again :(

@lepinay lepinay force-pushed the lepinay:AddExtraMacFixRebased branch from e74b036 to 819dddd Jun 17, 2019
@lepinay

This comment has been minimized.

Copy link
Contributor Author

lepinay commented Jun 17, 2019

@miniak rebase is done
Add/RemoveExtraParameter is already working on windows for node child processes

@miniak

This comment has been minimized.

Copy link
Contributor

miniak commented Jun 17, 2019

@lepinay how does it work?

@lepinay

This comment has been minimized.

Copy link
Contributor Author

lepinay commented Jun 18, 2019

@miniak
My bad, I forgot I added an additional layer on my side that make it "works" on windows by restarting the crash reporter on windows ;)

So, in that case #18483 will need to add the support for it in the Windows implementation. It will make more sense to put it there. Because my PR is more a bug fix for the current OSX implementation of crashpad and could be fixed now indepently of #18483.

Or maybe we can create a separate PR once #18483 has been merged to add this support on windows as well ?

@miniak

This comment has been minimized.

Copy link
Contributor

miniak commented Jun 22, 2019

I would modify this PR. It should be basically changing #if defined(OS_MACOSX) to #if !defined(OS_LINUX), right?

@lepinay

This comment has been minimized.

Copy link
Contributor Author

lepinay commented Jun 24, 2019

@miniak Yes indeed, now that #18483 is merged, that would be the change to make + testing that it works now on windows too.

@miniak

This comment has been minimized.

Copy link
Contributor

miniak commented Jul 5, 2019

@lepinay please rebase and do the change as requested

@lepinay lepinay force-pushed the lepinay:AddExtraMacFixRebased branch from 819dddd to c167655 Jul 8, 2019
@lepinay

This comment has been minimized.

Copy link
Contributor Author

lepinay commented Jul 8, 2019

@miniak done, please review.

@zcbenz
zcbenz approved these changes Jul 11, 2019
@zcbenz zcbenz requested a review from miniak Jul 11, 2019
@miniak
miniak approved these changes Jul 28, 2019
@miniak

This comment has been minimized.

Copy link
Contributor

miniak commented Jul 28, 2019

@zcbenz let’s get this merged before 7 is branched

@zcbenz zcbenz merged commit 4e0e615 into electron:master Jul 29, 2019
8 of 9 checks passed
8 of 9 checks passed
build-mac Workflow: build-mac
Details
Artifact Comparison No Changes
Details
Semantic Pull Request ready to be squashed
Details
WIP Ready for review
Details
appveyor: win-ia32-testing-pr AppVeyor build succeeded
Details
appveyor: win-x64-testing-pr AppVeyor build succeeded
Details
build-linux Workflow: build-linux
Details
lint Workflow: lint
Details
release-notes Release notes found
@release-clerk

This comment has been minimized.

Copy link

release-clerk bot commented Jul 29, 2019

Release Notes Persisted

Fixed crashReporter addExtraParameter / removeExtraParameter methods undefined in macOS node child processes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.