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

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Jorg K (CEST = GMT+2)]

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

Comment 1

[Jorg K (CEST = GMT+2)]

Attachment: 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

Comment 2

[Jorg K (CEST = GMT+2)]

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?

Comment 3

[Jorg K (CEST = GMT+2)]

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

[:aceman]

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

[:aceman]

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?

Comment 6

[Jorg K (CEST = GMT+2)]

Attachment: 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.

Comment 7

[Jorg K (CEST = GMT+2)]

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/[e-mail]-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.

Comment 10

[ISHIKAWA, Chiaki]

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

Comment 11

[ISHIKAWA, Chiaki]

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

Comment 12

[Jorg K (CEST = GMT+2)]

Done.

Comment 13

[ISHIKAWA, Chiaki]

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?)

Comment 14

[Jorg K (CEST = GMT+2)]

Attachment: 1318776-js-type-errors.patch

Let's land the agreed hunks here and move the rest to bug 1338798.

Comment 15

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/c52f5a6cdd14fa2c1d605889d24bd9b40a111c42