Closed Bug 532050 Opened 15 years ago Closed 13 years ago

Can't save as template via File > Save As Menu

Categories

(Thunderbird :: Toolbars and Tabs, defect)

x86
All
defect
Not set
normal

Tracking

(blocking-thunderbird5.0 needed, blocking-thunderbird3.0 -)

RESOLVED FIXED
Thunderbird 5.0b1
Tracking Status
blocking-thunderbird5.0 --- needed
blocking-thunderbird3.0 --- -

People

(Reporter: Fallen, Assigned: Bienvenu)

References

Details

(Whiteboard: [has patch for review])

Attachments

(1 file, 5 obsolete files)

Using IMAP and Thunderbird 3.0rc1:


* Search for a message.
* Save the selected message as a tab via File > Save As > Template -> FAILS

* Open selected message in a new tab
* Try again to save the opened message via File > Save As > Template -> FAILS


Expected:

* Saving message as template works from within tabs
Flags: blocking-thunderbird3?
blocking-thunderbird3.0: --- → .1
Flags: blocking-thunderbird3? → blocking-thunderbird3-
This is unfortunate, but you can still save as template from the compose window, and drag a message from the message list to a template folder. Marking as a 3.01 blocker, but not a 3.0 blocker.
This fails also in WinXP (probably Mac also) Platform to all.
This is not a recent regression, it's been around for years.
OS: Linux → All
(In reply to comment #1)
> This is unfortunate, but you can still save as template from the compose
> window, and drag a message from the message list to a template folder. Marking
> as a 3.01 blocker, but not a 3.0 blocker.

When searching, there is no folder pane to drag the item to. Also, with the default settings, doubleclicking on the message opens it in a new tab, which then again has the same problems, so to save a message as template from the search, you have to explicitly open in a new window and then save as template. Quite confusing I'd say. Its up to you though :)
given that we got all the way to rc1 without noticing this being broken, and given that there are several work arounds, and creating templates from existing messages must be fairly rare, we're not going to block on this.
I've dug around a bit with the debugger. We are correctly calling the expected functions and we get into messenger.SaveAs. Once we get that far, we try to set up a download/save to a file which then appears that is should trigger a save to the actual folder.

However, something isn't happening (obviously) we set up nsSaveMsgListener as the listener for the save, but afaict that gets called into twice: once for onStartRunngingUrl and once for onStopRunningUrl. In the stop running case, nothing has failed (exitcode is zero), but the function also doesn't see the url as really finished (because other callbacks haven't been called) and so it doesn't do anything. As the exitcode is zero, we also don't put up a save failed dialog.

I'm thinking David probably knows more about how the urls work and may be able to see the setup error better than me. Therefore assigning to him to look at for now.
Assignee: nobody → bienvenu
Whiteboard: [needs patch]
blocking-thunderbird3.0: .1+ → needed
blocking-thunderbird3.0: needed → -
blocking-thunderbird5.0: --- → ?
Ok, I think we either need to fix this, or remove the UI. Its been around since 3.0 and I've not seen many complaints.
blocking-thunderbird5.0: ? → needed
I'm always for removing more than adding. :)
Isn't this part of a class of issues https://bugzilla.mozilla.org/buglist.cgi?bug_id=559532%2C227720%2C199995%2C11013%2C634860%2C26201%2C425455%2C284381%2C333880%2C22878%2C241213%2C324954%2C226877%2C35587%2C548070%2C270068%2C227360%2C270772%2C204350%2C463129%2C11076%2C516240%2C141364%2C387061%2C166541%2C227994%2C389650%2C612621%2C559852%2C523796%2C56908%2C326303%2C333074%2C314351%2C204612%2C258570%2C160141%2C273793%2C580017%2C219240%2C271094%2C193281%2C339595&bug_id_type=anyexact&query_format=advanced&short_desc=sav%20cop%20mov&short_desc_type=anywordssubstr

if so, this issue may be in good company.

I arrived here because I've currently got several news articles open in standalone window that I'd like copy to a folder for later reference - but "Copy to" is greyed out. 

Next I tried to fool it with "Copy Again" but got Error: An error occurred executing the cmd_moveToFolderAgain command: [Exception... "'[JavaScript Error: "gDBView is null" {file: "chrome://messenger/content/mailWindowOverlay.js" line: 1266}]' when calling method: [nsIController::doCommand]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: chrome://global/content/globalOverlay.js :: goDoCommand :: line 96"  data: yes]
Source File: chrome://global/content/globalOverlay.js
Line: 100

Last, I try Save As, and File - but File and Template is greyed out. Now I'm stuck with cut and paste :(
(In reply to comment #9)
> Isn't this part of a class of issues
> https://bugzilla.mozilla.org/buglist.cgi?bug_id=559532%2C227720%2C199995%2C11013%2C634860%2C26201%2C425455%2C284381%2C333880%2C22878%2C241213%2C324954%2C226877%2C35587%2C548070%2C270068%2C227360%2C270772%2C204350%2C463129%2C11076%2C516240%2C141364%2C387061%2C166541%2C227994%2C389650%2C612621%2C559852%2C523796%2C56908%2C326303%2C333074%2C314351%2C204612%2C258570%2C160141%2C273793%2C580017%2C219240%2C271094%2C193281%2C339595&bug_id_type=anyexact&query_format=advanced&short_desc=sav%20cop%20mov&short_desc_type=anywordssubstr

I don't think so.
This is a very specific failure of "save as template"
Templates in general, are little used, but could be important for future functionality such as "reply with template"
 
> if so, this issue may be in good company.
> 
> I arrived here because I've currently got several news articles open in
> standalone window that I'd like copy to a folder for later reference - but
> "Copy to" is greyed out. 

How many is "several" I have 6 windows open and can't reproduce here.
 

> 
> Last, I try Save As, and File - but File and Template is greyed out. Now I'm
> stuck with cut and paste :(

Save as eml is another area which I personally consider as "low value"
I'd rather save in a "web page" format.

Looks like in your situation TB just "lost track" of your opened windows.
Attached patch proposed fix (obsolete) — Splinter Review
this fixes the basic bug. Remaining work:

1. Need to create the templates folder if it doesn't exist.
2. Need unit test

I'm very tempted to make the js caller(s) of nsMessenger::SaveAs create the templates folder if it doesn't exist, even though that means both sm and tb will need that code. To test that code, I'd need to make the test a mozmill test, even though I'd really prefer an xpcshell test.
this fixes the initial creation of the templates folder, and fixes an issue with the initial copy into a just created imap folder (the latter issue affects sent/drafts as well, which is a separate, filed bug, I believe). Still need to write a unit test for this - I think I'm just going to write an xpcshell test that duplicates what the front end code does to create the folder...
Attachment #527861 - Attachment is obsolete: true
Whiteboard: [needs patch] → [has patch, needs review and test]
Attached patch fix with unit test (obsolete) — Splinter Review
Kent, I was hoping you could review this since Standard8 is swamped with reviews. There are several parts to this fix:

1. - the reported bug, save as template completely broken, because m_outputStream isn't set in the save as template case. But m_templateUri is set if and only if we're saving an existing msg as a template, so check that.

2. Some code was treating 0 as the magic value for "we don't know the UID Validity of the folder". We should use kUidUnknown everywhere, so I did that.

3. Front end code needs to make sure the templates folder exists before saving as a template. I did this in the front end because it's easier in js, and the backend code is already complicated. Suite should do something similar if they want this part of the fix.

4. Added a unit test to test all this.
Attachment #527891 - Attachment is obsolete: true
Attachment #528142 - Flags: review?(kent)
Whiteboard: [has patch, needs review and test] → [has patch for review]
I tested this patch and its unit test, and it generally works. The code also appears fine when reviewed.

But I have two issues, that may not be directly related to this patch but appear when I use and test this patch.

The first is that, if I change the templates folder in the account manager for an IMAP message, the old template folder does not lose its Template decoration in the folder pane, and cannot be deleted. So once a folder is used as a template folder it can never be deleted within Thunderbird. (POP3 does not have this behavior)

The second is a crash, that happens to me about 50% of the time so it may be timing related.

STR:
  1) Using a webmail client, deleted the IMAP Templates folder.
  2) Start Mirimar
  3) Select an existing message, and Save As Template.
  4) As soon as the new Template folder appears in the folder pane, select the folder. (It may also be necessary to select the message in the thread pane.)

  This gives me a crash. From Visual Studio we are on this line in nsMsgMailNewsUrl.cpp:

  NOTIFY_URL_LISTENERS(OnStartRunningUrl, (this));
  
  with this call stack:

 	xul.dll!nsCOMPtr<nsIUrlListener>::nsCOMPtr<nsIUrlListener>(const nsCOMPtr<nsIUrlListener> & aSmartPtr={...})  Line 568 + 0xd bytes	C++
 	xul.dll!nsMsgMailNewsUrl::SetUrlState(int aRunningUrl=1, unsigned int aExitCode=0)  Line 132 + 0x2c bytes	C++
 	xul.dll!nsImapMailFolder::SetUrlState(nsIImapProtocol * aProtocol=0x06b7f0d8, nsIMsgMailNewsUrl * aUrl=0x041c1510, int isRunning=1, unsigned int statusCode=0)  Line 6771 + 0x19 bytes	C++
 	xul.dll!NS_InvokeByIndex_P(nsISupports * that=0x066f05ac, unsigned int methodIndex=29, unsigned int paramCount=4, nsXPTCVariant * params=0x054b7600)  Line 103	C++
 	xul.dll!nsProxyObjectCallInfo::Run()  Line 182 + 0x2d bytes	C++
 	xul.dll!nsThread::ProcessNextEvent(int mayWait=1, int * result=0x002cd4d0)  Line 618 + 0x19 bytes	C++
 	xul.dll!NS_ProcessNextEvent_P(nsIThread * thread=0x005bbfa8, int mayWait=1)  Line 250 + 0x16 bytes	C++
 	xul.dll!mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate * aDelegate=0x005b9db0)  Line 134 + 0xe bytes	C++
 	xul.dll!MessageLoop::RunInternal()  Line 220	C++
 	xul.dll!MessageLoop::RunHandler()  Line 203	C++
  ...

