Bug 128397

Summary: Scipt causes KHTML to freeze
Product: [Applications] konqueror Reporter: Frans Leerink <f.leerink>
Component: khtmlAssignee: Konqueror Developers <konq-bugs>
Status: RESOLVED FIXED    
Severity: crash CC: maksim
Priority: NOR    
Version: unspecified   
Target Milestone: ---   
Platform: unspecified   
OS: Linux   
Latest Commit: Version Fixed In:
Attachments: patch
updated patch

Description Frans Leerink 2006-06-01 01:24:50 UTC
Version:           3.5.1 (using KDE 3.5.1 Level "a" , SUSE 10.0 UNSUPPORTED)
Compiler:          Target: i586-suse-linux
OS:                Linux (i686) release 2.6.13-15.8-default

Browsing with Konqueror to the following URL (radio programs overview):
http://www.omroep.nl  or  http://portal.omroep.nl/

generates allways the following error:

A script on this page is causing KHTML to freeze. If it continues to run, other applicationsmay become less responsive. Do you want to abort the script?

If I browse to the same URL with Firefox 1.0.7 I do not get an error, so I think it is konqueror/khtml related.

Regards,
          Frans
Comment 1 Thiago Macieira 2006-06-05 00:46:04 UTC
KJS is much slower than SpiderMonkey. So long JavaScript scripts will make Konqueror slow. That's what that message is showing.

That is no bug.

And, besides, the webpage works fine here.
Comment 2 Maksim Orlovich 2006-06-05 06:53:32 UTC
(1)Please don't spread performance myths. KHTML/KJS and Gecko/SpiderMonkey performance is generally comparable. 
(2)This has nothing to do with speed of KJS, but rather with speed of a text-escaping function in KHTML, which does tons of memory allocations per-character.
(3)Slowness is legitimate bug. In case where (1) doesn't hold, it usually means something really stupid is happening. 
Comment 3 Maksim Orlovich 2006-06-05 07:27:20 UTC
... And in this case, the "other thing" is absolutely horrific implementation of innerHTML (in particular escapeHTML) which means there are tons of calls that take like 200ms each if I slow my machine down to 600 MHz)...
Comment 4 Maksim Orlovich 2006-06-05 07:44:41 UTC
Created attachment 16483 [details]
patch

OK, this makes innerHTML on the page about 10x-20x faster, and makes things
much smoother. Unfortunately, I am not 100% confident in safety of the
DOMString change --- it could do funny things if someone is sharing a
DOMStringImpl but not ref'ing, though I hope that sort of thing isn't
happening; but I don't have time to test/commit this anyway...
Comment 5 Maksim Orlovich 2006-06-05 07:52:01 UTC
Ugh. Except after a few more tries, it's only 2x faster. I blame dialup for lack of reproducibility. Looks like tons of mess inside ElementImpl::openTagStartToString, etc. DOMString just doesn't work for this...
Comment 6 Maksim Orlovich 2006-06-05 22:16:59 UTC
Created attachment 16495 [details]
updated patch

This one I feel a lot safer about
Comment 7 Maksim Orlovich 2006-06-30 03:14:15 UTC
SVN commit 556347 by orlovich:

Make reading of innerHTML a couple times less slow
BUG:128397


 M  +5 -3      html/html_elementimpl.cpp  
 M  +4 -3      xml/dom_elementimpl.cpp  
 M  +40 -0     xml/dom_stringimpl.cpp  
 M  +2 -0      xml/dom_stringimpl.h  
 M  +1 -19     xml/dom_textimpl.cpp  


--- branches/KDE/3.5/kdelibs/khtml/html/html_elementimpl.cpp #556346:556347
@@ -450,9 +450,11 @@
 
 DOMString HTMLElementImpl::innerHTML() const
 {
-    DOMString result = "";
-    for (NodeImpl *child = firstChild(); child != NULL; child = child->nextSibling())
-        result += child->toString();
+    QString result; //Use QString to accumulate since DOMString is poor for appends
+    for (NodeImpl *child = firstChild(); child != NULL; child = child->nextSibling()) {
+        DOMString kid = child->toString();
+        result += QConstString(kid.unicode(), kid.length()).string();
+    }
     return result;
 }
 
