1106225 - Moving emails from Search Messages results to other folders creates duplicate emails and/or phantom mail, because oncommand is placed in <menupopup type="folder"> instead of in container <menu> of the menupopup, and because Bug 1110583 is not resolved yet

Status: RESOLVED FIXED

(Thunderbird :: Search, defect)

Description

[hippiefreak]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; Trident/7.0; SLCC2; .NET CLR 2.0.50727; .NET CLR 3.5.30729; .NET CLR 3.0.30729; Media Center PC 6.0; .NET4.0C; .NET4.0E; BRI/2; rv:11.0) like Gecko

Steps to reproduce:

I perform Shift-Control-F to open the Search Box. I search and find emails ok. I choose to highlight emails from the Search Results then use the Move To button at the bottom of the Search Box screen to move the highlighted emails to a desired destination folder. (The emails I highlight have been read so their Subject lines are not in bold font.)


Actual results:

The highlighted emails successfully arrive into the desired destination folder but in this destination folder there are now three unread copies of each email in addition to the original read email. For instance, If I moved Email1, Email2, and Email3, what I end up with are those same Email1, Email2, and Email3, but also I now have three unread Email1, three unread Email2, and three unread Email3, for a total of 12 emails in the destination folder.


Expected results:

Emails moved from the Search Results box should move to the desired folder without creating unread copies of themselves.

Comment 1

[:aceman]

Confirming. For me, the unread "copy" of the message is actually an empty message header with date of 1.1.1970 and no body. There is no representation of this message in the backend mbox file. If I remove the msf file of the folder, the message is gone. So it seems to be just an incorrectly created message header entry in memory.

Comment 2

[WADA:World Anti-bad-Duping Agency]

