1645647 - C-C TB mochitest - JavaScript error: chrome://messenger/content/virtualFolderProperties.js, line 287: TypeError: window.arguments[0].previewSelectedColorCallback is not a function

Status: RESOLVED FIXED

(Thunderbird :: Folder and Message Lists, defect)

Description

[ISHIKAWA, Chiaki]

Attachment: log of comm/mail/test/browser/folder-pane/browser_folderPaneConsumers.js with C-C TB Full DEBUG version

I see the following error in my local mochitest log of C-C TB Full
DEBUG version.

JavaScript error: chrome://messenger/content/virtualFolderProperties.js, line 287: TypeError: window.arguments[0].previewSelectedColorCallback is not a function

I see the error also in tryserver.
For example, benc's job.:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=b36d20f09a11e8a3af3076d85ba00c5da913613b
https://firefoxci.taskcluster-artifacts.net/JA4NgwR_RSStM_hnZLcCrA/0/public/logs/live.log

The error in question happens on the following line:

function onCancel(event) {
  if (window.arguments[0].folder) {
    // Restore the icon to the previous color and discard edits.
    window.arguments[0].previewSelectedColorCallback(    <==== here
      window.arguments[0].folder,
      kCurrentColor
    );
  }
}

Maybe due to some debug macro and other setting, I don't see the stacktrace in
the tryserver log, so I am attaching the local log.
There are some debug dumps added locally, so it is a bit verbose than
the one on the tryserver.

Comment 1

[Alessandro Castellani [:aleca]]

I can't seem to recreate this issue as my local tests all pass correctly.
./mach mochitest comm/mail/test/browser/folder-pane/browser_folderPaneConsumers.js

Is this still happening for you?

Comment 2

[ISHIKAWA, Chiaki]

Attachment: Recent error log of comm/mail/test/browser/folder-pane/browser_folderPaneConsumers.js

The error still occurs. This excerpt is from a log this morning (I refreshed the source in the last 24 hours or so.).
Now please notice that

1. I mention an error, a JavaScript error, is reported.
   HOWEVER, mochitest framework is not very robust. Even though such a JavaScript error exists and obviously the test file does not get executed from that point on as was originally intended, mochitest framework ignores it, and reports a "Pass" of that particular test.
   So you see

TEST_END: Test OK. Subtests passed 2/2. Unexpected 0

This is an utter failure of mozilla test framework IMHO.
I can't really trust this kind of test framework and have tough time to understand why people have put up with this so long.
But this is the state of the test framework at mozilla tryserver and on local development environment and I have to put up with it for now. Tough luck.

2. I use a FULL DEBUG version of C-C TB when I run this. I wonder if optimized, non-debug version reports the error.

In any case, the problem persists.

I am not quire sure if the gloda error in my log (I am not sure of what nature) what I observe has anything to do with the error. (The gloda error may be due to my local patch.)

HOWEVER, the bug reported in this bugzilla was in benc's job. (not my submission)
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=b36d20f09a11e8a3af3076d85ba00c5da913613b&selectedTaskRun=JA4NgwR_RSStM_hnZLcCrA.0
Choose Linux64 debug, M (bct5), then job details and pick up live.log.
https://firefoxci.taskcluster-artifacts.net/JA4NgwR_RSStM_hnZLcCrA/0/public/logs/live_backing.log
That was on June 12.

The recent tryserver job of benc yesterday [Friday 19 June] (sorry I chose benc's submission again. Not many people submit debug version test often) also shows the same error.
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=951a672a61f4920bec3a37bb85a28d4ce86f9149
Choose linux64 debug, M(bct5), and then Job details, and again live.log.
https://firefoxci.taskcluster-artifacts.net/cu2pIkAJSkSVEsSeRtRMPA/0/public/logs/live_backing.log

Also, you can check windows 7 debug version log from the benc's submission.
Again choose windows 7 debug, M(bct5), job details and live.log.
https://firefoxci.taskcluster-artifacts.net/TO0413ENQuSLuxVDXo3N9A/0/public/logs/live_backing.log
The bug is there.:

[task 2020-06-19T09:24:52.469Z] 09:24:52     INFO - TEST-START | comm/mail/test/browser/folder-pane/browser_folderPaneConsumers.js
[task 2020-06-19T09:24:52.470Z] 09:24:52     INFO - GECKO(8084) | Chrome file doesn't exist: Z:\task_1592558042\build\tests\mochitest\browser\comm\mail\test\browser\folder-pane\head.js
[task 2020-06-19T09:24:52.677Z] 09:24:52     INFO - GECKO(8084) | [8084, Main Thread] WARNING: '!mPresContext', file /builds/worker/checkouts/gecko/dom/events/UIEvent.cpp, line 137
[task 2020-06-19T09:24:54.217Z] 09:24:54     INFO - GECKO(8084) | [8084, Main Thread] WARNING: NS_ENSURE_TRUE(root) failed: file /builds/worker/checkouts/gecko/layout/base/nsDocumentViewer.cpp, line 2921
[task 2020-06-19T09:24:54.304Z] 09:24:54     INFO - GECKO(8084) | [8084, Main Thread] WARNING: Forced to copy ObserverTable due to nested notifications: file /builds/worker/checkouts/gecko/image/ProgressTracker.h, line 82
[task 2020-06-19T09:24:54.791Z] 09:24:54     INFO - GECKO(8084) | [8084, Main Thread] WARNING: NS_ENSURE_TRUE(root) failed: file /builds/worker/checkouts/gecko/layout/base/nsDocumentViewer.cpp, line 2921
[task 2020-06-19T09:24:55.034Z] 09:24:55     INFO - GECKO(8084) | JavaScript error: chrome://messenger/content/virtualFolderProperties.js, line 287: TypeError: window.arguments[0].previewSelectedColorCallback is not a function
[task 2020-06-19T09:24:55.289Z] 09:24:55     INFO - GECKO(8084) | [8084, Main Thread] WARNING: '!mPresContext', file /builds/worker/checkouts/gecko/dom/events/UIEvent.cpp, line 137
[task 2020-06-19T09:24:56.864Z] 09:24:56     INFO - GECKO(8084) | [8084, Main Thread] WARNING: NS_ENSURE_TRUE(root) failed: file /builds/worker/checkouts/gecko/layout/base/nsDocumentViewer.cpp, line 2921
[task 2020-06-19T09:24:57.659Z] 09:24:57     INFO - GECKO(8084) | MEMORY STAT | vsize 956MB | vsizeMaxContiguous 423MB | residentFast 372MB | heapAllocated 147MB
[task 2020-06-19T09:24:57.659Z] 09:24:57     INFO - TEST-OK | comm/mail/test/browser/folder-pane/browser_folderPaneConsumers.js | took 5183ms
[task 2020-06-19T09:24:57.659Z] 09:24:57     INFO - checking window state
[task 2020-06-19T09:25:05.388Z] 09:25:05     INFO - GECKO(8084) | Completed ShutdownLeaks collections in process 8084
[task 2020-06-19T09:25:05.388Z] 09:25:05     INFO - TEST-START | Shutdown
[task 2020-06-19T09:25:05.388Z] 09:25:05     INFO - Browser Chrome Test Summary
[task 2020-06-19T09:25:05.388Z] 09:25:05     INFO - Passed:  22
[task 2020-06-19T09:25:05.389Z] 09:25:05     INFO - Failed:  0
[task 2020-06-19T09:25:05.389Z] 09:25:05     INFO - Todo:    1

Hope this helps.

Comment 3

[Alessandro Castellani [:aleca]]

Attachment: 1645647-folder-props-test.diff

This should fix it.

Comment 4

[ISHIKAWA, Chiaki]

(In reply to Alessandro Castellani (:aleca) from comment #3)

Created attachment 9158360 [details] [diff] [review]
1645647-folder-props-test.diff

This should fix it.

So we avoid using window.arguments[0].previewSelectedColorCallback is not valid. That fixes the superficial problem.
Actually, I am not that familiar with the code.

Just to be sure, I ask the following question.
In the folderProps.js, there is a function folderPropsOnLoad() that follows the modified part.
On line 225, it checks for the null-ness of window.arguments first, and then window.arguments[0], even.
We don't have to check for these basic values on 218? Or do we?

Also, I noticed that in virutalFolderProperties.js,
(aside from the issue of window.arguments, and window.arguments[0] check mentioned above),
we check window.arguments[0].folder, presumably the folder for virtual folder(?) [I don't use unified folder and not familiar with it.].
The parallel of checking the value of folder in folderProps.js would be the checking the value of gMsgFolder.
Are we sure that this variable is always non-null or defined?
I have to ask just to be sure. I have seen some variables that start with 'g', presumably global variable, get undefined after certain operation and caused errors in tests.
gMsgFolder gets assigned in https://searchfox.org/comm-central/source/mailnews/base/content/folderProps.js#232
But if we have to check for window.arguments.[0].folder, then gMsgFolder may not get set (I have no idea what value it holds after the if on line 232 is not taken.
gMsgFolder may not be initialized or gMsgFolder retains the old value that was used before???)

Again, I am not familiar with the code, and so I assume the proper checking of previewSelectedColorCallback certainly avoids the function call, and
window.arguments[0].folder check is to make sure the target folder is defined.
However, if we need to check for the window.arguments[0].folder being defined and non null, then we probably may want to check for gMsgFolder on https://searchfox.org/comm-central/source/mailnews/base/content/folderProps.js#218 ? No ?

For example, there is |if (gMsgFolder)| on https://searchfox.org/comm-central/source/mailnews/base/content/folderProps.js#262.
However, on https://searchfox.org/comm-central/source/mailnews/base/content/folderProps.js#366, gMsgFolder.retentionSettings is referenced without checking gMsgFolder first.
I see some confusion here.

I wish I could patch this locally quickly, but due to the massive formatting only changes to many files committed in the last couple of days, my local patches do not apply cleanly any more and I am in the middle of fixing them and it may take a couple of days.

In the meantime, if you can explain the above and maybe put in short comments to make it easier for the future maintainer to understand the transition of value of gMsgFolder, say, that would be certainly a plus.

TIA

Comment 5

[Alessandro Castellani [:aleca]]

(In reply to ISHIKAWA, Chiaki from comment #4)

So we avoid using window.arguments[0].previewSelectedColorCallback is not valid. That fixes the superficial problem.
Actually, I am not that familiar with the code.

Since in the tests we're using a shared helper function that opens the folderProps dialog, that function doesn't come with the custom arguments recently implemented.
This condition prevents running those callback methods if not available.

In the folderProps.js, there is a function folderPropsOnLoad() that follows the modified part.
On line 225, it checks for the null-ness of window.arguments first, and then window.arguments[0], even.
We don't have to check for these basic values on 218? Or do we?

Those are unrelated as we're not storing any checks or conditional variable to know if the previewSelectedColorCallback callback is available.
We call that callback only if the user cancels the dialog.

Also, I noticed that in virutalFolderProperties.js,
(aside from the issue of window.arguments, and window.arguments[0] check mentioned above),
we check window.arguments[0].folder, presumably the folder for virtual folder(?) [I don't use unified folder and not familiar with it.].

Correct.

The parallel of checking the value of folder in folderProps.js would be the checking the value of gMsgFolder.
Are we sure that this variable is always non-null or defined?
I have to ask just to be sure. I have seen some variables that start with 'g', presumably global variable, get undefined after certain operation and caused errors in tests.
gMsgFolder gets assigned in https://searchfox.org/comm-central/source/mailnews/base/content/folderProps.js#232
But if we have to check for window.arguments.[0].folder, then gMsgFolder may not get set (I have no idea what value it holds after the if on line 232 is not taken.
gMsgFolder may not be initialized or gMsgFolder retains the old value that was used before???)

If the gMsgFolder is not defined the value is null.
When this dialog is closed, the variable doesn't persist.

Again, I am not familiar with the code, and so I assume the proper checking of previewSelectedColorCallback certainly avoids the function call, and
window.arguments[0].folder check is to make sure the target folder is defined.
However, if we need to check for the window.arguments[0].folder being defined and non null, then we probably may want to check for gMsgFolder on https://searchfox.org/comm-central/source/mailnews/base/content/folderProps.js#218 ? No ?

Sure, we can add that extra condition to be absolutely sure. If you follow those callbacks you will notice that we have a check in place to see if the folder passed is not null, so that extra condition is not absolutely necessary.

For example, there is |if (gMsgFolder)| on https://searchfox.org/comm-central/source/mailnews/base/content/folderProps.js#262.
However, on https://searchfox.org/comm-central/source/mailnews/base/content/folderProps.js#366, gMsgFolder.retentionSettings is referenced without checking gMsgFolder first.
I see some confusion here.

Indeed, that's strange. I'm not sure what was the thinking behind that as I didn't originally coded that section.
You're more than welcome to open a dedicated bug and patch it.

In the meantime, if you can explain the above and maybe put in short comments to make it easier for the future maintainer to understand the transition of value of gMsgFolder, say, that would be certainly a plus.

We don't comment the fact that global variables belonging to a file initialized in a dialog gets "destroyed" once the dialog is closed, that's implied.
I'm not against adding those type of comment, but that's not my area and I can't make this type of decisions.

Thanks for highlighting these problems in this section, indeed the folderProps.js file should receive some love and optimization.
For now, we're gonna land this tiny patch to prevent the error when running tests.

Comment 6

[Alessandro Castellani [:aleca]]

Attachment: 1645647-folder-props-test.diff

Comment 7

[Geoff Lankow (:darktrojan)]

Comment on attachment 9158723 [details] [diff] [review]
1645647-folder-props-test.diff

LGTM.

Comment 8

[Alessandro Castellani [:aleca]]

Comment on attachment 9158723 [details] [diff] [review]
1645647-folder-props-test.diff

[Approval Request Comment]
Regression caused by (bug #): bug 1637668
User impact if declined: very low as it affects the console output during a mochitest run but not the actual test.
Testing completed (on c-c, etc.): soon on c-c
Risk to taking this patch (and alternatives if risky): very low

Comment 9

[ISHIKAWA, Chiaki]

(In reply to Alessandro Castellani (:aleca) from comment #5)

(In reply to ISHIKAWA, Chiaki from comment #4)

So we avoid using window.arguments[0].previewSelectedColorCallback is not valid. That fixes the superficial problem.
Actually, I am not that familiar with the code.

Since in the tests we're using a shared helper function that opens the folderProps dialog, that function doesn't come with the custom arguments recently implemented.
This condition prevents running those callback methods if not available.

In the folderProps.js, there is a function folderPropsOnLoad() that follows the modified part.
On line 225, it checks for the null-ness of window.arguments first, and then window.arguments[0], even.
We don't have to check for these basic values on 218? Or do we?

Those are unrelated as we're not storing any checks or conditional variable to know if the previewSelectedColorCallback callback is available.
We call that callback only if the user cancels the dialog.

Also, I noticed that in virutalFolderProperties.js,
(aside from the issue of window.arguments, and window.arguments[0] check mentioned above),
we check window.arguments[0].folder, presumably the folder for virtual folder(?) [I don't use unified folder and not familiar with it.].

Correct.

The parallel of checking the value of folder in folderProps.js would be the checking the value of gMsgFolder.
Are we sure that this variable is always non-null or defined?
I have to ask just to be sure. I have seen some variables that start with 'g', presumably global variable, get undefined after certain operation and caused errors in tests.
gMsgFolder gets assigned in https://searchfox.org/comm-central/source/mailnews/base/content/folderProps.js#232
But if we have to check for window.arguments.[0].folder, then gMsgFolder may not get set (I have no idea what value it holds after the if on line 232 is not taken.
gMsgFolder may not be initialized or gMsgFolder retains the old value that was used before???)

If the gMsgFolder is not defined the value is null.
When this dialog is closed, the variable doesn't persist.

Again, I am not familiar with the code, and so I assume the proper checking of previewSelectedColorCallback certainly avoids the function call, and
window.arguments[0].folder check is to make sure the target folder is defined.
However, if we need to check for the window.arguments[0].folder being defined and non null, then we probably may want to check for gMsgFolder on https://searchfox.org/comm-central/source/mailnews/base/content/folderProps.js#218 ? No ?

Sure, we can add that extra condition to be absolutely sure. If you follow those callbacks you will notice that we have a check in place to see if the folder passed is not null, so that extra condition is not absolutely necessary.

For example, there is |if (gMsgFolder)| on https://searchfox.org/comm-central/source/mailnews/base/content/folderProps.js#262.
However, on https://searchfox.org/comm-central/source/mailnews/base/content/folderProps.js#366, gMsgFolder.retentionSettings is referenced without checking gMsgFolder first.
I see some confusion here.

Indeed, that's strange. I'm not sure what was the thinking behind that as I didn't originally coded that section.
You're more than welcome to open a dedicated bug and patch it.

In the meantime, if you can explain the above and maybe put in short comments to make it easier for the future maintainer to understand the transition of value of gMsgFolder, say, that would be certainly a plus.

We don't comment the fact that global variables belonging to a file initialized in a dialog gets "destroyed" once the dialog is closed, that's implied.
I'm not against adding those type of comment, but that's not my area and I can't make this type of decisions.

Thanks for highlighting these problems in this section, indeed the folderProps.js file should receive some love and optimization.
For now, we're gonna land this tiny patch to prevent the error when running tests.

Thank you for the detailed explanation.
I might check for gMsgFolder handling once I take care of my local patches to cope with the massive reformatting.

Comment 10

[Magnus Melin [:mkmelin] (away, back June 30)]

(In reply to Alessandro Castellani (:aleca) from comment #5)

Since in the tests we're using a shared helper function that opens the folderProps dialog, that function doesn't come with the custom arguments recently implemented.
This condition prevents running those callback methods if not available.

Hmm, sounds like we should make sure the tests test what we really are doing? What are we calling?

Comment 11

[Alessandro Castellani [:aleca]]

This is the test we're calling: https://searchfox.org/comm-central/rev/a8444d358c7abb921d81ee97d73b6f6ba26c7c8a/mail/test/browser/folder-pane/browser_folderPaneConsumers.js#48

The issue presents when the dialog gets cancelled.
We're opening the dialog with the helper method wait_for_modal_dialog() which doesn't support callbacks, and since we implemented the previewSelectedColorCallback() method to reset the icon color if the dialog gets cancelled, the error appears because the test cancels the dialog.

Comment 12

[Magnus Melin [:mkmelin] (away, back June 30)]

Slightly wrong explanation I think, but the patch should be ok. It handles the situation when you do New | Search Folder... and then cancel. Like the test.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/51c48cabdf4c
handle the case where the virtual folder properties dialog was not given an initial folder. r=darktrojan

Comment 14

[Wayne Mery (:wsmwk)]

Comment on attachment 9158723 [details] [diff] [review]
1645647-folder-props-test.diff

Approved for beta

Comment 15

[Rob Lemley [:rjl]]

Thunderbird 78.0b4:
https://hg.mozilla.org/releases/comm-beta/rev/2be1591aed0a