Issue 73682

Setted the milestone.

Created attachment 42330 [details]
bugdoc 1

Created attachment 42331 [details]
bugdoc 2

Confirming with OO 2.1, 9107 on WinXP and OO 2.1 on Suse 10.2 - Writer crashes.

Huston, confirmed, we have a writer crash on WinXP - OOo2.1

change to P2

FME->AMA: Also reproducible with SO7. I guess this is yours. 

The problem could be reproduced using the attached files (1234.odt and abcd.odt)
on the following system configuration:

Platform: PC		OS: Windows XP SP2	Build: OOo 2.1

However, through some follow-up tests, I have noticed that the bug is not
consistent. I have tried to create 2 different text documents from scratch with
OOo Writer. Each of the documents contains a single line of text which is
completely different from each other. Then, I followed the replication sequence
above, but I wasn’t able to replicate the bug.

This is a crucial bug which interferes with the basic functionality of the
document comparator in OOo Writer. And this could be one of the reasons it is in
priority P2 which defines as a severe problem that would affect a significant
number of customers. And I totally agree with that.

Created attachment 42585 [details]
the patch of this bug

I have successfully been able to replicate this bug with the files provided as
well as with files I created on Windows XP Home edition SP2 installed using OOo
2.0.3.

I found that if you have two files, one with a single line (e. g. abc.odt) and
the other with multiple lines (e.g. def.odt), then using the compare document
function in def.odt (and comparing it to abc.odt) no crash occurs. But if you
call the compare document function in abc.odt and insert def.odt, a crash occurs
following the "reject" and then "accept" sequence. 
So basically, accepting multiple lines is a problem and accepting one single
line is not. 

Also, after the recovery of documents, the "Accept or Reject Change" window is
opened in each of the opened documents, which might be annoying to the user. 

I have successfully replicated the bug with the files provided with my windows
environment Windows XP(SP2) media edition with Open Office Version 2.1. 

After the crash occurs  and you are opening some different file,still the
crashed document will recover and  accept and reject changes window is opened on
the document which shouldn't be there and  is annoying for the customers. 

I have also been able to reproduce the bug on:

OpenOffice Version: 2.1
OS: Windows XP Version 2002 SP2
Machine: Pentium 4 HT 3.06 GHz 512 MB RAM

I have also noticed that the bug was not consistent with all text files. It
seems that it may be related to the size of the documents being compared. I
tried comparing completely different documents with very few characters using
the replication steps and the bug did not reproduce. However, I was able to
reproduce it when comparing different documents of about one page long.

Created attachment 42677 [details]
testing bug1

Created attachment 42678 [details]
testing bug2

Created attachment 42679 [details]
testing bug3

Created attachment 42680 [details]
testing bug4

I was able to successfully replicate this problem on my machine using Windows XP
Sp2 and open office 2.1. One thing that i found a bit surprising was that if I
created two odt files, one containing one word of a character of varied length
and the other containing b (all in one line for both). Now regardless of the
size of two it would only crash when I compare the b document from inside of a.
I am attaching the files I used for you guys to have a look 

I was able to successfully replicate this problem on my machine using Windows XP
Sp2 and open office 2.1. One thing that i found a bit surprising was that if I
created two odt files, one containing one word of "a" character of varied length
and the other containing "b" (all in one line for both). Now regardless of the
size of two it would only crash when I compare the "b" document from inside of
"a". I am attaching the files I used for you guys to have a look 

what about that patch ? Is it right to solve this crash issue?
can anyone give me some advices?

Hi liutao,
thank you for your patch. We are just discussing this issue at our mailinglist
dev@sw.openoffice.org. Please have a look at the thread "new person and one issue".
Your patch prevents the nodes array to become empty by insertion of an empty
text node under circumstances. This would solve the issue but would cause other
trouble with undo:
new document
insert a word
insert a frame
undo
undo => crash

lijian->ama:
Is somebody working on this issue ? If not, I would like to continue working 
on this. Any ideas ?

