Bug 73112 - Crash in KHTML on www.tweakers.net
Summary: Crash in KHTML on www.tweakers.net
Status: RESOLVED FIXED
Alias: None
Product: konqueror
Classification: Applications
Component: khtml (show other bugs)
Version: unspecified
Platform: Compiled Sources Linux
: NOR crash
Target Milestone: ---
Assignee: Konqueror Developers
URL:
Keywords:
: 70919 73052 (view as bug list)
Depends on:
Blocks:
 
Reported: 2004-01-21 09:30 UTC by Jonathan Brugge
Modified: 2004-02-15 21:54 UTC (History)
3 users (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments
how IE and mozilla handle the invalid HTML (152.96 KB, image/jpeg)
2004-01-22 11:35 UTC, Stephan Kulow
Details
patch that fixes the problem (517 bytes, patch)
2004-01-22 15:24 UTC, Waldo Bastian
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Brugge 2004-01-21 09:30:17 UTC
Version:            (using KDE Devel)
Installed from:    Compiled sources
Compiler:          GCC 3.3.3 GCC from Debian Sid
OS:          Linux

This report is a copy of http://lists.kde.org/?l=kfm-devel&m=107463167703496&w=2 - I repost this on Bugzilla by request.

THE PROBLEM: http://www.tweakers.net crashes KHTML while loading.  Safari has 
the same problem, according to user reports on 
http://gathering.tweakers.net/forum/list_messages/863577. The crash only 
happens when loading of banners has been enabled - while I had them disabled, 
I experienced no crashes at all. Reportedly, the banner is loaded by some 
javascript-wizardry, but the crash doesn't seem to be there - though it may 
be that the problems already start in KJS, of course.

DEBUG INFO: I created a backtrace and a valgrind log. First the backtrace (I 
skipped the last part, since it doesn't seem to be relevant - if you need it, 
just ask for it):
--------------------
[New Thread 1102615200 (LWP 17538)]
0x4188f30e in __waitpid_nocancel () from /lib/tls/libpthread.so.0
#0  0x4188f30e in __waitpid_nocancel () from /lib/tls/libpthread.so.0
#1  0x408e44b4 in KCrash::defaultCrashHandler(int) (sig=11) at kcrash.cpp:246
#2  <signal handler called>
#3  0x40e008b2 in khtml::KHTMLParser::popOneBlock() (this=0x83298f8)
    at htmlparser.cpp:1195
#4  0x40e00aea in khtml::KHTMLParser::freeBlock() (this=0x83298f8)
    at htmlparser.cpp:1236
#5  0x40dfe0f9 in ~KHTMLParser (this=0x83298f8) at htmlparser.cpp:158
#6  0x40e06b58 in ~HTMLTokenizer (this=0x83297c0) at htmltokenizer.cpp:1595
#7  0x40dde672 in DOM::DocumentImpl::close() (this=0x8322838)
    at khtmlview.h:110
#8  0x40e0fdc8 in DOM::HTMLDocumentImpl::close() (this=0x8322838)
    at html_documentimpl.cpp:292
#9  0x40d9f516 in KHTMLPart::checkEmitLoadEvent() (this=0x8202a48)
    at khtml_part.cpp:2025
#10 0x40d9ec5a in KHTMLPart::checkCompleted() (this=0x8202a48)
    at khtml_part.cpp:1947
#11 0x40d9e698 in KHTMLPart::slotLoaderRequestDone(khtml::DocLoader*, 
khtml::CachedObject*) (this=0x8202a48, dl=0x5f006e00, obj=0x5f006e00)
    at khtml_part.cpp:1834
#12 0x40db94cd in KHTMLPart::qt_invoke(int, QUObject*) (this=0x8202a48, 
    _id=57, _o=0xbfffe740) at qucom_p.h:312
#13 0x41306b47 in QObject::activate_signal(QConnectionList*, QUObject*) (
    this=0x81dea50, clist=0x82f91c0, o=0xbfffe740) at kernel/qobject.cpp:2383
#14 0x40eaa765 in khtml::Loader::requestDone(khtml::DocLoader*, 
khtml::CachedObject*) (this=0x81dea50, t0=0x5f006e00, t1=0x5f006e00) at 
loader.moc:240
--------------------

