PDA

View Full Version : [FIXED-798] Huge slowdown in Konqueror; have patch



ousia
29 Mar 2010, 8:02 AM
This bug report outlines the problem, diagnoses it, and presents a solution. I know of only one browser this affects, but the problems are so dangerous I think they deserve wide attention.

Ext versions tested:

Ext 3.1.0
Ext 3.1.1


Adapter used:

ext


CSS used:

only default ext-all.css


Browser versions tested against:

Konqueror 3.5.10 (where the problem occurred and which this fix resolves)
Konqueror 4.3.3 (all OK except for problem I need to investigate separately)
Firefox 2 (with and without Firebug; works fine with and without patch)
Firefox 3 (with Firebug 1.5.3; works fine with and without patch)
Opera 9.64 (no problems with or without patch)
Chromium 1 (open-source version of Google Chrome; no problems with or without patch)
Seamonkey 1 (no problems with or without patch)
Arora 0 (no problems with or without patch)
MSIE 6 on XP (confirmed patched code still works)
Safari on XP (confirmed patched code still works)
Firefox on XP (confirmed patched code still works)
Android Browser on Android 1.5 (confirmed patched code still works)


Operating System:

Linux (Gentoo; various installations of KDE on x86 and amd64)
Windows XP (for confirmation of fix)
Google Android 1.5 (for confirmation of fix)


Description:

Konqueror 3.5 works very well with Ext 2 versions but became painfully slow with Ext 3 (I observed the slowdown beginning with Ext 3.0.0).
I inserted function calls to measure the how long it took for the script to reach a given point in the code. The library loaded without delay but bogged down when my code instantiated a TabPanel object.
Continued tracing with my probes revealed the problem occurred in DomHelper.applyStyles(). When a derivative of the Element class calls the unselectable() method, Konqueror 3.5 freezes for about half a minute. Element.unselectable() calls applyStyles with the argument "-moz-user-select:none;-khtml-user-select:none;", which then passes the string to DomHelper.applyStyles().
An optimization applied to applyStyles in the transition from Ext 2 to Ext 3 introduced the problem. DomHelper.applyStyles() decomposes the argument string and in the current situation should iterate through DomHelper.setStyle() only twice: once with arguments ('-moz-user-select', 'none') and then with ('-khtml-user-select', 'none'). There is an erroneous third iteration, with setStyles('', undefined). This latter call causes the hangup in Konqueror.


Steps to reproduce the problem:

This is easy to reproduce: using the browser under test, exercise any of the standard Ext samples which create components with an unselectable heading, such as any of the grid, tab panel, or window examples.


Konqueror 3.5 freezes for several seconds when Javascript tries to set an empty CSS property:

var el = document.getElementById('whatever');
el.style[''] = 'any value you like';.

Your browser of choice may recover from this, but I hope you can agree that it is bad business to pull this kind of stunt. Just wait for some future browser to do something even worse when you try to set an empty property--or to one with a bad value. This is not likely to win you friends either:

var el = document.getElementById('whatever');
el.style.color = null;

The current Ext 3.x implementation of DomHelper.applyStyles() applies two optimizations which bother me. Here's the offending code:

styles = styles.trim().split(/\s*(?::|;)\s*/);
for(len = styles.length; i < len;){
el.setStyle(styles[i++], styles[i++]);
}

The regex decomposes the argument string into ['-moz-user-select', 'none', '-khtml-user-select', 'none', '']. Note the empty string that is the last element. This comes from the not-unreasonable assumption that callers of applyStyles() may use an argument with a trailing semicolon. CSS rules are often written with a trailing semicolon after the last declaration, and many places within the Ext library make calls to Element.applyStyles() that way. This means that applyStyles(), all over the installations of Ext 3.x in the world, is making thousands of erroneous calls every day to setStyle() with an empty property name and an undefined value.

The second problem is one of cowboy programming--or at least of adherence to a detail in the Javascript standard that I missed. In C programming, standards from Kernighan and Richie onward have always warned that the order of evaluation of arguments to a function is undefined. Sensible programmers in every language derived from C, therefore, avoid writing function arguments that have side effects--especially multiple arguments whose side effects interact. In my reading of ECMA standard 262, I find no explicit statement about the order of evaluation of the items in a list. This would make me very reticent to code the call to setStyle() as the library has it now.

