Closed Bug 1403366 Opened 7 years ago Closed 7 years ago

Remove MOZILLA_FIVE_HOME

Categories

(Core :: XPCOM, enhancement)

Product:
Core
Component:
XPCOM
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: mccr8, Assigned: glandium)

References

Details

Attachments

(7 files)

Bug 1403366 - Don't use nsDirectoryService::Create in nsDirectoryService::GetCurrentProcessDirectory.
59 bytes, text/x-review-board-request
froydnj
: review+
Details
Bug 1403366 - Remove GetXULRunnerStubPath.
59 bytes, text/x-review-board-request
froydnj
: review+
Details
Bug 1403366 - Stop requiring argv[0] for XRE_GetBinaryPath and the underlying BinaryPath::Get.
59 bytes, text/x-review-board-request
froydnj
: review+
Details
Bug 1403366 - When NS_XPCOM_INIT_CURRENT_PROCESS_DIR is not already set, fallback to BinaryPath's parent.
59 bytes, text/x-review-board-request
froydnj
: review+
Details
Bug 1403366 - Don't set MOZILLA_FIVE_HOME from multiple scripts.
59 bytes, text/x-review-board-request
froydnj
: review+
Details
Bug 1403366 - Remove outdated comment.
59 bytes, text/x-review-board-request
froydnj
: review+
Details
Bug 1403366 - Remove the --with-default-mozilla-five-home configure flag.
59 bytes, text/x-review-board-request
froydnj
: review+
Details
There still some code related to MOZILLA_FIVE_HOME in XPCOM and the build system and a few test harnesses. Maybe this is just related to XULRunner stuff that is no longer supported, so we can remove it?
The comment above when we getenv MOZILLA_FIVE_HOME says: // In the absence of a good way to get the executable directory let // us try this for unix: // - if MOZILLA_FIVE_HOME is defined, that is it // - else give the current directory But that's actually not true as of bug 239929 from 13 years ago, which used something different than MOZILLA_FIVE_HOME to find the executable. (plus, MOZILLA_FIVE_HOME could cause problems when running Firefox from Thunderbird and vice versa) The code introduced there changed and moved around, it's now in xpcom/build/BinaryPath.h and has the following comment: // on unix, there is no official way to get the path of the current binary. // instead of using the MOZILLA_FIVE_HOME hack, which doesn't scale to // multiple applications, we will try a series of techniques: (...)
Assignee: nobody → mh+mozilla
Petr, Landry, Jan, could you check this compiles (and works) on *BSD and Solaris?
Flags: needinfo?(petr.sumbera)
Flags: needinfo?(landry)
Flags: needinfo?(jbeich)
I never got around to learn mozreview, but is there a way to qimport all the patches in a single hg voodoo command ?
Flags: needinfo?(landry)
Comment on attachment 8913037 [details] Bug 1403366 - Don't use nsDirectoryService::Create in nsDirectoryService::GetCurrentProcessDirectory. https://reviewboard.mozilla.org/r/184406/#review189748
Attachment #8913037 - Flags: review?(nfroyd) → review+
Comment on attachment 8913039 [details] Bug 1403366 - Stop requiring argv[0] for XRE_GetBinaryPath and the underlying BinaryPath::Get. https://reviewboard.mozilla.org/r/184410/#review189750 This is a nice cleanup. The non-OpenBSD code looks right, judging from reading the man page, and I'm just going to assume the Solaris code is correct. ::: xpcom/build/BinaryPath.h:198 (Diff revision 1) > + int mib[4]; > + mib[0] = CTL_KERN; > + mib[1] = KERN_PROC_ARGS; > + mib[2] = getpid(); > + mib[3] = KERN_PROC_ARGV; > + > + size_t len = 0; > + if (sysctl(mib, 4, nullptr, &len, nullptr, 0) < 0) { > + return NS_ERROR_FAILURE; > + } > + > + auto argv = MakeUnique<const char*[]>(len / sizeof(const char*)); > + if (sysctl(mib, 4, argv, &len, nullptr, 0) < 0) { > + return NS_ERROR_FAILURE; > + } This doesn't seem right...the man page for `sysctl(3)` and `KERN_PROC_ARGV` (https://man.openbsd.org/sysctl.3#KERN_PROC_ARGS) says "`KERN_PROC_ARGV` returns the argv array...The buffer pointed to by `oldp` is filled with an array of char pointers followed by the strings themselves." But we're not allocating memory for the strings themselves here, just the pointers to the strings. Where are the strings being stored?
Attachment #8913039 - Flags: review?(nfroyd) → review+
Comment on attachment 8913040 [details] Bug 1403366 - When NS_XPCOM_INIT_CURRENT_PROCESS_DIR is not already set, fallback to BinaryPath's parent. https://reviewboard.mozilla.org/r/184412/#review189756 I do like patches that delete code like this.
Attachment #8913040 - Flags: review?(nfroyd) → review+
Comment on attachment 8913041 [details] Bug 1403366 - Don't set MOZILLA_FIVE_HOME from multiple scripts. https://reviewboard.mozilla.org/r/184414/#review189758
Attachment #8913041 - Flags: review?(nfroyd) → review+
Comment on attachment 8913042 [details] Bug 1403366 - Remove outdated comment. https://reviewboard.mozilla.org/r/184416/#review189760
Attachment #8913042 - Flags: review?(nfroyd) → review+
Comment on attachment 8913043 [details] Bug 1403366 - Remove the --with-default-mozilla-five-home configure flag. https://reviewboard.mozilla.org/r/184418/#review189762
Attachment #8913043 - Flags: review?(nfroyd) → review+
Like said in comment #12, fails here... 22:46.80 In file included from /home/landry/src/m-c/xpcom/io/nsDirectoryService.cpp:40: 22:46.87 /home/landry/src/m-c/xpcom/build/BinaryPath.h:210:9: error: no matching function for call to 'sysctl' 22:46.87 if (sysctl(mib, 4, argv, &len, nullptr, 0) < 0) { 22:46.87 ^~~~~~ 22:46.97 /usr/include/sys/sysctl.h:984:5: note: candidate function not viable: no known conversion from 'mozilla::UniquePtr<const char *[], mozilla::DefaultDelete<const char *[]> >' to 'void *' for 3rd argument; take the address of the argument with & 22:46.97 int sysctl(const int *, u_int, void *, size_t *, void *, size_t);
Oh and, if the intent is to mimic /proc/self/exe on linux, on openbsd there's no way to get the full path of the actual binary, and that's by design. yeah. right. KERN_PROC_ARGS will give you a string/'something' that might not resolve to a filesystem path. but that shouldnt be different from the existing code...
Here's a conceptual chunk im testing: + len = 128; + if ((argv = malloc(len)) == NULL) + return NS_ERROR_FAILURE; - auto argv = MakeUnique<const char*[]>(len / sizeof(const char*)); - if (sysctl(mib, 4, argv, &len, nullptr, 0) < 0) { - return NS_ERROR_FAILURE; + for (;; len *= 2) { + if ((argv = realloc(args, len)) == NULL) + return NS_ERROR_FAILURE; + if (sysctl(mib, 4, argv, &len, nullptr, 0) == 0) + break; + if (errno != ENOMEM) // ESRCH: process disappeared + return NS_ERROR_FAILURE; } shoplifeted from https://git.xfce.org/apps/xfce4-taskmanager/tree/src/task-manager-bsd.c#n115 where i took it from top(1).
Comment on attachment 8913038 [details] Bug 1403366 - Remove GetXULRunnerStubPath. https://reviewboard.mozilla.org/r/184408/#review189884
Attachment #8913038 - Flags: review?(nfroyd) → review+
(In reply to Landry Breuil (:gaston) from comment #17) > Like said in comment #12, fails here... > > 22:46.80 In file included from > /home/landry/src/m-c/xpcom/io/nsDirectoryService.cpp:40: > 22:46.87 /home/landry/src/m-c/xpcom/build/BinaryPath.h:210:9: error: no > matching function for call to 'sysctl' > 22:46.87 if (sysctl(mib, 4, argv, &len, nullptr, 0) < 0) { > 22:46.87 ^~~~~~ > 22:46.97 /usr/include/sys/sysctl.h:984:5: note: candidate function not > viable: no known conversion from 'mozilla::UniquePtr<const char *[], > mozilla::DefaultDelete<const char *[]> >' to 'void *' for 3rd argument; take > the address of the argument with & > 22:46.97 int sysctl(const int *, u_int, void *, size_t *, void *, > size_t); Ah right, try replacing argv with argv.get(). (In reply to Landry Breuil (:gaston) from comment #19) > Here's a conceptual chunk im testing: > > + len = 128; > + if ((argv = malloc(len)) == NULL) > + return NS_ERROR_FAILURE; > > - auto argv = MakeUnique<const char*[]>(len / sizeof(const char*)); > - if (sysctl(mib, 4, argv, &len, nullptr, 0) < 0) { > - return NS_ERROR_FAILURE; > + for (;; len *= 2) { > + if ((argv = realloc(args, len)) == NULL) > + return NS_ERROR_FAILURE; > + if (sysctl(mib, 4, argv, &len, nullptr, 0) == 0) > + break; > + if (errno != ENOMEM) // ESRCH: process disappeared > + return NS_ERROR_FAILURE; > } > > shoplifeted from > https://git.xfce.org/apps/xfce4-taskmanager/tree/src/task-manager-bsd.c#n115 > where i took it from top(1). That's the same thing, really.
(In reply to Nathan Froyd [:froydnj] from comment #12) > This doesn't seem right...the man page for `sysctl(3)` and `KERN_PROC_ARGV` > (https://man.openbsd.org/sysctl.3#KERN_PROC_ARGS) says "`KERN_PROC_ARGV` > returns the argv array...The buffer pointed to by `oldp` is filled with an > array of char pointers followed by the strings themselves." But we're not > allocating memory for the strings themselves here, just the pointers to the > strings. Where are the strings being stored? They are not stored. KERN_PROC_ARGV returns a list of pointers to the strings already in the process address space (meaning, if e.g. main() modifies them, you get the modified values, but that's not different from the existing code, which somehow was getting the stuff from main)
(In reply to Mike Hommey [:glandium] from comment #22) > (In reply to Nathan Froyd [:froydnj] from comment #12) > > This doesn't seem right...the man page for `sysctl(3)` and `KERN_PROC_ARGV` > > (https://man.openbsd.org/sysctl.3#KERN_PROC_ARGS) says "`KERN_PROC_ARGV` > > returns the argv array...The buffer pointed to by `oldp` is filled with an > > array of char pointers followed by the strings themselves." But we're not > > allocating memory for the strings themselves here, just the pointers to the > > strings. Where are the strings being stored? > > They are not stored. KERN_PROC_ARGV returns a list of pointers to the > strings already in the process address space (meaning, if e.g. main() > modifies them, you get the modified values, but that's not different from > the existing code, which somehow was getting the stuff from main) The manpage certainly makes it sound like the strings are stored ("...followed by the strings themselves"), and the kernel syscall certainly looks like it copies out pointers to the strings and the strings themselves: https://github.com/openbsd/src/blob/master/sys/kern/kern_sysctl.c#L1766 Then again, we're having an argument about the implementation for a tier3 platform, sooo... :)
Ah indeed, they are copied too, in the same buffer. And the first sysctl with a null pointer returns ARG_MAX in len, so we allocate enough for all of it.
Comment on attachment 8913039 [details] Bug 1403366 - Stop requiring argv[0] for XRE_GetBinaryPath and the underlying BinaryPath::Get. https://reviewboard.mozilla.org/r/184410/#review189912 Builds fine on FreeBSD. However, my testing with `<gredir>/dependentlibs.list` not existing or containing just `libxul.so` failed. ::: xpcom/build/BinaryPath.h:175 (Diff revision 1) > + static nsresult Get(char aResult[MAXPATHLEN]) > + { > + int mib[4]; > + mib[0] = CTL_KERN; > + mib[1] = KERN_PROC; > + mib[2] = KERN_PROC_PATHNAME; NetBSD has slightly different order of arguments. Maybe confirm with martin@netbsd.org via feedback? flag. ```c++ #if defined(__NetBSD__) mib[1] = KERN_PROC_ARGS; mib[2] = -1; mib[3] = KERN_PROC_PATHNAME; #else mib[1] = KERN_PROC; mib[2] = KERN_PROC_PATHNAME; mib[3] = -1; #endif ``` http://netbsd.gw.com/cgi-bin/man-cgi?sysctl+7+NetBSD-current https://github.com/llvm-mirror/lldb/blob/ea5ad7e0dd29/source/Host/netbsd/HostInfoNetBSD.cpp#L85 ::: xpcom/build/BinaryPath.h:189 (Diff revision 1) > + } > + > +#elif defined(XP_SOLARIS) > + static nsresult Get(char aResult[MAXPATHLEN]) > + { > + const char* path = getexecname(); [getexecname()](http://docs.oracle.com/cd/E19253-01/816-5168/6mbb3hrb1/index.html) isn't guaranteed to return an absolute path unlike `/proc/self/path/a.out`. Maybe merge with Linux block and confirm with Petr via feedback? flag.
Works fine on FreeBSD but downstream doesn't use --with-default-mozilla-five-home.
Flags: needinfo?(jbeich)
(In reply to Jan Beich from comment #25) > Comment on attachment 8913039 [details] > Bug 1403366 - Stop requiring argv[0] for XRE_GetBinaryPath and the > underlying BinaryPath::Get. > > https://reviewboard.mozilla.org/r/184410/#review189912 > > Builds fine on FreeBSD. However, my testing with > `<gredir>/dependentlibs.list` not existing or containing just `libxul.so` > failed. That's not supposed to work.
Can you try the new patch?
Flags: needinfo?(landry)
Comment on attachment 8913039 [details] Bug 1403366 - Stop requiring argv[0] for XRE_GetBinaryPath and the underlying BinaryPath::Get. Thanks, builds fine on OpenBSD with argv.get() !
Flags: needinfo?(landry)
Attachment #8913039 - Flags: feedback+
Ok, let's go with this, and if it breaks Solaris, that can be fixed in a followup.
Pushed by mh@glandium.org: https://hg.mozilla.org/integration/autoland/rev/3ac2c898f94f Don't use nsDirectoryService::Create in nsDirectoryService::GetCurrentProcessDirectory. r=froydnj https://hg.mozilla.org/integration/autoland/rev/13c57542ba14 Remove GetXULRunnerStubPath. r=froydnj https://hg.mozilla.org/integration/autoland/rev/3eb67e350f38 Stop requiring argv[0] for XRE_GetBinaryPath and the underlying BinaryPath::Get. r=froydnj https://hg.mozilla.org/integration/autoland/rev/7e08706f9f34 When NS_XPCOM_INIT_CURRENT_PROCESS_DIR is not already set, fallback to BinaryPath's parent. r=froydnj https://hg.mozilla.org/integration/autoland/rev/28b00bdf83a3 Don't set MOZILLA_FIVE_HOME from multiple scripts. r=froydnj https://hg.mozilla.org/integration/autoland/rev/f9db424c4c6a Remove outdated comment. r=froydnj https://hg.mozilla.org/integration/autoland/rev/48a74b8750b4 Remove the --with-default-mozilla-five-home configure flag. r=froydnj
I can build latest patch without any issue. The Solaris specific change looks good. But right now I'm not in position to run it...
Flags: needinfo?(petr.sumbera)
Backed out for failing xpcshell tests netwerk/test/httpserver/test/test_basic_functionality.js and test_bug337744.js: https://hg.mozilla.org/integration/autoland/rev/aa0a7cd0d56a755422f7ca2592ed370b0f347ffb https://hg.mozilla.org/integration/autoland/rev/8d60c66e11b70e77b723bbb9699ef2eaa5c5eb1b https://hg.mozilla.org/integration/autoland/rev/938a07d707e7982a784bb28b8a0aeb9a6127b258 https://hg.mozilla.org/integration/autoland/rev/ef48f3c779cef8c4826e49f58a773ca170fb6074 https://hg.mozilla.org/integration/autoland/rev/ef0ab64b05a65c7e5da56078bcd6e7ca50318870 https://hg.mozilla.org/integration/autoland/rev/7bc5c9ba9b65a1e14669e35d3f88adddc4150dd2 https://hg.mozilla.org/integration/autoland/rev/97208f2bbe78c06ba814edff5c307dc3cddc8506 Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=af23e7b6eb9395aecf2a255316da782ca618750a&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=134061355&repo=autoland [task 2017-09-29T14:04:29.333Z] 14:04:29 INFO - TEST-START | netwerk/test/httpserver/test/test_basic_functionality.js [task 2017-09-29T14:04:33.341Z] 14:04:33 WARNING - TEST-UNEXPECTED-FAIL | netwerk/test/httpserver/test/test_basic_functionality.js | xpcshell return code: 0 [task 2017-09-29T14:04:33.341Z] 14:04:33 INFO - TEST-INFO took 4007ms [task 2017-09-29T14:04:33.341Z] 14:04:33 INFO - >>>>>>> [task 2017-09-29T14:04:33.341Z] 14:04:33 INFO - netwerk/test/httpserver/test/test_basic_functionality.js | xpcw: cd /storage/sdcard/tests/xpc/netwerk/test/httpserver/test [task 2017-09-29T14:04:33.342Z] 14:04:33 INFO - netwerk/test/httpserver/test/test_basic_functionality.js | xpcw: xpcshell -r /storage/sdcard/tests/xpc/c/httpd.manifest --greomni /data/local/xpcb/target.apk -m -s -e const _HEAD_JS_PATH = "/storage/sdcard/tests/xpc/head.js"; -e const _MOZINFO_JS_PATH = "/storage/sdcard/tests/xpc/p/mozinfo.json"; -e const _TESTING_MODULES_DIR = "/storage/sdcard/tests/xpc/m"; -f /storage/sdcard/tests/xpc/head.js -e const _SERVER_ADDR = "localhost" -e const _HEAD_FILES = ["/storage/sdcard/tests/xpc/netwerk/test/httpserver/test/head_utils.js"]; -e const _JSDEBUGGER_PORT = 0; -e const _TEST_FILE = ["test_basic_functionality.js"]; -e const _TEST_NAME = "netwerk/test/httpserver/test/test_basic_functionality.js" -e _execute_test(); quit(0); [task 2017-09-29T14:04:33.342Z] 14:04:33 INFO - (xpcshell/head.js) | test MAIN run_test pending (1) [task 2017-09-29T14:04:33.342Z] 14:04:33 INFO - 2147942487 [task 2017-09-29T14:04:33.342Z] 14:04:33 INFO - <<<<<<<
Flags: needinfo?(mh+mozilla)
Depends on: 1405174
Flags: needinfo?(mh+mozilla)
Pushed by mh@glandium.org: https://hg.mozilla.org/integration/autoland/rev/bc7ca663b817 Don't use nsDirectoryService::Create in nsDirectoryService::GetCurrentProcessDirectory. r=froydnj https://hg.mozilla.org/integration/autoland/rev/78f4fbeaac20 Remove GetXULRunnerStubPath. r=froydnj https://hg.mozilla.org/integration/autoland/rev/582d112281f9 Stop requiring argv[0] for XRE_GetBinaryPath and the underlying BinaryPath::Get. r=froydnj https://hg.mozilla.org/integration/autoland/rev/207925bbf88d When NS_XPCOM_INIT_CURRENT_PROCESS_DIR is not already set, fallback to BinaryPath's parent. r=froydnj https://hg.mozilla.org/integration/autoland/rev/ff0705eda4bd Don't set MOZILLA_FIVE_HOME from multiple scripts. r=froydnj https://hg.mozilla.org/integration/autoland/rev/4a0129621fe8 Remove outdated comment. r=froydnj https://hg.mozilla.org/integration/autoland/rev/aa58d5c8fe1a Remove the --with-default-mozilla-five-home configure flag. r=froydnj
Flags: needinfo?(mh+mozilla)
Pushed by mh@glandium.org: https://hg.mozilla.org/integration/autoland/rev/d6dc8ba8b076 Don't use nsDirectoryService::Create in nsDirectoryService::GetCurrentProcessDirectory. r=froydnj https://hg.mozilla.org/integration/autoland/rev/3b49525269b1 Remove GetXULRunnerStubPath. r=froydnj https://hg.mozilla.org/integration/autoland/rev/7e9878e20731 Stop requiring argv[0] for XRE_GetBinaryPath and the underlying BinaryPath::Get. r=froydnj https://hg.mozilla.org/integration/autoland/rev/e39909e9029e When NS_XPCOM_INIT_CURRENT_PROCESS_DIR is not already set, fallback to BinaryPath's parent. r=froydnj https://hg.mozilla.org/integration/autoland/rev/68765cc0c530 Don't set MOZILLA_FIVE_HOME from multiple scripts. r=froydnj https://hg.mozilla.org/integration/autoland/rev/69ef6d083959 Remove outdated comment. r=froydnj https://hg.mozilla.org/integration/autoland/rev/118c5ac3af29 Remove the --with-default-mozilla-five-home configure flag. r=froydnj
@glandium: In https://hg.mozilla.org/mozilla-central/rev/68765cc0c530 , you've made a typo in build/unix/run-mozilla.sh which breaks the script: if [ -z "$MRE_HOME" ]; then - MRE_HOME=$MOZILLA_FIVE_HOME + MRE_HOME=$MOZ_DIST_BIN" fi You have added an extraneous quote " at the end of the line, which comments out the script until the next quote. Thus we can't start Firefox on Linux.
Flags: needinfo?(mh+mozilla)
Please file a separate bug, but fwiw, run-mozilla.sh has not been required to run Firefox for years. Its only use is for xpcshell.
Flags: needinfo?(mh+mozilla)
FWIW, I filed bug 1407115 to stop shipping run-mozilla.sh, since it's actually not necessary, and xpcshell is not shipped.
The script is used in the Fedora RPM package to start Firefox.
@glandium: I filed bug #1407211.
Depends on: 1407211
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: