81690 - Offline: Saving as Template from IMAP acct doesn't work offline.

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Backend, defect, P2)

Description

[laurel]

Using may17 commercial trunk build

Saving a message as Template from IMAP account doesn't work when offline,
regardless of whether the Template setting is for Local Folders or IMAP folder.

Note:  I was indeed able to save as template when in POP account in offline state.

Steps:
1.  Go to mail window, login to IMAP account.
2.  File|Offline|Download/Sync Now.  Choose Select button and select the IMAP
INBOX for offline use, confirm.  In Download/Sync, enable Mail Messages
checkbox, uncheck Send Unsent Messages, enable the Work Offline After Sync
checkbox.  Click OK to Download and go offline.
3.  When downloaded and offline, select/read a message which was downloaded for
offline use.
4.  File|Save as Template.
5.  Result/issue #1:  the message pane now displays "this message not downloaded
for offline use" text.  It shouldn't do this.
6.  Check the acct settings specified templates folder -- message is not there.
7.  Go online.  Check the templates folders (both on the account and local
folders) -- message not there.

Result:  message pane display changed to "not downloaded for offline use" and
message not saved as template (anywhere).

Comment 1

[David :Bienvenu]

pretty much the same as Save as Draft while offline not working, but if you want
two bugs, that's OK.

Comment 2

[laurel]

Drafts works if the setting is set to local folder, whereas templates doesn't
work in any fashion. 

Comment 3

[David :Bienvenu]

I tried saving a template to a local folder while offline and it worked fine -
the new template msg appeared in the local folder, and nothing untoward
happened. So there must be something else going on here that I'm not getting.

Comment 4

[grylchan]

Using 2001051804 build on NT 4.0

If I try saving a downloaded message, while offline, as
text,html, or eml to a folder on my hard drive or a template
to the designated template folder,
nothing happens. I get "this message not downloaded
for offline use" text after saving it. But if I go to my hard drive
folder or the template folder, the files aren't there. I tried
saving a downloaded message as a template to the template folder
under "local folders" or to a online folder and neither worked.

Another question I had: Should "Save as file/template" from the File menu
be active if you are offline and reading a message
that hasn't been downloaded? In 4.x, it's greyed out. Not sure
if we should open a new bug or 'tack it on' to this bug or Diane's
bug 81321 where she is disabling the other file menu commands when
offline.


 

Comment 5

[David :Bienvenu]

Ah, I see the confusion - I was bringing up a new compose window, typing some
stuff in, and saving it as a template.

Comment 6

[scottputterman]

To answer Gary's other question, I would think the save items would be 
disabled if the message isn't downloaded.  I'd say create another bug.

Comment 7

[scottputterman]

moving to 0.9.3

Comment 8

[David :Bienvenu]

Attachment: partial fix

Comment 9

[David :Bienvenu]

this is a partial fix for the problem - now, you won't see the "this message has
not been downloaded..." text in the message area, but the save won't work, and
will give you an error that it failed. The remaining problem is that saving as
template is done in such a hacky way that it doesn't go through the code that
uses the disk cache. I'll look at trying to fix that, but that might be beyond
the scope of the current release. I don't know if we care about checking the
partial fix in - it doesn't ultimately help the user that much, but it will be
part of the complete fix.

Comment 10

[David :Bienvenu]

requesting review

Comment 11

[scottputterman]

adding nsenterprise keyword

Comment 12

[scottputterman]

adding nsenterprise+

Comment 13

[David :Bienvenu]

I think the way this should work is to remove the hacky way we save imap
messages to disk, and use a stream listener that just reads the data and writes
it to disk. Then everything will just work.

Comment 14

[David :Bienvenu]

Attachment: patch to FileImpl to get WriteFrom working

Comment 15

[David :Bienvenu]

Doug, could you review/sr the change to nsIFileStream.cpp? - I need it to get
WriteFrom working for file streams. It can't break anything since it didn't work
before. (eventually, we will get rid of all that deprecated file stuff, but we
need to do it all at once).

Comment 16

[Doug Turner (:dougt)]

Add a check for a null closure and r=me.

Comment 17

[David :Bienvenu]

Attachment: imap diffs

Comment 18

[David :Bienvenu]

I'll add the null closure check, though we don't need WriteFrom anymore (but I'd
like it to work anyway).

The imap diffs are all so that setUrlState will get called when we're running a
url which ends up getting stuff from the disk or memory cache, just like it does
when we get stuff from the server. This should help with lots of problems, I
think. I moved SetUrlState into the nsImapMailFolderSink so I could call it
through the xpcom-proxied interface instead of the hand-rolled miscellaneous
sink, which is responsible for most of the changes. tia for reviews.

Comment 19

[Scott MacGregor]

sr=mscott

Comment 20

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

r=sspitzer

Comment 21

[David :Bienvenu]

fix checked in.

Comment 22

[grylchan]

Commercial trunk builds
2001082403 - win nt 4.0
2001082408 - linux 2.2, mac 9.0.4

Basically there are 4 tests for this test case:
 1. offline, template folder is online imap folder,
    save an existing downloaded message as template
 2. offline, template folder is a folder under Local Folders,
    save an existing downloaded message as template
 3. offline, template folder is online imap folder,
    compose a message & save as template
 4. offline, template folder is a folder under Local Folders,
    compose a message & save as template

This bug was only written based on test cases 1 and 2 but I
decided to test all 4 cases.

Test 1
   -user can save a downloaded mesg to a template folder that is a
    online imap folder
   -but there are some problems:
     -while still offline, if you go to template folder and double
      click the mesg, the body is blank 
     -go back online, a error message pops up
       * "The current command did not succeed. The mail server responded
         Message has no header/body seperator"
       * the message is not saved to template folder when you go back online
         It disappears. (4.x currently behaves same way for this)
       * And sometimes I have seen it hang the mail server by doing this:
         (can't always reproduce, may need to do few times)
           -offline, save a mesg as template
           -click template folder, click mesg
           -click inbox
           -go online (get that error message)
           -go back to template folder
            Result: Mouse arrow becomes hourglass everytime
                    I click on that folder
Test 2
  -No problems
  -user can save a downloaded mesg to a template folder that is a
    under the Local folders account
  -user can access it online/offline
  -user double clicks it (while online/offline) and it goes
   into compose message mode. Body of message is saved and editable.

Test 3 & 4 pass as expected. No problems opening or creating a
template either online or offline.

I think test case 1 is kind of having same problems as this 
disappearing message bug, bug 84249.

On Linux, when I try Test 1 and save the message as template
I get the error mesg: 'Unable to save the mesage. Please
                       check your file name and try again later.'

On Mac, when I try Test 1 and save it as a template, it works.
I can double click on it (still offline) and see message body and
it's not blank (like in windows).
But it still disappears when you go back online.


          

      

Comment 23

[Cavin Song]

I tried test case #1 and a few problems were discovered:
1. The size of the message to be copied (to Template folder) was zero so when we 
issued the append command to the server we did not pass anything along with the 
command.  The reason is that in nsImapService::OfflineAppendFromFile we relied 
on GetFileSize() call (of nsIFileSpec) to return the message size but the call 
always returned 0 even if after adding a Flush() call to flush all written data 
to file.  The only time it worked was in debugger when it stopped at the 
breakpoint right before the GetFileSize() call and the message size was then 
returned fine.  Not sure if it's a timing related issue or not.  In any case, 
the fix is to figure out the message size during the copy process because you 
know how many bytes you read and write from and to files.

2. In certain scenario, we ran into an infinite (while) loop thinking that we  
were not done reading data from a file input stream.  The while loop in 
nsImapOfflineSync::ProcessAppendMsgOperation() would never end if the message 
size was not equal to the total bytes read from the input stream for some 
reason. In other words, the physical data is more than what we calculated.  The 
fix is to test end-of-file condition and end the loop there, plus we should add 
an assertion for the case where the physical data is more than the calculated 
size.

3. Sometimes (5 out of 10 times), when "Work Online" was selected the 
synchronization process would not happen at all and it's like the event had 
never triggered.  I'm still investigating this problem.
4. When (3) happened, if you opened the imap Template folder and tried to delete 
the message you just copied during offline the operation failed with the 
following error message:

The current command did not succeed. The mail server responded: Invalid sequence 
in Uid.

So I guess we did not recover the failure very well in this case (maybe we 
should remove the message from the local Template DB if synchronization fails).

Comment 24

[Cavin Song]

Problem #1 causes "The current command did not succeed. The mail server 
responded: Message has no header/body seperator" error msg to pop up.

Problem #2 casuse the program to hang.

These two problems were seen by gchen.

Comment 25

[grylchan]

thanks for the help Cavin!

Comment 26

[Scott MacGregor]

Nice detective work Cavin. re-assigning to you now since you are digging at it.
David should be back on Tuesday if you need to tap him for help.

Comment 27

[Scott MacGregor]

this work will happen during .9.5 but is still scheduled to land on the branch.

Comment 28

[Cavin Song]

More info on problem #3 (see my note on 2001-08-29 19:04):

Looks like it happens every other time if you keep doing a sequence of "Work 
offline, save a msg in INBOX to Template (ie, Save as Template) and then back to 
online" operations without exiting the program, and it's caused by the fact that 
nsMsgDatabase::GetNextFakeOfflineMsgKey() returns an unique key every other 
time. So if you repeat it four times then the keys returned are something like: 
123, 123, 124, 124, instead of 123, 124, 125 and 126.  Copying msgs to other 
imap folders works fine because we use the uids as the msg keys and they are 
unique within a folder.  David, I may need your help to track this down and we 
need to address problem #4 as well.  I'll attach a draft patch which fixes 
problems #1 and #2 (and still contains some printf statements) in a few minutes.

Comment 29

[Cavin Song]

Attachment: draft patch for problems #1 and #2.

Comment 30

[David :Bienvenu]

I saw the hang for the first time today - maybe something else changed, perhaps
in the underlying file or file stream stuff. GetFileSize should work, or lots of
things will break. Are we doing a close after the flush?

Re reusing fake id's, that's not neccesarily a problem, and is probably in fact
the expected behaviour. If you've gone online and played back the template
append operation, we don't need that fake id anymore for the previous operation
and can reuse it for the new operation. I'll investigate more.

Comment 31

[Cavin Song]

David,

The hang problem is not very easy to reproduce as I only saw it once (out of 15
tries) last week and I still don't know what causes it to happen. I was only
able to find out where it happened in the code.

I think we never close the file stream and there seems to be a timing issue with
GetFileSize() as it worked fine if a break point was set right before the call.
Like you said maybe something has changed.

Reusing the fake id itself may not be a problem, but somwhow in
nsMailDatabase::GetOfflineOpForKey() we never call
m_mdbAllOfflineOpsTable->AddRow() if we resue the id because
m_mdbStore->GetRow() seems to find the row (which it should not).  In other
words, one part of the code thinks that id 123 should be reused but other part
of the code seems to find the row for this id anyway.  Maybe something is not
cleaned up properly (in DB)?

Comment 32

[David :Bienvenu]

perhaps closing the file stream, if we're not already doing that, would help -
I'll look into it.

The fake id is used for the msg hdr, and is only guaranteed to be unique for the
message hdr, not the offline op key. We don't need to call AddRow if there's
already a row there, but I agree that there shouldn't be one. The offline op and
its corresponding row should have been removed when the offline append was
played back.

Comment 33

[David :Bienvenu]

OK, I'm having a very different take on the hang problem - I think it's very
straightforward, and I'll attach a patch in a bit. Basically, we, I mean, I, was
using an unsigned var to see how much more data we should be reading, and then
checking if it was > 0, so bad things were happening.

Comment 34

[David :Bienvenu]

Attachment: fix for the hang, at least the one I could reproduce

Comment 35

[David :Bienvenu]

I was able to reproduce the hang by saving two very small messages as templates
offline and then going back online. The fix was to only read as much data as we
had left in the message, and also make bytes left a signed int. Cavin said it
looked ok to him, so asking Seth for an sr.

Comment 36

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

Comment on attachment 48226 [details] [diff] [review]
fix for the hang, at least the one I could reproduce

sr=sspitzer

Comment 37

[David :Bienvenu]

OK, problem 3 is because of what Cavin pointed out about us not adding the
header to the table - this is because of a mork problem (it doesn't really
really remove the row until the db is closed and opened, as near as I can tell),
but I think the fix is to simply add the row to the table. I'll try that.

Comment 38

[David :Bienvenu]

Attachment: fix for repeated save as template offline, go online, save as template...scenario

Comment 39

[David :Bienvenu]

fix was to make sure the row got added to the table in this case. seeking r/sr
from Cavin and Seth.

Comment 40

[Cavin Song]

Assigning it back to David since he's actively working on the problems.

Comment 41

[Cavin Song]

From update of attachment 48415 [details] [diff] [review], r=cavin.

Comment 42

[David :Bienvenu]

Cavin, can I get a review for the upcoming patch? Basically, we were't deleting
the temp file because the stream wasn't getting closed, and I think that was
causing problems.

Comment 43

[David :Bienvenu]

Attachment: proposed fix for temp file leak, and perhaps related problems

Comment 44

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

Comment on attachment 48979 [details] [diff] [review]
proposed fix for temp file leak, and perhaps related problems

sr=sspitzer, looks good to me.

Comment 45

[Cavin Song]

Comment on attachment 48979 [details] [diff] [review]
proposed fix for temp file leak, and perhaps related problems

r=cavin.

Comment 46

[David :Bienvenu]

most recent fix checked into trunk. This works for me now, but should be
verified by QA so we can see about checking this into the branch.

Comment 47

[grylchan]

Using commercial trunk build 
2001091203 NT 4.0
2001091208 linux 2.2, mac 9.1

for windows/mac:
 test1: 
      Template folder not selected for downloaded use
          -works fine for saving one downloaded mesg
           (can double click it and use it as a form
            in the compose window, can use while online, etc.)
          -sometimes I see weird stuff when trying to save
           more than one message :
             -if offline and save 2 downloaded mesgs as template
               (click mesg 1, save as template, click msg 2, save
                as template). Go back online and visit template
                folder: only 2nd mesg is in template folder.
             -some times prev templates don't appear in the template
              folder while offline (works fine if online) or don't
              have access to use previous saved templates as you
              get the message hasn't been downloaded which is expected.
             
       Template folder selected for downloaded use
         -no problems. works fine. All template messages are downloaded 
          and available to use online or offline.

for linux:
  File|Save As is all messed up. I'm going to have file a new bug on this.
  broken in branch and trunk builds.

  test 1: While offline, I couldn't save a downloaded message to a online
         template folder. The error message 'unable to save
         the message. Please check your file name and try
         again later' keeps popping up. 

         In fact while online I couldn't save a mesg (downloaded or non
         downloaded) to the template folder or as a 'file' of type (elm,
         html,etc.) to my unix home directory. No error message or anything.
         It just didnt work.

         The only time save as template works is for brand new composed
         message. It worked online and offline.
 Test 2: 
         offline, online:
                 trying to save a downloaded/non-downloaded mesg to
                 the template folder under 'local folders' results
                 in error mesg: 'Unable to save the messages. Please
                 check your file name and try again later'.

         no problem saving a brand new composed mesg (on/offline) to
         local template folder.

  Test 3,4 fine.

       

