377242 - [SoC] Enable Roaming Support in Thunderbird

Status: NEW

(Thunderbird :: General, defect)

Description

[chris hofmann]

Tracking bug for Summer of Code project

Comment 1

[Hendrik Saly]

Roaming should also provide support for remote signatures (see Bug 65574).

Comment 2

[Mark Banner (:standard8)]

Note that the code in extensions/sroaming *may* be a useful place to start/for reference. Though it only works with non-toolkit builds (see bug 378647).

In either case, it'd be really nice if this code could be designed/positioned so that other apps could use it as well (e.g. SeaMonkey).

Comment 3

[Nick Kreeger]

(In reply to comment #2)
> Note that the code in extensions/sroaming *may* be a useful place to start/for
> reference. Though it only works with non-toolkit builds (see bug 378647).
> 
> In either case, it'd be really nice if this code could be designed/positioned
> so that other apps could use it as well (e.g. SeaMonkey).
> 

I agree, the hope is to design the feature so that other mailnews apps (SeaMonkey, Correo, etc) can take advantage as well.

Comment 4

[Ben Bucksch (:BenB)]

Glad to see some interest in picking this up!
See bug 249323 comment 8.

Comment 5

[Ben Bucksch (:BenB)]

FYI, I have very limited time, but if I can help with answering a few questions, I'd be glad to help.

Comment 6

[Magnus Melin [:mkmelin]]

Bug 78858: (ability to store filters on IMAP).

Comment 7

[Nick Kreeger]

Attachment: July 5th Patch

This is just to post some of my progress to the feature.. It listens to events for adding to the addressbook, sending off the notice, parsing the TOC file, exporting the attachments, and (not done yet) begin to move the new files back in.

Comment 8

[Nick Kreeger]

Attachment: July 19th Patch

This is a significant upgrade to the roaming service since the last patch. The focus on this version is to enhance the update message sniffing, processing, and logging. Update messages are tracked by their date in seconds time, it provides a easy (and cheap) way of checking to see if a header has been processed. Messages are then downloaded and saved in the OS temporary directory, under a folder with a corresponding name as the date in seconds time. Once all the attachments for a message have been downloaded, then the files get copied into where they need to go (not in this patch, I'm still thinking about timing). Then the next oldest message is processed so that updates do not get overridden.

As with the previous version of the patch posted here, this only works with the address book and IMAP accounts. There are also some prefs that need to be saved as well to turn on roaming. I will post those here later if there is any interest in giving this thing a spin.

Comment 9

[Nick Kreeger]

Attachment: Aug. 3 Diff

Continued updates, this time I added a class to sync prefs based off of a file from an attachment. There are also numerous updates, including logic for full and delta update messages. I will update the wiki as appropriate this afternoon.

Comment 10

[David :Bienvenu]

Cool, Nick, I'm looking forward to trying this!

Comment 11

[Nick Kreeger]

(In reply to comment #9)
> Created an attachment (id=275149) [details]
> Aug. 3 Diff
> 
> Continued updates, this time I added a class to sync prefs based off of a file
> from an attachment. There are also numerous updates, including logic for full
> and delta update messages. I will update the wiki as appropriate this
> afternoon.
> 

Sorry, I jumped the gun on this patch, the attachment downloading code is broken (again)...

Comment 12

[Nick Kreeger]

Attachment: Aug 15 Patch

Here is an update patch. This patch includes the patch I have made to make the composer listener code work.

Comment 13

[Nick Kreeger]

Attachment: Aug 15 Patch2

Oops, I diff'd off a new tree and forgot to su-do CVS add my new files. Disregard the old patch.... Doh!

Comment 14

[Nick Kreeger]

Attachment: Aug. 19th Diff

This patch introduces full POP3 support, API's for setting the sync server (and turn it on), and fixes some troubles with the AB sync. 

There are still a couple of issues:
1.) UpdateMessageProcessor instances are leaking (I've been tracking this down all weekend)
2.) IMAP sync folder needs to be opened at startup. 
3.) If the Address Book has been opened, the copied in abook.mab isn't loaded until the app restarts. However, if the AB service hasn't been started, the sync works perfect. 
4.) The update message's aren't getting purged (removed) because I have some logic that needs to be finished (messages are getting removed before they are done processing).
5.) Update messages aren't getting marked as read (or hidden from the user yet, this is for easy development sake).

