Convert rdf consumers to use the folder-lookup service

RESOLVED FIXED in Thunderbird 67.0

Status

defect
RESOLVED FIXED
11 years ago
5 months ago

People

(Reporter: jminta, Assigned: benc)

Tracking

(Blocks 2 bugs)

Trunk
Thunderbird 67.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments, 28 obsolete attachments)

57.59 KB, patch
standard8
: review+
Bienvenu
: superreview+
Details | Diff | Splinter Review
63.37 KB, patch
aceman
: review+
Details | Diff | Splinter Review
6.43 KB, patch
aceman
: review+
Details | Diff | Splinter Review
4.06 KB, patch
aceman
: review+
Details | Diff | Splinter Review
2.68 KB, patch
aceman
: review+
Details | Diff | Splinter Review
10.79 KB, patch
aceman
: review+
Details | Diff | Splinter Review
95.43 KB, patch
aceman
: review+
aceman
: feedback+
Details | Diff | Splinter Review
Posted patch crashing patch (obsolete) — Splinter Review
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
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.
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.
See bug 424024 comment #7 for a similar problem.
(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.
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.
Assignee: nobody → jminta
Attachment #337150 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #337285 - Flags: superreview?(bienvenu)
Attachment #337285 - Flags: review?(bienvenu)
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. :-(
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...
this makes the account manager use the folder lookup service (this is really jminta's patch - I'm just trying to drive it in)
Attachment #345228 - Flags: superreview?(bienvenu)
Attachment #345228 - Flags: review?(bugzilla)
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 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.
Attachment #345228 - Flags: review?(bugzilla)
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)
Attachment #345228 - Attachment is obsolete: true
Attachment #345284 - Flags: review?(bugzilla)
Attachment #345228 - Flags: superreview?(bienvenu)
Attachment #345284 - Flags: review?(bugzilla) → review-
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.
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.
Attachment #345284 - Attachment is obsolete: true
Attachment #345812 - Flags: review?(bugzilla)
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).
Attachment #345812 - Flags: review?(bugzilla) → review-
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.
Attachment #345812 - Flags: review- → review?(bugzilla)
Attachment #345812 - Flags: review?(bugzilla) → review+
Attachment #345812 - Flags: superreview+
Comment on attachment 345812 [details] [diff] [review]
[checked in] convert account settings v2