The important part of the valgrind log, where it states a problem in 
popOneBlock() - the same function that can be seen as #3 in the backtrace 
above (again, the complete valgrind log can be posted upon request; I didn't 
do that yet since it's 26KB large):
---------------------
khtml (xml):  using compatibility parseMode
NodeImpl::toHTML
NodeImpl::toHTML
khtml (css): CSSStyleDeclarationImpl::setProperty invalid property: [width] 
value: [-1px]
khtml (css): CSSStyleDeclarationImpl::setProperty invalid property: [height] 
value: [-1px]
NodeImpl::toHTML
NodeImpl::toHTML
==19744==
==19744== Invalid read of size 4
==19744==    at 0x4997790D: khtml::KHTMLParser::popOneBlock() (shared.h:34)
==19744==    by 0x49977AE9: khtml::KHTMLParser::freeBlock() 
(htmlparser.cpp:1236)
==19744==    by 0x499750F8: khtml::KHTMLParser::~KHTMLParser() 
(htmlparser.cpp:158)
==19744==    by 0x4997DB57: khtml::HTMLTokenizer::~HTMLTokenizer() 
(htmltokenizer.cpp:1595)
==19744==  Address 0x4D0858DC is not stack'd, malloc'd or free'd
==19744==
==19744== Invalid read of size 4
==19744==    at 0x499778AD: khtml::KHTMLParser::popOneBlock() 
(htmlparser.cpp:1195)
==19744==    by 0x49977AE9: khtml::KHTMLParser::freeBlock() 
(htmlparser.cpp:1236)
==19744==    by 0x499750F8: khtml::KHTMLParser::~KHTMLParser() 
(htmlparser.cpp:158)
==19744==    by 0x4997DB57: khtml::HTMLTokenizer::~HTMLTokenizer() 
(htmltokenizer.cpp:1595)
==19744==  Address 0x4D0858D8 is not stack'd, malloc'd or free'd
==19744==
==19744== Invalid read of size 4
==19744==    at 0x499778B2: khtml::KHTMLParser::popOneBlock() 
(htmlparser.cpp:1195)
==19744==    by 0x49977AE9: khtml::KHTMLParser::freeBlock() 
(htmlparser.cpp:1236)
==19744==    by 0x499750F8: khtml::KHTMLParser::~KHTMLParser() 
(htmlparser.cpp:158)
==19744==    by 0x4997DB57: khtml::HTMLTokenizer::~HTMLTokenizer() 
(htmltokenizer.cpp:1595)
==19744==  Address 0xD0 is not stack'd, malloc'd or free'd
==19744== Warning: invalid file descriptor 821 in syscall close()
==19744==    Use --logfile-fd=<number> to select an alternative logfile fd.
---------------------

VERSION: GCC 3.3.3 (or maybe 3.3.2 a week ago, don't know for sure). 
kdelibs/kdebase CVS HEAD about a week old. Can't test with newer versions due 
to problems when compiling kdelibs (that's a separate issue which surely will 
be solved and might be a problem on my side). There's one report of it 
working correctly with Konqueror 3.1.3, but I don't know for sure whether the 
reporter was loading banners or not.

If there's anything I can do to help fixing this crash, just ask for it.

Jonathan Brugge
Comment 1 Kai Lahmann 2004-01-21 13:37:04 UTC
bug 73052 same page, other backtrace... tons of others are resolved (?)
Comment 2 Dik Takken 2004-01-21 14:13:25 UTC
I can confirm this, Konqueror did not crash very often on tweakers.net when I used Beta 2, but in the two days I have RC1 I crashed konqueror six times already.
Comment 3 Waldo Bastian 2004-01-21 14:28:02 UTC
*** Bug 73052 has been marked as a duplicate of this bug. ***
Comment 4 Waldo Bastian 2004-01-21 14:56:56 UTC
I could reproduce yesterday, but not today. I don't think my khtml has changed in the mean time though.
Comment 5 Jeroen Wijnhout 2004-01-21 15:03:00 UTC
Subject: Re:  Crash in KHTML on www.tweakers.net

> I could reproduce yesterday, but not today. I don't think my khtml has
> changed in the mean time though.

I have posted the KDebug info of a short Konqueror session here (duplicate 
bug, sorry about that btw):
http://bugs.kde.org/show_bug.cgi?id=73052      
Maybe this is useful?

best,
Jeroen

Comment 6 Waldo Bastian 2004-01-21 15:46:05 UTC
Can now reproduce with MSIE useragent for adserver.webads.nl
kio_httprc:
[webads.nl]
UserAgent=Mozilla/4.0 (compatible; MSIE 5.5; Windows NT 5.0)

Comment 7 Jonathan Brugge 2004-01-21 15:48:12 UTC
No reproduction possible currently, because WebAds and Tweakers.net decided not to show the offending banner to Konqueror/Safari...to test it, it should be possible to change your useragent string to Mozilla/IE. Bugzilla just tells me that Waldo said the same a minute ago, but now you know the reason.

By the way: Waldo, welcome at GoT, the forum of Tweakers.net :).
(http://gathering.tweakers.net/forum/list_message/19791809#19791809)
Comment 8 Waldo Bastian 2004-01-21 17:30:00 UTC
Testcase:

<div id='a'></div>
<div id='b'>
<center><a href=\"http://www.kde.org/\">
</div>

<script language='javascript'>
var html = document.getElementById('b').innerHTML;
document.getElementById('a').innerHTML = html;
document.getElementById('b').innerHTML="";
</script>
Comment 9 Waldo Bastian 2004-01-21 17:35:37 UTC
Valgrind:
==13185== Invalid read of size 4
==13185==    at 0x40360D96: khtml::TreeShared<DOM::NodeImpl>::ref() (shared.h:34)
==13185==    by 0x403C9A88: khtml::KHTMLParser::freeBlock() (htmlparser.cpp:1238)
==13185==    by 0x403C7357: khtml::KHTMLParser::~KHTMLParser() (htmlparser.cpp:158)
==13185==    by 0x403CFDE8: khtml::HTMLTokenizer::~HTMLTokenizer() (htmltokenizer.cpp:1596)
==13185==    by 0x403A8223: DOM::DocumentImpl::close() (khtmlview.h:110)
==13185==    by 0x403D831B: DOM::HTMLDocumentImpl::close() (html_documentimpl.cpp:293)
==13185==    by 0x4036EA82: KHTMLPart::checkEmitLoadEvent() (khtml_part.cpp:2025)
==13185==    by 0x4036DB69: KHTMLPart::slotFinishedParsing() (khtml_part.cpp:1792)

==13185==    Address 0x4692A788 is 4 bytes inside a block of size 60 free'd
==13185==    at 0x4002A0B3: __builtin_delete (vg_replace_malloc.c:244)
==13185==    by 0x4002A0D1: operator delete(void*) (vg_replace_malloc.c:253)
==13185==    by 0x403D5A1F: DOM::HTMLGenericElementImpl::~HTMLGenericElementImpl() (html_elementimpl.cpp:631)
==13185==    by 0x403B30D0: DOM::NodeBaseImpl::removeChildren() (dom_nodeimpl.cpp:1286)
==13185==    by 0x403D56AE: DOM::HTMLElementImpl::setInnerHTML(DOM::DOMString const&) (html_elementimpl.cpp:547)
==13185==    by 0x404FB0AE: DOM::HTMLElement::setInnerHTML(DOM::DOMString const&) (html_element.cpp:144)
==13185==    by 0x4049EAC9: KJS::HTMLElement::putValueProperty(KJS::ExecState*, int, KJS::Value const&, int) (kjs_html.cpp:3030)
==13185==    by 0x404A9CED: void KJS::DOMObjectLookupPut<KJS::HTMLElement, KJS::DOMElement>(KJS::ExecState*, KJS::Identifier const&, KJS::Value const
==13185==    by 0x4049DEDA: KJS::HTMLElement::tryPut(KJS::ExecState*, KJS::Identifier const&, KJS::Value const&, int) (kjs_html.cpp:2364)
==13185==    by 0x40475BD0: KJS::DOMObject::put(KJS::ExecState*, KJS::Identifier const&, KJS::Value const&, int) (kjs_binding.cpp:70)
==13185==    by 0x4063C6C6: KJS::Reference::putValue(KJS::ExecState*, KJS::Value const&) (reference.cpp:165)
==13185==    by 0x4060929F: KJS::AssignNode::evaluate(KJS::ExecState*) const (nodes.cpp:1574)
==13185==    by 0x4060AC8A: KJS::ExprStatementNode::execute(KJS::ExecState*) (nodes.cpp:1916)
==13185==    by 0x40610DBA: KJS::SourceElementsNode::execute(KJS::ExecState*) (nodes.cpp:3035)
==13185==    by 0x4060AAB2: KJS::BlockNode::execute(KJS::ExecState*) (nodes.cpp:1878)
==13185==    by 0x406102BB: KJS::FunctionBodyNode::execute(KJS::ExecState*) (nodes.cpp:2881)
==13185==    by 0x40602484: KJS::InterpreterImp::evaluate(KJS::UString const&, KJS::Value const&) (internal.cpp:873)
==13185==    by 0x40637238: KJS::Interpreter::evaluate(KJS::UString const&, KJS::Value const&) (interpreter.cpp:166)
==13185==    by 0x404C32AC: KJSProxyImpl::evaluate(QString, int, QString const&, DOM::Node const&, KJS::Completion*) (kjs_proxy.cpp:148)
==13185==    by 0x4036940A: KHTMLPart::executeScript(QString const&, int, DOM::Node const&, QString const&) (khtml_part.cpp:965)
==13185==    by 0x403CB9E8: khtml::HTMLTokenizer::scriptExecution(QString const&, QString const&, int) (khtmlview.h:110)
==13185==    by 0x403CB5D6: khtml::HTMLTokenizer::scriptHandler() (htmltokenizer.cpp:406)
==13185==    by 0x403CB3A3: khtml::HTMLTokenizer::parseSpecial(khtml::DOMStringIt&) (htmltokenizer.cpp:324)
==13185==    by 0x403CE2A2: khtml::HTMLTokenizer::parseTag(khtml::DOMStringIt&) (htmltokenizer.cpp:1138)
==13185==    by 0x403CEE36: khtml::HTMLTokenizer::write(QString const&, bool) (htmltokenizer.cpp:1272)
==13185==    by 0x4036D6DA: KHTMLPart::write(char const*, int) (khtml_part.cpp:1719)
==13185==    by 0x4036AC86: KHTMLPart::slotData(KIO::Job*, QMemArray<char> const&) (khtml_part.cpp:1407)
Comment 10 Stephan Kulow 2004-01-22 10:47:02 UTC
JFYI: KDE 3.1 crashes on the same test case, so I guess it's no recent regression but a change in tweakers.net
Comment 11 Stephan Kulow 2004-01-22 11:10:13 UTC
the test case is really mean. konqueror leaves the <a> open when the </div> closes, so it adds the <script> to the <center><a>, which then alters the surrounding <div>, so when the parser finishes, it finds the currently parsed
elements changed.

This is of course no problem if the <a> tag is closed correctly (or the <center> tag - which closes the <a> tag too). The fix here would propbably to close <a> on </div>, but that might break other pages.
Comment 12 Stephan Kulow 2004-01-22 11:35:32 UTC
Created attachment 4289 [details]
how IE and mozilla handle the invalid HTML

Interesting: only konqueror keeps the <div> open, while mozilla closes the
<center> but leaves the <a> open, IE leaves both open, but still closes the
<div>
Comment 13 Stephan Kulow 2004-01-22 13:18:30 UTC
I tried with safari and it a) displays the invalid HTML as konqueror does and
b) it crashes too [safari insisted I report a bug to apple - which I did with a reference to this bug ;]
Comment 14 Waldo Bastian 2004-01-22 15:23:50 UTC
Re: #11

I think the parser should refcount the elements it has open so that they can't get deleted from underneath it. See patch.
Comment 15 Waldo Bastian 2004-01-22 15:24:43 UTC
Created attachment 4291 [details]
patch that fixes the problem

patch that fixes the problem
Comment 16 Jeroen Wijnhout 2004-01-22 16:18:05 UTC
Subject: Re:  Crash in KHTML on www.tweakers.net

JFYI, this www.tweakers.net is apparently fixed (didn't crash today, without 
the patch that is).

I'm using a patched khtml now, I will let you know if I find anything 
peculiar. So far, so good.

best,
Jeroen

Comment 17 Stephan Kulow 2004-01-22 19:18:56 UTC
Subject: khtmltests/regression

CVS commit by coolo: 

do never crash again
CCMAIL: 73112-done@bugs.kde.org


  A            baseline/unsorted/73112.html-dom   1.1
  A            tests/unsorted/73112.html   1.1
  M +1 -0      baseline/unsorted/.cvsignore   1.2


--- khtmltests/regression/baseline/unsorted/.cvsignore  #1.1:1.2
@@ -2,2 +2,3 @@
 70488.html-dom
 70608.html-dom
+73112.html-render


Comment 18 Jonathan Brugge 2004-01-22 22:59:36 UTC
Thanks anyone for the invested time - it's really great to see (again) how fast and precise the developers react to bug reports like this one!
Comment 19 Dirk Mueller 2004-01-26 07:55:51 UTC
*** Bug 70919 has been marked as a duplicate of this bug. ***
Comment 20 Stephan Kulow 2004-02-15 21:54:49 UTC
*** Bug 75274 has been marked as a duplicate of this bug. ***