[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]