Closed Bug 1559793 Opened 1 year ago Closed 1 year ago

OTR: JS exception in OTRUI.jsm:497, this.globalDoc is null

Categories

(Chat Core :: Security: OTR, defect, P1, major)

Tracking

(thunderbird68 fixed, thunderbird69 fixed, thunderbird70 fixed)

RESOLVED FIXED
Instantbird 70
Tracking Status
thunderbird68 --- fixed
thunderbird69 --- fixed
thunderbird70 --- fixed

People

(Reporter: KaiE, Assigned: aleca)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

When receiving an OTR message, but the conversation window hasn't been loaded yet, we get a call to OTRUI.notifyUnverified(), but this.globalDoc hasn't been set yet.

Two consequences:

  • exception in JS console
  • yellow notification box not shown
Status: NEW → ASSIGNED
Severity: normal → major
Priority: -- → P1
Attached patch 1559793-otrui.patch (obsolete) — Splinter Review

This patch fixes the issue by forcing the opening of the Chat Tab if it hasn't been opened before.
I think we're forced to open the Chat Tab in order to initialize the OTR, which it can't happen without the loaded UI. Am I wrong?

Another UX related issue I noticed is that if the user has the Chat Tab opened, but the focus is on another Tab, the request of verification doesn't produce any visible notification, like a count bubble or the highlight of the conversation.
The Notification Box with the request for verification gets properly generated tho, so it should be only a matter of hooking up the OTR to use the same chat bubble notification happening in a regular conversation.
Will do that in a follow up bug.

P.S.: I had to wrap all the conditions around brackets as eslint was screaming at me.

Attachment #9076255 - Flags: review?(kaie)

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

This patch fixes the issue by forcing the opening of the Chat Tab if it hasn't been opened before.

Thanks, the patch fixes the issue for me.

I suggest a small rewrite to avoid the duplicate call to OTRUI.notifyUnverified:

        if (!this.globalDoc) {
          let win = Services.wm.getMostRecentWindow("mail:3pane");
          if (!win) {
            return;
          }
          win.focus();
          win.showChatTab();
          this.globalDoc = win.document;
        }
        OTRUI.notifyUnverified(aObject, aMsg);
        break;

I think we're forced to open the Chat Tab in order to initialize the OTR, which it can't happen without the loaded UI. Am I wrong?

I'm fine with opening the chat tab in this scenario.

To answer your question, receiving of OTR messages works just fine, even with the chat tab closed. As an example, if your buddy is already verified, your code path isn't triggered. If the chat tab isn't open yet, the OTR messages are received fine.

This means, with your change, if the chat tab is closed:

  • receiving an OTR message from an unverified contact will open the chat tab
  • receiving an OTR message from a verified contact will NOT open the chat tab. Rather, the status bar icon for chats will show a red notification icon.

It's a small inconsistency, but I think fixing this bug is more important. I recommend to take your patch. If you'd like to improve the inconsistency, we can do that in a follow-up bug.

Another UX related issue I noticed is that if the user has the Chat Tab opened, but the focus is on another Tab, the request of verification doesn't produce any visible notification, like a count bubble or the highlight of the conversation.
The Notification Box with the request for verification gets properly generated tho, so it should be only a matter of hooking up the OTR to use the same chat bubble notification happening in a regular conversation.
Will do that in a follow up bug.

Thanks!

Comment on attachment 9076255 [details] [diff] [review]
1559793-otrui.patch

r=kaie, but removing the flag, because of the suggested change
Attachment #9076255 - Flags: review?(kaie)

Alex, please set r+ if you agree with my small change.

Then we'll need checkin-needed and approval-esr68

Attachment #9076862 - Flags: review?(alessandro)
Attachment #9076255 - Attachment is obsolete: true
Attachment #9076862 - Flags: review?(alessandro)
Attachment #9076862 - Flags: review+
Attachment #9076862 - Flags: approval-comm-esr68?
Keywords: checkin-needed
Comment on attachment 9076862 [details] [diff] [review]
1559793-otrui-v2.patch

no uplifts directly to comm-esr68 for the moment, I'll do a bulk move from the 68 beta. What a pain we don't have a release flag to distinguish the beta 68 from beta 69 :-(
Attachment #9076862 - Flags: approval-comm-esr68? → approval-comm-beta?
Attachment #9076862 - Flags: approval-comm-beta? → approval-comm-beta+
Target Milestone: --- → Instantbird 70

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/42e87103b398
Fix OTRUI.jsm exception this.globalDoc is null. r=kaie

Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Keywords: checkin-needed
Resolution: --- → FIXED
Component: General → Security: OTR
You need to log in before you can comment on or make changes to this bug.