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

@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Jun 10, 2019
@miniak miniak force-pushed the miniak/app-metrics branch 3 times, most recently from 06e0890 to d6a912a Compare June 10, 2019 21:40
@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 Compare June 11, 2019 16:10
@miniak miniak self-assigned this Jun 11, 2019
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Jun 11, 2019
Copy link
Member

@ckerr ckerr left a comment

Choose a reason for hiding this comment

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

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?

Copy link
Member

@zcbenz zcbenz left a comment

Choose a reason for hiding this comment

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

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
@release-clerk
Copy link

release-clerk bot commented Jun 14, 2019

Release Notes Persisted

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

@deepak1556
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 June 14, 2019 21:09
@nornagon
Copy link
Member

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants