214889 - remove nsFileSpec from xpinstall

Status: RESOLVED FIXED

(Core Graveyard :: Installer: XPInstall Engine, defect, P2)

Description

[cls]

We need to remove the uses of nsFileSpec & friends from xpinstall.

Comment 1

[cls]

Attachment: v1.0

Comment 2

[Simon Paquet [:sipaq]]

Comment on attachment 129117 [details] [diff] [review]
v1.0

Benjamin, Dan, I found this patch lying around. It would be great if you could
review it.

Comment 3

[Benjamin Smedberg]

Comment on attachment 129117 [details] [diff] [review]
v1.0

>+    char buffer[1024];
>+    PRUint32 dummy;
>+    snprintf(buffer, sizeof(buffer), "-------------------------------------------------------------------------------\n");
>+    rv = mLogStream->Write(buffer, strlen(buffer), &dummy);
>+    if (NS_FAILED(rv)) return rv;
>+    snprintf(buffer, sizeof(buffer), "%s -- %s\n", NS_ConvertUCS2toUTF8(URL).get(), time);
>+    rv = mLogStream->Write(buffer, strlen(buffer), &dummy);
>+    if (NS_FAILED(rv)) return rv;
>+    snprintf(buffer, sizeof(buffer), "-------------------------------------------------------------------------------\n\n");
>+    rv = mLogStream->Write(buffer, strlen(buffer), &dummy);
>+    if (NS_FAILED(rv)) return rv;
>     PL_strfree(time);

to reduce codesize and prevent leaking "time", let's coalesce the failed case
by using rv |=	 In addition, we can avoid the strlen() by using the return
value from nsprintf.

Come to think of it, why do we need three separate calls anyway? Let's just do
one large nsprintf with all three lines.

Same pattern occurs frequently, so we need to fix all callers. Otherwise, looks
reasonable.

Comment 4

[Simon Paquet [:sipaq]]

Reassigning to Benjamin per IRC conversation.

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: alternative

I created this patch unaware of the other one here...

this is using an overloaded operator<< for the output (required less changes).
what do you think, can I keep it or should I switch to directly calling Write?

Comment 6

[Benjamin Smedberg]

Comment on attachment 163903 [details] [diff] [review]
alternative

>Index: src/Makefile.in

Are we ready to remove $(MOZ_XPCOM_OBSOLETE_LIBS) from the link list?