Landed as rev 814:8c591aa8bc9d
Attachment #345812 - Attachment description: convert account settings v2 → [checked in] convert account settings v2
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]
(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...
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.
(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:
Whiteboard: [patchlove][has patch]
Attachment #337285 - Attachment is obsolete: true
Attachment #337285 - Flags: superreview?(bienvenu)
Attachment #337285 - Flags: review?(bienvenu)
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.
Attachment #337285 - Attachment is obsolete: false
Attachment #337285 - Attachment description: patch v1 → [needs update & review] Convert many consumers
Pretty sure Joey isn't working on this any longer.
Assignee: jminta → nobody
Status: ASSIGNED → NEW
Keywords: helpwanted
I have refreshed the patch and now running try builds.
Assignee: nobody → acelists
Status: NEW → ASSIGNED
Posted patch C++ patch (obsolete) — Splinter Review
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?
Attachment #337285 - Attachment is obsolete: true
Attachment #8854310 - Flags: review?(rkent)
Attachment #8854310 - Flags: feedback?(jorgk)
Posted patch JS patch (obsolete) — Splinter Review
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
Attachment #8854311 - Flags: review?(mkmelin+mozilla)
Attachment #8854311 - Flags: feedback?(jorgk)
(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).
Blocks: 1354011
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 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.
Attachment #8854311 - Flags: feedback?(jorgk) → feedback+
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.).
Attachment #8854310 - Flags: feedback?(jorgk)
Yes, I just unbitrotted this. I'll see if I can address Kent's comments.
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.
Attachment #8854310 - Flags: review?(rkent) → review-
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 on attachment 8854311 [details] [diff] [review]
JS patch

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

LGTM! r=mkmelin
Attachment #8854311 - Flags: review?(mkmelin+mozilla) → review+
So just comment 28 / comment 33 to go?
Flags: needinfo?(acelists)
Keywords: helpwanted
Whiteboard: [patchlove][has patch]
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.
Flags: needinfo?(acelists)
aceman/jörg: is all wip work attached to this bug?

I think benc is going to look at it eventually
Assignee: acelists → benc
Status: ASSIGNED → NEW
(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.
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
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!
(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.
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 ;)
(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
Posted patch WIP-fls1.patch (obsolete) — Splinter Review
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.
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?
Attachment #9012812 - Attachment is obsolete: true
Posted patch part3_use_fls_in_c++_1.patch (obsolete) — Splinter Review
Attachment #8854310 - Attachment is obsolete: true
Posted patch part4_use_fls_in_js_1.patch (obsolete) — Splinter Review
Attachment #8854311 - Attachment is obsolete: true
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.
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.
(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).
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?
(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.
(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.
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).
Posted patch js-use-fls.patch (obsolete) — Splinter Review
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)
Attachment #9016146 - Attachment is obsolete: true
Attachment #9016147 - Attachment is obsolete: true
Attachment #9016148 - Attachment is obsolete: true
Attachment #9016149 - Attachment is obsolete: true
Attachment #9019923 - Flags: review?(geoff)
You do not need to do 5) and 6) in this bug :)
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 :-)
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
(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).
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.
Posted patch cpp-use-fls.patch (obsolete) — Splinter Review
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
Attachment #9020259 - Flags: review?(jorgk)
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 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 //.
Attachment #9020259 - Flags: review?(jorgk)
Attachment #9020259 - Flags: review?(acelists)
Attachment #9020259 - Flags: feedback+
Attachment #9019923 - Flags: review?(acelists)
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.
Attachment #9019923 - Flags: review?(geoff) → review+
Posted patch js-use-fls-2.patch (obsolete) — Splinter Review
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?
Attachment #9019923 - Attachment is obsolete: true
Attachment #9019923 - Flags: review?(acelists)
Attachment #9020670 - Flags: review?(geoff)
(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),
Posted patch cpp-use-fls-2.patch (obsolete) — Splinter Review
Attachment #9020259 - Attachment is obsolete: true
Attachment #9020259 - Flags: review?(acelists)
Attachment #9020671 - Flags: review?(acelists)
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.
Attachment #9020670 - Flags: review?(geoff) → review+
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
(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 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]
Attachment #9020671 - Flags: review?(acelists) → feedback+
Status: NEW → ASSIGNED
Component: General → Backend
Product: Thunderbird → MailNews Core
Depends on: 1503438
Posted patch cpp-use-fls-3.patch (obsolete) — Splinter Review
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.
Attachment #9020671 - Attachment is obsolete: true
Attachment #9021690 - Flags: feedback?(acelists)
(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
(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!
Posted patch cpp-use-fls-4.patch (obsolete) — Splinter Review
rebased version of the c++ patch.
Attachment #9021690 - Attachment is obsolete: true
Attachment #9021690 - Flags: feedback?(acelists)
Attachment #9021970 - Flags: feedback?(acelists)
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.
Flags: needinfo?(benc)
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?
Attachment #9020670 - Flags: feedback?(acelists)
(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.
Flags: needinfo?(benc)
(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.
Posted patch js-use-fls-3.patch (obsolete) — Splinter Review
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
Attachment #9020670 - Attachment is obsolete: true
Attachment #9020670 - Flags: feedback?(acelists)
Attachment #9022812 - Flags: review?(geoff)
(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 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'
(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.
(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...
Attachment #9022812 - Flags: review?(geoff) → review+
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).
Can you post what you have, perhaps someone can help spot the problem?
Posted patch js-use-fls-4.patch (obsolete) — Splinter Review
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?
Attachment #9022812 - Attachment is obsolete: true
Attachment #9025981 - Flags: review?(geoff)
Posted patch cpp-use-fls-5.patch (obsolete) — Splinter Review
And here's the C++ patch, as it stands (also rebased, but still with the two unit test failures).
Attachment #9021970 - Attachment is obsolete: true
Attachment #9021970 - Flags: feedback?(acelists)
Which are the failing tests?
We can surely land partial patches to avoid bitrot, but they must not cause test failures.
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?
Attachment #9025981 - Flags: review?(geoff) → review+
(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!)
(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 :-)
Posted patch js-use-fls-5.patch (obsolete) — Splinter Review
Attachment #9025981 - Attachment is obsolete: true
Attachment #9026006 - Flags: review?(geoff)
(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
Attachment #9026006 - Flags: review?(geoff) → review+
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 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.
Attachment #9026006 - Flags: review+
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
Attachment #9026006 - Attachment is obsolete: true
Depends on: 1508851
Attachment #9026203 - Flags: review?(acelists)
Comment on attachment 9026203 [details] [diff] [review]
js-use-fls-6.patch [checked in, comment #104]

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

Thanks.
Attachment #9026203 - Flags: review?(acelists) → review+
OK, if everyone's OK with it, I'd like to submit my `js-use-fls-6.patch` for landing.
Keywords: checkin-needed
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/2c8a0efaccfe
Convert RDF consumers to use the folder-lookup service in JavaScript code. r=darktrojan,aceman
Posted patch cpp-use-fls-6.patch (obsolete) — Splinter Review
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.
Attachment #9025983 - Attachment is obsolete: true
Posted patch cpp-use-fls-7.patch (obsolete) — Splinter Review
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).
Attachment #9030192 - Attachment is obsolete: true
Posted patch cpp-use-fls-8.patch (obsolete) — Splinter Review
Eighth time's the charm!

Try build:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=6fa14c7b095e3fe60d1cb54d560090d1e08c27e2
Attachment #9031330 - Attachment is obsolete: true
Attachment #9031331 - Flags: review?(acelists)
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 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?

Attachment #9031331 - Flags: review?(acelists)
Posted patch cpp-use-fls-9.patch (obsolete) — Splinter Review

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

Attachment #9031331 - Attachment is obsolete: true
Attachment #9035213 - Flags: review?(acelists)

(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 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.

Attachment #9035213 - Flags: review?(acelists)
Posted patch cpp-use-fls-10.patch (obsolete) — Splinter Review

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

Attachment #9035213 - Attachment is obsolete: true
Attachment #9035248 - Flags: review?(acelists)
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?
Attachment #9035248 - Flags: review?(acelists) → feedback+
Posted patch cpp-use-fls-11.patch (obsolete) — Splinter Review
Attachment #9035248 - Attachment is obsolete: true

(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

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)

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

Attachment #9037881 - Flags: review?(acelists)

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).

Attachment #9037882 - Flags: review?(acelists)

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.

Attachment #9038455 - Flags: review?(acelists)

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)

Attachment #9038456 - Flags: review?(acelists)

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

Attachment #9038459 - Flags: review?(acelists)
Attachment #9037881 - Flags: review?(acelists) → review+
Attachment #9037882 - Flags: review?(acelists) → review+
Attachment #9038455 - Flags: review?(acelists) → review+
Attachment #9026203 - Attachment description: js-use-fls-6.patch → js-use-fls-6.patch [checked in, comment #104]

Can we land some of this?

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/a770f9f6ca81
remove some left-over direct RDF uses from JavaScript code. r=aceman
Attachment #9038455 - Attachment description: mop-up-leftover-js-rdf-use-1.patch → mop-up-leftover-js-rdf-use-1.patch [checked in, comment #125]

(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).

Keywords: checkin-needed

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

(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

Attachment #9037455 - Attachment is obsolete: true
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

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?)

Posted patch add-findfolder-2.patch (obsolete) — Splinter Review

Just updated commit message. No code changes.

Attachment #9037882 - Attachment is obsolete: true
Attachment #9038734 - Flags: review+

Pushed by mozilla@jorgk.com:
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

Keywords: checkin-needed
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.
Attachment #9038734 - Attachment is obsolete: true
Attachment #9037881 - Attachment description: rename-getorcreatefolder-1.patch → rename-getorcreatefolder-1.patch [checked in, comment #132]
Attachment #9037882 - Attachment description: add-findfolder-1.patch → add-findfolder-1.patch [checked in, comment #132]
Attachment #9037882 - Attachment is obsolete: false

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 :-(

Aceman, any update on getting these reviewed and landed?

Flags: needinfo?(acelists)
Comment on attachment 9038456 [details] [diff] [review]
use-findfolder-1.patch

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

Thanks.
Attachment #9038456 - Flags: review?(acelists) → review+
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.
Attachment #9038459 - Flags: feedback+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/c789067c3bf3
Replace some GetExistingFolder() uses with FindFolder(). r=aceman

(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 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.
Attachment #9038459 - Flags: review?(acelists) → review+

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

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.

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.

Flags: needinfo?(acelists)

Try looks good, let's land.

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

Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 67.0

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

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