Closed Bug 1251959 Opened 4 years ago Closed 4 years ago

JavaScript error: chrome://chat/content/convbrowser.xml, line 321: TypeError: this.contentChatNode is null

Categories

(Instantbird :: Conversation, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Instantbird 47

People

(Reporter: aleth, Assigned: freaktechnik)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

Happens on shutdown. Seen in Thunderbird.
Assignee: nobody → martin
Blocks: 953706
Keywords: regression
Attached patch bug1251959-fix.patch (obsolete) — Splinter Review
This patch fixes the error message, however I've only requested feedback because I'm not sure if I should also ensure that the listener is removed whenever the browser's document is destroyed.

I'd assume the reference to the listener should also be destroyed if we have no more document so it should be fine.
Attachment #8724577 - Flags: feedback?(aleth)
(In reply to Martin Giger [:freaktechnik] from comment #1)
> This patch fixes the error message, however I've only requested feedback
> because I'm not sure if I should also ensure that the listener is removed
> whenever the browser's document is destroyed.

From the error message, it is clear the contentDocument exists. Under what circumstances is there no chat node?

> I'd assume the reference to the listener should also be destroyed if we have
> no more document so it should be fine.

If the document is destroyed, any listeners registered on its elements are also destroyed.
(In reply to aleth [:aleth] from comment #2)
> From the error message, it is clear the contentDocument exists. Under what
> circumstances is there no chat node?

nsIWebNavigation creates a nsIDOMDocument if there is none, so the content could be empty or similar. From what I've seen the "Chat" node is generated whenever a conversation is displayed, and I don't think the browser should display anything else (at lest I couldn't find anything).
Yes, this error happens when a convbrowser element is present but has not been initialized with a conversation yet.

So your patch is fine if you fix the comment and the indentation ;)
Attachment #8724577 - Flags: feedback?(aleth) → feedback+
Fixed the indentation and tried to improve the comment, but I'm not sure it's what you meant.
Attachment #8724577 - Attachment is obsolete: true
Attachment #8724926 - Flags: review?(aleth)
Comment on attachment 8724926 [details] [diff] [review]
bug1251959-fix-v2.patch

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

::: chat/content/convbrowser.xml
@@ +318,5 @@
>  
>              this.uninitMagicCopy();
>  
> +            if (this.contentChatNode) {
> +                // Only remove the listener if the content still exists

FYI We use 2 spaces indentation, not 4.

OK, I'll fix the misleading comment.

Please use standard commit messages (Bug N - Title. r=reviewer).

I'll take care of it on checkin this time.
Attachment #8724926 - Flags: review?(aleth) → review+
https://hg.mozilla.org/comm-central/rev/1a196501bc05394c7e70d5935762ab2f5d656e20
Bug 1251959 - Fix "this.contentChatNode is null" on shutdown. r=aleth
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Instantbird 47
Huh, I fixed the indentation, I must have totally forgotten to hg qrefresh or something. And I didn't think of adding bug and reviewer when creating the commit, sorry!
You need to log in before you can comment on or make changes to this bug.