Fix for crash when assigning document.body

[Dirk Mueller]

On Saturday 04 October 2003 02:10, Maciej Stachowiak wrote:

> Ah, I see. Yes, I've run into this before, and it's sort of on my
> mental TODO list to rationalize the KHTML refcounting a bit.

Argh, I was afraid that you would say that. Look, there is nothing irrational 
about our refcounting sheme, and which sheme you consider irrational pretty 
much depends on which books you read in your life before.

Both ways have advantages and disadvantages. A disadvantage might be that 
newly created objects look "half-alive". This would be a problem if we'd use 
garbage collection, because then the garbage collection would sweep the 
object away before we had a chance to install it in its final position in the 
tree (or dict or whatever). However, as we don't use garbage collection, and 
don't plan to either, this is not an issue for us. On the other side, the way 
it is done now has two advantages: a) the tree always stays alive, and you 
have to worry less about refcount cycles and b) all ref() and deref() 
perfectly pair up, which makes debugging recounting problems significantly 
easier. if you create objects with a refcount of one, you have to add a 
deref() somewhere that doesn't pair intuitively with the (hidden) ref. 

On the other side, kjs different refcounting handling is okay as well, because 
kjs makes significant use of mark&sweep garbage collection, which has 
different constraints on the objects, including the ugly problem that objects 
which are not yet fully constructed must stay alive. 

So please accept that we did put some thought into the code and our design 
decisions, and at least sometimes things are the way they are for good 
reasons. :-)

>          * khtml/html/htmlparser.cpp: (KHTMLParser::finished): Call
> freeBlock here rather than
>          waiting until the parser is destroyed. This fixes the bug
> because when the parser is
>          destroyed, the document is already destroyed, so we have a
> dangling current pointer to an
>          already-destroyed node.

Well, at least the way it is now this cannot be an issue, because "current" 
can by definition never be a dangling pointer, since we keep a ref() of it. 

> The specific page in question is
> http://www.library.arizona.edu/library/teams/fah/subpathpages/
> Russian.Slavic/RIL/technology/fonts/fonts.htm
>
> Maybe our fix was wrong, and if so, I'd be glad to change it in our
> tree.

This page does not reproduce the described problem with Konqueror. It works 
fine. Anyway, there must have been something terrible wrong if the document 
was destructed before the parser, and I would say this is a workaround. 
Please revert it and tell me if you still see this problem. 

> Sounds OK, but I usually like for loops to have all three clauses,
> otherwise it can get confusing to read. 

Yeah, I prefer that too but it is not as important to me than the basic for/
while loop distinction. This particular example is an edge case. The reason I 
picked for() was that it allows limiting the scope of the iterating pointer, 
to avoid that it is accidentally used in a scope where it has no useful 
meaning anymore. Limiting the scope of variables is extremely important, 
especially if you use autocompleting editors :). We often had bugs in the 
past because xemacs auto-completed the wrong variable name, or even 
consistently replicated typos in method names because nobody typed more than 
the first two letters and then <tab> ;-)

> I think the separate list is better, although I'm not yet subbed to
> kde-cvs (will be soon).

I'll talk to the sysadmins then if we can create a khtml-cvs mailinglist. 

> Yep, I've used that in the past, though it usually hasn't been quite as
> nice as a real ChangeLog due to the function names.

echo "diff -u5 -d -p" >> ~/.cvsrc

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