Fix for crash when assigning document.body

[Maciej Stachowiak]

On Oct 3, 2003, at 4:47 PM, Dirk Mueller wrote:

> 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.

OK, I won't sweat it for now, then.

>> 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.

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.

>
>> 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 :(

Here's the explanation of the finished() from our ChangeLog:

  - fixed reproducible crash in KHTMLParser::freeBlock on a page at  
www.library.arizona.edu

         * 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.

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.

>> 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.

OK, I'll 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".

Sounds OK, but I usually like for loops to have all three clauses,  
otherwise it can get confusing to read. I don't care much though, so  
I'll be glad to change it your way in the interest of converging our  
trees more.

>> 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.

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

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

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.

> 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).

There's no need to make one for stuff in the past, we can run cvs2cl.pl  
as well as you can. :-) But it might make things easier going forward  
if you started one, and that way it would also be easier to see which  
changes have been merged to what tree, just by looking at the  
ChangeLog.

>> 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?

We will make it available. I need to get official approval (sucks  
working for a big company sometimes).

Regards,
Maciej