1746863 - Incoming verification notification bar does not disappear after verification is completed

Status: RESOLVED FIXED

(Chat Core :: Security: OTR, defect)

Description

[Martin Giger [:freaktechnik]]

When verifying someone's identity in OTR the bar prompting you to verify it doesn't disappear after completing the procedure.

The following error also gets logged a lot, though that may be for all notification bars OTR shows:

JavaScript error: resource:///modules/OTRUI.jsm, line 695: TypeError: can't access property "setAttribute", notification.messageDetails is undefined

Comment 1

[Alessandro Castellani [:aleca]]

I'm having an issue I'm not sure is related to the whole OTR breakage.
When trying to start and encrypted conversation, I get this message inline the chat:

?OTRv2?
[e-mail] has requested an Off-the-Record (OTR) encrypted conversation.
However, you do not have a plugin to support that. See https://en.wikipedia.org/wiki/Off-the-Record_Messaging 
for more information.

What am I missing?

Comment 2

[Alessandro Castellani [:aleca]]

Nevermind, I needed to apply Magnus' patch to both instances of TB to make it work.

Comment 3

[Alessandro Castellani [:aleca]]

I'm working on this and I'm gonna push a WIP patch just so I can easily test it on multiple machines.
The notification usage of OTR is a bit messed up, specifically for the fact that we show it in the right sidebar, but then we sometimes use the notifications on top of the conversation for general requests, but then sometimes we hide them and sometimes we clear them, and so on.
Lots of inconsistencies and repeated code for no real reason.
I worked on this section in the past and I think it shows how little I properly understood the notifications API and how to use them in a sane way.

Anyway, I'm almost there with the fix, and should have it ready for tomorrow.

Comment 4

[Alessandro Castellani [:aleca]]

Attachment: WIP-bug1746863-otr-notification.diff

Comment 5

[Alessandro Castellani [:aleca]]

Attachment: Bug 1746863 - [OTR] Fix incoming verification notifications.r=freaktechnik

Comment 6

[Alessandro Castellani [:aleca]]

Patch updated to make those notification usable again.
There are still a lot of things that need polishing and fixes, but this at least makes it usable.

Among all the changes, these are the most relevant:

• I made all notification non-dismissable, except the final ones (success, fail, error).
• I added the "Ignore" button for those notifications that require a verification action in order to properly close the encryption request when the user closes the notification.
• I'm using the main notification area above the conversation pane, since notifications don't stack on top of each other anymore, we can use the same area for all chat related notifications.
• All verification requests have a "WARNING" flag to look yellow. I think it's fine if we agree that the importance level of these requests is higher than a regular "INFO" level.

Let me know if I miss some use case, or if something looks wrong.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/49a1fbc3bd30
[OTR] Fix incoming verification notifications.r=freaktechnik

Comment 8

[Alessandro Castellani [:aleca]]

Comment on attachment 9256418 [details]
Bug 1746863 - [OTR] Fix incoming verification notifications.r=freaktechnik

[Approval Request Comment]
Regression caused by (bug #): didn't find the regression
User impact if declined: Off-the-record verification requests are unusable and it's impossible to start an encrypted conversation.
Testing completed (on c-c, etc.): on c-c
Risk to taking this patch (and alternatives if risky): it should be fairly low as OTR has been broken on 91 for some time.

Comment 9

[Kai Engert (:KaiE:)]

Comment on attachment 9256418 [details]
Bug 1746863 - [OTR] Fix incoming verification notifications.r=freaktechnik

Patch does not apply cleanly to esr91. Please provide a merged patch prior to asking for uplift.

Comment 10

[Wayne Mery (:wsmwk)]

Comment on attachment 9256418 [details]
Bug 1746863 - [OTR] Fix incoming verification notifications.r=freaktechnik

No more 96 betas, so this will go out in 97 beta

Comment 11

[Alessandro Castellani [:aleca]]

There are a lot of substantial differences between 91 and trunk related to the chat code.
I'll need to create a dedicated 91 patch from scratch as the changes done on trunk don't translate at all to 91 due to many missing methods and missing files that have been added in the past months.
I'll work on this and ping Kai for a 91 specific review.

Marking this bug temporarily REOPENED so it's easier for me to track it.

Comment 12

[Alessandro Castellani [:aleca]]

Attachment: otr-fix-91.diff

Here's the updated patch for 91.
Lots of differences so it wasn't a complete 1 to 1 port, but everything seems to work as expected for the notifications.

Comment 13

[Kai Engert (:KaiE:)]

Comment on attachment 9258401 [details] [diff] [review]
otr-fix-91.diff

Looks ok to me, the changes are similar to the reviewed comm-central patch, and a brief test with a local esr91 build gave me notifications that looked fine.

Comment 14

[Kai Engert (:KaiE:)]

Comment on attachment 9258401 [details] [diff] [review]
otr-fix-91.diff

re-requesting uplift (from Alessandro)

Comment 15

[Wayne Mery (:wsmwk)]

Comment on attachment 9258401 [details] [diff] [review]
otr-fix-91.diff

[Triage Comment]
Approved for esr91

Comment 16

[Rob Lemley [:rjl]]

Thunderbird 91.5.1:
https://hg.mozilla.org/releases/comm-esr91/rev/41e456d8f39d

Comment 17

[Rob Lemley [:rjl]]

Thunderbird 91.5.1:
https://hg.mozilla.org/releases/comm-esr91/rev/d187626d32b7