33451 - convert mailnews to nsIFile

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect, P3)

Description

[Warren Harris]

Mailnews should be converted to use nsIFile instead of nsFileSpec. nsFileSpec 
has been deprecated. Bug 20891 is an example bug that can be solved by making 
this change.

Comment 1

[ppandit]

changing QA contact from lchiang to ppandit

Checked source code using LXR on June 15. There was no /mailnews code that
contained nsFileSpec.

putterman - can you put this into a fixed state and I will then verify it.

Par

Comment 2

[Alec Flett]

Actually, lxr is reporting only the first 1000 hits... we're definately using
nsFileSpec (I count 44 uses just in mailnews/base)

besides, we also have to eradicate uses of nsIFileSpec

Comment 3

[scottputterman]

moving to future milestone.  My understanding is that this is nice to have but 
not critical for rtm. If there are individual occurrences that need to be 
changed  to fix other bugs we should do that.

Comment 4

[lchiang]

per scott's comments, moving to future.

Comment 5

[Jason Eager]

Adding myself to cc because I'll have to change some of my code when we change
to nsIFile.

Comment 6

[scottputterman]

reassigning to sspitzer

Comment 7

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

Attachment: convert one part of nsMsgCompose

Comment 8

[Alec Flett]

I'll review this after a mail person has done a review.

Comment 9

[Alec Flett]

while I'm really glad to see this happening, is there anyone else you can ask
for a review? I'm pretty busy in the next few weeks...

Comment 10

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 121060 [details] [diff] [review]
convert one part of nsMsgCompose


>+    nsCOMPtr<nsIURI> uri;
>+    NS_NewURI(getter_AddRefs(uri), url);

Need to get and check the rv in the above statement, otherwise you might end up
calling through a NULL pointer to QI.

>+    nsCOMPtr<nsIFileURL> fileURL(do_QueryInterface(uri));
>+    if (fileURL) {

The correct way to do this check is to use the 2 arg form of do_QI() and check
the rv.

>+      nsCOMPtr<nsIFile> file;
>+      fileURL->GetFile(getter_AddRefs(file));

Check rv, please.

>+      nsAutoString leafName;
>+      file->GetLeafName(leafName);
>+      

Check rv, please.

Comment 11

[Alec Flett]

Comment on attachment 121060 [details] [diff] [review]
convert one part of nsMsgCompose

clearing review request since dmose r-'ed it.

Comment 12

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

Attachment: Part of nsMsgCompose, v2

ok, addressed those comments; actually I rewrote that function to use nsIURI
throughout the whole function, which seemed better to me

Comment 13

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

Attachment: convert part of base/search/src/nsMsgFilterList.cpp

Comment 14

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

Attachment: -y diff of "Part of nsMsgCompose, v2"

Comment 15

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 125658 [details] [diff] [review]
Part of nsMsgCompose, v2

a) add your name to the contributor list in the license boilerplate, please

> nsresult nsMsgCompose::AttachmentPrettyName(const char* url, PRUnichar** _retval)

b) As long as you're here, please fix this method to be an NS_IMETHODIMP, not
an nsresult.

> {
>-  nsCAutoString unescapeURL(url);
>-  nsUnescape(NS_CONST_CAST(char*, unescapeURL.get()));
>-  if (unescapeURL.IsEmpty())

c) Why get rid of the unescaping logic?  Pretty names are supposed to be
readable, after all.

>+  nsCOMPtr<nsIURI> uri;
>+  nsresult rv = NS_NewURI(getter_AddRefs(uri), url);
>+  if (NS_FAILED(rv))
>   {
>-    nsAutoString unicodeUrl;
>-    unicodeUrl.AssignWithConversion(url);
>-h
>-    *_retval = ToNewUnicode(unicodeUrl);
>+    // fall back to what we were given
>+    *_retval = ToNewUnicode(NS_ConvertASCIItoUCS2(url));

d) From reading through MsgComposeCommands (where AttachmentPrettyName is
called from) I suspect that in order to get the semantics correct here, we need
to change the interface so that url can contain more than just ASCII.  Do you
agree?

