Closed Bug 1231123 Opened 4 years ago Closed 4 years ago

Remove BSD code no longer used downstream

Categories

(Core :: IPC, defect)

Unspecified
FreeBSD
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox45 --- fixed
firefox46 --- fixed

People

(Reporter: jbeich, Assigned: jbeich)

References

(Blocks 1 open bug)

Details

(Whiteboard: [npotb] )

Attachments

(1 file)

All BSDs support posix_spawn API nowadays. The last one to use fork/exec was NetBSD 5.x [1]. By removing it process_util_bsd.cc becomes close to process_util_mac.mm. Let's also remove __dso_public as it shouldn't be used outside of BSDs source.

[1] https://blog.netbsd.org/tnf/entry/end_of_life_for_netbsd
Bug 1231123 - Simplify LaunchApp on BSDs by dropping fork/exec version. r?jld f?gaston f?martin@NetBSD.ORG
Attachment #8696684 - Flags: review?(jld)
Attachment #8696684 - Flags: feedback?(martin)
Attachment #8696684 - Flags: feedback?(landry)
I have no opinion on this, but consistency is always better :) I know some of our developers dont like this API but since it is in POSIX....
Comment on attachment 8696684 [details]
MozReview Request: Bug 1231123 - Simplify LaunchApp on BSDs by dropping fork/exec version. r?jld f?gaston f?martin@NetBSD.ORG

Wont be able to test it soon since last i tried m-c was broken on openbsd, but i welcome this change :)
Attachment #8696684 - Flags: feedback?(landry) → feedback+
Comment on attachment 8696684 [details]
MozReview Request: Bug 1231123 - Simplify LaunchApp on BSDs by dropping fork/exec version. r?jld f?gaston f?martin@NetBSD.ORG

I even managed to build it fine on OpenBSD.
Attachment #8696684 - Flags: feedback?(martin) → feedback+
Comment on attachment 8696684 [details]
MozReview Request: Bug 1231123 - Simplify LaunchApp on BSDs by dropping fork/exec version. r?jld f?gaston f?martin@NetBSD.ORG

https://reviewboard.mozilla.org/r/27401/#review25249

::: ipc/chromium/src/base/process_util_bsd.cc:69
(Diff revision 1)
>    while(environ[pos] != NULL) {

Not your change, but accessing `environ` like that is racy.  I'll file a bug to fix that once bug 1133073 lands and it's fixable.
Attachment #8696684 - Flags: review?(jld) → review+
Also land in Aurora mainly to ease maintenance of ESR45 downstream. Completely NPOTB unlike bug 1231109.
Assignee: nobody → jbeich
Status: NEW → ASSIGNED
Whiteboard: [npotb] → [npotb] [checkin-needed-aurora]
https://hg.mozilla.org/mozilla-central/rev/bf2c46cbeeb3
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
a=NPOTB DONTBUILD per comment 7 or whiteboard. process_util_bsd.cc is not built on Tier1.
Keywords: checkin-needed
(In reply to Jan Beich from comment #10)
> a=NPOTB DONTBUILD per comment 7 or whiteboard. process_util_bsd.cc is not
> built on Tier1.

this landed in comment 8 or ? For aurora not sure but i guess this need approval
Keywords: checkin-needed
Comment on attachment 8696684 [details]
MozReview Request: Bug 1231123 - Simplify LaunchApp on BSDs by dropping fork/exec version. r?jld f?gaston f?martin@NetBSD.ORG

To reduce maintenance cost of ESR45 downstream.

Approval Request Comment
[Feature/regressing bug #]: None, dead code; originally added by bug 753046
[User impact if declined]: None for supported *BSD releases
[Describe test coverage new/current, TreeHerder]: landed in m-c
[Risks and why]: NPOTB, may break build on some BSDs
[String/UUID change made/needed]: None

(In reply to Carsten Book [:Tomcat] from comment #11)
> For aurora not sure but i guess this need approval

In the past NPOTB patches could land under implicit approval e.g., bug 985848 comment 6, bug 986357 comment 6, bug 969932 comment 5. Looking at mercurial log occasional a=NPOTB for other folks still happen.
Attachment #8696684 - Flags: approval-mozilla-aurora?
Whiteboard: [npotb] [checkin-needed-aurora] → [npotb]
Comment on attachment 8696684 [details]
MozReview Request: Bug 1231123 - Simplify LaunchApp on BSDs by dropping fork/exec version. r?jld f?gaston f?martin@NetBSD.ORG

No, I do want the normal uplift process to be used for this kind of changes.
NPOTB is more for documentation or this kind of things.
Attachment #8696684 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
See Also: → 1400051
You need to log in before you can comment on or make changes to this bug.