Fix for crash when assigning document.body

[Maciej Stachowiak]

On Oct 3, 2003, at 3:19 PM, Dirk Mueller wrote:

> On Friday 03 October 2003 22:37, Maciej Stachowiak wrote:
>
>> Second, the HTML parser needs to keep the "current" node ref'd, since
>> script execution can remove arbitrary nodes from the document while
>> parsing.
>
> Yeah, aljazera.net, KDE bugreport 57020. I've implemented almost 
> exactly your
> patch as well a few months ago (attached):
>
> dirk at matrix:~/t> ls -l alja.html
> -rw-r--r--    1 dirk     users          98 2003-04-14 06:23 alja.html
>
> and since then I have this patch applied to my tree. The reason I 
> didn't
> commit yet is I still keep finding regressions due to the additional
> refcounting.

Here's a patch for a regression my change caused that I don't think is 
addressed by your patches:

-------------- next part --------------
Index: khtml/html/html_elementimpl.cpp
===================================================================
RCS file: /local/home/cvs/Labyrinth/WebCore/khtml/html/html_elementimpl.cpp,v
retrieving revision 1.18
diff -u -p -r1.18 khtml/html/html_elementimpl.cpp
--- khtml/html/html_elementimpl.cpp	2003/08/18 21:12:40	1.18
+++ khtml/html/html_elementimpl.cpp	2003/10/06 04:03:28
@@ -416,6 +416,7 @@ DocumentFragmentImpl *HTMLElementImpl::c
         return NULL;
 
     DocumentFragmentImpl *fragment = new DocumentFragmentImpl( docPtr() );
+    fragment->ref();
     {
         HTMLTokenizer tok( docPtr(), fragment );
         tok.begin();
@@ -460,6 +461,12 @@ DocumentFragmentImpl *HTMLElementImpl::c
 	    node = node->nextSibling();
 	}
     }
+
+    // Trick to get the fragment back to the floating state, with 0
+    // refs but not destroyed.
+    fragment->setParent(this);
+    fragment->deref();
+    fragment->setParent(0);
 
     return fragment;
 }
Index: khtml/html/html_elementimpl.h
===================================================================
RCS file: /local/home/cvs/Labyrinth/WebCore/khtml/html/html_elementimpl.h,v
retrieving revision 1.8
diff -u -p -r1.8 khtml/html/html_elementimpl.h
--- khtml/html/html_elementimpl.h	2003/03/01 03:15:17	1.8
+++ khtml/html/html_elementimpl.h	2003/10/06 04:03:28
@@ -30,7 +30,7 @@ namespace DOM {
 class DOMString;
 class CSSStyleDeclarationImpl;
 class HTMLFormElementImpl;
- class DocumentFragmentImpl;
+class DocumentFragmentImpl;
 
 class HTMLElementImpl : public ElementImpl
 {
Index: khtml/html/htmlparser.cpp
===================================================================
RCS file: /local/home/cvs/Labyrinth/WebCore/khtml/html/htmlparser.cpp,v
retrieving revision 1.58
diff -u -p -r1.58 khtml/html/htmlparser.cpp
--- khtml/html/htmlparser.cpp	2003/10/03 21:49:11	1.58
+++ khtml/html/htmlparser.cpp	2003/10/06 04:03:30
@@ -132,6 +132,7 @@ KHTMLParser::KHTMLParser( KHTMLView *_pa
 }
 
 KHTMLParser::KHTMLParser( DOM::DocumentFragmentImpl *i, DocumentPtr *doc )
+    : current(0)
 {
     HTMLWidget = 0;
     document = doc;
-------------- next part --------------


To me, this illustrates the pitfalls of the TreeShared refcounting 
scheme. When you create objects floating, temporarily refing and 
unrefing doesn't necessarily restore the object to the state it was in 
originally. In fact, the object could be destroyed. However, if you ref 
the object across any temporary ref/deref pairs, now you have the 
problem of how to return it to floating state to return it to your 
caller. Fortunately I figured out how to pull this off by abusing 
setParent. But two wrongs don't make a right! :-)

Regards,
Maciej

P.S. If you'd like I can add a derefWithoutDestroying() or 
derefAndFloat() to TreeShared instead of the gross hack with 
setParent().