[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]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic