In WeaveCrypto.js, it loads libnss3.so. For OpenSolaris bundled Firefox, it is built with system nss libraries in /usr/lib/mps So it failed to find libnss3.so in Firefox directory. If I change the line to append("/usr/lib/mps/libnss3.so"); it will work. Ideally, we should honor LD_LIBRARY_PATH env variable and RUNPATH in firefox-bin or libxul.so ELF. (OpenSolaris bundled Firefox doesn't set LD_LIBRARY_PATH.) I'm not sure jsctypes support it. A workaround can be: Try libnss3.so in GRE directory, if not found, try /usr/lib/mps/libnss3.so.

Debian's build of Firefox ("Iceweasel") apparently puts libnss3.so in /usr/lib or /usr/lib64 as well, causing Sync to disable itself on that platform.

Using ctypes.libraryName to get the library prefix/suffix might get the right filename, but we'll still have to check various directories for the right file? Like on android, it checks for GRE/lib instead of just GRE. But is there a better way than check GRE then GRE/lib then LD_LIBRARY_PATH then more?

Speaking for Linux here: dlopen() which is used from NSPR in the end lets the dynamic linker check where to find the lib if it's not given an absolute/relative path but just a filename. Apparently that option is not provided through NSPR. At least I couldn't find it. There is PR_GetLibraryPath() which seems to do something but it's not very up to date as it hardcodes /usr/lib:/lib but nowadays we are supporting /usr/lib64:/lib64 as well. But probably this is a possible way and fixing remaining stuff in NSPR then? So a proposal could be: 1. try to load with defined path 2. if not successful, find PR_GetLibraryPath() and iterate through the returned list (3. fix PR_GetLibraryPath() do actually be a bit more up to date and complete) ?

(In reply to comment #4) > 1. try to load with defined path > 2. if not successful, find PR_GetLibraryPath() and iterate through the returned > list This would need to be done in the JS ctypes implementation probably.

If it's going to get us more standard semantics, I'd rather use dlopen() (and the corresponding Win32 API) rather than NSPR. Doing so would remove the jseng dependency on NSPR when ctypes is built, too.

(Note that to get RTLD_DEFAULT we need to use platform native APIs anyway -- NSPR doesn't provide that.)

I tested this on the try server http://hg.mozilla.org/try/rev/67a06bf57496 and it apparently works.

(In reply to comment #6) > If it's going to get us more standard semantics, I'd rather use dlopen() (and > the corresponding Win32 API) rather than NSPR. Doing so would remove the jseng > dependency on NSPR when ctypes is built, too. IIRC, ctypes is not the only thing that requires NSPR in js.

It is if you're not building JS_THREADSAFE.

Comment on attachment 468627 [details] [diff] [review] Use ctypes.libraryName and don't use a full path to load libnss3 from weavecrypto > let os = Services.appinfo.OS; I think you can get rid of this, too. So by not using absolute path it magically works on Linux distros and Solaris where libnss is tucked away in /usr/lib?

(In reply to comment #14) > So by not using absolute path it magically works on Linux distros and Solaris > where libnss is tucked away in /usr/lib? Most likely it will take whatever libnss was loaded by firefox.

Comment on attachment 468627 [details] [diff] [review] Use ctypes.libraryName and don't use a full path to load libnss3 from weavecrypto >+++ b/services/crypto/WeaveCrypto.js ... >- let nssfile = Services.dirsvc.get("GreD", Ci.nsILocalFile); > let os = Services.appinfo.OS; >- switch (os) { As noted above, the "let os = ..." can be removed now. >- case "WebOS": // Palm Pre >- nssfile.append("libnss3.so"); >- break; >- case "Android": >- // Android uses a $GREDIR/lib/ subdir. >- nssfile.append("lib"); >- nssfile.append("libnss3.so"); >- break; Should ping owners for these platforms, since they don't really have test coverage through try. At the least, so they're aware of this patch should Sync suddenly break.

Mossop / mwu: CCing you to be on the lookout for any possible WebOS or Android breakage (see comment 16).

There was discussion on #developers a few days ago about this and bug 593484, and as to why the full path was needed in the first place. It would be better to double check the patch here works as expected when the system also has a system nss library installed, especially after bug 552864. I can definely check for Linux, but can't for other systems, though. I guess the main other system being affected would be Solaris ; I doubt windows and mac systems are likely to have a system nss.

(In reply to comment #18) > I can definely check for Linux Any news on this, Mike?

I can confirm that applying both attachment 468627 [details] [diff] [review] and bug 552864 doesn't break on linux when there is a system libnss3.so library.

Transferring Justin's r+ ; this is the same patch, only removing the now useless let os = ... part, and with commit message embedded.

(In reply to comment #17) > Mossop / mwu: CCing you to be on the lookout for any possible WebOS or Android > breakage (see comment 16). We have some try server builds on http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/mh@glandium.org-6539ab9ab9d0/ now. I don't have an Android or Maemo phone handy, can somebody from the mobile team confirm that Sync still works with these builds?

Richard from Sync Ops just confirmed that the Android build worked.

So we're just waiting on fx-sync approval now, or do we need approval2.0 as well?

It can land on fx-sync now, and we'll pick it up with the next merge to m-c.

Landed on fx-sync http://hg.mozilla.org/services/fx-sync/rev/4ad5b1467331

Backed out again on fx-sync and m-c since it breaks OS X.

I can confirm I reproduce the problem with the OSX nightly.

Can you install this restartless add-on? It'll open a new tab printing out the value of your nss filepath and ctypes path: My results: file: /Applications/Firefox x.x - Minefield.app/Contents/MacOS/libnss3.dylib path: libnss3.dylib bootstrap.js: const Ci = Components.interfaces; const Cu = Components.utils; Cu.import("resource://gre/modules/AddonManager.jsm"); Cu.import("resource://gre/modules/ctypes.jsm"); Cu.import("resource://gre/modules/Services.jsm"); function _() { let gBrowser = Services.wm.getMostRecentWindow("navigator:browser").gBrowser; gBrowser.selectedTab = gBrowser.addTab("data:text/html," + Array.join(arguments, "<br/><br/>")); } function startup(data, reason) { // One-time scripts: auto-uninstall after running AddonManager.getAddonByID(data.id, function(addon) { addon.uninstall(); }); let file = Services.dirsvc.get("GreD", Ci.nsILocalFile); file.append("libnss3.dylib"); let path = ctypes.libraryName("nss3"); _("file: " + file.path, "path: " + path); }

(In reply to comment #29) > file: /Applications/Firefox x.x - Minefield.app/Contents/MacOS/libnss3.dylib > path: libnss3.dylib Well, this is not unexpected, considering that was the whole point of the patch.

So, it appears that it works when running the firefox script directly from the command line, because it sets the proper DYLD_LIBRARY_PATH.

This one works for me. IMHO, the mac build should be arranged so that ctypes.open("somelib.dylib") without a full path, and for a library that is in the GRE directory, just works properly.

So one interesting thing I ran into when debugging this is that if you load the fullpath library, the short path loading will succeed.

(In reply to comment #33) > So one interesting thing I ran into when debugging this is that if you load the > fullpath library, the short path loading will succeed. Which kind of makes sense, except that one would expect that to also happen after libnss is loaded as being depended upon by libxul. Looks like dlopen still needs some love on the system side on OSX.

Comment on attachment 481491 [details] [diff] [review] Use ctypes.libraryName and don't use a full path to load libnss3 from weavecrypto r=dwitte

Comment on attachment 481491 [details] [diff] [review] Use ctypes.libraryName and don't use a full path to load libnss3 from weavecrypto Although... >+ this.log("Using NSS library " + path); This is kinda useless now. Move it inside the 'try', to say we're trying without path: >+ try { >+ nsslib = ctypes.open(path); >+ } catch(e) { >+ // In case opening the library without a full path fails, >+ // try again with a full path. and add a log line here to say that we're retrying with the full path. >+ let file = Services.dirsvc.get("GreD", Ci.nsILocalFile); >+ file.append(path); >+ nsslib = ctypes.open(file.path);

Comment on attachment 481491 [details] [diff] [review] Use ctypes.libraryName and don't use a full path to load libnss3 from weavecrypto >+ var nsslib; >+ try { nits: Why not let here? Also both lines have trailing spaces.

(In reply to comment #38) > >+ var nsslib; > >+ try { > nits: Why not let here? Also both lines have trailing spaces. I thought let was used for let var = value type of definitions only. (In reply to comment #37) > Comment on attachment 481491 [details] [diff] [review] > Use ctypes.libraryName and don't use a full path to load libnss3 from > weavecrypto > > Although... > > >+ this.log("Using NSS library " + path); > > This is kinda useless now. Move it inside the 'try', to say we're trying > without path: > > >+ try { > >+ nsslib = ctypes.open(path); > >+ } catch(e) { > >+ // In case opening the library without a full path fails, > >+ // try again with a full path. > > and add a log line here to say that we're retrying with the full path. > > >+ let file = Services.dirsvc.get("GreD", Ci.nsILocalFile); > >+ file.append(path); > >+ nsslib = ctypes.open(file.path); Will do.

As landed on fx-sync (carrying over r+ ; addressed dwitte's comments) http://hg.mozilla.org/services/fx-sync/rev/e947b5400606

Resolved and tracked for the next merge (bug 604603)