Closed Bug 1124948 Opened 5 years ago Closed 5 years ago

Search folders dont work on maildir (SearchFolder is normally created under Maildir and is usable, but it's deleted by restart and garbled SearchFolder.msf is kept)

Categories

(MailNews Core :: Backend, defect)

x86_64
Windows 8.1
defect
Not set

Tracking

(thunderbird38+ fixed)

RESOLVED FIXED
Thunderbird 39.0
Tracking Status
thunderbird38 + fixed

People

(Reporter: rkent, Assigned: rkent)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

When I try to create a search folder using maildir, it creates the .msf file. If I restart, that search folder is not found.

mbox seems to create an empty directory entry with the name of the search folder. If I manually create this for a maildir search folder, then the folder is now recognized, but it is not recognized as a search folder, so it appears empty.
As a workaround, I can create a dummy POP3 mbox local store account and store saved searches there. The saved searches can then search and display maildir folders from other accounts just fine.
So this turns out to be a little different than I expected. The problem is not creating virtual folders, it is deleting them. When you try to delete a virtual folder under maildir, the info is deleted from the virtualFolders.dat file, but the .msf file does not get deleted, and the folder continues to show in the folder tree. Upon restart, the folder no longer shows - but you cannot create a new one with the same name because the existing .msf file opens as invalid.
Attached patch fix and test (obsolete) — Splinter Review
Fix was simple, I added also virtual folder deletes to a test so that I get a test that fails without the fix.
Attachment #8553917 - Flags: review?(Pidgeot18)
Bug 1124105 is probably not needed for this fix, but these patches were done on top of that bug. You'll need it if you want to apply this patch.
Depends on: 1124105
Comment on attachment 8553917 [details] [diff] [review]
fix and test

I'm going to remove the review request for now. Although this fix works fine in my test profile, I am still having problems creating virtual folders in maildir in my main profile.
Attachment #8553917 - Flags: review?(Pidgeot18)
Attached patch with create issue solved as well (obsolete) — Splinter Review
Turns out there was also a problem creating folders as well as deleting them. I'd fixed that in the dist folder of the build but not the source, then forgot about it when I started to focus on the delete problem. Simple change though.
Attachment #8553917 - Attachment is obsolete: true
Attachment #8554031 - Flags: review?(Pidgeot18)
Comment on attachment 8554031 [details] [diff] [review]
with create issue solved as well

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

::: mail/base/test/unit/xpcshell_maildir.ini
@@ +15,5 @@
> +#[test_viewWrapper_virtualFolderCustomTerm.js]
> +#run-sequentially = Avoid bustage.
> +#[test_windows_font_migration.js]
> +#skip-if = os != "win"
> +#[test_mailGlue_distribution.js]

What's with all the commented-out lines?

::: mailnews/local/src/nsLocalMailFolder.cpp
@@ +778,5 @@
> +  NS_WARN_IF_FALSE(NS_SUCCEEDED(rv), "Could not delete msg summary file");
> +
> +  rv = msgStore->DeleteFolder(this);
> +  if (rv == NS_ERROR_FILE_NOT_FOUND)
> +    rv = NS_OK; // virtual folders do not have a msgStore file

When I see stuff like this, I'm reminded that we really ought to have a separate implementation of nsIMsgFolder for virtual folders.
(In reply to Joshua Cranmer [:jcranmer] from comment #7)
> Comment on attachment 8554031 [details] [diff] [review]
> with create issue solved as well
> 
> Review of attachment 8554031 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mail/base/test/unit/xpcshell_maildir.ini
> @@ +15,5 @@
> > +#[test_viewWrapper_virtualFolderCustomTerm.js]
> > +#run-sequentially = Avoid bustage.
> > +#[test_windows_font_migration.js]
> > +#skip-if = os != "win"
> > +#[test_mailGlue_distribution.js]
> 
> What's with all the commented-out lines?
>

They are there as a reminder that these tests are specifically not being run for maildir. But I see I did not do the same thing when I added a maildir manifest in imap, so I should remove them.
> ::: mailnews/local/src/nsLocalMailFolder.cpp
> @@ +778,5 @@
> > +  NS_WARN_IF_FALSE(NS_SUCCEEDED(rv), "Could not delete msg summary file");
> > +
> > +  rv = msgStore->DeleteFolder(this);
> > +  if (rv == NS_ERROR_FILE_NOT_FOUND)
> > +    rv = NS_OK; // virtual folders do not have a msgStore file
> 
> When I see stuff like this, I'm reminded that we really ought to have a
> separate implementation of nsIMsgFolder for virtual folders.
Summary: Search folders dont work on maildir → Search folders dont work on maildir (SearchFolder is normally created under Maildir and is usable, but it's deleted by restart and garbled SearchFolder.msf is kept)
No longer blocks: 1135309
Comment on attachment 8554031 [details] [diff] [review]
with create issue solved as well

I've got another patch coming that incorporates comments as well as fixes on small issue in the tests.
Attachment #8554031 - Flags: review?(Pidgeot18)
See try run https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=da34039a1436  Those errors are also on trunk.
Attachment #8554031 - Attachment is obsolete: true
Attachment #8577454 - Flags: review?(Pidgeot18)
(In reply to Kent James (:rkent) from comment #2)
> So this turns out to be a little different than I expected. 
> The problem is not creating virtual folders, it is deleting them.
> When you try to delete a virtual folder under maildir, the info is deleted from the virtualFolders.dat file,
> but the .msf file does not get deleted, and the folder continues to show in the folder tree.
> Upon restart, the folder no longer shows - but you cannot create a new one with the same name
> because the existing .msf file opens as invalid.

Delete virtual folder by whom? Manual delete of Virtual folder? Or delete by Virtual Folder himself?

Virtual Folder's design/implementation.
   If search target folder is not found, remove it from definitin of a Virtual Folder in virtualFolders.dat. 
   If no search target folder, delete the Virtual Folder.
This occurs upon rename/move/delete(both move to trash and immediate remove) of Search Target dolder. move/rename is not tracked.
If Virtuaal Folder under MaildirStore fails to find Search Target Folder upon restart, above occurs.
Comment on attachment 8577454 [details] [diff] [review]
fix tests on Linux

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

::: mail/base/test/unit/test_viewWrapper_virtualFolder.js
@@ +333,5 @@
>  
>    // now the view wrapper should have closed itself.
>    do_check_eq(null, viewWrapper.displayedFolder);
> +  // This fails because virtFolder.parent is null, not sure why
> +  //virtFolder.parent.propagateDelete(virtFolder, true, null);

This test seems to pass if you uncomment this line.

::: mailnews/db/msgdb/src/nsMailDatabase.cpp
@@ +116,5 @@
>  
>  NS_IMETHODIMP nsMailDatabase::SetSummaryValid(bool aValid)
>  {
>    nsMsgDatabase::SetSummaryValid(aValid);
> +  

EWHITESPACE
Attachment #8577454 - Flags: review?(Pidgeot18) → review+
Comment on attachment 8577454 [details] [diff] [review]
fix tests on Linux

We'll take on comm-central after a nightly cycle.

https://hg.mozilla.org/comm-central/rev/232a61c2aefa
Attachment #8577454 - Flags: approval-comm-aurora?
> > +  // This fails because virtFolder.parent is null, not sure why
> > +  //virtFolder.parent.propagateDelete(virtFolder, true, null);

>  This test seems to pass if you uncomment this line.

This still fails for me on windows, so I left the line commented out.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 39.0
Comment on attachment 8577454 [details] [diff] [review]
fix tests on Linux

http://hg.mozilla.org/releases/comm-aurora/rev/bc07ed1b0df8
Attachment #8577454 - Flags: approval-comm-aurora? → approval-comm-aurora+
You need to log in before you can comment on or make changes to this bug.