If so, I believe a good new IDL decl for this method would be:

AString attachmentPrettyName(in AString aUrl);

and the string code above could be replaced with:

*_retval = ToNewUnicode(aUrl);

should do the right thing, and save copying and conversion to boot.

>     return NS_OK;
>   }
>   
>-  if (PL_strncasestr(unescapeURL.get(), "file:", 5))
>-  {
>-    nsFileURL fileUrl(url);
>-    nsFileSpec fileSpec(fileUrl);
>-    char * leafName = fileSpec.GetLeafName();
>-    if (leafName && *leafName)
>-    {
>-#ifdef MOZ_UNICODE
>-        /* file URL is now in UTF-8 */
>-        *_retval = ToNewUnicode(NS_ConvertUTF8toUCS2(leafName));
>-#else
>-        nsAutoString tempStr;
>-        nsresult rv = ConvertToUnicode(nsMsgI18NFileSystemCharset(), leafName, tempStr);
>-        if (NS_FAILED(rv))
>-          tempStr.AssignWithConversion(leafName);
>-      *_retval = ToNewUnicode(tempStr);
>-#endif /* MOZ_UNICODE */
>-      nsCRT::free(leafName);
>-      return NS_OK;
>-    }

e) Since we're getting rid of MOZ_UNICODE here, can we get rid of it in
Makefile.in also?

>+  // We want to show just the filename for file: urls
>+  nsCOMPtr<nsIFileURL> fileURL(do_QueryInterface(uri, &rv));
>+  if (NS_SUCCEEDED(rv)) {
>+    nsCOMPtr<nsIFile> file;
>+    rv = fileURL->GetFile(getter_AddRefs(file));
>+    if (NS_FAILED(rv))
>+      return rv;
>+
>+    nsAutoString leafName;
>+    rv = file->GetLeafName(leafName);
>+    if (NS_FAILED(rv))
>+      return rv;
>+

f) So for these two previous failures, why not just fall back to the string we
were passed in?

>+    *_retval = ToNewUnicode(leafName);
>+    return NS_OK;
>   }
>
>-  if (PL_strncasestr(unescapeURL.get(), "http:", 5))
>-    unescapeURL.Cut(0, 7);
>+  nsCAutoString spec;
>+  rv = uri->GetSpec(spec);
>+  if (NS_FAILED(rv))
>+    return rv;
> 
>-  *_retval = ToNewUnicode(unescapeURL);
>-  return NS_OK;
>+  PRBool isHttp;
>+  if (NS_SUCCEEDED(uri->SchemeIs("http", &isHttp)) && isHttp)
>+    *_retval = UTF8ToNewUnicode(Substring(spec, 7, spec.Length() - 7));

g) Instead of using a constant 7, |sizeof("http://")| would more clearly
express what is meant here (and would be evaluated at compile time, so no
performance penalty).

Comment 16

[Brian King [:kinger]]

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

Comment 17

[Mark Banner (:standard8)]

Please note that the address book part of mailnews is being converted in bug 132180.

Comment 18

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 125664 [details] [diff] [review]
convert part of base/search/src/nsMsgFilterList.cpp

Review request cancelled as per IRC discussion with biesi.

Comment 19

[Mark Banner (:standard8)]