->lijian
It would be great, if you will work on this issue. I will support you.
I will create a CWS for OOo2.4 for change tracking/document comparison bugs
because I want to fix issue 16398 for the next version of OOo. Into this CWS
issue 73682 would fit perfectly!
Without any debugging I could imagine that the problem is that we need at least
an empty paragraph in some structures (e.g. the document must have at least one
paragraph, every table cell and every fly frame, too). If this last paragraph is
deleted by a "wrong" change tracking selection crashes could occur. This "wrong"
change tracking selections could be created by document comparison. But that's
only a wild guess, we will see...

lijian->ama:
I have not looked deep into this issue, but I am interested in this. So I will 
assign this issue to me. Also I expect your help :-)

assigned to lijian

Created attachment 49154 [details]
Non-crashing document

ama->lijian: I have attached a new document. Please compare this document with
the document you get if you open 1234.odt and compare it with abcd.odt.
It looks nearly the same, doesn't it?
But there is a big difference, in my document you could reject the insertion and
accept the deletion and you will not crash. Doing the same with the comparison
document 1234-abcd will end with a crash!
Why?
If you debug the function SwDoc::AppendRedline(..) and have a look at the
pNewRedl, especially into its member pPoint and pMark, you will see a small
difference. Please open my document, accept all changes and then activate a
compiler breakpoint in SwDoc::AppendRedline(..). If you now call "undo" in
Writer, the method will be called twice, for the insertion-redline and for the
deletion-redline. Have a look at the pPoint and pMark from pNewRedl.
And now do the same with the document 1234.odt, compared with abcd.odt. First
accept all changes and then press "undo". Again SwDoc::AppendRedline(..) will be
called twice.

ama: In both cases the insertion-redline starts at node position 9, content
position 0 (n9,c0) and ends at (n13,c0). The deletion-redline starts at
(n13,c0), but in my document it ends at (n16,25), in the bug document at (n17,c0).
What happens when the insertion is rejected? Then the paragraph 9,10,11 and 12
will be deleted completely.
What happens when the deletion is accepted?
The paragraphs 13,14 and 15 will be deleted completely, in the bug case the
paragraph 16 will be deleted, too. In my document the paragraph 16 will not be
deleted completely, only its content but he will stay as an empty paragraph.
In the bug case the document lost its last paragraph and this is not allowed, it
crashes. In my document an empty paragraph resist and all works fine.

There are two ways to fix this problem.
1. Do some tricky things and do not delete the last paragraph even if it is
requested by redline accept/reject.
2. Do not create such redlines during the document comparison.
I prefer the 2. because a change in doccomp.cxx is more locally and should not
compromise any other functionality.
So our next step will be to analyze the document comparison...

lijian->ama:
Sorry to reply so late.I am still debugging it with your suggestions.
Hope to discuss with you after some time on debugging.

lijian->ama:
I have compared these 2 documents according to your reply.
I have found the problem you said. I think the reason may 
be the position of the last node of delete-redline. It should
point to the last txtnode other than endnode.
I will debugging more... 

Created attachment 49450 [details]
patchfile for this issue

lijian->ama:
I have submitted a patch file. Please review it. :)
I found it was the reason that the end of delete-redline 
should point to end of the last txtnode whose nContent = 25,
not the start of the Endnode(nIdx = 17).

I still have a question:
You said pNext && pPrev of SwNodeIndex points to Prev/Next 
SwNodeIndes in the Nodes-Array list. But then what is the 
difference of the result between pNext/pPrev of SwNodeIndes
and Operator++()/Opeartor--() ?

Thank you!

ama->lijian:
The operator ++/-- has nothing to do with pPrev/pNext.
I'll give you an example:
If a document contains 20 paragraphs and you have, let's say, three
SwNodeIndexes A,B and C, which points to paragraph no. 7, paragraph no. 10 and
paragraph no. 15.
These three SwNodeIndexes builds an unsorted list, so maybe B is the first in
the list (i.e. SwNodes.pRoot points to B), C is the pNext of B and A is pNext of C.
If you now call operator()++ at SwNodeIndex C, then this list rest unchanged but
SwNodeIndex C will point to paragraph no. 16 instead of paragraph no. 15.
The operator++/-- changes the SwNode a SwNodeIndex refers to. The pNext/pPrev
points to other (unsorted) SwNodeIndexes.

