Closed Bug 1152989 Opened 5 years ago Closed 5 years ago

Account Manager Extensions broken in Thunderbird 37/38

Categories

(MailNews Core :: Account Manager, defect, major)

defect
Not set
major

Tracking

(thunderbird38+ fixed, thunderbird39 fixed, thunderbird40 fixed)

RESOLVED FIXED
Thunderbird 40.0
Tracking Status
thunderbird38 + fixed
thunderbird39 --- fixed
thunderbird40 --- fixed

People

(Reporter: brummolix, Assigned: aceman)

References

Details

Attachments

(1 file, 1 obsolete file)

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:37.0) Gecko/20100101 Firefox/37.0
Build ID: 20150402191859

Steps to reproduce:

I develop an add-on:AutoarchiveReloaded, see https://addons.mozilla.org/de/thunderbird/addon/autoarchivereloaded/

The add-on adds options to accounts.
It was done as written in:
https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/NsIMsgAccountManagerExtension/Building_an_Account_Manager_Extension

In Thunderbird Thunderbird 38 it is broken (and also in TB 37)
If I open the properties of an account it says "Invalid account NAME_OF_ACCOUNT"
If I disable the add-on everything is fine with the properties.

In the error console it says:
Fehler: Error accessing account XXX: [Exception... "Component returned failure code: 0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE) [nsIJSCID.getService]"  nsresult: "0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE)"  location: "JS frame :: chrome://messenger/content/AccountManager.js :: at_build :: line 1568"  data: no]
Quelldatei: chrome://messenger/content/AccountManager.js
Zeile: 1583

According to a discussion in https://forums.mozilla.org/viewtopic.php?f=15&t=24177 "it looks like your component needs to implement nsIMsgAccountManagerExtension"
But it does, it has:
QueryInterface:XPCOMUtils.generateQI([Components.interfaces.nsIMsgAccountManagerExtension])

Is this an error of TB or do I have to change the add on?
Will there be any update of the example https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/NsIMsgAccountManagerExtension/Building_an_Account_Manager_Extension to reflect changes which have to be done?


Actual results:

account properties are broken


Expected results:

account properties should be there
The "Invalid account" message was implemented in bug 813929 to protect from broken accounts.
The whole set up of the account panels is inside a try{}catch{} block to catch exceptions from misbehaving accounts.

I confirm that the extension triggers the message.

We need to find out why your extension triggers throws the error. However, bug 813929 was done for TB29. Can you test in which TB version your problem started? If it isn't 29 then it is not a problem of bug 813929 and something probably changed in the account manager extensions registration.
Assignee: nobody → acelists
Blocks: 813929
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 8.1 → All
Product: Thunderbird → MailNews Core
Hardware: x86_64 → All
I can try to improve the AM in this case to only reject the extension-specified panels if they do not work. The standard configuration panels could still be shown.
Why do you have this in your extension, in am-autoarchiveprefs.xul:
		<description/>
		<description id="description"/>
		<description id="description2"/>
		<script type="application/x-javascript">
		//we must set this elements this way, otherwise the text is not multiline (automatic word wrap)
		//sad, sad...
		document.getElementById('description').textContent = strbundle.getString("settingsDescription");
		document.getElementById('description2').textContent = strbundle.getString("settingsDescription2");
		</script>

What is the first <description/> without ID used for?

For the other 2 descriptions, didn't <description>&settingsDescription;</description> work? See https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/description#Examples . We surely have properly wrapping multiline descriptions in the AM, e.g. in the Junk panel.

For the "id=*Days" <textbox>es tied to preftype="int", wouldn't type="number" be beneficial to only allow inputting digits?
Brummolix, you can also check e.g. on the MDN account manager extension in http://mxr.mozilla.org/comm-central/source/mailnews/extensions/mdn/src/ whether you set up everything that is needed.

Also, do you really want to show the panel for all account types (the show_panel function)?
Attached patch patch (obsolete) — Splinter Review
This limits the breakage to the extension panels and allows the base panels to show normally.
Attachment #8591273 - Flags: review?(mkmelin+mozilla)
Attachment #8591273 - Flags: review?(iann_bugzilla)
Severity: normal → major
Status: NEW → ASSIGNED
Version: 38 → Trunk
The extension works in TB31, so I guess it has nothing to do with the TB29 bug.

I have now cut off everything from the plugin until it worked again (and then put it all back :) )
I ended up at the following piece of code who is responsible for the failure:


if (XPCOMUtils.generateNSGetFactory) 
{
    // Gecko >= 2.0
    const NSGetFactory = XPCOMUtils.generateNSGetFactory([AutoarchiveManagerExtension]);
}
else 
{
    // Gecko <= 1.9.x
    var NSGetModule = XPCOMUtils.generateNSGetModule([AutoarchiveManagerExtension], postModuleRegisterCallback);
}


If I replace this one by the following it is working again:

const NSGetFactory = XPCOMUtils.generateNSGetFactory([AutoarchiveManagerExtension]);