Attachment: Follow up to bug 132180 (mostly checked in, see comment #21)

Now that nsFileSpec usage is almost totally removed from address book, I was
scanning through makefiles and found a couple of simple changes that we could
do to remove some more xpcom_obsolete dependencies. Hence this patch.

The mailnews/mime/cthandlers/vcard change is a follow-up to one of the patches
on bug 132180. The others (mailnews/mime/emitters and
mailnews/extensions/bayesian-spam-filter) are easy ones that I found.

Comment 20

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 198334 [details] [diff] [review]
Follow up to bug 132180 (mostly checked in, see comment #21)

r=dmose

Comment 21

[Mark Banner (:standard8)]

Comment on attachment 198334 [details] [diff] [review]
Follow up to bug 132180 (mostly checked in, see comment #21)

Most of this patch is checked in, I had to back out the changes for
EXTRA_DSO_LDOPTS (removing MOZ_XPCOM_OBOSLETE_LIBS) in
mailnews/extensions/bayesian-spam-filter/build/Makefile.in and
mailnews/mime/cthandlers/vcard/Makefile.in due to library dependencies on
Windows and Mac. This will only be able to be fixed once we've sorted out
libmsgbaseutil and any others they depend on that use xpcom_obsolete.

Comment 22

[cls]

Attachment: mime v1.0

Here's a first pass at removing nsFileSpec references from mailnews/mime.  The mimedrft.cpp changes are incomplete due to dependencies upon nsIMsgCompose but I wanted to get these out here so that people can let me know if I'm doing something obviously wrong before I get too involved.  The mailnews code uses int as a return value in a lot of places and I'm concerned about needing to cast from PRUint32 to int on 64-bit platforms.

Comment 23

[Mark Banner (:standard8)]

Comment on attachment 204851 [details] [diff] [review]
mime v1.0

Here's some of my comments on this patch from what I've done so far in other areas on this:

> The
mimedrft.cpp changes are incomplete due to dependencies upon nsIMsgCompose

I've not looked at the dependencies you mention, but if it helps, you could always temporarily use NS_FileSpecToIFile or the one that does it the other way round (which I can't find/remember at the moment) whilst in-between patches.

+nsILocalFile *
+nsMsgCreateTempLocalFile(const char *tFileName)
...
+    nsCOMPtr<nsILocalFile> tmpFile = nsnull;
+    nsresult rv = directoryService->Get(NS_OS_TEMP_DIR, 
+                                        NS_GET_IID(nsILocalFile),
+                                        getter_AddRefs(tmpFile));


You don't need the nsnull after tmpFile - just use the default nsCOMPtr constructor.

I'm also not sure this function is right, I think you'll find that tmpFile releases its ref (and hence the object) before the item that uses the pointer can addref it - hence you'll probably get some strange effects. I may be wrong, someone on #developers should be able to confirm.

+ fallback:

I think it would be a good idea to remove the goto here. Using goto is not good.

+          nsCAutoString afsStr("/afs/.");
+          nsCOMPtr<nsILocalFile> afsFile;
+          PRBool exists;
+          nsresult rv = NS_NewNativeLocalFile(afsStr, PR_TRUE, getter_AddRefs(afsFile));

You should be able to wrap the "/afs/." by a NS_LITERAL_STRING or NS_LITERAL_CSTRING and loose the nsCAutoString wrapper.

Nit:
+    nsILocalFile        *file_buffer_spec; /* temp file used when we run out 

loose the _spec here. it's not longer a nsFileSpec ;-)

> The mailnews code uses
int as a return value in a lot of places and I'm concerned about needing to
cast from PRUint32 to int on 64-bit platforms.

That's probably best covered in a separate bug.

Anyway, from what I've done before, the general idea looks good its just the com ptr that I'd be concerned about, but someone should be able confirm if I am right to be concerned or not.

Comment 24

[Mark Banner (:standard8)]

(In reply to comment #23)
> (From update of attachment 204851 [details] [diff] [review] [edit])
> +nsILocalFile *
> +nsMsgCreateTempLocalFile(const char *tFileName)
> ...
> +    nsCOMPtr<nsILocalFile> tmpFile = nsnull;
> +    nsresult rv = directoryService->Get(NS_OS_TEMP_DIR, 
> +                                        NS_GET_IID(nsILocalFile),
> +                                        getter_AddRefs(tmpFile));
> I'm also not sure this function is right, I think you'll find that tmpFile
> releases its ref (and hence the object) before the item that uses the pointer
> can addref it - hence you'll probably get some strange effects. I may be wrong,
> someone on #developers should be able to confirm.

From #developers:
NeilAway	you've got three options a) make all the callers use dont_AddRef(nsMsgCreateTempLocalFile) b) use an nsILocalFile** parameter c) use an already_AddRefed<nsILocalFile> return value (may need casting on the way out)
NeilAway	actually, for c) you would use an nsILocalFile* internally, not a COMPtr, I think

Comment 25

[Mike Cowperthwaite]

The recently added dependencies (bug 234681 and bug 332108) involve handling Unicode attachment names -- under Windows, at least, it's not currently 
possible to attach or save such files.  This may be the most visible case of 
the need for nsIFile conversion.

Comment 26

[Andrew Schultz]

Attachment: unbitrotted conversion of nsMsgFilterList (checked in)

This is biesi's attachment 125664 [details] [diff] [review] unbitrotted and with the method name changed.

Comment 27

[David :Bienvenu]

Comment on attachment 239880 [details] [diff] [review]
unbitrotted conversion of nsMsgFilterList (checked in)

thx for reviving this.

Comment 28

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

Comment on attachment 239880 [details] [diff] [review]
unbitrotted conversion of nsMsgFilterList (checked in)

Checking in nsMsgFilterList.cpp;
/cvsroot/mozilla/mailnews/base/search/src/nsMsgFilterList.cpp,v  <--  nsMsgFilterList.cpp
new revision: 1.99; previous revision: 1.98
done
Checking in nsMsgFilterList.h;
/cvsroot/mozilla/mailnews/base/search/src/nsMsgFilterList.h,v  <--  nsMsgFilterList.h
new revision: 1.30; previous revision: 1.29
done

Comment 29

[David :Bienvenu]

FYI - I'm working on a patch to get rid of lot of the uses of nsFileSpec in mailnews/base, mailnews/local, mailnews/news, and mailnews/imap. I'll attach it as soon as I get it building...

Comment 30

[David :Bienvenu]

Attachment: workin progress

This patch is starting to work, but it needs a lot more testing.

Comment 31

[David :Bienvenu]

Attachment: more work in progress

this fixes a few problems with the last patch - reading rss feeds now works, and I fixed the handling of the timestamp in popstate.dat. I also removed a few more uses of nsIFileSpec. I'm still working on handling attachments in .eml files opened. I'm not sure if I broke this, or if it's just broken on the trunk in general (on Windows). I'm doing a build w/o my patch to check.

Comment 32

[David :Bienvenu]

Attachment: patch for review

Comment 33

[David :Bienvenu]

Attachment: fix nsIMsgFolderCache

Comment 34

[Scott MacGregor]

Comment on attachment 259338 [details] [diff] [review]
patch for review

w00t

Comment 35

[David :Bienvenu]

Attachment: stop using nsFileSpecs for newsgroup posting

This is fairly self-contained, and safe.

Comment 36

[Joe Sabash [:JoeS1]]

I can't say that I fully understand the thrust of this recent checkin, but I keep
a recent trunk copy of TB and use newsgroups extensively.
What areas should I look at for effect/regressions.

Comment 37

[Mark Banner (:standard8)]

(In reply to comment #36)
> I can't say that I fully understand the thrust of this recent checkin, but I
> keep
> a recent trunk copy of TB and use newsgroups extensively.
> What areas should I look at for effect/regressions.
> 
Generally any areas of mail/newsgroups that use file. This bug will be touching most of the mail & newsgroup file handling within TB.

Comment 38

[walter sheets]

(In reply to comment #35)
> Created an attachment (id=259459) [details]
> stop using nsFileSpecs for newsgroup posting

For me, this check-in has broken the Send function for news
(but not for mail).  When Sending a news post, tb looks like
it's working normally, but no packets are ever sent to the
server, and so tb sits and waits until it gets a timeout.

Comment 39

[David :Bienvenu]

Attachment: remove a few more nsIFileSpec uses...

Comment 40

[Howard Chu]

The March 23 patch to nsPop3Protocol.cpp has a few problems. I already described one to David in IRC (net_pop3_write_state doesn't work when called from MarkMsgsForHost because the mailDirectory already had "popstate.dat" appended to it by net_pop3_load_state.) Because that bug left me with an out-of-date popstate.dat, I deleted it and restarted Seamonkey. On the next startup I got a SEGV in GetMsg, because m_pop3ConData->uidlinfo was NULL. I think this is the right fix for that problem:

RCS file: /cvsroot/mozilla/mailnews/local/src/nsPop3Protocol.cpp,v
retrieving revision 1.257
diff -u -r1.257 nsPop3Protocol.cpp
--- nsPop3Protocol.cpp  23 Mar 2007 23:35:45 -0000      1.257
+++ nsPop3Protocol.cpp  28 Mar 2007 15:57:24 -0000
@@ -249,10 +249,10 @@

   nsCOMPtr<nsIInputStream> fileStream;
   nsresult rv = NS_NewLocalFileInputStream(getter_AddRefs(fileStream), mailDirectory);
-  NS_ENSURE_SUCCESS(rv, nsnull);
+  NS_ENSURE_SUCCESS(rv, result);

   nsCOMPtr<nsILineInputStream> lineInputStream(do_QueryInterface(fileStream, &rv));
-  NS_ENSURE_SUCCESS(rv, nsnull);
+  NS_ENSURE_SUCCESS(rv, result);

   PRBool more = PR_TRUE;
   nsXPIDLCString line;

Comment 41

[David :Bienvenu]

Attachment: fix regressions in popstate.dat handling

Per Howard's previous comments...

Comment 42

[Howard Chu]

(In reply to comment #41)
> Created an attachment (id=260047) [details]
> fix regressions in popstate.dat handling
> 
> Per Howard's previous comments...
> 
It's probably time to delete your commented-out DEBUG_David_Bienvenu code in net_pop3_write_state too, since it was using the old filespec stuff.

Comment 43

[David :Bienvenu]

Attachment: work in progress - remove a bunch more

this patch seems to be working well enough on the mac. I'll have to try it on Windows. I think I'm missing some uuid changes as well...

Comment 44

[David :Bienvenu]

Attachment: patch ready for review

got this working on Windows, and changed uuids for all the interfaces I changed. Also fixed a problem in previous patch doing append to imap folders.

Comment 45

[David :Bienvenu]

Attachment: remove filespec from filter code

Comment 46

[Jungshik Shin]

Comment on attachment 261306 [details] [diff] [review]
remove filespec from filter code

Using 'Native' methods of nsILocalFile defeats one of the most
important benefits of switching to nsILocalFile, which is to support all the Unicode characters on Windows regardless of the default system codepage. However, in some cases (as shown below) where a path or a leaf name involved is always ASCII, it's a matter of performance. On Windows, using Uncode methods is faster while on Linux and Mac OS X, the opposite is the case.

In the following case, finalLeafName can contain non-ASCII characters so that
it should use |MoveTo| and |GetLeafName|.

+    nsXPIDLCString finalLeafName;
+    filterFile->GetNativeLeafName(finalLeafName);
+    ret = tmpFiltersFile->MoveToNative(parentDir, finalLeafName);

    nsXPIDLString finalLeafName;
    filterFile->GetLeafName(finalLeafName);
    ret = tmpFiltersFile->MoveTo(parentDir, finalLeafName);
    
In the following code, the 2nd parameter is always ASCII so that it's a matter of performance.


-  return localFilterFile->CopyToNative(localParentDir, NS_LITERAL_CSTRING("rulesbackup.dat"));
+  return aFilterFile->CopyToNative(localParentDir, NS_LITERAL_CSTRING("rulesbackup.dat"));  
   
   return aFilterFile->CopyTo(localParentDir, NS_LITERAL_STRING("rulesbackup.dat"));

Comment 47

[David :Bienvenu]

Attachment: remove nsFilespec from MAPI

Comment 48

[David :Bienvenu]

Attachment: remove xpcom obsolete from mailnews/extensions

getting some of the independent stuff ready to land

Comment 49

[David :Bienvenu]

Attachment: work in progress to remove all filespecs except for mailnews/import

this is the patch I'm currently testing - the basic stuff seems to be working but I need to do a lot more testing...

Comment 50

[Masatoshi Kimura [:emk]]

I've filed bug 378601 to track the comment #46 issue.

Comment 51

[David :Bienvenu]

Attachment: patch for review

Things seem to be working OK with this patch - I think I'm at the stage where testing in the wild would be helpful...

Comment 52

[Scott MacGregor]

Comment on attachment 262690 [details] [diff] [review]
patch for review

w00t!

Comment 53

[Mark Banner (:standard8)]

This seems to have caused bustage on SeaMonkey's xpfe trunk (suiterunner/toolkit is ok), looks like its some sort of include problem in nsMsgDBFolder.cpp/h - I'm not at a suiteable location to try anything at the moment.

Comment 54

[Frank Wein [:mcsmurf]]

Attachment: Bustage Fix

Dunno if this is a good patch or "the" correct patch, but it works locally :).

Comment 55

[David :Bienvenu]

Attachment: remove nsfilespec from 4.x migration code

don't know if we'll need to support to this on the trunk...but might as well have the code.

the rest of the files in mail/components/migration will require the changes to mailnews/import, so I'll do those at the same time...

Comment 56

[Scott MacGregor]

Comment on attachment 262981 [details] [diff] [review]
remove nsfilespec from 4.x migration code

I think we should think hard about ripping the 4.x migration code out of the trunk as well but we can do that in a separate bug.

Comment 57

[Karsten Düsterloh]

> I think we should think hard about ripping the 4.x migration code out of the
> trunk as well but we can do that in a separate bug.

Depends upon what "ripping out" means, we'd probably like to keep that import option for a while in SM. But this is not the right bug to discuss this, agreed.

Comment 59

[David :Bienvenu]

Attachment: remove file spec from import code

I've only done a little testing on this - we'll probably need to enlist the community for the real testing...

Comment 60

[David :Bienvenu]

I believe this is all done - marking fixed.

Comment 61

[Mark Banner (:standard8)]

(In reply to comment #60)
> I believe this is all done - marking fixed.

David, From observation (I've not tested it), I think import may be broken: http://mxr.mozilla.org/seamonkey/source/mailnews/import/resources/content/importDialog.js#531

And there's possibly a problem in the account manager (or at least maybe some code to remove):

http://mxr.mozilla.org/seamonkey/source/mailnews/base/prefs/resources/content/AccountManager.js#988

There seems to be some mac-specific filespec code left in mailnews/compose:

http://mxr.mozilla.org/seamonkey/search?string=FileSpec&find=%2Fmailnews%2Fcompose&findi=&filter=&tree=seamonkey

though I don't quite see how that's compiling on mac without xpcom_obsolete in the requires...

Are we dealing with the remaining file specs in nsMessengerMigrator in bug 379068?

Comment 62

[David :Bienvenu]

I'll look at the import dialog stuff...

The account manager code just needs to be removed...

XP_MAC code should just be removed - that's pre OSX stuff.

Yes, if we remove 4.x migration support, we can remove the #ifdeffed code in nsMessengerMigrator.

Comment 63

[David :Bienvenu]

Attachment: remove uses Mark noticed

remove some of the uses Mark noticed.

Comment 64

[Masatoshi Kimura [:emk]]

Comment on attachment 263470 [details] [diff] [review]
remove uses Mark noticed

+      file.InitWithNativePath = inPath;
InitWithNativePath is not an attribute and is not a scriptable.
http://mxr.mozilla.org/seamonkey/source/xpcom/io/nsILocalFile.idl#82
+              file = CreateNewFileFromPath( filePicker.file.path);
I think CreateNewFileFromPath is not needed at all. You don't have to convert nsILocalFile to path, then convert path to nsILocalFile :-)
What about this change?
+              file = filePicker.file;

Comment 65

[David :Bienvenu]

Thx very much, Masatoshi - you're right. I was also able to remove CreateNewFile - much nicer.

Comment 66

[Howard Chu]

Sometime in the past 2 weeks this code has broken for my POP3 accounts. Periodically I see the "Total:" messages counter in my Inbox increase by the number of messages. This seems to coincide with the time interval for automatically retrieving messages, while the actual downloads no longer occur. I'm also occasionally seeing individual messages showing up doubled in the Inbox. Also I'm seeing the Unread counter stay non-zero even when there are no unread messages. And my expiration settings on my Trash folder are no longer taking effect. That seems to coincide with a CVS update I did around May 1, as I normally the Trash messages after 1 month but at this moment I still see messages from April 1 in my Trash. Sometimes running the command Tools/Delete Mail Marked as Junk makes Seamoneky use 100% CPU and it doesn't seem to come back, I have to kill the process. Sometimes on startup it says it's rebuilding the Inbox summary file, even though it was exited cleanly before. 

It's almost totally unusable.

(This is Seamonkey/Suiterunner, by the way.)

A couple other folks on IRC mentioned seeing similar problems.

I've tried to rebuild with cvs upd -D 2007/04/30 just to see if that helps but the mailnews code no longer compiles (I guess I'd have to revert the rest of my tree to make that work, which doesn't seem like a good idea.)

Comment 67

[Christian Eyrich]

Comment on attachment 259338 [details] [diff] [review]
patch for review

I think this patch needs to be fixed for net_pop3_load_state() in nsPop3Protocol.cpp.

The comment
  /* without space to also get realnames - see bug 225332 */
in the old code was ignored and two strtoks for parsing the host&user line replaced by
  lineElems.ParseString(line.get(), " \t");
which introduces a regression to bug 225332.
If tabs are forbidden and intercepted for user names a solution would be to write and parse fields with tab as separator.

Parsing an UID line is also buggy:
If such a line only contains two fields dateReceivedStr will be NULL and TB SIGSEVs on
  if (!dateReceivedStr->IsEmpty())

Comment 68

[David :Bienvenu]

OK, thx, Christian. The fix would be to sorta revert the code a bit, and do something like this:

use lineElems.BeginWriting to get a char * back, then use NS_Strtok on that char *, instead of nsCRT::strtok, which we can't use anymore. It'll require some other minor changes since they take slightly different parameters, but I'll work up a patch - if you could try it out once I do, that would be great!

Comment 69

[David :Bienvenu]

Attachment: fix regressions in pop3protocol that Christian pointed out

Thx again for pointing this out, Christian - could you review this patch? Thx!

Comment 70

[Scott MacGregor]

Comment on attachment 269122 [details] [diff] [review]
fix regressions in pop3protocol that Christian pointed out

looks good to me.

Comment 71

[Christian Eyrich]

Comment on attachment 269122 [details] [diff] [review]
fix regressions in pop3protocol that Christian pointed out

I'm missing
  if (host == NULL || user == NULL) continue;
after the strtoks as it was in the old code. Though it's unusual having such, it's possible crashing TB with a defective popstate without that checks.

Comment 72

[David :Bienvenu]

Attachment: [checked in]address Christian's comment

carrying forward Scott's sr, requesting r from Christian.

Comment 73

[Christian Eyrich]

Comment on attachment 269379 [details] [diff] [review]
[checked in]address Christian's comment

Sorry, just noticed you changed  PL_strdup to strdup in two places. I can't see that's bad, but please have a look if that's really what you wanted.

Comment 74

[David :Bienvenu]

Comment on attachment 269379 [details] [diff] [review]
[checked in]address Christian's comment

yes, thx, that's fine. I iterated around on the fix a bit, which is why I was touching those lines. I've landed this patch.

Comment 75

[Steffen Wilberg]

When searching for nsFileSpec in mailnews, I still get 5 hits, all in comments, but those explain why the code was written in this way, so it might be a good idea to check the comments and the code:
http://mxr.mozilla.org/seamonkey/search?string=nsFileSpec&find=mail&findi=&filter=&tree=seamonkey

Comment 76

[Serge Gautherie (:sgautherie)]

(In reply to comment #75)
> When searching for nsFileSpec in mailnews, I still get 5 hits, all in comments,

I've just filed bug 459693 ;->

Comment 77

[Serge Gautherie (:sgautherie)]

Ftr, the part I'm interested in in bug 611233:
http://bonsai.mozilla.org/cvsquery.cgi?module=SeaMonkeyAll&sortby=Date&hours=2&date=explicit&mindate=2007-03-23+16%3A35&maxdate=2007-03-23+16%3A48