>Index: src/nsInstallFolder.cpp
>@@ -273,25 +271,21 @@ nsInstallFolder::SetDirectoryPath(const 
>                     nsCOMPtr<nsILocalFile> localFile;
>+                    rv = NS_NewLocalFile(aRelativePath, PR_TRUE,
>+                                         getter_AddRefs(localFile));
> 

Is it really a relative path? You can't init a localfile with a relative path.

>                     if (NS_SUCCEEDED(rv))
>                     {
>-                        mFileSpec = do_QueryInterface(localFile);
>+                        mFileSpec = localFile;
>                     }

Why do we need the temporary? Can't we NS_NewLocalFile directly into mFileSpec?

>Index: src/nsLoggingProgressNotifier.cpp

>+nsIOutputStream& operator<<(nsIOutputStream& stream, const char* text) {

>+nsIOutputStream& operator<<(nsIOutputStream& stream, PRInt32 val) {

Why do these signatures take a reference, instead of nsIOutputStream*? I would
prefer the pointer.

>Index: src/nsLoggingProgressNotifier.h

>+class nsIOutputStream;

>+        nsCOMPtr<nsIOutputStream>  mLogStream;

On some platforms we still require the full declaration of nsIOutputStream;
you're going to need to #include the header.

Comment 7

[cls]

Attachment: v1.1

Here's an updated patch with the snprintfs & leaks fixed.  I couldn't figure out how to trigger that code path so I couldn't determine if aRelativePath is always relative.  I do know that the nsFileURL/nsFileSpec interaction would Canonify any given path (using getcwd) so I made an approximation of that interaction.  The temporary, localFile, is needed because you can't call NS_NewNativeLocalFile on a nsIFile (afaict).

Comment 8

[Benjamin Smedberg]

Comment on attachment 204817 [details] [diff] [review]
v1.1

>Index: xpinstall/src/nsInstallFolder.cpp

>-                    nsCOMPtr<nsILocalFile> localFile;
>+                    nsCOMPtr<nsILocalFile> localFile = nsnull;

This shouldn't be necessary.

>Index: xpinstall/src/nsLoggingProgressNotifier.cpp

>+        NS_WARNING("We're being destroyed before script finishes!");

Perhaps this should be an assertion?

This file has lots of buffer[1024] with magic numbers. I'm not sure how these constants were chosen, but I would like some #defines to make it more clear:

#define LARGE_TEXT_BUFFER 2048 // or something like this

>Index: xpinstall/src/nsLoggingProgressNotifier.h

> class nsLoggingProgressListener : public nsIXPIListener

>-        nsOutputFileStream  *mLogStream;
>+        nsIOutputStream  *mLogStream;

COMPtr here, please.

r=me with nits picked

Comment 9

[cls]

Comment on attachment 204817 [details] [diff] [review]
v1.1

> >-                    nsCOMPtr<nsILocalFile> localFile;
> >+                    nsCOMPtr<nsILocalFile> localFile = nsnull;
> 
> This shouldn't be necessary.

gcc doesn't automatically initialize variables to 0.   Is it expected that the nsCOMPtr constructor will automatically null the value or that directoryService->Get() will null the result upon failure?  

> > class nsLoggingProgressListener : public nsIXPIListener
> 
> >-        nsOutputFileStream  *mLogStream;
> >+        nsIOutputStream  *mLogStream;
> 
> COMPtr here, please.

I was trying to avoid the overuse of nsCOMPtr.  Is that a non-issue nowadays?

Comment 10

[Benjamin Smedberg]

We rely on the COMPtr constructor to initialize to 0, and we almost always prefer the use of COMPtrs whenever possible.

Comment 11

[cls]

Attachment: v1.2

Updated patch with nits addressed.

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

-> cls, as he's now working on this bug and I'm not :)

Comment 13

[Benjamin Smedberg]

Comment on attachment 205388 [details] [diff] [review]
v1.2

cls, what happens if xpcom-obsolete is disabled and somebody builds firefox-static? It doesn't look like mozreg_s would ever get linked into the final static binary.

In xpinstall/src/Makefile.in perhaps mozreg_s ought to be in SHARED_LIBRARY_LIBS, but  only if xpcom-obsolete is disabled...

Comment 14

[cls]

Attachment: build libreg if building xpcom_obsolete or xpinstall

I missed this change to the toplevel Makefile when generating the previous patch.    I think it's better to have the single dependency upon mozreg_s rather than pulling on all of xpcom_obsolete in some cases and parts of mozreg_s in others.

Comment 15

[Benjamin Smedberg]

Sure, but that doesn't solve the static-linking question... mozreg_s isn't going to end up in the static link list, and since xpcomobsolete isn't bringing it in we're missing it altogether.

Perhaps we should EXPORT_LIBRARY = 1 in modules/libreg/src/Makefile.in and change xpcom/obsolete/Makefile.in to use EXTRA_DSO_LIBS instead of SHARED_LIBRARY_LIBS?

Comment 16

[Benjamin Smedberg]

Attachment: Fix export list, rev. 1

Comment 17

[cls]

Turns out that libxpinstall is dynamic in the static build so it's a non-issue.  My only concern about your patch is that, in the non-static build, you'd need to explicitly add mozreg_s in cases where people were depending upon xpcom_obsolete to pull in the bits of mozreg_s that aren't used directly by xpcom_obsolete.

Comment 18

[Benjamin Smedberg]

Attachment: Fix export list, rev. 1.1

This fixes the in-tree usage

Comment 19

[cls]

Comment on attachment 216113 [details] [diff] [review]
Fix export list, rev. 1.1

That works but I still don't think it's absolutely necessary.  Btw, none of our apps seem to build with --disable-xpcom-obsolete.

Comment 20

[Benjamin Smedberg]

I'm planning on removing xpcomobsolete from xulrunner and firefox imminently.

Comment 21

[Benjamin Smedberg]

Comment on attachment 205388 [details] [diff] [review]
v1.2

I'm going to go ahead and land this on my own authority as an xpinstall peer. so that I can remove xpcomobsolete from xulrunner.

Comment 22

[Benjamin Smedberg]

All fixed on trunk.

Comment 23

[Christian :Biesinger (don't email me, ping me on IRC)]

looks like these mozreg_s changes caused a belated bustage:
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1143619680.17111.gz

I checked in a possible fix, but I'm not sure how mozreg_s is supposed to interact with xpcom_compat.

why is the windows (clobber) tinderbox ok?

Comment 24

[Reed Loden [:reed]]

The bug 333201 folks claim this bug caused a regression in the uninstaller.

Comment 25

[Steffen Wilberg]

When searching for nsFileSpec, I still get a few hits in xpinstall, mostly in comments, two inside #ifdef XXX_SSU, and a couple in xpinstall/standalone. I've filed bug 389807 to remove the latter, but it wouldn't hurt to check the others: http://mxr.mozilla.org/seamonkey/search?string=nsFileSpec&find=xpinstall&findi=&filter=&tree=seamonkey