1075981 - Sync broken on OS X nightly (crypto check failed / can't sync: weave is disabled) due to packaging changes (FirefoxNightly.app/Contents/Resources/libnss3.dylib does not exist)

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Nickolay_Ponomarev]

Using a profile with sync account set up:

about:sync-log shows:
> Sync.Service  INFO    Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:35.0) Gecko/20100101 Firefox/35.0
> Sync.Service  DEBUG   Crypto check failed: Error: couldn't open library /Applications/FirefoxNightly.app/Contents/Resources/libnss3.dylib
> Sync.Service  INFO    Could not load the Weave crypto component. Disabling Weave, since it will not work correctly.

followed by:
> Sync.Service  DEBUG   Exception: Error: Can't sync: Can't sync: Weave is disabled

libnss.dylib is in a different location:
$ find /Applications/FirefoxNightly.app/ -name "libnss3.dylib"
/Applications/FirefoxNightly.app//Contents/MacOS/libnss3.dylib

Guessing it's regression from bug 1047584.

Comment 2

[Richard Newman [:rnewman]]

Given that we use

        // Open the NSS library.
        let path = ctypes.libraryName("nss3");

to load this, and the fallback code in WeaveCrypto:

        } catch(e) {
            // In case opening the library without a full path fails,
            // try again with a full path.
            let file = Services.dirsvc.get("GreD", Ci.nsILocalFile);
            file.append(path);
            this.log("Trying again with path " + file.path);
            nsslib = ctypes.open(file.path);
        }

doesn't work on my machine (Services.dirsvc.get("GreD"...) returns undefined), I imagine it's ctypes.open that needs fixing.

Comment 3

[Richard Newman [:rnewman]]

Desktop/toolkit/etc. folks, could someone be assigned to track this down and fix it? It looks like a ctypes library load issue, but it might be a directoryservice bug.

Comment 4

[Robert Strong (they/them - no direct email)]

Attachment: patch rev1

Richard, could you try this out as a possible fix? What happened is Apple now requires executable binaries to be in Contents/MacOS and just about everything else to be under Contents/Resources for their v2 signatures so we had to either go with GreD being Contents/Resources or Contents/MacOS and went with the former. Instead of using GreD this uses the application binary's parent directory.

Comment 5

[Robert Strong (they/them - no direct email)]

Pushed to oak so there will be nightly oak builds with this patch
https://hg.mozilla.org/projects/oak/rev/09d82d7ed24e

Comment 6

[Richard Newman [:rnewman]]

That returns the correct path when I test in the console (though I haven't tested a build).

But what will this change do on other platforms?

Do we need to keep the GreD fallback for Windows, Linux, other?

Is there perhaps a better way to ask for the absolute path of the libnss3 that we include with Firefox? I think the "ask for the system one" approach is likely to be undesirable these days (see Bug 1034979), so perhaps we could avoid composing paths altogether.

Comment 7

[Robert Strong (they/them - no direct email)]

(In reply to Richard Newman [:rnewman] from comment #6)
> That returns the correct path when I test in the console (though I haven't
> tested a build).
> 
> But what will this change do on other platforms?
It should still work the same except possibly for xulrunner builds. I'll run this by bsmedberg for review since he tends to have the big picture in mind for these types of changes.

> Do we need to keep the GreD fallback for Windows, Linux, other?
> 
> Is there perhaps a better way to ask for the absolute path of the libnss3
> that we include with Firefox? I think the "ask for the system one" approach
> is likely to be undesirable these days (see Bug 1034979), so perhaps we
> could avoid composing paths altogether.
Personally, I was kind of surprised this doesn't try to use the one provided by the application first and if it wasn't available fallback to trying to load it without the path. I'll run that by bsmedberg as well.

Comment 8

[Robert Strong (they/them - no direct email)]

Comment on attachment 8498634 [details] [diff] [review]
patch rev1

Benjamin, this should get things working again for sync. Could you please also see comment #7 regarding a couple of concerns and if / how you think they should be addressed.

Comment 9

[Benjamin Smedberg]

Comment on attachment 8498634 [details] [diff] [review]
patch rev1

ISTM that "XREExeF" is the wrong key for this theoretically: in the case of webapprt for example, XREExeF will be pointing at the webapp bundle, not the Firefox bundle.

Do we need to have a key name specifically for "the location of the GRE DLLs" which will be the same as GreD on non-mac and point to Contents/MacOS on mac?

As a short-term fix, I guess this is better than nothing...

Comment 10

[Richard Newman [:rnewman]]

Comment on attachment 8498634 [details] [diff] [review]
patch rev1

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

f+ on one of:

* Using two fallbacks, one for Mac and one for other platforms
* Defining a new directoryservice key, as Benjamin suggested
* Exposing some mechanism (e.g., that directoryservice key) to fetch the correct local path, and using that as the primary approach.

Comment 11

[Robert Strong (they/them - no direct email)]

Going to go with the additional service key

Comment 12

[Robert Strong (they/them - no direct email)]

Attachment: patch

This works with the patch in bug 1077099 which adds a new dir service key. Requesting review so they both can land at the same time.

Comment 13

[Robert Strong (they/them - no direct email)]

Try was locked up so I pushed to oak
https://hg.mozilla.org/projects/oak/rev/bb4b10aa46ea

Comment 14

[Robert Strong (they/them - no direct email)]

Found a bug in the dependent patch and had to backout. We'll get this figured out tomorrow.

Comment 15

[Richard Newman [:rnewman]]

Comment on attachment 8499331 [details] [diff] [review]
patch

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

Assuming that the dependent bug correctly defines GreBinD on all platforms, this looks fine to me, and thanks for the speedy fix.

Comment 16

[Stephen A Pohl [:spohl]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/382cc6a75564
https://hg.mozilla.org/projects/oak/rev/ed340699421e

Comment 17

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/382cc6a75564

Comment 18

[Robert Strong (they/them - no direct email)]

Landed on aurora in the Mac V2 signing combined patch in bug 1047584