1338798 - JavaScript error: chrome://messenger/content/folderDisplay.js, line 2514: TypeError: this.view.dbView is null

Status: RESOLVED FIXED

(Thunderbird :: Folder and Message Lists, defect)

Description

[ISHIKAWA, Chiaki]

I see the following error 11 times during the run of |make mozmill| test suite run locally:

JavaScript error: chrome://messenger/content/folderDisplay.js, line 2514: TypeError: this.view.dbView is null

Looking at the code below, we may simply return from the function when this.view.dbView is null (given the comment near the beginning of the function.).

 2483	  /**
 2484	   * Ensure the given view index is visible, optionally with some padding.
 2485	   * By padding, we mean that the index will not be the first or last message
 2486	   *  displayed, but rather have messages on either side.
 2487	   * We have the concept of a 'lip' when we are at the end of the message
 2488	   *  display.  If we are near the end of the display, we want to show an
 2489	   *  empty row (at the bottom) so the user knows they are at the end.  Also,
 2490	   *  if a message shows up that is new and things are sorted ascending, this
 2491	   *  turns out to be useful.
 2492	   */
 2493	  ensureRowIsVisible: function FolderDisplayWidget_ensureRowIsVisible(
 2494	      aViewIndex, aBounced) {
 2495	    // Dealing with the tree view layout is a nightmare, let's just always make
 2496	    //  sure we re-schedule ourselves.  The most particular rationale here is
 2497	    //  that the message pane may be toggling its state and it's much simpler
 2498	    //  and reliable if we ensure that all of FolderDisplayWidget's state
 2499	    //  change logic gets to run to completion before we run ourselves.
 2500	    if (!aBounced) {
 2501	      let dis = this;
 2502	      window.setTimeout(function() {
 2503	          dis.ensureRowIsVisible(aViewIndex, true);
 2504	        }, 0);
 2505	    }
 2506	
 2507	    let treeBox = this.treeBox;
 2508	    if (!treeBox)
 2509	      return;
 2510	
 2511	    // try and trigger a reflow...
 2512	    treeBox.height;
 2513	
 2514	    let maxIndex = this.view.dbView.rowCount - 1;   // <=== ****
 2515	
 2516	    let first = treeBox.getFirstVisibleRow();
 2517	    // Assume the bottom row is half-visible and should generally be ignored.
 2518	    // (We could actually do the legwork to see if there is a partial one...)
 2519	    const halfVisible = 1;
 2520	    let last  = treeBox.getLastVisibleRow() - halfVisible;
 2521	    let span = treeBox.getPageLength() - halfVisible;
 2522	    let [topPadding, bottomPadding] = this.visibleRowPadding;
 2523	
 2524	    let target;
 2525	    // If the index is after the last visible guy (with padding), move down
 2526	    //  so that the target index is padded in 1 from the bottom.
 2527	    if (aViewIndex >= (last - bottomPadding))
 2528	      target = Math.min(maxIndex, (aViewIndex + bottomPadding)) - span;
 2529	    // If the index is before the first visible guy (with padding), move up
 2530	    else if (aViewIndex <= (first + topPadding))  // move up
 2531	      target = Math.max(0, (aViewIndex - topPadding));
 2532	    else // it is already visible
 2533	      return;
 2534	
 2535	    // this sets the first visible row
 2536	    treeBox.scrollToRow(target);
 2537	  },

Given that the error caused by the null reference prohibited the execution of the rest of the function (I suppose), returning as soon as this.view.dbView is null is appropriate.

I create such a patch and I didn't see this error any more.

Comment 1

[ISHIKAWA, Chiaki]

Attachment: Avoid null reference of this.view.dbView

This is the patch which I suggested in my initial comment.
(BTW, the comment is a follow-up to the comment in Bug 978559 comment 3).

I ran this locally and all was well. No more errors.

It is possible that I am hiding some real bugs under the rug, but
basically the current state of the affairs is that this function is causing premature abort due to JavaScript error and so my patch is not causing additional harm, but just exiting in a graceful manner.

