User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:93.0) Gecko/20100101 Firefox/93.0

Steps to reproduce:

The problem probably started after installing firefox version 93.

Actual results:

If I am logged into an account in a Windows domain, Firefox sync does not work. The roaming profile is hosted on a corporate server on the LAN network, this is easily accessible. This seems to be bothering more users on our network. If I am logged into a local account on the same machine, the error does not occur.
Local computers: Win10, FF 93
Server: Windows 2012

The Bugbug bot thinks this bug should belong to the 'Firefox::Sync' component, and is moving the bug to that component. Please revert this change in case you think the bot is wrong.

Really, I uninstalled version 93, then installed version 90.0.2 - and synchronization works there.

The error in question is:

Could not initialize engine : OperationError: PathUtils.filename: Could not initialize path: NS_ERROR_FILE_UNRECOGNIZED_PATH Stack trace: load()@resource://gre/modules/JSONFile.jsm:234

Sync doesn't do anything fancy with JSONFile and JSONFile changed how it opens files in bug 1649604, which landed in 92 (which explains why rolling back to 90 worked for you) - so I suspect there's alot more than just Sync broken here. It looks like an issue with PathUtils/IOUtils, so adjusting the component to where that lives.

Per comment #3, seems like a regression of bug 1649604?

The PathUtils.filename method implementation lives in DOM, so the previous component was right. Though it defers pretty immediately to nsIFile: https://searchfox.org/mozilla-central/rev/5122357c497684e01c5bb2d4a9bf8be1fe97a413/dom/system/PathUtils.cpp#111-112 .

Is there a way of getting debug info so we know exactly what the path was that fails? It looks like that would have come from https://searchfox.org/mozilla-central/rev/5122357c497684e01c5bb2d4a9bf8be1fe97a413/services/sync/modules/util.js#351 , and I'm worried that timing changes impact the availability of profileDir now that other consumers of OS.File have switched to IOUtils (which also means that I'm not 100% sure the regressing bug is set correctly, though it hardly matters).

Set release status flags based on info from the regressing bug 1649604

Is there a way of getting debug info so we know exactly what the path was that fails?

Unfortunately not - we consider the profile directory PII, so we'd consider it a bug if it ever appeared in the logs. The OP could almost certainly tell you what about:support reports if that's helpful.

Note also that:

• Sync is initialized roughly 10 seconds after Firefox starts, so it being timing related seems unlikely.
• I've never seen other reports of this, so it seems highly likely it's due to the fact the profile is on a "roaming profile" rather than something more general.

Without looking into this too deeply... It's possible this could be a problem with the file path being too long. See bug 1536796 which (conservatively) addressed that problem for QuotaManager.