ama->lijian:
Regarding the patch:
Yes, the idea is right. If we change the position of some redlines from the
start of the next paragraph to the end of the previous paragraph, these
paragraph will not destroyed completely, it will stay as an empty paragraph.
This will prevent our document from crashing.
But we cannot change this position for every redline, we have to detect the
critical cases. 
Why? Test the following with your patch, a document contains 5 paragraphs...
A
B
C
D
E
Save his document under test1.odt.
Delete the 3rd and 4th paragraphs:
A
B
E
Compare this document with test1.odt.
First it looks all fine, because the 3rd and 4th paragraph reappear and are
marked as deletion.
But now accept all changes.
Expected:
A
B
E
But you get two empty paragraph instead:
A
B


E
This shouldn't be.
To be honest, I do not have the right idea at the moment how to detect when we
should change the position and when we shouldn't. Normally we shouldn't, but in
every cell, fly frame, header/footer and in the document itself we need at least
one paragraph which cannot be deleted by accept/reject of the redlines.

lijian->ama:
Can you tell me how to create the non-crashing document i73682x.odt ?

ama->lijian:
I have created a new Writer document,
inserted the paragraph aaaa,bbbb,ccc and ddddd
next step was to enable change tracking: Edit/Changes../Record
Then I inserted the paragraphs containing 1111,2222,33333 etc. at the top of the
document
and at last I deleted the paragraphs aaa,bbb,ccc and ddd.
That's all.

So I created this document with (online) change tracking, not with document
comparison!

lijian->ama:
I have an idea now:
I think the critical condition should be if the next node is endnode.
If so, we can move the position to the end of the previous paragraph,
if not, we can move the poistion to the start of the next paragraph.

What do you think about this idea ?

Yes, that's a condition we should think about. Maybe it's not enough but it is
an idea to start.

If we compare two documents, we have to assure two things:
1. No crash if all deletions were accepted and all insertions are rejected.
2. If any change has been accepted, the document should looks like before the
comparison.

So please check your idea with critical structures like
sections and tables, e.g. in my five paragraph example put every paragraph into
a section.

Created attachment 49644 [details]
patchfile

lijian->ama:
Please review this patch. It seems that all works fine. :-)
Looking forward to your reply.

This patch may solve the crashes which is fine but it has side effects for
simple situations.
Create a Writer document containing one paragraph  with content "ABC" and
another with one paragraph containing "DEF".
If you compare the first document with the second document, you will run into
following bugs:

1. "Undo" after the comparison => Suddenly the document contains a second
(empty) paragraph, this shouldn't be.
2. "Accept All" changes => Like 1. there is an additional empty paragraph.

BTW: "Reject All" works fine, only one paragraph remains.

Created attachment 49681 [details]
patch file

lijian->ama:
Here is a new patch. Please have a look.
I think the key point is if the deletion-redline is the last paragraph 
remaining in the document. It's difficult to know this when creating the 
comparision document. We can just detect this when executing the deletion 
operatios.
Maybe it's not appropriate to change content of (const)pDelEnd. But I think 
it's right place here for this issue.
What do you think about this ?

ama->lijian: I've got mixed feelings.
My first thought was: No, we do not want to fix the symptom we wanna fix the
root cause.
But then I had a deeper thought about it and now I like your idea.
Even we would fix the root cause there may be other situations (e.g. a odf file
created by another application) were we could crash. So the idea to fix it in
docredln.cxx is a good idea.
OTOH if we only fix the crash, there will be remain another problem:
If you save the document after the comparison and reload it, you will see the
problem :-(
How to proceed?
I would propose to follow your idea and make it "ready for QA" ;-)
Then we either try to fix the comparison problem or write another issue for a
later target.

ama->lijian:
With your patch we will not crash with our simple document, but if you add a fly
frame into the document before you accept/reject the changes, we will crash even
with your patch.
The reason is that your code will find the text node inside the fly frame and
will therefor not recognize the critical situation.
And if I fix issue 16398 we have to assert that the content of table cells could
not be deleted completely by accept/reject changes.

