taking.

Created attachment 3370 [details]
patch that fixes the bug

I tried to get away with a quick-fix, but it didn't work out. So I essentially ported my first-letter handling code from the times before the big RenderFlow split to the latest CVS, and made it work again.

The patch does (besides of some minor fixes) not change anything else. However, I had to introduce an additional member variable to RenderFlow. I really don't like that, but I cannot avoid it as the first-letter flow can be either RenderInline or RenderBlock.

Please review the patch:
http://bugs.kde.org/attachment.cgi?id=3370&action=view

I fail to see why the caret code would need this first-letter stuff, but
anyway, I see several things wrong here: 

a) khtml_part/view moving a caret when caret mode is disabled
b) khtml_part emitting a caretMoved when the caret is not even enabled. 

those will fix the crash for now. 

ad a) Even when extending the selection, the caret should be positioned to not make it look like a bug when the selection is extended, but the caret is not moving with it. Granted that there's only a need to actually position it when caret mode is activated, but then the crash would occur only under less likely occasions.

ad b) Oops, this is a blunder. It makes no sense to emit the caretPositionChanged signal within the mousemove event. I'll remove it.

Arrgh, I've just tested the quick-fix. It now crashes on mousepress. *Sigh*, it looks like there's no way around attachment 3370 [details].

I have just managed to crash it without placeCaret being involved. It's because  the first InlineTextBox contains a null string on
<p>B<br>la</p> when B is a first-letter.
Font::drawText chokes on this.

Making drawText to no-op on null strings will fix the crash. However, I'm not sure if RenderText should ever contain a null string (instead of the empty string), because this will also induce a crash when navigating the caret with the keyboard.

See attachment for quick-fix.

Bt:
[New Thread 1024 (LWP 4205)]
0x414d35c9 in __wait4 () from /lib/libc.so.6
#0  0x414d35c9 in __wait4 () from /lib/libc.so.6
#1  0x4153bb18 in __DTOR_END__ () from /lib/libc.so.6
#2  0x413a4e37 in waitpid () from /lib/libpthread.so.0
#3  0x40825bbd in KCrash::defaultCrashHandler (sig=11)
    at /leo/projekte/Fremde/kde/src/kdelibs/kdecore/kcrash.cpp:246
#4  0x413a289d in pthread_sighandler () from /lib/libpthread.so.0
#5  <signal handler called>
#6  0x40faed58 in QChar::unicode (this=0x0) at ../include/qstring.h:195
#7  0x40c26c4c in QFontMetrics::width (this=0x823dcb4, str=@0xbfffe044, len=1)
    at kernel/qfont.cpp:2246
#8  0x4198455d in khtml::Font::width (this=0x823dca8, chs=0x0, pos=0, len=1)
    at /leo/projekte/Fremde/kde/src/kdelibs/khtml/rendering/font.cpp:316
#9  0x41983a9c in khtml::Font::drawText (this=0x823dca8, p=0x82a48c8, x=20, 
    y=27, str=0x0, slen=0, pos=0, len=1, toAdd=0, d=LTR, from=0, to=1, 
    bg=0xbfffe1f4, uy=15, h=16, deco=0)
    at /leo/projekte/Fremde/kde/src/kdelibs/khtml/rendering/font.cpp:128
#10 0x4195d98a in khtml::InlineTextBox::paintSelection (this=0x8297550, 
    f=0x823dca8, text=0x8297250, p=0x82a48c8, style=0x82311a8, tx=10, ty=10, 
    startPos=0, endPos=1, deco=0)
    at /leo/projekte/Fremde/kde/src/kdelibs/khtml/rendering/render_text.cpp:208
#11 0x4195fa7d in khtml::RenderText::paintObject (this=0x8297250, p=0x82a48c8, 
    y=10, h=24, tx=10, ty=10, paintAction=PaintActionForeground)
    at /leo/projekte/Fremde/kde/src/kdelibs/khtml/rendering/render_text.cpp:1016
#12 0x4195ff2c in khtml::RenderText::paint (this=0x8297250, p=0x82a48c8, x=10, 
    y=10, w=778, h=24, tx=10, ty=10, paintAction=PaintActionForeground)
    at /leo/projekte/Fremde/kde/src/kdelibs/khtml/rendering/render_text.cpp:1074
#13 0x41948db3 in khtml::RenderBlock::paintObject (this=0x82971e0, 
    p=0x82a48c8, _x=10, _y=10, _w=778, _h=24, _tx=10, _ty=10, 
    paintAction=PaintActionForeground)
    at /leo/projekte/Fremde/kde/src/kdelibs/khtml/rendering/render_block.cpp:1165
#14 0x41948c5c in khtml::RenderBlock::paint (this=0x82971e0, p=0x82a48c8, 
    _x=10, _y=10, _w=778, _h=24, _tx=10, _ty=10, 
    paintAction=PaintActionForeground)
    at /leo/projekte/Fremde/kde/src/kdelibs/khtml/rendering/render_block.cpp:1128
#15 0x41948db3 in khtml::RenderBlock::paintObject (this=0x829716c, 
    p=0x82a48c8, _x=10, _y=10, _w=778, _h=24, _tx=10, _ty=10, 
    paintAction=PaintActionForeground)
    at /leo/projekte/Fremde/kde/src/kdelibs/khtml/rendering/render_block.cpp:1165
#16 0x41948c5c in khtml::RenderBlock::paint (this=0x829716c, p=0x82a48c8, 
    _x=10, _y=10, _w=778, _h=24, _tx=10, _ty=10, 
    paintAction=PaintActionForeground)
    at /leo/projekte/Fremde/kde/src/kdelibs/khtml/rendering/render_block.cpp:1128
#17 0x41948db3 in khtml::RenderBlock::paintObject (this=0x82970b4, 
    p=0x82a48c8, _x=10, _y=10, _w=778, _h=24, _tx=0, _ty=0, 
    paintAction=PaintActionForeground)
    at /leo/projekte/Fremde/kde/src/kdelibs/khtml/rendering/render_block.cpp:1165
#18 0x41948c5c in khtml::RenderBlock::paint (this=0x82970b4, p=0x82a48c8, 
    _x=10, _y=10, _w=778, _h=24, _tx=0, _ty=0, 
    paintAction=PaintActionForeground)
    at /leo/projekte/Fremde/kde/src/kdelibs/khtml/rendering/render_block.cpp:1128
#19 0x4196374d in khtml::RenderLayer::paintLayer (this=0x8297124, 
    rootLayer=0x829706c, p=0x82a48c8, paintDirtyRect=@0xbfffe7f8, 
    selectionOnly=false)
    at /leo/projekte/Fremde/kde/src/kdelibs/khtml/rendering/render_layer.cpp:722
#20 0x419637ca in khtml::RenderLayer::paintLayer (this=0x829706c, 
    rootLayer=0x829706c, p=0x82a48c8, paintDirtyRect=@0xbfffe7f8, 
    selectionOnly=false)
    at /leo/projekte/Fremde/kde/src/kdelibs/khtml/rendering/render_layer.cpp:734
#21 0x41963271 in khtml::RenderLayer::paint (this=0x829706c, p=0x82a48c8, 
    damageRect=@0xbfffe7f8, selectionOnly=false)
    at /leo/projekte/Fremde/kde/src/kdelibs/khtml/rendering/render_layer.cpp:620
#22 0x418b7350 in KHTMLView::drawContents (this=0x82d2c10, p=0xbfffe8dc, 
    ex=10, ey=10, ew=778, eh=24)
    at /leo/projekte/Fremde/kde/src/kdelibs/khtml/khtmlview.cpp:484
#23 0x40d64f3d in QScrollView::drawContentsOffset (this=0x82d2c10, 
    p=0xbfffe8dc, offsetx=-2000, offsety=-2000, clipx=10, clipy=10, clipw=778, 
    cliph=24) at widgets/qscrollview.cpp:2332
