Closed
Bug 448550
Opened 16 years ago
Closed 16 years ago
can't move more than 2 folders to an imap folder
Categories
(MailNews Core :: Backend, defect)
MailNews Core
Backend
Tracking
(Not tracked)
VERIFIED
FIXED
Thunderbird 3.0b3
People
(Reporter: wsmwk, Assigned: Bienvenu)
References
Details
(Keywords: regression)
Attachments
(4 files, 2 obsolete files)
(deleted),
text/plain
|
Details | |
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
standard8
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
standard8
:
review+
neil
:
superreview+
|
Details | Diff | Splinter Review |
can't move more than 2 folders to another imap folder
Steps
1. create 3 folders in local or imap account
2. attempt to copy/move 3 folders to an imap folder
Results: only two folders can be copied. must restart thunderbird.
fails: version 3.0a2pre (2008071603) and iirc builds from early this year
works: TB 2.0.0.13
so trunk regression
* OK [CAPABILITY IMAP4REV1 LITERAL+ SASL-IR LOGIN-REFERRALS STARTTLS LOGINDISABL
ED] rain IMAP4rev1 2006h.380 at Mon, 28 Jul 2008 04:38:22 -0400 (EDT)
. capability
* CAPABILITY IMAP4REV1 LITERAL+ IDLE UIDPLUS NAMESPACE MAILBOX-REFERRALS BINARY
UNSELECT SCAN SORT THREAD=REFERENCES THREAD=ORDEREDSUBJECT MULTIAPPEND SASL-IR L
OGIN-REFERRALS STARTTLS LOGINDISABLED
. OK CAPABILITY completed
Comment 2•16 years ago
|
||
WFM using Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b1pre) Gecko/20080927030807 Shredder/3.0b1pre
Reporter | ||
Comment 3•16 years ago
|
||
2-3 local to imap fails, only moves first folder
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b1pre) Gecko/20080927031346 Shredder/3.0b1pre
Comment 4•16 years ago
|
||
Do they need to be any certain size?
Had a few crashes (bug 457515) testing it, but at least with small folders it worked.
Reporter | ||
Comment 5•16 years ago
|
||
in my tests folders are empty
Comment 6•16 years ago
|
||
See details in bug 450910.
Fails also with empty folder.
Comment 7•16 years ago
|
||
Ok, I see it on linux now, testing with a few empty folders
Flags: blocking-thunderbird3+
OS: Windows XP → All
Hardware: PC → All
Updated•16 years ago
|
Product: Core → MailNews Core
Assignee | ||
Comment 9•16 years ago
|
||
Are you copying the folders one at a time, or copying an empty parent folder with multiple children?
Assignee | ||
Updated•16 years ago
|
Assignee: bienvenu → nobody
Status: NEW → ASSIGNED
Component: Networking: IMAP → Backend
QA Contact: networking.imap → backend
Target Milestone: --- → Thunderbird 3.0b3
Assignee | ||
Comment 10•16 years ago
|
||
this fixes the problem, but I'm having a heck of a time getting a test case to work...
Assignee: nobody → bienvenu
Assignee | ||
Comment 11•16 years ago
|
||
We were not always notifying the copy service when the folder copy finished, in the case of copying imap folders. That left us thinking there was still a request active on the target parent folder, which makes subsequent move/copies to that folder fail.
Attachment #368191 -
Attachment is obsolete: true
Attachment #368273 -
Flags: superreview?(neil)
Attachment #368273 -
Flags: review?(bugzilla)
Comment 12•16 years ago
|
||
Comment on attachment 368273 [details] [diff] [review]
proposed fix with test case
The test case works, the code looks reasonable but it doesn't work on my imap server :-( I'll attach a log in a moment.
>+ function copyFolder1() {
>+ dump("gEmpty1 " + gEmptyLocal1.URI + "\n");
>+ let folders = new Array;
nit: I think it should be either new Array(), or even better just []
Attachment #368273 -
Flags: review?(bugzilla) → review-
Comment 13•16 years ago
|
||
Assignee | ||
Comment 14•16 years ago
|
||
I could be wrong, but that log looks like you moved/copied a folder hierarchy within the server. This patch is for when you copy across servers, or copy from a local folder account to an imap server. I don't believe the code I changed gets called at all when you move within a server...
Comment 15•16 years ago
|
||
Comment on attachment 368273 [details] [diff] [review]
proposed fix with test case
>+ nsImapMailFolder *newImapFolder = static_cast<nsImapMailFolder*>(m_newDestFolder.get());
Hmm, I think it would be clearer to make m_newDestFolder an nsRefPtr<nsImapMailFolder> and static cast on assignment.
Or do what /local/ does and add onCopyCompleted to nsIImapMailFolder.
Assignee | ||
Comment 16•16 years ago
|
||
Standard8, pinging about #14
Comment 17•16 years ago
|
||
(In reply to comment #16)
> Standard8, pinging about #14
You're right I was doing it in-server. I've just tried copying across servers - to another imap server, nothing happened; to local folders it seemed to work but I got the following error raised twice:
************************************************************
* Call to xpconnect wrapped JSObject produced this error: *
[Exception... "'[JavaScript Error: "aFolder is not defined" {file: "file:///Users/moztest/comm/main/tb/mozilla/dist/ShredderDebug.app/Contents/MacOS/modules/activity/moveCopy.js" line: 211}]' when calling method: [nsIMsgFolderListener::folderMoveCopyCompleted]" nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)" location: "<unknown>" data: yes]
************************************************************
Assignee | ||
Comment 18•16 years ago
|
||
(In reply to comment #17)
> You're right I was doing it in-server. I've just tried copying across servers -
> to another imap server, nothing happened;
I'll try that.
> to local folders it seemed to work
this bug is about copying from local folders to imap server.
> but I got the following error raised twice:
>
> ************************************************************
> * Call to xpconnect wrapped JSObject produced this error: *
> [Exception... "'[JavaScript Error: "aFolder is not defined" {file:
> "file:///Users/moztest/comm/main/tb/mozilla/dist/ShredderDebug.app/Contents/MacOS/modules/activity/moveCopy.js"
> line: 211}]' when calling method:
> [nsIMsgFolderListener::folderMoveCopyCompleted]" nsresult: "0x80570021
> (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)" location: "<unknown>" data:
> yes]
> ************************************************************
that happens without this patch as well - I'll look at it, but I suspect it's a problem in the activity manager.
Assignee | ||
Comment 19•16 years ago
|
||
this fixes the error you saw moving folders, and corrects the status message to show source and dest accounts, when different, like we do for message move/copy.
Attachment #370300 -
Flags: review?(bugzilla)
Assignee | ||
Comment 20•16 years ago
|
||
Standard8, sadly, copying imap folders between imap servers is working for me - I tried both a single folder and a two folder hierarchy - can you see if there's anything on the console, or in an imap log when it fails for you.
Assignee | ||
Comment 21•16 years ago
|
||
This patch is the same as before, except I did Neil's suggestion, which I agree does make for nicer code.
Attachment #368273 -
Attachment is obsolete: true
Attachment #370302 -
Flags: superreview?(neil)
Attachment #370302 -
Flags: review?(bugzilla)
Attachment #368273 -
Flags: superreview?(neil)
Updated•16 years ago
|
Attachment #370302 -
Flags: superreview?(neil) → superreview+
Updated•16 years ago
|
Whiteboard: [needs review/test Standard8]
Updated•16 years ago
|
Attachment #370300 -
Flags: review?(bugzilla) → review+
Updated•16 years ago
|
Attachment #370302 -
Flags: review?(bugzilla) → review+
Comment 22•16 years ago
|
||
Comment on attachment 370302 [details] [diff] [review]
fix addressing Neil's comment
ok, that's better now I know what I'm testing, though we possibly need to look at the other copies as well.
Note that moveCopy.js is already in your other patch I just reviewed.
Updated•16 years ago
|
Whiteboard: [needs review/test Standard8]
Assignee | ||
Comment 23•16 years ago
|
||
both patches checked in.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Reporter | ||
Comment 24•16 years ago
|
||
v.fixed move 2 empty local folders to imap in one action.
however, moving imap folder with mail to different imap account crashed - filed Bug 487288
note: /undo/ after moving folders leaves the moved folders in imap target. But I think it did that before this fix. (related to Bug 59688? Undo not working completely for imap move/copy message)
Status: RESOLVED → VERIFIED
Comment 25•15 years ago
|
||
I opened what sounds like a dupe of this bug (see bug 496905) because I can still reproduce this problem w/ Shredder built from a fresh checkout yesterday, 2009-06-08.
Assignee | ||
Comment 26•15 years ago
|
||
When we get to nsImapMailFolder::OnStopRunningUrl when doing an imap folder move, m_copyState is null (which it might just be for an intra-server imap folder move), and we don't send any notifications for the imapAction, which I believe is nsImapMoveFolderHierarchy - maybe it's just a matter of sending the copy completed notification for this type of url (though I think we might need a copy state to pass the right info to the copy service).
Assignee | ||
Comment 27•15 years ago
|
||
My suggestion would be to make imap intra-server folder moves call InitCopyState (see nsImapMailFolder::CopyFolder, where it calls imapService->MoveFolder), make InitCopyState handle a null messages parameter, and add a case in nsImapMailFolder::OnStopRunningUrl for the imap action nsImapMoveFolderHierarchy, which would call OnCopyCompleted.
Comment 28•15 years ago
|
||
InitCopyState uses nsImapMailCopyState - shouldn't we instead use nsImapFolderCopyState ... ?
What's the "right way" of getting the srcFolder to OnStopRunningUrl, so that I could invoke nsMsgFolderNotificationService::NotifyFolderMoveCopyCompleted?
I'm going to try using nsImapFolderCopyState and see how far it gets me ... :-)
Assignee | ||
Comment 29•15 years ago
|
||
(In reply to comment #28)
> InitCopyState uses nsImapMailCopyState - shouldn't we instead use
> nsImapFolderCopyState ... ?
>
> What's the "right way" of getting the srcFolder to OnStopRunningUrl, so that I
> could invoke nsMsgFolderNotificationService::NotifyFolderMoveCopyCompleted?
The copy state has the src folder.
The notification service is not the same as the copy service - I suspect the notification service is notified in a different place. You do want to call nsImapMailFolders' OnCopyCompleted(m_copyState->m_srcSupport, aExitCode) method...
>
> I'm going to try using nsImapFolderCopyState and see how far it gets me ...
You can try that - it's used for cross server copies right now, because cross-server copies also have to copy the messages.
Comment 30•15 years ago
|
||
OK, I gave up trying to use nsImapFolderCopyState - I think the names of the classes involved are a bit misleading - and am using nsImapMailCopyState now. Will let you know how it goes.
Comment 31•13 years ago
|
||
This bug still exists in :
Mozilla/5.0 (X11; Linux i686; rv:6.0) Gecko/20110812 Thunderbird/6.0
Build ID:20110812055110
If I copy one folder from the hierarchy to the top (INBOX), then I can't copy another folder.
There isn't any error message.
After restarting Thunderbird, i can move another folder...then restart.. move..
It isn't too easy, when you have 5-10 folder to move.
Assignee | ||
Comment 32•13 years ago
|
||
pnagy, with TB 6, you should be able to turn on copy service logging, along with IMAP logging, and get us a bit of info about what's failing.
https://wiki.mozilla.org/MailNews:Logging
The interesting log modules would be IMAP,MsgCopyService,timestamp
Comment 33•13 years ago
|
||
First I tested some move operations.
The folder moves are working if i only move folders below INBOX.
When I moving folder from a subfolder of INBOX to INBOX(ex: INBOX.some.test to INBOX.test), i can do it once at the second move i got:
2011-08-17 21:30:19.093272 UTC - -1260102960[b4c17060]: request a13cb1f0 QueueRequest - src imap://info%40xyz.tld@imap.xyz.tld/INBOX/Hosting/SOMEDIR dest imap://info%40xyz.tld@imap.xyz.tld numItems 0 type=2
Assignee | ||
Comment 34•13 years ago
|
||
Can you e-mail me the whole log? The fact that the second request was queued indicates that something happened with the first request, and I'm hoping that's in the log.
You need to log in
before you can comment on or make changes to this bug.
Description
•