326668 - compreg.dat and xpti.dat still live in Camino.app

Status: VERIFIED FIXED

(Camino Graveyard :: General, defect)

Description

[Mark Mentovai]

compreg.dat and xpti.dat still live in Camino.app/Contents/MacOS.  Other applications have moved these files into the profile, which is really a better location.

Currently, the Camino build script pregenerates compreg.dat and xpti.dat before packaging.  xpti.dat contains some paths from the Tinderbox that was used to build it and is regenerated (if writable) on first launch anyway.  Moving these files to the profile means no more pregeneration.  This carries the potential drawback of slowing the initial startup slightly.

We should think about moving these files into the profile for Camino too.  This  is probably doable entirely within AppServiceDirProvider.

Comment 1

[Mark Mentovai]

Attachment: nsILocalFileMac patch

We need a function to create an nsILocalFileMac from an FSRef before xpcom has been initialized, since we want to change the location of compreg.dat to something based on an FSRef.

(I said I wouldn't add the function to nsLocalFileMac because it's obsolete, but I accidentally added it to that file first, so here ya go.)

Comment 2

[Mark Mentovai]

Attachment: Camino patch (will be updated)

This moves the .dat files into the profile directory, depending on the xpcom patch for NS_NewLocalFileWithFSRef.  It also gets rid of autoRegister, since autoRegister prior to shipping is pointless now.  I'll need to update the build scripts before checking this in, or else builds will hang because Camino will no longer exit when asked to autoRegister.

Comment 3

[Mark Mentovai]

Comment on attachment 211399 [details] [diff] [review]
nsILocalFileMac patch

 void
+nsresult 
+NS_NewLocalFileWithFSRef(const FSRef* aFSRef, PRBool aFollowLinks, nsILocalFileMac* *result)

"void" does not belong!

Comment 4

[Simon Fraser [no longer active]]

I assume we don't want this for 1.0?

Comment 5

[Mark Mentovai]

There's not really enough time to test it in the wild, although there's nothing dangerous going on.  Pinkerton said we should leave it for 1.1.

Comment 6

[Mark Mentovai]

Actually, this might need build ID checking like during XRE startup:

http://lxr.mozilla.org/mozilla/source/toolkit/xre/nsAppRunner.cpp#2108

And, come to think of it, the compatibility check should probably look at TARGET_XPCOM_ABI to be sure the component list is regenerated when switching architectures.  (Can't rely on the build ID in a universal, it's the same for both CPUs.)

And come to think of that, that's probably the best motivator to move these files into the profile in ANY app - although for now, the only difference in components between ppc and x86 is Talkback, and what we do there is safe, so it's doesn't need to hold up 1.0.  (It'd be a different story if we supported xpxtension components.)

Comment 7

[Mark Mentovai]

I filed bug 326772 to handle reregistration on switching architectures in XRE.

Comment 8

[Josh Aas]

Comment on attachment 211399 [details] [diff] [review]
nsILocalFileMac patch

+    if (file == nsnull)
+        return NS_ERROR_OUT_OF_MEMORY;

Is that kind of check worth anything on Mac OS X these days? I know it is standard protocol to do it, but is that *ever* going to be hit by a single user? I don't recall how much address space processes have...

Comment 9

[Mark Mentovai]

Sure, you could hit a resource limit or the system could be unable to create a new swap file.  Either of those will cause a malloc failure.  Likely?  No.  But hey, they can't blame xpcom if it crashes.

Comment 10

[timeless]

i've actually had seamonkey crash on osx blaming oom. i've also had it crash in the os string land because of oom. these crashes really do happen, and i never appreciate it when they do.

Comment 11

[Mark Mentovai]

dougt, is the nsILocalFileMac patch OK with you?

Comment 12

[Mark Mentovai]

Comment on attachment 211400 [details] [diff] [review]
Camino patch (will be updated)

Cancelling r? on the Camino patch.  I'll want to force rereg when the buildid or other things change (like compatibility.ini) and it won't apply after bug 323780 anyway.

Comment 13

[Doug Turner (:dougt)]

Comment on attachment 211399 [details] [diff] [review]
nsILocalFileMac patch

Why do you have to change the uuid for this?

I would rather have you just say something in a comment about what version it was included with.

Comment 14

[Mark Mentovai]

I know the uuid change isn't strictly necessary code-wise, but there was no guidance about using the uuid to track non-interface changes like this, so I was playing it safe.  I've got no problem leaving it alone.

Comment 15

[Doug Turner (:dougt)]

this is exactly why we try to keep c++ cruft out of idl's. ;-)

leave the uuid alone.  Since this is a public function, you should add a comment as I suggested indicated where this function will be availabe.

Comment 16

[Mark Mentovai]

*** Bug 329138 has been marked as a duplicate of this bug. ***

Comment 17

[Mark Mentovai]

Attachment: nsILocalFileMac patch, v2 (on trunk)

Updated per comments 13-15.

Requesting approval1.8.1 because this is targeted at Camino 1.1, which will come from that branch.

Comment 18

[Doug Turner (:dougt)]

Comment on attachment 214255 [details] [diff] [review]
nsILocalFileMac patch, v2 (on trunk)

if Josh is happy, I am happy.

Comment 19

[Mark Mentovai]

Comment on attachment 214255 [details] [diff] [review]
nsILocalFileMac patch, v2 (on trunk)

Doug is happy!  Checked in on trunk.

Comment 20

[Mark Mentovai]

Doug, is this branchable yet?  We'll need this in 1.8.1 for Camino 1.1.

Comment 21

[Doug Turner (:dougt)]

Comment on attachment 214255 [details] [diff] [review]
nsILocalFileMac patch, v2 (on trunk)

