931492 - Thunderbird status bar and SeaMonkey messaging tab not shown/broken due to error in calendar-extract.js

Status: RESOLVED FIXED

(Calendar :: Lightning Only, defect)

Description

[Richard Marti (:Paenglab)]

With Lightning from 20131027 the calendar-status-todaypane-button is no more shown in statusbar and also the resizer is moved to the left. No such problems with Lightning from 20131026.

With commenting out <script type="application/javascript" src="chrome://calendar/content/calendar-extract.js"/> in messenger-overlay-sidebar.xul the button is shown again.

I see nothing in console about this issue.

Comment 1

[Merike Sell (:merike)]

I'm doubly puzzled. Firstly, because I've been running Thunderbird for months without noticing this bug. Secondly, because the source of it makes no sense to me at the moment.

Could you try replacing original addListeners function with the following?

addListeners: function addListeners() {
    if (window.top.document.location == "chrome://messenger/content/messenger.xul") {
        // covers initial load and folder change
        document.getElementById("folderTree_");
        //document.getElementById("folderTree");
        /*let folderTree = document.getElementById("folderTree");
        folderTree.addEventListener("select", this.setState, false);

        // covers selection change in a folder
        let msgTree = window.top.GetThreadTree();
        msgTree.addEventListener("select", this.setState, false);*/
    }
}

If I do that the problem goes away. As soon as I enable 
document.getElementById("folderTree");
it breaks again. Why a getElementById call with specific argument would cause such breakage I have no idea.

Comment 2

[Richard Marti (:Paenglab)]

Yes, commenting this out brings the button back.

Comment 3

[Merike Sell (:merike)]

Attachment: bug931492

This fixes the bug for me. I'm not sure why I need calendarExtract.setState instead of this.setState though...

Comment 4

[Richard Marti (:Paenglab)]

Comment on attachment 823628 [details] [diff] [review]
bug931492

I'm not a expert on js and don't want to say it's correct or not. So moving the review to Philipp.

But I tested it and it does what it should, so f+

Comment 5

[Stefan Sitter [:ssitter]]

My comments:

I assume the wrong line |calendarExtract.addListeners();| causes an failure during an loading but it seems the error is nowhere reported. Commenting out this line allows initialization to continue and the status bar is shown again.

I think you cannot use this. because there is no object involved. In my understanding calendar-extract.js contains just a bunch of methods that are only defined inside |calendarExtract| namespace to not pollute the global namespace. (But I might be wrong here)

If you enable strict warnings you will get a notification about the unknown |this| like
> Warning: ReferenceError: reference to undefined property this.setState
> Source File: chrome://calendar/content/calendar-extract.js Line: 207

You should pass |false| as the 3rd parameter to addEventListener() like done everywhere else in calendar code to not rely upon default values. The default value might change like currently happening in Bug 937461 and therefore causing Lightning Bug 937470.

I think once onLoad / addListeners is called you should remove the "load" listener to not leak it. In addition I think there should be an onUnload handler to remove the listeners from folder tree and thread tree.

Comment 6

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 823628 [details] [diff] [review]
bug931492

r- based on Stefan's comments. Removing the listener would be nice indeed.  It might make sense to bind the this object to those calls in some cases, to avoid trouble in case |this| is used in setState some time in the future:

let setState = this.setState.bind(this);
folderTree.addEventListener("select", setState, false);
msgGree.addEventListener("select", setState, false);
window.addEventListener("unload", () => {
  folderTree.removeEventListener("select", setState, false);
  msgTree.removeEventListener("select", setState, false);
}, false);

Comment 7

[Merike Sell (:merike)]

Attachment: bug931492

This adds third parameter and also removes listeners when unloading a window.

(In reply to Stefan Sitter from comment #5)
> If you enable strict warnings you will get a notification about the unknown
> |this| like
> > Warning: ReferenceError: reference to undefined property this.setState
> > Source File: chrome://calendar/content/calendar-extract.js Line: 207
Strangely even after setting that preference I still didn't get any.

(In reply to Philipp Kewisch [:Fallen] from comment #6)
> r- based on Stefan's comments. Removing the listener would be nice indeed. 
> It might make sense to bind the this object to those calls in some cases, to
> avoid trouble in case |this| is used in setState some time in the future:
> 
> let setState = this.setState.bind(this);
This turned out a bit different as this inside setState appeared to be set to window object intially.

> window.addEventListener("unload", () => {
>   folderTree.removeEventListener("select", setState, false);
>   msgTree.removeEventListener("select", setState, false);
> }, false);
If it weren't for this example I wouldn't have learned that arrow functions exist. When I first looked at it it just refused to compile in my head :)

Comment 8

[Merike Sell (:merike)]

(In reply to Merike (:merike) from comment #7)
> This turned out a bit different as this inside setState appeared to be set
> to window object intially.
I meant addListeners instead of setState here (although it's probably true for setState too).

Comment 9

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 8335554 [details] [diff] [review]
bug931492

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

r+ with this change:

::: calendar/base/content/calendar-extract.js
@@ +243,4 @@
>      }
>  };
>  
> +window.addEventListener("load", calendarExtract.addListeners, false);

Try changing this to calendarExtract.addListeners.bind(calendarExtract). That will give addListeners() the right |this| object, then you can use |this| correctly in that function.

Comment 10

[Barry Edwin GIlmour]

Plain text editing the diff works like a charm! Full SeaMonkey Messaging services are back!
Attachment #8335554 [details] [diff]: bug931492 patched into my file: lightning-3.0a1.en-US.linux-x86_64/chrome/calendar/content/calendar/calendar-extract.js) without using Philipp's extra recommendation to the patch <https://bugzilla.mozilla.org/show_bug.cgi?id=931492#c9> restores Messaging to trunk SeaMonkey. Many thanks (I didn't try the patch,

calendarExtract.addListeners.bind(calendarExtract)

 on the grounds of - if it ain't broke, don't fix it!).

