Bug 1957749 - Detach Linux sandbox broker threads if needed to avoid memory leak.
ClosedPublic
Actions

Authored by jld on Apr 3 2025, 5:24 AM.

Details

Reviewers

gerard-majax

Commits

rMOZILLACENTRAL1c57033291da: Bug 1957749 - Detach Linux sandbox broker threads if needed to avoid memory…

Bugzilla Bug ID

1957749

Diff Detail

Repository: rMOZILLACENTRAL mozilla-central

Event Timeline

jld created this revision.Apr 3 2025, 5:24 AM

Herald added a project: secure-revision. · View Herald TranscriptApr 3 2025, 5:24 AM

phab-bot published this revision for review.Apr 3 2025, 5:24 AM

phab-bot changed the visibility from "Custom Policy" to "Public (No Login Required)".

phab-bot changed the edit policy from "Custom Policy" to "Restricted Project (Project)".

phab-bot removed a project: secure-revision.

gerard-majax accepted this revision.Apr 3 2025, 5:37 AM

gerard-majax added inline comments.

security/sandbox/linux/gtest/TestBroker.cpp
687	maybe we dont need to force terminate now? and just rely on `broker = nullptr` as below
720	is this enough? above we had to `broker->Terminate()`

This revision is now accepted and ready to land.Apr 3 2025, 5:37 AM

This revision requires a Testing Policy Project Tag to be set before landing. Please apply one of testing-approved, testing-exception-unchanged, testing-exception-ui, testing-exception-elsewhere, testing-exception-other. Tip: this Firefox add-on makes it easy!

Harbormaster completed remote builds in B774561: Diff 1010545.Apr 3 2025, 5:44 AM

jld added inline comments.Apr 4 2025, 4:10 AM

security/sandbox/linux/gtest/TestBroker.cpp
720	That is a very good point. If I don't `Terminate()`, this could intermittently fail because too many threads didn't exit yet. But if I do `Terminate()`, then that will `pthread_join` the threads and the test won't be useful because it won't provoke the bug. Maybe it wouldn't actually fail that way in practice, and maybe those earlier failures of the fd leak test were because of this memory leak and not because all of those threads were still running… but in theory it could start most or all of those 16k threads before any of them are ever scheduled. I can't think of a good way to fix this right now, so I think I'll just disable the test and file a followup bug.