Closed Bug 1133073 Opened 5 years ago Closed 4 years ago

Use PR_DuplicateEnvironment from NSPR instead of the temporary local copy

Categories

(Core :: IPC, defect)

All
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: jld, Assigned: jld)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

This is being broken out of bug 1132760, which will be to copy PR_DuplicateEnvironment into upstream NSPR.  Once it's migrated back downstream into mozilla-central, the copy in process_util_linux and mozglue can be removed, and the NSPR version used on all Linux platforms.
This patch is for use after the patch in bug 1132760 lands in NSPR and makes its way into nsprpub.  I might also need an actual IPC peer for this.
Attachment #8571776 - Flags: review?(dhylands)
Comment on attachment 8571776 [details] [diff] [review]
bug1133073-dupenv-from-nspr-hg4.diff

Review of attachment 8571776 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me. Thanks for doing this.
Attachment #8571776 - Flags: review?(dhylands) → review+
No longer blocks: 986397
No longer depends on: 1162780
I just realized: Solaris doesn't use process_util_linux.cc.  That means this can actually land now (modulo whatever rebasing it needs), because NSPR_MINVER is already up to NSPR 4.11, which has PR_DuplicateEnvironment everywhere except Solaris (where bug 1209397 means it's not exported until 4.12).
See Also: → 1233275
Rebase patch to current m-c; carrying over r+.
Attachment #8571776 - Attachment is obsolete: true
Attachment #8699531 - Flags: review+
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8bcc2a251c0b (tests chosen are slightly arbitrary; goal is to make sure child processes aren't entirely broken).
Keywords: checkin-needed
No longer depends on: 1209397
sorry had to back this out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=18826447&repo=mozilla-inbound
Flags: needinfo?(jld)
In hindsight, I agree that I should have run Try on Android as well — it doesn't do content e10s like desktop and B2G, but it does use IPC, and it is affected by the PR_{Get,Set}Env interceptions.

But I'm at a loss as to how my patch caused that particular build breakage — “error: cannot find -lc”, “error: cannot open crtend_android.o: No such file or directory”, etc. suggest a problem with a linker path, and I wasn't directly doing anything to any of those — and it affects one of the executables from the NSS build, which the patch doesn't go anywhere near.  (Also… while one can argue whether the tool that makes those weird FIPS library signatures is ever useful, it's markedly less useful when compiled for the target rather than the host, as it seems to be here.  But that's probably beside the point.)

In any case, I've pushed to Try to check if it's reproducible: https://treeherder.mozilla.org/#/jobs?repo=try&revision=56e1192466b0
Flags: needinfo?(jld)
seems its reproducible,same error in the try run somehow :(
https://treeherder.mozilla.org/#/jobs?repo=try&revision=69fcf055fce2

glandium, that $(SHARED_LIBS:$(DEPTH)%=$(MOZ_BUILD_ROOT)%) you added to config/external/nss/Makefile.in in bug 1077148 seems to have become load-bearing for the Android build, such that it breaks (see treeherder links in comment #8, comment #9) if MOZ_GLUE_WRAP_LDFLAGS is empty.  Any suggestions?
Flags: needinfo?(mh+mozilla)
(In reply to Jed Davis [:jld] from comment #11)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=69fcf055fce2
> 
> glandium, that $(SHARED_LIBS:$(DEPTH)%=$(MOZ_BUILD_ROOT)%) you added to
> config/external/nss/Makefile.in in bug 1077148 seems to have become
> load-bearing for the Android build, such that it breaks (see treeherder
> links in comment #8, comment #9) if MOZ_GLUE_WRAP_LDFLAGS is empty.  Any
> suggestions?

Make it conditional to MOZ_GLUE_WRAP_LDFLAGS being non empty or ANDROID being set. You can replace the second complex conditional with a check on whether NSS_EXTRA_LDFLAGS is empty.
Flags: needinfo?(mh+mozilla)
So that burned the tests with a lot of errors like this:

libsoftokn3.so: Relocation to NULL @0x00024e10 for symbol "__wrap_PR_GetEnv"

Looking at the logs, I can see some linker steps still using -Wl,--wrap=PR_GetEnv,--wrap=PR_SetEnv even though it's no longer in the source tree.  Which of course didn't happen on Try.  My guess is that there's some kind of issue with subprojects with their own configury or build systems (I noticed some of the problematic linker command lines where for things in js/) where touching the toplevel configure.in isn't enough to reconfigure them (or equivalent), and I'll need to do a full clobber instead.

(Treeherder link for reference: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=7f6bb9f7e60d )
A few failures on https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a9f906890d9 but they look like they're not due to this patch.
https://hg.mozilla.org/mozilla-central/rev/f75b461c2c3c
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
See Also: → 1240247
You need to log in before you can comment on or make changes to this bug.