499306 - Backend for retention-policy UI not initialized, tangled with XUL code

Status: RESOLVED FIXED

(MailNews Core :: Account Manager, defect)

Description

[rsx11m]

As bug 326584 comment #27 and following have shown, the retention policy settings (shared by the Synchronization & Storage pane and Folder Properties) are heavily tied into the XUL code, which essentially glues the onInit() and onSave() ends to retain preference values. Thus, it is not possible to remove UI elements without damaging the underlying preferences. Following analysis from bug 326584:

> - nsMsgRetentionSettings doesn't initialise any of its member variables [1].
> - The retention code for loading and saving prefs creates and uses separate
> nsIMsgRetentionSettings instances - not transferring the existing prefs between
> them, see [2] and the code that uses about it.
> 
> The uninitialised nature of nsMsgRetentionSettings then means that in saving
> the prefs in [3] we're using an uninitialised value for keepUnreadOnly.
> 
> [1] http://mxr.mozilla.org/comm-central/source/mailnews/db/msgdb/src/nsMsgDatabase.cpp#4866
> [2] http://mxr.mozilla.org/comm-central/source/mailnews/base/resources/content/retention.js
> [3] http://mxr.mozilla.org/comm-central/source/mailnews/db/msgdb/src/nsMsgDatabase.cpp#4483

> In am-offline.js, onInit() uses initRetentionSettings() to set the UI values
> from gIncomingServer using its own retentionSettings instance. In onSave(),
> another retentionSettings is created from saveCommonRetentionSettings() which
> knows nothing about the initial settings and only retrieves those from the UI
> status. This instance is then assigned to gIncomingServer.retentionSettings,
> thus leaving retentionSettings.keepUnreadMessagesOnly in an undefined state.
> In folderProps.js, the same thing happens in folderPropsOKButton() with
> gMsgFolder.retentionSettings.
> 
> As a workaround to the problem, keepUnreadMessagesOnly could be initialized
> from gIncomingServer and gMsgFolder before assigning the new instance back,
> assuming those won't change between onInit() and onSave().

The latter did not work, as stated in bug 326584 comment #28. Thus, potential charge for the bug here would be to utilize some other/better mechanism for the preferences (e.g., many pages use prefstring="mail.server.%serverkey%.xxx" instead for tying UI with the preferences, which may not work for the Folder Properties though). Also, the retention-policy variables should be initialized with a "safe" value to avoid unpredictable behavior and potential dataloss.

Fixes here may possibly also solve the related bug 472203 and/or bug 499303.

Comment 1

[rsx11m]

Based on bug 464355 comment #8 and following, the introduction of default preferences "mail.server.default.xxx" in favor of hard-wired defaults at several different locations should be considered. It is my understanding that the generic "%serverkey%" mechanism takes those into account.

Comment 2

[Samuel Sidler (old account; do not CC)]

Not going to block a 1.9.0 release on a mailnews bug because we don't have any mailnews products that ship on 1.9.0... Unless I'm missing something?

Comment 3

[Mark Banner (:standard8)]

Attachment: WIP fix

This should be a reasonable fix - the general idea here is when saving the retention settings, use the existing settings as a base for that. This should allow us to remove the retention.keepUnread checkbox safely.

In some ways I'd probably split retention settings up, but I don't think that is really necessary - maybe I just need to add a warning onto the idl to ensure the retention settings object is initialised.

Not tested this yet (and not removed the retention.keepUnread checkbox yet).

Comment 4

[Mark Banner (:standard8)]

Attachment: Proposed fix

I think this is about the best we can do for this without reworking the interfaces completely. This patch:

- Initialises the member variables in nsMsgRetentionSettings to reasonable defaults.
- Requires that the previous retentionSettings are passed to saveCommonRetentionSettings so that they are just altered and then saved to disk. Hence existing ones not altered will stay the same.
- Removes the hidden UI and strings that had to be kept as a result of the problems (see bug 326584).

Comment 5

[David :Bienvenu]

Comment on attachment 396192 [details] [diff] [review]
Proposed fix

this looks reasonable.

Comment 6

[rsx11m]

(In reply to comment #4)
> - Removes the hidden UI and strings that had to be kept as a result of the
> problems (see bug 326584).

Per discussion in bug 326584 comment #17 and following, there may be use cases for retaining the hidden UI for the purpose of users migrating profiles who use this function, specifically to access it in the Folder Properties (no pref in the Config Editor for that attribute on a per-folder basis, thus potentially difficult to verify or change that setting manually).

Comment 7

[Mark Banner (:standard8)]

Attachment: Simpler Fix

Simpler version - just protect against retention settings being uninitialised. This will allow appropriate userChrome.css should anyone require it.

If we go with this, then I'll file a bug to completely remove the UI in Thunderbird 3.next (and possibly the backend support for it?). If they haven't fixed the value in Thunderbird 3 then that's up to them, but we shouldn't carry the extra load unnecessarily.

Comment 8

[rsx11m]

I agree with comment #7 and would suggest to at least remove the backend for the folder-based keep-unread-only setting before making the UI unaccessible. Deferring this to 3.next sounds like a good approach.

Comment 9

[Mark Banner (:standard8)]

Patch checked in: http://hg.mozilla.org/comm-central/rev/1268379f1c02

Raised bug 514762 for the final removal.