Silence a few JS errors in folderPane.js, messageWindow.js

RESOLVED FIXED in Thunderbird 54.0

Status

Thunderbird
General
RESOLVED FIXED
9 months ago
6 months ago

People

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

Tracking

Trunk
Thunderbird 54.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

9 months ago
These are frequently seen in Mozmill runs:

INFO -  JavaScript error: chrome://messenger/content/folderDisplay.js, line 1166: TypeError: this.view.displayedFolder is null
INFO -  JavaScript error: chrome://messenger/content/folderDisplay.js, line 2514: TypeError: this.view.dbView is null
INFO -  JavaScript error: chrome://messenger/content/folderPane.js, line 2185: TypeError: null has no properties
INFO -  JavaScript error: chrome://messenger/content/messageWindow.js, line 254: TypeError: this.folderDisplay.view.dbView is null
(Assignee)

Comment 1

9 months ago
Created attachment 8812344 [details] [diff] [review]
1318776-js-type-errors.patch (v1).

I know you're going to start this discussion and want to analyse each case to see why things are null when it's not expected :-(

I haven't looked into it, but for folderPane.js I ran
mozmake SOLO_TEST=folder-pane/test-folder-names-in-recent-mode.js mozmill-one
since that's where we see this error.

Doing something controlled is a lot better than just letting JS abort the function on a TypeError.

Here's a try run to see whether the errors have really disappeared and whether anything broke:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=1baee75255d7489f87689272b65e6e601d487166
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Attachment #8812344 - Flags: review?(acelists)
(Assignee)

Comment 2

9 months ago
Comment on attachment 8812344 [details] [diff] [review]
1318776-js-type-errors.patch (v1).

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

::: mail/base/content/folderPane.js
@@ +2185,2 @@
>        this._rowMap[index]._parent._children = null;
> +      

I've seen it ;-(

::: mail/base/content/messageWindow.js
@@ +254,1 @@
>          this.folderDisplay.view.dbView.rowCount == 0) {

Maybe this should be:
!this.aboutToLoadMessage && 
(!this.folderDisplay.view.dbView || this.folderDisplay.view.dbView.rowCount == 0)
What do you think?
(Assignee)

Comment 3

9 months ago
Aecman, please check the log of my try run:

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=1baee75255d7489f87689272b65e6e601d487166
All of the errors listed in comment #0 have disappeared.

The question remains whether messageWindow.js should be like this, which is what was run on try:

     if (!this.aboutToLoadMessage && this.folderDisplay.view.dbView &&
         this.folderDisplay.view.dbView.rowCount == 0) {
       window.close();
       return true;
     }
     return false;

or like this:

     if (!this.aboutToLoadMessage &&
         (!this.folderDisplay.view.dbView || this.folderDisplay.view.dbView.rowCount == 0)) {
       window.close();
       return true;
     }
     return false;

Comment 4

9 months ago
I don't doubt that, but I will try to see why this is needed. Maybe we just need to wait a bit in some of the tests.

Comment 5

8 months ago
Comment on attachment 8812344 [details] [diff] [review]
1318776-js-type-errors.patch (v1).

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

I have yet to check the folderDisplay.js change, but I checked the other 2 and have found this:

::: mail/base/content/folderPane.js
@@ +2180,5 @@
>      let index = this.getIndexOfFolder(aItem);
>      if (index == null)
>        return;
>      // forget our parent's children; they'll get rebuilt
> +    if (aRDFParentItem && this._rowMap[index]._parent)

This seems fine, the test is removing top level folders (at least in the display (recent folders) so there should be no parent).

@@ +2185,2 @@
>        this._rowMap[index]._parent._children = null;
> +      

Then remove the spaces :)

::: mail/base/content/messageWindow.js
@@ +254,1 @@
>          this.folderDisplay.view.dbView.rowCount == 0) {

This looks fine in theory (close the window when there is no dbView), but it fails the test test-message-window.js in test_next_message() because it expects the window to be kept open on the switch to be able to show the next message.

My observations:
the test switches folders (selects next message in the next folder) so that seems to be a reason this.folderDisplay.view.dbView is null temporarily. Please add a comment to that effect.

Can you return true without closing the window in case of null, similarly to what onMessagesRemoved does below?
Attachment #8812344 - Flags: review?(acelists) → feedback+
(Assignee)

Comment 6

8 months ago
Created attachment 8821456 [details] [diff] [review]
1318776-js-type-errors.patch (v2).

Thanks for the thorough review and all the research that I should have done myself. I changed messageWindow.js to what you said.
Attachment #8821456 - Flags: review?(acelists)
(Assignee)

Comment 7

8 months ago
Green try here:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=6eeb14c76ee28a38e5d8793e357bf099d5eba657

Looking at the log
https://archive.mozilla.org/pub/thunderbird/try-builds/mozilla@jorgk.com-6eeb14c76ee28a38e5d8793e357bf099d5eba657/try-comm-central-linux64-debug/try-comm-central_ubuntu64_vm-debug_test-mozmill-bm114-tests1-linux64-build7.txt.gz
all targeted errors disappeared and the occurrences of "JavaScript error" were greatly reduced.

Note: The lovely "JavaScript error: chrome://global/content/browser-content.js, line 1772" is not our fault, it's been reported in bug 1325645 and was caused by bug 1297867 which has already been backed out.
(Assignee)

Updated

6 months ago
Duplicate of this bug: 1338798
(Assignee)

Updated

6 months ago
Duplicate of this bug: 1338815

Comment 10

6 months ago
Great to some remaining js errors are addressed in this bugzilla!

Comment 11

6 months ago
Oops, I hit the save changes button too early. 
Isn't it a good idea to include the filename and the particular JS variable and attribute so that anyone searching bugzilla can find this bugzilla?

TIA
(Assignee)

Comment 12

6 months ago
Done.
Summary: Silence a few JS errors → Silence a few JS errors in folderDisplay.js, folderPane.js, messageWindow.js

Comment 13

6 months ago
From Bug 1338798 Comment 2:
[ This is about JavaScript error: chrome://messenger/content/folderDisplay.js, line 2514: TypeError: this.view.dbView is null, and a patch to return early if view is null. ]

(In reply to :aceman from comment #2)
> I think this one is covered by bug 1318776 too and I am still analyzing if
> it is correct.
> 
> Do you know why the view is null and why it is expected condition?

I have no idea why it can be null. It seems that the layout (reflow just above) seems to 
go astray or has not settled down nicely ?
(I am not sure about this at all. Can it be that operation is asynchronous?)
(Assignee)

Comment 14

6 months ago
Created attachment 8838516 [details] [diff] [review]
1318776-js-type-errors.patch

Let's land the agreed hunks here and move the rest to bug 1338798.
Attachment #8812344 - Attachment is obsolete: true
Attachment #8821456 - Attachment is obsolete: true
Attachment #8821456 - Flags: review?(acelists)
Attachment #8838516 - Flags: review+
(Assignee)

Comment 15

6 months ago
https://hg.mozilla.org/comm-central/rev/c52f5a6cdd14fa2c1d605889d24bd9b40a111c42
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
Resolution: --- → FIXED
Summary: Silence a few JS errors in folderDisplay.js, folderPane.js, messageWindow.js → Silence a few JS errors in folderPane.js, messageWindow.js
Target Milestone: --- → Thunderbird 54.0
(Assignee)

Updated

6 months ago
Duplicate of this bug: 1340800
You need to log in before you can comment on or make changes to this bug.