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

Status: RESOLVED FIXED

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

Description

[Dana Keeler (she/her) (use needinfo) [:keeler]]

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.

Comment 1

[Joel Maher ( :jmaher ) (UTC -8)]

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.

Comment 3

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Attachment: bug 1435376 - set NSS_SDB_USE_CACHE to yes if the profile is on a remote drive (windows version)

Review commit: https://reviewboard.mozilla.org/r/226296/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/226296/

Comment 4

[Dana Keeler (she/her) (use needinfo) [:keeler]]

This is just on Windows for now - I'll file follow-ups for other platforms.

Comment 5

[Molly Howell (she/her) (mostly inactive)]

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

Comment 6

[Masatoshi Kimura [:emk]]

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 7

[Masatoshi Kimura [:emk]]

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 8

[Masatoshi Kimura [:emk]]

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 9

[J.C. Jones [:jcj] (he/they)]

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

Comment 10

[Molly Howell (she/her) (mostly inactive)]

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.

Comment 11

[Dana Keeler (she/her) (use needinfo) [:keeler]]

(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.

Comment 12

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8957386 [details]
bug 1435376 - set NSS_SDB_USE_CACHE to yes if the profile is on a remote drive (windows version)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/226296/diff/1-2/

Comment 13

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=db4ad84704c2053042fa6f12b546ae907cf688ca
Thanks for the reviews!

Comment 14

[Pulsebot]

Pushed by [e-mail]:
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

Comment 15

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/4622767450bd

Comment 16

[mws]

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?