I'm suppose to have a "evaluation" ready patch by 12 PM Pacific today. This is what I'm turning in. I will continue hacking away for the August 31st code submission deadline.

Comment 15

[David :Bienvenu]

cool, I'll look through this...

Comment 16

[Nick Kreeger]

Attachment: Patch V1

Review ready patch. If you want me to, I can #ifdef the code that touches existing modules in the interim.

Comment 17

[David :Bienvenu]

very cool!

+    PRBool useSyncService;
+    nsresult rv = prefBranch->GetBoolPref(MSG_PREF_ENABLESYNC, &useSyncService);
+    if (NS_SUCCEEDED(rv) && useSyncService)

don't need to redeclare rv; it's declared above.

same thing here:

+    PRBool useSyncService;
+    nsresult rv = prefBranch->GetBoolPref(MSG_PREF_ENABLESYNC, &useSyncService);

+ *   Nick Kreeger <[e-mail]>
+ * Portions created by the Initial Developer are Copyright (C) 1998
+ * the Initial Developer. All Rights Reserved.

should be 2007 (this occurs more than once)

+  [noscript] boolean IsUpdateMessageHdr(in nsIMsgDBHdr aMsgHdr);
+  [noscript] void FoundUpdateMessage(in nsIMsgDBHdr aMsgHdr);

why [noscript]?

+    nsresult rv = prefBranch->GetBoolPref(MSG_PREF_ENABLESYNC, &useSyncService);
+    if (NS_SUCCEEDED(rv) && useSyncService)
+      nsCOMPtr<nsIMsgRoamingService> roamingService = do_GetService(NS_MSGROAMINGSERVICE_CONTRACTID);


maybe a comment why we're causing the roaming service to get instantiated here?

+nsresult GetContentTypeForFileName(nsACString &inFileName, nsACString &aFileType)
+{
+  nsCAutoString fileType;
+  nsCAutoString fileName(inFileName);
+  
+  if (fileName.Equals("abook.mab"))
+    fileType.AppendLiteral("application/x-rdf");
+  
+  return NS_OK;
+

this doesn't look right - do we want to be filling in aFileType here?

Comment 18

[David :Bienvenu]

more later - just checkpointing...

Comment 19

[David :Bienvenu]

+  void SetRoamingServer(in nsIMsgIncomingServer inIncomingServer);
+  
+  /**
+   * Get the incoming server that the roaming service is using for bootstrapping the
+   * update messages with.
+   * @return The incoming server the roaming service is using. If one is not set, then
+   *         nsnull will be returned.
+   */
+  nsIMsgIncomingServer GetRoamingServer();


why not just attribute nsIMsgIncomingServer roamingServer?

same here:

+  /**
+   * Toggle the roaming service on or off.
+   * @param inShouldSync State to set the roaming service.
+   */
+  void SetShouldSync(in boolean inShouldSync); 
+  
+  /**
+   * Get the current roaming sync state.
+   * @return The state of the roaming service.
+   */
+  boolean GetShouldSync();
+

attribute boolean shouldSync;



+  nsMsgRoamingService   *mMsgRoamingServiceRef;  // weak ref
...
+  NS_IF_ADDREF(mMsgRoamingServiceRef = aMsgRoamingService);

this looks like a strong ref, and I don't see it getting released anywhere...

+// Pick one or the other listener
+NS_IMPL_ISUPPORTS5(nsMsgRoamingService, nsIMsgRoamingService, nsITimerCallback, 
+                   nsIObserver, nsIUrlListener, nsIMsgSendListener)
+

Is that comment still valid, or do you need both?

+  if (mCreateFullUpdateMsg)
+    rv = GetFullUpdateMsgSubject(subjectStr);
+  else
+    rv = GetDeltaUpdateMsgSubject(subjectStr);

this can be rv = mCreateFullUpdateMsg ? GetFullUpdateMsgSubject(subjectStr) : GetDeltaUpdateMsgSubject(subjectStr);

why does base/src/Makefile.in need these two include dirs? (especially profile)
+		  profile \
+		  intl \

So if the sync pref is turned off, we won't init the roaming service, and it will be pretty much passive?

I'd like to give this a try, but I'm thinking applying a patch with these comments addressed would avoid a round of backing out the patch and applying a new one.

Comment 20

[David :Bienvenu]

Comment on attachment 279759 [details] [diff] [review]
Patch V1

minusing in favor of patch with comments addressed.

Comment 21

[Nick Kreeger]

Attachment: Patch V2

Here's a followup patch. Sorry for the delay, got stuck moving the past week or so.

> why does base/src/Makefile.in need these two include dirs? (especially profile)
> +                 profile \
> +                 intl \
> 

We need 'profile' for access to the profile name (used in the update message subject line).
We also need 'intl' for access to the string bundle service, to access the branded name of the application (also used in the subject line of an update message).

> So if the sync pref is turned off, we won't init the roaming service, and it
> will be pretty much passive?
> 

In the current implementation, yes.

If we want to be able to prompt the user that roaming information exists on an account, we can remove the pref-checks in the storage classes. However, before we can do this, I think I'm going to need to add some additional information to the full-update message.

Comment 22

[Nick Kreeger]

Attachment: Roaming Setup Extension

Here is the XUL roaming setup extension for Thunderbird. It adds a menu item to the "Tools" menu called "Roaming Settings...".

You can turn on the roaming service, choose the account to use as a bootstrapper for update messages, and pick what type of data to sync (currently just prefs and addressbook).

This is my first attempt at making a XUL extension, so if something is broke, please be sure to note it here. I think it turned out pretty decent considering my crash-course on XUL over the past couple of days ;-)

P.S. You will have to install this one by hand in Tbird (sorry).

Comment 23

[Nick Kreeger]

> P.S. You will have to install this one by hand in Tbird (sorry).
>

Using the supplied Add-Ons manager of course... 

Comment 24

[David :Bienvenu]

there's a small case problem with the extension - curServerUri vs curServerURI, or something like that, but it breaks the account picker because of the js exception.

Comment 25

[David :Bienvenu]

In nsresult nsMsgRoamingService::Init()

it should be

  nsCOMPtr<nsIMsgFolder> rootFolder;
  rv = mSyncServer->GetRootFolder(getter_AddRefs(rootFolder));
  NS_ASSERTION(NS_SUCCEEDED(rv), "### Could not get the specified sync folder..!");


instead of

  nsCOMPtr<nsIMsgFolder> rootFolder;
  rv = mSyncServer->GetRootFolder(getter_AddRefs(rootFolder));
  NS_ASSERTION(rv, "### Could not get the specified sync folder..!");

because rv == 0 is success.

I'm having some problems getting this working. I set up two profiles to use roaming, and to sync both prefs and address books, using the same pop3 account. Then I added a card to one address book and shut down. It looks like we try to send the update notification on shutdown, which is problematic unless we block shutdown from happening while we're sending, because components get torn down from under us while the send is happening. Doing things on shutdown is problematic - one approach is to open a window, perhaps hidden, and leave it open until we've finished the operation, which should prevent the app from exiting. I'll try to dig a little deeper into that.


I did get a couple full update messages to get sent,  but I don't see them get applied to the profile they're downloaded in... should this scenario work?

Comment 26

[Nick Kreeger]

Attachment: Roaming Setup Extension V2

Here is an updated extension package that fixes the JS bustage.

Comment 27

[Ben Bucksch (:BenB)]

> Doing things on shutdown is problematic

Yes. I hit the same problem with my "session roaming", which inherently relies on that concept. Better is (both for the user and to sovle this problem) to sync immediately when things are changed, e.g. directly after the address in the abook has been added or edited, similar with other data types you sync (which is the hard part).

Comment 28

[David :Bienvenu]

See bug 397498 - while I agree that doing too much at shutdown could be annoying for the user, it's pretty much unavoidable to have to sometimes do things at shutdown, so Nick is creating a general mechanism that mailnews components can use to do asynchronous things at shutdown.

Comment 29

[Nick Kreeger]

Attachment: Roaming Setup Extension V3

Here is a new extension that makes the dialog a real dialog!

Comment 30

[Nick Kreeger]

Attachment: Patch V3

Here is the next go-round of the roaming patch. There isn't any architectural changes in this patch with the exception of wiring up the roaming service to the shiny new shutdown service.

Comment 31

[Nick Kreeger]

+  rv = directoryService->Get(NS_OS_TEMP_DIR, NS_GET_IID(nsIFile), getter_AddRefs(tempDirFile));
+  NS_ENSURE_SUCCESS(rv, rv);
+  
+  nsCOMPtr<nsIFile> syncFileDir;
+  rv = tempDirFile->Clone(getter_AddRefs(syncFileDir));
+  NS_ENSURE_SUCCESS(rv, rv);
+  syncFileDir->AppendNative(NS_LITERAL_CSTRING(MSG_ROAMINGSYNCFILEFOLDER));
+  
+  PRBool exists;
+  syncFileDir->Exists(&exists);
+  if (!exists)
+    syncFileDir->Create(nsIFile::DIRECTORY_TYPE, 0600);