I guess it will remove compatibility to older TB version? Thunderbird 3.1? If it is about such an old version I won't care.

But the code I used before is the same as written here: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/NsIMsgAccountManagerExtension/Building_an_Account_Manager_Extension
Either it should work or at least you should change the example code then.




About your other questions (but they have nothing to do with the error):
* preftype="int" vs. preftype="Number" - I don't understand, what is wrong about int?
* <description>&settingsDescription;</description> seem to work, I can't remember if it did not in older TB
(In reply to brummolix from comment #6)
> if (XPCOMUtils.generateNSGetFactory) 
> I guess it will remove compatibility to older TB version? Thunderbird 3.1?
> If it is about such an old version I won't care.

So maybe XPCOMUtils changed in some way.
But see at http://mxr.mozilla.org/comm-central/source/mozilla/extensions/inspector/base/js/inspector-cmdline.js#57 , there is still occurrence of the example pattern in mozilla code. So it should work.
I have also just tested it and "if (XPCOMUtils.generateNSGetFactory)" does evaluate as true.

> But the code I used before is the same as written here:
> https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/
> Interface/NsIMsgAccountManagerExtension/Building_an_Account_Manager_Extension
> Either it should work or at least you should change the example code then.

We can fix the docs if we find out why it doesn't work.

> About your other questions (but they have nothing to do with the error):
> * preftype="int" vs. preftype="Number" - I don't understand, what is wrong
> about int?

I meant <textbox type="number">, see https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/textbox .
Nothing about the preftype.
Comment on attachment 8591273 [details] [diff] [review]
patch

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

::: mailnews/base/prefs/content/AccountManager.js
@@ +1561,5 @@
>          let catMan = Components.classes["@mozilla.org/categorymanager;1"]
>                                 .getService(Ci.nsICategoryManager);
>          const CATEGORY = "mailnews-accountmanager-extensions";
>          let catEnum = catMan.enumerateCategory(CATEGORY);
> +        let string = Components.interfaces.nsISupportsCString;

not to fond of naming aliases like this. name it nsISupportsCString or use the full thing
Attachment #8591273 - Flags: review?(mkmelin+mozilla) → review+
I found the problem in my code.

if (XPCOMUtils.generateNSGetFactory) 
{
    // Gecko >= 2.0
    const NSGetFactory = XPCOMUtils.generateNSGetFactory([AutoarchiveManagerExtension]);
}
else 
{
    // Gecko <= 1.9.x
    var NSGetModule = XPCOMUtils.generateNSGetModule([AutoarchiveManagerExtension], postModuleRegisterCallback);
}

should be

if (XPCOMUtils.generateNSGetFactory) 
{
    // Gecko >= 2.0
    var NSGetFactory = XPCOMUtils.generateNSGetFactory([AutoarchiveManagerExtension]);
}
else 
{
    // Gecko <= 1.9.x
    var NSGetModule = XPCOMUtils.generateNSGetModule([AutoarchiveManagerExtension], postModuleRegisterCallback);
}

The "const" was wrong. A const is only valid inside the block.
Also in the example in https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/NsIMsgAccountManagerExtension/Building_an_Account_Manager_Extension there is no const.
Shame on me...



P.S.
About the <textbox type="number">
That's a good idea.
Cool, thanks for finding the problem. Yeah, the semantics of 'let' and maybe also 'const' are being tightened in core Javascript engine. So it is possible this worked once but no longer does. E.g. 'let' declared variable only exists in the block and only after it is actually declared. Which is a recent change. 'const' could be similar.
Attached patch patch v1.1Splinter Review
Attachment #8591273 - Attachment is obsolete: true
Attachment #8591273 - Flags: review?(iann_bugzilla)
Attachment #8591738 - Flags: review?(iann_bugzilla)
Comment on attachment 8591738 [details] [diff] [review]
patch v1.1

[Approval Request Comment]

Could I have some comments on the riskiness of this? I'd like to take it in the next beta if we can.
Attachment #8591738 - Flags: approval-comm-beta?
Attachment #8591738 - Flags: approval-comm-aurora?
Duplicate of this bug: 1154346
(In reply to Kent James (:rkent) from comment #12)
> Could I have some comments on the riskiness of this? I'd like to take it in
> the next beta if we can.
Unless I made some typo in the patch, this should not be risky. It just increases the granularity of error reporting in the account manager. So that means it is no longer a "all panes OR none" choice. The extension produced panes are now separated in their own try-catch block.

We have 2 reviewers and you can check it too. So hopefully it is not risky.

This is also not a recent regression, the change showing the "invalid account X" message was done for TB29. But it seems now there is a timeframe when people try the TB38 and have extensions that are not yet updated to it. Until the extensions are updated, people get broken account configuration.
So I think this patch is good to have specifically in this timeframe, that is why I set tracking TB38.
Iann, could you try to get to this review? We'd like to push this into the next release.
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 40.0
You need to log in before you can comment on or make changes to this bug.