Get rid of/Change GetPersistentDescriptor/SetPersistentDescriptor

RESOLVED FIXED in mozilla1.9.3a1

Status

()

Core
XPCOM
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: vlad, Assigned: adw)

Tracking

Trunk
mozilla1.9.3a1
All
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [ts])

Attachments

(1 attachment, 1 obsolete attachment)

1.30 KB, patch
Josh Aas
: review+
Details | Diff | Splinter Review
We use Get/SetPersistentDescriptor in a bunch of prefs, xpti, etc. code to obtain a "persistent descriptor" for a particular file.  On all platforms other than OSX, it's just a plain pathname.  On OSX, it's some magic OS Alias reference, which is able to track files across moves, renames, whatever; it's also the cause of base64-encoded filename-blobs in our OSX profile dirs.

The other platforms get away with using native pathnames, and nothing blows up if the user moves or renames one of those files; given that they're generally in the profile dir, they probably won't.

So we should:

- change nsLocalFileOSX's Get/SetPersistent* to just use the pathname (keeping the parsing in Set for backwards compat, since it already handles pathnames that start with /)

and/or

- stop using the PersistentDescriptor stuff and just use pathnames everywhere (Windows does some stuff with UTF8 vs non UTF8 though, so maybe that's harder).

Going through this stuff makes us hit Carbon file URLs and end up super slow.
Hardware: x86 → All
-> adw for investigation
Assignee: nobody → adw
(Assignee)

Comment 2

9 years ago
Vlad, is this comment relevant?  A reason aliases are used on OS X rather than paths?
http://mxr.mozilla.org/mozilla-central/source/xpcom/io/nsIFile.idl#231
(Assignee)

Comment 3

9 years ago
Created attachment 406075 [details] [diff] [review]
patch

This is too simplistic, right?  Who can review this?
(Assignee)

Updated

9 years ago
Attachment #406075 - Flags: review?(vladimir)

Comment 4

9 years ago
Comment on attachment 406075 [details] [diff] [review]
patch

You can't change those methods. Their behavior is part of the API, with your modifications those methods do not create persistent descriptors. You should change the consumers to use path methods.
Attachment #406075 - Flags: review-
(Assignee)

Updated

9 years ago
Attachment #406075 - Flags: review?(vladimir)
(Assignee)

Comment 5

9 years ago
Thanks Josh.

Recording my thoughts:  Existing prefs (e.g., complex prefs of type nsILocalFile [1]), xpti.dat entries, etc. already contain base64-encoded aliases.  If we change over to paths, we don't know whether a given pref value, xpti.dat entry, etc. is an alias or path -- whether to set persistentDescriptor or call initWithPath.  A simple solution is to first assume it's an alias, and if SetPersistentDescriptor fails because the data is not base64-encoded, then it must be a path.  (OS X's InitWithPath could do this check, or a new method InitWithPathOrPersistentDescriptor, or worst rewriting all consumers to do it themselves.)  Unix and Windows won't feel the difference, but there's extra cost at runtime on OS X.  Or, is there some migration path to hook into, and if so, can the locations on disk of all existing base64-encoded aliases be determined during migration?  Is this even worth worrying about?

[1] http://mxr.mozilla.org/mozilla-central/source/modules/libpref/src/nsPrefBranch.cpp#281
(In reply to comment #2)
> Vlad, is this comment relevant?  A reason aliases are used on OS X rather than
> paths?
> http://mxr.mozilla.org/mozilla-central/source/xpcom/io/nsIFile.idl#231

That was true back in MacOS pre-X days; I honestly don't think we care any more, and shouldn't treat OSX any different than we do any other Unix system.

Comment 7

9 years ago
I agree with vlad: paths should *be* the persistent descriptors we use. The only issue I have is whether we need to provide any upgrade path, in case these persistent descriptors are stored anywhere in prefs that persists between versions. It's not an issue for fastload or the other caches.
(Assignee)

Comment 8

9 years ago
Are prefs really the only place persistent descriptors are persistently stored?  Looking through mxr they are also potentially stored in at least:

* extensions.ini
* download manager DB: mime type => path of preferred application
* profiles.ini
* nsNetscapeProfileMigratorBase, nsProfileMigrator get descriptors from some NSPR registry

1) Are these all caches?
2) If descriptors are persistently stored, and it appears they certainly can be, then there's no choice but to do a migration (or check at every Set per comment 5).  Where's the best place to do it?  Is it possible to hook into .autoreg/compatibility.ini invalidation, or can the updater do stuff like this?

Comment 9

9 years ago
I don't think it matters. A path-style persistent descriptor will always start with /. The old style will always start with an alphanumeric. Just detect which kind you have in ReadPersistentDescriptor, keep the code to load the old kind, but only write the new kind.
(Assignee)

Comment 10

9 years ago
Created attachment 406364 [details] [diff] [review]
patch v2

This should be it then...
Attachment #406075 - Attachment is obsolete: true
Attachment #406364 - Flags: review?(joshmoz)

Updated

8 years ago
Attachment #406364 - Flags: review?(joshmoz) → review+
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/5372b453a52e
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1

Updated

8 years ago
Blocks: 574952
You need to log in before you can comment on or make changes to this bug.