--- branches/KDE/3.5/kdelibs/khtml/xml/dom_elementimpl.cpp #556346:556347
@@ -824,17 +824,18 @@
 
 DOMString ElementImpl::toString() const
 {
-    DOMString result = openTagStartToString();
+    QString result = openTagStartToString().string(); //Accumulate in QString, since DOMString can't append well.
 
     if (hasChildNodes()) {
 	result += ">";
 
 	for (NodeImpl *child = firstChild(); child != NULL; child = child->nextSibling()) {
-	    result += child->toString();
+	    DOMString kid = child->toString();
+	    result += QConstString(kid.unicode(), kid.length()).string();
 	}
 
 	result += "</";
-	result += tagName();
+	result += tagName().string();
 	result += ">";
     } else if (result.length() == 1) {
 	// ensure we dont get results like < /> can happen when serialize document
--- branches/KDE/3.5/kdelibs/khtml/xml/dom_stringimpl.cpp #556346:556347
@@ -416,5 +416,45 @@
     return QConstString(s, i).string().toInt(ok);
 }
 
+static const unsigned short amp[] = {'&', 'a', 'm', 'p', ';'};
+static const unsigned short lt[] =  {'&', 'l', 't', ';'};
+static const unsigned short gt[] =  {'&', 'g', 't', ';'};
 
+DOMStringImpl *DOMStringImpl::escapeHTML()
+{
+    unsigned outL = 0;
+    for (unsigned int i = 0; i < l; ++i ) {
+        if ( s[i] == '&' )
+            outL += 5; //&amp;
+        else if (s[i] == '<' || s[i] == '>')
+            outL += 4; //&gt;/&lt;
+        else
+            ++outL;
+    }
+    if (outL == l)
+        return this;
 
+    
+    DOMStringImpl* toRet = new DOMStringImpl();
+    toRet->s = QT_ALLOC_QCHAR_VEC(outL);
+    toRet->l = outL;
+
+    unsigned outP = 0;
+    for (unsigned int i = 0; i < l; ++i ) {
+        if ( s[i] == '&' ) {
+            memcpy(&toRet->s[outP], amp, sizeof(amp));
+            outP += 5; 
+        } else if (s[i] == '<') {
+            memcpy(&toRet->s[outP], lt, sizeof(lt));
+            outP += 4;
+        } else if (s[i] == '>') {
+            memcpy(&toRet->s[outP], gt, sizeof(gt));
+            outP += 4;
+        } else {
+            toRet->s[outP] = s[i];
+            ++outP;
+        }
+    }
+    return toRet;
+}
+
--- branches/KDE/3.5/kdelibs/khtml/xml/dom_stringimpl.h #556346:556347
@@ -74,6 +74,7 @@
         return new DOMStringImpl(s,l);
     };
 
+
     DOMStringImpl *substring(unsigned int pos, unsigned int len);
     DOMStringImpl *collapseWhiteSpace(bool preserveLF, bool preserveWS);
 
@@ -89,6 +90,7 @@
     DOMStringImpl *lower() const;
     DOMStringImpl *upper() const;
     DOMStringImpl *capitalize(bool noFirstCap=false) const;
+    DOMStringImpl *escapeHTML();
 
     QChar *unicode() const { return s; }
     uint length() const { return l; }
--- branches/KDE/3.5/kdelibs/khtml/xml/dom_textimpl.cpp #556346:556347
@@ -39,25 +39,7 @@
 
 static DOMString escapeHTML( const DOMString& in )
 {
-    //FIXME: this is rather slow
-    DOMString s;
-    for ( unsigned int i = 0; i < in.length(); ++i ) {
-        switch( in[i].latin1() ) {
-        case '&':
-            s += "&amp;";
-            break;
-        case '<':
-            s += "&lt;";
-            break;
-        case '>':
-            s += "&gt;";
-            break;
-        default:
-            s += DOMString( in[i] );
-        }
-    }
-
-    return s;
+    return in.implementation()->escapeHTML();
 }
 
 CharacterDataImpl::CharacterDataImpl(DocumentPtr *doc, DOMStringImpl* _text)