1133073 - Use PR_DuplicateEnvironment from NSPR instead of the temporary local copy

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

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.

Comment 1

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Attachment: bug1133073-dupenv-from-nspr-hg4.diff

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.

Comment 2

[Dave Hylands [: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.

Comment 3

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

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

Comment 4

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Attachment: bug1133073-dupenv-from-nspr-hg5.diff

Rebase patch to current m-c; carrying over r+.

Comment 5

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

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

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8b4bffb2ce7c

Comment 7

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a87da113e0ab

Comment 8

[Carsten Book [:Tomcat]]

sorry had to back this out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=18826447&repo=mozilla-inbound

Comment 9

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

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

Comment 10

[Carsten Book [:Tomcat]]

seems its reproducible,same error in the try run somehow :(

Comment 11

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

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?

Comment 12

[Mike Hommey [:glandium]]

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

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7f6bb9f7e60d

Comment 14

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/18f5d9d83ab8

Comment 15

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

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 )

Comment 16

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

A few failures on https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a9f906890d9 but they look like they're not due to this patch.

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f75b461c2c3c

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f75b461c2c3c