Fix for crash when assigning document.body

[Dirk Mueller]

On Saturday 04 October 2003 00:55, Maciej Stachowiak wrote:

> OK. Can you tell me a way to reproduce one of these regressions? 

I think I fixed almost all of them in our tree. There is one outstanding issue 
with stylesheet loading, but I can't find the testcase right now. 

> I'm 
> willing to look into it. I'm pretty sure that in my version all refs
> and derefs are paired up, so I don't think it could cause a leak, but I
> may have missed something. 

Thats not the issue. Due to maybe an unfortunate design decision in khtml 
recounted objects are created with a refcount 0, but they stay alive, because 
they're only checked for deletion upon deref(). so when you do a ref() and 
deref(), and the node was not yet added to the tree or otherwise ref'ed, you 
get a deleted object and most likely corruption or a crash. To make it worse, 
it works the other way around in kjs. 

> Your patch does not seem to include the 
> change to KHTMLParser::finished, is that a difference between the
> WebCore and khtml trees?

thats why I added the deref to the destructor. There was never a finished 
method in our KHTMLParser. I don't know why you added this code, seems 
redundant to me, but maybe you can prove me wrong so that we can finally 
merge this hunk in our tree. There are many such issues btw :(

the way it works is here is that KIO tells the part that there is no more data 
coming. khtmlpart flushes the decoder and tells the tokenizer. the tokenizer 
will then finish its buffered input, hand each token to the parser, and wait 
for eventually still pending external scripts etc. when all is done, the 
tokenizer destructs the parser and then notifies the part that it is 
complete, which will destruct the tokenizer and fire an onload event (or 
somewhen later, when all images etc are loaded). 

so with other words, our "finished" method is ~KHTMLParser. pretty damn 
straight forward. 

> Also, that's this about:
> > -            insertNode(e);

Sorry, that shouldn't have ended up in the patch. I was editing the patch 
because I'm working on other things. just ignore it. 

> I don't see why you changed the while loop to a two-clause for loop:
> > -    NodeImpl *node = fragment->firstChild();
> > -    while (node != NULL) {
> > +    for ( NodeImpl* node = fragment->firstChild(); node; ) {

Thats a habit of mine. I prefer to use for() for things that do something "for 
each element in this set" and while for loops that do stuff "while the 
condition is true". 

Such things make it easier for me to grasp at the first look what the code is 
doing, without having to read through a screenful of comments or the gory 
loop body. Its just a coding style issue, and I tend to react allergic when 
somebody breaks this. ;-)

> Or why you reindented a bunch of lines inconsistently (note the extra
> space on the second line, and then the lines that follow in the patch):

thats just a tab vs space issue. Thanks for catching it. 

> We do have a gnu-style ChangeLog for all our changes and we are
> considering sending it out, but we have to make sure it doesn't include
> any Apple confidential information first so we don't get fired. I'm
> hoping we can get to this soon.

Great, this is really great news. I might spend a round of free beer/tea (KDE 
developers drink tea, (C) David Faure) then :-)

> Maybe you guys could consider keeping a ChangeLog for the master KHTML
> tree too, that would make merging easier for us since we could see what
> changes go together. 

hmm, I remember in the not so distant past that you said that you guys monitor 
the kde-cvs mailing list. You can see which changes go together by looking at 
the emails, because each commit is one email. I once again repeat my offer to 
forward khtml cvs commit messages to this mailing list (or a separate one if 
you want to keep this one clean) automatically.

There is actually a nice skript (cvs2cl.pl) that generates a GNU style 
ChangeLog file from CVS history, which is what we use. 

However, I'm willing to help you with anything you need to get finally some 
changes from our tree merged. if you want a changelog then I can do that 
(preferable when I have internet connection again, somewhen next week). 

> We have a perl script that generates the skeleton 
> of the ChangeLog (the names of files and functions changed) so it's not
> a huge burden.

Ah. is that available somewhere?


-- 
> Looking for a KDE-related EMail-Alias ? Get one at kdemail.net for FREE! <