ama->lijian:
With your patch we will not crash with our simple documents, but if you add a
fly frame into the document before you accept/reject the changes, we will crash
even with your patch.
The reason is that your code will find the text node inside the fly frame and
will therefor not recognize the critical situation.
And if I fix issue 16398 we have to assert that the content of table cells could
not be deleted completely by accept/reject changes.

lijian->ama:
It's so bad! I seem to have no better idea 
regarding this issue for the moment :-(  !
I can only to debug more ...

What about you ?

ama->lijian:
Your idea was (and is) good. It would improve the stability of the Writer and
solve this bug. But we could make your good idea better if we change the
condition if the selection has to be adjusted. It is okay, to look for the next
and previous SwTxtNode. But if we found such a SwTxtNode we have to check if it
is in the same nodes enviroment like the redlined text node. If e.g. our deleted
nodes are in the body text area and the previous text node is in the "fly frame
area", this cannnot be accepted and the redline has to be adjusted.

lijian->ama:
So I could fix this issue in docredln.cxx? That means I just need to improve 
my codes in this file ?
Another question : What will happen generally when reloading a document ? And 
how to deal with the reloading problem resulted by my patch ?

lijian->ama:
Sorry. I knew that the comparision problem was just an original problem, not 
resulted by my patch.

ama->lijian:
Yes, you could fix this issue in docredln.cxx. Actually your patch does it. And
if you improve your patch it will solve even more issues in slightly different
situations.
Then the Writer is more stable than today regarding document comparison.

Remaining problems are:
1. Document comparison should not result into such "risky" redlines
2. Save and load of such a "compared" document should not loose information.
For 2. I need to investigate more, I do not know at the moment, what happens. If
we could solve 1. then 2. hopefully disappears, too.

But we should first concentrate on the crash and create additional issues for
the remaining problems.

lijian->ama:
Not only the fly frame(e.g. text frame) but also header etc. can also crash I 
think. They all stay in different nodes environment like our delete nodes. 
For the moment, I don't know how to judge if they stay in the same nodes 
environment. Would you tell me something about this ?

Yes, for sure.

The member pStartOfSection of a SwNode points to its environment. If two text
nodes are in the same table cell, their pStartOfSection will be the same. So you
may compare the pStartOfSection of the deleted text node with the
pStartOfSection of the found text node (by GoPrev/GoNext).
But you shouldn't compare these pointers directly, because there is at least one
type of start node which has no negative influence  and should be ignored for
our problem, the SwSectionNode. If the pStartOfSection is a SwSectionNode,
simply take the pStartOfSection of this node. 

BTW: I added some words to our Wiki
http://wiki.services.openoffice.org/wiki/Writer_Core_And_Layout#Some_Notes_about_Nodes

Created attachment 49757 [details]
patch file

lijian->ama:
In order to find the appropriate pStartOfSection, I added 
a local function "lcl_StartExceptSection()"in docredln.cxx.
Please have a look at this patch.
I need your suggestions.

lijian->ama:
I have seen your comments on the wiki page. They are very helpful.
Thank you very much !

Yes, the locale function looks sensible. Your patch seems to work quiet well
even I have some remarks. Please have a look into my attachment...

Created attachment 49777 [details]
Some remarks on last patch

Created attachment 49782 [details]
patch file

lijian->ama:
I have modified my patch according to your remarks.
Though I don't know if I understood you correctly.
Please have a look at this new patch.

ama->lijian:
Looks fine!
I just created a CWS swredline02 for this issue and issue 16398
My fix for issue 16398 issue causes crashes which hopefully are fixed with your
patch :-)

lijian->ama:
So could this issue be finished ? If so, please let me know
if there are some remaining issues regarding this.
Thank you for your kind help.

ama->lijian:
After some checks I've found some minor issues with your patch, please check my
comments in the attached file...

Created attachment 49816 [details]
Some more remarks

lijian->ama:
Actually I don't understand you regarding your latest remarks in the patch.
Would you please tell me more about that ? Thank you !

ama->lijian:

My second remark was a small one (told by the compiler):
you have to change aTmpDelEnd->nNode-- by aTmpDelEnd.nNode--.

My third remark was about
aTmpEndDel.nContent = pCNd->Len();
this is superfluous because in the next line
aTmpEndDel.nContent.Assign( pCNd, pCNd->Len() );
will set nContent.nIndex accordingly.

