453908 - Convert rdf consumers to use the folder-lookup service

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Joey Minta]

Attachment: crashing patch

The attached patch converts the majority of rdf consumers to use the folder-lookup service instead. For some strange reason though, it reliably crashes my Thunderbird on startup, with the following (bizarre) stack. If anyone has any thoughts as to what might be going wrong, please chime in, because I'd really like to land this patch.

Thread 0 Crashed:
0   libmailnews.dylib             	0x1392eeb1 nsMsgAccountManager::addListenerToFolder(nsISupports*, void*) + 81 (nsMsgAccountManager.cpp:507)
1   libxpcom_core.dylib           	0x002e22f9 nsSupportsArray::EnumerateForwards(int (*)(nsISupports*, void*), void*) + 57 (nsSupportsArray.cpp:626)
2   libmailnews.dylib             	0x13938cde nsMsgAccountManager::createKeyedServer(nsACString_internal const&, nsACString_internal const&, nsACString_internal const&, nsACString_internal const&, nsIMsgIncomingServer**) + 958 (nsCOMPtr.h:752)
3   libmailnews.dylib             	0x1393935d nsMsgAccountManager::GetIncomingServer(nsACString_internal const&, nsIMsgIncomingServer**) + 765 (nsMsgAccountManager.cpp:456)
4   libmailnews.dylib             	0x139490d7 nsMsgAccount::createIncomingServer() + 455 (nsCOMPtr.h:792)
5   libmailnews.dylib             	0x139493e0 nsMsgAccount::GetIncomingServer(nsIMsgIncomingServer**) + 80 (nsMsgAccount.cpp:93)
6   libmailnews.dylib             	0x139354e1 nsMsgAccountManager::LoadAccounts() + 2529 (nsCOMPtr.h:792)
7   libmailnews.dylib             	0x139338cd nsMsgAccountManager::GetAllIdentities(nsISupportsArray**) + 29 (nsMsgAccountManager.cpp:1027)
8   libxpcom_core.dylib           	0x00353378 NS_InvokeByIndex_P + 88 (xptcinvoke_unixish_x86.cpp:179)
9   libxpconnect.dylib            	0x14bec050 XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) + 1600 (xpcprivate.h:3567)
10  libxpconnect.dylib            	0x14bf4df8 XPC_WN_GetterSetter(JSContext*, JSObject*, unsigned int, long*, long*) + 504 (xpcwrappednativejsops.cpp:1505)
11  libmozjs.dylib                	0x001998a1 js_Invoke + 2327 (jsinterp.cpp:1308)
12  libmozjs.dylib                	0x00199b39 js_InternalInvoke + 119 (jsinterp.cpp:1384)
13  libmozjs.dylib                	0x00199cdb js_InternalGetOrSet + 305 (jsinterp.cpp:1441)
14  libmozjs.dylib                	0x001a419d js_NativeGet + 511 (jsobj.cpp:3622)
15  libmozjs.dylib                	0x001ab266 js_GetPropertyHelper + 866 (jsobj.cpp:3772)
16  libmozjs.dylib                	0x001704b7 js_Interpret + 5815 (jsinterp.cpp:4262)
17  libmozjs.dylib                	0x00199918 js_Invoke + 2446 (jsinterp.cpp:1326)
18  libmozjs.dylib                	0x00199b39 js_InternalInvoke + 119 (jsinterp.cpp:1384)
19  libmozjs.dylib                	0x00133db8 JS_CallFunctionValue + 134 (jsapi.cpp:5066)
20  libgklayout.dylib             	0x12727e79 nsJSContext::CallEventHandler(nsISupports*, void*, void*, nsIArray*, nsIVariant**) + 761 (nsJSEnvironment.cpp:1995)
21  libgklayout.dylib             	0x127a15e4 nsJSEventListener::HandleEvent(nsIDOMEvent*) + 1076 (nsCOMPtr.h:792)
22  libgklayout.dylib             	0x124f616b nsEventListenerManager::HandleEventSubType(nsListenerStruct*, nsIDOMEventListener*, nsIDOMEvent*, nsISupports*, unsigned int) + 107 (nsEventListenerManager.cpp:1079)
23  libgklayout.dylib             	0x124f6b9e nsEventListenerManager::HandleEvent(nsPresContext*, nsEvent*, nsIDOMEvent**, nsISupports*, unsigned int, nsEventStatus*) + 1598 (nsEventListenerManager.cpp:1184)
24  libgklayout.dylib             	0x1253391d nsEventTargetChainItem::HandleEvent(nsEventChainPostVisitor&, unsigned int) + 221 (nsCOMPtr.h:990)
25  libgklayout.dylib             	0x12533cf3 nsEventTargetChainItem::HandleEventTargetChain(nsEventChainPostVisitor&, unsigned int, nsDispatchingCallback*) + 531 (nsEventDispatcher.cpp:271)
26  libgklayout.dylib             	0x125345d8 nsEventDispatcher::Dispatch(nsISupports*, nsPresContext*, nsEvent*, nsIDOMEvent*, nsEventStatus*, nsDispatchingCallback*) + 1720 (nsEventDispatcher.cpp:479)
27  libgklayout.dylib             	0x1213ec1d DocumentViewerImpl::LoadComplete(unsigned int) + 1021 (nsDocumentViewer.cpp:985)
28  libdocshell.dylib             	0x11e3ff41 nsDocShell::EndPageLoad(nsIWebProgress*, nsIChannel*, unsigned int) + 273 (nsDocShell.cpp:5113)
29  libdocshell.dylib             	0x11e4c7c0 nsWebShell::EndPageLoad(nsIWebProgress*, nsIChannel*, unsigned int) + 560 (nsWebShell.cpp:1018)
30  libdocshell.dylib             	0x11e407a3 nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, unsigned int) + 1443 (nsCOMPtr.h:523)
31  libdocshell.dylib             	0x11e62f51 nsDocLoader::FireOnStateChange(nsIWebProgress*, nsIRequest*, int, unsigned int) + 817 (nsDocLoader.cpp:1235)
32  libdocshell.dylib             	0x11e63666 nsDocLoader::doStopDocumentLoad(nsIRequest*, unsigned int) + 134 (nsDocLoader.cpp:869)
33  libdocshell.dylib             	0x11e63996 nsDocLoader::DocLoaderIsEmpty() + 710 (nsDocLoader.cpp:765)
34  libdocshell.dylib             	0x11e639b4 nsDocLoader::DocLoaderIsEmpty() + 740 (nsDocLoader.h:205)
35  libdocshell.dylib             	0x11e64104 nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, unsigned int) + 212 (nsDocLoader.cpp:679)
36  libnecko.dylib                	0x13fc858f nsLoadGroup::RemoveRequest(nsIRequest*, nsISupports*, unsigned int) + 815 (nsLoadGroup.cpp:691)
37  libgklayout.dylib             	0x12428589 nsDocument::DoUnblockOnload() + 313 (nsDocument.cpp:6516)
38  libgklayout.dylib             	0x12430281 nsDocument::DispatchContentLoadedEvents() + 1809 (nsCOMPtr.h:523)
39  libgklayout.dylib             	0x12449aeb nsRunnableMethod<nsDocument>::Run() + 43 (nsThreadUtils.h:265)
40  libxpcom_core.dylib           	0x0033adc2 nsThread::ProcessNextEvent(int, int*) + 450 (nsThread.cpp:511)
41  libxpcom_core.dylib           	0x002c82ea NS_ProcessNextEvent_P(nsIThread*, int) + 42 (nsThreadUtils.cpp:227)
42  libxpcom_core.dylib           	0x0033badf nsThread::Shutdown() + 655 (nsThread.cpp:464)
43  libxpcom_core.dylib           	0x00353378 NS_InvokeByIndex_P + 88 (xptcinvoke_unixish_x86.cpp:179)
44  libxpcom_core.dylib           	0x00344c4c nsProxyObjectCallInfo::Run() + 92 (nsAutoPtr.h:1082)
45  libxpcom_core.dylib           	0x0033adc2 nsThread::ProcessNextEvent(int, int*) + 450 (nsThread.cpp:511)
46  libxpcom_core.dylib           	0x002c82ea NS_ProcessNextEvent_P(nsIThread*, int) + 42 (nsThreadUtils.cpp:227)
47  libxpcom_core.dylib           	0x0033badf nsThread::Shutdown() + 655 (nsThread.cpp:464)
48  libxpcom_core.dylib           	0x00353378 NS_InvokeByIndex_P + 88 (xptcinvoke_unixish_x86.cpp:179)
49  libxpcom_core.dylib           	0x00344c4c nsProxyObjectCallInfo::Run() + 92 (nsAutoPtr.h:1082)
50  libxpcom_core.dylib           	0x0033adc2 nsThread::ProcessNextEvent(int, int*) + 450 (nsThread.cpp:511)
51  libxpcom_core.dylib           	0x002c82ea NS_ProcessNextEvent_P(nsIThread*, int) + 42 (nsThreadUtils.cpp:227)
52  libxpcom_core.dylib           	0x0033badf nsThread::Shutdown() + 655 (nsThread.cpp:464)
53  libxpcom_core.dylib           	0x00353378 NS_InvokeByIndex_P + 88 (xptcinvoke_unixish_x86.cpp:179)
54  libxpcom_core.dylib           	0x00344c4c nsProxyObjectCallInfo::Run() + 92 (nsAutoPtr.h:1082)
55  libxpcom_core.dylib           	0x0033adc2 nsThread::ProcessNextEvent(int, int*) + 450 (nsThread.cpp:511)
56  libxpcom_core.dylib           	0x002c82ea NS_ProcessNextEvent_P(nsIThread*, int) + 42 (nsThreadUtils.cpp:227)
57  libxpcom_core.dylib           	0x0033badf nsThread::Shutdown() + 655 (nsThread.cpp:464)
58  libxpcom_core.dylib           	0x00353378 NS_InvokeByIndex_P + 88 (xptcinvoke_unixish_x86.cpp:179)
59  libxpcom_core.dylib           	0x00344c4c nsProxyObjectCallInfo::Run() + 92 (nsAutoPtr.h:1082)
60  libxpcom_core.dylib           	0x0033adc2 nsThread::ProcessNextEvent(int, int*) + 450 (nsThread.cpp:511)
61  libxpcom_core.dylib           	0x002c8427 NS_ProcessPendingEvents_P(nsIThread*, unsigned int) + 71 (nsThreadUtils.cpp:181)
62  libwidget_mac.dylib           	0x14b00412 nsBaseAppShell::NativeEventCallback() + 98 (nsBaseAppShell.cpp:122)
63  libwidget_mac.dylib           	0x14ac100a nsAppShell::ProcessGeckoEvents(void*) + 634 (nsAppShell.mm:303)
64  com.apple.CoreFoundation      	0x95a39615 CFRunLoopRunSpecific + 3141
65  com.apple.CoreFoundation      	0x95a39cf8 CFRunLoopRunInMode + 88
66  com.apple.HIToolbox           	0x93056da4 RunCurrentEventLoopInMode + 283
67  com.apple.HIToolbox           	0x93056bbd ReceiveNextEventCommon + 374
68  com.apple.HIToolbox           	0x93056a31 BlockUntilNextEventMatchingListInMode + 106
69  com.apple.AppKit              	0x91e6a505 _DPSNextEvent + 657
70  com.apple.AppKit              	0x91e69db8 -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] + 128
71  com.apple.AppKit              	0x91e62df3 -[NSApplication run] + 795
72  libwidget_mac.dylib           	0x14ac07ca nsAppShell::Run() + 282 (nsAppShell.mm:591)
73  libtoolkitcomps.dylib         	0x148448fe nsAppStartup::Run() + 158 (nsAppStartup.cpp:183)
74  XUL                           	0x000a5e94 XRE_main + 20644 (nsAppRunner.cpp:3222)
75  org.mozilla.thunderbird       	0x00002b6c main + 396 (nsMailApp.cpp:103)
76  org.mozilla.thunderbird       	0x0000234c _start + 210
77  org.mozilla.thunderbird       	0x00002279 start + 41

Comment 1

[Kent James (:rkent)]

The root cause of the crash is that nsIFolderlookupService is returning a null m_rootFolder at this point:

rv = lookupSvc->GetFolderById(serverUri, getter_AddRefs(m_rootFolder));

in nsMsgIncomingServer.cpp line 388.

serverURI is "mailbox://nobody@Local%20Folders"

Once you get into the js, it's harder to trace.

Comment 2

[Joey Minta]

I looked into the js further, and with this patch, our map-building seems to fail.
http://mxr.mozilla.org/comm-central/source/mailnews/base/src/folderLookupService.js#111
Here, we're actually passing null to addFolder(). Obviously, this is getting called pretty early in startup, but I've had other issues on mac where folders aren't ready when I ask for them, so I'm currently going on the assumption that this is a more systemic problem. If people have other ideas as to why we can get accounts/servers with null root-folders, let's hear them though.

Comment 3