I'm noticing that trying to create a directory in the designated temporary directory of my Ubuntu Linux distribution won't give me the correct permissions to do the work that needs to be done by the roaming service. I've tried playing with the permissions octal - still no luck. I'm thinking about doing all the sync work in the profile directory (maybe just for Linux..).

Comment 32

[Nick Kreeger]

(In reply to comment #31)
> +  rv = directoryService->Get(NS_OS_TEMP_DIR, NS_GET_IID(nsIFile),
> getter_AddRefs(tempDirFile));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  
> +  nsCOMPtr<nsIFile> syncFileDir;
> +  rv = tempDirFile->Clone(getter_AddRefs(syncFileDir));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  syncFileDir->AppendNative(NS_LITERAL_CSTRING(MSG_ROAMINGSYNCFILEFOLDER));
> +  
> +  PRBool exists;
> +  syncFileDir->Exists(&exists);
> +  if (!exists)
> +    syncFileDir->Create(nsIFile::DIRECTORY_TYPE, 0600);
> 
> I'm noticing that trying to create a directory in the designated temporary
> directory of my Ubuntu Linux distribution won't give me the correct permissions
> to do the work that needs to be done by the roaming service. I've tried playing
> with the permissions octal - still no luck. I'm thinking about doing all the
> sync work in the profile directory (maybe just for Linux..).
> 

Disregard.. I'm an idiot ;-)

Comment 33

[Nick Kreeger]

Attachment: Patch V4

Here is another roaming update patch. This time, I tried a different class for listening to AB changes - so data can be refreshed at real time.

The roaming service also has start/stop methods in this patch to work with the extension better.

I'm also thinking about syncing conflicts at this stage - 'cause I know they are going to happen...

Comment 34

[Nick Kreeger]

Attachment: Patch V5

This is another upgrade patch to V4. This time, I fixed most of the address book syncing issues by implementing a basic sync-conflict prompt to the roaming service. A listening class can inform the roaming service that there is a conflict in the data and the user can choose to either override, discard, or merge (optional to the calling class).

The only issue that remains is the ability to refresh the AB view when the address book window is open. For testing, make your changes - then be sure to close the address book window. I'm working on finding a way to inform the XUL tree to refresh.....

Comment 35

[Mark Banner (:standard8)]

 nsresult nsAbMDBDirectory::NotifyItemChanged(nsISupports *item)
 {
   nsresult rv;
   nsCOMPtr<nsIAddrBookSession> abSession = do_GetService(NS_ADDRBOOKSESSION_CONTRACTID, &rv);
   NS_ENSURE_SUCCESS(rv,rv);
 
   rv = abSession->NotifyItemPropertyChanged(item, nsnull, nsnull, nsnull);
   NS_ENSURE_SUCCESS(rv,rv);
+  
+  NotifyRoamingService();
   return rv;
 }

So, why can't the roaming service just hook into the nsIAddrBookSession notifications if its enabled? At the moment this has a perf impact on the current notification methods even if the roaming service isn't enabled. Its also another notification method that we need to maintain and not forget (which if it was hooked into nsIAddrBookSession would be easier to manage).

It also means that it is more difficult for extensions to provide an address book type and have that synced via the roaming service as well (I don't know if any do yet, but even if there aren't any, we'd have to reimplement this bit on each ab type we want to support e.g. migrating away from mork).

(In reply to comment #34)
> The only issue that remains is the ability to refresh the AB view when the
> address book window is open. For testing, make your changes - then be sure to
> close the address book window. I'm working on finding a way to inform the XUL
> tree to refresh.....

My instinct here would be to add another nsIAddrBookSession based notification that is called when the database is reloaded. nsAbView can then hook onto this notification (easily because it already hooks onto nsIAddrBookSession). You can then check if the nsAbView is currently displaying the updated directory.

You probably need to be slightly careful with mailing list changes (i.e. add/remove of lists). You'd need to get the RDF trees to refresh somehow.

Comment 36

[Nick Kreeger]

> So, why can't the roaming service just hook into the nsIAddrBookSession
> notifications if its enabled? At the moment this has a perf impact on the
> current notification methods even if the roaming service isn't enabled. Its
> also another notification method that we need to maintain and not forget (which
> if it was hooked into nsIAddrBookSession would be easier to manage).

When we originally started to plan the service, we wanted to try and keep as many things generic as possible to allow for new data types to be added to the sync queue without having to throughly change nsMsgRoamingService. While there are exceptions right now, we don't really want to have the roaming service listen for AB changes...

Instead, I have been rethinking the communication system between the roaming service and the data classes. Rather than having the data classes inform the roaming service of changes, whenever the roaming service decides its time to think about sending an update message - it asks the listening data classes through the |nsIObserver| interface. This removes the problems that have been pointed out above. Since this version of AB syncing is meant to only sync mork-databases - we decided to keep the code in that class since it gives us easy access to the actual |nsIAddrDatabase| and makes it easy to use the fancy file APIs.

> It also means that it is more difficult for extensions to provide an address
> book type and have that synced via the roaming service as well (I don't know if
> any do yet, but even if there aren't any, we'd have to reimplement this bit on
> each ab type we want to support e.g. migrating away from mork).

I'm not aware of any address book extensions.. Do you have a link to one so that I can look into this?

Comment 37

[Nick Kreeger]

Attachment: Patch V6

This patch reworks the notification system of the roaming service. The service will ask the callers if they have any changes via the |nsIObserver| interface. I've also reworked the signature of the "add-listener" function for the roaming service to take in information about the data-types sync file. This way the roaming service can be kept much more generic, rather than having statically typed definitions and file information in the service itself.

I also have a new version of the extension, which I will post shortly.

Comment 38

[Nick Kreeger]

Attachment: Roaming Extension V4

Comment 39

[David :Bienvenu]

I haven't had a chance to run this, but some initial comments:

Wow, this is a lot of work!

wrong indentation here:
+    if (shouldSync)
+    StartListening();

Does this leak memory? Does GetCharPref allocate memory? If so, I think you want prefValue.Adopt(...)

+      rv = prefBranch->GetCharPref(curPrefName.get(), &prefStr);
+      if (NS_SUCCEEDED(rv))
+        prefValue.Assign(prefStr);
+      break;



Here, I think you can just use a break statement instead of the doSearch var
+  PRBool doSearch = PR_TRUE;
+  PRInt32 arrSize = mRoamingListeners.Count();
+  for (PRInt32 i = 0; i < arrSize && doSearch; i++)
+  {
+    RoamingListener *roamingListener = mRoamingListeners[i];
+    if (roamingListener->observer == aObserver && roamingListener->syncTopic.Equals(aTopic))
+    {
+      mRoamingListeners.RemoveObject(roamingListener);
+      doSearch = PR_FALSE;
+    }
+  }


If you want native line endings (and I think you do), you could use MSG_LINEBREAK or something like that...

+    changedFileContent.Append('\n'); // Use a OS-independent call here.


+  nsString fullMessage, message1, message2;
+  roamingStrBundle->GetStringFromID(SYNC_CONFLICT_DESC1, getter_Copies(message1));
+  roamingStrBundle->GetStringFromID(SYNC_CONFLICT_DESC2, getter_Copies(message2));
+  fullMessage.Append(message1);
+  fullMessage.Append(' ');
+  fullMessage.Append(inDataName);
+  fullMessage.Append(message2);

You can't assume that all languages will want inDataName at the end of the sentence. You should do something like:

+1001=There has been a sync conflict with the %S

and use a string formatting routine to generate the string...

Comment 40

[hansen]

Bienvenu: With all the work done here, will you put it in your todo after 3.0?

Comment 41

[David :Bienvenu]

Comment on attachment 291749 [details] [diff] [review]
Patch V6

clearing obsolete review request. This patch has a some cool stuff in it, but I don't know if it's going to get picked up.

Comment 42

[Dan Mosedale (:dmosedale, :dmose)]

David, what exactly would the next steps be for someone who is interested in respoonding to the helpwanted keyword?

Comment 43

[David :Bienvenu]

de-bitrotting the patch, and addressing my review comments, for a start.

Comment 44

[Gervase Markham [:gerv]]

Does/would Weave not do the same job/be a better direction to take things in for "roaming profile"-type support?

Gerv

Comment 45

[David :Bienvenu]

Weave integration is probably a better way to go - but one advantage of the direction of this patch is that it doesn't require any server other than your mail server, which you already have...