TIA

Comment 2

[:aceman]

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?

Comment 3

[Jorg K (CEST = GMT+2)]

Comment on attachment 8836379 [details] [diff] [review]
Avoid null reference of this.view.dbView

Sorry, I got there first ;-)

Comment 5

[Jorg K (CEST = GMT+2)]

Attachment: 1338798-js-type-errors.patch

I think we need two changes here.

Comment 6

[Jorg K (CEST = GMT+2)]

JavaScript error: chrome://messenger/content/folderDisplay.js, line 1166: TypeError: this.view.displayedFolder is null
JavaScript error: chrome://messenger/content/folderDisplay.js, line 2514: TypeError: this.view.dbView is null

Comment 7

[:aceman]

For the record, tests showing these warnings:
folder-display/test-deletion-from-virtual-folders.js (for the this.view.displayedFolder case)
folder-display/test-message-pane-visibility.js       (for the this.view.dbView case)

Comment 8

[:aceman]

Attachment: alternate solution for this.view.dbView

I have looked at the patches and what the tests do. For the this.view.dbView I propose an alternate solution here.

Using treeBox.view returns the correct number of rows in this particular test (and does not visibly regress others, https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=7336e0359c5dc4480516ff0ee2ca4ef016cfd8b5). I don't know why treeBox.view is not null and this.view.dbView is in this case. But I think when the function ensureRowsIsVisible and also ensureRowRangeIsVisible calculate everything based on attributes of treeBox, why should we take rowCount from a different object (this.view.dbView).

Comment 9

[:aceman]

Comment on attachment 8838517 [details] [diff] [review]
1338798-js-type-errors.patch

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

I proposed a different solution for the second hunk of the patch.

For the first part with this.view.displayedFolder I still don't know why it happens. The test switches from an empty search folder to the smart (unified) folder mode. It could be there is a time window where some of the view is not yet set up when the tree is rebuilt. But I don't know how bad an empty this.view.displayedFolder condition is. There is a similar check in canArchiveSelectedMessages() where it checks this.view.isSingleFolder together with this.view.displayedFolder as in the proposed patch. But if you add the check, the block is skipped and the following code is run (this.navigate(nsMsgNavigationType.lastMessage, /* select */ false)) or this.ensureRowIsVisible(0)). And I don't know if that is the right thing to do (it changes behaviour). Maybe if we are in some temporary state of disorder, we should just exit the function not run the code breaking the stored view position (remembered last message).

Comment 10

[Magnus Melin [:mkmelin]]

Comment on attachment 8839820 [details] [diff] [review]
alternate solution for this.view.dbView

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

Yes, all the operations later are on treeBox, so this seems like a good approach. r=mkmelin

Comment 11

[:aceman]

Attachment: 1338798-js-type-errors.patch v2

Magnus, the first hunk here is not superseded by the other patch. So this needs looking at too. I have now left only the relevant part after you accepted the other patch.

Comment 12

[Jorg K (CEST = GMT+2)]

Comment on attachment 8841364 [details] [diff] [review]
1338798-js-type-errors.patch v2

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

::: mail/base/content/folderDisplay.js
@@ +1164,3 @@
>        // use the displayed folder; nsMsgDBView goes to the effort to save the
>        //  state to the viewFolder, so this is the correct course of action.
>        let lastLoadedMessageKey = this.view.displayedFolder.lastMessageLoaded;

Hard to see, but here is |this.view.displayedFolder| and if it's null, there is no joy to be had ;-(

Comment 13

[Magnus Melin [:mkmelin]]

Comment on attachment 8841364 [details] [diff] [review]
1338798-js-type-errors.patch v2

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

Great, thx! r=mkmelin

Comment 14

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/68b03a27f0f349b5d54053aca4efe39ea3247e2d
https://hg.mozilla.org/comm-central/rev/12954899022214532ab4d38d4054c4936411b2a5