1253931 - Console message: JavaScript error: chrome://messenger/content/mail3PaneWindowCommands.js, line 1124: TypeError: gFolderDisplay is null

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Jorg K (CEST = GMT+2)]

STR:
Open message in tab.
Right click tab, select: Move to New Windows.

Patch coming.

Comment 1

[Jorg K (CEST = GMT+2)]

Attachment: Fix (v1).

Comment 2

[:aceman]

Somehow I do not get that error on trunk, without the patch.

Comment 3

[Jorg K (CEST = GMT+2)]

Sorry, missed a step:
Open the Add-ons manager.
Open message in tab.
Right click message tab, select: Move to New Windows.

(I'm working on an add-on, so I had the add-ons manager up.)

Does that show the error for you?

Comment 4

[:aceman]

No, I do not see it in that way.

Comment 5

[Jorg K (CEST = GMT+2)]

So what do we do? I've just run my local build in safe mode. Open the add-ons manager, then open a message in a tab, then move the message to a new window. Result:
JavaScript error: chrome://messenger/content/mail3PaneWindowCommands.js, line 1127: TypeError: gFolderDisplay is null
1127 is this line: var currentFocusedElement = gFolderDisplay.focusedPane;

It's three off from what was reported since I added
//   if (!gFolderDisplay)
//    return;
(empty line).

That's from my patch which I commented out to see the error again.

Comment 6

[Jorg K (CEST = GMT+2)]

OK, since you didn't believe me and today is branch day I clobbered my (not so fresh) build and rebuilt it again.

I started in safe mode.
I opened the add-ons manager (must do it in this session, if it's open from a previous session, it doesn't fail).
I opened a message which is now the second tab.
Move to a new window. Problem didn't happen.
Close the second window. Open the message again in the first window next to the add-ons tab.
Move to new windows and bing:
JavaScript error: chrome://messenger/content/mail3PaneWindowCommands.js, line 1124: TypeError: gFolderDisplay is null
I tried a third time. No problem. Then a fourth time: Yes, problem there.

I'm not making it up, you need to try until it happens. Seems intermittent, perhaps a timing issue, perhaps not visible on Linux.

If the patch is OK, we can give it to Richard Marti to try. Or you can just take my word for it.

Comment 7

[:aceman]

So why is it happening randomly? Also, why can you skip doing the stuff in FocusRingUpdate_Mail just because there is an addon tab?

We have no real justification on why the patch is the right thing to do.

Comment 8

[Jorg K (CEST = GMT+2)]

So who is the pedant now ;-)

Do you think it works better when JS just stops the function due to the error? The function is about setting focus somewhere. Most likely the folder isn't ready yet in some cases.

So what according to you is the right thing to do?

Let me get you a JS stack, OK?

Comment 9

[Jorg K (CEST = GMT+2)]

chrome://messenger/content/mail3PaneWindowCommands.js (1119)
chrome://messenger/content/messenger.xul (1)
JavaScript error: chrome://messenger/content/mail3PaneWindowCommands.js, line 1127: TypeError: gFolderDisplay is null

Not helpful, call comes from here:
https://dxr.mozilla.org/comm-central/source/mail/base/content/messenger.xul#112

Good cases look like this:
chrome://messenger/content/mail3PaneWindowCommands.js (1119)
chrome://messenger/content/messenger.xul (1)
chrome://messenger/content/mailTabs.js (577)

This is the stack dump code I've added if you want to try it:

function FocusRingUpdate_Mail()
{
  for (var frame = Components.stack; frame; frame = frame.caller) {
    dump(frame.filename + " (" + frame.lineNumber + ")\n");
  }

Actually. I though it had something to do with the add-ons manager tab. I lied. It happens just like this. Open an e-mail in a tab, move it to another new window. Bing.

So what are we doing?

Comment 10

[:aceman]

(In reply to Jorg K (GMT+1) from comment #8)
> Do you think it works better when JS just stops the function due to the
> error? The function is about setting focus somewhere. Most likely the folder
> isn't ready yet in some cases.
> 
> So what according to you is the right thing to do?

Then the right thing should be to wait for the folder to become ready.
Can you find out whether the code wants to run in the old or the new window?

Comment 11

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #10)
> Then the right thing should be to wait for the folder to become ready.
> Can you find out whether the code wants to run in the old or the new window?
Maybe.

It's gets called various times, so I think its safe to ignore the call that complains. We don't see any bad effects due to the call being ignored silently now on an error condition.

Frankly I think we have more important bugs to fix then to worry about this.

Comment 12

[Jorg K (CEST = GMT+2)]

Magnus, do you think it's safe to ignore the execution of FocusRingUpdate_Mail() when gFolderDisplay is null (as per my patch). I don't have a clue what this is for.

Comment 13

[:aceman]

(In reply to Jorg K (GMT+1) from comment #11)
> It's gets called various times, so I think its safe to ignore the call that
> complains. We don't see any bad effects due to the call being ignored
> silently now on an error condition.

That does not mean there are no bad effects.

Is there a problem if we leave the error as is? The exception is probably preventing all remaining code in that function to run? Does it stop any other code later on?
If your patch allowing the other code outside that function to run? Is it safe to run it when this function does not run?

> Frankly I think we have more important bugs to fix then to worry about this.
Yes, so if we do not know what is going on, let's do not hide the bug in case anybody can figure it out in the future.

Comment 14

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #13)
> Is there a problem if we leave the error as is?
No visible problem.

> The exception is probably
> preventing all remaining code in that function to run?
That's what I understand.

> Does it stop any other code later on?
There is no other code later on.
> Is your patch allowing the other code outside that function to run?
There is no other code later on.
> Is it safe to run it when this function does not run?
There is no other code later on.

As I understand it, this is triggered by XUL here:
https://dxr.mozilla.org/comm-central/source/mail/base/content/messenger.xul#112
  <commandset id="FocusRingUpdate_Mail"
        commandupdater="true"
        events="focus"
        oncommandupdate="FocusRingUpdate_Mail()"/>
when the window gains focus.

At times, the folders aren't ready, so gFolderDisplay is not set.

We have no control over the inner workings of XUL neither will we fix them.

So we just ignore the unfortunate case of the function not working. Either we leave it to JS to stop the execution or we actively detect the error case.

I'd much prefer to actively catch the case instead of having it get ignored "by accident".

Comment 15

[Magnus Melin [:mkmelin]]

Comment on attachment 8727167 [details] [diff] [review]
Fix (v1).

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

Yeah it sounds entirely plausible we could end up here without gFolderDisplay set, so r=mkmelin

Comment 16

[Jorg K (CEST = GMT+2)]

Aleth, I'm busy with other things right now, if you don't mind, you could check this in. No hurry. Maybe when you need something to check in to check the tree status or with other things. Thanks!
I've changed reviewers in the process, so please put r=mkmelin.

Comment 17

[Jorg K (CEST = GMT+2)]

Comment on attachment 8727167 [details] [diff] [review]
Fix (v1).

[Approval Request Comment]
Regression caused by (bug #): No regression.
User impact if declined: None.
Testing completed (on c-c, etc.): Manual.
Risk to taking this patch (and alternatives if risky):
None. We're testing a variable before accessing through it.

Comment 18

[Jorg K (CEST = GMT+2)]

Comment on attachment 8727167 [details] [diff] [review]
Fix (v1).

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: 
Testing completed (on c-c, etc.): 
Risk to taking this patch (and alternatives if risky):

Comment 19

[Kent James (:rkent)]

http://hg.mozilla.org/releases/comm-aurora/rev/ad6b817d917e

Comment 20

[Jorg K (CEST = GMT+2)]

I'll land it on C-C myself using a transplant from Aurora.

Comment 21

[Kent James (:rkent)]

The problem with removing the checkin-needed is that it is the only real way we have of flagging something as landed on aurora but not central. I supposed you could come up with an alternate search, but nobody is watching that. I think it would be better to make a note in the bug that you want to do the checkin yourself, but leave the checkin-needed.

Comment 22

[Magnus Melin [:mkmelin]]

Well we do have the status-thunderbird-xx flags. And the target milestone :)

Comment 23

[Kent James (:rkent)]

As I said, it is possible to generate a search that could support early checkin of c-aurora without checkin-needed. But that assumes someone actually runs those searches. There is no active process to assure c-c checkins other than checkin-needed. I have relied on that flag to pre-land on release repos reliably. Without it, there is no existing process to ensure that things ever get landed in c-c, other than whatever private process Jorg is using.

Comment 24

[Kent James (:rkent)]

Comment on attachment 8727167 [details] [diff] [review]
Fix (v1).

http://hg.mozilla.org/releases/comm-beta/rev/0c4109ac2b8a
http://hg.mozilla.org/releases/comm-esr45/rev/62b1083e559e

Comment 25

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/9c79a2d4424c