(In reply to Andrew Sutherland [:asuth] (he/him) from comment #8)

Without looking into this too deeply... It's possible this could be a problem with the file path being too long. See bug 1536796 which (conservatively) addressed that problem for QuotaManager.

Reporter, is it possible the path to which the file is being saved is "long" (more than 200 or so characters) ? If not, are there any non-ascii characters in it?

Hello,
I'm going all the way to the roaming profile:
\OAJL6\DATA\kulhavy\AppData\Roaming\Mozilla\Firefox\Profiles\l9kxafpy.default-release-1
(less than 100 characters)
Thanks for caring.

Would you mind checking if the problem goes away with this build: https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/XEOQivH9TWWdY-PTRIcQSQ/runs/0/artifacts/public/build/target.zip ? It has the patch from bug 1731049 in it, and I'm wondering if that addresses the same issue.

Hm, actually, never mind, it is very unlikely that that is the case... We'll need to come up with a way to gather more details about what's going wrong here.

I can reproduce this by attempting to use FxA sync with a profile on a network share.

This is due to sync creating invalid file paths on Windows: https://searchfox.org/mozilla-central/rev/674b6582bafedc11fba6dc537769c88e9ed921d3/services/sync/tests/unit/test_syncengine.js#119

In bug 1649604, JSONFile was rewritten to use IOUtils and PathUtils for file IO
and path management. This means that all path operations go through nsIFile
methods. However, sync engines were generating paths that always contained a
forward slash. If that path is an UNC path (i.e., if your profile is located on
a network drive), all IOUtils and PathUtils operations on that file will fail
due to nsLocalFile::InitWithPath on windows rejecting UNC paths that contain a
forward slash with NS_ERROR_FILE_UNRECOGNIZED_PATH. This meant that you could
not use FxA sync with a profile on a network drive.

Updating these engines to use PathUtils to join the paths fixes this issue and
allows sync to work with profiles on network drives.

Depends on D131161

(In reply to Barret Rennie [:barret] (they/them) from comment #15)

However, sync engines were generating paths that always contained a
forward slash.

That's very interesting - thanks! But is just fixing Sync the best approach here? Isn't it possible there are other cases of this which haven't yet been reported by someone using a profile on a ram drive, and isn't there a risk forward slashes will be introduced in the future, with the developer feeling safe in the knowledge that all QA and CI works, just to discover it breaks in these edge-cases? How can we be confident we've discovered all cases even in sync here?

Looking again at nsLocalFileWin, it seems it always rejects file paths that contain a forward slash. We only catch this on UNC paths because OS.File.Normalize normalizes \ to / but only when the path isnt a UNC path.

We should probably also improve the error messages in PathUtils so that the paths are included in the error messages. It would make cases like this obvious (seeing a UNC path with forward slashes in the error message).

As for introducing invalid paths with forward slashes, I don't know how much we can do. We could perhaps add a lint for calls to PathUtils.join(...args) and check for string literals containing forward slashes, but that would not catch cases like this one where there is >= 1 level of indirection.

I guess my greater point is that JSONFile has used paths like that for well over 10 years and this seems like an unintentional change in semantics that may impact other existing code. I don't feel incredibly strongly about this, but it seems like a new footgun is being introduced. I'd love to hear from Gijs and :asuth here.

I am affected, too.
Samba (4.10) server, with HOME shares (z:) on the (SAMBA) server.

The simple reason why the profile are on a network: backup.
I dont need to care about any users computer, I just backup the NTUSER PROFILE and SAMBA (HOME) shares.

The user's directory is accessible through 2 ways:

• \SERVER\USER\SAMBASHARE
• z:\

As there is a mapping of \SERVER\USER\SAMBSHARE === z:\

(In reply to Mark Hammond [:markh] [:mhammond] from comment #18)

I guess my greater point is that JSONFile has used paths like that for well over 10 years

Not to quibble too much about specifics, but the oldest blame I see on toolkit/modules/JSONFile.jsm is from 2014. Are you talking about a different bit of code? Did sync have its own copy of the module?

and this seems like an unintentional change in semantics that may impact other existing code. I don't feel incredibly strongly about this, but it seems like a new footgun is being introduced. I'd love to hear from Gijs and :asuth here.

I can see that it feels like a footgun if OS.File coped with this, but XPCOM IO interfaces never did, and NSPR and other places are full of #ifdef'd path separator defines for precisely that reason.

XPCOM files mitigated this via the append API and passing objects rather than paths around (and indeed, IIRC there are source code admonitions in nsLocalFIle telling you that passing around just paths is going to make you sad, so perhaps this bug is evidence those admonitions were right - but that was already true for JSONFile/OS.File for a long time, so that ship has sailed...)

We could try to mitigate this "footgun" in IOUtils by preprocessing every instance of an incoming path, but I'm inclined not to try to layer magic of our own on top of OS path munging. From the downloads work, we have extensive experience with the fact that platform-specific requirements coupled with filesystem-specific requirements cause no end of pain (you can mount NTFS disks on Linux, and now you have, as they say, 2 problems). Just yesterday I saw a report basically asking us to allow people to add / into filenames on macOS (ie as part of a filename, not a path separator, which is possible on macOS!) when using the "save as" dialog for downloads. Trying to automate arbitrary-input-into-what-they-really-meant processing here feels super duper risky.

Instead, I think what we should do is add a Windows-only automation-only assertion that fires whenever you call any IOUtils API with a path that contains / on Windows. That will fire even when not actually used on shares, and will allow catching these cases more proactively, should there be any others. It should probably be a separate bug though, as AIUI the patch Barret attached here fixes the issue as reported, and is safe enough we could uplift to 95.

I agree with :Gijs' perspective that:

• Callers should be using PathUtils.join or equivalent. Thanks to being able to do PathUtils.join(...someArray) this can be quite ergonomic.
• It makes sense to add assertions to help immediately identify any existing problems and in recognition of the fact that this is a very easy mistake to make.

At a meta-level, it's unfortunate (but understandable) that PathUtils.join is operating in terms of DOMStrings on both the input and the output. nsIFile has a major advantage that you can't just accidentally use a string as an nsIFile in the type system. It seems like we might need/want some linter rules to try and help. Alternately maybe typescript definitions could be added for PathUtils that could be automatically properly exposed to VSCode so that its embedded typescript magic could help out a little. It might not totally work out with the current design, but the idea could be that PathUtils methods return a synthetic @typedefed type like PlatformPathString but take { PlatformPathString | string } and JSONFile would want a PlatformPathString and so calls at the very least would need to be passed through PathUtils.normalize or something.

(In reply to :Gijs (he/him) from comment #20)

Not to quibble too much about specifics, but the oldest blame I see on toolkit/modules/JSONFile.jsm is from 2014. Are you talking about a different bit of code? Did sync have its own copy of the module?

heh - I didn't dig that deep - I just went back far enough to see that calls to jsonLoad() with forward slashes went back at least 10 years - but I didn't bother seeing what was underneath it, nor seeing how far back it actually went - I stopped at a large refactor :)

Thank you both for your feedback!

Backed out 3 changesets (Bug 1741247, Bug 1723082, Bug 1736175) for causing PathUtils.normalize failures.
Backout link: https://hg.mozilla.org/integration/autoland/rev/72c397486d4cec809c954f4be3df881c2db93d56
Push with failures, failure log.

Backed out for causing xpcshell failures on test_tracker_addChanged.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/6098b54fe064e5dd3b0b312f8bb0b335f24690a8
Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&selectedTaskRun=b6BWjHETSZOLZA-NWDxZqQ.0&revision=7e2d97447e313ad1f0a9d366f1ce434cc1656e0c
Failure log: https://treeherder.mozilla.org/logviewer?job_id=358505037&repo=autoland&lineNumber=2840

https://hg.mozilla.org/mozilla-central/rev/64f6bb909b1a

The patch landed in nightly and beta is affected.
:barret, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment on attachment 9250792 [details]
Bug 1736175 - Use PathUtils for generating paths in sync engines r?markh

Beta/Release Uplift Approval Request

• User impact if declined: Firefox profiles will continue to not work on network shares.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This patch changes some methods to do path joining through an API instead of using forward slashes. The path joining API has been well tested for several versions.
• String changes made/needed:

EDIT: yes this is covered by unit tests for the sync engines module, which all passed.

Comment on attachment 9250792 [details]
Bug 1736175 - Use PathUtils for generating paths in sync engines r?markh

We are late in the beta cycle but this has tests and fixes a recent regression. Approved for 95 beta 11, thanks.

https://hg.mozilla.org/releases/mozilla-beta/rev/21d0a81a7c13

Can you change the bug component to Firefox :: Sync? I think that would make this bug easier to find, since that was the affected feature, and that's where the fix was made. The bug summary should mention Sync too. Maybe "Firefox Sync does not work on a network-based "roaming" profile".

I think that would make this bug easier to find

Given there don't seem to be any other places where the API was abused in a similar way, I think that does make sense.