Last Comment Bug 742238 - Unified view loses some folders and does not allow reset
: Unified view loses some folders and does not allow reset
Status: RESOLVED FIXED
regression?
:
Product: Thunderbird
Classification: Client Software
Component: Folder and Message Lists (show other bugs)
: 14 Branch
: x86_64 Windows 7
: P2 major (vote)
: Thunderbird 14.0
Assigned To: Florian Quèze [:florian] [:flo]
:
:
Mentors:
Depends on:
Blocks: 749200
  Show dependency treegraph
 
Reported: 2012-04-04 02:10 PDT by Jb Piacentino
Modified: 2012-04-26 07:55 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed


Attachments
Patch (1.45 KB, patch)
2012-04-19 03:44 PDT, Florian Quèze [:florian] [:flo]
mozilla: review-
Details | Diff | Splinter Review
Patch v2 (10.63 KB, patch)
2012-04-20 06:30 PDT, Florian Quèze [:florian] [:flo]
mozilla: review+
mozilla: approval‑comm‑aurora+
Details | Diff | Splinter Review

Description Jb Piacentino 2012-04-04 02:10:11 PDT
I updated this morning to Daily. In the unified view, some folders in the Inbox virtual folder disappear (Inbox & Sent IMAP folders). 
They are indeed disabled from the list of selected folders (rclick on Inbox/properties/choose). When I try to re-enable them (check & click OK), the following error appears in the console:
Error: 'JavaScript component does not have a method named: "setInVFEditSearchScope"' when calling method: [nsIMsgFolder::setInVFEditSearchScope] = NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED
Source file: chrome://messenger/content/virtualFolderListDialog.js
Line: 90
Comment 1 :aceman 2012-04-05 00:17:56 PDT
Daily should be version 14. So what is your exact version? And which one did you use before the update and it worked fine?

Did you intentionally assign this to David Bienvenu? Is he content with it?
Comment 2 Jb Piacentino 2012-04-05 00:47:09 PDT
Changed to Daily. 
Assigned to bienvenu on purpose.
But not response... yet :-)
Comment 3 David :Bienvenu 2012-04-05 07:00:45 PDT
this is all front end code, so I'm not very familiar with it. That error is very strange, since the folder is not a js object. Some sort of js weirdness, perhaps with the jit?
Comment 4 David :Bienvenu 2012-04-05 07:04:51 PDT
yeah, I can't reproduce this here. Perhaps you could send me your virtualfolders.dat and I can see if anything is weird there. Did you delete any accounts recently?
Comment 5 David :Bienvenu 2012-04-18 19:10:09 PDT
I now believe this is caused by having chat accounts. The js folder w/o the attribute is probably a chat account folder. I'll try to figure out why the saved search ui is getting chat accounts. I suspect that AllFolders needs to filter out chat folders/accounts.
Comment 6 Florian Quèze [:florian] [:flo] 2012-04-19 02:51:03 PDT
I quickly looked at the code in the virtualFolderListDialog.js file and I don't see any obvious cause for the error.
The two places in the code that use accountManager.allServers to then access all folders do so by calling ListDescendents on the rootFolder of each incoming server.
ListDescendants is implemented as a no-op function at http://mxr.mozilla.org/comm-central/source/mail/components/im/imIncomingServer.js#233 so IM accounts should automatically be ignored without us adding any special case.

I'll try to reproduce to see if I can understand what's going on.
Comment 7 Florian Quèze [:florian] [:flo] 2012-04-19 02:54:07 PDT
Wait, ignore comment 6, there's actually a call to processSearchSettingForFolder before the rootFolder.ListDescendants call.
Comment 8 Florian Quèze [:florian] [:flo] 2012-04-19 03:44:01 PDT
Created attachment 616511 [details] [diff] [review]
Patch

