[prev in list] [next in list] [prev in thread] [next in thread] 

List:       koffice-devel
Subject:    Re: Review Request: Fix a crash which appears in several documents
From:       "Boudewijn Rempt" <boud () valdyas ! org>
Date:       2010-05-25 11:42:43
Message-ID: 20100525114243.4933.68270 () localhost
[Download RAW message or body]



> On 2010-05-21 12:14:46, Boudewijn Rempt wrote:
> > Hm, maybe something like:
> > 
> > diff --git a/kword/part/frames/KWAnchorStrategy.cpp \
> > b/kword/part/frames/KWAnchorStrategy.cpp index a893ec6..c4f6f03 100644
> > --- a/kword/part/frames/KWAnchorStrategy.cpp
> > +++ b/kword/part/frames/KWAnchorStrategy.cpp
> > @@ -114,7 +114,10 @@ bool KWAnchorStrategy::checkState(KoTextDocumentLayout::LayoutState \
> > *state, int } else {
> > Q_ASSERT(layout->lineCount());
> > QTextLine tl = layout->lineForTextPosition(m_anchor->positionInDocument() - \
> >                 block.position());
> > -            Q_ASSERT(tl.isValid());
> > +            if (!tl.isValid()) {
> > +                m_finished = false;
> > +                return false;
> > +            }
> > x = tl.cursorToX(m_anchor->positionInDocument() - block.position());
> > recalcFrom = 0;
> > }
> > 
> > Would be better then, since that at least conforms to the apidox which says:
> > 
> > "The layout process will query the state of the anchored data after each line is layouted \
> > by calling checkState() which will return false as long as there is not enough layout \
> > information to properly position the anchored frame." 
> > On the other hand, and I'm not yet sure whether it's related, when opening mumi0.odt (from \
> > kofficetests) I see that at certain pages the while loop KWTextDocumentLayout::Line is \
> > executed up to a quarter million times before exiting, leading to kword taking 100% cpu \
> > when showing some pages of that document.
> 
> Thomas Zander wrote:
> The removing of that assert sounds wrong to me as that would hide the real bug.
> To avoid end-users to hit a crash your isvalid+return may be ok, but I'm not sure loop a \
> million times is so much better. 
> Bottom line is that the test doc triggers a state that shows a bug, which should be fixed. \
> And the assert is just a symptom of that bug. 
> Boudewijn Rempt wrote:
> Actually, the loop and the assert don't seem related. I have isolated one page from the test \
> document that does trigger the loop, but doesn't trigger the assert. I'm still trying to \
> isolate the two bits in the test document that trigger the assert.

I have created two test documents based on the original mumi0.odt document. One shows the loop, \
the other shows the assert. These documents are attached to two bugs:

https://bugs.kde.org/show_bug.cgi?id=238782 -  Assert triggered when layouting the attached \
document https://bugs.kde.org/show_bug.cgi?id=238665 - layout loop when loading attached \
document

My conclusion is that these are two separate issues: the looping is not caused by the patch, \
since it is present in an unpatched kword as well.


- Boudewijn


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/4047/#review5794
-----------------------------------------------------------


On 2010-05-19 13:15:24, Jean-Nicolas Artaud wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/4047/
> -----------------------------------------------------------
> 
> (Updated 2010-05-19 13:15:24)
> 
> 
> Review request for KOffice.
> 
> 
> Summary
> -------
> 
> In KWAnchorStrategy::checkState when the alignment was horizontal or vertical, if the asked \
> position was not accessible by the actual cursor, but was still valid for the document, kword \
> crashed. This patch fix the crash by putting the position of the last line accessible by the \
> cursor. It is the same as in oowriter or msword, because you cannot add pictures in any \
> position in the doc, but only at an accessible position with cursor. 
> You can find enclosed the screen-shot of the document mw03_picture_from_clipart.doc after the \
> fix. 
> 
> Diffs
> -----
> 
> /trunk/koffice/kword/part/frames/KWAnchorStrategy.cpp 1128398 
> 
> Diff: http://reviewboard.kde.org/r/4047/diff
> 
> 
> Testing
> -------
> 
> mw03_picture_from_clipart.doc
> mumi0.doc
> mw03_footnote_roman.doc
> 
> 
> Screenshots
> -----------
> 
> After patch applied
> http://reviewboard.kde.org/r/4047/s/400/
> 
> 
> Thanks,
> 
> Jean-Nicolas
> 
> 

_______________________________________________
koffice-devel mailing list
koffice-devel@kde.org
https://mail.kde.org/mailman/listinfo/koffice-devel


[prev in list] [next in list] [prev in thread] [next in thread]