For the first remark I will attach two small documents...

Created attachment 49929 [details]
Open this document 'HelloTextTable'

Created attachment 49930 [details]
and compare it with this document (HelloWorldTable)

If you now reject all changes you will get an additional empty paragraph above
the table. This is caused by the condition in the patch (my first remark).

lijian->ama:
I think what we should do at last for this issue is to deal with situations 
of "table" before or after the "deletion-redline".
I will make a new patch soon.

ama->lijian:
Don't get me wrong, it's not necessary to do special things for table. What I
meant is that the condition has to changed a little bit.
It checks the paragraphs before and after the SwPaM, so far, so good.
If they _both_ doesn't exist, the SwPaM is corrected, fine.
If only one paragraph exists, but it is in the "wrong" environment, the SwPaM is
corrected, fine.
If both exist and both are in the wrong environment, the SwPaM is corrected, fine.
But if both exists, one paragraph in wrong environment, the other is "right"
environment, then the SwPaM is corrected and this is _not_ necessary.

Created attachment 49959 [details]
patch file

Created attachment 49960 [details]
bugdoc-table1

Created attachment 49961 [details]
bugdoc-table2

lijian->ama:
I have made a new patch according to your remarks. It's OK now with your 
bugdocs.
ButI still found some issues regarding comparison of tables. Please compare 
the 2 documents attached above. After acception and rejection, writer will 
crash clicking around the table.
So I think we still need to consider the situations of "table".
What do you think about ?

lijian->ama:
In fact, txtnodes in table cells are in the right environment. Maybe we can 
fix the "table" issues by improving local fucntion I added. But I have not got 
a proper way. Hope for your help.

lijian->ama:
In fact, txtnodes in table cells are in the right environment. Maybe we can 
fix the "table" issues by improving local fucntion I added. But I have not got 
a proper way. Hope for your help.

lijian->ama:
In fact, txtnodes in table cells are in the right environment. Maybe we can 
fix the "table" issues by improving local fucntion I added. But I have not got 
a proper way. Hope for your help.

Created attachment 49962 [details]
the newest patch file

lijian->ama:
I changed the local function lcl_StartExceptSection for the "table" issues.
Please have a look at the newest patch.
Looking forward to your suggestions. :-)

ama->lijian:
Now it becomes really complicated :-(
I will need more time for analysis, but here my first thoughts:

Our plan was to protect Writer from running into trouble because of a complete
empty document. So you found a fix in docredln.cxx where a rejection/acceptance
of a redline was checked if this would result in an empty Writer element. If
this is detected the patch shrinked the SwPaM of the redline in a way that at
least an empty paragraph will stay.
I still think this is a good idea.
In your last scenario the condition found a problem and manipulated the SwPaM.
Later on you get a crash. With your last patch the condition will not detect a
problem, not manipulate the SwPaM and not crash afterwards.
But what I do not understand is why do we crash? Even if the condition was not
perfect this should result into a superfluous empty paragraph but never into a
crash?!
So I would like to understand the crash before we adjust the condition.
That's my first thought.
My second thought is the condition. Because I want to allow change tracking
inside tables (issue 16398) this check should avoid empty table cells as well.
Your last patch would not detect a problem inside a table cell.

New target OOo3.0.
I set the target for swredline02 to OOo3.0 because I don't think that we should
integrate this CWS in the current version into OOo2.4.

Train model:
if it will be ready for OOo3.0 we will integrate it, if not we will wait for the
next "train" (OOo3.1).

.

*** Issue 103733 has been marked as a duplicate of this issue. ***

I'm adding this comment to all open issues with Issue Type == PATCH.  We have 220 such issues, many of them quite old.  I apologize for that.  

We need your help in prioritizing which patches should be integrated into our next release, Apache OpenOffice 4.0.

If you have submitted a patch and think it is applicable for AOO 4.0, please respond with a comment to let us know.

On the other hand, if the patch is no longer relevant, please let us know that as well.

If you have any general questions or want to discuss this further, please send a note to our dev mailing list:  dev@openoffice.apache.org

Thanks!

-Rob

Reset assignee on issues not touched by assignee in more than 1000 days.