Comment 48

[grylchan]

Linux 'save as' bugs are bug 97863 and bug 96314

Comment 49

[Scott MacGregor]

PDT+ per PDT meeting today. Gary has signed off to do the QA on the branch.

Comment 50

[Jaime Rodriguez, Jr.]

Mscott - Ready to accept checkins.

Comment 51

[Scott MacGregor]

I landed this on the branch for David.

Comment 52

[David :Bienvenu]

I think Scott only checked in a small part of this fix. Re-opening until I can
get the right fix in.

Comment 53

[David :Bienvenu]

fix checked in.

Comment 54

[grylchan]

Commercial branch builds
 2001-09-19-05-0.9.4/ win nt 4.0
 2001-09-19-04-0.9.4/ linux 2.2
 2001-09-19-08-0.9.4/ mac 9.1

On windows and Mac 
   (test 1) Online template folder
        -you can save a message (downloaded that is)
         as a template while offline 
        -No problems saving a message(non or downloaded) while 
         online. 
        -NO problems saving a composed message on/offline as template. 
     
  (test 2)Template folder under Local Folders - no probs
  (test 3,4) ok to

I did some minor bugs in test case 1 (which I will file a new bug on)
  -when you try to save a new template in the folder where
   there is an existing template already, it will overlay one of
   the existing templates, so it looks you saved the same template
   twice while offline. Go back online, then folder displays correct
   templates and the template that got overlayed is back.

