Last Comment Bug 532050 - Can't save as template via File > Save As Menu
: Can't save as template via File > Save As Menu
Status: RESOLVED FIXED
[has patch for review]
:
Product: Thunderbird
Classification: Client Software
Component: Toolbars and Tabs (show other bugs)
: 3.0
: x86 All
: -- normal (vote)
: Thunderbird 5.0b1
Assigned To: David :Bienvenu
:
Mentors:
: 613738 (view as bug list)
Depends on: 739782
Blocks: 654007 654966
  Show dependency treegraph
 
Reported: 2009-12-01 01:20 PST by Philipp Kewisch [:Fallen]
Modified: 2012-03-27 14:03 PDT (History)
7 users (show)
mozilla: blocking‑thunderbird3-
mozilla: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
needed
-


Attachments
proposed fix (2.50 KB, patch)
2011-04-22 14:20 PDT, David :Bienvenu
no flags Details | Diff | Splinter Review
fix initial create of templates folder (4.92 KB, patch)
2011-04-22 17:21 PDT, David :Bienvenu
no flags Details | Diff | Splinter Review
fix with unit test (14.98 KB, patch)
2011-04-25 12:06 PDT, David :Bienvenu
rkent: review+
Details | Diff | Splinter Review
Prevent multiple copies (15.87 KB, patch)
2011-04-26 11:53 PDT, Kent James (:rkent)
no flags Details | Diff | Splinter Review
proposed fix (14.69 KB, patch)
2011-04-27 16:58 PDT, David :Bienvenu
rkent: review+
Details | Diff | Splinter Review
patch for checkin (14.71 KB, patch)
2011-04-28 11:52 PDT, David :Bienvenu
mozilla: review+
Details | Diff | Splinter Review

Description Philipp Kewisch [:Fallen] 2009-12-01 01:20:26 PST
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
Comment 1 David :Bienvenu 2009-12-01 09:28:04 PST
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.
Comment 2 Joe Sabash [:JoeS1] 2009-12-01 16:22:39 PST
This fails also in WinXP (probably Mac also) Platform to all.
This is not a recent regression, it's been around for years.
Comment 3 Philipp Kewisch [:Fallen] 2009-12-03 06:49:21 PST
(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 :)
Comment 4 David :Bienvenu 2009-12-03 07:08:58 PST
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.
Comment 5 Mark Banner (:standard8) (afk until 26th July) 2009-12-16 02:16:51 PST
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.
Comment 6 Mark Banner (:standard8) (afk until 26th July) 2010-11-21 23:58:13 PST
*** Bug 613738 has been marked as a duplicate of this bug. ***
Comment 7 Mark Banner (:standard8) (afk until 26th July) 2011-02-23 07:10:59 PST
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.
Comment 8 Bryan Clark (DevTools PM) [@clarkbw] 2011-03-02 17:13:04 PST
I'm always for removing more than adding. :)
Comment 9 Wayne Mery (:wsmwk, NI for questions) 2011-03-25 16:57:04 PDT
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 :(
Comment 10 Joe Sabash [:JoeS1] 2011-03-25 17:44:36 PDT
(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.
Comment 11 David :Bienvenu 2011-04-22 14:20:29 PDT
Created attachment 527861 [details] [diff] [review]
proposed fix

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.
Comment 12 David :Bienvenu 2011-04-22 17:21:32 PDT
Created attachment 527891 [details] [diff] [review]
fix initial create of templates folder

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...
Comment 13 David :Bienvenu 2011-04-25 12:06:27 PDT
Created attachment 528142 [details] [diff] [review]
fix with unit test

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.
Comment 14 Kent James (:rkent) 2011-04-25 17:23:25 PDT
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.
Comment 15 David :Bienvenu 2011-04-25 20:00:58 PDT
I think proceeding with the patch is fine - I'll try to reproduce the crashes, though.
Comment 16 Kent James (:rkent) 2011-04-26 10:17:36 PDT
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.
Comment 17 Kent James (:rkent) 2011-04-26 11:53:11 PDT
Created attachment 528379 [details] [diff] [review]
Prevent multiple copies

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 18 Kent James (:rkent) 2011-04-26 11:53:50 PDT
Comment on attachment 528142 [details] [diff] [review]
fix with unit test

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

... with previously noted crash prevention change.
Comment 19 David :Bienvenu 2011-04-26 17:46:37 PDT
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.
Comment 20 David :Bienvenu 2011-04-27 14:12:59 PDT
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.
Comment 21 David :Bienvenu 2011-04-27 16:58:05 PDT
Created attachment 528748 [details] [diff] [review]
proposed fix

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).
Comment 22 Kent James (:rkent) 2011-04-28 10:26:04 PDT
Comment on attachment 528748 [details] [diff] [review]
proposed fix

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

Looks fine.
Comment 23 David :Bienvenu 2011-04-28 11:52:09 PDT
Created attachment 528918 [details] [diff] [review]
patch for checkin

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.
Comment 24 David :Bienvenu 2011-04-28 11:54:36 PDT
fixed on trunk - http://hg.mozilla.org/comm-central/rev/736c1952482a
Comment 25 Serge Gautherie (:sgautherie) 2011-05-04 14:29:51 PDT
(In reply to comment #24)
> fixed on trunk - http://hg.mozilla.org/comm-central/rev/736c1952482a

http://hg.mozilla.org/releases/comm-2.0/rev/b6993f6a1e0a

Note You need to log in before you can comment on or make changes to this bug.