1644345 - C-C TB JavaScript error: .../MsgComposeCommands.js, line 1068: TypeError: can't access property "compFields", gMsgCompose is null - Fix command updating for cmd_toggleReturnReceipt

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[ISHIKAWA, Chiaki]

Attachment: Excerpt from the log to show which tests produced the error.

I get the following error 10 times in local mochitest log of FULL
DEBUG version of C-C TB.

JavaScript error: chrome://messenger/content/messengercompose/MsgComposeCommands.js, line 1068: TypeError: can't access property "compFields", gMsgCompose is null

I have not seen the bug until June 6th. The previous job on May 30 did
not have it. So this has crept in the last week or so.

The error can be observed in try-c-c job, too. For example,
benc's linux64 debug job,
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=43227542500df69c784d0a6d3c11cd8b46cfde35
https://firefoxci.taskcluster-artifacts.net/NZxVbf0KToq04gGMNbbpvQ/0/public/logs/live_backing.log

I am attaching the excerpt from the log
created by

egrep "(TEST_START|TEST_END|Leaving test|Entering test|gMsgCompose is null)" /FF-NEW/log1234-mochitest.txt

to show which tests produced the error.
I really cringe at the failure of mochitest framework to fail the test that produces such errors. Note that some of the tests did not report any errors.

Bandaid fix ala Bug 1250605 may be in order here.

TIA

Comment 1

[ISHIKAWA, Chiaki]

Attachment: console.trace() of the errors.

The console.trace() stackdump when the error occurs.
There are two patterns. The first one does not have
chrome://messenger/content/mailWidgets.js 892 focus
line.

The rest has the line and are identical.

Comment 2

[ISHIKAWA, Chiaki]

I think the number 10 I mention as the frequency of occurrences are actually twice the real ocurrences. There are duplicate error messages reported by mochitest framework. So it turned out there were five real errors.

The attachment in comment 1 has only 4 of the stacktraces.
I failed to add the following stacktrace which are identical to the last three stacktraces in the previous attachment up to the lines marked with "*" at the beginning of the line.

 console.trace:
 chrome://messenger/content/messengercompose/MsgComposeCommands.js 1069 isEnabled
 chrome://messenger/content/messengercompose/MsgComposeCommands.js 1150 isCommandEnabled
 chrome://global/content/globalOverlay.js 84 goUpdateCommand
 chrome://messenger/content/messengercompose/MsgComposeCommands.js 1576 updateComposeItems
 chrome://messenger/content/messengercompose/MsgComposeCommands.js 1537 CommandUpdate_MsgCompose
 chrome://messenger/content/messengercompose/messengercompose.xhtml 1 oncommandupdate
 chrome://messenger/content/mailWidgets.js 892 focus
 chrome://messenger/content/messengercompose/MsgComposeCommands.js 7137 MakeFromFieldEditable
 chrome://messenger/content/messengercompose/MsgComposeCommands.js 3539 ComposeStartup
 chrome://messenger/content/messengercompose/MsgComposeCommands.js 3771 ComposeLoad
 chrome://messenger/content/messengercompose/messengercompose.xhtml 1 onload
 resource://testing-common/mozmill/utils.jsm 425 waitFor
 resource://testing-common/mozmill/WindowHelpers.jsm 262 WindowWatcher_waitForWindowOpen
 resource://testing-common/mozmill/WindowHelpers.jsm 633 wait_for_new_window
 resource://testing-common/mozmill/ComposeHelpers.jsm 273 wait_for_compose_window
*resource://testing-common/mozmill/ComposeHelpers.jsm 90 open_compose_with_reply
 chrome://mochitests/content/browser/comm/mail/test/browser/composition/browser_replyCatchAll.js 219 test_reply_identity_selection
 chrome://mochikit/content/browser-test.js 1064 Tester_execTest/<
 chrome://mochikit/content/browser-test.js 1104 Tester_execTest
 chrome://mochikit/content/browser-test.js 927 nextTest/<
 chrome://mochikit/content/tests/SimpleTest/SimpleTest.js 918 SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<
 JavaScript error: chrome://messenger/content/messengercompose/MsgComposeCommands.js, line 1071: TypeError: can't access property "compFields", gMsgCompose is null

