Closed Bug 1724940 Opened 1 year ago Closed 1 year ago

Event handlers linger if Exchange credentials dialog is shown twice in one session

Categories

(Thunderbird :: Account Manager, defect, P2)

Thunderbird 91

Tracking

(thunderbird_esr91+ fixed, thunderbird92+ fixed)

RESOLVED FIXED
93 Branch
Tracking Status
thunderbird_esr91 + fixed
thunderbird92 + fixed

People

(Reporter: neil, Assigned: aleca)

References

(Regression)

Details

Attachments

(1 file, 1 obsolete file)

The code to prompt the user to log in to their Exchange autodiscover server does not uninstall all, or in some cases, any, of its event handlers. This results in outdated event handlers lingering which might interfere with a potential reuse of the dialog in the same session. A typical reason why the dialog would be reused is if the autodiscover server needs the username in domain\user format, in which case the initial login would fail and the user would be prompted to send the new credentials to the autodiscover server.

Attached patch Proposed patch (obsolete) — Splinter Review

Using event handler properties seemed to be the easiest way to ensure that only the current event handlers are active, but I'm open to other approaches.

Attachment #9235601 - Flags: review?(alessandro)
Attachment #9235601 - Flags: feedback+

Ah, good find.
The { once: true } attribute of the addEventListener() method only removes the event if the button is actually clicked, but if the user dismisses the dialog, more click events will probably be stacked upon reopening.

I'm okay with this solution, but maybe we could remove the event listener when the dialog is closed.
This is most likely a code styling decision.

Pinging Magnus to see which one he thinks we should adopt.
And we should also do the same for the _showCalendarDialog() method.

Flags: needinfo?(mkmelin+mozilla)

The patch seems like maybe the best solution to me.

Flags: needinfo?(mkmelin+mozilla)
Comment on attachment 9235601 [details] [diff] [review]
Proposed patch

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

All right, let's go with this approach.
Could you please update in this patch also the event listeners for the calendar dialog? So we can fix the identical problem.

here: https://searchfox.org/comm-central/rev/5c9b0b497bd70b605446d82d5c556bdd417a4021/mail/components/accountcreation/content/accountSetup.js#2546
and here: https://searchfox.org/comm-central/rev/5c9b0b497bd70b605446d82d5c556bdd417a4021/mail/components/accountcreation/content/accountSetup.js#2569

Also, little nit, please update the commit message to:
"Bug 1724940 - Use onclick instead of event listeners in account setup tab. r=aleca"
Attachment #9235601 - Flags: review?(alessandro)

Apologies Neil for taking over this bug.
This issue is fairly important so I wanted to fix it as soon as possible.

Assignee: neil → alessandro
Blocks: tb91found
Severity: -- → S2
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: --- → 93 Branch

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/fcf8c3d07382
Use onclick instead of event listeners in account setup tab to prevent lingering handlers. r=mkmelin

Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Attachment #9235601 - Attachment is obsolete: true

Comment on attachment 9236243 [details]
Bug 1724940 - Use onclick instead of event listeners in account setup tab to prevent lingering handlers. r=mkmelin

[Approval Request Comment]
Regression caused by (bug #): -
User impact if declined: Issues with the dialog buttons when setting up an exchange account or multiple calendars
Testing completed (on c-c, etc.): on c-c
Risk to taking this patch (and alternatives if risky): low

Attachment #9236243 - Flags: approval-comm-beta?

Comment on attachment 9236243 [details]
Bug 1724940 - Use onclick instead of event listeners in account setup tab to prevent lingering handlers. r=mkmelin

[Approval Request Comment]
Regression caused by (bug #): -
User impact if declined: Issues with the dialog buttons when setting up an exchange account or multiple calendars
Testing completed (on c-c, etc.): onc-c
Risk to taking this patch (and alternatives if risky): low

Attachment #9236243 - Flags: approval-comm-esr91?

Comment on attachment 9236243 [details]
Bug 1724940 - Use onclick instead of event listeners in account setup tab to prevent lingering handlers. r=mkmelin

[Triage Comment]
Approved for beta

Attachment #9236243 - Flags: approval-comm-beta? → approval-comm-beta+

We're building 91.0.2 today, but this hasn't yet been on beta. So this will be eligible for 91.0.3 which will probably happen next week.

Comment on attachment 9236243 [details]
Bug 1724940 - Use onclick instead of event listeners in account setup tab to prevent lingering handlers. r=mkmelin

[Triage Comment]
Approved for esr91

Attachment #9236243 - Flags: approval-comm-esr91? → approval-comm-esr91+

(In reply to Alessandro Castellani from comment #6)

Apologies Neil for taking over this bug.
This issue is fairly important so I wanted to fix it as soon as possible.

No need to apologise, I simply haven't been responsive because I've been recovering from an emergency operation.

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