Closed Bug 1644345 Opened 9 months ago Closed 9 months ago

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

Categories

(Thunderbird :: Message Compose Window, defect)

defect

Tracking

(thunderbird_esr78 fixed, thunderbird78 wontfix)

RESOLVED FIXED
Thunderbird 79.0
Tracking Status
thunderbird_esr78 --- fixed
thunderbird78 --- wontfix

People

(Reporter: ishikawa, Assigned: khushil324)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: regression)

Attachments

(3 files, 7 obsolete files)

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

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.

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
Assignee: nobody → mkmelin+mozilla
Keywords: regression
Regressed by: 378224
Attached patch bug1644345_compFields_null.patch (obsolete) — Splinter Review
Attachment #9155905 - Flags: review?(khushil324)
Attached patch bug1644345_compFields_null.patch (obsolete) — Splinter Review
Attachment #9155905 - Attachment is obsolete: true
Attachment #9155905 - Flags: review?(khushil324)
Attachment #9155906 - Flags: review?(khushil324)
Status: NEW → ASSIGNED
Component: General → Message Compose Window
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.
Attachment #9155906 - Flags: review?(khushil324)

We can do something like this.

Attachment #9155925 - Flags: feedback?(mkmelin+mozilla)
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
Attachment #9155925 - Flags: feedback?(mkmelin+mozilla)
Attachment #9155925 - Flags: feedback?
Attachment #9155925 - Flags: feedback+
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.
Attachment #9155925 - Flags: feedback? → feedback?(mkmelin+mozilla)
Assignee: mkmelin+mozilla → khushil324

(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.

Attachment #9155906 - Attachment is obsolete: true
Attachment #9155925 - Attachment is obsolete: true
Attachment #9155925 - Flags: feedback?(mkmelin+mozilla)
Attachment #9156593 - Flags: review?(mkmelin+mozilla)
Attachment #9156593 - Attachment is patch: true
Attachment #9156593 - Attachment is obsolete: true
Attachment #9156593 - Flags: review?(mkmelin+mozilla)
Attachment #9156657 - Flags: review?(mkmelin+mozilla)
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
Attachment #9156657 - Flags: review?(mkmelin+mozilla)

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

Attachment #9156659 - Flags: review?(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.
Attachment #9156659 - Flags: review?(khushil324)

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

This will create problems.

What problems? Anyway, over to you :)

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?
Attachment #9156657 - Attachment is obsolete: true
Attachment #9156659 - Attachment is obsolete: true
Attachment #9156669 - Flags: review?(mkmelin+mozilla)
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.
Attachment #9156669 - Flags: review?(mkmelin+mozilla) → review-

Updated your patch.

Attachment #9156669 - Attachment is obsolete: true
Attachment #9156678 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9156678 [details] [diff] [review]
bug1644345_compFields_null-updated-3.patch

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

Thanks!
Attachment #9156678 - Flags: review?(mkmelin+mozilla) → review+
Target Milestone: --- → Thunderbird 79.0

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/ec663969e246
fix command updating for cmd_toggleReturnReceipt. r=mkmelin

Status: ASSIGNED → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Summary: C-C TB JavaScript error: chrome://messenger/content/messengercompose/MsgComposeCommands.js, line 1068: TypeError: can't access property "compFields", gMsgCompose is null → C-C TB JavaScript error: .../MsgComposeCommands.js, line 1068: TypeError: can't access property "compFields", gMsgCompose is null - Fix command updating for cmd_toggleReturnReceipt
Blocks: 1646875
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.
Attachment #9156678 - Flags: approval-comm-esr78?
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?)
Flags: needinfo?(rob)
Attachment #9156678 - Flags: approval-comm-esr78? → approval-comm-esr78+

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

Flags: needinfo?(rob)

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

You need to log in before you can comment on or make changes to this bug.