#24 0x40d63517 in QScrollView::viewportPaintEvent (this=0x82d2c10, 
    pe=0xbfffee24) at widgets/qscrollview.cpp:1694
#25 0x40d62c8e in QScrollView::eventFilter (this=0x82d2c10, obj=0x82d21e8, 
    e=0xbfffee24) at widgets/qscrollview.cpp:1490
#26 0x418bad3f in KHTMLView::eventFilter (this=0x82d2c10, o=0x82d21e8, 
    e=0xbfffee24)
    at /leo/projekte/Fremde/kde/src/kdelibs/khtml/khtmlview.cpp:1351
#27 0x40c5d46e in QObject::activate_filters (this=0x82d21e8, e=0xbfffee24)
    at kernel/qobject.cpp:902
#28 0x40c5d285 in QObject::event (this=0x82d21e8, e=0xbfffee24)
    at kernel/qobject.cpp:735
#29 0x40c8fd28 in QWidget::event (this=0x82d21e8, e=0xbfffee24)
    at kernel/qwidget.cpp:4408
#30 0x40c108d5 in QApplication::internalNotify (this=0xbffff224, 
    receiver=0x82d21e8, e=0xbfffee24) at kernel/qapplication.cpp:2582
#31 0x40c10322 in QApplication::notify (this=0xbffff224, receiver=0x82d21e8, 
    e=0xbfffee24) at kernel/qapplication.cpp:2470
#32 0x407a4072 in KApplication::notify (this=0xbffff224, receiver=0x82d21e8, 
    event=0xbfffee24)
    at /leo/projekte/Fremde/kde/src/kdelibs/kdecore/kapplication.cpp:509
#33 0x41047004 in QApplication::sendEvent (receiver=0x82d21e8, 
    event=0xbfffee24) at .moc/debug-shared-mt/../../kernel/qapplication.h:492
#34 0x40be1f75 in QWidget::repaint (this=0x82d21e8, reg=@0x826e0a0, 
    erase=false) at kernel/qwidget_x11.cpp:1521
#35 0x40c11a66 in QApplication::sendPostedEvents (receiver=0x0, event_type=0)
    at kernel/qapplication.cpp:3201
#36 0x40c117cf in QApplication::sendPostedEvents ()
    at kernel/qapplication.cpp:3115
#37 0x40bc135d in QEventLoop::processEvents (this=0x80808d0, flags=4)
    at kernel/qeventloop_x11.cpp:202
#38 0x40c223f0 in QEventLoop::enterLoop (this=0x80808d0)
    at kernel/qeventloop.cpp:198
#39 0x40c22315 in QEventLoop::exec (this=0x80808d0)
    at kernel/qeventloop.cpp:145
#40 0x40c10ad5 in QApplication::exec (this=0xbffff224)
    at kernel/qapplication.cpp:2705
#41 0x4005acae in kdemain (argc=2, argv=0xbffff384)
    at /leo/projekte/Fremde/kde/src/kdebase/konqueror/konq_main.cc:177
#42 0x8048706 in main (argc=2, argv=0xbffff384) at konqueror.la.cc:2
#43 0x4144abaf in __libc_start_main () from /lib/libc.so.6

Created attachment 3533 [details]
quick-fix that seems to work this time

This makes Font::drawText ignore null strings.

reviewed by me - I doubt not crashing on 0 pointers is incorrect. The question
I ask myself though is why a 0 pointer with len=1 is passed

So should I apply attachment 3533 [details] for KDE 3.2, and postpone attachment 3370 [details] for KDE 3.2.1? 3370 *has* to be applied one day to fix ::first-letter navigation.

Btw, I found out why len is 1. RenderBlock::addChildToFlow resets RenderText's string (and its length), but does not reset the InlineTextBox (which still thinks it contains the 'x').

My handling of ::first-letter never changes the RenderText's text itself, so the InlineTextBox's information stays valid.

I don't think that 3370 should be applied. there are a few flaws in it:

a) it propagates the minOffset hack. Thats a hack, and I think it breaks more things that it fixes. For example calcMinMax and layouting do not properly
respect it in all cases. 

b) storing the first letter in the first character of the rendertext is wrong.
it can have a totally different style attached to it.

c) the only problem is that the first-letter box is kind of special: it can only contain one character, so when the caret is placed there you cannot
add text. a boolean flag in rendertext should be added for this to handle.
the firstletterbit should not be in renderflow, since it does not belong there. 

d) I'm not sure how caret handles first-line currently, but this is a similiar
issue. when the user edits the caret code has to update the dom tree and then trigger a recalculation of the affected render subtree. this will take care of properly updating firstletter/firstline. 


the other problem, that creating the firstletter textchild somehow corrupts the inlinebox (which you investigated), should to be fixed instead. 

Subject: Re:  khtml crashes when selecting in khtmltests/rendering/22020-crash.html

Am Donnerstag, 4. Dezember 2003 02:34 schrieb Dirk Mueller:
[...]
> ------- Additional Comments From mueller@kde.org  2003-12-04 02:34 -------
> I don't think that 3370 should be applied. there are a few flaws in it:
>
> a) it propagates the minOffset hack. Thats a hack, and I think it breaks
> more things that it fixes. For example calcMinMax and layouting do not
> properly respect it in all cases.

Ok, that's a point. Clean solution pending, I just don't know how.
>
> b) storing the first letter in the first character of the rendertext is
> wrong. it can have a totally different style attached to it.

It simply leaves the first letter in the RenderText for the minOffset hack to 
work, but this indeed has some small but not negligible impact on layouting.
>
> c) the only problem is that the first-letter box is kind of special: it can
> only contain one character, so when the caret is placed there you cannot
> add text. a boolean flag in rendertext should be added for this to handle.
> the firstletterbit should not be in renderflow, since it does not belong
> there.

I introduced the firstletter flag merely to be able to detect a first-letter 
flow when determining which actual RenderText would apply to the given offset 
in a Text dom node (and therefore I need minOffset, too), of which the caret 
position is to be calculated appropriately.

The problem is that khtml assumes a 1 to 0/1 relationship between Dom node and 
render object. But for the first-letter case, we have two render objects 
related to the same Dom node. We'd need a general approach to handle those 
cases sanely. What do you think about it?
>
> d) I'm not sure how caret handles first-line currently, but this is a
> similiar issue. when the user edits the caret code has to update the dom
> tree and then trigger a recalculation of the affected render subtree. this
> will take care of properly updating firstletter/firstline.

Yes, that's how it's intended. Changes should be done on the DOM tree and the 
built-in khtml logic should update the rendertree accordingly.
>
>
> the other problem, that creating the firstletter textchild somehow corrupts
> the inlinebox (which you investigated), should to be fixed instead.

I'm not yet aware whether the bug is caused directly by a change of mine, or 
if it emerged as an unwanted side-effect of the RenderBlock/-Inline switch. I 
can't look into it soon.

I don't see where we assume a 1 to 0/1 relationship in khtml. what do you refer to?

Sorry, I currently don't have much time for looking into the actual cause of the crash either. 

IMHO the best would be to disable placeCaret when caret mode was not enabled, to minimize impact of the new code. at least as long as the real fix isn't found yet. 

CVS commit by waba: 

Don't crash. Partial fix/workaround for BR68753. (attachment 3533 [details])
Patch by Leo Savernik
CCMAIL: 68753@bugs.kde.org


  M +2 -1      font.cpp   1.24.2.1


--- kdelibs/khtml/rendering/font.cpp  #1.24:1.24.2.1
@@ -99,4 +99,5 @@ void Font::drawText( QPainter *p, int x,
         int toAdd, QPainter::TextDirection d, int from, int to, QColor bg, int uy, int h, int deco ) const
 {
+    if (!str) return;
     QConstString cstr = QConstString(str, slen);
     QString qstr = cstr.string();

Consider it fixed. If it regresses, feel free to reopen.