Now let me propose a solution that not only solves the Konqueror problem, but is more robust and also takes care to avoid a regression of a fix applied earlier (see http://www.extjs.com/forum/showthread.php?t=4053).
This replaces lines I quoted from DomHelper.applyStyles() in versions 3.0 and 3.1.x of the Ext library:

var re=/([a-z0-9-]+)\s*:\s*([^;\s]+(?:\s*[^;\s]+)*);?/gi, matches;
while (matches = re.exec(styles)) {
el.setStyle(matches[1], matches[2]);
}

This regex requires that each CSS declaration in the argument string be of the form "property: value", where property must be alphanumerics with optional hyphens and value is a sequence of non-semicolons which may contain whitespace. The regex trims any leading or trailing whitespace of both the property and value.


Just to make things more clear, this is the revised version of the DomHelper.applyStyles() method. In the Ext 3.1.1 files, these lines are at 278-97 of DomHelper.php and and the same lines of ext-all-debug.js.

applyStyles : function(el, styles){
if(styles){
el = Ext.fly(el);
if(Ext.isFunction(styles)){
styles = styles.call();
}
if(Ext.isString(styles)){
var re=/([a-z0-9-]+)\s*:\s*([^;\s]+(?:\s*[^;\s]+)*);?/gi, matches;
while (matches = re.exec(styles)) {
el.setStyle(matches[1], matches[2]);
}
}else if (Ext.isObject(styles)){
el.setStyle(styles);
}
}
},



Not being content to propose this change without testing it, I wrote a small test suite that not only tests my parser over various test inputs, but also shows what the existing Ext DomHelper.applyStyles() would do in parsing these same input strings. This code is completely self-contained and should not even need a web server. Since the existing applyStyles() method relies on the trim() method that Ext supplies for String objects, I have copied the code which sets up that prototype.

Load this page in any browsers you have.

<html>
<head>
<title>Test of regex to decompose CSS declarations</title>
<style type="text/css">
body {margin: 0 10%}
table {border-collapse: collapse}
th, td {border: 1px solid black; padding: 2px 5px; text-align: left}
th {color: #555}
tr.heading th {text-align: center; color: black}
</style>
<script>
window.onload = function () {
//Copy of prototype for String.trim() from Ext-more.js
String.prototype.trim = (function() {
re = /^\s+|\s+$/g;
return function() { return this.replace(re, ''); }
})();

var targ = document.getElementById('targ');
var hdr = ['Input declarations', 'Old way', 'New way'];
var rows = ['<tr class="heading"><th>' + hdr.join('</th><th>') +
'</th></tr>'];
var src = ['color:red', 'color: red', ' color:#123', 'padding: 0 1px 10px',
'padding: 0 1px 10px;color:green', 'padding: 0 1px 10px ; color: green',
'padding: 0 1px 10px; color: green;', 'font-size:120%; color: blue;',
'font-size:120%; color: blue ; ',
//We throw in an intentionally garbled case for good measure
'garbage; prop: value; border:0;'];
for (var sx=0; sx < src.length; ++sx) {
var styles = src[sx];
rows.push('<tr><th>"' + styles + '"</th><td>' +
oldWay(styles).join('<br/>') + '</td><td>' +
newWay(styles).join('<br/>') + '</td></tr>');
}

targ.innerHTML = '<table>' + rows.join('') + '</table>';

//From lines 289-92 in DomHelper.js of Ext 3.1.0
function oldWay(styles) {
var grp = [];
var i = 0, len, style;
styles = styles.trim().split(/\s*(?::|;)\s*/);
for (len = styles.length; i < len;) {
grp.push('"' + styles[i++] + '" = "' + styles[i++] + '"');
}
return grp;
}

function newWay(styles) {
var grp = [];
var re=/([a-z0-9-]+)\s*:\s*([^;\s]+(?:\s*[^;\s]+)*);?/gi, matches;
while (matches = re.exec(styles)) {
grp.push('"' + matches[1] + '" = "' + matches[2] + '"');
}
return grp;
}
}
</script>
</head>
<body>
<h1>Test of decomposition of CSS declarations</h1>
<div id="targ"></div>
</body>
</html>

evant
20 Apr 2010, 11:22 PM
Thanks for the detailed test case/fix. It's been added to SVN.