670445 - When creating a virtual folder, the selected folder for search is incorrect. (it may miss the top-most folder in the search target, includes unexpected folder from previous search?)

Status: RESOLVED FIXED

(MailNews Core :: Search, defect, P2)

Description

[ISHIKAWA, Chiaki]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:5.0) Gecko/20100101 Firefox/5.0
Build ID: 20110615151330

Steps to reproduce:

I am using the following 3.1.1 on Windows 7.
Mozilla/5.0 (Windows; U; Windows NT 6.1; ja; rv:1.9.2.18) Gecko/20110616 Thunderbird/3.1.11

Choose a folder with lower sub-folders for search target and search with a certain criteria such as last 195 days (and including sub-directory as target.)

Looked at the search results and noted the message count, 5000+.

Now saved the search as the virtual search folder using the button at the lower-right corner of the search result window.

Noticed that the search result in the created virtual folder  showed the message count as only about 3500+.
Much smaller than the original.

Strange (!?)



Actual results:

I investigated and found that the
virtual save folder LOST the top-most folder as its target.
All the sub-folders and below are marked as target.

That explains the missing messages.
When I ticked and marked the top-most folder as its target,
the searched message count matched the original 5000+ number.




Expected results:

When creating the virtual save folder, the top-most folder
should not be removed from the search target.

I think this is a newly introduced bug in the last 3 months or so.
I used the virtual search folders in February and March and
didn't realize this problem back then.
 
I tried a different folder and different last N days condition and noticed the missing top-level search target directory when
the virtual folder is created by hitting the button in the lower-right corner of the search result button.

Comment 1

[ISHIKAWA, Chiaki]

OK, I am reporting this bug to a user-created folder at the same level as
the Inbox folder under my account: let me call it "eticket" folder.
It seems that Inbox doesn't show this problem. 
Also when I tried the search agains a subfolder, the symptom didn't show up.

Only when I tried the last N days search against this eticket folder, the problem
manifests.
Last 195 days.
Last 7 days even.

Also, when I create the virtual search folder, I create it under "Local Folders".

Hope above helps others to re-create the problematic symptom on the computers.

Comment 2

[ISHIKAWA, Chiaki]

Agah. 

This is worse. The bug manifests in a history sensitive manner.
I think there is a variable or something that is not initialized for a search against a user-created folder or something after a startup.

Start TB3.

Try search against eticket as noted above, and save the virtual search folder e-last-7-days under Local Folder.
(for last 7 days).
The bug manifests.
Delete the virtual folder e-last-7-days.

Now tried the same for Inbox.
search for last 7 days, and created r-last-7-days under Local Folder.
The bug doesn't manifest itself.
Delete the virtual folder r-last-7-days.

Tried AGAIN for eticket folder.
Try search against eticket, and save the virtual search folder e-last-7-days under Local Folder.
(for last 7 days).
The bug does NOT manifest itsefl THIS TIME AROUND! The count in the virtual folder matches the original search count.

This type of bug keeps both the reporter and the people trying to fix the bug
really confused...

Comment 3

[Wayne Mery (:wsmwk)]

Still see this?

Which of the following is the folder relationship you describe?

Inbox
  +-- eticket
Drafts

Or

Inbox
Drafts
...
eticket

Comment 4

[ISHIKAWA, Chiaki]

(In reply to Wayne Mery (:wsmwk) from comment #3)
> Still see this?
> 
> Which of the following is the folder relationship you describe?
> 
> Inbox
>   +-- eticket
> Drafts
> 
> Or
> 
> Inbox
> Drafts
> ...
> eticket

The directory structure was the latter.

Now do I still see this with 24.4.0?
Oops. I should not see this any more. I have done operations similar to the original report
without noticing the bug.
Was I not attentive to be unaware of the bug (?) :-(

Let me check. Let me try the procedure in Comment 2.

Short Answer: YES!!! I hit a different bug of a sort before I see the bug.
It does seem to be history-sensitive. You have to create a virtual folder and then delete it
and recreate the virtual folder to see the problem.


Long Answer: here is the result of test.

1. Select "eticket" folder.
Let me search for e-mails for the last 7 days.
It found 305 messages. (Some are in the subdirectories of "eticket").

1-(b) Now save this search result as virtual folders under "Local Folder"
I select "e-last-7-days" as the name of the virtual folder and create it under "Local Folder"s.
Initially it does not have any articles (# of articles is not shown.)

1-(c) visit "e-last-7-days" virtual folders.
TB searches the articles.

Agah, I may have found a new issue: TB showed "Searching" in the lower left pane of the window
(in Japanese because I am using 
TB in Japanese locale on Japanese Windows 7. Even though it apparently finished searching,
it kept this "Searching" message in the window pane.
I got curious and select an article in now visible window after waiting for a long time, 
and ONLY  THEN this
"Searching..." message disappeared. It should have disappeared spontaneously.

As far as the number of articles searched is OK. It is 305.
I checked that the top-level "eticket" is in the search target by
clicking "Property" of "e-last-7-days" and checked the search (target) folder.
When I reported the issue in the original post, "eticket" was not in the target (!)

2. Let me do the same for Inbox by searching the last 7 days worth of messages.

I clicked on Inbox, and searched for messages in the last 7 days.
About 12 seconds later, I got 884 messages.
Some are in the subdirectories of Inbox.

2-(b) let me create a virtual folder r-last-7-days by
saving the search result under "Local Folders".
Initially it is empty. No numbers are shown for unread, all, etc. in the folder pane for this folder.

2-(c) I visit "r-last-7-days" by clicking on it.
Funny. This time, the search finished very quickly, it seemed, and TB
didn't bother to show "Searching..." in the lower-left pane.

Very inconsistent behavior.

The total # of messages shown is 884. So the number is OK.
And the property of "r-last-7-days" showed the "Inbox" is properly the target of search (and its subdirectories).

 
Things get interesting after I delete and recreate virtual folders. See below.

3. Now at this point, I realized that I had not deleted e-last-7-days before trying the
test for r-last-7-days (like in comment 3.), and so
re-tried the creation of r-last-7-days after deleting both
e-last-7-days and r-last-7-days.

Here I began to see strange problem(s).

I deleted the two virtual folders.

First strange problem.

Agah, now when I tried to search for the messages in the last 7 days for Inbox, I got the
bug of 
Bug 625678 - Missing input search fields from the search window
the screen was like
https://bug625678.bugzilla.mozilla.org/attachment.cgi?id=8356019
Namely, there was no horizontal empty row to select the type of search and type the detailed condition.
Resetting produced the empty such row. 

So I thought. But the situation is worse today.

When I try to choose the search type of "Days since receipt" as "in the last 7 days", I realize
the left-most pull-down menu was a white empty rectangle {!?) and nothing is shown
in the pull down list!
This was not the case in bug625678 when I reported the issue.

Something is totally broken here.
Sorry, I failed to capture the screen.

It seems there are subtle bugs lurking around.

After I am typing this post furiously to report the very strange issue
I just described immediately above, I tried the same test procedure.
(It may be the in-memory structure may be already corrupt.)

The original problem recurred!

Search for Inbox for last 7 days of messages. Now 880 messages found. (The number is 4 messages short of the last search. Hmm...
Create a virtual folder, "r-last-7-days" again. But now it only shows 845 messages. 5 messages fewer than before!?
There is a discrepancy of # of messages now !

Then I checked the property of "r-last-7-days".
AGAH!
This time I found that the search target folders missed the top-most "Inbox" folder !!!

The problem was reproduced!

When I set the Inbox as the search target, the # of messages increased to 877, still
3 messages short of the first search result (???).
But maybe the internal in-memory database is corrupt already.
I am going to restart TB now.

Today's test was all under windows7 64-bit.

I hope that someone tries similar tests and can recreate the issue reported here.

TIA

Comment 5

[ISHIKAWA, Chiaki]

With a smaller message data, the current C-C TB created under 64-bit Debian GNU/Linux
did not show the particular problem reported here.

So I will try to see if I can capture interesting warning/error messages with the debug build of the
current version (if I can find it somewhere.).

However, the current C-C TB also produced disturbing warning messages:

When a virtual folder is created, DEBUG version of TB printed the following:

JavaScript strict warning: chrome://messenger/content/virtualFolderProperties.js, line 71: assignment to undeclared variable folderNameField

This seems rather problematic. I will file a bug on this.

There is also the following error when virtual folder is created (?)

************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "[JavaScript Error: "gFolderDisplay.treeSelection is null" {file: "chrome://messenger/content/SearchDialog.js" line: 555}]'[JavaScript Error: "gFolderDisplay.treeSelection is null" {file: "chrome://messenger/content/SearchDialog.js" line: 555}]' when calling method: [nsIController::isCommandEnabled]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: chrome://global/content/globalOverlay.js :: goUpdateCommand :: line 78"  data: yes]
 
************************************************************

Also, for whatever the reason, C-C TB seems to invoke "compaction" under the hood (!?) for 
virtual folder creation.
This is more problematic since compaction is known to invalidate some tacit assumptions
about msgid == message offset 
(for a large folder)
and is known to cause trouble.

I will leave this C-C issue for a later investigation, but
try the debug version of the current ESR to learn more about
why/how the problem here is caused.

TIA

Comment 6

[WADA:World Anti-bad-Duping Agency]

(In reply to ISHIKAWA, Chiaki from comment #4)
> Funny. This time, the search finished very quickly, it seemed,
> and TB didn't bother to show "Searching..." in the lower-left pane.

Open of virtual folder named VF consists of;
1. open VF.msf, read VF.msf
2. for(Folder in search target){open Folder;select MsgDBHdr by rules;add MsgDNHdrs to VF.msf;}
If MsgDatabase of Folder is not opened yet when VF is opened, "open Folder" invokes MsgDatabase open which requires read of entire Folder.msf.
If MsgDatabase of Folder is already opened when VF is opened, "cached MsgDatabase" is quickly returned to "open Folder".
Because open Folder is OpenWithReparse type, rebuild-index is invoked if "outdated msf condition" exists in Folder.msf and if MsgDatabase for Folder is not opened yet  when VF is opened.

As you say, if search target folder has deleted mails, Compact may be invoked silently when Virtual Folder tries to read search target folder, if Auto-Compact is enabled and "dialog before start auto-compact" is prohibited by you. Please note that "Virtual Folder open" will never invoke Compact.
Compact(Auto-Compact) may invoke Rebuild-Index if "outdated msf condition" exists. But Compact may not execute actual compaction when "outdated msf condition" exists, even when compactcompleted event is notified to Folder event listener. This is because Compact expects Rebuild-Index upon next explicit folder open, such as folder open by folder click at folder pane.

If you talk about performance, time etc., please compare with same condition.

For "number of mails returned by "in the last 7 days".
Do you know definition of "Age" in virtual folder and filter?
IIRC, Age in MsgPurge in POP3 was "time from download", although I'm not sure.
Is same search result guaranteed in your test?

Second concern. MsgPurge can remove mails between "a search by Virtual Folder" and "another Search by same Virtual Folder".

Third concern : "outdated msf condition".

When filter move, mail copy/move, draft/sent mail save etc. may generate "outdated msf condition". If msgDatabase is somehow not closed just after the filter move, "outdated msf condition" is not correctly detected until "close msgDatabase by idle time threshold" occurs and msgDatabase is opened by someone who invokes Rebuild-Index when "outdated msf condition" exists.
The "who invokes Rebuild-Index" is :
  Explicit folder open by folder click at folder pane
  Showing Folder Properties
  Search target folder open by Virtual Folder
If Inbox is accessed after "outdated msf condition generation by filter move" before "msgDatabase close of Inbox", MsgHDBHdr count is one before filter move. If Inbox is accessed by Virtual Folder after "msgDatabase close of Inbox", Rebuild-index is executed, so MsgDBHdr count contains mail moved by filter.
If this kind of problem occurred on deleted mail, following may occur.
  Deleted flag is set On in X-Mozilla-Status: header, but msgDatabase/Inbox.msf is not updated.
  Before msgDatabase close : MsgDBHdr count is not decreased.
  After msgDatabase close/reopen of msgDatabase/Rebuild-Index : MsgDBHdr count is decreased.
See bug 905576 for this kind of issues, please.

Please keep "comparison with surely same condition".
And please surely isolate problem with "outdated msf condition", problem without "outdated msf condition"(clean case), problem with Compact, problem without Compact(clean case).

> [JavaScript Error: "gFolderDisplay.treeSelection is null" {file: "chrome://messenger/content/SearchDialog.js" line: 555}
> http://mxr.mozilla.org/comm-central/source/mail/base/content/SearchDialog.js#555
> 555   return gFolderDisplay.treeSelection.count;
gFolderDisplay.treeSelection==null may occur, if nothing is selected at thread pane, if account centrl is shown, ... It may be timing dependent.
Code like next is safer always, unless "gFolderDisplay.treeSelection is set" is surely guaranteed:
 let cnt=0; if(gFolderDisplay.treeSelection&&0<gFolderDisplay.treeSelection.count)
            cnt=gFolderDisplay.treeSelection.count; return cnt;

> Then I checked the property of "r-last-7-days".
> AGAH!
> This time I found that the search target folders missed the top-most "Inbox" folder !!!
If a search target folder is deleted, the search target folder is removed from definition of search folder(virtual folder). And if last search target folder is deleted, the search folder(virtual folder) is silently deleted, and the search folder is automatically removed from folder pane.
This kind of phenomenon?

By the way, name, target, rule of Virtual Folder is saved in virtualFolders.dat file. Please paste definition data, instead of describing in words.

Comment 7

[WADA:World Anti-bad-Duping Agency]

ISHIKAWA san、 shall I open my tool for following?
(a) Log DBOpened/HdrAdded/HdrDelete/compactcompleted event etc. of selected folders
    using Folder/MsgDB event listener.
(b) Print property of msgFolder / msgDaabase of selectedfolders
(c) Print property of msgDBHdr of selected mail in a mail folder
This was used in analysis of bug 905576. In that bug, Virtual Folder was fully utilized as "outdated msf condition" detector :-)
And I recently requested data by the tool to opener of Bug 482836. 
Tracking of open/close by MSGDB:5 logging is pretty hard due to too many log lines. Tracking by "DBOpened event of selected folder only" is handy for testing.
"Dump of messageKey/messageOffset of all messages in a folder before test and after test" is usable for comparison of content of folder before test and after test. Because of text data, same or different is known merely by biff. And added/deleted mail can be tracked by HdrAdded/HdrDeleted event log.

Comment 8

[WADA:World Anti-bad-Duping Agency]

(In reply to ISHIKAWA, Chiaki from comment #4)
> Agah, now when I tried to search for the messages in the last 7 days for Inbox,
> I got the bug of Bug 625678 - Missing input search fields from the search window
With addon relevant to filter installed? Do you see your problem with -safe-mode of Tb?

Comment 9

[ISHIKAWA, Chiaki]

(In reply to WADA from comment #8)
> (In reply to ISHIKAWA, Chiaki from comment #4)
> > Agah, now when I tried to search for the messages in the last 7 days for Inbox,
> > I got the bug of Bug 625678 - Missing input search fields from the search window
> With addon relevant to filter installed? Do you see your problem with
> -safe-mode of Tb?

Yes, the problem of Bug 625678 occurred EVEN AFTER I disabled add-ons. (windows7).

Also, I was not sure the last time I reported, but this time I confirm that
when the menu shows empty rows, and after search reset, EMPTY box (for condition type)
where string should be shown, something WITHIN TB takes a long time to finish and
I could not get immediate feedback when I clicked save searched virtual folder.
Only after 20-30 seconds (or could be longer), TB responded.
(In reply to WADA from comment #7)

> ISHIKAWA san、 shall I open my tool for following?
> (a) Log DBOpened/HdrAdded/HdrDelete/compactcompleted event etc. of selected
> folders
 [...]

Thank you for the offer, but I am not sure where the root cause is hiding this moment.
However, if the warning/error message from the full debug version does not help,
I can certainly use your tools to see if the tools may display symptoms that may
help us home in the root cause.

TIA

Comment 10

[WADA:World Anti-bad-Duping Agency]

FYI.
"Loss of all search target folders in Folder Properties of Virtual Folder" was observed by rename of "parent folder of the Virtual Folder and search target folder".
(1) Initial status:
    Folders : Test, Test/A, Test/B, Test/C
    Create Virtual Folder Test/VF1. Search target=Test, Test/A, Test/B, Test/C 
    Entry is created in virtualFolders.dat -> uri=mailbox://nobody@Local%20Folders/Test/VF1
(2) Rename Test to Test###(for ease of viewing). actual file name = Testd9d89810.msf(hashed due to #)
    uri in virtualFolders.dat is still uri=mailbox://nobody@Local%20Folders/Test/VF1
    and scope=mailbox://nobody@Local%20Folders/Test|...
    But Virtual Folder is normally accessible, because MsgDB of search target is already opened.
(3) Search, Save As Search Folder under Test###.
    Try to save as VF1 -> "already exists" error
    Save as VF2        -> uri=mailbox://nobody@Local%20Folders/Testd9d89810/VF2
    uri of VF1 is also updated, but scope of VF1 is not updated
      uri=mailbox://nobody@Local%20Folders/Testd9d89810/VF1
      scope=mailbox://nobody@Local%20Folders/Test|mailbox://nobody@Local%20Folders/Test/A| ...
      terms=AND (subject,doesn't contain,!!!!!!!!!!!!!!!!!!!!!!)
(4) Nothing is shown by VF1.
    Folder Properties of VF1 :
      Search target folder : because folders specified in scope doesn't exist, nothing is selected
      Rule : correctly shown
(5) Shutdown, Restart Tb
    uri=mailbox://nobody@Local%20Folders/Testd9d89810/VF1 is removed from virtualFolders.dat,
    because no scope folder.
    VF1 is still shown at Folder Pane
(6) Shutdown, Restart Tb
    uri=mailbox://nobody@Local%20Folders/Testd9d89810/VF1 doesn't exist in virtualFolders.dat,
    VF1 is not removed from folder pane even though VF1 is not defined in virtualFolders.dat.
    This may be due to hashed name.    

Above phenomenon is perhaps;
  If both uri= and scope= is modified at same time, Tb fails to update scope=.

Comment 11

[WADA:World Anti-bad-Duping Agency]

FYI.
There was no need of "rename PARENT folder". "Rename search target folder" was sufficient.
1. Create a Virtual Folder, 2. Rename a search target folder
=> scope in virtualFolders.dat is not updated
=> the search target folder is unchecked in Folder Properties of the Virtual Folder.

ISHIKAWA san does rename happen in your test procedure?
How about "delete of .msf for search target folder? (see bug 393352, please)

Comment 12

[ISHIKAWA, Chiaki]

(In reply to WADA from comment #11)
> FYI.
> There was no need of "rename PARENT folder". "Rename search target folder"
> was sufficient.
> 1. Create a Virtual Folder, 2. Rename a search target folder
> => scope in virtualFolders.dat is not updated
> => the search target folder is unchecked in Folder Properties of the Virtual
> Folder.
> 
> ISHIKAWA san does rename happen in your test procedure?
> How about "delete of .msf for search target folder? (see bug 393352, please)

I don't think so because basically it is creating a couple of virtual folder and then delete them, 
and then  creating another virtual folder when the problem exhibits.

(Well, I trashed my Debian GNU/Linux image: after the latest aptitude update/upgrade, something went wrong and it no longer runs well in VMplayer under windows7 64-bit. So I am reverting to the older kernel 3.2.x, and in the process, screwed up something. I think the grub loader from testing repository
was to blame. Anyway, due to this OS platform issue on my local PC, I could not compile 24.4.0 source
in full debug fashion to test the problem with enough warning/error messages.
I hope I can get it done in a couple of days.)

TIA

Comment 13

[ISHIKAWA, Chiaki]

(I posted this to mozilla.dev.apps.thunderbird under the title of
Re: 24.4.0 full debug build for linux 64-bit? 
There was a typo of "shoud" which ought to have been "should" and I fixed that.)


After a disk trouble, I could finally build the full debug version of TB 24.4.0
(linux 64-bit) on my local PC.

I have  not been able to reproduce the particular issues I saw under Windows7
and I suspect that it may be triggered by a large number of folders, multiple
user account, etc.
So I need to move at least a part of real-world e-mail folders into test
environment.

But in the meantime, I encountered a strange issue
of message header pane showing bogus (almost empty) message rows(?),
that was seen in the bug 944194
"After deleting a message by hitting delete button in the subject area, a
blank subject line still remains in subject pane"
https://bugzilla.mozilla.org/show_bug.cgi?id=944194

How to reproduce: After I create a virtual folder from the search result,
and moved the focus to the virtual folder to see the headers of the
searched messages in the message header area,
I delete the virtual folder. Voila, very strange almost empty rows remaining
in the message header area. I wonder what TB should do in this case.
Maybe moving its focus into the top-most folder and show what should be
displayed
when one picks up this top-most folder?)

So, I will proceed with debugging, etc.

Comment 14

[WADA:World Anti-bad-Duping Agency]

(In reply to ISHIKAWA, Chiaki from comment #13)
> and I suspect that it may be triggered by a large number of folders, multiple user account, etc.

Does "file open error due to file handle shortage" occur during restart of Tb?
"xxx.msf file open error during folder re-discopery" is same as "deleted xxx.msf" from perspective of ".msf file content access". See bug 855751.

> How to reproduce: After I create a virtual folder from the search result, and moved the focus to the
> virtual folder to see the headers of the searched messages in the message header area,
> I delete the virtual folder.

Did you do "delete Virtual Folder" when the Virual Folder is opened and messages in the Virtual Folder are shown at Thread Pane"?

Comment 15

[ISHIKAWA, Chiaki]

Now it seems to make a difference between the two methods:

(1) leave the search window open and use that to re-create the virtual folder, 

(2) or close the search window and then re-open search window, by clicking search from
context menu after choosing the folder as target.

It looks that these actions produce different results.

I will report more once I figure this out.

At least, I can reproduce the problem, but it is very history-sensitive :-(

TIA

Comment 16

[ISHIKAWA, Chiaki]

Attachment: A screen dump that shows the problem. (see comment).`

This is a screenshot that captures the problem.

When creating a virtual folder, the selected folder for search is incorrect.

How to reproduce the problem.

1. Start TB
2. Choose Inbox.
3. search from context-menu (right click)
4. age in days: 21 
5. search
6. save
7. choose "Local Folder" as the parent directory of the virtual folder.
8. type "r-21-days-dir" as the name.
9. Create.

Make sure to leave the search window.

10. visit the folder "r-21-days-dir" to see that it shows proper # of messages.

From here, use the remaining search window to search and save virtual folder.

11. select "extra-folder" as the target folder using the remaining search  
    window.

12. (keep the age in days: 21) and search
13. save
14. select "Local Folder" as the parent of the virtual folder.
15. type in "e-21-days-dir"
16. Create.
17. Visit "e-21-days-dir" to see if it contains proper # of messages.

Here I found too many messages, and so
while the pointer is on "e-21-days-dir" I opened the context-menu (right-click)
and choose property.
From there I checked the "choose" folder(s) [i.e., search target.]

Expected. The chosen folder should include
"extra-folder" and its sub-folder, in this case, "e-sub-2-dir".

What happened: The chosen folder somehow
included unexpected "Inbox", "e-sub-2-dir", but NOT "extra-folder".
 
I could reproduce this sequence a few times, and so I am sure this is a reproducible bug.

You can see the folder structure
Inbox
  sub-1-dir
Trash
extra-folder
   e-sub-2-dir

I will investigate if the debug build of 24.4 linux 64-bit version
produced any tell-tale sign of the problem.

TIA

Comment 17

[ISHIKAWA, Chiaki]

With the steps mentioned in my previous post, comment 16,
I can reproduce the problem with CURRENT comm-central TB.
So the problem still persists!

So I changed the version to 24, but it should be "unspecified?"

Anyway, this problem ought to be fixed.

Comment 18

[ISHIKAWA, Chiaki]

(In reply to WADA from comment #14)
> (In reply to ISHIKAWA, Chiaki from comment #13)
> > and I suspect that it may be triggered by a large number of folders, multiple user account, etc.
> 
> Does "file open error due to file handle shortage" occur during restart of
> Tb?
> "xxx.msf file open error during folder re-discopery" is same as "deleted
> xxx.msf" from perspective of ".msf file content access". See bug 855751.

I don't think I have seen this file open error during TB startup (yet).

> 
> > How to reproduce: After I create a virtual folder from the search result, and moved the focus to the
> > virtual folder to see the headers of the searched messages in the message header area,
> > I delete the virtual folder.
> 
> Did you do "delete Virtual Folder" when the Virual Folder is opened and
> messages in the Virtual Folder are shown at Thread Pane"?

Yes, exactly.

Please see mmy comment 16, 1and 17. Now I know how to re-produce the error.

TIA

Comment 19

[WADA:World Anti-bad-Duping Agency]

(In reply to ISHIKAWA, Chiaki from comment #18)
> > > How to reproduce: After I create a virtual folder from the search result, and moved the focus to the
> > > virtual folder to see the headers of the searched messages in the message header area,
> > > I delete the virtual folder.
> > 
> > Did you do "delete Virtual Folder" when the Virual Folder is opened and
> > messages in the Virtual Folder are shown at Thread Pane"?
> 
> Yes, exactly.

I could observe "blank thread pane raws" by "delete Virtual Folder while the Virual Folder is opened and messages in the Virtual Folder are shown at Thread Pane".
Virtual Folder is correctly deleted.
  virtualFolders.dat is coorectly updated. Entry is correctly removed.
  .msf file for the Virtual Folder is correctly deleted.
This is Thread Pane refresh issue.

Comment 20

[WADA:World Anti-bad-Duping Agency]

(In reply to ISHIKAWA, Chiaki from comment #18)
> Please see mmy comment 16, 1and 17. Now I know how to re-produce the error.

This is not problem of "saved search folder misses the top-most folder in the search target"?
Problem of "mismatch of pre-selected search target folders between actual pre-selected search target folders and expected pre-selected search target folders"?

1. Select real folder FolderA, Search Messages with "Search subfolders" checked
   => Search target=FolderA and his subfolders of FolderA
2. Save as Search Folder
   ~> pre-selected search target folder = FolderA subfolders of FolderA
3. Change search target folder from FolderA to FolderB at "Search Messages" panel,
   Save as Search Folder
   ~> pre-selected search target folder = FolderA, FolderB and subfolders of FolderB
FolderA which is currently selected at Folder Pane is not removed upon "Save as Search Folder" after search target folder change a "Search Mesages" panel?

Comment 21

[ISHIKAWA, Chiaki]

Attachment: error/warning printed out by full debug build of 24.4.0 under linux during testing described in comment 16

Here is the warning/error messages printed during testing procedure of comment 16
(printed by local full debug build of TB 24.4.0 under linux 64-bit (Debian GNU/Linux amd64).

Comment 22

[ISHIKAWA, Chiaki]

Attachment: error/warning printed out by full debug build of comm-central TB under linux during testing described in comment 16

Ditto for the local full debug build of comm-central TB.

Comment 23

[ISHIKAWA, Chiaki]

Attachment: Summary of messages from warning/error log 24.4.0.

A summary of messages from 24.4.0 ( created by a local shell script )

Comment 24

[ISHIKAWA, Chiaki]

Attachment: Summary of messages from warning/error log of current comm-central TB

ditto for the error/warning log from the current comm-central TB.

Comment 25

[ISHIKAWA, Chiaki]

This is an important bug for me.
I use TB for work.
So I take this up.

I feel this bug is a data loss bug because I used the virtual folder to
pick up some important messages and copy them to a note PC just before
my business trip.
This bug failed me to copy proper messages (!).

TIA

Comment 26

[ISHIKAWA, Chiaki]

Thank you for your comment.

n reply to WADA from comment #19)
> (In reply to ISHIKAWA, Chiaki from comment #18)
> > > > How to reproduce: After I create a virtual folder from the search result, and moved the focus to the
> > > > virtual folder to see the headers of the searched messages in the message header area,
> > > > I delete the virtual folder.
> > > 
> > > Did you do "delete Virtual Folder" when the Virual Folder is opened and
> > > messages in the Virtual Folder are shown at Thread Pane"?
> > 
> > Yes, exactly.
> 
> I could observe "blank thread pane raws" by "delete Virtual Folder while the
> Virual Folder is opened and messages in the Virtual Folder are shown at
> Thread Pane".
> Virtual Folder is correctly deleted.
>   virtualFolders.dat is coorectly updated. Entry is correctly removed.
>   .msf file for the Virtual Folder is correctly deleted.
> This is Thread Pane refresh issue.


Right. This is a Thread Pane refresh issue.
I see many failure warnings in the log from 24.4.0 and comm-central TB in the uploaded logs.

from 24.4.0:
   1912 WARNING: NS_ENSURE_SUCCESS(rv, highIndex) failed with result 0x8000FFFF: file /home/ishikawa/TB-24.4.0/comm-esr24/mailnews/base/src/nsMsgDBView.cpp, line 5211
    207 WARNING: NS_ENSURE_TRUE(IsValidIndex(aRow)) failed: file /home/ishikawa/TB-24.4.0/comm-esr24/mailnews/base/src/nsMsgSearchDBView.cpp, line 167
      8 WARNING: NS_ENSURE_TRUE(selection->GetRangeCount()) failed: file /home/ishikawa/TB-24.4.0/comm-esr24/mozilla/editor/libeditor/base/nsEditor.cpp, line 3844

from comm-central:

   1906 [19811] WARNING: NS_ENSURE_SUCCESS(rv, highIndex) failed with result 0x80070057: file /REF-COMM-CENTRAL/comm-central/mailnews/base/src/nsMsgDBView.cpp, line 5183

I am not if these are directly related to the refresh issue, but
I will discuss this in Bug 

bug 944194
"After deleting a message by hitting delete button in the subject area, a
blank subject line still remains in subject pane"
https://bugzilla.mozilla.org/show_bug.cgi?id=944194

(In reply to WADA from comment #20)
> (In reply to ISHIKAWA, Chiaki from comment #18)
> > Please see mmy comment 16, 1and 17. Now I know how to re-produce the error.
> 
> This is not problem of "saved search folder misses the top-most folder in
> the search target"?
> Problem of "mismatch of pre-selected search target folders between actual
> pre-selected search target folders and expected pre-selected search target
> folders"?
> 
> 1. Select real folder FolderA, Search Messages with "Search subfolders"
> checked
>    => Search target=FolderA and his subfolders of FolderA
> 2. Save as Search Folder
>    ~> pre-selected search target folder = FolderA subfolders of FolderA
> 3. Change search target folder from FolderA to FolderB at "Search Messages"
> panel,
>    Save as Search Folder
>    ~> pre-selected search target folder = FolderA, FolderB and subfolders of
> FolderB
> FolderA which is currently selected at Folder Pane is not removed upon "Save
> as Search Folder" after search target folder change a "Search Mesages" panel?

Well yes and no. 
Yes: Folder A seems to remain as the target of search even AFTER I selected the folder B as the search target.
No:  Folder B is NOT selected as the search folder. This is the problem.

Maybe I was unclear in comment 17:
>Expected. The chosen folder should include
>"extra-folder" and its sub-folder, in this case, "e-sub-2-dir".
 and NOTHING ELSE.

>What happened: The chosen folder somehow
>included unexpected "Inbox", 
and EXPECTED,
>"e-sub-2-dir", but 
>NOT "extra-folder".
EXPECTED "extra-folder" was MISSING (!).

I change the title to
When creating a virtual folder, the selected folder for search is incorrect. (it may miss  the top-most folder in the search target, includes unexpected folder from previous search?)

It is a mouthful, but reflect what I have found so far.

Comment 27

[ISHIKAWA, Chiaki]

I look in the warning/error messages and nothing stood out.
So I suspect that it is a simple logic bug that handles
the user interaction of creating a virtual folder from a search result.

Can anyone point me to the exact JS file (or C++ file?) which does exactly this?
I looked at
TB-SRC/mailnews/base/search/content/searchWidgets.xml
but it seems to handle the search involved in filters (?)

TIA

Comment 28

[WADA:World Anti-bad-Duping Agency]

"Save as Search Folder" button in "Search Messages" panel.
DOM Inspector shows following fo this button.
> <button xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
>    label="Save as Search Folder"
>    id="saveAsVFButton"
>    command="saveas_vf_button" 
>    accesskey="v" 
>    oncommand="goDoCommand('saveas_vf_button')"/>
command="saveas_vf_button" is defined at next code only.
> http://mxr.mozilla.org/comm-central/source/mail/base/content/SearchDialog.xul#191
oncommand="goDoCommand('saveas_vf_button')" is defined at next code only.
> http://mxr.mozilla.org/comm-central/source/mail/base/content/SearchDialog.xul#48
In XUL level, command="xxx" looks replaced by "command id=xxx" definition.

"saveas_vf_button" is refererd at.
> http://mxr.mozilla.org/comm-central/search?string=saveas_vf_button&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central
Dispatching command is done at.
> http://mxr.mozilla.org/comm-central/source/mail/base/content/SearchDialog.js#106
> 106         case "saveas_vf_button":
> 107             saveAsVirtualFolder();
> 108             return true;
"saveAsVirtualFolder(" is referred at.
> http://mxr.mozilla.org/comm-central/search?string=saveAsVirtualFolder%28&find=&findi=filter=^[^\0]*%24&hitlimit=&tree=comm-central
function saveAsVirtualFolder() is defined at.
> http://mxr.mozilla.org/comm-central/source/mail/base/content/SearchDialog.js#586

Comment 29

[WADA:World Anti-bad-Duping Agency]

(In reply to ISHIKAWA, Chiaki from comment #26)
>    1906 [19811] WARNING: NS_ENSURE_SUCCESS(rv, highIndex) failed with result
> 0x80070057: file
> /REF-COMM-CENTRAL/comm-central/mailnews/base/src/nsMsgDBView.cpp, line 5183

Difference between Rename and Delete of VF while VF is opened and shown at Thread pane.
- Delete : remove from virtualFolder.dat, delete VF.msf,
           remove from Folder Pane. Nothing is selected at Folder Pane after Delete VF.
           window.gDBView is deleted(undefined, null, ...)
           Because gDBView is lost, UI code perhaps fails to update Thread Pane correctly. 
- Rename : Rename looks "Delete + Create" from perspective of "VF content/display".
           virtualFolder.dat is updated, VF.msf is correctly removed.
           Folder Pane is correctly updated. Nothing is selected at Folder Pane after Rename VF.
           "Rename of VF" is correctly refrected to gDBView(getter is used, so it's normal)

> bug 944194
> "After deleting a message by hitting delete button in the subject area,
> a blank subject line still remains in subject pane"

"Delete mails at VF" is; (Call Search Target "ST1" to "STn")
  1. Delete msg#N in STx => HdrDeleted event on msg#N at STx
  2. VF tracks HdrDeleted event of search target folder.
     => remove "msg#N of STx" from VF.msf.
  3. Thread Pane UI receives "mail is deleted" event, and remove it from Thread Pane.
1/2/3 are executed asynchronously.
  Task #1                              Task #2                   Task #3
  for(STz in Delete related folders)
  {
    for(selected msg#N of STz)         Upon HdrDeleted event     When msg#N is removed from gDBView
    {                                    on msg#N in STx
      Delete MsgDBHdr of msg#N in STz      remove from VF.msf    remove from Thread Pane Row,
    }                                      remove from gDBView   and re-construnct Thread Pane display
  }
Timing related problem may occur.

Comment 30

[WADA:World Anti-bad-Duping Agency]

(In reply to ISHIKAWA, Chiaki from comment #26)
> Well yes and no. 
> Yes: Folder A seems to remain as the target of search even AFTER I selected
> the folder B as the search target.
> No:  Folder B is NOT selected as the search folder. This is the problem.

At Search Messages... panel, folder selected at "Search for messages in:" by folder picker is saved in "uri" attribute of menulist elemnt with id="searchableFolders".
   menulist id="searchableFolders"  uri="mailbox://nobody@Local%20Folders/TestX" <= Call FolderB 
"searchableFolders" is referred by following code, and is peraps passed to "Search" code via gFolderPicker variable. 
> http://mxr.mozilla.org/comm-central/search?string=searchableFolder

saveAsVirtualFolder() uses window.arguments[0].folder.URI.
This is "folder which was selected when the Search Messages was initially opened". <= Call FolderA
saveAsVirtualFolder() doesn't look to use above "menulist id=searchableFolders" itself <= Call FolderB
saveAsVirtualFolder() looks to use "subfolders of menulist id=searchableFolders" only.

Pre-selected search target folders at Virtual Folder defiition panel
  = FolderA + Subfolders of FolderB(if search sunolders is checked)
So, FolderB is ignored.
Right?

Comment 31

[ISHIKAWA, Chiaki]

(In reply to WADA from comment #30)
> (In reply to ISHIKAWA, Chiaki from comment #26)
> > Well yes and no. 
> > Yes: Folder A seems to remain as the target of search even AFTER I selected
> > the folder B as the search target.
> > No:  Folder B is NOT selected as the search folder. This is the problem.
> 
> At Search Messages... panel, folder selected at "Search for messages in:" by
> folder picker is saved in "uri" attribute of menulist elemnt with
> id="searchableFolders".
>    menulist id="searchableFolders" 
> uri="mailbox://nobody@Local%20Folders/TestX" <= Call FolderB 
> "searchableFolders" is referred by following code, and is peraps passed to
> "Search" code via gFolderPicker variable. 
> > http://mxr.mozilla.org/comm-central/search?string=searchableFolder
> 
> saveAsVirtualFolder() uses window.arguments[0].folder.URI.
> This is "folder which was selected when the Search Messages was initially
> opened". <= Call FolderA
> saveAsVirtualFolder() doesn't look to use above "menulist
> id=searchableFolders" itself <= Call FolderB
> saveAsVirtualFolder() looks to use "subfolders of menulist
> id=searchableFolders" only.
> 
> Pre-selected search target folders at Virtual Folder defiition panel
>   = FolderA + Subfolders of FolderB(if search sunolders is checked)
> So, FolderB is ignored.
> Right?

Seems like you have the cause of the bug!

BTW,

(In reply to WADA from comment #29)
> (In reply to ISHIKAWA, Chiaki from comment #26)
> >    1906 [19811] WARNING: NS_ENSURE_SUCCESS(rv, highIndex) failed with result
> > 0x80070057: file
> > /REF-COMM-CENTRAL/comm-central/mailnews/base/src/nsMsgDBView.cpp, line 5183
> 

Wada san, the above comment 29 is for 
bug 944194
"After deleting a message by hitting delete button in the subject area, a
blank subject line still remains in subject pane"

and not this bug 670445 ?

The asynchronous nature ringgs a bell, but it is better discussed in bug 944194 IMHO.

TIA

Comment 32

[WADA:World Anti-bad-Duping Agency]

(In reply to ISHIKAWA, Chiaki from comment #31
> and not this bug 670445 ?

That is applicable to this bug before you correct bug summary.
That is not applicable to this bug after you correct bug summary.

Anyway, we observed following in this bug.
0. If search target folder is deleted, it's removed from scope of Virtual Folder(known issue)
1. If search target folder is renamed, scope of Virtual Folder is not updated.
2. If mails are deleted at Virtual Folder, blank row is generated at Thread Pane of the Virtual Folder.
   (You already opened bug 944194)
3. If Virtual Folder is deleted while the Virtual Folder is opened and shown at Thread Pane,
   blank rows is shown at Thread Pane.
4. This bug after you correct bug summary.
   "Search Target folder of Search Messages" itself is ignored by Virtual Folder definition panel,
   if "Search Target folder of Search Messages" is changed at "Search Messages" panel,
   and if "Virtual Folder definition panel" is opened via ""Save as Search Folder" button.
   Workaround : Define Virtual Folder via "File/New/Saved Search".
                This is official method of "Virtual Folder" creation,
                althouh "save as VF from search message panel" was implemented first.
                Problem in this workaround : There is no support of "Search subfolders".
                                             (known issue. enhancement is already requested)
We are better open some bugs around Virtual Folder after sorting out and summarizing.

Comment 33

[ISHIKAWA, Chiaki]

Attachment: Proposal : Possible modification (debug dump) and a fix. Feedback solicited.

(In reply to WADA from comment #32)
> (In reply to ISHIKAWA, Chiaki from comment #31
> > and not this bug 670445 ?
> 
> That is applicable to this bug before you correct bug summary.
> That is not applicable to this bug after you correct bug summary.
> 
> Anyway, we observed following in this bug.

　　[ ... snip ... ]

> 4. This bug after you correct bug summary.
>    "Search Target folder of Search Messages" itself is ignored by Virtual
> Folder definition panel,
>    if "Search Target folder of Search Messages" is changed at "Search
> Messages" panel,
>    and if "Virtual Folder definition panel" is opened via ""Save as Search
> Folder" button.
>    Workaround : Define Virtual Folder via "File/New/Saved Search".
>                 This is official method of "Virtual Folder" creation,
>                 althouh "save as VF from search message panel" was
> implemented first.
>                 Problem in this workaround : There is no support of "Search
> subfolders".
>                                              (known issue. enhancement is
> already requested)
> We are better open some bugs around Virtual Folder after sorting out and
> summarizing.

Thank you for summarizing the issues. I didn't know that there are so many subtle issues here.

Thanks to comment 28 and comment 30, I could focus on the
following function to look for the cause of the bug (of No. 4 above: the bug discussed in
the original comment).

saveAsVirtualFolder() in 
mail/base/content/SearchDialog.js

A little  background when interested parties read this file:

|gCurrentFolder|: this is a global variable that seems to point to the
(top-most) folder for the search target that is set when
the search window is entered (OnLoad, etc.).
It has a few attributes.
gCurrentFolder
	.isserver  : I think when it is true, the
	folder is the remote newsgroup, or remote IMAP folder.)
	.noselect  : true when the folder is not selected(?)

	Any offlineSupportLevel > 0 is an online server like IMAP or news.
	.server.offlineSupportLevel : IMAP and offline ? 

It is set as in the following code.

300 function updateSearchFolderPicker(folder)
301 {
302   gCurrentFolder = folder;
303   gFolderPicker.menupopup.selectFolder(folder);

What is a scope?

I suppose it is obvious to many readers of bugzilla what a "Scope" is:
But the file does not have large amount of meaningful comments (in contrast to 
other well commented file.)
 
So here is a brief reminder of what "scope" is to a newbie like me:
this is what I found out.

"Scope" is used to identify whether the search target folder is a local
mail folder, remote IMAP folder, remote news folder, etc. so that
the UI to set search condition will show only RELEVANT (and
applicable) search terms (or whatever it is called).

321   setSearchScope(GetScopeForFolder(gCurrentFolder));

For example:

getScopeForFolder(...)

	offline mail =>w
	nsMsgSearchScope.offlineMail, 

	etc.

=============
PROBLEM in saveAsVirtualFolder
=============

Here is the crux of the problem.

First the code itself.

586 function saveAsVirtualFolder()
587 {
588   var searchFolderURIs = window.arguments[0].folder.URI;
589 
590   var searchSubfolders = document.getElementById("checkSearchSubFolders").checked;
591   if (gCurrentFolder && (searchSubfolders || gCurrentFolder.isServer || gCurrentFolder.noSelect))
592   {
593     var subFolderURIs = AddSubFoldersToURI(gCurrentFolder);
594     if (subFolderURIs.length > 0)
595       searchFolderURIs += '|' + subFolderURIs;
596   }
597 
598   var searchOnline = document.getElementById("checkSearchOnline");
599   var doOnlineSearch = searchOnline.checked && !searchOnline.disabled;
600 
601   var dialog = window.openDialog("chrome://messenger/content/virtualFolderProperties.xul", "",
602                                  "chrome,titlebar,modal,centerscreen",
603                                  {folder: window.arguments[0].folder,
604                                   searchTerms: getSearchTerms(),
605                                   searchFolderURIs: searchFolderURIs,
606                                   searchOnline: doOnlineSearch});
607 }

This is the code to set and create virtual folder, when I read the
code I had questions such as
    - what about the parent folder?
    - what about the list of folders?
    - what happens to the values above when user selects a
       folder as the (top-most) search target?
    - what about the previous search target folder? Is it marked off properly?
    - somehow |document| holds the value to decide whether we should
      search the subfolders?
      searchSubFolders :  document.getElementById("checkSearchSubFolders").checked

Then as I read it, I noticed a grave problem:

It seems that one assumption is made that 
window.arguments[0]...  holds folder name folder.URI and
|gCurrentFolder| contains the information to that folder also!  That
is, the following line pushed the subfolders of |gCurrentFolder| while
the primary |searchFolderURIs| is set to that of window.arguments[0].

593     var subFolderURIs = AddSubFoldersToURI(gCurrentFolder);

However, we don't have the guarantee that |gCurrentFolder| has the
value associated with the folder designated by window.arguments[0] any
more on the second and subsequent reuse of remaining search window.

Consider the scenario I produced in comment 16 to reproduce the buggy behavior.

That is, if we start with "Inbox" as the search target
initially, and leave the search window after the search result is
saved as virtual folder, AND using the same remaining search window to
choose a different folder, "extra-folder", using the selection in
the search box, and then hit "search", it is now likely that
|gCurrentFolder| points to "extra-folder" while window.arguments[0] is
STILL POINTING TO "Inbox". This matches the observed errant behavior!

If my analsyis above is right, when |gCurrentFolder| is overwritten to a
different value by user action (on the second reuse and later), then
|searchFolderURIs| should be changed from the value of
|window.argumetns[0].folder.URI|. 

I checked the operation by dumping the values and my observation and
bug analysis was CORRECT. See the debug change posted.

The real log showed that my guess was right!
(This is a log without the resetting of |searchFolerURIs| operation in
the posted change.)

First, I chose "Input" as the search target. When the execution
reached saveAsVirtualFolder():

Calling saveAsVirtualFolder() and before calling window.openDialog.
window.arguments[0].folder.URI =mailbox://ishikawa@localhost/Inbox
gCurrentFolder.URI             =mailbox://ishikawa@localhost/Inbox
searchFolderURIs               =mailbox://ishikawa@localhost/Inbox|mailbox://ishikawa@localhost/Inbox/sub-1-dir

	   [lots of lines snipped]

[21977] WARNING: NS_ENSURE_SUCCESS(rv, highIndex) failed with result 0x80070057: file /REF-COMM-CENTRAL/comm-central/mailnews/base/src/nsMsgDBView.cpp, line 5183
	...

[21977] WARNING: NS_ENSURE_SUCCESS(rv, highIndex) failed with result 0x80070057: file /REF-COMM-CENTRAL/comm-central/mailnews/base/src/nsMsgDBView.cpp, line 5183

Now, I re-used the remaining search box to search for "extra-folder".
See the dump print below.

Calling saveAsVirtualFolder() and before calling window.openDialog.
window.arguments[0].folder.URI =mailbox://ishikawa@localhost/Inbox
gCurrentFolder.URI             =mailbox://ishikawa@localhost/extra-folder
searchFolderURIs               =mailbox://ishikawa@localhost/Inbox|mailbox://ishikawa@localhost/extra-folder/e-sub-2-dir

Note that the |gCurrentFolder.URI| correctly points to
"extra-folder", but since |window.argumetns[0].folder.URI| still points
to "Inbox", the |searchFolderURIs| ended up 
with "Inbox" as the top-most directory initially , and the subdirectory
of "extra-folder" is pushed into |searchFolderURIs|.
(via  "593     var subFolderURIs = AddSubFoldersToURI(gCurrentFolder);")

This is INCORRECT.

Possible Fix:
(I am uploading a patch that prints out the values for debugging purposes,
and a fix.)

So I placed a fix. Namely, if |gCurrentFolder.URI| no longer points the
the same URI in |window.arguments[0].folder.URI|, I reset the initial
|searchFolderURIs| to |gCurrentFolder.URI|.

After the fix, the log now showed the expected behavior.

Excerpt from the log:

I chose "Input" as the search target.

searchFolderURIs is not reset.
Calling saveAsVirtualFolder() and before calling window.openDialog.
window.arguments[0].folder.URI =mailbox://ishikawa@localhost/Inbox
gCurrentFolder.URI             =mailbox://ishikawa@localhost/Inbox
searchFolderURIs               =mailbox://ishikawa@localhost/Inbox|mailbox://ishikawa@localhost/Inbox/sub-1-dir

			       ...

Now, I re-used the remaining search box to search for "extra-folder".

searchFolderURIs is reset to gCurretFolder.
Calling saveAsVirtualFolder() and before calling window.openDialog.
window.arguments[0].folder.URI =mailbox://ishikawa@localhost/Inbox
gCurrentFolder.URI             =mailbox://ishikawa@localhost/extra-folder
searchFolderURIs               =mailbox://ishikawa@localhost/extra-folder|mailbox://ishikawa@localhost/extra-folder/e-sub-2-dir

Note now that |gCurrentFolder.URI| correctly points to
"extra-folder", but |window.argumetns[0].folder.URI| still points
to "Inbox". So the proposed fix resets
|searchFolderURIs| to |gCurrentFolder.URI|. 
Now the resulting |searchFolderURIs| is correct!

I am going to request feedback ([e-mail], who touched the code
in 2009, but not sure if the e-mail address suggested by bugzilla correct or not.) and if the fix is deemed in the correct direction, I will remove the dump statements to produce the final patch proposal.

TIA

Comment 34

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8407413 [details] [diff] [review]
Proposal : Possible modification (debug dump) and a fix. Feedback solicited.

Very thorough investigation! Kudos!

Please note that I'm not a reviewer for this code anymore (I think :mkmelin might be your best option there), but I think you have the right idea.  It's not clear to me if you're already suggesting doing this and the debug code is just for debugging, but either way, I'd suggest changing saveAsVirtualFolder to just always use gCurrentFolder instead of checking window.arguments[0].folder.URI.

I think your logic is already equivalent to doing that if we assume gCurrentFolder is non-null/truthy.  Since that is done by updateSearchFolderPicker and searchOnLoad calls that, I think we can assume that.

So, to re-state, I think you want to distill the patch to just being
-  var searchFolderURIs = window.arguments[0].folder.URI;
+  var searchFolderURIs = gCurrentFolder.URI;

Comment 35

[WADA:World Anti-bad-Duping Agency]

> 601 var dialog = window.openDialog("chrome://messenger/content/virtualFolderProperties.xul", "",
> 602              "chrome,titlebar,modal,centerscreen",
> 603   { folder:           window.arguments[0].folder,  <= parent folder of this new VF
>                                                           parent part of "uri" in virtualFolders.dat
> 604     searchTerms:      getSearchTerms(),            <= for "terms" in virtualFolders.dat
> 605     searchFolderURIs: searchFolderURIs,            <= Search Target of this new VF == "scope"
> 606     searchOnline:     doOnlineSearch});            <= Online Search for IMAP

serchFoldersURI object at this call is equivallent to :
  [ window.arguments[0].folder, subfolders of gCurrentFolder.URI ].join("|"} ;
serchFoldersURI object at this call should be equivallent to :
  [ gCurrentFolder.URI,         subfolders of gCurrentFolder.URI ].join("|"} ;
This can be achieved by;
> -  var searchFolderURIs = window.arguments[0].folder.URI;
> +  var searchFolderURIs = gCurrentFolder.URI;

Virtual Folder is finally created via VirtualFolderHelper.createNewVirtualFolder().
> File/New/Saved Search menu definition.
>    <menuitem id="menu_newVirtualFolder" label="Saved Search…"
>              oncommand="gFolderTreeController.newVirtualFolder();" accesskey="S"/>
> http://mxr.mozilla.org/comm-central/source/mailnews/base/src/virtualFolderWrapper.js#19
> 43   createNewVirtualFolder: function (aFolderName, aParentFolder, aSearchFolders,
> 44                                     aSearchTerms, aOnlineSearch) {
(1) Paramteer order of SearchFolders and SearchTerms is different between UI and Back End :-)
(2) If JavaScript code, any parameter can be an Object object, or an JavaScipt Array object.
    Why string of "mbox://,,,|imap:...|,,," is used at any place/any code,
    even though such format is merely a local rule in virtualFolders.dat or msgFilterRules.dat?
(3) "Subfolders support" in menu_newVirtualFolder is pretty simple.
    (i)  Add check box of "include subfolders" to a panel
    (ii) Append URI of subfolders to aSearchFolders value upon "VF creation" request,
         as done by saveAsVirtualFolder().

Comment 36

[ISHIKAWA, Chiaki]

(In reply to Andrew Sutherland (:asuth) from comment #34)
> Comment on attachment 8407413 [details] [diff] [review]
> Proposal : Possible modification (debug dump) and a fix. Feedback solicited.
> 
> Very thorough investigation! Kudos!
> 
> Please note that I'm not a reviewer for this code anymore (I think :mkmelin
> might be your best option there), but I think you have the right idea.  It's
> not clear to me if you're already suggesting doing this and the debug code
> is just for debugging, but either way, I'd suggest changing
> saveAsVirtualFolder to just always use gCurrentFolder instead of checking
> window.arguments[0].folder.URI.
> 
> I think your logic is already equivalent to doing that if we assume
> gCurrentFolder is non-null/truthy.  Since that is done by
> updateSearchFolderPicker and searchOnLoad calls that, I think we can assume
> that.
> 
> So, to re-state, I think you want to distill the patch to just being
> -  var searchFolderURIs = window.arguments[0].folder.URI;
> +  var searchFolderURIs = gCurrentFolder.URI;

Thank you for your comment and analysis.

I will prepare the patch as you suggest.

TIA

Comment 37

[ISHIKAWA, Chiaki]

(In reply to WADA from comment #35)
> > 601 var dialog = window.openDialog("chrome://messenger/content/virtualFolderProperties.xul", "",
> > 602              "chrome,titlebar,modal,centerscreen",
> > 603   { folder:           window.arguments[0].folder,  <= parent folder of this new VF
> >                                                           parent part of "uri" in virtualFolders.dat
> > 604     searchTerms:      getSearchTerms(),            <= for "terms" in virtualFolders.dat
> > 605     searchFolderURIs: searchFolderURIs,            <= Search Target of this new VF == "scope"
> > 606     searchOnline:     doOnlineSearch});            <= Online Search for IMAP
> 
> serchFoldersURI object at this call is equivallent to :
>   [ window.arguments[0].folder, subfolders of gCurrentFolder.URI ].join("|"}
> ;
> serchFoldersURI object at this call should be equivallent to :
>   [ gCurrentFolder.URI,         subfolders of gCurrentFolder.URI ].join("|"}
> ;
> This can be achieved by;
> > -  var searchFolderURIs = window.arguments[0].folder.URI;
> > +  var searchFolderURIs = gCurrentFolder.URI;
> 
> Virtual Folder is finally created via
> VirtualFolderHelper.createNewVirtualFolder().
> > File/New/Saved Search menu definition.
> >    <menuitem id="menu_newVirtualFolder" label="Saved Search…"
> >              oncommand="gFolderTreeController.newVirtualFolder();" accesskey="S"/>
> > http://mxr.mozilla.org/comm-central/source/mailnews/base/src/virtualFolderWrapper.js#19
> > 43   createNewVirtualFolder: function (aFolderName, aParentFolder, aSearchFolders,
> > 44                                     aSearchTerms, aOnlineSearch) {
> (1) Paramteer order of SearchFolders and SearchTerms is different between UI
> and Back End :-)

Ouch. But I am not repsponsible for it :-)

> (2) If JavaScript code, any parameter can be an Object object, or an
> JavaScipt Array object.
>     Why string of "mbox://,,,|imap:...|,,," is used at any place/any code,
>     even though such format is merely a local rule in virtualFolders.dat or
> msgFilterRules.dat?

I don't know the history of the usage of such format. But I didn't realize
that msgFilterRules.dat uses such format. Maybe we should document these formats somewhere.

> (3) "Subfolders support" in menu_newVirtualFolder is pretty simple.
>     (i)  Add check box of "include subfolders" to a panel
>     (ii) Append URI of subfolders to aSearchFolders value upon "VF creation"
> request,
>          as done by saveAsVirtualFolder().

This no 3 sounds something that I can do.
So after fixing this bug, I will try to address the issue above.

TIA

Comment 38

[WADA:World Anti-bad-Duping Agency]

FYI.
> http://mxr.mozilla.org/comm-central/source/mailnews/base/src/virtualFolderWrapper.js#113
As seen in VirtualFolderWrapper.prototype, searchFolders is defined using getter/setter, and "local rule of '| separated URI string 'in virtualFolders.dat" is carefully wrapped.
Off-Topic.
Why UI side always uses local rule of "| separated URI string" in virtualFolders.dat file?
"Search Target Folders" is logically an "array of Mboxes(folderURL)" in UI always. As for VF, "logically an array of Mboxes(folderURL)" is achieved by wrapper.
History is perhaps. "Search subfolders" support => multiple search target folders => "| separated FolderURL" format in Search => "| separated FolderURL" format is still used even after VirtualFolder Wrapper is implemented.

Comment 39

[ISHIKAWA, Chiaki]

Attachment: Final patch: one line patch.

This is the cleaned up one-line patch.

TIA

Comment 40

[ISHIKAWA, Chiaki]

(In reply to ISHIKAWA, Chiaki from comment #39)
> Created attachment 8407975 [details] [diff] [review]
> Final patch: one line patch.
> 
> This is the cleaned up one-line patch.
> 
> TIA

See comment 33, comment 34 for the crux of the issue that led to the patch.

There are other virtual folder issues discussed in this bugzilla, which I hope I can
address once this bug is fixed.

TIA

Comment 41

[WADA:World Anti-bad-Duping Agency]

(In reply to ISHIKAWA, Chiaki from comment #39)
> Final patch: one line patch.
I believe it's perfect, but I prefer code like next.
  let Targets=new Array(); 
  Targets[Targets.length]=gCurrentFolder.URI; // Bug 670445
  let SubTargets = gCurrentFolder.getAllSubFolders();
  for(let nn=0;nn<SubTargets.length;nn++){Targets[Targets.length]=SubTargets[nn].URI;}
  Upon call, pass Targets.join("|") as searchFolderURIs.
"search target folders is array in this function" is explicitly represented.
"| separated FolderURL is requested by calling module" is explicitly represented.

Comment 42

[ISHIKAWA, Chiaki]

(In reply to WADA from comment #41)
> (In reply to ISHIKAWA, Chiaki from comment #39)
> > Final patch: one line patch.
> I believe it's perfect, but I prefer code like next.
>   let Targets=new Array(); 
>   Targets[Targets.length]=gCurrentFolder.URI; // Bug 670445
could this be
    Targets[0] = gCurrentFolder.URI ?

>   let SubTargets = gCurrentFolder.getAllSubFolders();
>   for(let
> nn=0;nn<SubTargets.length;nn++){Targets[Targets.length]=SubTargets[nn].URI;}
>   Upon call, pass Targets.join("|") as searchFolderURIs.
> "search target folders is array in this function" is explicitly represented.
> "| separated FolderURL is requested by calling module" is explicitly
> represented.

WADA san,

Why don't we file a new bugzilla to change the string manipulation (folder URIs separated by "|") within
this file to something like above (using Array explicitly) and wrap when it become necessary (join("|")).

I would prefer to have the current patch land first (to fix the issue, very serious to those who got bitten by this bug), and then take care of this rewrite.
What do you think?

TIA

Comment 43

[WADA:World Anti-bad-Duping Agency]

(In reply to ISHIKAWA, Chiaki from comment #42)
> I would prefer to have the current patch land first (to fix the issue, (snip)
I absolutely agree with you.

> could this be Targets[0] = gCurrentFolder.URI ?
Yes of course. [0] may be better in this case, because "first element" is explicitly represented.

> Why don't we file a new bugzilla to change the string manipulation (folder URIs separated by "|")
> within this file to something like above (using Array explicitly) and wrap when it become necessary (join("|")).
I think it's good idea. Because VirtualFolder Wrapper is well desinged(far new feature than BerkleyStore or MorkDB), I think we are better to fully utilize it and benefit of JavaScript.

Comment 44

[ISHIKAWA, Chiaki]

(In reply to WADA from comment #43)
> (In reply to ISHIKAWA, Chiaki from comment #42)
> > I would prefer to have the current patch land first (to fix the issue, (snip)
> I absolutely agree with you.

Thank you for your agreement :-
I think I can handle the rewrite mentioned in comment 41 (in a different bugzilla), and also
the subfolder extension in comment 35 may be within my reach (in a different bugzilla). I have not hacked
UI very much before.
> (3) "Subfolders support" in menu_newVirtualFolder is pretty simple.
>    (i)  Add check box of "include subfolders" to a panel
>    (ii) Append URI of subfolders to aSearchFolders value upon "VF creation" request,
>         as done by saveAsVirtualFolder().

Once I finish with this bug, I will these bugs separately (or would you do it first?).

TIA

Comment 45

[Magnus Melin [:mkmelin]]

Comment on attachment 8407975 [details] [diff] [review]
Final patch: one line patch.

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

::: mail/base/content/SearchDialog.js
@@ +584,5 @@
>  }
>  
>  function saveAsVirtualFolder()
>  {
> +  var searchFolderURIs = gCurrentFolder.URI; // Bug 670445

we don't typically include bug numbers for things like this

Comment 46

[ISHIKAWA, Chiaki]

Attachment: Really final patch (take 2) carrying over mkmelin+mozilla: review+

(In reply to Magnus Melin from comment #45)
> Comment on attachment 8407975 [details] [diff] [review]
> Final patch: one line patch.
> 
> Review of attachment 8407975 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mail/base/content/SearchDialog.js
> @@ +584,5 @@
> >  }
> >  
> >  function saveAsVirtualFolder()
> >  {
> > +  var searchFolderURIs = gCurrentFolder.URI; // Bug 670445
> 
> we don't typically include bug numbers for things like this

I took out the bug number comment.
Carrying over the review+.
If there is no objection, I will put checkin-needed keyword maybe on Saturday.

TIA

Comment 47

[Magnus Melin [:mkmelin]]

For future reference, please put an entire descriptive comment on the first line on the patch comment (that you edit with hg qrefresh -e). hg will only show the first non-broken line in the short logs. It's also nice for people checking your patches in if you add the r=mkmelin to the patch comment (or whoever reviewed it) once you have the review

Comment 48

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/2b84947b0586

Comment 49

[ISHIKAWA, Chiaki]

(In reply to Magnus Melin from comment #47)
> For future reference, please put an entire descriptive comment on the first
> line on the patch comment (that you edit with hg qrefresh -e). hg will only
> show the first non-broken line in the short logs. It's also nice for people
> checking your patches in if you add the r=mkmelin to the patch comment (or
> whoever reviewed it) once you have the review

Will do!

TIA