Closed Bug 1435376 Opened 5 years ago Closed 5 years ago

set NSS_SDB_USE_CACHE to yes if the profile is on a networked drive (windows)

Categories

(Core :: Security: PSM, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: keeler, Assigned: keeler)

References

Details

(Whiteboard: [psm-assigned])

Attachments

(1 file)

Performance can be significantly impacted if we're using the sqlite-backed cert/key dbs and the user's profile is on a networked drive. Setting the environment variable NSS_SDB_USE_CACHE to "yes" appears to help: bug 1432484 comment 11 bug 1432484 comment 12 and probably bug 1434548.

Windows has GetDriveType() ( https://stackoverflow.com/questions/3517175/detecting-if-path-is-on-a-windows-mapped-network-drive )
Linux might be more involved: https://superuser.com/questions/338033/check-if-a-folder-is-on-a-shared-or-local-disk
OS X hopefully has a similar method or maybe the linux way will just work.
I did a try push to make sure nothing on localhost would change with this env set:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=9f210b3916fc&newProject=try&newRevision=100cb77415dc&framework=1&showOnlyImportant=1

no issues- so it looks to be isolated to NFS.
Duplicate of this bug: 1434548
This is just on Windows for now - I'll file follow-ups for other platforms.
Summary: set NSS_SDB_USE_CACHE to yes if the profile is on a networked drive → set NSS_SDB_USE_CACHE to yes if the profile is on a networked drive (windows)
Comment on attachment 8957386 [details]
bug 1435376 - set NSS_SDB_USE_CACHE to yes if the profile is on a remote drive (windows version)

https://reviewboard.mozilla.org/r/226296/#review232232
Attachment #8957386 - Flags: review?(mhowell) → review+
Comment on attachment 8957386 [details]
bug 1435376 - set NSS_SDB_USE_CACHE to yes if the profile is on a remote drive (windows version)

https://reviewboard.mozilla.org/r/226296/#review232234

::: security/manager/ssl/nsNSSComponent.cpp:1778
(Diff revision 1)
> +  if (!profileFileWin) {
> +    MOZ_LOG(gPIPNSSLog, LogLevel::Debug, ("couldn't get nsILocalFileWin?"));
> +    return;
> +  }
> +  nsAutoString profilePath;
> +  rv = profileFileWin->GetCanonicalPath(profilePath);

Please just use GetPath(). The "canonical" path means 8.3 format short path.

I consider removing GetCanonicalPath() once NSS has supported Unicode paths fully.
Comment on attachment 8957386 [details]
bug 1435376 - set NSS_SDB_USE_CACHE to yes if the profile is on a remote drive (windows version)

https://reviewboard.mozilla.org/r/226296/#review232234

> Please just use GetPath(). The "canonical" path means 8.3 format short path.
> 
> I consider removing GetCanonicalPath() once NSS has supported Unicode paths fully.

Ah, NativePath() would be even better.
Comment on attachment 8957386 [details]
bug 1435376 - set NSS_SDB_USE_CACHE to yes if the profile is on a remote drive (windows version)

https://reviewboard.mozilla.org/r/226296/#review232238

::: security/manager/ssl/nsNSSComponent.cpp:1778
(Diff revision 1)
> +  if (!profileFileWin) {
> +    MOZ_LOG(gPIPNSSLog, LogLevel::Debug, ("couldn't get nsILocalFileWin?"));
> +    return;
> +  }
> +  nsAutoString profilePath;
> +  rv = profileFileWin->GetCanonicalPath(profilePath);

Furthermore, the path must ends with a trailing backslash[1]. It would be better to expose IsRemoteFilePath[2] and use it instead of re-inventing the wheel.

[1] https://msdn.microsoft.com/en-us/library/windows/desktop/aa364939(v=vs.85).aspx
[2] https://dxr.mozilla.org/mozilla-central/rev/a6a32fb286fa9e5d5f6d5b3b77423ab6b96c9502/xpcom/io/nsLocalFileWin.cpp#1701
Comment on attachment 8957386 [details]
bug 1435376 - set NSS_SDB_USE_CACHE to yes if the profile is on a remote drive (windows version)

https://reviewboard.mozilla.org/r/226296/#review232354

LGTM modulo :emk's comments.

::: security/manager/ssl/nsNSSComponent.cpp:1793
(Diff revision 1)
> +    MOZ_LOG(gPIPNSSLog, LogLevel::Debug,
> +            ("profile is remote (and NSS_SDB_USE_CACHE wasn't set): "
> +             "setting NSS_SDB_USE_CACHE"));
> +    PR_SetEnv("NSS_SDB_USE_CACHE=yes");
> +  } else {
> +    MOZ_LOG(gPIPNSSLog, LogLevel::Debug, ("not setting NSS_SDB_USE_CACHE"));

nit: feels like this would be more useful as "profile is not remote, not setting NSS_SDB_USE_CACHE" but no one reads debug logs on Windows :D
Attachment #8957386 - Flags: review?(jjones) → review+
Comment on attachment 8957386 [details]
bug 1435376 - set NSS_SDB_USE_CACHE to yes if the profile is on a remote drive (windows version)

https://reviewboard.mozilla.org/r/226296/#review232238

> Furthermore, the path must ends with a trailing backslash[1]. It would be better to expose IsRemoteFilePath[2] and use it instead of re-inventing the wheel.
> 
> [1] https://msdn.microsoft.com/en-us/library/windows/desktop/aa364939(v=vs.85).aspx
> [2] https://dxr.mozilla.org/mozilla-central/rev/a6a32fb286fa9e5d5f6d5b3b77423ab6b96c9502/xpcom/io/nsLocalFileWin.cpp#1701

This path does not have to end with a trailing backslash, only the volume passed to GetDriveType does. The call to GetVolumePathName is taking care of that requirement for us.
(In reply to Masatoshi Kimura [:emk] from comment #8)
> Comment on attachment 8957386 [details]
> bug 1435376 - set NSS_SDB_USE_CACHE to yes if the profile is on a remote
> drive (windows version)
> 
> https://reviewboard.mozilla.org/r/226296/#review232238
> 
> ::: security/manager/ssl/nsNSSComponent.cpp:1778
> (Diff revision 1)
> > +  if (!profileFileWin) {
> > +    MOZ_LOG(gPIPNSSLog, LogLevel::Debug, ("couldn't get nsILocalFileWin?"));
> > +    return;
> > +  }
> > +  nsAutoString profilePath;
> > +  rv = profileFileWin->GetCanonicalPath(profilePath);
> 
> Furthermore, the path must ends with a trailing backslash[1]. It would be
> better to expose IsRemoteFilePath[2] and use it instead of re-inventing the
> wheel.
> 
> [1]
> https://msdn.microsoft.com/en-us/library/windows/desktop/aa364939(v=vs.85).
> aspx
> [2]
> https://dxr.mozilla.org/mozilla-central/rev/
> a6a32fb286fa9e5d5f6d5b3b77423ab6b96c9502/xpcom/io/nsLocalFileWin.cpp#1701

I think this code is actually incorrect - PathRemoveFileSpec only removes the trailing file name from a path, whereas (as I understand it) GetDriveType requires the root directory for a drive. I filed bug 1444511 for this.
Pushed by dkeeler@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4622767450bd
set NSS_SDB_USE_CACHE to yes if the profile is on a remote drive (windows version) r=jcj,mhowell
https://hg.mozilla.org/mozilla-central/rev/4622767450bd
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
See Also: → 1456888
This seems like it would be helpful once 60esr hits my workplace. Everyone's profiles are stored on the network for virtual desktop access. Could this be added to 60.1esr?
You need to log in before you can comment on or make changes to this bug.