Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Fix browser-crashing bug #71

Merged
merged 2 commits into from

3 participants

@clintharrison

If a user writes a line with an appropriate number of characters that ends in a tag with an entity immediately following, ajaxChat.breakLongWords() will insert the break string in the middle of the entity.

This is reproducible with the default settings of 32 character line lengths with the following message:
1234567890123456789012345678:)&

This results in the :) being replaced by the proper <img> tag while the &amp; does not get recognized as the beginning of an entity and &amp&#8203;; is the result.

Consequently the exception (DOM Exception 12--invalid syntax) is caught, but in the handling of it, there seems to be an infinite loop. In Firefox, Safari, and Chrome, memory usage will rise by about 100 MB per second until the task is killed. Removing the lines that report this error will stop the leak, but after a few minutes Chrome will crash anyway. The solution is to not produce invalid HTML in the first place ;)

Additionally a small fix, E_ALL is actually all errors but strict. This fixes that, too.

@Frug
Owner

Thank you for the report. I'll review and merge shortly.

Why would we want to show strict errors though? That's useful for development but showing strict warnings to users is usually not what they want. I was debating shutting off error reporting in the release code.

@clintharrison

Well, if you're going to show any errors at all, I would say just go ahead and show them all. In production though you probably don't want to show any. I'm not sure what the best option is right now.

@Frug
Owner

STRICT messages aren't errors though, they're recommendations that can safely be ignored. Showing them when it's not necessary will actually cause problems that wouldn't otherwise be problems by, for example, breaking the XML output of a page.

@clintharrison

Fair enough. Feel free not to merge that, though I've seen the other bug hit accidentally, killing the browsers of an entire room :P

@AfzalivE

I can confirm this, just tried with the example message and it killed the browsers of the whole room :+1:

@Frug
Owner

I can also confirm this spectacularly crashed firefox for me.

@Frug Frug merged commit 415e51f into Frug:master
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 25, 2013
  1. @clintharrison

    Actually show all errors

    clintharrison authored
  2. @clintharrison
This page is out of date. Refresh to see the latest.
Showing with 4 additions and 2 deletions.
  1. +1 −1  chat/index.php
  2. +3 −1 chat/js/chat.js
View
2  chat/index.php
@@ -8,7 +8,7 @@
*/
// Show all errors:
-error_reporting(E_ALL);
+error_reporting(E_ALL | E_STRICT);
// Path to the chat directory:
define('AJAX_CHAT_PATH', dirname($_SERVER['SCRIPT_FILENAME']).'/');
View
4 chat/js/chat.js
@@ -2489,7 +2489,9 @@ var ajaxChat = {
// Reset the charCounter after newline tags (<br/>):
if(i>4 && text.substr(i-5,4) == '<br/')
charCounter = 0;
- } else if(currentChar == '&') {
+ }
+
+ if(!withinTag && currentChar == '&') {
withinEntity = true;
} else if(withinEntity && i>0 && text.charAt(i-1) == ';') {
withinEntity = false;
Something went wrong with that request. Please try again.