110371 - remove nsFileSpec functions that require XPCOM_STANDALONE

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P1)

Description

[Alec Flett]

As a part of removing the XPCOM_STANDALONE #define (part of embedding
requirements) I am cleaning up nsFileSpec to remove any functions that require
non-xpcom libraries.

Specifically, I need to remove:
GetFileSystemCharset(nsString&) (no consumers but nsFileSpec itself)
ConvertFromFilesystemCharset()  ("     "       "       "        "  )
GetNativePathString(nsString&)  (only used by mail)
GetLeafName(nsString&)          ( "     "  "   "  )

So, I'm adding a little helper routine to mail, and moving all the mail
consumers to start using that. Mail already has most of the infrastructure to
make use of this.

Comment 1

[Alec Flett]

Attachment: remove XPCOM_STANDALONE-based functions, and update callers

Comment 2

[Alec Flett]

Can I get a review on this stuff? Also adding nhotta..

Comment 3

[nhottanscp]

Question: nsFileSpec::ConvertFromFileSystemCharset is removed then where the
conversion to happen?

Comment 4

[Alec Flett]

nhotta: see the patch - I moved it into mail (mail is the only consumer of this API)

Comment 5

[nhottanscp]

I see, I was confused it with nsILocalFile.

Comment 6

[(not reading, please use [e-mail] instead)]

Comment on attachment 58038 [details] [diff] [review]
remove XPCOM_STANDALONE-based functions, and update callers

sr=sspitzer

Comment 7

[nhottanscp]

Comment on attachment 58038 [details] [diff] [review]
remove XPCOM_STANDALONE-based functions, and update callers

r=nhotta

Comment 8

[Conrad Carlen (not reading bugmail)]

How much more work would it be to, instead of adapting these callers of unicode
nsFileSpec stuff, just change them to use nsILocalFile?

I guess that's bug 58039, which needs to come to the front burner now.

Comment 9

[Alec Flett]

yeah, its MUCH more work to switch mail over to nsILocalFile

Comment 10

[Alec Flett]

fix checked in. thanks guys.

Comment 11

[David :Bienvenu]

this change breaks writing out the folder cache at shutdown because of the
following code:

  nsCOMPtr <nsICharsetConverterManager2> ccm2 =
do_GetService(NS_CHARSETCONVERTERMANAGER_CONTRACTID, &res);
  NS_ENSURE_SUCCESS(res, res);

but we can't get the service because we're in the middle of shutdown and it's
aleady been shutdown. It also causes a bunch of assertions.

Comment 12

[Alec Flett]

where is that code? in mail?

Comment 13

[David :Bienvenu]

nsDebug::WarnIfFalse(const char * 0x03858cc8, const char * 0x03858cb0, const
char * 0x03858c7c, int 0x00000143) line 396 + 21 bytes
ConvertToUnicode(const char * 0x03859e6c, const char * 0x0483ae20, nsString &
{...}) line 323 + 38 bytes
nsMsgGetNativePathString(const char * 0x0483ae20, nsString & {...}) line 772 +
19 bytes
nsMsgLocalMailFolder::AddDirectorySeparator(nsFileSpec & {...}) line 417 + 23 bytes
nsMsgLocalMailFolder::GetSubFolders(nsMsgLocalMailFolder * const 0x0483a6cc,
nsIEnumerator * * 0x0012fc84) line 442
nsMsgDBFolder::WriteToFolderCache(nsMsgDBFolder * const 0x0483a6cc,
nsIMsgFolderCache * 0x047d37a8, int 0x00000001) line 1051 + 36 bytes
nsMsgDBFolder::WriteToFolderCache(nsMsgDBFolder * const 0x02c782d4,
nsIMsgFolderCache * 0x047d37a8, int 0x00000001) line 1070 + 32 bytes
nsMsgIncomingServer::WriteToFolderCache(nsMsgIncomingServer * const 0x047902b0,
nsIMsgFolderCache * 0x047d37a8) line 185 + 32 bytes
nsMsgAccountManager::writeFolderCache(nsHashKey * 0x047903e8, void * 0x047902b0,
void * 0x047d37a8) line 976
_hashEnumerate(PLHashEntry * 0x04790440, int 0x00000006, void * 0x0012fda4) line
198 + 26 bytes
PL_HashTableEnumerateEntries(PLHashTable * 0x03d8a890, int (PLHashEntry *, int,
void *)* 0x10028330 _hashEnumerate(PLHashEntry *, int, void *), void *
0x0012fda4) line 429 + 15 bytes
nsHashtable::Enumerate(int (nsHashKey *, void *, void *)* 0x03726900
nsMsgAccountManager::writeFolderCache(nsHashKey *, void *, void *), void *
0x047d37a8) line 361 + 21 bytes
nsMsgAccountManager::WriteToFolderCache(nsMsgAccountManager * const 0x03d8a830,
nsIMsgFolderCache * 0x047d37a8) line 1480
nsMsgAccountManager::Shutdown() line 212

Comment 14

[David :Bienvenu]

at the risk of beating an obviously dead horse, this is just another instance of
the fact that we need a more sophisticated shutdown mechanism, where each
service gets a chance to do some (possibly asynchronous) work prior to shutdown,
and tell the shutdown manager when it's done.

Comment 15

[Alec Flett]

Could we just make nsMsgAccountManager::Shutdown happen during the
xpcom-shutdown notification?

Comment 16

[David :Bienvenu]

sorry I didn't put the whole stack trace in there, but it *is* happening during
xpcom shutdown notification.

Comment 17

[Alec Flett]

that's really wierd actually, because services should be available at that
point. I think that's even the reason we originally added the xpcom-shutdown
notification...

Comment 18

[David :Bienvenu]

sorry, my mistake. What's happening (I don't believe this is happening again!)
is that observers are removing themselves on shutdown notification, so the
enumeration of shutdown notification skips observers. I thought I fixed this.
Did I get backed out, or did I fix a similar bug?

Comment 19

[Alec Flett]

oh.. there's a recent bug on that.. if you had fixed it in the observer service,
then its possible that your fix broke when dougt froze the interface.
I can't seem to find that bug though..

Comment 20

[David :Bienvenu]

yes, that's what happened. Looks like Dougt broke my fix, which explains a lot
of the regressions in shutdown leaks I'm seeing. Doug, the problem is that
observers remove themselves on shutdown, screwing up the iteration of observers
unless you account for the possibility that the list is changing out from under
you while you iterate over observers.

Comment 21

[R.K.Aa.]

in a CVS build, linux, from around 3 hours ago, "Local Folders" vanished from
mailnews, and seems to be replaced by a large nameless envelope icon placed
inbetween newsaccounts. No twisty.

Comment 22

[R.K.Aa.]

large folder icon, that is. Any idea?

Comment 23

[Scott MacGregor]

I see exactly the same thing with my debug build from a few hours. Local folders
are gone. Did you think it was related to this checkin in particular? 

Comment 24

[R.K.Aa.]

Wrong bug. Built twice, few hours apart. First no bug - then bug. Looking at
mail-related checkins this was checked in before the folder went astray.