Comment 11

[Barry Edwin GIlmour]

Linux-x86_64 SeaMonkey Messaging + Lightning also play nicely, using "calendarExtract.addListeners.bind(calendarExtract)" if it's more suitable.

Referencing Attachment #8335554 [details] [diff] [diff]: bug931492:- 

Text-edited calendar-extract.js line 205, from
let setState = calendarExtract.setState.bind(calendarExtract); to 
let setState = calendarExtract.addListeners.bind(calendarExtract);
and restored line 207 and 211  to 
folderTree.addEventListener("select", this.setState, false);
msgTree.addEventListener("select", this.setState, false);

Comment 12

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 8335554 [details] [diff] [review]
bug931492

Stefan, I'm fine with you approving these kinds of patches too :)

Comment 13

[Stefan Sitter [:ssitter]]

Merike, do you have checkin permissions? If not please drop a note if someone else should take over the checkin.

Comment 14

[Merike Sell (:merike)]

Yes, but the tree was closed when I checked in the weekend and also got closed yesterday when I was about to check this in. I'll try again today when I get home so that it gets into tomorrow's nightly.

Comment 15

[Merike Sell (:merike)]

https://hg.mozilla.org/comm-central/rev/115b4d5ecf6b for central.
Aurora will happen when I see open tree again :)

Comment 16

[Joe Sabash [:JoeS1]]

The patch seems to have been checked in here:
https://hg.mozilla.org/comm-central/rev/115b4d5ecf6b

Testing with a local build Built from http://hg.mozilla.org/mozilla-central/rev/34f431863037
The checkin does _not_ seem to fix the problem for me.

It seems that other addons may be affected as well, so it might be more than just a lightning problem.
see https://bugzilla.mozilla.org/show_bug.cgi?id=935251#c6

Comment 17

[Barry Edwin GIlmour]

Stefan & Merike
https://hg.mozilla.org/comm-central/rev/115b4d5ecf6b is corrupted.
It's not the patch in the attachment.

To use Philip's "addListeners" suggestion and keep the this. function, it's not that last line that needs changing.

It's near line 205 ~~ addListeners: function addListeners() {
that needs changing:--
 
-- let setState = calendarExtract.setState.bind(calendarExtract)
++ let setState = calendarExtract.addListeners.bind(calendarExtract); 

For me, the last line was fine as
window.addEventListener("load", calendarExtract.addListeners, false);
HTH

Comment 18

[Barry Edwin GIlmour]

Please Ignore https://bugzilla.mozilla.org/show_bug.cgi?id=931492#c17
I just built from http://hg.mozilla.org/mozilla-central/rev/77b5c6edfe96 using gcc version 4.7.2 20130108 [gcc-4_7-branch revision 195012] (SUSE Linux) 
Mozilla/5.0 (X11; Linux x86_64; rv:28.0) Gecko/20100101 Firefox/28.0 SeaMonkey/2.25a1 ID:20131127122516 CSet: 5e1f20de9a5a
and Lightning works fine without blocking any of SeaMonkey Messaging pop-email, RSS and newsgroups (and SM status bar works fine with https://hg.mozilla.org/comm-central/rev/115b4d5ecf6b.
Sincerest Apologies for the noise.

Comment 19

[Joe Sabash [:JoeS1]]

OK, this is wfm on current trunk, but only after a re-download of lightning and a reintall of lightning

Comment 20

[Stefan Sitter [:ssitter]]

Verified fixed using Lightning 3.0a1 (BuildID 20131127030203) with Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Thunderbird/28.0a1.

Comment 21

[Merike Sell (:merike)]

Pushed to aurora as well: https://hg.mozilla.org/releases/comm-aurora/rev/fa8fc2f73a39

Comment 22

[crow]

I'm seeing this same problem on Thunderbird 24.4.0 and Lightning 2.6.4 on Windows 8.1 x86_64. The file in this patch does not exist in that release, so I can't apply it to my installation.

This is a security issue for me because I cannot view the URL for a link in an email since the status bar content becomes hidden as the resize in the lower right is moved completely the left adjacent to the offline/online indicator icon.

Should I open another bug?  Any thoughts as to where to look?

Comment 23

[crow]

"Where to look" as in where to look for an additional bug.

Comment 24

[Merike Sell (:merike)]

By using 2.6.4 you cannot be affected by this bug.

You should try disabling extensions and seeing if this solves the problem. If the problem goes away after disabling only Lightning then please file a new bug here. If it persists after disabling all of them you could either file a bug for Thunderbird or try posting on https://support.mozilla.org/questions/thunderbird for help.

Comment 25

[crow]

Disabling and enabling Lightning with the corresponding restart using the restart link in the Add-ons Manager definitely causes the issue to occur.  The symptoms are exactly the same in that some times when I start Thunderbird the Lightning today pane button in the status bar is visible and sometimes it is not.  When it is visible on launch, some yet to be determined activity can cause it to disappear.

I'll open another bug.

Comment 26

[crow]

Bug 1005101 opened.