(In reply to :aceman from comment #1)
> So it seems to be just an incorrectly created message header entry in memory.

No. It's not in-memory only msgDBHdr.
If Tb is terminated and restarted, phantom mall is still shown at Thread pane. So, it's written in .msf file.
   When szeOnDisk of local folder=XXX, the phantom mail has messageKey=messageOffset=XXX, but messageSize=0.
If  a mail is copied after it, it's written at Offset=XXX, and messageKey=messageOffset=XXX is used for the copied mail. So, phantom mail disppears. 

for(ii=0;ii<=RowArray.length;ii++) like code is used instead of for(ii=0;ii<RowArray.length;ii++)?

Comment 3

[hippiefreak]

This is the OP. I forgot to say that each duplicate email has the identical Subject line, identical date stamp, and identical time stamp as the original email. The body of each duplicate email is not empty but contains the identical content as the body of the original email. Thank you.

Comment 4

[WADA:World Anti-bad-Duping Agency]

(In reply to hippiefreak from comment #3)

Please show "Order Received" column and "Size" column, and check value.
   Order Received : messageKey is shown. If local mail folder messageKey=messageOffset, If imp, messageKey=UID.
What :aceman: and I could see is phantom mail : Date=Epoc Time=no Date;, with Size=0.
In my case, Subject=null. And moved from local to local, then offset of phantom mail=sizeOnDisk(==EOF position), so phantom mail is replced by other mail when other mail is copied.

Comment 5

[WADA:World Anti-bad-Duping Agency]

Interesting phenomenon was observed with IMAP folder search and move from search.

Problem-A.
When Search at IMAP folder, and when multiple mails are moved from Search panel to local mail folder(call FolderX), move itself was  normally done, However, "move to FolderX" was imposible after it. If "Repair Folder" is requested to the FolderX, following error occurred.
>  Alert
>   !  The operation failed because another operation is using the folder.
>      Please wait for that operation to finish and then try again.
This is consistently reproducible with Gmail IMAP.
If "move single mail from IMAP search to local", problem couldn't be observed. 

Problem-B.
I also saw phenomenon of "duped mail" once while testing "Move from imap search to local mail folder".
  After some moving at  imap search panel, when I moved single mail, 3 same mails were generated at local mail folder.
This is could be observed again by;
   1. At Search of local folder, move an Unread mail to local FolderX => one mail is copied, a phantom mail is generated.
   2. At Search of imap folder, move an Unread mail to local FolderX => 3 same mails are generated.
       If multiple mails are moved from imap folder, duped mails are not observed. 
       Instead, Problem-A occurs always.

"Filter move" silently appends mail data to local mail folder if "outdated msf condition" exists.
When "move from imap to local", it's fetch body to local mail folder.
"Filter move of multiple mails from imap to local" has problem.
I suspect problem like following.
1. "Move from Search to local folder" has problem of "excess move of phantom mail".
2. If local to local, the "excess move" is masked by "phantom mail is replaced by next normal mail copy",
   but it's exposed when last move from Search.
3. If imap folder to local, each "move of a mail" is "fetch body to local FolderX" + "Store \Deleted".
    So, "excess move of phantom mail" may fetch last mail multiple times in some cases,
    and may produce "lock of FolderX" in some cases.
"dup mails" may be :
     Outdated msf condition is generated in FolderX ny problem
    -> mail data is silently appended to FolderX -> These mails are show at FolderX upon folder open by Rebuild-Index.
However, Unread count(and Total count/Size by Extra folder column) of move target folder was updated as expected.
So, I think "outdated msf condition" is not relevant.

"Move from search of IMAP folder to local folder" seems IPS cell of "IMAP filter move relevant problems" :-)

Comment 6

[WADA:World Anti-bad-Duping Agency]

"Move from Search" consists of :
       "Message Filter/Run selected filter(s) on a folder with single rue is selected"  (filter rule/action is temporary)
   + "Run Filter on Selected Messages" (target of filter is selected messages in gDBView, instead of folder)
So, it's one of most complicated feature :-)

Comment 7

[WADA:World Anti-bad-Duping Agency]

Problem-C.
When I tried "Search at IMAP mbox, Move to IMAP folder". I saw funny phenomenon.
    Search result is 4 mails, Selection at Search is one mail only, request move to imap folder
    -> even though one message is selected, 3 mail were moved.
This was reproduced by repeated "Search at FolderA, Move 1 mail to FolderB(3 moved), move last mail to FolderB" and "Search at FolderB, Move 1 mail to FolderA(3 moved), move last mail to FolderA".
However, after restart of Tb, I couldn't observe such phenomenon with other test folder set and other test mail set.
Phenomenon is perhaps due to "different subject, same message-id". If so, it's perhaaps similar phenoenon to one was observed in MailDir test with crafted mail.

Comment 8

[WADA:World Anti-bad-Duping Agency]

Attachment: NSPR log for move 1 mail(local to local), move 1 mail(imap to local), move 3 mails(imap to local), then local folder is locked.

If "move from local folder to local folder", log by MsgCopyService only, so it's hard to know what happens, although "3  CopyMessages requests for 1 mail move request" is known by MsgCopyService log.
If IMAP, "copy from imap to local" is converted to "uid fetch aa:bb body.peek[]" and "delete" is converted to "uid store aa:bb +Flags(\Deleted)", phenomenon is seen by IMAP log.
So, when "move of UID=1,2,3 to local folder", 3 times of CopyMessages requests, 3 times of "uid fetch 1,2,3 body[]", 3 times of "uid store 1,2,3 +Flags(\Deleted)" are seen in log.

Comment 9

[hippiefreak]

This is the OP. FYI: I opened tkt 1089452 last October for a separate Search Box issue but no one has looked at that tkt yet. Maybe I should apprise you of it, in case you decide it is relevant. Thank you.

Comment 10

[WADA:World Anti-bad-Duping Agency]

Attachment: NSPR log for move 1 mail(IMAP to Other IMAP, 3 dup is copied), move 1 mail(Local to IMAP, 2 dup copied).

If Move from IMAP(Gmail IMAP) to different IMAP(Yahoo! IMAP), "3 CopyMessages requests for single move, 3 times of append for single move" is seen by log.
If Move from Local to IMAP(Yahoo! IMAP), "3 CopyMessages requests for single move, 2 times of append for single move" is seen by log.

Difference in "number of append" is perhaps:
   Setting of Gmail IMAP : Delete model = Just mark it as deleted. So, msgDBHdr is not deleted by "store flag \Deleted".
   If move from IMAP, msgDBHdr of moved(deleted) msgDBHdr can be accessed even after delete.
   If move from Local, msgDBHdr of moved(deleted) msgDBHdr is deleted after 2nd CopyMessage(append), so 3rd CopyMessage fails.
Phantom mail in "Move from Local to Local" is perhaps :
   msgDBHdr of moved(deleted) mail is deleted after 2nd CopyMessage(append), so 3rd CopyMessage generates phantom mail.
   1st CopyMessage and 2nd CopyMessage writes same data at same location of msgStore file, so same messageKey==messgeOffset is used.

Comment 11

[WADA:World Anti-bad-Duping Agency]

Move button is processed by MoveMessageInSearch(destFolder).
http://mxr.mozilla.org/comm-central/source/mail/base/content/SearchDialog.js#564
MoveMessageInSearch calls  gFolderDisplay.doCommandWithFolder.
http://mxr.mozilla.org/comm-central/source/mailnews/base/src/nsMsgSearchDBView.cpp#767
gFolderDisplay.doCommandWithFolder calls nsMsgDBView::DoCommandWithFolder
http://mxr.mozilla.org/comm-central/source/mailnews/base/src/nsMsgDBView.cpp#2462
  2474   switch (command) {
  2475     case nsMsgViewCommandType::copyMessages:
  2476     case nsMsgViewCommandType::moveMessages:
  2477         NoteStartChange(nsMsgViewNotificationCode::none, 0, 0);
  2478         rv = ApplyCommandToIndicesWithFolder(command, indices, numIndices, destFolder);
  2479         NoteEndChange(nsMsgViewNotificationCode::none, 0, 0);
  2480         break;

If number of selected messages are N, when "move from IMAP to other IMAP",  "3 CopyMessage requests" and 3 times of "append N mails to other IMAP server" is always seen in IMAP log.
NoteStartChange() and NoteEndChange looks to invoke same action as ApplyCommandToIndicesWithFolder(command, indices, numIndices, destFolder).
NoteStartChange() does do nothing. NoteEndChange() invokes 2nd/3rd action which is same as previous ApplyCommandToIndicesWithFolder()?

Code of NoteChange(), NoteStartChange(), and NoteEndChange()
  http://mxr.mozilla.org/comm-central/source/mailnews/base/src/nsMsgDBView.cpp#6128
Wrong use of NoteStartChange() and NoteEndChange()?
Or wrong call of ApplyCommandToIndicesWithFolder.() when NoteStartChange() and NoteEndChange is used?
NoteStartChange() does do nothing. Bug in NoteStartChange()?
Or bug in NoteChange()?
What is spec of NoteStartChange(), NoteEndChange() and Change()?

Comment 12

[WADA:World Anti-bad-Duping Agency]

http://mxr.mozilla.org/comm-central/source/mailnews/base/src/nsMsgDBView.cpp#2805
     nsMsgDBView::ApplyCommandToIndicesWithFolder calls  CopyMessages. this is not nsMsgCopyService::CopyMessages
http://mxr.mozilla.org/comm-central/source/mailnews/base/src/nsMsgDBView.cpp#2780
    nsMsgDBView::CopyMessages does
         mIndicesToNoteChange.AppendElements(indices, numIndices);
         then calls  nsMsgCopyService::CopyMessages.
         http://mxr.mozilla.org/comm-central/source/mailnews/base/src/nsMsgCopyService.cpp#430
             PR_LOG(gCopyServiceLog, PR_LOG_DEBUG, ("CopyMessages")); is done in nsMsgCopyService::CopyMessages.

In nsMsgDBView, "removing of row of view after move mail" is requested by nsMsgDBView::CopyMessages.
I beleve "removing of row of view after move mail" shouldn't be requested in both nsMsgDBView::ApplyCommandToIndicesWithFolder and nsMsgDBView::CopyMessages which is called nsMsgDBView::ApplyCommandToIndicesWithFolder.
I don't know this is cause of "3 CopyMessage log by MsgCopyService:5 for each move request at Search panel" or not.
Cause may be "3 times of nsMsgDBView::ApplyCommandToIndicesWithFolder call".
But at least double "removing of row of view after move mail by nsMsgDBView::ApplyCommandToIndicesWithFolder and "nsMsgDBView::CopyMessages should be removed.

Confusion on CopyMessages() function due to pretty similar full name with same function name of  nsMsgDBView::CopyMessages and nsMsgCopyService::CopyMessages?
I wasn't aware of CopyMessages() of nsMsgDBView, because it's not exposed to JavaScript code via nsIMsgDBView.idl.
http://mxr.mozilla.org/comm-central/source/mailnews/base/public/nsIMsgDBView.idl

Comment 13

[WADA:World Anti-bad-Duping Agency]

In "Move To Again" menu of Context menu at Thread Pane, <command id="cmd_moveToFolderAgain"> is used.
http://mxr.mozilla.org/comm-central/source/mail/base/content/messageWindow.js#1104
   Command controler of it calls  MsgMoveMessage(folder).
http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.js#1525
    gDBView.doCommandWithFolder() iwas called too at Thread Pane of  3pame window, aalthough JavaScript is used. 
    1525 function MsgMoveMessage(aDestFolder)
    1526 {
    1527   gFolderDisplay.hintAboutToDeleteMessages();
    1528   gDBView.doCommandWithFolder(nsMsgViewCommandType.moveMessages, aDestFolder);
    1529   Services.prefs.setCharPref("mail.last_msg_movecopy_target_uri", aDestFolder.URI);
    1530   Services.prefs.setBoolPref("mail.last_msg_movecopy_was_move", true);
    1531 }

There are  3 doCommandWithFolder().

http://mxr.mozilla.org/comm-central/source/mail/base/content/folderDisplay.js#1792
    doCommandWithFolder: function FolderDisplayWidget_doCommandWithFolder(
       aCommandName, aFolder) {
      return this.view.dbView &&
      this.view.dbView.doCommandWithFolder(aCommandName, aFolder);
   },
http://mxr.mozilla.org/comm-central/source/mailnews/base/src/nsMsgSearchDBView.cpp#767
   nsMsgSearchDBView::DoCommandWithFolder(nsMsgViewCommandTypeValue command, nsIMsgFolder *destFolder)
   mCommand = command;
   mDestFolder = destFolder;
   return nsMsgDBView::DoCommandWithFolder(command, destFolder);
   }
http://mxr.mozilla.org/comm-central/source/mailnews/base/src/nsMsgDBView.cpp#2462
    nsMsgDBView::DoCommandWithFolder(nsMsgViewCommandTypeValue command, nsIMsgFolder *destFolder)
      switch (command) {
      case nsMsgViewCommandType::copyMessages:
      case nsMsgViewCommandType::moveMessages:
      NoteStartChange(nsMsgViewNotificationCode::none, 0, 0);
      rv = ApplyCommandToIndicesWithFolder(command, indices, numIndices, destFolder);
      NoteEndChange(nsMsgViewNotificationCode::none, 0, 0);
      break;

Finally, nsMsgDBView::DoCommandWithFolder is called. So it's ot fault in nsMsgDBView::DoCommandWithFolder. If fault in it, problem should occur by Context menu of 3pane window.

nsMsgSearchDBView Object.
   http://mxr.mozilla.org/comm-central/source/mailnews/base/src/nsMsgSearchDBView.h#16
nsMsgDBView Object.
  http://mxr.mozilla.org/comm-central/source/mailnews/base/src/nsMsgDBView.h#98
Some onXXX handler re defined. It looks that event handler is killed by "virtual" in another Object if conflicts.
However, some events may be processed by both. If so, "problem only when nsMsgSearchDBView is used" may occur.

Comment 14

[WADA:World Anti-bad-Duping Agency]

Attachment: NSPR log for move 3 mails(Gmail IMAP) to Yahoo!_IMAP/Inbox/AAA/F1/F2/F3/F4/F5 -> 24 append of mail(3 mails * depth of 8)

(In reply to WADA from comment #12)
> Cause may be "3 times of nsMsgDBView::ApplyCommandToIndicesWithFolder call".

I 'm inclined to it after code viewing. I couldn't find fault in used modules, objects.
I suspected multiple "Move To Target Folder" calls upon button/menu click.
So, I moved 3 mails from Gmail IMAP to Yahoo!_IMAP/Inbox/AAA/F1/F2/F3/F4/F5.
Bingo!
8 CopyMessage were requested, and append was requested "3(mails) * 8(depth of target)" times,  then "3(mails) * 8(depth of target)" mails were generated in move target folder.
Phenomenon is perhaps UI issue:
  At expanded sub menu for move target folder under Move button, oncommand event(or click event) fires at each folder hierarchy level.

Comment 15

[WADA:World Anti-bad-Duping Agency]

Fault is in UI definition of Search Window.
However, funny phenomenon, such as folder lock, phantom mail, is issue in DoCopy when multiple "Move of multiple mails to same move target folder" are requested at same time, or when "Delete at source folder while trying to do move to FolderB" occurred due to "end of move from same source folder to other FolderA". It is surely rare, but it can occur if "Message Filter move", "Junk filter move", "Manual Filter Run" etc. happened at same time.

Comment 16

[[e-mail]]

A <menupopup type="folder"> clones itself otherwise its subfolders don't work properly on OSX. However this causes a duplication of attributes which is not wanted in the case of the oncommand attribute. There are three cases:
1. The oncommand attribute is on the parent element, so no duplication occurs
2. The oncommand action just updates a variable without doing anything twice
3. The oncommand action tries perform an action multiple times
I don't know whether it would be a good idea to blocklist certain attributes.
In this case the oncommand attribute would probably work on the parent element (which is where it was in SeaMonkey before bug 1001071 moved it, oops!) but I don't know for sure whether this would be appropriate in all cases.

Comment 17

[WADA:World Anti-bad-Duping Agency]

(In reply to WADA from comment #5)
> Alert
>  !  The operation failed because another operation is using the folder.
>     Please wait for that operation to finish and then try again.

I've oened Bug 1110577 for this issue.

Comment 18

[WADA:World Anti-bad-Duping Agency]

(In reply to WADA from comment #2)
>  When szeOnDisk of local folder=XXX, the phantom mail has messageKey=messageOffset=XXX, but messageSize=0.

I've oened Bug 1110583 for this issue.

Comment 19

[WADA:World Anti-bad-Duping Agency]

(In reply to [e-mail] from comment #16)

Copy of your bug 1001071 comment 14.
> This caused bug 1106225 for SeaMonkey because you moved the oncommand from the button to the menupopup 
> where it gets duplicated throughout the folder menu and fires multiple times.

As you say, oncommand was placed in menupopup of Search Dialog of Thunderbird too.
Thanks for patch by bug 1001071. Without this bug and that bug, I couldn't find bug 1001071 and bug 1110577 :-)
I now know how to invoke multiple "move mails from FolderA to FoldeB" requests on a folder at same time, and how to force "move or delete mails from FolderX" while other task is moving mails from FolderX. This is perhaps phenomenon what happend when Filter Move, Junk Move, Message Purge, Junk Purge, occurred at same time.

"Move To" of main menu/app menu/context menu => No problem
> http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.xul#2792
>      <menu id="moveMenu"
>            label="&moveMsgToMenu.label;"
>            accesskey="&moveMsgToMenu.accesskey;"
>            oncommand="MsgMoveMessage(event.target._folder)">
>        <menupopup type="folder" mode="filing"
>                   showFileHereLabel="true"
>                   showRecent="true"
>                   recentLabel="&moveCopyMsgRecentMenu.label;"
>                   recentAccessKey="&moveCopyMsgRecentMenu.accesskey;"/>
>      </menu>

"Move To" of Search dialog => This bug occurs.
> http://mxr.mozilla.org/comm-central/source/mail/base/content/SearchDialog.xul#183
> <button label="&openButton.label;" id="openButton" command="cmd_open" accesskey="&openButton.accesskey;"/>
> <button id="fileMessageButton" type="menu" label="&moveButton.label;"
>         accesskey="&moveButton.accesskey;"
>         observes="file_message_button">
>   <menupopup type="folder" showFileHereLabel="true" mode="filing"
>              oncommand="MoveMessageInSearch(event.target)"/>
> </button>

Comment 20

[WADA:World Anti-bad-Duping Agency]

http://hg.mozilla.org/comm-central/annotate/46e2006812f2/mail/base/content/SearchDialog.xul#l186
http://hg.mozilla.org/comm-central/diff/62ad06c7ba12/mail/base/content/SearchDialog.xul
http://hg.mozilla.org/comm-central/rev/62ad06c7ba12
Change on relevant lines looks done by Bug 878805. This looks ported to SeaMokey by Bug 1001071 :-)

Comment 21

[WADA:World Anti-bad-Duping Agency]

Minimum required change for this bug
  http://mxr.mozilla.org/comm-central/source/mail/base/content/SearchDialog.xul#183
               183  <button id="fileMessageButton" type="menu" label="&moveButton.label;"
  Add                       oncommand="MoveMessageInSearch(event.target) 
               184          accesskey="&moveButton.accesskey;"
               185          observes="file_message_button">
  Change 186    <menupopup type="folder" showFileHereLabel="true" mode="filing"   />
  Delete   187    ...
               188  </button>

However, as known by "Move To" of main menu/app menu/context menu, event.target._folder can be used in oncommand.
If event.target._folder is used, absolutely same code as MsgMoveMessage(aDestFolder) of mailWindowOverlay.js can be used as MoveMessageInSearch of SearchDialog.js.
Further, if same code can be used commonly, commn MoveSelectedMessagesInView(Selection,destFolder) like implementtion is possible. This can be a function in mail related xxx.jsm. If such common function will be implemented, there is no need to maintain same or similar code of same or different function name in multiple xxx.js files with diferent file name for each window type, and if singleton by jsm is utilized, there is no need to load/hold copy of code in each XUL/DOM Window.
See following for exmple of jsm named "utils.jsm".
http://mxr.mozilla.org/comm-central/search?string=import&find=utils.jsm&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central

Is there need to keep Search Dialog only MoveMessageInSearch(), even though MsgMoveMessage(aDestFolder) of mailWindowOverlay.js is available?

Comment 22

[[e-mail]]

(In reply to WADA from comment #21)
> Is there need to keep Search Dialog only MoveMessageInSearch(), even though
> MsgMoveMessage(aDestFolder) of mailWindowOverlay.js is available?

(I accidentally looked at old copies of these two functions and they were identical excepting whitespace changes!)

Comment 23

[WADA:World Anti-bad-Duping Agency]

Problem was easily reprduced by menu in messenger + Virtual Folder.
(1) Define following menu in messenger window.
(2) Create a Virtual Folder(Search Folder), At the Virtual Folder, Do move via the menu
      => Problem occurs. If N messages x D depth, "moved N*D" is shown at Activity Manager. 
(3) At real folder, Do move via the menu
      => Event is invoked D times, but problem can't be reproduced.
            If N messages x D depth, "moved N" is always shown at Activity Manager.
What is difference between Real Folder and Virtual Folder?

>  <menu>
>   <menupopup>
>    <menu>
>      <menupopup type="folder" showFileHereLabel="true" mode="filing"
>                oncommand="Button_3_MoveMessageInSearch(event);"/>
>       </menu>
>  function Button_3_MoveMessageInSearch(,event)
>  {
>     const nsMsgViewCommandType = Components.interfaces.nsMsgViewCommandType;
>     var MoveMessageInSearch = function MoveMessageInSearch(destMsgFolder)
>         {
>            gFolderDisplay.hintAboutToDeleteMessages();
>            gFolderDisplay.doCommandWithFolder(nsMsgViewCommandType.moveMessages,destMsgFolder);
>         }
>     if( event.target._folder && gFolderDisplay.selectedCount )
>     {
>        MoveMessageInSearch(event.target._folder);
>     }
>  }

Comment 24

[WADA:World Anti-bad-Duping Agency]

(In reply to WADA from comment #23)
> What is difference between Real Folder and Virtual Folder?

When Read/Unread istatus of mails is changed at Search Folder, Unread count of search target folder(real  folder) is automatically updated at folder pane, but unread count of the Search folder is not updated at folder pane, even though Read/Unread status change ws done at the Search Folder. This kind of difference may be relevant to problem like "phantom mail" or "duped mails".

Comment 25

[ISHIKAWA, Chiaki]

(In reply to WADA from comment #24)
> (In reply to WADA from comment #23)
> > What is difference between Real Folder and Virtual Folder?
> 
> When Read/Unread istatus of mails is changed at Search Folder, Unread count
> of search target folder(real  folder) is automatically updated at folder
> pane, but unread count of the Search folder is not updated at folder pane,
> even though Read/Unread status change ws done at the Search Folder. This
> kind of difference may be relevant to problem like "phantom mail" or "duped
> mails".

I observe two things.

a) It seems that we need some type of folder lock on the original is in order when a move/copy operation is in progress to move/copy messages to the other folder.

b) The phantom mail issues rings a bell.
The internal db/cache for this type of status (read/unread) may not have proper sync behavior all the time. I noticed that status of wiped out folder (deleted [move to Trash], then empty Trash]
is somehow remembered by TB and assigned to freshly created folder with the same name
until it seems cleared when the first fetch of the non-existing phantom message occurs.
This is Bug 153720

Comment 26

[WADA:World Anti-bad-Duping Agency]

Attachment: Tb 45: event log of phenomenon that many phantom mails are generated by Search+Move at local mail folder

Thunderbird 45.4.0 already does do following any time when local mail folder.
  starts messageKey from 1, and increments messageKey by 1 always.
So, in Tb 45, phantom mail is never masked by next phantom mail or next mail.
Therefore, many phantom mails can be pretty easily observed in Thunderbird 45.4.0 by utilizing Bug 1106225(this bug) :
  Do search mails at local mail folder, then move messages

Comment 27

[WADA:World Anti-bad-Duping Agency]

FYI.

Phantom mails by "move mails in local folder to other local folder" is consistently created by :
(a) Menu/button definition with folder selection popup like "Move To of Search Panel".
    This is different from menu/button definition of "Move To of messenger".
(b) "Move To" is requested on mails in Virtual folder.
To see phantom mails consistently, both (a) and (b) is required.

(1) Module defined in oncommand of menu/button for "Move To" is invioked multiple times by (a).
(2) Even if (1) occurs due to (a), if "Move To via. (a)"  is requested on real local folder,
    CopyMove is not actually invoked or executed multiple times.
(4) When "Move To" is invoked multiple times due to (a),
    if it's requested on Virtual Folder, CopyMove is requested multiple times at almost same time.
    And, excess execution of CopyMove produces phantom mails,
    because the excess CopyMove generates msgDBHdr even after CopyMove source mail is already deleted by first CopyMove.
(4) In Search Panel(Advanced Search), search result is held in a Virtual Folder internally.
    So, (a) and (b) is always satisfied if Search Panel.

In Thunderbird 45, phenomenon was changed from "duplicate copies at move target folder + 1 phantom mail" to "single copy at move target folder + many phantom mails in move target folder", because messageKey is never Offset in Thunderbiurd 45.
Please see attached event log.
In excess CopyMove, "delete of messageKey=X,messageOffset=X,messageSize=0" is seen at move source folder.
messageKey=X is messageKey of moved mail.
This indicates difference between before Tb 45 and after Tb 45.
- Before Tb 45 :
  messageKey=Offset/messageOffset=Offset of already deleted mail.
  Mail data is still held at Offset in msgStore because of BerkleyStore.
  So, "From -" line, X-Mozilla-Status: etc. can be normally read.
- In Tb 45 :
  "copy using messageKey=X/messageOffset=X" silently fails
  because there is no valid data at Offset=X of msgStore,
  or because messageKey=X doesn't exist any more in move source folder.

Some problems are involved in above symptom.

If (a) is removed in Thunderbird too like done by Bug 1001071 in SeaMonkey, "consistent phantom mails generated by Move mails at Search Panel" won't occur, even if other problems like following still remains.
  problem in in Virtual Folder, CopyMove status/return code check, CopyMove serialization,
  folder/msgDB locking/serialization, msgDBHdr creation serialization, and so on.
Problem in removing (a) : We loose method to generate phantom mails consistently.

Comment 28

[ISHIKAWA, Chiaki]

(In reply to ISHIKAWA, Chiaki from comment #25)
...

> I observe two things.
> 
> a) It seems that we need some type of folder lock on the original is in
> order when a move/copy operation is in progress to move/copy messages to the
> other folder.
> 
> b) The phantom mail issues rings a bell.
> The internal db/cache for this type of status (read/unread) may not have
> proper sync behavior all the time. I noticed that status of wiped out folder
> (deleted [move to Trash], then empty Trash]
> is somehow remembered by TB and assigned to freshly created folder with the
> same name
> until it seems cleared when the first fetch of the non-existing phantom
> message occurs.
> This is Bug 153720

I meant to say it is BUG 1153720

Comment 29

[:aceman]

Attachment: patch

The specific case of multiple copies of message from the Search dialog is now fixed in bug 1319299.

But the general problem still exists. The oncommand attribute is copied into all menupopups in the folder picker generated dynamically. Once an item (folder) is clicked the oncommand of all the menupopups on the path to the top level parent are executed. The commands are all the same. It just isn't visible in most folder pickers because they happen to be case 1. and 2. from Neil's comment 16.

This patch tries to implement Neil's comment 16. I have chosen the blacklist approach for now. All attributes are copied by the clone, but I remove the *command ones. If we wanted the whitelist approach, we would want to preserve only the attributes that are now visible in the patch to remove.

This also reverts bug 1319299 and bug 1133212 and their problem should not re-appear with this patch. It seems clicking a menuitem in the hierarchy of menupopups still executes the top level parent menupopup (event bubbles up running all commands found on the way, so if it only finds one, we won).

I've chosen squib for review as he also reviewed bug 878805 which consolidated the oncommand attributes onto menupopups. I like consolidations and having attributes always on the same object (e.g. for future bug 473009). So let's try to preserve the unity and fix the underlying issue.

This patch may be risky (Neil is also not sure whether we need the blacklist or whitelist) so is for trunk only, not for TB 52.

Comment 30

[:aceman]

Comment on attachment 8813366 [details] [diff] [review]
patch

Alta, this tries to address Neil's bug 878805 comment 35. Can you check it too?

For all, try run at https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=b988dc0e00373b70dafff84f3e37bb52b457a59a

Comment 31

[Jorg K (CEST = GMT+2)]

Comment on attachment 8813366 [details] [diff] [review]
patch

Review of attachment 8813366 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/base/content/folderWidgets.xml
@@ +523,5 @@
> +              // Cloning "command" attribute would cause execution of the same command
> +              // for each level of menupopup that is between the top parent
> +              // and the clicked item.
> +              popup.removeAttribute("oncommand");
> +              popup.removeAttribute("command");

Do you need to remove this or simply not set it?

Comment 32

[:aceman]

(In reply to Jorg K (GMT+1) from comment #31)
> > +              popup.removeAttribute("oncommand");
> > +              popup.removeAttribute("command");
> 
> Do you need to remove this or simply not set it?

According to 
https://developer.mozilla.org/en-US/docs/Web/API/Node/cloneNode clone copies all attributes. So I have to REMOVE them. Yes, the original code SET all the attributes we were interested in (including command), but that seems like a duplication (setting what is already set).

But yes, let's test this on all platforms for style correctness. In the past there were bugs where the classes we not set on the parent or not copied uniformly to the child popups and it was seen in the appearance. So if this patch is wrong and does not clone "class" it should be visible. Paenglab, can you check the folder pickers with this patch on Windows/Mac ?
On the other hand, if the folder picker appears to work fine with submenus, it should be a proof the "type=folder" is being cloned.

Comment 33

[:aceman]

OK, I also propose to add removal of "id" just in case somebody manages to set one on the top level menupopup. Speaking of that, there is at least one at https://dxr.mozilla.org/comm-central/rev/6298ffcae715b3b37a885ddc74eaf01bd7240293/mail/base/content/mailWindowOverlay.xul#2340 . That case is lucky, because in that "mode" it does not open sub-menupopups so no duplicate ids should appear.

Comment 34

[:aceman]

But https://dxr.mozilla.org/comm-central/rev/6298ffcae715b3b37a885ddc74eaf01bd7240293/mail/components/compose/content/messengercompose.xul#687 duplicates IDs (while the picker is open), we are just lucky we never look for that ID.

Comment 35

[WADA:World Anti-bad-Duping Agency]

(In reply to :aceman from comment #29)
> But the general problem still exists. The oncommand attribute is copied into
> all menupopups in the folder picker generated dynamically. 
> Once an item (folder) is clicked the oncommand of all the menupopups on the path to the
> top level parent are executed. The commands are all the same. 

Have you imagined following usage?
  <button oncommand="DoActionOnTheSelectedFolder();">
  <menupopup type="folder" mode="filing"
     oncommand="PrepareAtParentDirectoriesForActionOnSelectedFolder()";>

When action on selected folder is "rename folder", "delete folder", "move folder" like one,
future event/job at parent directory/parent folder will surely occur, and some actions will be surely needed.
In such case, preparation for "rename/delete/move folder etc." is possible.
This is similar to relation between onBeforerUnload event and onUnload event and so on.

I think design/implementation of type="folder" is to fulfil above usage.

In SearchDialog.xul / MoveMessageInSearch, what had happened is:
(1) Before bug 878805
  MoveMessageInSearch(event.target) = designed/implemented only to do action on the selected folder
  DoActionOnTheSelectedFolder = MoveMessageInSearch(event.target) 
  PrepareAtParentDirectoryForActionOnSelectedFolder = null
(2) After bug 878805
  MoveMessageInSearch(event.target) = not changed from before
  DoActionOnTheSelectedFolder = null 
  PrepareAtParentDirectoryForActionOnSelectedFolder = MoveMessageInSearch(event.target)
(3) After bug 1319299 (changed back to original of "Before bug 878805")
  MoveMessageInSearch(event.target) = not changed from before 
  DoActionOnTheSelectedFolder = MoveMessageInSearch(event.target) 
  PrepareAtParentDirectoryForActionOnSelectedFolder = null
  
I think that bug 878805 wrongly placed MoveMessageInSearch in menupopup,
even though MoveMessageInSearch is still designed/implemented only to do an action on selected folder.
I think that bug 878805 did wrong job merely to place oncommand on menupopup, only for his satisfaction on beautiful layout of source code.

Comment 36

[:aceman]

(In reply to WADA from comment #35)
> Have you imagined following usage?
>   <button oncommand="DoActionOnTheSelectedFolder();">
>   <menupopup type="folder" mode="filing"
>      oncommand="PrepareAtParentDirectoriesForActionOnSelectedFolder()";>

I don't think we use rely on this in current code, but yes, this works with the patch.
PrepareAtParentDirectoriesForActionOnSelectedFolder() is executed once, then DoActionOnTheSelectedFolder() once.

> I think that bug 878805 did wrong job merely to place oncommand on
> menupopup, only for his satisfaction on beautiful layout of source code.

The intent of bug 878805 was good. It is good to be able to rely that oncommand (and the other attributes) are always on the same element. Either menulist/button or its child menupopup. So menupopup was chosen for now. If it proves to cause problems, we can switch all the pickers to the menulist later. But for now menupopup seems to work, except that we must take care of the cloned command. All other attributes still need to be cloned.

Comment 37

[Jorg K (CEST = GMT+2)]

Comment on attachment 8813366 [details] [diff] [review]
patch

I'm sorry to say, but I have no opinion, I'll forward this to someone who hopefully has one.

In bug 878805 we read:
5. oncommand should be either on menulist or menupopup, but consistent.

So somehow the option was chosen that caused some severe regressions. I would have opted for the other side of consistent ;-) I see no good reason for having it on the popup, given that even Aceman said in comment #36: "Either menulist/button or its child menupopup. So menupopup was chosen for now. If it proves to cause problems, we can switch all the pickers to the menulist later."

Well, it has caused problems, so why not switch it back as already done in bug 1319299 and bug 1133212? I find it funny that a clone is not a clone but a "clone, however, this we don't want to clone".

How many cases of this do we have?

Comment 38

[WADA:World Anti-bad-Duping Agency]

(In reply to :aceman from comment #36)
> The intent of bug 878805 was good. It is good to be able to rely that
> oncommand (and the other attributes) are always on the same element.

There are two kinds of *DESIGN* in type="folder".
> (A) oncommand of type="folder" is invoked at each parent folder in expanded folder picker
>     <button oncommand="DoActionAtThisButtonWhenAnFolderIsSelected();">
>     <menupopup type="folder" mode="filing"
>       oncommand="DoActionAtAnyParentFolderInFolderHiearchyWhenAnFolderIsSelected()";>
> (B) oncommand of type="folder" is invoked at explicitly defined xul element,
>     instead of at each hidden elements expanded by type="folder"
>     <button oncommand="DoActionAtThisButtonWhenAnFolderIsSelected();">
>     <menupopup type="folder" mode="filing"
>       oncommand="DoActionAtThisMenupopupWhenAnFolderIsSelected()";>

I'm talking about right/wrong, instead of good/bad.
If design is (A), "change code from (A) to (B) without design change" is wrong, and expecting (B) is merely misuse.

From perspective of SearchDialog.xul creater, I agrere on that design (A) is slightly confusing for us.
I think that user's understanding is usually (B), because expanded folder picker elements are invisible.

Your code change means "design change from (A) to (B)".
If any user of menupopup type="folder" expects (B), "design change from (A) to (B)" is good and right.
So, your code change is good and right, as far as "design change from (A) to (B)" is accepted by anyone.

Which is actual desin of type="folder"?
No design, and it's merely that oncommand/command is currently copied to expanded folder picker elements?

Anyway, because change by bug 878805 is already made at many places, I belive that your proposal(not copy oncommand/command to expanded elements) should be landed as soon as possible.
Even if critical problems like Bug 1133212(SeaMonkey)/Bug 1319299(Thunderbird) won't happen or is not exposed yet on other elements,
unwanted problems may be generated at other elements, because oncommand in menupopup type="folder" is invoked multiple times after change by bug 878805.

By the way, we loose powerful tool to reproduce problem of Bug 1110583 pretty easily/consistently, due to Bug 1319299 and your proposal :-)

Comment 39

[alta88]

Comment on attachment 8813366 [details] [diff] [review]
patch


The core problem is unexpected and undesirable bubbling of oncommands and onpopupshowing and such to ancestor nodes in nested menus. If it's intentional, it's bad design; this is seen not just in folderpicker but in View menu, eg (Bug 867393) and elsewhere where commands are actually different functions, and forces including code to prevent the event from bubbling in such functions (Bug 1115183). It's also unlikely anyone will address this in core xul.

As a workaround, removing oncommand etc. when cloning nested folderpicker menupopups is fine; so is moving the oncommand to the menulist (except that would be more change).

What is not ok, is mixing and expecting multiple commands, one from the menupopup and one from the menulist/button, though it's not clear if this is even intentional design in folderpicker.  This is confusing, difficult to maintain, ugly, and should be ripped out if found in code.  The entirety of an action should be defined on one element only.

Comment 40

[Richard Marti (:Paenglab)]

The folder menupopups are looking good on Windows and OS X.

Comment 41

[:aceman]

(In reply to Jorg K (GMT+1) from comment #37)
> I'm sorry to say, but I have no opinion, I'll forward this to someone who
> hopefully has one.
I only thought you could comment if this patch does not regress bug 1319299 (as it is reverting your patch from there).

> Well, it has caused problems, so why not switch it back as already done in
> bug 1319299 and bug 1133212?
Why switch it back if it can be fixed easier with less code churn?

> I find it funny that a clone is not a clone but
> a "clone, however, this we don't want to clone".
As can be seen in the comment in the code, we do not actually wanted a clone by design. It is just a must due to other bug. So we are forced to do a clone and then take the necessary steps to get it into shape.

So it seems  that if we were free to do what is needed "by design", then we would just made a .createElement() and copy only the wanted attributes to it. Which is the whitelist approach.
I could simulate this by making a clone and removing all except the whitelisted attributes.
This could be semantically closer to the original intent. Do you ask for that?
 
> How many cases of this do we have?
Around 40 occurrences of type="folder".

(In reply to WADA from comment #38)
> There are two kinds of *DESIGN* in type="folder".
> > (A) oncommand of type="folder" is invoked at each parent folder in expanded folder picker
> >     <button oncommand="DoActionAtThisButtonWhenAnFolderIsSelected();">
> >     <menupopup type="folder" mode="filing"
> >       oncommand="DoActionAtAnyParentFolderInFolderHiearchyWhenAnFolderIsSelected()";>
> > (B) oncommand of type="folder" is invoked at explicitly defined xul element,
> >     instead of at each hidden elements expanded by type="folder"
> >     <button oncommand="DoActionAtThisButtonWhenAnFolderIsSelected();">
> >     <menupopup type="folder" mode="filing"
> >       oncommand="DoActionAtThisMenupopupWhenAnFolderIsSelected()";>

> Your code change means "design change from (A) to (B)".
> If any user of menupopup type="folder" expects (B), "design change from (A)
> to (B)" is good and right.
> So, your code change is good and right, as far as "design change from (A) to
> (B)" is accepted by anyone.

I think there was no design change in bug 878805, we always wanted code executing as case (B) AND looking like code (B), but due to the overlooked copying of oncommand attributes we got code looking as (B), but executing as (A). So my patch just tries to fix the bug and make the execution as (B), as intended.

> Which is actual desin of type="folder"?
> No design, and it's merely that oncommand/command is currently copied to
> expanded folder picker elements?

I think what is Alta wants to say is a bug in "bubbling" of events (clicks) and executing of commands in ancestor menupoups. If you click on a folder (menuitem) in the hierarchy of the menupopups in the folder picker, it would be expected only that one oncommand of that leaf menupopup is executed (and no parents). If that was the case, the copying of the command to all child menupopups would be needed (and it was there in the code).

But there exists this bubbling, whether a bug or intentional, so we do not need the copying of command.


> Anyway, because change by bug 878805 is already made at many places, I
> belive that your proposal(not copy oncommand/command to expanded elements)
> should be landed as soon as possible.
> Even if critical problems like Bug 1133212(SeaMonkey)/Bug
> 1319299(Thunderbird) won't happen or is not exposed yet on other elements,
> unwanted problems may be generated at other elements, because oncommand in
> menupopup type="folder" is invoked multiple times after change by bug 878805.

Yes, that is what I said. We get multiple executions in all folder pickers now, it just isn't seen as there are no visible actions (usually just saving some state variables or updating items). Executing those multiple times does no harm. But is inefficient.

(In reply to alta88 from comment #39)
> As a workaround, removing oncommand etc. when cloning nested folderpicker
> menupopups is fine; so is moving the oncommand to the menulist (except that
> would be more change).

Yes, thanks.

> What is not ok, is mixing and expecting multiple commands, one from the
> menupopup and one from the menulist/button, though it's not clear if this is
> even intentional design in folderpicker.  This is confusing, difficult to
> maintain, ugly, and should be ripped out if found in code.  The entirety of
> an action should be defined on one element only.

We have code like this e.g. at https://dxr.mozilla.org/comm-central/rev/5aa6e96146e52ae578d36c5ac5afc246c5b77c5a/mail/base/content/mailWindowOverlay.xul#3128 where the button has a different command than the menupopup items.
And yes, it was also bubbling improperly until it fixed it in bug 989090. So it is as you say, we didn't want both commands to run, always only one of them.

Comment 42

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #41)
> I only thought you could comment if this patch does not regress bug 1319299
> (as it is reverting your patch from there).
I'm sure you checked this, right? Of course I can try it for you.

> > Well, it has caused problems, so why not switch it back as already done in
> > bug 1319299 and bug 1133212?
> Why switch it back if it can be fixed easier with less code churn?
We should implement the *correct* solution and not worry about code churn.

Reading comment #39 I'm still unsure about what the *correct* solution would be, in fact, both solutions are a workaround, so perhaps there is no *correct* solution.

Maybe Ratty can comment. I trust his judgement.

> As can be seen in the comment in the code, we do not actually wanted a clone
> by design. It is just a must due to other bug. So we are forced to do a
> clone and then take the necessary steps to get it into shape.
I read that: "(We must use cloneNode here because ...".

> This could be semantically closer to the original intent. Do you ask for
> that?
If we go with this approach, I don't really care how you do it as long as it's documented.

> > How many cases of this do we have?
> Around 40 occurrences of type="folder".
And all those were touched in bug 878805? All of those have oncommand?
I just picked one at random:
https://dxr.mozilla.org/comm-central/rev/5aa6e96146e52ae578d36c5ac5afc246c5b77c5a/mail/base/content/mailWindowOverlay.xul#2931
oncommand is *not* on the popup there.

So I'm not convinced that is it more code churn to move them all up. Let's ask differently:
How many are on the popup that could profit from being moved up? How many are on the popup? Since yesterday, one less. In fact, is there any need for action in this bug here at all? The bug "Moving emails from search box ..." was fixed in bug 1319299, so here we got into a philosophical discussion about how to structure the menus.

Instead of the code change you're proposing here we could have some debug code that alerts the developer to the fact that an oncommand attribute is cloned onto the popup to ask them whether they really want that.

Something like:
*** Your clone carries an oncommand attribute.
*** Be careful, this will cause the command to be execute multiple times
*** with possibly undesired effects.

Comment 43

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #41)
> We have code like this e.g. at
> https://dxr.mozilla.org/comm-central/rev/5aa6e96146e52ae578d36c5ac5afc246c5b77c5a/mail/base/content/mailWindowOverlay.xul#3128
> where the button has a different command than the menupopup items.
> And yes, it was also bubbling improperly until it fixed it in bug 989090. So
> it is as you say, we didn't want both commands to run, always only one of
> them.
Interesting, in bug 989090 you added the event.stopPropagation() in the popup.

So looks like bubbling up is a XUL "feature" and it would therefore be good to put the oncommand as high in the tree as possible and not place it onto popups and sub-popups where avoidable. This is in order to use the "feature" and not work against it.

Let's see how many oncommands are really on the popup, certainly not the 40 you mentioned.

Comment 44

[:aceman]

(In reply to Jorg K (GMT+1) from comment #42)
> (In reply to :aceman from comment #41)
> > I only thought you could comment if this patch does not regress bug 1319299
> > (as it is reverting your patch from there).
> I'm sure you checked this, right? Of course I can try it for you.

Sure I did. But the point of reviews/feedback is not to blindly trust the author :)

> Reading comment #39 I'm still unsure about what the *correct* solution would
> be, in fact, both solutions are a workaround, so perhaps there is no
> *correct* solution.

Exactly.

> > As can be seen in the comment in the code, we do not actually wanted a clone
> > by design. It is just a must due to other bug. So we are forced to do a
> > clone and then take the necessary steps to get it into shape.
> I read that: "(We must use cloneNode here because ...".

So we really do not want a clone, we are just forced to call it.

> > > How many cases of this do we have?
> > Around 40 occurrences of type="folder".
> And all those were touched in bug 878805? All of those have oncommand?
> I just picked one at random:
> https://dxr.mozilla.org/comm-central/rev/
> 5aa6e96146e52ae578d36c5ac5afc246c5b77c5a/mail/base/content/mailWindowOverlay.
> xul#2931
> oncommand is *not* on the popup there.

Yes, it seems those that actually carry out a permanent action (like message move) have already been left on the menulist.

> So I'm not convinced that is it more code churn to move them all up. Let's
> ask differently:
> How many are on the popup that could profit from being moved up? How many
> are on the popup?

But there are cases where we have a different command on the parent button/menu/menulist and a different command on menupopup. In those cases you can't move the command to the parent. So you will get back the mess that bug 878805 was trying to clean up. Some commands on menupopups some on the parents. Yes, you could argue that we should separate the command out and not consider it part of folder picker widget semantics. It could have different status then the other special attributes that we need to clone in the widget.

> Since yesterday, one less. In fact, is there any need for
> action in this bug here at all? The bug "Moving emails from search box ..."
> was fixed in bug 1319299, so here we got into a philosophical discussion
> about how to structure the menus.

No, there is still a real bug on those pickers that have the oncommand on the menupopup, it just isn't obviously visible.
 
> Instead of the code change you're proposing here we could have some debug
> code that alerts the developer to the fact that an oncommand attribute is
> cloned onto the popup to ask them whether they really want that.
> 
> Something like:
> *** Your clone carries an oncommand attribute.
> *** Be careful, this will cause the command to be execute multiple times
> *** with possibly undesired effects.

But currently we never want it to execute multiple times, as far as I could see. Why add code to detect a situation we do not want to support? If we can prevent it to run multiple times automatically. And IF somebody finds a need for that in the future, let him code support for it :) It is hard to imagine a need for it as if you get the leaf folder, it contains the path of all its parent folders in its nsIMsgFolder object. If we wanted run code on all the parents, we can easily use that. No need to run that same code from oncommands in the parent menupopups.

Comment 45

[Jorg K (CEST = GMT+2)]

Let's not get confused here, or maybe I'm confused. Take
https://dxr.mozilla.org/comm-central/rev/5aa6e96146e52ae578d36c5ac5afc246c5b77c5a/mail/base/content/mailWindowOverlay.xul#3128

The button does one thing, the popup another. In the popup you need to put stopPropagation(). That's a XUL "feature" and we won't change that. Even when you fix your clone you can't remove the stopPropagation().

The issue with the clone seems to be this. There is already a popup and we modify this dynamically by adding sub-menus. At least that's how I understand:
https://dxr.mozilla.org/comm-central/rev/5aa6e96146e52ae578d36c5ac5afc246c5b77c5a/mailnews/base/content/folderWidgets.xml#514
We add something to the already existing popup, right? And in the leaves we add, the oncommand gets cloned onto. And in the end, three commands get run: The one from the owning element, the one from the popup and the one from the new leaf?

Anyway, once again: How many cases do we have that would profit from this fix and where are they? We need to know anyway so we can test them. I'm not against fixing the clone code for those cases, but I don't see any necessity to move the commands back down into the popup just because we can.

I'd close this bug as duplicate of bug 1319299 and open a new one to investigate the clone issue and spell out the cases that we fix with it.

Comment 46

[:aceman]

(In reply to Jorg K (GMT+1) from comment #45)
> Let's not get confused here, or maybe I'm confused. Take
> https://dxr.mozilla.org/comm-central/rev/
> 5aa6e96146e52ae578d36c5ac5afc246c5b77c5a/mail/base/content/mailWindowOverlay.
> xul#3128
> 
> The button does one thing, the popup another. In the popup you need to put
> stopPropagation(). That's a XUL "feature" and we won't change that. Even
> when you fix your clone you can't remove the stopPropagation().

Yes, correct. And for cases like this we need my code, even if you move all the other oncommands from menupopups which can be moved up. But this one can't. Unless we somehow rewrite this command to do the right thing for both cases, e.g. by passing some argument.
 
> The issue with the clone seems to be this. There is already a popup and we
> modify this dynamically by adding sub-menus. At least that's how I
> understand:
> https://dxr.mozilla.org/comm-central/rev/
> 5aa6e96146e52ae578d36c5ac5afc246c5b77c5a/mailnews/base/content/folderWidgets.
> xml#514
> We add something to the already existing popup, right? And in the leaves we
> add, the oncommand gets cloned onto. And in the end, three commands get run:
> The one from the owning element, the one from the popup and the one from the
> new leaf?

Yes. But in the example in question we do not add any more sub popups.
 
> Anyway, once again: How many cases do we have that would profit from this
> fix and where are they? We need to know anyway so we can test them. I'm not
> against fixing the clone code for those cases,

> but I don't see any necessity
> to move the commands back down into the popup just because we can.

Ok, I could leave them.
 
> I'd close this bug as duplicate of bug 1319299 and open a new one to
> investigate the clone issue and spell out the cases that we fix with it.

It fixes all cases where oncommand is on the menupopup (which are many for now). That can be fixed here without enumerating them specifically.

If you make a new bug for moving some oncommands to the parent element, I'm fine with that. If you get past Alta and squib ;)

These 2 patches are independent, as even after your bug, some oncommands may stay on menupopup. Or new ones will be added in the future.

Comment 47

[Jorg K (CEST = GMT+2)]

OK, the summary of this bug is:

Bug 1106225 - Moving emails from Search Messages results to other folders creates duplicate emails and/or phantom mail, because oncommand is placed in <menupopup type="folder"> instead of in container <menu> of the menupopup, and because Bug 1110583 is not resolved yet

That bug is fixed by bug 1319299. There can't be any discussion about this. Any discussion we want to have about oncommand positioning and cloning should go elsewhere. I will raise a new bug for it. Stand by.

So: FIXED BY BUG 1319299.

Comment 48

[Jorg K (CEST = GMT+2)]

Comment on attachment 8813366 [details] [diff] [review]
patch

Sorry, let's review this elsewhere.

Comment 49

[:aceman]

But then copy all the discussion to the new bug, including comment 16.

Comment 50

[Jorg K (CEST = GMT+2)]

Raised bug 1319930 as continuation.

Comment 51

[WADA:World Anti-bad-Duping Agency]

(In reply to Jorg K (GMT+1) from comment #47)
> So: FIXED BY BUG 1319299.

I agree.
I already separated following part, which is essential cause of phantom mails, to Bug 1110583.
  Generates phantom mails merely by multiple move mails requests at same time.
And "duplicated mails" part was already removed by Thunderbird 45.
  Because messageKey starts from 1 with increment by 1 in Thunderbird 45,
  and because "messageKey from Offset" is removed in Thunderbird 45,
  "Duplicated mails of this bug" doesn't occur in Thunderbird 45,
  instead, phantom mails are consistently generated in Thunderbird 45.
  And, the phantom mails are not hidden by overwriting, so phantom mails are always exposed.
  This is a reason why I rejected "duping Bug 1319299 to this bug".
Thanks for quick fixing of Bug 1319299.

And, sorry for repeated referring to bug 1001071 instead of bug 1133212 in that bug.
Before aceman pointed in Bug 1319299, I was aware of that I was confusing bug 1001071(who generated problem) and bug 1133212(who resolved problem), and while I'm writing correct relation and history, aceman kindly pointed correct bug 1133212.
Sorry for my confusion.