The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Thunderbird 48.0

Status

Thunderbird
Mail Window Front End
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: Jorg K (GMT+1), Assigned: Jorg K (GMT+1))

Tracking

Trunk
Thunderbird 48.0

Thunderbird Tracking Flags

(thunderbird46 fixed, thunderbird47 fixed, thunderbird48 fixed, thunderbird_esr45 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

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

Patch coming.
(Assignee)

Comment 1

a year ago
Created attachment 8727167 [details] [diff] [review]
Fix (v1).
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
Attachment #8727167 - Flags: review?(acelists)

Comment 2

a year ago
Somehow I do not get that error on trunk, without the patch.
(Assignee)

Comment 3

a year ago
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

a year ago
No, I do not see it in that way.
(Assignee)

Comment 5

a year ago
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.
(Assignee)

Comment 6

a year ago
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

a year ago
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.
(Assignee)

Comment 8

a year ago
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?
(Assignee)

Comment 9

a year ago
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

a year ago
(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?
(Assignee)

Comment 11

a year ago
(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.
(Assignee)

Comment 12

a year ago
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.
Flags: needinfo?(mkmelin+mozilla)

Comment 13

a year ago
(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.
(Assignee)

Comment 14

a year ago
(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".
(Assignee)

Updated

a year ago
Attachment #8727167 - Flags: review?(mkmelin+mozilla)

Comment 15

a year ago
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
Attachment #8727167 - Flags: review?(mkmelin+mozilla)
Attachment #8727167 - Flags: review?(acelists)
Attachment #8727167 - Flags: review+
(Assignee)

Comment 16

a year ago
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.
Flags: needinfo?(mkmelin+mozilla)
Keywords: checkin-needed
(Assignee)

Comment 17

a year ago
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.
Attachment #8727167 - Flags: approval-comm-release?
Attachment #8727167 - Flags: approval-comm-beta?
Attachment #8727167 - Flags: approval-comm-aurora+
Comment hidden (obsolete)

Updated

a year ago
Target Milestone: --- → Thunderbird 48.0
http://hg.mozilla.org/releases/comm-aurora/rev/ad6b817d917e
status-thunderbird47: --- → fixed
(Assignee)

Comment 20

a year ago
I'll land it on C-C myself using a transplant from Aurora.
Keywords: checkin-needed
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

a year ago
Well we do have the status-thunderbird-xx flags. And the target milestone :)
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 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
Attachment #8727167 - Flags: approval-comm-esr45?
Attachment #8727167 - Flags: approval-comm-esr45+
Attachment #8727167 - Flags: approval-comm-beta?
Attachment #8727167 - Flags: approval-comm-beta+

Updated

a year ago
status-thunderbird46: --- → fixed
status-thunderbird48: --- → affected
status-thunderbird_esr45: --- → fixed
(Assignee)

Comment 25

a year ago
https://hg.mozilla.org/comm-central/rev/9c79a2d4424c
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-thunderbird48: affected → fixed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.