Closed Bug 955250 Opened 10 years ago Closed 10 years ago

Don't disconnect when failing to parse a message

Categories

(Chat Core :: IRC, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: aleth, Assigned: clokep)

Details

Attachments

(1 file, 1 obsolete file)

*** Original post on bio 1817 at 2012-11-22 14:12:00 UTC ***

This has come up a couple of times, most recently in the bug reported by qlum.
*** Original post on bio 1817 at 2012-11-22 14:15:31 UTC ***

(Looks like that bug wasn't filed, so I added bug 955251 (bio 1818))
Attached patch Untested patch (obsolete) — Splinter Review
*** Original post on bio 1817 as attmnt 2124 at 2012-11-22 18:37:00 UTC ***

I think this should fix the issue...
Attachment #8353885 - Flags: feedback?
Assignee: nobody → clokep
Status: NEW → ASSIGNED
Comment on attachment 8353885 [details] [diff] [review]
Untested patch

*** Original change on bio 1817 attmnt 2124 at 2012-11-22 18:40:48 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353885 - Flags: feedback? → feedback?(aleth)
Comment on attachment 8353885 [details] [diff] [review]
Untested patch

*** Original change on bio 1817 attmnt 2124 at 2012-11-22 19:23:28 UTC ***

This looks like it should do the trick, but it does really need testing ;)

I don't think we should log an unhandled message warning when we couldn't parse the message in the first place, so I'd suggest moving the if (!handled) inside the try.
Attachment #8353885 - Flags: feedback?(aleth) → feedback+
*** Original post on bio 1817 at 2012-11-23 04:03:15 UTC ***

(In reply to comment #3)
> Comment on attachment 8353885 [details] [diff] [review] (bio-attmnt 2124) [details]
> Untested patch
> 
> This looks like it should do the trick, but it does really need testing ;)
I agree! I'll see if I can figure out a way to test it soon...

> I don't think we should log an unhandled message warning when we couldn't parse
> the message in the first place, so I'd suggest moving the if (!handled) inside
> the try.
You're probably right, we're already displaying the error in that case saying it couldn't be parsed. Thanks!
*** Original post on bio 1817 at 2012-11-23 14:33:03 UTC ***

(In reply to comment #4)
> > This looks like it should do the trick, but it does really need testing ;)
> I agree! I'll see if I can figure out a way to test it soon...
Intentionally breaking the regex should do the trick.

> > I don't think we should log an unhandled message warning when we couldn't parse
> > the message in the first place, so I'd suggest moving the if (!handled) inside
> > the try.
> You're probably right, we're already displaying the error in that case saying
> it couldn't be parsed. Thanks!
On second thoughts, would that mean we no longer get the message displayed if there is an error in some message handler?
Attached patch PatchSplinter Review
*** Original post on bio 1817 as attmnt 2130 at 2012-11-25 04:55:00 UTC ***

I tested this by making the regexp not match an IRC message.
Attachment #8353891 - Flags: review?(aleth)
Comment on attachment 8353885 [details] [diff] [review]
Untested patch

*** Original change on bio 1817 attmnt 2124 at 2012-11-25 04:55:43 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353885 - Attachment is obsolete: true
Comment on attachment 8353891 [details] [diff] [review]
Patch

*** Original change on bio 1817 attmnt 2130 at 2012-11-25 13:41:31 UTC ***

Tested it for errors in handlers and it works great :)
Thanks for this improvement.
Attachment #8353891 - Flags: review?(aleth) → review+
Whiteboard: [checkin-needed]
*** Original post on bio 1817 at 2012-11-25 23:38:41 UTC ***

Committed as http://hg.instantbird.org/instantbird/rev/e2ff32e19c78
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [checkin-needed]
Target Milestone: --- → 1.4
You need to log in before you can comment on or make changes to this bug.