Skip to content

Commit

Permalink
PERF: editor could be crashed if you entered huge inline blocks
Browse files Browse the repository at this point in the history
  • Loading branch information
SamSaffron committed Jul 4, 2014
1 parent 026a11e commit dc9b6b5
Show file tree
Hide file tree
Showing 2 changed files with 27 additions and 14 deletions.
11 changes: 7 additions & 4 deletions app/assets/javascripts/discourse/dialects/html.js
Expand Up @@ -25,12 +25,15 @@ var blockTags = ['address', 'article', 'aside', 'audio', 'blockquote', 'canvas',
};

Discourse.Dialect.registerBlock('html', function(block, next) {
var split;
var split, pos;

// Fix manual blockquote paragraphing even though it's not strictly correct
if (block.search(/[^\s]+<blockquote/) === 0) {
split = splitAtLast('blockquote', block, next, true);
if (split) { return this.processInline(split[0]); }
// PERF NOTE: /\S+<blockquote/ is a perf hog for search, try on huge string
if (pos = block.search(/<blockquote/) >= 0) {
if(block.substring(0, pos).search(/\s/) === -1) {
split = splitAtLast('blockquote', block, next, true);
if (split) { return this.processInline(split[0]); }
}
}

var m = /^<([^>]+)\>/.exec(block);
Expand Down
30 changes: 20 additions & 10 deletions vendor/assets/javascripts/better_markdown.js
Expand Up @@ -1155,28 +1155,38 @@
inline: {

__oneElement__: function oneElement( text, patterns_or_re, previous_nodes ) {
var m, res;
var m, res, pos, search_re, match_re;

// PERF NOTE: rewritten to avoid greedy match regex \([\s\S]*?)(...)\
// greedy match performs horribly with large inline blocks, it can be so
// slow it will crash chrome

patterns_or_re = patterns_or_re || this.dialect.inline.__patterns__;
var re = new RegExp( "([\\s\\S]*?)(" + (patterns_or_re.source || patterns_or_re) + ")" );
search_re = new RegExp(patterns_or_re.source || patterns_or_re);

pos = text.search(search_re);

m = re.exec( text );
if (!m) {
// Just boring text
// Just boring text
if (pos === -1) {
return [ text.length, text ];
}
else if ( m[1] ) {

if (pos !== 0) {
// Some un-interesting text matched. Return that first
return [ m[1].length, m[1] ];
return [pos, text.substring(0,pos)];
}

if ( m[2] in this.dialect.inline ) {
res = this.dialect.inline[ m[2] ].call(
match_re = new RegExp( "^(" + (patterns_or_re.source || patterns_or_re) + ")" );
m = match_re.exec( text );

if ( m[1] in this.dialect.inline ) {
res = this.dialect.inline[ m[1] ].call(
this,
text.substr( m.index ), m, previous_nodes || [] );

This comment has been minimized.

Copy link
@SamSaffron

SamSaffron Jul 4, 2014

Author Member

I think this is still safe, but uncertain. the way it passes match around is crazytown

This comment has been minimized.

Copy link
@eviltrout

eviltrout Jul 4, 2014

Contributor

Did you try it on markdown-js and see if all their tests pass? Also I worry that this is at least the second patch I've seen that has not been submitted upstream. If we don't submit upstream it makes it far harder to upgrade in the future.

}

// Default for now to make dev easier. just slurp special and output it.
res = res || [ m[2].length, m[2] ];
res = res || [ m[1].length, m[1] ];
return res;
},

Expand Down

2 comments on commit dc9b6b5

@coding-horror
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sure we submit this upstream as @eviltrout noted, that is the correct open source course of action... we would expect the same for Discourse.

@SamSaffron
Copy link
Member Author

@SamSaffron SamSaffron commented on dc9b6b5 Jul 4, 2014 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please sign in to comment.