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

feat: add creationTime / sandboxed / integrityLevel to app.getAppMetrics() #18718

Merged
merged 2 commits into from Jun 14, 2019

Conversation

@miniak
Copy link
Contributor

miniak commented Jun 10, 2019

Description of Change

This is useful for checking which processes are sandboxed on OS level.

Regarding creationTime, since the pid can be reused after a process dies, it is useful to use both the pid and the creationTime to uniquely identify a process.

Required for tests in #18650

/cc @MarshallOfSound, @deepak1556

Checklist

Release Notes

Notes: Added creationTime / sandboxed / integrityLevel to app.getAppMetrics() output.

@miniak miniak requested review from deepak1556 and MarshallOfSound Jun 10, 2019
@miniak miniak force-pushed the miniak/app-metrics branch 3 times, most recently from 06e0890 to d6a912a Jun 10, 2019
@miniak miniak changed the title feat: add sandboxed / integrityLevel to app.getAppMetrics() feat: add creationTime / sandboxed / integrityLevel to app.getAppMetrics() Jun 10, 2019
@miniak miniak force-pushed the miniak/app-metrics branch 3 times, most recently from e9d67e8 to a23362e Jun 11, 2019
@miniak miniak self-assigned this Jun 11, 2019
@electron-cation electron-cation bot removed the new-pr 🌱 label Jun 11, 2019
@miniak miniak force-pushed the miniak/app-metrics branch from a23362e to a12df50 Jun 13, 2019
@ckerr
ckerr approved these changes Jun 13, 2019
Copy link
Member

ckerr left a comment

This looks OK to me. Since this is new API, I'd like to see more feedback from other maintainers instead of landing it now.

As an aside, it often seems like there's hesitancy to jump on PRs that involve new API / API changes. Maybe we should revisit the idea of an API WG so that the roles are clearer. Since that doesn't exist right now, though -- CC'ing @electron/wg-releases for feedback?

@miniak miniak mentioned this pull request Jun 13, 2019
5 of 6 tasks complete
@zcbenz
zcbenz approved these changes Jun 14, 2019
Copy link
Member

zcbenz left a comment

The new API looks good to me.

docs/api/structures/process-metric.md Outdated Show resolved Hide resolved
@codebytere codebytere merged commit d9215dd into master Jun 14, 2019
11 of 13 checks passed
11 of 13 checks passed
electron-arm-testing Build #20190614.27 failed
Details
electron-arm64-testing Build #20190614.27 failed
Details
Artifact Comparison Changes Detected
Details
Semantic Pull Request ready to be squashed
Details
WIP Ready for review
Details
appveyor: win-ia32-testing AppVeyor build succeeded
Details
appveyor: win-ia32-testing-pr AppVeyor build succeeded
Details
appveyor: win-x64-testing AppVeyor build succeeded
Details
appveyor: win-x64-testing-pr AppVeyor build succeeded
Details
build-linux Workflow: build-linux
Details
build-mac Workflow: build-mac
Details
lint Workflow: lint
Details
release-notes Release notes found
@release-clerk

This comment has been minimized.

Copy link

release-clerk bot commented Jun 14, 2019

Release Notes Persisted

Added creationTime / sandboxed / integrityLevel to app.getAppMetrics() output.

@deepak1556

This comment has been minimized.

Copy link
Member

deepak1556 commented Jun 14, 2019

I think this could add support for linux by reading the value of service_manager::ZygoteHostImpl::GetInstance()->GetRendererSandboxStatus but its not useful at the moment, having the zygote enabled essentially means some sort of sandbox enabled, maybe helpful in knowing what type of sandbox is enabled similar to https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/sandbox_internals_ui.cc?g=0&l=31-61

But to make this return useful value in mixed sandbox mode were we bypass zygote https://github.com/electron/electron/blob/master/patches/chromium/support_mixed_sandbox_with_zygote.patch, maybe check for switches::kNoZygote ?

/cc @nornagon

@miniak miniak deleted the miniak/app-metrics branch Jun 14, 2019
@miniak miniak mentioned this pull request Jun 16, 2019
6 of 6 tasks complete
@nornagon

This comment has been minimized.

Copy link
Member

nornagon commented Jun 17, 2019

Hmm, I don't think sandbox status of the zygote tells you whether or not the process is sandboxed, because the way we implement mixed-sandbox mode in linux is that sandboxed processes are launched through the zygote and unsandboxed processes are launched directly.

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.