Here's a simple patch to add another IM-specific check.
I tried to look at the other consumers of accountManager.allServers with MXR and haven't found any other problematic one, but given how many there are, it's possible I missed one.
Comment 9 David :Bienvenu 2012-04-19 06:57:07 PDT
(In reply to Florian Quèze from comment #8)
> Created attachment 616511 [details] [diff] [review]
> Patch
> 
> Here's a simple patch to add another IM-specific check.

Do you need accountManager.allServers to return im servers? If not, you could probably avoid a lot of trouble by not returning im servers here. I thought the primary reason for making im servers implement nsIMsgIncomingServer was so that you could be in the account settings dialog, not that you wanted to play fully in the rest of the mailnews handling of incoming servers...
Comment 10 Florian Quèze [:florian] [:flo] 2012-04-19 07:21:42 PDT
(In reply to David :Bienvenu from comment #9)

> Do you need accountManager.allServers to return im servers?

No. For some reasons I thought the account settings dialog used it; it doesn't, the "accounts" getter is used instead. It seems I was confused between accountManager.allServers and accountManager.accounts, thinking they were the same.

Now I just wish we found this earlier, before adding IM checks in lots of places.
At least these 2 can be reverted after doing what you are suggesting:
http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.js#2304
http://mxr.mozilla.org/comm-central/source/mailnews/base/prefs/content/amUtils.js#77

And possibly more if some .accounts calls that actually only use the .incomingServer property of each account can be switched to .allServers.
Comment 11 David :Bienvenu 2012-04-19 07:25:39 PDT
(In reply to Florian Quèze from comment #10)

> Now I just wish we found this earlier, before adding IM checks in lots of
> places.
> At least these 2 can be reverted after doing what you are suggesting:
> http://mxr.mozilla.org/comm-central/source/mail/base/content/
> mailWindowOverlay.js#2304
> http://mxr.mozilla.org/comm-central/source/mailnews/base/prefs/content/
> amUtils.js#77

That's good news. Probably want to annotate the .idl to say that allServers doesn't include im servers.
> 
> And possibly more if some .accounts calls that actually only use the
> .incomingServer property of each account can be switched to .allServers.
That would be good too. If you link me to them, I can try to help figuure out if that switch can be made.
Comment 12 David :Bienvenu 2012-04-19 08:47:13 PDT
Comment on attachment 616511 [details] [diff] [review]
Patch

minusing based on the idea of doing this at a lower level.
Comment 13 Florian Quèze [:florian] [:flo] 2012-04-20 06:30:51 PDT
Created attachment 616957 [details] [diff] [review]
Patch v2

Another attempt, taking into account comments 9-11.
5 tests for type == "im" removed :-).
I'm not sure I've been able to fully test the code that this patch touches, so you may want to be careful while reviewing the .accounts -> .allServers changes.
Comment 14 David :Bienvenu 2012-04-20 12:54:40 PDT
Comment on attachment 616957 [details] [diff] [review]
Patch v2

this looks good to me. I'm running the mozmill tests, but assuming they're ok, r=me.
Comment 15 Florian Quèze [:florian] [:flo] 2012-04-23 07:38:13 PDT
http://hg.mozilla.org/comm-central/rev/594481ecb285
Comment 16 Florian Quèze [:florian] [:flo] 2012-04-23 07:40:33 PDT
Comment on attachment 616957 [details] [diff] [review]
Patch v2

[Approval Request Comment]
Even though we have decided to pref off IM for Tb 13, we may still want to fix this regressions on 13 for users who would flip the pref.
Comment 17 David :Bienvenu 2012-04-23 07:51:06 PDT
Comment on attachment 616957 [details] [diff] [review]
Patch v2

I'd like to take this for our existing alpha and nightly users.
Comment 18 Mark Banner (:standard8) 2012-04-23 13:29:39 PDT
Looks like this was pushed to aurora earlier today:

http://hg.mozilla.org/releases/comm-aurora/rev/ef0b70ed8c0e

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