[Joey Minta]

See bug 424024 comment #7 for a similar problem.

Comment 4

[Kent James (:rkent)]

(In reply to comment #2)
> but I've had other issues on mac where folders
> aren't ready when I ask for them.

The tests I did were on a Windows 2003 Server 64 bit, so the issues are not Mac-specific.

Comment 5

[Joey Minta]

Attachment: [needs update & review] Convert many consumers

This patch removes the nsIncomingServer changes, which I'll need to look at more closely. After that, everything works fine (albeit with the slight correction in folderLookupService.js). Note also that a lot of the imap changes are smaller than they look, since I had to do some whitespace shifting.

Comment 6

[Bob Clary [:bc] (inactive)]

I tried the patch in attachment 337285 [details] [diff] [review] out and during startup my favorite accounts failed to display and I got a 

###!!! ASSERTION: You can't dereference a NULL nsCOMPtr with operator->().: 'mRawPtr != 0', file ../../../mozilla/dist/include/xpcom/nsCOMPtr.h, line 811

before I died. :-(

Comment 7

[David :Bienvenu]

I believe this work has happened in the kill-rdf repo. Once the folder lookup service goes back in, I can start landing the parts of this that are in the repo...

Comment 8

[David :Bienvenu]

Attachment: make account settings use folder lookup service

this makes the account manager use the folder lookup service (this is really jminta's patch - I'm just trying to drive it in)

Comment 9

[David :Bienvenu]

Comment on attachment 345228 [details] [diff] [review]
make account settings use folder lookup service

the folder tree menu stuff isn't needed for this change, but it won't break anything either (I still have to fix the scrollbars on this menu before I can add code that uses this widget)

Comment 10

[Mark Banner (:standard8)]

Comment on attachment 345228 [details] [diff] [review]
make account settings use folder lookup service

This doesn't look like the right patch, or maybe not the right bits.

There is no folderUtils.jsm, and additionally its not actually using the folder lookup service from bug 441437, which the title implies it should be.

Cancelling review, please re-request if I've missed something.

Comment 11

[David :Bienvenu]

Comment on attachment 345228 [details] [diff] [review]
make account settings use folder lookup service

it's the patch I intended to attach, but the wrong description - it doesn't use the folder lookup service; it stops using rdf instead...I'll attach a patch w/o the folderWidgets.xml changes, since those aren't needed (which is where folderUtils.jsm was used)

Comment 12

[David :Bienvenu]

Attachment: make account settings stop using rdf

Comment 13

[Mark Banner (:standard8)]

Comment on attachment 345284 [details] [diff] [review]
make account settings stop using rdf

I've still got to test this, although in general it is looking reasonable. Here's the comments I've got from looking at the patch.

>  var accountArray;
> +var accountMap = {};

These two seem directly linked. From what I can tell if a server id is in one, it'll be in the other as well. I think they should be one object, not two, and we could do with some documentation as to what these contain.

>   var selectedServer;
>   var selectPage = null;
>   if ("arguments" in window && window.arguments[0]) {
>     selectedServer = window.arguments[0].server;
>     selectPage = window.arguments[0].selectPage;
>   }
...
>-  setTimeout(selectServer, 0, selectedServer ? selectedServer.serverURI : "", selectPage);
>+  setTimeout(selectServer, 0, selectedServer, selectPage);
> }
> 
...
>+function selectServer(account, selectPage)
...
>+  // Find the tree-node for the account we want to select
>+  if (!account) {
>+    // Just get the first account
>+    accountNode = document.getElementById("account-tree-children").firstChild;
>+  } else {
>+    var childrenNode = document.getElementById("account-tree-children");
>+    for (var i = 0; i < childrenNode.childNodes.length; i++) {
>+      if (account == childrenNode.childNodes[i]._account.incomingServer) {
>+        accountNode = childrenNode.childNodes[i];
>+        break;
>     }

This is a little confusing now. The window parameter is supposedly the selectedServer, however, you get into selectServer and it turns into an account, but then you compare it with incoming server.

I think we need some consistency in here and to add a comment as to what window.arguments we're expecting.

Also please fix the brackets for this function. Its hard to tell from the diff, but when you look in AccountManager.js the closing ones are indented wrong in several places.

> function replaceWithDefaultSmtpServer(deletedSmtpServerKey)
> {
>   //First we replace the smtpserverkey in every identity
...
>+  var Ci = Components.interfaces;

I think const would be better.

> // Check if the user and/or host names have been changed and
> // if so check if the new names already exists for an account.
> //
> function checkUserServerChanges(showAlert) {
>-
>+  var Cc = Components.classes;
>+  var Ci = Components.interfaces;

const again?

>   if (smtpService.defaultServer) {
>     try {
>       var smtpHostName = top.frames["contentFrame"].document.getElementById("smtp.hostname");
>-      if (hostnameIsIllegal(smtpHostName.value)) {
>-        var alertTitle = gBrandBundle.getString("brandShortName");
>-        var alertMsg = gPrefsBundle.getString("enterValidHostname");
>-        var promptService =
>-          Components.classes["@mozilla.org/embedcomp/prompt-service;1"]
>-                    .getService(Components.interfaces.nsIPromptService);
>-        promptService.alert(window, alertTitle, alertMsg);
>+      if (smtpHostName && hostnameIsIllegal(smtpHostName.value)) {
>+        var alertTitle = document.getElementById("bundle_brand")
>+                                 .getString("brandShortName");
>+        var alertMsg = document.getElementById("bundle_prefs")
>+                               .getString("enterValidHostname");
>+
>+        Cc["@mozilla.org/embedcomp/prompt-service;1"].
>+        getService(Ci.nsIPromptService).alert(window, alertTitle, alertMsg);

dot at the beginning of the line, and two-space indent please.

>   // If something is changed then check if the new user/host already exists.
>   if ( (oldUser != newUser) || (oldHost != newHost) ) {
>+    var accountManager = Cc["@mozilla.org/messenger/account-manager;1"].
>+                         getService(Ci.nsIMsgAccountManager);

ditto with the dot, but two-space indent from Cc please.

>     var newServer = accountManager.findRealServer(newUser, newHost, newType, 0);
>     if (newServer) {
>       if (showAlert) {
>-        var alertText = gPrefsBundle.getString("modifiedAccountExists");
>-        var promptService =
>-          Components.classes["@mozilla.org/embedcomp/prompt-service;1"]
>-                    .getService(Components.interfaces.nsIPromptService);
>-        promptService.alert(window, null, alertText);
>+        var alertText = document.getElementById("bundle_prefs")
>+                                .getString("modifiedAccountExists");
>+        window.alert(alertText);

window.alert is obsolete e.g. bug 146392, bug 432608. Prompt Service is the correct item to use.

>   }
>   catch (ex) {
>     dump("failure to remove account: " + ex + "\n");
>-    var alertText = gPrefsBundle.getString("failedRemoveAccount");
>-    var promptService =
>-      Components.classes["@mozilla.org/embedcomp/prompt-service;1"]
>-                .getService(Components.interfaces.nsIPromptService);
>-    promptService.alert(window, null, alertText);
>+    var alertText = bundle.getString("failedRemoveAccount");
>+    window.alert(alertText);

ditto we shouldn't be using window.alert.

>-function updateButtons(tree,serverId) {
>+function updateButtons(tree,account) {

nit: space after the comma please.

>   // check for disabled preferences on the account buttons.  
>   //  Not currently handled by WSM or the main loop yet since these buttons aren't
>   //  under the IFRAME
>-  if (nsPrefBranch.prefIsLocked(addAccountButton.getAttribute("prefstring")))
>+  var addAccountButton = document.getElementById("addAccountButton");
>+  var removeButton = document.getElementById("removeButton");
>+  var setDefaultButton = document.getElementById("setDefaultButton");
>+
>+  var prefService = Components.classes["@mozilla.org/preferences-service;1"]
>+                              .getService(Components.interfaces.nsIPrefService)
>+                              .getBranch(null);

If you using Components.interfaces.nsIPrefBranch, then you don't need to have the .getBranch on the end.

Also, I think this would be better as prefBranch rather than prefService.

>-  // Nothing is selected. This is the case when an account was removed.
>-  // A call of selectServer(null,null) after the removal ensures that
>-  // onAccountClick() is called again after that with a valid selection.
>-  if (!currentSelection)
>+  if (tree.view.selection.count < 1) return null;

The return should be on a new line with a blank line after it.


> //
> // get the value array for the given serverId
> //
>-function getValueArrayFor(serverId) {
>-  if (serverId == undefined) serverId="global";
>+function getValueArrayFor(account) {

nit: the comment needs to be changed: serverId -> account. Also, drop the // lines before and after please.

>+  _build: function at_build() {
>+    var Ci = Components.interfaces;

const again (please just search & replace throughout the patch, there's more after this I won't mention again).

>diff --git a/mailnews/base/prefs/resources/content/am-server.js b/mailnews/base/prefs/resources/content/am-server.js
>--- a/mailnews/base/prefs/resources/content/am-server.js
>+++ b/mailnews/base/prefs/resources/content/am-server.js
>@@ -48,16 +48,17 @@ function onInit(aPageId, aServerId)
> function onInit(aPageId, aServerId) 
> {
>     initServerType();
> 
>     onCheckItem("server.biffMinutes", "server.doBiff");
>     onCheckItem("nntp.maxArticles", "nntp.limitArticles");
>     setupMailOnServerUI();
>     setupFixedUI();
>+    if (document.getElementById("server.type").getAttribute("value") == "imap")
>     setupImapDeleteUI(aServerId);

nit: this second line needs indenting.

>diff --git a/mailnews/extensions/dsn/src/dsn-service.js b/mailnews/extensions/dsn/src/dsn-service.js
...
>   // don't show the panel for news accounts
>-  return (server.type != "nntp");
>+  return (server.type != "nntp" && server.type != "rss" && server.type != "none");

nit: need to update the comment.

Comment 14

[Joey Minta]

Attachment: [checked in] convert account settings v2

This is an updated version of the patch, with Standard8's comments addressed. Looking more closely at accountArray (and related objects) I realized that the old code was abusing arrays heavily. (Just because something uses [] notation doesn't mean it's an array!) I've fixed that abuse as well.

Comment 15

[Mark Banner (:standard8)]

Comment on attachment 345812 [details] [diff] [review]
[checked in] convert account settings v2

This is looking much better (and I especially like the array -> object fixes).

I can't test it though, because of the following:

JavaScript error: , line 0: uncaught exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIStringBundle.GetStringFromName]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: chrome://messenger/content/AccountManager.js :: at_build :: line 1135"  data: no]

It looks like am-smime.properties is missing some string additions, maybe some others as well? (though I'm sure I've seen a patch with some of these on).

Comment 16

[Joey Minta]

Comment on attachment 345812 [details] [diff] [review]
[checked in] convert account settings v2

Re-requesting review, since the bustage was caused by checkin, not this patch.

Comment 17

[Joey Minta]

Comment on attachment 345812 [details] [diff] [review]
[checked in] convert account settings v2

Landed as rev 814:8c591aa8bc9d

Comment 18

[[e-mail]]

Comment on attachment 345812 [details] [diff] [review]
[checked in] convert account settings v2

> function updateElementWithKeys(account, element, type) {
...

>+      var account = currentAccount
Nit: missing ;

>+    if (serverId in accountArray) {
>+      delete accountArray[serverId];
>+    }
[You don't need to protect delete like this]

>-        updateElementWithKeys(account,pageElements[i],type);
You inlined this function, but you didn't remove it...

>+            element["identitykey"] = account.defaultIdentity.key;
[You can write this as element.identitykey]

Comment 19

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to comment #5)
> Created an attachment (id=337285) [details]
> patch v1
> 
> This patch removes the nsIncomingServer changes, which I'll need to look at
> more closely. After that, everything works fine (albeit with the slight
> correction in folderLookupService.js). Note also that a lot of the imap changes
> are smaller than they look, since I had to do some whitespace shifting.

This patch still needed? (it has pending r / sr bienvenu request, and for quite a few months.) A later patch seems to have been checked in...

Comment 20

[David :Bienvenu]

we haven't landed the folder lookup service (or we backed it out), and this patch relies on that. We felt that the js-driven folder pane was really the important part, and that landed.

Comment 21

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to comment #5)
> Created an attachment (id=337285) [details]
> patch v1
> 
> This patch removes the nsIncomingServer changes, which I'll need to look at
> more closely. After that, everything works fine (albeit with the slight
> correction in folderLookupService.js). Note also that a lot of the imap changes
> are smaller than they look, since I had to do some whitespace shifting.


This patch has bitrotted:

$ patch -p1 --dry-run < ~/Desktop/lookup-folder_v2.diff 
patching file mail/components/migration/src/Makefile.in
patching file mail/components/migration/src/nsNetscapeProfileMigratorBase.cpp
patching file mailnews/base/search/src/Makefile.in
Hunk #1 succeeded at 53 (offset 2 lines).
patching file mailnews/base/search/src/nsMsgFilterService.cpp
Hunk #1 succeeded at 54 (offset -1 lines).
Hunk #2 succeeded at 517 (offset 3 lines).
patching file mailnews/base/search/src/nsMsgSearchTerm.cpp
Hunk #2 succeeded at 911 (offset 14 lines).
can't find file to patch at input line 164
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|diff -r bbfb7a098337 mailnews/base/src/folderLookupService.js
|--- a/mailnews/base/src/folderLookupService.js Fri Sep 05 16:02:37 2008 -0700
|+++ b/mailnews/base/src/folderLookupService.js Sun Sep 07 08:47:20 2008 -0400
--------------------------
File to patch:

Comment 22

[Mark Banner (:standard8)]

Comment on attachment 337285 [details] [diff] [review]
[needs update & review] Convert many consumers

Whilst this patch may not apply now, obsoleting it doesn't help with people realising that it is here and still a valid starter for finishing this bug.

Comment 23

[Mark Banner (:standard8)]

Pretty sure Joey isn't working on this any longer.

Comment 24

[:aceman]

I have refreshed the patch and now running try builds.

Comment 25

[:aceman]

Attachment: C++ patch

This converts all the C++ callers of RDFService way of getting a folder via URI to the folderLookupeService.js::getFolderForURL(). The JS service still uses RDF internally and some of the callers still rely on the RDF behaviour of returning also non-existent folders (I think e.g. the basic set of default folders like Drafts, Outbox). But at least those callers are now identified via the mustExist = true argument.

Try run at https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=a7caf59492bd19181822241219cd5e9361869f10 
There is a single almost permanent failure on the try-server in mailnews/imap/test/unit/test_offlineStoreLocking.js.
This does not happen for me locally (thus the test passes). It may be timing related (and only happens on linux). The test starts an operation and then another one to see if the mailbox is still locked. IT may be the patch changes the timing a but and now the second operation does not start fast enough to still see the locked mailbox.
Can you see if this is true? Do we need to update the test to be more reliable or is there a problem in my patch?

Comment 26

[:aceman]

Attachment: JS patch

This changes some JS callers from RDF service to the folderLookupService.js (via MailUtils to save getting the service). It depends on the new mustExist argument added in the other patch.

Try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=a7caf59492bd19181822241219cd5e9361869f10

Comment 27

[:aceman]

(In reply to :aceman from comment #25)
> This converts all the C++ callers of RDFService way of getting a folder via
> URI to the folderLookupeService.js::getFolderForURL(). The JS service still
> uses RDF internally and some of the callers still rely on the RDF behaviour
> of returning also non-existent folders (I think e.g. the basic set of
> default folders like Drafts, Outbox). But at least those callers are now
> identified via the mustExist = true argument.

Sorry, it is when mustExist = false (so return also non-existent folders).

Comment 28

[Kent James (:rkent)]

Comment on attachment 8854310 [details] [diff] [review]
C++ patch

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

I put several hours into this, and ultimately could not get very far because there are some fundamental questions unanswered.

RDF has this issue that we ALWAYS return something from GetResource, so there is really no way to do a lookup without fallback create. nsIRDFService mentions "FindResource is used to find out whether there already exists a resource corresponding to that url" but that method does not actually exist.

We have come to rely in subtle unexpected ways on this RDF behavior, and I have fought this for years in account creation and initialization. Do we want to try to get beyond this and do a cleanup, or not? Is it even possible without adding the missing FindResource method?

I'm afraid that the aMustExist parameter does not really handle this completely. I would like to say that we only use aMustExist == false in cases where we are clearly expecting to create and initialize a folder, but there are existing cases (like in SetSpecialFolders) where I think we are expecting to create folders (but not completely).

It would be good if we could make sure that the design of folder lookup does not keep the flaws of the existing RDF service, but improves some of these ambiguities, at least eventually when we get rid of RDF.

I'm tempted to make folder lookup have two methods, a lookup and  create, with the expectation that we would not create folders in unexpected places.

Comments?

::: mailnews/base/src/folderLookupService.js
@@ +33,1 @@
>  

Prior to your change in the original code we have:

return folder.parent != null || folder.rootFolder == folder;

That does not look right to me, should be

return folder.parent !== null || folder.rootFolder === folder;

(You cannot compare two objects with ==)

BUT direct object comparison can create issues with JsAccount, as any of the mailnews objects can contain multiple representations as objects. It's complicated. Since we are going to be using this more widely, can we try to improve this? Like, I would prefer:

return !!folder.parent || (folder.rootFolder.URI == folder.URI);

@@ +57,5 @@
> +   * @param aMustExist {boolean}  Only return real existing folders.
> +   * @return {nsIMsgFolder}  The folder found.
> +   */
> +  getFolderForURL: function (aUrl, aMustExist = true) {
> +dump("ACE:"+aUrl+":"+aMustExist+"\n");

You'll need to rid of the dumps.

@@ +64,5 @@
>      if (this._map.has(aUrl)) {
>        let valid = false;
>        try {
>          folder = this._map.get(aUrl).QueryReferent(Ci.nsIMsgFolder);
> +        valid = !aMustExist || isValidFolder(folder);

So for false aMustExit, this will return whatever the QueryReferent returns. That is, it assumes that the map in folderLookupService is now the only true source of knowledge about whether folder objects exist. If that is true, then why fallback to RDF ever (or use RDF to create)?

It is unclear to me if the intention here is to eliminate RDF fallback or not.

Let's assume not. Then, isn't the point of aMustExist that you do not want to return invalid folders? So I would keep the RDF fallback if the map returns nothing, but check for validity before returning. If aMustExist == true, not only return null, but also unregister any folder that the GetResource created.

I thought that we were not really ready to completely get rid of RDF (something about old data queries in certain XUL). So I don't understand why you are getting rid of RDF fallback when aMustExist is false.

::: mailnews/base/src/nsMsgAccountManager.cpp
@@ +1437,4 @@
>        nsCOMPtr<nsIMsgFolder> folder;
>        thisIdentity->GetFccFolder(folderUri);
> +      if (!folderUri.IsEmpty() &&
> +          NS_SUCCEEDED(lookupSvc->GetFolderForURL(folderUri, false, getter_AddRefs(folder))))

This method has been the source of create grief in account setup. Here you are creating folders, but not setting flags since they are invalid. Yes you are following existing behaviour, but this folder create is really a side effect (though one that we have come to rely on).

Could we at least add a "TODO" that these should really be aMustExist == true and separate the creation from the flag setting?

Comment 29

[Jorg K (CEST = GMT+2)]

Comment on attachment 8854311 [details] [diff] [review]
JS patch

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

This looks fine upon visual inspection. Since you have a try run, I didn't run this.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +3961,5 @@
>      {
>        var imapFolder = folder.QueryInterface(Components.interfaces.nsIMsgImapMailFolder);
> +      if (imapFolder) {
> +        keyArray[0] = msgKey;
> +        let keyArray = [];

This looks funny.

Comment 30

[Jorg K (CEST = GMT+2)]

Comment on attachment 8854310 [details] [diff] [review]
C++ patch

I don't think I can provide any educated feedback here. Kent has already given you lots of comments to consider and take on board. This isn't your original work, right?, which makes things even harder. Welcome to the club of having to deal with legacy stuff (me: cache->cache2, sync proxy->async proxy, etc.).

Comment 31

[:aceman]

Yes, I just unbitrotted this. I'll see if I can address Kent's comments.

Comment 32

[Kent James (:rkent)]

Comment on attachment 8854310 [details] [diff] [review]
C++ patch

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

Clearing the review request since I gave comments that will be looked at.

Comment 33

[alta88]

This is an important bug, thanks for rescuing it aceman.

It's unclear if the isValidFolder() check is enough; I worked around rdf returning non existent folders (usually deleted outside of Tb) by using folder.filePath.exists().  It would also be useful to send a folderDeleted and OnItemRemoved notification to nsIMsgFolderListener and nsIFolderListener consumers, respectively, if a folder is found to really be gone.

Comment 34

[Magnus Melin [:mkmelin]]

Comment on attachment 8854311 [details] [diff] [review]
JS patch

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

LGTM! r=mkmelin

Comment 35

[Wayne Mery (:wsmwk)]

So just comment 28 / comment 33 to go?

Comment 36

[:aceman]

Yes, but comment 28 is quite critical. We have an alternative implementation of this patch done by jcranmer so we will see if maybe those problems are fixed in that version.

Comment 37

[Magnus Melin [:mkmelin]]

aceman/jörg: is all wip work attached to this bug?

I think benc is going to look at it eventually

Comment 38

[Jorg K (CEST = GMT+2)]

(In reply to Magnus Melin from comment #37)
> aceman/jörg: is all wip work attached to this bug?
I don't think so, but Aceman can answer this better. I believe his plan was to use some of Joshua's work from:
https://hg.mozilla.org/users/Pidgeot18_gmail.com/patch-queues/file/tip/patches-derdf/

The stuff in subscribe-* has already been used in bug 1466705 (I'm not sure about the remaining subscribe*, without "-"). But folder-lookup-service is certainly something Aceman wanted to use.

Note that open RDF bugs are linked in bug 420506, and currently I only see bug 1467238 which is the continuation of bug 732106.

Comment 39

[:aceman]

While the patches here more or less fix the problem, there are open problems as commented on by rkent and his r-.

I looked over jcranmer's patches at the link Jorg pasted and I think those are a better solution.
So should use and refresh these patches from the link:
folder-lookup-service
use-fls-in-mailnews-base
use-getfolder-in-cpp

Comment 40

[Ben Campbell]

I've been going through and updating these three:
folder-lookup-service
use-fls-in-mailnews-base
use-getfolder-in-cpp
(the last two seem to overlap a lot)

I've got them compiling and (I think) basically working, although I think there are a bunch of places which still use the rdfservice to get at folders (either outside the scope of what kcranmer was looking at, postdating those patches - not sure which).

Happy to post a work-in-progress patch if anyone's interested. Hopefully I'll have something a bit cleaner next week.

My planned next steps are:
1) go through aceman's work and see what needs to be incorporated
2) start converting any other rdf folder-lookups I can find
3) pick through it all with a fine tooth comb!
4) try and round out the test coverage (the existing folderlookupservice tests pass OK, but there's some more that could be done)

Does that sound like a sane plan? Very much open to suggestions!

Comment 41

[:aceman]

(In reply to Ben Campbell from comment #40)
> Happy to post a work-in-progress patch if anyone's interested. Hopefully
> I'll have something a bit cleaner next week.

Thanks!
 
> My planned next steps are:
> 1) go through aceman's work and see what needs to be incorporated
> 2) start converting any other rdf folder-lookups I can find

Yes, please kill any new places that appeared after jcranmer's conversion. If using my patch to find spots, please use GetExistingFolder() where possible.

> 3) pick through it all with a fine tooth comb!
> 4) try and round out the test coverage (the existing folderlookupservice
> tests pass OK, but there's some more that could be done)

> Does that sound like a sane plan? Very much open to suggestions!

Yes, thank you.

Comment 42

[Magnus Melin [:mkmelin]]

Sure, go ahead an upload the wip patch. 
If you think they are basically working, might be worth sending to try also to see if it agrees ;)

Comment 43

[Jorg K (CEST = GMT+2)]

(In reply to Ben Campbell from comment #40)
> ... postdating those patches - not sure which.
Sure, landed a week ago :-(
https://hg.mozilla.org/comm-central/rev/97064b1acf5339bb5e78fd2f90bac5a4ea332d76#l1.21

Comment 44

[Ben Campbell]

Attachment: WIP-fls1.patch

Very rough work-in-progress based on jcranmers patches.

The main conceptual change is that this folderLookupService registers itself as a nsIFolderListener, so it tracks folders and doesn't fall back to the RDF service.
It's GetFolderByUrl() takes a mayCreate flag to allow it to create non-existent folders (default is to return null if a folder isn't found).

With this patch applied, a rough grep across c-c seems to indicate about 80 further places in C++ and 120 in JS where the GetResource() is still called. Not sure how much faith to put in that estimate (could be some unrelated GetResource() fn, or some lingering non-folder-related RDF use), but at least it's an upper-bound ballpark figure.

Comment 45

[Ben Campbell]

Oh, one quick question: this folderLookupService registers itself as a listener:

  MailServices.mailSession.AddFolderListener(this,
    Ci.nsIFolderListener.added | Ci.nsIFolderListener.removed);

This is done during the first call to GetFolderByUrl(), which just smells a little wrong to me. I think it'll be OK, as it does scan for folders at this time too, so hopefully it won't miss any.... but it seems like there should be a more elegant method to set it up.
Is there a comparable javascript-based-service-with-listener I should look at for inspiration?

Comment 46

[Ben Campbell]

Attachment: part1_remove_rdf_from_fls_1.patch

Comment 47

[Ben Campbell]

Attachment: part2_rename_getorcreatefolder_1.patch

Comment 48

[Ben Campbell]

Attachment: part3_use_fls_in_c++_1.patch

Comment 49

[Ben Campbell]

Attachment: part4_use_fls_in_js_1.patch

Comment 50

[Ben Campbell]

There's my patch set as it currently stands (spoiler: getting closer, but we're not quite there yet!):

part1: an update of jcranmers patches (which provides the core non-rdf-based folder lookup service).
part2: In msgUtils I rename GetOrCreateFolder() to GetOrCreateJunkFolder(), because a) it only creates the junk folder, and b) I want to a GetOrCreateFolder() as a counterpart to GetExistingFolder().
part3: an update of aceman's C++ patch. Switches almost all C++ folder lookups to use FLS.
part4: an update of aceman's JS patch. Switches most JS folder lookups to use FLS.


Disclaimer: it crashes horribly and fails huge numbers of unit tests :-)
I'll cover the issues in separate comments next.

Comment 51

[Ben Campbell]

Minor issue (just want to flag it up here so it's not lost):

Both aceman and jcranmer's patches add an extra flag to the FLS folder lookup function to govern whether or not it's allowed to create folders if they don't already exist.
aceman's patch used a "mustExist" flag, and jcranmer used a "mayCreate" flag.
I went with "mayCreate", as the default (false) value is the more common behaviour we want - usually we want to find an existing folder, not create a dangling non-parented one.

However, after I went through aceman's JS patch, I realised that MailUtils.js (which handles most of the FLS lookups in JS) uses the "mustExist" convention.
So the flag passed to MailUtils.js getFolderForURI() is the opposite to the flag passed into the FLS getFolderByUrl() fn. I currently sort this out in the MailUtils.js fn, but it _will_ bite someone if it's not sorted out.

Comment 52

[:aceman]

(In reply to Ben Campbell from comment #50)
> There's my patch set as it currently stands (spoiler: getting closer, but
> we're not quite there yet!):

Great, thanks :)

> Disclaimer: it crashes horribly and fails huge numbers of unit tests :-)
> I'll cover the issues in separate comments next.

That's kinda strange if it is based on my patches, because I took great case to make all tests pass. I had to find out which places need GetExistingFolder() and which need GetOrCreateFolder() (the old equivalent of those functions).

Comment 53

[Ben Campbell]

The Major, crashy issue:

I've not quite sorted out all the details of the folder creation in FLS yet. (At the moment it just dives into an infinite loop upon startup!)

Here's what I _think_ should happen:

Root folders are created by the nsIMsgIncomingServer::CreateRootFolder() - the particular server implementation knows what type of folder it needs to create.

Other folders are created by calling AddSubFolder() upon their prospective parent folder.

So the rough algorithm FLS should use to create non-existant folders is:

1) if it's a root URI, find the appropriate nsIMsgIncomingServer and call GetRootFolder() on it (which will create the folder if needed).
2) for non-root URIs, get the parent folder, and call addSubFolder() upon it.

step 2 is potentially recursive, calling back into the FLS until it finds an actual, existing folder, then working back downward, creating child folders as it goes.

Does that sound like the way it should work?

It just occurred to me that this operation of the FLS means we'll never have a dangling folder without a parent (except for the root, server folder, of course). What are the implications of this?
The old RDF code seemed pretty blasé about dangling folders with no parent. Are there any parts of the code that rely on having dangling folders?

Comment 54

[Ben Campbell]

(In reply to :aceman from comment #52)
> That's kinda strange if it is based on my patches, because I took great case
> to make all tests pass. I had to find out which places need
> GetExistingFolder() and which need GetOrCreateFolder() (the old equivalent
> of those functions).

It's crashing because the new (jcranmer) FLS ditches the folder-creation-via-RDF mechanism entirely, and I've not quite sorted out all the wrinkles.
All your stuff looks fine (barring the inevitable screw-ups I've made updating it ;-)

So still very much work-in-progress - I just wanted to get what I've got so far up here so I can step back and take stock.

Comment 55

[:aceman]

(In reply to Ben Campbell from comment #54)
> It's crashing because the new (jcranmer) FLS ditches the
> folder-creation-via-RDF mechanism entirely, and I've not quite sorted out
> all the wrinkles.

True, that's the final step I couldn't yet do, but jcranmer may have it solved.
It will be good if that gets finished.

(In reply to Ben Campbell from comment #53)
> 1) if it's a root URI, find the appropriate nsIMsgIncomingServer and call
> GetRootFolder() on it (which will create the folder if needed).
> 2) for non-root URIs, get the parent folder, and call addSubFolder() upon it.
> 
> step 2 is potentially recursive, calling back into the FLS until it finds an
> actual, existing folder, then working back downward, creating child folders
> as it goes.
> 
> Does that sound like the way it should work?

That looks reasonable.
 
> It just occurred to me that this operation of the FLS means we'll never have
> a dangling folder without a parent (except for the root, server folder, of
> course). What are the implications of this?
> The old RDF code seemed pretty blasé about dangling folders with no parent.
> Are there any parts of the code that rely on having dangling folders?

Possibly. This may need some investigation. When you create a new account (say POP3), does it immediatelly create all the special folders, like Drafts, Templates, Sent, maybe Archive? I don't think so right now. They may be created upon first use (store of a message in it). But it seems to me there may be code that gets a reference to the folder (e.g. Drafts) but then ultimately does not store anything in it. In the old code I think this would work and the folder would be really created. In the new code you would create it upon first reference. I don't say it is wrong, it may just change the behaviour.
But I just assume this may be the case now, it needs verification.

Comment 56

[Ben Campbell]

OK, so my careful approach of trying to do it all bit-by-bit, refactoring a patch at a time fell apart horribly and devolved into a pile of spaghetti :-) It was mostly working without RDF (give or take the odd issue that still needed ironing out), but it ended up as an all-or-nothing affair - my patches were mutually dependent on each other.

So I've started picking it apart into stand-alone steps that can be landed one at a time.

Just to reiterate the Goal:

I want folder creation to be well defined. If you've got a pointer to a folder, you can be sure about the kind of state it's in. No more "detached" folders. And no more implicit creation of folders. All folders should be discoverable by starting at through the root folders and walking through the hierarchy.
RDF-based implicit creation makes the folder lifecycle really opaque, and I'm convinced this complexity is the root cause of so many of the small fiddly bugs that we've got (or at least making them much harder to fix than they should be).
Most of the code already copes with this just fine - but there are a few places which expect to be able to use detached folders.

How I'm planning to get there:

1) replace all RDF folder lookups with calls to utility functions getExistingFolder() or getOrCreateFolder(). These just call the folder-lookup service which still use RDF internally. Previous iterations of these patches had a single folder lookup function with extra "mayCreate", "mustExist" or "checkAttributes" params. This got confusing. So I decided to draw a strong distinction and make two separate functions.

2) go through all the getOrCreateFolder() calls, and convert them to use getExistingFolder() and be able to cope with missing folders (ie either fail gracefully or take steps to set the folder up properly).

3) delete getOrCreateFolder() entirely

4) remove RDF usage from the folder-lookup-service (do hierarchy-searching instead, starting at the root folders)

5) mop up any other stray RDF usage

6) remove the RDF code entirely

(2 is the big step here, although maybe not as big as you'd think - I've gone up to step 5 by just making getOrCreateFolder() act the same as getExistingFolder() (ie returning null for non-existent folders), and it was remarkably solid. Some unit tests failed, and moving virtual folders failed, but other than that it was looking good).

Comment 57

[Ben Campbell]

Attachment: js-use-fls.patch

Here's my first stand-alone patch. It adds getExistingFolder() and getOrCreateFolder() to MailUtils.jsm (replacing getFolderforURL()), and converts  JS to use them.

Geoff: I flagged you for review as you know the JS side waaaaay better than me. But feel free to punt it on to someone else if more appropriate!

(next up: the same but for the C++ side)

Comment 58

[Ben Campbell]

Just kicked off a linux build with unit tests here:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=6c6a5bcaf5330b0de25a292a625c0da21ea152ab

Comment 59

[:aceman]

You do not need to do 5) and 6) in this bug :)

Comment 60

[Jorg K (CEST = GMT+2)]

This is *very* promising. The try is basically green, modulo a known intermittent failure in test_trr.js and the one caused by bug 1501943.

(In reply to :aceman from comment #59)
> You do not need to do 5) and 6) in this bug :)
Right, I will most personally remove RDF since it was me who forked it into C-C in the first place :-)

Comment 61

[Geoff Lankow (:darktrojan)]

Can I recommend adding nsIFolderLookupService to MailServices.jsm? I'm going to do it in the near future anyway. Here, I even have a patch you can have (just the first part): https://bugzilla.mozilla.org/attachment.cgi?id=9019181&action=diff

Comment 62

[Ben Campbell]

(In reply to Geoff Lankow (:darktrojan) from comment #61)
> Can I recommend adding nsIFolderLookupService to MailServices.jsm? I'm going
> to do it in the near future anyway. Here, I even have a patch you can have
> (just the first part):
> https://bugzilla.mozilla.org/attachment.cgi?id=9019181&action=diff

Yes! I did think that might be a good idea, but wasn't confident enough to do it without screwing it up (was concerned that circular dependencies might be an issue).

Comment 63

[Ben Campbell]

Actually, I've a suspicion that the folder-lookup service is going to end up so small that it might as well be a single lookup function rolled in to one of the existing services instead. My working-but-most-likely-filled-with-nasty-surprises tangled-spaghetti patch has an folder lookup service of about 20 lines of js.

Comment 64

[Ben Campbell]

Attachment: cpp-use-fls.patch

And here's the c++ part, replacing most rdf-service lookups with GetExistingFolder()/GetOrCreateFolder()

running try build + tests:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=08cb6472ece111a49fcce39384d0044f9167cc64&selectedJob=207923562

Comment 65

[Ben Campbell]

I think that gets us down to about remaining 3 places in javascript to remove rdf-service usage, and 2 on the C++ side.
The C++ side is for creating virtual folders rather than folder lookup, and I've got a version already running against my rdf-free folder-lookup-service. It's got some issues, so I'll try and refine out something more landable.

Comment 66

[Jorg K (CEST = GMT+2)]

Comment on attachment 9020259 [details] [diff] [review]
cpp-use-fls.patch

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

This looks a whole less painful than I thought. Overall, I'm the wrong reviewer since so far I've kept out of RDF ;-) - A few comments and nits below.

::: mailnews/base/src/nsMsgAccountManager.cpp
@@ -1470,5 @@
> -        folder = do_QueryInterface(res, &rv);
> -        nsCOMPtr <nsIMsgFolder> parent;
> -        if (folder && NS_SUCCEEDED(rv))
> -        {
> -          rv = folder->GetParent(getter_AddRefs(parent));

Why don't you need to get the parent any more?

@@ -1482,5 @@
> -        folder = do_QueryInterface(res, &rv);
> -        nsCOMPtr <nsIMsgFolder> parent;
> -        if (folder && NS_SUCCEEDED(rv))
> -        {
> -          rv = folder->GetParent(getter_AddRefs(parent));

And here.

@@ -1501,5 @@
> -        folder = do_QueryInterface(res, &rv);
> -        if (folder && NS_SUCCEEDED(rv))
> -        {
> -          nsCOMPtr <nsIMsgFolder> parent;
> -          rv = folder->GetParent(getter_AddRefs(parent));

And here.

@@ +3067,5 @@
>          for (uint32_t folderIndex = 0; folderIndex < vfCount; folderIndex++)
>          {
> +          nsCOMPtr <nsIMsgFolder> msgFolder(do_QueryElementAt(virtualFolders, folderIndex));
> +          if (!msgFolder)
> +            continue;

That wasn't in the original code. Why is it necessary now?

::: mailnews/base/util/nsMsgUtils.cpp
@@ +860,2 @@
>  nsresult
> +GetOrCreateJunkFolder(const nsACString &aURI, nsIUrlListener *aListener)

Good move renaming this.

::: mailnews/imap/src/nsImapMailFolder.cpp
@@ +1014,5 @@
>    GetParent(getter_AddRefs(msgParent));
>  
>    // parent is probably not set because *this* was probably created by rdf
>    // and not by folder discovery. So, we have to compute the parent.
> +  // TODO: once rdf is gone, this is a non issue, and this code can be zapped.

We usually use XXX TODO.

So since RDF is going, why can that be adjusted accordingly now?

::: mailnews/local/src/nsLocalMailFolder.cpp
@@ +485,5 @@
>    GetParent(getter_AddRefs(msgParent));
>  
>    // parent is probably not set because *this* was probably created by rdf
>    // and not by folder discovery. So, we have to compute the parent.
> +  // TODO: this is redundant once RDF is gone (all folders should have parents!)

XXX TODO. Why not do it now?

@@ +3336,5 @@
>    NS_ENSURE_SUCCESS(rv,rv);
>    nsCOMPtr<nsIMsgFolder> msgFolder;
>    rv = FindSubFolder(escapedFolderName, getter_AddRefs(msgFolder));
> +  if (rv == NS_MSG_ERROR_FOLDER_MISSING) {
> +    return rv; // don't warn. This is fine.

Two spaces before the //.

Comment 67

[Geoff Lankow (:darktrojan)]

Comment on attachment 9019923 [details] [diff] [review]
js-use-fls.patch

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

Looks good to me. I'm sure aceman will find some more nits.

::: mail/base/content/messageWindow.js
@@ +1078,5 @@
>          if (!targetURI)
>            return false;
> +        let targetFolder = MailUtils.getExistingFolder(targetURI);
> +        // If null, folder doesn't exist.
> +        return (targetFolder!==null);

!== should have spaces around it.

::: mail/test/mozmill/folder-display/test-message-commands.js
@@ +423,5 @@
>    if (keep_structure) {
>      firstArchiveUri += "/ArchiveSrc";
>      lastArchiveUri += "/ArchiveSrc";
>    }
> +  let firstArchiveFolder =  MailUtils.getOrCreateFolder(firstArchiveUri);

Stray extra space.

::: mailnews/base/search/content/searchWidgets.xml
@@ +447,5 @@
>              {
>                case "movemessage":
>                case "copymessage":
>                  let msgFolder = document.getAnonymousNodes(actionTarget)[0].value ?
> +                  this.MailUtils.getOrCreateFolder(document.getAnonymousNodes(actionTarget)[0].value) : null;

Please fix this long line.

::: mailnews/base/src/folderLookupService.js
@@ +109,5 @@
> +    let rdf = Cc["@mozilla.org/rdf/rdf-service;1"]
> +                .getService(Ci.nsIRDFService);
> +    try {
> +      let folder = rdf.GetResource(aUrl)
> +                  .QueryInterface(Ci.nsIMsgFolder);

Line up the .QueryInterface with .GetResource.

::: mailnews/base/util/folderUtils.jsm
@@ +7,5 @@
>   */
>  
>  this.EXPORTED_SYMBOLS = ["getFolderProperties", "getSpecialFolderString",
> +                         "allAccountsSorted",
> +                         "getMostRecentFolders", "folderNameCompare"];

I'm guessing this is due to a rebase onto my recent change here. Probably should just leave the file unchanged.

Comment 68

[Ben Campbell]

Attachment: js-use-fls-2.patch

Thanks Geoff! New patch with the changes you suggested.

I also removed an unused rdf service lookup, which was flagged by the js linting:

TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/comm/mail/components/compose/content/MsgComposeCommands.js:4162:9 | 'rdf' is assigned a value but never used. (no-unused-vars)


I'll reset the "r?" again (sorry), but is there is there any 'better' procedure for handling small revisions to big patches like this?

Comment 69

[Ben Campbell]

(In reply to Jorg K (GMT+2) from comment #66)
> This looks a whole less painful than I thought. Overall, I'm the wrong
> reviewer since so far I've kept out of RDF ;-) - A few comments and nits
> below.

Thanks for taking a look over it anyway. It's my first proper deep dig into this stuff, so I appreciate the extra eyeballs!

> ::: mailnews/base/src/nsMsgAccountManager.cpp
> @@ -1470,5 @@
> > -        folder = do_QueryInterface(res, &rv);
> > -        nsCOMPtr <nsIMsgFolder> parent;
> > -        if (folder && NS_SUCCEEDED(rv))
> > -        {
> > -          rv = folder->GetParent(getter_AddRefs(parent));
> 
> Why don't you need to get the parent any more?

The GetParent() check is used to detect 'dangling' (parentless) folders, but that's not needed here because the folder was found using GetExistingFolder(), which never returns dangling folders.
(ditto for the other two cases)

GetOrCreateFolders() does potentially return dangling folders. I made it a separate function so it'll be easy to search for and eradicate. I'm pretty sure reliance on dangling folders is a rarity, but we need to pick through carefully to make sure.

> @@ +3067,5 @@
> >          for (uint32_t folderIndex = 0; folderIndex < vfCount; folderIndex++)
> >          {
> > +          nsCOMPtr <nsIMsgFolder> msgFolder(do_QueryElementAt(virtualFolders, folderIndex));
> > +          if (!msgFolder)
> > +            continue;
> 
> That wasn't in the original code. Why is it necessary now?

Good point. I don't think it is needed, so I'll remove it.
(I extracted this patch from a more intrusive set of changes and I think that one just leaked in accidentally)
 
> ::: mailnews/imap/src/nsImapMailFolder.cpp
> @@ +1014,5 @@
> >    GetParent(getter_AddRefs(msgParent));
> >  
> >    // parent is probably not set because *this* was probably created by rdf
> >    // and not by folder discovery. So, we have to compute the parent.
> > +  // TODO: once rdf is gone, this is a non issue, and this code can be zapped.
> 
> We usually use XXX TODO.
> 
> So since RDF is going, why can that be adjusted accordingly now?

Some folder lookups still use GetOrCreateFolder() which can return dangling folders. Once GetOrCreateFolder() is eradicated, we can remove this code.

> @@ +3336,5 @@
> >    NS_ENSURE_SUCCESS(rv,rv);
> >    nsCOMPtr<nsIMsgFolder> msgFolder;
> >    rv = FindSubFolder(escapedFolderName, getter_AddRefs(msgFolder));
> > +  if (rv == NS_MSG_ERROR_FOLDER_MISSING) {
> > +    return rv; // don't warn. This is fine.
> 
> Two spaces before the //.

Noted. Actually, on reflection I think it's misleading, so I ditched the comment completely (I just wanted to avoid the NS_ENSURE_SUCCESS debug message for the missing folder, as it's a legitimate condition rather than something exceptional),

Comment 70

[Ben Campbell]

Attachment: cpp-use-fls-2.patch

Comment 71

[Geoff Lankow (:darktrojan)]

Comment on attachment 9020670 [details] [diff] [review]
js-use-fls-2.patch

> I'll reset the "r?" again (sorry), but is there is there any 'better' procedure
> for handling small revisions to big patches like this?

Not really, Bugzilla managed to show me the difference between the patches without breaking for once, so it was easy enough.

Comment 72

[Jorg K (CEST = GMT+2)]

Mostly interdiff works, so for the JS patch, it's just this using the "Diff":
https://bugzilla.mozilla.org/attachment.cgi?oldid=9019923&action=interdiff&newid=9020670&headers=1

Comment 73

[:aceman]

(In reply to Ben Campbell from comment #69)
> > > +  if (rv == NS_MSG_ERROR_FOLDER_MISSING) {
> > > +    return rv; // don't warn. This is fine.
> > 
> > Two spaces before the //.
> 
> Noted. Actually, on reflection I think it's misleading, so I ditched the
> comment completely (I just wanted to avoid the NS_ENSURE_SUCCESS debug
> message for the missing folder, as it's a legitimate condition rather than
> something exceptional),

Thanks for thinking of this. The console is already full of warnings of which we do not know if they are OK or not. We should not produce a warning if there is no problem and the result is expected or OK.
However, we should only propagate critical problems via rv and not application logic or return values.

Why why is it expected that in nsMsgLocalMailFolder::setSubfolderFlag() the folder we want to set is missing?

Comment 74

[:aceman]

Comment on attachment 9020671 [details] [diff] [review]
cpp-use-fls-2.patch

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

Looks great to me, thanks.

Also please grep through the code to look for rdf mentions that may be obsoleted by this patch.
E.g.:
local/src/nsMailboxService.cpp:#include "nsIRDFService.h"

::: mailnews/base/src/nsSpamSettings.cpp
@@ +516,2 @@
>    if (!folder)
>      return NS_ERROR_UNEXPECTED;

May this happen that folder is null but rv was NS_OK?

There are many other places calling GetExistingFolder() and some check the return value, some check the folder for null, some do both. This could be consolidated.

::: mailnews/base/util/nsMsgDBFolder.cpp
@@ +3006,5 @@
>    uri.Append(mURI);
>    uri.Append('/');
>    uri.Append(aEscapedSubFolderName);
>  
> +  return GetExistingFolder(uri, aFolder);

So you basically make this behave the same as GetChildNamed() ?
Maybe we should just merge the functions :)
Also if you changed this to no longer create the folder, I assume you checked all callers if they are OK with that.

::: mailnews/base/util/nsMsgIdentity.cpp
@@ +281,5 @@
>  
>    nsresult rv = mPrefBranch->GetCharPref(prefname, retval);
>    if (NS_SUCCEEDED(rv) && !retval.IsEmpty()) {
> +    nsCOMPtr <nsIMsgFolder> folderResource;
> +    GetOrCreateFolder(retval, getter_AddRefs(folderResource));

Rename the variable to folder if it is one now.

@@ -296,3 @@
>      if (folderResource)
>      {
> -      // don't check validity of folder - caller will handle creating it

So what does it mean? Will the caller create it before entering this function? Or after? If so, you could use GetExistingFolder().

::: mailnews/base/util/nsMsgUtils.cpp
@@ +777,5 @@
> +  nsresult rv;
> +  nsCOMPtr<nsIFolderLookupService> fls(do_GetService(NSIFLS_CONTRACTID, &rv));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  rv = fls->GetOrCreateFolderForURL(aFolderURI, aFolder);

GetOrCreateFolderForURL does not seem to exist, this does not compile.

@@ +862,3 @@
>  {
>    nsresult rv;
> +  nsCOMPtr<nsIMsgFolder> folderResource;

No "Resource".

::: mailnews/compose/src/nsMsgCopy.cpp
@@ +418,1 @@
>      nsCOMPtr <nsIMsgFolder> folderResource;

No "Resource" in name.

::: mailnews/imap/src/nsImapIncomingServer.cpp
@@ +3253,5 @@
>      // we didn't find the folder so we will have to create a new one.
>      if (namespacePrefixAdded)
>      {
> +      nsCOMPtr<nsIMsgFolder> folderResource;
> +      rv = GetOrCreateFolder(folderUriWithNamespace, getter_AddRefs(folderResource));

Can this be msgFolder directly?

@@ +3262,5 @@
>        msgFolder = aFolderResource;
>    }
>  
>    msgFolder.forget(aFolder);
> +  return (aFolder ? NS_OK : NS_ERROR_FAILURE);

So this is a unexpected failure that we tried to create the folder and it still doesn't exist?

::: mailnews/imap/src/nsImapMailFolder.cpp
@@ +278,5 @@
>      return NS_MSG_FOLDER_EXISTS;
>  
> +  nsCOMPtr<nsIMsgFolder> folder;
> +  rv = CreateChildFromURI(uri, getter_AddRefs(folder));
> +  NS_ENSURE_SUCCESS(rv, NS_ERROR_FAILURE);

Why returning the specific error and not rv?

@@ +339,5 @@
>      return NS_MSG_FOLDER_EXISTS;
>  
> +  nsCOMPtr<nsIMsgFolder> folder;
> +  rv = CreateChildFromURI(uri, getter_AddRefs(folder));
> +  NS_ENSURE_SUCCESS(rv, NS_ERROR_FAILURE);

Why returning the specific error and not rv?

@@ +866,5 @@
>      leafName.Assign(Substring(parentName, folderStart + 1));
>      parentName.SetLength(folderStart);
>  
>      rv = CreateDirectoryForFolder(getter_AddRefs(path));
> +    NS_ENSURE_SUCCESS(rv,rv);

There should be a space after comma. More occurrences below.

@@ +6814,3 @@
>    NS_ENSURE_SUCCESS(rv, rv);
> +
> +  nsCOMPtr <nsIDBFolderInfo> folderInfo;

Please kill the space before <> where you have the chance (touching it).

::: mailnews/local/src/nsLocalMailFolder.cpp
@@ +481,3 @@
>  NS_IMETHODIMP nsMsgLocalMailFolder::CreateStorageIfMissing(nsIUrlListener* aUrlListener)
>  {
>    nsresult rv;

Somehow gcc complains this 'rv' may stay unintialized, which seems true, but I do not see how it was caused by this patch:
comm/mailnews/local/src/nsLocalMailFolder.cpp: In member function ‘virtual nsresult nsMsgLocalMailFolder::CreateStorageIfMissing(nsIUrlListener*)’:
comm/mailnews/local/src/nsLocalMailFolder.cpp:483:12: warning: ‘rv’ may be used uninitialized in this function [-Wmaybe-uninitialized]

Comment 75

[Ben Campbell]

Attachment: cpp-use-fls-3.patch

A revised version of the patch.
The main thrust is that I think the previous patch was abusing the GetExistingFolder() nsresult code to determine between cases where a missing folder was an actual error, and cases where a missing folder was expected and dealt with.
To more explicitly distinguish between these cases, I've split off another util function, FindFolder(). So, to recap:

GetExistingFolder() - find folder by uri, error if it doesn't exist. Will never return a "dangling" folder.
FindFolder() - find folder by uri (null returned if non-existant, and not an error) Will never return a "dangling" folder.
GetOrCreateFolder() - find folder by uri, creating a dangling folder if it doesn't already exist (ie the old rdf GetResource() behaviour).

(FindFolder does sound a bit generic, but it does turn out to be nicely greppable).

So for these folder look-ups, a failing nsresult code should be _always_ be an exceptional occurrence, not part of the expected program flow.

Comment 76

[Ben Campbell]

(In reply to :aceman from comment #73)
> However, we should only propagate critical problems via rv and not
> application logic or return values.

I agree. My thinking on the previous patch was a little muddled, and there were places where it wasn't clear enough.

> Why why is it expected that in nsMsgLocalMailFolder::setSubfolderFlag() the
> folder we want to set is missing?

It's only used by nsMsgLocalMailFolder::SetFlagsOnDefaultMailboxes(), which goes through a bunch of default folders, setting their flags if they exist.
https://dxr.mozilla.org/comm-central/source/comm/mailnews/local/src/nsLocalMailFolder.cpp#3346

Comment 77

[Ben Campbell]

(In reply to :aceman from comment #74)
> Comment on attachment 9020671 [details] [diff] [review]
> cpp-use-fls-2.patch
[snip]
> ::: mailnews/base/util/nsMsgDBFolder.cpp
> @@ +3006,5 @@
> >    uri.Append(mURI);
> >    uri.Append('/');
> >    uri.Append(aEscapedSubFolderName);
> >  
> > +  return GetExistingFolder(uri, aFolder);
> 
> So you basically make this behave the same as GetChildNamed() ?
> Maybe we should just merge the functions :)
> Also if you changed this to no longer create the folder, I assume you
> checked all callers if they are OK with that.

Sorry, I screwed up this one - it _should_ be GetOrCreateFolder() for now and the new patch fixes this.

> ::: mailnews/base/util/nsMsgUtils.cpp
> @@ +777,5 @@
> > +  nsresult rv;
> > +  nsCOMPtr<nsIFolderLookupService> fls(do_GetService(NSIFLS_CONTRACTID, &rv));
> > +  NS_ENSURE_SUCCESS(rv, rv);
> > +
> > +  rv = fls->GetOrCreateFolderForURL(aFolderURI, aFolder);
> 
> GetOrCreateFolderForURL does not seem to exist, this does not compile.

Ahh - the C++ patch relies on the javascript one, which updates the folder-lookup-service. I should have been clearer on that!

Comment 78

[Ben Campbell]

Attachment: cpp-use-fls-4.patch

rebased version of the c++ patch.

Comment 79

[Geoff Lankow (:darktrojan)]

Ben, is the JS patch ready to land? I'm planning to hit mail/base/content with the linter soon, and I'd rather avoid bitrot if possible.

Comment 80

[Jorg K (CEST = GMT+2)]

Comment on attachment 9020670 [details] [diff] [review]
js-use-fls-2.patch

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

::: mailnews/base/src/folderLookupService.js
@@ +43,5 @@
>  
>    // nsIFolderLookupService impl
>    getFolderForURL: function (aUrl) {
> +    // NOTES on transitioning away from RDF (BenC):
> +    // I _think_ this getFolderForURL() implementation

Maybe remove or clarify this?

@@ +106,5 @@
> +  getOrCreateFolderForURL: function (aUrl) {
> +    // NOTE: this doesn't update _map, but it'll work fine and
> +    // it's a transitional function we want deleted anyway.
> +    let rdf = Cc["@mozilla.org/rdf/rdf-service;1"]
> +                .getService(Ci.nsIRDFService);

So what's the plan for removing this again later?

Comment 81

[Ben Campbell]

(In reply to Geoff Lankow (:darktrojan) from comment #79)
> Ben, is the JS patch ready to land? I'm planning to hit mail/base/content
> with the linter soon, and I'd rather avoid bitrot if possible.

Yes, please. I'm happy with the javascript changes.

Comment 82

[Ben Campbell]

(In reply to Jorg K (GMT+2) from comment #80)
> Comment on attachment 9020670 [details] [diff] [review]
> js-use-fls-2.patch
> ::: mailnews/base/src/folderLookupService.js
> @@ +43,5 @@
> >  
> >    // nsIFolderLookupService impl
> >    getFolderForURL: function (aUrl) {
> > +    // NOTES on transitioning away from RDF (BenC):
> > +    // I _think_ this getFolderForURL() implementation
> 
> Maybe remove or clarify this?

Good point. I'll strip it back.
Geoff: ignore my last comment #81! I'll tighten up the notes and rebase the patch while I'm at it.

> @@ +106,5 @@
> > +  getOrCreateFolderForURL: function (aUrl) {
> > +    // NOTE: this doesn't update _map, but it'll work fine and
> > +    // it's a transitional function we want deleted anyway.
> > +    let rdf = Cc["@mozilla.org/rdf/rdf-service;1"]
> > +                .getService(Ci.nsIRDFService);
> 
> So what's the plan for removing this again later?

1) grep through code looking for calls to getOrCreateFolderForURL()
2) replace them all with calls to getExistingFolder() instead, making sure the callers handle missing folders. Usually this will just be an error condition, but there a probably a few places where the caller will want to create a missing folder itself. And when I say "create a missing folder" I mean one that is properly linked in to the folder hierarchy, not a parentless/"dangling" folder like RDF GetResource() creates).
3) delete getOrCreateFolderForURL() entirely
4) switch getExistingFolder() to do folder lookups by simply searching the folder hierarchy, instead of via rdf.

Comment 83

[Ben Campbell]

Attachment: js-use-fls-3.patch

Rebased, and some hand-wavey uncertain comments removed.

Unit tests look OK locally, and here's a try build:

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=d68a26ee143c801161137f8bc7c01b7b8d14a0b1

Comment 84

[:aceman]

(In reply to Ben Campbell from comment #82)
> > @@ +106,5 @@
> > > +  getOrCreateFolderForURL: function (aUrl) {
> > > +    // NOTE: this doesn't update _map, but it'll work fine and
> > > +    // it's a transitional function we want deleted anyway.
> > > +    let rdf = Cc["@mozilla.org/rdf/rdf-service;1"]
> > > +                .getService(Ci.nsIRDFService);
> > 
> > So what's the plan for removing this again later?
> 
> 4) switch getExistingFolder() to do folder lookups by simply searching the
> folder hierarchy, instead of via rdf.

Why isn't it already doing this? I thought this was the whole point of it, that getExistingFolder() is already the safe one without RDF. And getOrCreateFolder is the one where we are unsure so still automatically create the folder if not existing.

Comment 85

[:aceman]

Comment on attachment 9021970 [details] [diff] [review]
cpp-use-fls-4.patch

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

::: mailnews/base/src/nsSpamSettings.cpp
@@ +517,2 @@
>    if (!folder)
>      return NS_ERROR_UNEXPECTED;

So this one looks like a case for GetExistingFolder() ?

::: mailnews/base/util/nsMsgUtils.cpp
@@ +888,1 @@
>    rv = server->GetMsgFolderFromURI(folderResource, aURI, getter_AddRefs(msgFolder));

What is this doing? I thought we already got/created the folder above, why are we getting it again  with the same aURI?

::: mailnews/imap/src/nsImapIncomingServer.cpp
@@ +3252,5 @@
>    if (NS_FAILED(rv) || !msgFolder) {
>      // we didn't find the folder so we will have to create a new one.
>      if (namespacePrefixAdded)
>      {
> +      nsCOMPtr<nsIMsgFolder> folderResource;

'folder'

Comment 86

[Ben Campbell]

(In reply to :aceman from comment #84)
> > 4) switch getExistingFolder() to do folder lookups by simply searching the
> > folder hierarchy, instead of via rdf.
> 
> Why isn't it already doing this? I thought this was the whole point of it,
> that getExistingFolder() is already the safe one without RDF. And
> getOrCreateFolder is the one where we are unsure so still automatically
> create the folder if not existing.

I was treating them as separate steps - this patch to switch all the C++ folder lookups over to use the folder-lookup-service, another pass to get rid of all the getOrCreateFolder() calls, then finally switch over to a non-rdf folder-lookup-service.

But it's a good observation - there's no reason the getExistingFolder implementation can't be switched over right now. I've even got some code for that, more-or-less ready to go. I'll tidy it up and post it as a separate patch.

Comment 87

[Ben Campbell]

(In reply to :aceman from comment #85)
> Comment on attachment 9021970 [details] [diff] [review]
> cpp-use-fls-4.patch
> 
> Review of attachment 9021970 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mailnews/base/src/nsSpamSettings.cpp
> @@ +517,2 @@
> >    if (!folder)
> >      return NS_ERROR_UNEXPECTED;
> 
> So this one looks like a case for GetExistingFolder() ?

I think I agree (I probably went with FindFolder() as the more direct translation of the original code).

> ::: mailnews/base/util/nsMsgUtils.cpp
> @@ +888,1 @@
> >    rv = server->GetMsgFolderFromURI(folderResource, aURI, getter_AddRefs(msgFolder));
> 
> What is this doing? I thought we already got/created the folder above, why
> are we getting it again  with the same aURI?

The code above uses GetOrCreateFolder(), which can return dangling folders (ie the original RDF GetResource() behaviour).
This GetMsgFolderFromURI() stuff is the original code that was there to cope with that.
The intention is to go through and replace all the GetOrCreateFolder() calls and replace them, and this is a great example of the sort of stuff that needs to be sorted out.
But definitely for a separate patch (and probably a separate bug).

Thanks for that! Tweaked patch on the way...

Comment 88

[Ben Campbell]

The C++ patch isn't quite there yet - there are a couple of unit test failures I need to figure out and fix
(one in test_junkingWhenDisabled.js and one in test_nsIMsgLocalMailFolder.js).

Comment 89

[Magnus Melin [:mkmelin]]

Can you post what you have, perhaps someone can help spot the problem?

Comment 90

[Ben Campbell]

Attachment: js-use-fls-4.patch

Rebased javascript patch. Sorry Geoff, I flagged you for review again, but there aren't any big changes over the last one.

I'd like to get this javascript fls patch landed if everyone's happy with it (as opposed to this ongoing rebasing).
Is there anything else I need to do to get it landed?

Comment 91

[Ben Campbell]

Attachment: cpp-use-fls-5.patch

And here's the C++ patch, as it stands (also rebased, but still with the two unit test failures).

Comment 92

[Magnus Melin [:mkmelin]]

Which are the failing tests?

Comment 93

[:aceman]

We can surely land partial patches to avoid bitrot, but they must not cause test failures.

Comment 94

[Geoff Lankow (:darktrojan)]

Comment on attachment 9025981 [details] [diff] [review]
js-use-fls-4.patch

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

Looks good, just check the one query and get it landed.

::: mail/base/content/SearchDialog.js
@@ +524,5 @@
>    let destUri = destFolder.getAttribute("id");
>    if (destUri.length == 0)
>      destUri = destFolder.getAttribute("file-uri");
>  
> +  let destMsgFolder = MailUtils.getExistingFolder(destUri);

This was getOrCreateFolder in the previous version. Mistake?

Comment 95

[Ben Campbell]

(In reply to :aceman from comment #93)
> We can surely land partial patches to avoid bitrot, but they must not cause
> test failures.

The tests are fine in the javascript patch, and it's a stand-alone chunk of work which doesn't rely on the C++ changes, so I'm keen to get it landed.

(In reply to Magnus Melin [:mkmelin] from comment #92)
> Which are the failing tests?

test_junkingWhenDisabled.js and test_nsIMsgLocalMailFolder.js

They are folder-creation issues, to do with dangling-folder handling. They are fixable, but they imply that the new fls-using C++ functions are doing something different to what the RDF service does (which is worrying, because they still use the RDF service underneath!)

Comment 96

[Ben Campbell]

(In reply to Geoff Lankow (:darktrojan) from comment #94)
> This was getOrCreateFolder in the previous version. Mistake?

Hmm. Yes. I think you're right. I'll update the patch.
(I didn't see any dangling-folder-handling and got a bit gung-ho :-)

Comment 97

[Ben Campbell]

Attachment: js-use-fls-5.patch

Comment 98

[Ben Campbell]

(In reply to Magnus Melin [:mkmelin] from comment #92)
> Which are the failing tests?

Oh, forgot that I kicked off a try build earlier:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=eb331b8dd39058c8a5a081ac6b42d84db49514da&selectedJob=212552354

Comment 99

[:aceman]

Do you have try run with the JS part only?
Also, please try on OSX or Windows too, as Linux has some intermittent test failures even if the patch is right.

Comment 100

[:aceman]

Comment on attachment 9026006 [details] [diff] [review]
js-use-fls-5.patch

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

::: mailnews/base/content/virtualFolderListEdit.js
@@ +12,4 @@
>      if (window.arguments[0].searchFolderURIs) {
>        let srchFolderUriArray = window.arguments[0].searchFolderURIs.split('|');
>        for (let uri of srchFolderUriArray) {
> +        this._selectedList.add(MailUtils.getOrCreateFolder(uri));

I'd hope folders spanned by a virtual folder already exist, but yes, you will investigate the cases later.

::: mailnews/base/search/content/searchWidgets.xml
@@ +234,5 @@
>              let foldersScanned = [];
>  
>              for (let identity of identities) {
>                let enumerator = null;
> +              let msgFolder = MailUtils.getExistingFolder(identity.stationeryFolder);

The other occurrence in this file uses this.MailUtils. because it imports MailUtils only in that 'validateAction' method.
So you do not have MailUtils available here unless you import it.

Comment 101

[Ben Campbell]

Attachment: js-use-fls-6.patch [checked in, comment #104]

New patch with proper use of MailUtils in searchWidgets.xml - thanks for catching that one, aceman!

Try build with just the javascript patch (and a spread of platforms):

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=9c414cd3b47ecea45ccdd198cc56aac3f22bd0f7

Comment 102

[:aceman]

Comment on attachment 9026203 [details] [diff] [review]
js-use-fls-6.patch [checked in, comment #104]

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

Thanks.

Comment 103

[Ben Campbell]

OK, if everyone's OK with it, I'd like to submit my `js-use-fls-6.patch` for landing.

Comment 104

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/2c8a0efaccfe
Convert RDF consumers to use the folder-lookup service in JavaScript code. r=darktrojan,aceman

Comment 105

[Ben Campbell]

Attachment: cpp-use-fls-6.patch

This patch fixes up the failing `test_junkingWhenDisabled.js` xpcshell unit test.

Try run here:

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=41ea4a2205cc24a5088786466316140bbc72861a

The core of the fix is a rewrite of the GetOrCreateJunkFolder(), which is a good test run for getting rid of the GetOrCreateFolder() calls.
I'll dump some more thoughts on this tomorrow - folder creation in general has some catch-22 situations relying on dangling folders, which need to be sorted out in order to complete the RDF removal.

Comment 106

[Ben Campbell]

Attachment: cpp-use-fls-7.patch

I thought the last patch nailed everything down, but the try run showed up some mozmill failures.
This new patch loosens off some of the more aggressive GetExistingFolder() calls, switching them back the original behaviour: GetOrCreateFolder().
It fixes a whole bunch of mozmill fails (on a local mozmill run, at least - I'm just kicking of a Try server build now).

Comment 107

[Ben Campbell]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=73f31b1dd95155d43ae8abd311a1c1e8c6d7c314

Comment 108

[Ben Campbell]

Attachment: cpp-use-fls-8.patch

Eighth time's the charm!

Try build:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=6fa14c7b095e3fe60d1cb54d560090d1e08c27e2

Comment 109

[Ben Campbell]

This patch is 3 separate commits - I'm not sure what the general approach is here. They are discrete changes, but I can merge them all if that's preferred.

Comment 110

[:aceman]

Comment on attachment 9031331 [details] [diff] [review]
cpp-use-fls-8.patch

Yes it seems there are 3 patches concatenated into that file. I'm not sure that works.
Sorry for taking so long that the patch does not apply cleanly now.
Please update it and split into 3 patches and describe what they do. E.g. you now disable the cache of the folders which is a new thing never mentioned before. Why is it needed?

Comment 111

[Ben Campbell]

Attachment: cpp-use-fls-9.patch

Sorry the bitrot got you, aceman - it was bad timing, as I was just rebasing it as you posted!

This new one is a single changeset:

• I ditched the changeset removing the FLS cache (I thought the cache was causing some unit test failures, but it turned out to be something else)
• I merged the changeset with the GetOrCreateJunkFolder() rewrite. The old GetOrCreateJunkFolder() implementation was causing a test failure, so really it needed to be rolled in with the other changes anyway.
• I found a bunch more places where GetOrCreateFolder could be removed.

Try build running here:

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=037b7bbebb4f29255bb65e78bb8bd2c4aaa9d11d

Comment 112

[Ben Campbell]

(In reply to Ben Campbell from comment #111)

• I found a bunch more places where GetOrCreateFolder could be removed.

Just to recap (it's been a while!): GetOrCreateFolder() is the old RDF-style behaviour which can return dangling folders. The aim is to ditch it completely. There are still a few places which rely upon dangling folders, and they need to be sorted out - but that's outside the scope of this bug - I'll open another issue for them when this stuff is landed.

Comment 113

[Ben Campbell]

Comment on attachment 9035213 [details] [diff] [review]
cpp-use-fls-9.patch

Gah. Looks like some of my GetOrCreateFolder() removals have caused some mozmill fails. I'll back some of them out and try again.

Comment 114

[Ben Campbell]

Attachment: cpp-use-fls-10.patch

Dropped my set of seemingly-straightforward GetOrCreateFolder() removals. Sigh.

Try build looking much better now:

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=c3ae3bbcdb23ab6ed13d26986b4671f5404028d9

Comment 115

[:aceman]

Comment on attachment 9035248 [details] [diff] [review]
cpp-use-fls-10.patch

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

::: mailnews/base/search/src/nsMsgFilterService.cpp
@@ +614,5 @@
>              !uri.Equals(actionTargetFolderUri))
>          {
> +          nsCOMPtr<nsIMsgFolder> destIFolder;
> +          rv = GetExistingFolder(actionTargetFolderUri, getter_AddRefs(destIFolder));
> +          NS_ENSURE_SUCCESS(rv, rv);

Shouldn't this be FindFolder? We cope with folder not existing by disabling the filter, so we must not return here.

::: mailnews/base/src/nsMsgAccountManager.cpp
@@ +1449,4 @@
>        nsCOMPtr<nsIMsgFolder> folder;
>        thisIdentity->GetFccFolder(folderUri);
> +      if (!folderUri.IsEmpty() &&
> +          NS_SUCCEEDED(GetOrCreateFolder(folderUri, getter_AddRefs(folder))))

This originally checked parent folder, so why does it not take GetExistingFolder() ? And all the folders below.

::: mailnews/base/src/nsMsgPurgeService.cpp
@@ +261,5 @@
>          // because the folder pane isn't built yet, for example
>          // skip this account
>          nsCOMPtr<nsIMsgFolder> junkFolder;
> +        rv = FindFolder(junkFolderURI, getter_AddRefs(junkFolder));
> +        NS_ENSURE_SUCCESS(rv,rv);

Space after , .

::: mailnews/base/util/nsMsgIdentity.cpp
@@ +280,5 @@
>      return NS_ERROR_NOT_INITIALIZED;
>  
>    nsresult rv = mPrefBranch->GetCharPref(prefname, retval);
>    if (NS_SUCCEEDED(rv) && !retval.IsEmpty()) {
> +    nsCOMPtr <nsIMsgFolder> folder;

No space before '<' please here and elsewhere. You even use it inconsistently in this same block.

@@ +284,5 @@
> +    nsCOMPtr <nsIMsgFolder> folder;
> +    rv = GetOrCreateFolder(retval, getter_AddRefs(folder));
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    nsCOMPtr<nsIMsgIncomingServer> server;
> +    //make sure that folder hierarchy is built so that legitimate parent-child relationship is established

Space after //, capital M and dot after end of sentence.

::: mailnews/base/util/nsMsgUtils.cpp
@@ +875,4 @@
>  {
>    nsresult rv;
> +  nsCOMPtr<nsIMsgFolder> folder;
> +  GetExistingFolder(aURI, getter_AddRefs(folder));

Why does this not use FindFolder() ?

::: mailnews/imap/src/nsImapIncomingServer.cpp
@@ +1619,5 @@
>  // the existing special folder. If not, set the special flag on the folder so
>  // it will be there if and when the folder is created.
>  // Return true if we found an existing special folder different than
>  // the one specified in prefs, and the one specified by prefs doesn't exist.
> +bool nsImapIncomingServer::CheckSpecialFolder( nsCString &folderUri,

Why the space?

@@ +1646,4 @@
>      }
> +
> +    nsString folderName;
> +    folder->GetPrettyName(folderName);

I wonder why we have to get the name here.

::: mailnews/imap/src/nsImapIncomingServer.h
@@ +97,5 @@
>    nsresult DoomUrlIfChannelHasError(nsIImapUrl *aImapUrl, bool *urlDoomed);
>    bool ConnectionTimeOut(nsIImapProtocol* aImapConnection);
>    nsresult GetFormattedStringFromName(const nsAString& aValue, const char* aName, nsAString& aResult);
>    nsresult GetPrefForServerAttribute(const char *prefSuffix, bool *prefValue);
> +  bool CheckSpecialFolder( nsCString &folderUri,

Remove the space.

::: mailnews/imap/src/nsImapMailFolder.cpp
@@ +871,5 @@
>      leafName.Assign(Substring(parentName, folderStart + 1));
>      parentName.SetLength(folderStart);
>  
>      rv = CreateDirectoryForFolder(getter_AddRefs(path));
> +    NS_ENSURE_SUCCESS(rv,rv);

Space after comma.

::: mailnews/imap/src/nsImapOfflineSync.cpp
@@ +405,4 @@
>  
> +    nsCOMPtr<nsIMsgFolder> destFolder;
> +    rv = GetOrCreateFolder(moveDestination, getter_AddRefs(destFolder));
> +    if (NS_WARN_IF(!NS_SUCCEEDED(rv)))

NS_FAILED(rv)

::: mailnews/local/src/nsLocalMailFolder.cpp
@@ -3352,5 @@
> -  // .msf file when we don't want to.
> -  nsCOMPtr <nsIMsgFolder> parent;
> -  msgFolder->GetParent(getter_AddRefs(parent));
> -  if (!parent)
> -    return NS_ERROR_FAILURE;

Why is this check removed? FindSubFolder calls GetOrCreateFolder(), which creates the folder, but it may be unparented (due to RDF). Or isn't it the case anymore?

Comment 116

[Ben Campbell]

Attachment: cpp-use-fls-11.patch

Comment 117

[Ben Campbell]

(In reply to :aceman from comment #115)

::: mailnews/base/search/src/nsMsgFilterService.cpp
@@ +614,5 @@

         !uri.Equals(actionTargetFolderUri))
     {

•      nsCOMPtr<nsIMsgFolder> destIFolder;
  

•      rv = GetExistingFolder(actionTargetFolderUri, getter_AddRefs(destIFolder));
  

•      NS_ENSURE_SUCCESS(rv, rv);
  

Shouldn't this be FindFolder? We cope with folder not existing by disabling
the filter, so we must not return here.

Yes! Now changed.

::: mailnews/base/src/nsMsgAccountManager.cpp
@@ +1449,4 @@

   nsCOMPtr<nsIMsgFolder> folder;
   thisIdentity->GetFccFolder(folderUri);

•  if (!folderUri.IsEmpty() &&
  

•      NS_SUCCEEDED(GetOrCreateFolder(folderUri, getter_AddRefs(folder))))
  

This originally checked parent folder, so why does it not take
GetExistingFolder() ? And all the folders below.

I did switch to GetExistingFolder() but later switched back as it was causing some test fails.
I just forgot to put the parent check back in. However, I've left it out still. It sets the folder flags even on dangling folders. Rationale: can't see any reason why this'd be an issue, and there are other places in the code which do this.

::: mailnews/base/util/nsMsgUtils.cpp
@@ +875,4 @@

{
nsresult rv;

• nsCOMPtr<nsIMsgFolder> folder;
• GetExistingFolder(aURI, getter_AddRefs(folder));

Why does this not use FindFolder() ?

I suspect it predated FindFolder :-) Now changed.

::: mailnews/imap/src/nsImapIncomingServer.cpp
@@ +1646,4 @@

 }

• nsString folderName;
• folder->GetPrettyName(folderName);

I wonder why we have to get the name here.

I was curious about this too (although haven't dug too deep - I just left the code as it was).
I suspect it's to do with massaging the names a little: "Inbox" vs "INBOX" etc.
There's also some localisation hacks scattered about the place. I'd like to take a look at the naming stuff some time and see if such policy can be factored into one place and documented properly. But for now, if it works I'll just leave it :-)

::: mailnews/local/src/nsLocalMailFolder.cpp
@@ -3352,5 @@

• // .msf file when we don't want to.
• nsCOMPtr <nsIMsgFolder> parent;
• msgFolder->GetParent(getter_AddRefs(parent));
• if (!parent)
• return NS_ERROR_FAILURE;

Why is this check removed? FindSubFolder calls GetOrCreateFolder(), which
creates the folder, but it may be unparented (due to RDF). Or isn't it the
case anymore?

Ahh yes - I was originally using GetExistingFolder() but had to roll it back, and forgot to restore this bit! Restored now.

I sorted out a bunch of the spacing/lack-of-spacing nits in the diff hunks I touched. There are a bunch of other offending places in those files - I might see about doing a couple of mass fix-up patches just to tidy them up. But for now, I've tried to keep too much noise out of this patch. It's noisy enough as is :-)

Try build running here:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=6c8dbd8bd92f522d341fa9e058de39686d320158

Comment 118

[Ben Campbell]

So... I think that last revision of the patch brings us back to some mozmill failures. This patch is too flakey.
I've come to the conclusion that it's too much to bite off in one go, so I'm going to do what I should have done to begin with and break it up into small, discrete steps.
Specifically, I'm going to leave out my ham-fisted attempts to tackle the dangling folders issue, which turned out to be way more subtle and tricky than I initially thought.

So, for this bug I'm purely going to replace the direct RDF-service calls with folder-lookup-service calls.
(and I've got a folder-lookup-service patch which replaces RDF completely, after which the RDF code can be removed).
The dangling folders can be dealt with in another bug. (or at least properly defined/documented)

Comment 119

[Ben Campbell]

Attachment: rename-getorcreatefolder-1.patch [checked in, comment #132]

Nice easy one to start with - renames the old GetOrCreateFolder() to GetOrCreateJunkFolder() so we can repurpose the name.

Comment 120

[Ben Campbell]

Attachment: add-findfolder-1.patch [checked in, comment #132]

Next up, this patch adds the FindFolder() and GetOrCreateFolder() utility functions, which use the folder-lookup-service (as does the GetExistingFolder() which is already in use).

Comment 121

[Ben Campbell]

Attachment: mop-up-leftover-js-rdf-use-1.patch [checked in, comment #125]

This patch is independent to the rest of the C++ patches, and simply removes a couple of the leftover direct RDF calls on the javascript side.

Comment 122

[Ben Campbell]

Attachment: use-findfolder-1.patch

This one just straightens out some of the nsresult usage on some folder lookups. A bunch of them were using GetExistingFolder() to test for folder existence, but now there's FindFolder() which doesn't treat missing folders as an error condition.
(requires the rename-getorcreatefolder and add-findfolder patches first)

Comment 123

[Ben Campbell]

Attachment: remove-direct-rdf-calls-1.patch

And this is the biggest one - it replaces all the direct rdf GetResource() calls by going through GetOrCreateFolder().
(again, requires the rename-getorcreatefolder and add-findfolder patches first)

aceman: Sorry to lumber you with so many r? flags, but I figured it was clearer to separate each patch out into deliberate, single-stage steps that just make one change at a time.

There's a try build here (I've tidied up the commit messages, but the changesets otherwise include all these new patches exactly):
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=617eab8ff04996c9fb425c65cbb2fc6ad7a95f4b

Comment 124

[Jorg K (CEST = GMT+2)]

Can we land some of this?

Comment 125

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/a770f9f6ca81
remove some left-over direct RDF uses from JavaScript code. r=aceman

Comment 126

[Ben Campbell]

(In reply to Jorg K (GMT+1) from comment #124)

Can we land some of this?

Yes please! Attachment #9037881 [details] [diff] and Attachment #9037882 [details] [diff] are good to go (in that order).

Comment 127

[Jorg K (CEST = GMT+2)]

What about the big patch "cpp-use-fls-11.patch"? That's superseded by "remove-direct-rdf-calls-1.patch" and more to come?

Comment 128

[Ben Campbell]

(In reply to Jorg K (GMT+1) from comment #127)

What about the big patch "cpp-use-fls-11.patch"? That's superseded by "remove-direct-rdf-calls-1.patch" and more to come?

Yes, ignore cpp-use-fls-11.patch (actually, I'll obsolete it now). I couldn't get it working right, so I split it up into these new discrete patches which just do a single thing at a time:

rename-getorcreatefolder-1.patch - r+ ready to land
add-findfolder-1.patch - r+ ready to land
mop-up-leftover-js-rdf-use-1.patch - landed
use-findfolder-1.patch - r? awaiting review
remove-direct-rdf-calls-1.patch - r? awaiting review

Comment 129

[Jorg K (CEST = GMT+2)]

Comment on attachment 9037882 [details] [diff] [review]
add-findfolder-1.patch [checked in, comment #132]

I think the commit message should be:
Bug 453908 - Add FindFolder() and GetOrCreateFolder() to nsMsgUtils. r=aceman

Comment 130

[Ben Campbell]

I think the commit message should be:
Bug 453908 - Add FindFolder() and GetOrCreateFolder() to nsMsgUtils. r=aceman

They're free-standing functions, and I used lower-case "msgutils" to avoid the impression there was a nsMsgUtils class (it's just the filename).
How about:

Bug 453908 - Add FindFolder() and GetOrCreateFolder() utility functions

Do I need to upload a new patch to do it? (and would that require another r?)

Comment 131

[Ben Campbell]

Attachment: add-findfolder-2.patch

Just updated commit message. No code changes.

Comment 132

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/5d2d5584e944
rename GetOrCreateFolder() to GetOrCreateJunkFolder(). r=aceman
https://hg.mozilla.org/comm-central/rev/0b1d7c60056b
Add FindFolder() and GetOrCreateFolder() to nsMsgUtils. r=aceman

Comment 133

[Jorg K (CEST = GMT+2)]

Comment on attachment 9038734 [details] [diff] [review]
add-findfolder-2.patch

Sorry, I saw comments #130 and #131 too late. I had the patches lined up already before checking bugmail.

Comment 134

[Jorg K (CEST = GMT+2)]

And to answer the questions: In general changing the commit messages doesn't need another patch, I can do it if I see it on time. And it also doesn't need another review. I change commit messages all the time, only this time I posted to the bug to let Geoff know since he also lands patches. It's not a big deal, however, it would be extremely useful being able to edit commit messages after the event to fix missing/wrong bug numbers, typos, wrong reviewers and other misc. mistakes. But we can't :-(

Comment 135

[Magnus Melin [:mkmelin]]

Aceman, any update on getting these reviewed and landed?

Comment 136

[:aceman]

Comment on attachment 9038456 [details] [diff] [review]
use-findfolder-1.patch

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

Thanks.

Comment 137

[:aceman]

Comment on attachment 9038459 [details] [diff] [review]
remove-direct-rdf-calls-1.patch

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

::: mailnews/base/search/src/nsMsgFilterService.cpp
@@ +614,5 @@
>              !uri.Equals(actionTargetFolderUri))
>          {
> +          nsCOMPtr<nsIMsgFolder> destIFolder;
> +          rv = GetOrCreateFolder(actionTargetFolderUri, getter_AddRefs(destIFolder));
> +          CONTINUE_IF_FAILURE(rv, "Could not get action folder");

Didn't you say you'll change this to FindFolder? We do not want to create missing folders here. You could also drop the parentFolder check below.

::: mailnews/base/src/nsMsgAccountManager.cpp
@@ +1454,4 @@
>        {
> +        nsCOMPtr <nsIMsgFolder> parent;
> +        rv = folder->GetParent(getter_AddRefs(parent));
> +        if (NS_SUCCEEDED(rv) && parent)

So we create the folder in any case, but only if it is real (with parent) we set the flag on it? This is the same as just finding the folder if it exists and setting the flag on it. If that caused failures that means we rely on this creating dangling folders, maybe because we store messages in them later and then the folders stop being dangling. In such a case, I wonder why the folders would not need those flags. The flags may be added on restart in some of our flag cleanup function.

So if we fix this place one day, we will need to choose whether to just create all the folders and with flags, or no.

::: mailnews/base/util/nsMsgUtils.cpp
@@ +875,3 @@
>  
> +  nsCOMPtr<nsIMsgFolder> folder;
> +  rv = GetOrCreateFolder(aURI, getter_AddRefs(folder));

Again, you promised to use FindFolder() here. Even though that was when the function looked quite differently. Now you have again totally changed it around (back to original), dropping the IMAP additions. So why is that?

@@ +878,3 @@
>    NS_ENSURE_SUCCESS(rv, rv);
>  
>    // don't check validity of folder - caller will handle creating it

Which 'caller' is meant here? We created it right above.

Comment 138

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/c789067c3bf3
Replace some GetExistingFolder() uses with FindFolder(). r=aceman

Comment 139

[Ben Campbell]

(In reply to :aceman from comment #137)

::: mailnews/base/search/src/nsMsgFilterService.cpp
@@ +614,5 @@

         !uri.Equals(actionTargetFolderUri))
     {

•      nsCOMPtr<nsIMsgFolder> destIFolder;
  

•      rv = GetOrCreateFolder(actionTargetFolderUri, getter_AddRefs(destIFolder));
  

•      CONTINUE_IF_FAILURE(rv, "Could not get action folder");
  

Didn't you say you'll change this to FindFolder? We do not want to create
missing folders here. You could also drop the parentFolder check below.

My previous attempts at this stuff were much more aggressive about using FindFolder()/GetExistingFolder(), but I kept running into loads of subtle issues with tests failing, I spent ages running around in circles trying to figure them out and ended up just buried in the complexity.

So I stepped back and broke it all down into pure refactoring steps. So this patch was purely to replace all the old remaining GetResource() calls with GetOrCreateFolder(), which should be exactly equivalent.

So, yes, likely a whole bunch of lost opportunities to tighten up the dangling-folder issue, but I'd rather attack that separately. I had no idea how fiddly it'd end up being :-(

::: mailnews/base/src/nsMsgAccountManager.cpp
@@ +1454,4 @@

   {

•    nsCOMPtr <nsIMsgFolder> parent;
  

•    rv = folder->GetParent(getter_AddRefs(parent));
  

•    if (NS_SUCCEEDED(rv) && parent)
  

So we create the folder in any case, but only if it is real (with parent) we
set the flag on it? This is the same as just finding the folder if it exists
and setting the flag on it. If that caused failures that means we rely on
this creating dangling folders, maybe because we store messages in them
later and then the folders stop being dangling. In such a case, I wonder why
the folders would not need those flags. The flags may be added on restart in
some of our flag cleanup function.

So if we fix this place one day, we will need to choose whether to just
create all the folders and with flags, or no.

Again, I've tried to stick to the pure GetResource() -> GetOrCreateFolder() refactor, keeping all the surrounding code, warts and all.
(incidentally, there are a few places where flags are set before the folder is properly created, relying on the dangling-folders behaviour to pick up the correct flags when the folder is created for real...)

::: mailnews/base/util/nsMsgUtils.cpp
@@ +875,3 @@

• nsCOMPtr<nsIMsgFolder> folder;
• rv = GetOrCreateFolder(aURI, getter_AddRefs(folder));

Again, you promised to use FindFolder() here. Even though that was when the
function looked quite differently. Now you have again totally changed it
around (back to original), dropping the IMAP additions. So why is that?

Same again - just dropping the patch back to a pure refactoring step.
The IMAP changes were a kludge. There are so many places where IMAP is handled via special casing. It'd be really nice to unify/simplify the behaviors of the various folder types (eg lots of localmailfolder operations are synchronous, but the IMAP equivalent is async, because it requires the round-trip to the server. This duality leads to a lot of special-casing and complexity). I don't currently see any easy wins to be had, but I'm still thinking.

@@ +878,3 @@

NS_ENSURE_SUCCESS(rv, rv);

// don't check validity of folder - caller will handle creating it

Which 'caller' is meant here? We created it right above.

Old comment left in, warts and all :-) I've an idea that there are some circumstances where the folder can be left dangling (IMAP maybe), in which case it is up to the caller to sort out...
I'd rather like to nuke the entire GetOrCreateJunkFolder() function anyway - I don't see what's so special about Junk that doesn't apply to any of the other special folders.

Comment 140

[:aceman]

Comment on attachment 9038459 [details] [diff] [review]
remove-direct-rdf-calls-1.patch

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

OK then, let's get it landed and then we can discuss the individual cases elsewhere.

Comment 141

[Jorg K (CEST = GMT+2)]

So, how much RDF is left after landing this? Is this the last patch in this bug or is there more to come?

I haven't seen any recent try, so here is one:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=a5e38f8815a488345f08345b5bf560382a781bd1

Comment 142

[Ben Campbell]

There are a couple of places out in javascript land where the RDF code is used for non-folder reasons (feed-parsing was one, off the top of my head). I'll take another look and report back.

On the folder side, I've got two more patches in the pipeline:

1. to remove RDF use from the folder-lookup-service (looking good, and I've done try builds with it which didn't raise any more unittest/mozmill fails, so I just need to tidy up all the debug dump()-spew).

2. to decouple the folders from RDF (remove the inheritance of nsIRDFResource). This was causing me no end of trouble, but I think I had a breakthrough yesterday - I'm just running tests locally, then I'll hit the try servers with it.

Comment 143

[:aceman]

We could call this one off after the 140 comments;) After all, we converted all folder lookups to the lookup service (as the summary says). RDF is still hidden in that service, but we could file followups for the next steps. This one is NOT the meta bug for all RDF removal.

Comment 144

[Magnus Melin [:mkmelin]]

Try looks good, let's land.

Comment 145

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/5fa6fd01e555
Replace direct rdf-service calls in C++ with folder-lookup-service calls. r=aceman DONTBUILD

Comment 146

[Jorg K (CEST = GMT+2)]

OK, please file follow-up bugs that depend on this one here.