177321 - nsIDirectoryServiceProvider needs to be factored out of nsProfile.cpp

Status: RESOLVED FIXED

(Core Graveyard :: Profile: BackEnd, defect)

Description

[Conrad Carlen (not reading bugmail)]

The are 2 implementations of nsIDirectoryServiceProvider which provide
profile-relative file locations:

(1) in modules/mpfilelocprovider which build a static lib
(2) in nsProfile.cpp

This is wrong, particularly for profile sharing. In that world, it should be
possible to share the single "profile" that exists when apps use the standalone
provider as well as for apps that use the profile mgr. Work will have to be done
to the profile provider for sharing and it needs to happen in only one place.

I have a patch which builds this provider as a static lib in the profile module.
The profile mgr dll links with it but, since the static lib is exported, apps
will be able to link with it as well (as the gtkWidget links with the static lib
produced in modules/mpfilelocprovider). With the patch, the entire
modules/mpfilelocprovider dir can be removed from the tree.

Comment 1

[Conrad Carlen (not reading bugmail)]

Attachment: patch v1

Comment 2

[Conrad Carlen (not reading bugmail)]

Attachment: patch v2

Slight fixes. Built now on Mac CFM, Mac Mach-0, and clobber on Linux. Need
r=/sr=.

Comment 3

[Conrad Carlen (not reading bugmail)]

Doug, Darin - can you do the r=/sr=? CC'ing blizzard for the changes to Gtk
widget (which actually uses this thing w/o profile mgr)

Comment 4

[Doug Turner (:dougt)]

Comment on attachment 105537 [details] [diff] [review]
patch v2

r=dougt

Comment 5

[Darin Fisher]

Comment on attachment 105537 [details] [diff] [review]
patch v2

>Index: embedding/browser/gtk/src/EmbedPrivate.cpp

>+    nsProfileDirServiceProvider *locProvider;
...
>+    locProvider = new nsProfileDirServiceProvider(PR_TRUE);
>+    rv = locProvider->Initialize();
>+    if (NS_FAILED(rv))
>+      return rv;

looks like locProvider would leak here as well as below on failure!

>+    rv = locProvider->SetProfileDir(profileDir);
>+    if (NS_FAILED(rv))
>+      return rv;


>Index: embedding/browser/photon/src/PtMozilla.cpp

>+    profileDir = do_CreateInstance(NS_LOCAL_FILE_CONTRACTID);
...
>+    rv = profileDir->InitWithNativePath(nsDependentCString(sProfileDir));

nit: how about calling NS_NewNativeLocalFile instead?  it avoids a hit
on the component manager.

>+    nsProfileDirServiceProvider *locProvider;
...
>+    locProvider = new nsProfileDirServiceProvider(PR_TRUE);
>+    rv = locProvider->Initialize();
>+    if (NS_FAILED(rv))
>+      return rv;
>+    rv = locProvider->SetProfileDir(profileDir);
>+    if (NS_FAILED(rv))
>+      return rv;

again locProvider would leak!


>Index: embedding/browser/powerplant/source/CBrowserApp.cp

>+    nsProfileDirServiceProvider *provider = new nsProfileDirServiceProvider(PR_TRUE);
>+    ThrowIfNil_(provider);
>+    rv = provider->Initialize();   
>+    ThrowIfError_(rv);    

possible leak??


>Index: profile/build/nsProfileFactory.cpp

>+ * Version: NPL 1.1/GPL 2.0/LGPL 2.1

NPL or MPL?  i thought MPL/GPL/LGPL was the preferred license for new
files, no?


>Index: profile/dirserviceprovider/Makefile.in

>+# The contents of this file are subject to the Netscape Public
>+# License Version 1.1 (the "License"); you may not use this file

this also looks to be the wrong license.  double check all the 
licenses for new files.

>Index: profile/dirserviceprovider/public/nsProfileDirServiceProvider.h

>+   * @param aNotifyObservers	If TRUE, profile change notifications will
>+   *													be sent on SetProfileDir
>+   */
>+
>+                          nsProfileDirServiceProvider(PRBool aNotifyObservers);

wierd indentation throughout this file.

>Index: profile/dirserviceprovider/src/nsProfileDirServiceProvider.cpp

>+  nsIAtom* inAtom = NS_NewAtom(prop);

nsCOMPtr<nsIAtom> inAtom ??


>+  if (localFile && NS_SUCCEEDED(rv))
>+    return localFile->QueryInterface(NS_GET_IID(nsIFile), (void**)_retval);

nit:
      return CallQueryInterface(localFile, _retval);

>Index: profile/src/nsProfileAccess.cpp

>+#if DEBUG
>+        printf("Size of struct flock = %d\n", sizeof(lock));
>+#endif

should this be DEBUG_ccarlen?  or maybe we don't even execute this code.

Comment 6

[Conrad Carlen (not reading bugmail)]

Attachment: patch v3

New patch cleans up the possible leaks on failure by making the constructor of
nsProfileDirServiceProvider protected. You can now only create one through
NS_NewProfileDirServiceProvider, which returns an addref'd object, so an
nsCOMPtr can be used for the local vars which may have leaked. Also,
Initialize() is protected now and called by the NS_New... function.


>+    profileDir = do_CreateInstance(NS_LOCAL_FILE_CONTRACTID);
...
>+    rv = profileDir->InitWithNativePath(nsDependentCString(sProfileDir));

nit: how about calling NS_NewNativeLocalFile instead?  it avoids a hit
on the component manager.

2. Changed that, though it wasn't mine.

3. Using MPL in all the new files.

>Index: profile/dirserviceprovider/public/nsProfileDirServiceProvider.h


wierd indentation throughout this file.

4. Fixed that - it was full of tabs.

>Index: profile/dirserviceprovider/src/nsProfileDirServiceProvider.cpp

>+  nsIAtom* inAtom = NS_NewAtom(prop);

nsCOMPtr<nsIAtom> inAtom ??

5. Kept that. it is realeased at the end, there are no early exits which get
avoid that, and if it were an nsCOMPtr, each == in the method would have to
invoke the operator T* of nsCOMPtr, or get() or whatever. The raw pointer is
safer and more efficient here.


>+  if (localFile && NS_SUCCEEDED(rv))
>+    return localFile->QueryInterface(NS_GET_IID(nsIFile), (void**)_retval);

nit:
      return CallQueryInterface(localFile, _retval);

6. Did that.

>Index: profile/src/nsProfileAccess.cpp

>+#if DEBUG
>+	  printf("Size of struct flock = %d\n", sizeof(lock));
>+#endif

7. This garbage shouldn't have been in the patch :-/ Gone.

Another look, Darin?

Comment 7

[Darin Fisher]

Comment on attachment 105621 [details] [diff] [review]
patch v3

sr=darin

Comment 8

[Conrad Carlen (not reading bugmail)]

Checked in.