Closed Bug 1075981 Opened 5 years ago Closed 5 years ago

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)

Categories

(Firefox :: Sync, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35
Tracking Status
firefox34 --- fixed
firefox35 --- fixed

People

(Reporter: asqueella, Assigned: rstrong)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

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.
Duplicate of this bug: 1075649
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.
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.
Flags: needinfo?(jorendorff)
Attached patch patch rev1 (obsolete) — Splinter Review
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.
Attachment #8498634 - Flags: feedback?(rnewman)
Pushed to oak so there will be nightly oak builds with this patch
https://hg.mozilla.org/projects/oak/rev/09d82d7ed24e
Assignee: nobody → robert.strong.bugs
Status: NEW → ASSIGNED
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.
Flags: needinfo?(jorendorff)
(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 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.
Attachment #8498634 - Flags: review?(benjamin)
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 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.
Attachment #8498634 - Flags: feedback?(rnewman) → feedback+
Going to go with the additional service key
Depends on: 1077099
Attached patch patchSplinter Review
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.
Attachment #8498634 - Attachment is obsolete: true
Attachment #8498634 - Flags: review?(benjamin)
Attachment #8499331 - Flags: review?(rnewman)
Found a bug in the dependent patch and had to backout. We'll get this figured out tomorrow.
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.
Attachment #8499331 - Flags: review?(rnewman) → review+
https://hg.mozilla.org/mozilla-central/rev/382cc6a75564
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Landed on aurora in the Mac V2 signing combined patch in bug 1047584
Component: Firefox Sync: Crypto → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.