I suspect this is due to issues with the ownership model for SaveMsgListener, which is flagged as a question in the code, though it is not obvious how unless OnStopCopy is called multiple times.

I'm curious if you can reproduce this crash, and whether you think these circumstances are rare enough to want to proceed with the patch anyway.
I think proceeding with the patch is fine - I'll try to reproduce the crashes, though.
I have investigated the crash. It is indeed crashing when trying to call a method on a pointer to a deleted nsSaveMsgListener object, which is caused by multiple calls to OnStopCopy. I will propose some minor changes to the patch that will replace the crash with a warning.

I think that we need to fix this, because you are removing the remnants of killSelf which I suspect was trying to solve this problem long ago.
Attached patch Prevent multiple copies (obsolete) — Splinter Review
Here is the patch slightly modified to prevent the crash. Initially I tried to prevent multiple deletes of the nsSaveMsgListener object, but that just got me into a loop. The root cause seems to be multiple calls to OnStopRunningUrl, which is associated with imap offline sync. I don't understand that enough to fix the root cause, but at least I think we should avoid introducing a new crash.

r=me with this additional change.
Comment on attachment 528142 [details] [diff] [review]
fix with unit test

Review of attachment 528142 [details] [diff] [review]:

... with previously noted crash prevention change.
Attachment #528142 - Flags: review?(kent) → review+
The root cause seems to be that in some situations we'll attempt to playback the offline operations multiple times. I rather suspect that can cause other problems, so I'll investigate fixing the root cause.
I suspect it's more the multiple calls to OnStopCopy and its self-release that cause the ref-counting issues. Removing the self add-ref and release fixes the crash, but there must be a cycle because we don't hit the destructor (or we had a leak before...) I'd much rather not have the self-add ref and release, so I'll investigate that approach.
Attached patch proposed fix (obsolete) — Splinter Review
this removes the ref-counting hacks, and does a variation of your fix to avoid doing multiple copies - I prefer this approach because it avoids adding a new member var and shows what the problem was - we don't want to copy the same template again...

This fix would work without removing the ref-counting hacks, but the fact that we're still passing the unit tests gives me some confidence that it's OK. I also verified that we're hitting the destructor for nsSaveMsgListener in a few scenarios (imap detach, save template, etc).
Attachment #528142 - Attachment is obsolete: true
Attachment #528379 - Attachment is obsolete: true
Attachment #528748 - Flags: review?(kent)
Comment on attachment 528748 [details] [diff] [review]
proposed fix

Review of attachment 528748 [details] [diff] [review]:

Looks fine.
Attachment #528748 - Flags: review?(kent) → review+
I discovered a bug in the local folder template case, where we weren't saving as template the very first message, when we had to create a local templates folder. I fixed that, using the same approach we use for archives.
Attachment #528748 - Attachment is obsolete: true
Attachment #528918 - Flags: review+
fixed on trunk - http://hg.mozilla.org/comm-central/rev/736c1952482a
Status: NEW → RESOLVED
Closed: 13 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.3a4
Blocks: 654007
Blocks: 654966
Depends on: 739782
You need to log in before you can comment on or make changes to this bug.