sorry.  feel free.

Comment 22

[Benjamin Smedberg]

Why is NS_NewLocalFileWithFSRef declared in nsILocalFileMac instead of in nsXPCOM.h? (Or, why aren't we holding the line against declaring exports in IDL?) In any case it really needs to use the NS_COM macro instead of NS_EXPORT.

You really didn't have to invent this new API; it's possible to do the same thing with:

nsCOMPtr<nsILocalFile> lf;
NS_NewNativeLocalFile(EmptyString(), PR_FALSE, getter_AddRefs(lf));

nsCOMPtr<nsILocalFileMac> lfm(do_QueryInterface(lf));
lfm->InitWithFSRef(theFSRef);

I don't think this patch should land on 1.8, and should be fixed in nsXPCOM.h or backed out on the trunk.

Comment 23

[Mark Mentovai]

We've got 15 or so calls to NS_NewLocalFileWithFSSpec throughout the codebase already.  Should those be removed too?

Comment 24

[Benjamin Smedberg]

I don't care nearly as much about alreay-bad code, just avoiding new-bad code. But if you want to remove those usages or move NS_NewLocalFileWithFSSpec into nsXPCOM.h I certainly don't mind.

Comment 25

[Mark Mentovai]

Moving the target up on this for bug 336447.

Comment 26

[Stuart Morgan]

*** Bug 337022 has been marked as a duplicate of this bug. ***

Comment 27

[Mark Mentovai]

I have a fully functional 1.8.0-branch patch ready to go, as soon as I clean it up to edit the nasty words out of the comments.

Comment 28

[Mark Mentovai]

NS_NewLocalFileWithFSSpec/FSRef removal in bug 337367.

Comment 29

[froodian (Ian Leue)]

*** Bug 337488 has been marked as a duplicate of this bug. ***

Comment 30

[Mark Mentovai]

Attachment: v2 branch: move compreg.dat and xpti.dat into the profile

Comment 31

[Mark Mentovai]

Attachment: v2 trunk: move compreg.dat and xpti.dat into the profile

These two patches are identical except for the AppDirServiceProvider sections: hwaara, there's no need to review the duplicated parts more than once.

Comment 32

[Håkan Waara]

Comment on attachment 222282 [details] [diff] [review]
v2 trunk: move compreg.dat and xpti.dat into the profile

>+static PRBool
>+CheckCompatibility(nsIFile* aProfileDir, const nsCString& aVersion,
>+                   const nsCString& aOSABI, nsIFile* aAppDir)
>+{

Maybe use abstract string type nsACString& instead as params in these two functions?

>+
>+static void
>+WriteVersion(nsIFile* aProfileDir, const nsCString& aVersion,
>+             const nsCString& aOSABI, nsIFile* aAppDir)

These two functions are some good stuff. Will we get them for free when we're on XULRunner? I'm thinking it's something a regular JohnDoe embeddor shouldn't need to do all by himself.

>+
> @interface PreferenceManager(PreferenceManagerPrivate)
> 
> - (void)registerNotificationListener;
> 
> - (void)termEmbedding: (NSNotification*)aNotification;
> - (void)xpcomTerminate: (NSNotification*)aNotification;
> 
> - (void)showLaunchFailureAndQuitWithErrorTitle:(NSString*)inTitleFormat errorMessage:(NSString*)inMessageFormat;
>@@ -363,17 +468,17 @@ static BOOL gMadePrefManager;
> 
>     NSString *path = [[[NSBundle mainBundle] executablePath] stringByDeletingLastPathComponent];
>     const char *binDirPath = [path fileSystemRepresentation];
>     nsCOMPtr<nsILocalFile> binDir;
>     rv = NS_NewNativeLocalFile(nsDependentCString(binDirPath), PR_TRUE, getter_AddRefs(binDir));
>     if (NS_FAILED(rv))
>     {
>       [self showLaunchFailureAndQuitWithErrorTitle:NSLocalizedString(@"StartupFailureAlert", @"")
>-                                      errorMessage:NSLocalizedString(@"StartupFailureBinPathMsg", @"")];
>+                                      errorMessage:NSLocalizedString(@"StartupFailureMsg", @"")];

You remember to edit that in the plist too, if it doesn't exist, right?

Comment 33

[Håkan Waara]

Comment on attachment 222281 [details] [diff] [review]
v2 branch: move compreg.dat and xpti.dat into the profile

Same comments as trunk patch apply.

Comment 34

[Mark Mentovai]

Comment on attachment 222281 [details] [diff] [review]
v2 branch: move compreg.dat and xpti.dat into the profile

> Maybe use abstract string type nsACString& instead as params in these two
> functions?

'k.

> These two functions are some good stuff. Will we get them for free when we're
> on XULRunner? I'm thinking it's something a regular JohnDoe embeddor shouldn't
> need to do all by himself.

Yeah, I think we'll get that with the toolkit profile stuff.

> You remember to edit that in the plist too, if it doesn't exist, right?

Actually, I didn't mean to change that one, I'll fix it.  I did only use keys that already exist in the strings file.

Comment 35

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 222281 [details] [diff] [review]
v2 branch: move compreg.dat and xpti.dat into the profile

sr=pink

Comment 36

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 222282 [details] [diff] [review]
v2 trunk: move compreg.dat and xpti.dat into the profile

sr=pink

Comment 37

[Mark Mentovai]

Checked in on trunk, MOZILLA_1_8_BRANCH (1.8.1), MOZILLA_1_8_0_BRANCH (1.8.0.4), and CAMINO_1_0_1_MINIBRANCH in case 1.0.2 comes from there.

Comment 38

[[e-mail]]

Verified on 1.0.2, 1.8.0, 1.8 branch and trunk