Comment 3

[Magnus Melin [:mkmelin]]

Attachment: bug1644345_compFields_null.patch

Comment 4

[Magnus Melin [:mkmelin]]

Attachment: bug1644345_compFields_null.patch

Comment 5

[Khushil Mistry [:khushil324]]

Comment on attachment 9155906 [details] [diff] [review]
bug1644345_compFields_null.patch

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

We should not keep the logic of UI update in the isEnabled function.

Comment 6

[Khushil Mistry [:khushil324]]

Attachment: bug1644345_compFields_null-updated.patch

We can do something like this.

Comment 7

[Thomas D. (:thomas8)]

Comment on attachment 9155925 [details] [diff] [review]
bug1644345_compFields_null-updated.patch

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

Yeah, if you so wish... LGTM, this should work (have you tested it?).
Let's polish some nits. Thank you!

::: mail/components/compose/content/MsgComposeCommands.js
@@ +5266,5 @@
>  function ToggleReturnReceipt() {
>    let msgCompFields = gMsgCompose.compFields;
>    if (msgCompFields) {
>      msgCompFields.returnReceipt = !msgCompFields.returnReceipt;
> +    UpdateReturnReceiptItems(msgCompFields.returnReceipt);

camelCase pls.
I see no advantage of passing the boolean into the function, what if we keep callers simple and just retrieve the current status inside the function? (see below)

@@ +5271,5 @@
>      gReceiptOptionChanged = true;
>    }
>  }
>  
> +function UpdateReturnReceiptItems(returnReceipt) {

camelCase please, and add jsdoc function comment.
I suggest to remove the argument (here and in callers) and simply retrieve it here:

let returnReceipt = gMsgCompose.compFields.returnReceipt;

I think Magnus prefers lean callers, too (e.g. "don't pass `this` when we can get it in function via event.target").

@@ +5273,5 @@
>  }
>  
> +function UpdateReturnReceiptItems(returnReceipt) {
> +  for (let item of document.querySelectorAll(`menuitem[command="cmd_toggleReturnReceipt"],
> +                                                 toolbarbutton[command="cmd_toggleReturnReceipt"]`)) {

Reformat with Prettier pls

@@ +6983,5 @@
>          prevReceipt == msgCompFields.returnReceipt &&
>          prevReceipt != newReceipt
>        ) {
>          msgCompFields.returnReceipt = newReceipt;
> +        UpdateReturnReceiptItems(msgCompFields.returnReceipt);

camelCase, remove argument

Comment 8

[Thomas D. (:thomas8)]

Comment on attachment 9155925 [details] [diff] [review]
bug1644345_compFields_null-updated.patch

I suggested nitfix (to use lean callers without needless passing of arguments) in my review, comment 7.

Comment 9

[Magnus Melin [:mkmelin]]

(In reply to Khushil Mistry [:khushil324] from comment #5)

We should not keep the logic of UI update in the isEnabled function.

Well, I do kind of agree. It's just that it best reflects what XUL do/did.

Maybe you could just add an optional parameter to ToggleReturnReceipt.? If the parameter == undefined we do what we do now. Else obey the paramater. That way we don't have to introduce another helper function either.

Comment 10

[Khushil Mistry [:khushil324]]

Attachment: bug1644345_compFields_null-updated-0.patch

Comment 11

[Khushil Mistry [:khushil324]]

Attachment: bug1644345_compFields_null-updated-1.patch

Comment 12

[Magnus Melin [:mkmelin]]

Comment on attachment 9156657 [details] [diff] [review]
bug1644345_compFields_null-updated-1.patch

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

Since the function is named toggle, maybe better to let it toggle by default.

I was just looking at this, will attach what I mean in a sec

Comment 13

[Magnus Melin [:mkmelin]]

Attachment: bug1644345_compFields_null-updated.patch

OTOH we don't seem to even need the parameter...

Comment 14

[Khushil Mistry [:khushil324]]

Comment on attachment 9156659 [details] [diff] [review]
bug1644345_compFields_null-updated.patch

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

We also want to add ToggleReturnReceipt() in the ComposeStartup() to reflect the initial state user has set up in the preference or the account settings. In that, we just want to update the UI state. So the call should be msgCompFields.returnReceipt(msgCompFields.returnReceipt);

::: mail/components/compose/content/MsgComposeCommands.js
@@ +5367,4 @@
>      gReceiptOptionChanged = true;
> +  } else {
> +    if (msgCompFields.returnReceipt != forcedState) {
> +      gReceiptOptionChanged = true;

This will create problems.

@@ +7081,5 @@
>          prevReceipt == msgCompFields.returnReceipt &&
>          prevReceipt != newReceipt
>        ) {
>          msgCompFields.returnReceipt = newReceipt;
> +        ToggleReturnReceipt();

It should be ToggleReturnReceipt(newReceipt) and remove the msgCompFields.returnReceipt = newReceipt;
ToggleReturnReceipt();(without arguement) will re-toggle the state. 
But with ToggleReturnReceipt(newReceipt), gReceiptOptionChanged will be true and we don't want that here. It will show save changes dialog when the user tries to close the compose window but actually, user is not changing any state of the compose window.

Comment 15

[Magnus Melin [:mkmelin]]

(In reply to Khushil Mistry [:khushil324] from comment #14)

This will create problems.

What problems? Anyway, over to you :)

Comment 16

[Khushil Mistry [:khushil324]]

Comment on attachment 9156657 [details] [diff] [review]
bug1644345_compFields_null-updated-1.patch

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

::: mail/components/compose/content/MsgComposeCommands.js
@@ +5360,2 @@
>   */
> +function ToggleReturnReceipt(toggle) {

We can just rename the function to updateReturnReceiptUI. Will this work?

Comment 17

[Khushil Mistry [:khushil324]]

Attachment: bug1644345_compFields_null-updated-2.patch

Comment 18

[Magnus Melin [:mkmelin]]

Comment on attachment 9156669 [details] [diff] [review]
bug1644345_compFields_null-updated-2.patch

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

::: mail/components/compose/content/MsgComposeCommands.js
@@ +1070,4 @@
>          return !gWindowLocked;
>        },
>        doCommand() {
> +        updateReturnReceiptUI(true);

It's a toggle command. You need to be able to both check and uncheck it.

Comment 19

[Khushil Mistry [:khushil324]]

Attachment: bug1644345_compFields_null-updated-3.patch

Updated your patch.

Comment 20

[Magnus Melin [:mkmelin]]

Comment on attachment 9156678 [details] [diff] [review]
bug1644345_compFields_null-updated-3.patch

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

Thanks!

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/ec663969e246
fix command updating for cmd_toggleReturnReceipt. r=mkmelin

Comment 22

[Rob Lemley [:rjl]]

Comment on attachment 9156678 [details] [diff] [review]
bug1644345_compFields_null-updated-3.patch

[Approval Request Comment]
I suggest taking this patch for 78.0.1.
This bug came up while uplifting 1646857 which caused a merge conflict due to the change this patch makes in LoadIdentity(). Return receipts will likely have problems without this patch.

Comment 23

[Wayne Mery (:wsmwk)]

Comment on attachment 9156678 [details] [diff] [review]
bug1644345_compFields_null-updated-3.patch

Approved for esr78

This blocks which patch uplift?  (or is this not that bug?)

Comment 24

[Rob Lemley [:rjl]]

Not blocking, I just noticed this one while figuring out the mess I made in bug 1646857 with the uplift.

Comment 25

[Rob Lemley [:rjl]]

Thunderbird 78.1.0:
https://hg.mozilla.org/releases/comm-esr78/rev/f6c3514fed6b

Comment 26

[Rob Lemley [:rjl]]

Thunderbird 78.1.0:
https://hg.mozilla.org/releases/comm-esr78/rev/24387e8c4f6c

Comment 27

[Rob Lemley [:rjl]]

Thunderbird 78.1.0:
https://hg.mozilla.org/releases/comm-esr78/rev/c571a4fa74d5

Comment 28

[Rob Lemley [:rjl]]

Backout was due to problems related to bug 1646857 having landed on comm-esr78 before this one.