Can't test linux build as bug 97863 is blocking. Don' think this
is critical but will leave this bug as resolved and not test the trunk
until that bug is fixed.

Comment 55

[grylchan]

Using branch 2001.10.05.04-0.9.4 on linux 22.2

Verified Saving as template passed these 4 test cases

 1. offline, template folder is online imap folder,
    save an existing downloaded message as template
 2. offline, template folder is a folder under Local Folders,
    save an existing downloaded message as template
 3. offline, template folder is online imap folder,
    compose a message & save as template

adding keyword vtrunk
 4. offline, template folder is a folder under Local Folders,
    compose a message & save as template

Comment 56

[grylchan]

commercial trunk
2001-10-08-09-trunk NT 4.0
2001-10-08-08-trunk linux 2.2
2001-10-08-08-trunk mac 9.1

Verified Saving as template passed these 4 test cases

 1. offline, template folder is online imap folder,
    save an existing downloaded message as template
 2. offline, template folder is a folder under Local Folders,
    save an existing downloaded message as template
 3. offline, template folder is online imap folder,
    compose a message & save as template
 4. offline, template folder is a folder under Local Folders,
    compose a message & save as template

removing keyword vtrunk

marking verified.

Comment 57

[Dawn Endico]

removing item for this fixed bug from 0.9.6 release notes.