Discussion:
#9145: LyX crash while accepting a change (reopened)
Alfredo Braunstein
2014-10-13 12:29:06 UTC
Permalink
The problem was indeed correctly identified by JM in changeset 7c3d1d7:

"The problem is the use of cursor movement methods to update cursor.
Cursor::forwardPos() steps into insets, which is not always what we
want. The problem here is that there is a math inset just after the
accepted change, and that the cursor steps into it for some reason.

This code is a nightmare anyway."

The fix however seems bogus to me:

bool findChange(DocIterator & cur, bool next)
{
if (!next)
cur.backwardPos();
for (; cur; next ? cur.forwardPos() : cur.backwardPos())
// if we search backwards, take a step forward
// to correctly set the anchor
- cur.forwardPos();
+ cur.top().forwardPos();
return true;
}
}

If you look at the top of the chunk, there is another
forwardPos/backwardPos that could step into an inset (and it is the
one iterated by the way). Besides, stepping into insets is needed when
looking for changes in other cases (accept all etc). Thus IMHO 7c3d1d7
should be reverted.

I think that we should first search the initial pos of the change in a
"flat" version of findNextChange as in the attachment. Note that at
the end I still call findNextChange, but just to select the full range
(a bit overkill).

I suspect that the code could be simplified enormously by adding an
acessor function to Paragraph that returns the range of the change a
given pos is in. But I'm not going to do it now...

Comments? (specially JM)

A/
Jean-Marc Lasgouttes
2014-10-13 15:39:22 UTC
Permalink
Post by Alfredo Braunstein
- cur.forwardPos();
+ cur.top().forwardPos();
Why bogus? Why do you want to revert it?
Post by Alfredo Braunstein
If you look at the top of the chunk, there is another
forwardPos/backwardPos that could step into an inset (and it is the
one iterated by the way). Besides, stepping into insets is needed when
looking for changes in other cases (accept all etc). Thus IMHO 7c3d1d7
should be reverted.
I think that the functions should be rewritten from scratch :)
Post by Alfredo Braunstein
I think that we should first search the initial pos of the change in a
"flat" version of findNextChange as in the attachment. Note that at
the end I still call findNextChange, but just to select the full range
(a bit overkill).
That makes sense, but I do not have the time/energy to make sure that
the patch is correct. I cannot understand why this thing needs to be so
complicated.
Post by Alfredo Braunstein
I suspect that the code could be simplified enormously by adding an
acessor function to Paragraph that returns the range of the change a
given pos is in. But I'm not going to do it now...
That would be a good idea indeed.

JMarc
Alfredo Braunstein
2014-10-13 16:15:48 UTC
Permalink
On Mon, Oct 13, 2014 at 5:39 PM, Jean-Marc Lasgouttes
Post by Jean-Marc Lasgouttes
Post by Alfredo Braunstein
- cur.forwardPos();
+ cur.top().forwardPos();
Why bogus? Why do you want to revert it?
To me it seems wrong (for the reasons explained below). But maybe I
don't understand it, could you explain? Was your intention to make the
same change also to the other instance of cur.forwardPos() in that
function? Right now it doesn't achieve what the commit message says
and doesn't stop the crash...
Post by Jean-Marc Lasgouttes
Post by Alfredo Braunstein
If you look at the top of the chunk, there is another
forwardPos/backwardPos that could step into an inset (and it is the
one iterated by the way). Besides, stepping into insets is needed when
looking for changes in other cases (accept all etc). Thus IMHO 7c3d1d7
should be reverted.
I think that the functions should be rewritten from scratch :)
You forgot the attachment? ;-)
SeriousIy though, I totally agree. But for the moment let's fix it...
If nothing else for the sake of the stable branch.
Post by Jean-Marc Lasgouttes
Post by Alfredo Braunstein
I think that we should first search the initial pos of the change in a
"flat" version of findNextChange as in the attachment. Note that at
the end I still call findNextChange, but just to select the full range
(a bit overkill).
That makes sense, but I do not have the time/energy to make sure that the
patch is correct. I cannot understand why this thing needs to be so
complicated.
IMO the exposed interface to the change tracking mechanism it's pretty
basic, and this is one reason why the code must be so complicated.
Post by Jean-Marc Lasgouttes
Post by Alfredo Braunstein
I suspect that the code could be simplified enormously by adding an
acessor function to Paragraph that returns the range of the change a
given pos is in. But I'm not going to do it now...
That would be a good idea indeed.
A/
Jean-Marc Lasgouttes
2014-10-13 16:27:32 UTC
Permalink
Post by Alfredo Braunstein
Post by Jean-Marc Lasgouttes
Post by Alfredo Braunstein
- cur.forwardPos();
+ cur.top().forwardPos();
Why bogus? Why do you want to revert it?
To me it seems wrong (for the reasons explained below). But maybe I
don't understand it, could you explain? Was your intention to make the
same change also to the other instance of cur.forwardPos() in that
function? Right now it doesn't achieve what the commit message says
and doesn't stop the crash...
If I remember correctly, I thought the change was obviously needed, but
I knew deep inside that it was not fixing the real bug. It may be that I
was wrong and that we want to go inside the inset if it can contains
changes.
Post by Alfredo Braunstein
Post by Jean-Marc Lasgouttes
I think that the functions should be rewritten from scratch :)
You forgot the attachment? ;-)
SeriousIy though, I totally agree. But for the moment let's fix it...
If nothing else for the sake of the stable branch.
Sometimes I think that it would be easier to drop all. But the fact that
I have never been brave enough to do it probably shows that it is not
that easy :)
Post by Alfredo Braunstein
Post by Jean-Marc Lasgouttes
That makes sense, but I do not have the time/energy to make sure that the
patch is correct. I cannot understand why this thing needs to be so
complicated.
IMO the exposed interface to the change tracking mechanism it's pretty
basic, and this is one reason why the code must be so complicated.
I suggest that you commit your changes.

JMarc
Alfredo Braunstein
2014-10-14 06:51:26 UTC
Permalink
On Mon, Oct 13, 2014 at 6:27 PM, Jean-Marc Lasgouttes
Post by Alfredo Braunstein
Post by Jean-Marc Lasgouttes
Post by Alfredo Braunstein
- cur.forwardPos();
+ cur.top().forwardPos();
Why bogus? Why do you want to revert it?
To me it seems wrong (for the reasons explained below). But maybe I
don't understand it, could you explain? Was your intention to make the
same change also to the other instance of cur.forwardPos() in that
function? Right now it doesn't achieve what the commit message says
and doesn't stop the crash...
If I remember correctly, I thought the change was obviously needed, but I
knew deep inside that it was not fixing the real bug. It may be that I was
wrong and that we want to go inside the inset if it can contains changes.
Post by Alfredo Braunstein
Post by Jean-Marc Lasgouttes
I think that the functions should be rewritten from scratch :)
What about the following? I've factored out the change-finding part
(that could enter into insets) from the change-selection part, that
should not, and rewritten most of it. From my limited testing it seems
fine (more welcome of course). As a plus, the logic is tons simpler
and we save 25 lines.

A/

Text.cpp | 7 ---
lyxfind.cpp | 121 +++++++++++++++++++++++-------------------------------------
lyxfind.h | 5 ++
3 files changed, 54 insertions(+), 79 deletions(-)
Alfredo Braunstein
2014-10-14 15:17:41 UTC
Permalink
Post by Alfredo Braunstein
On Mon, Oct 13, 2014 at 6:27 PM, Jean-Marc Lasgouttes
Post by Alfredo Braunstein
Post by Jean-Marc Lasgouttes
Post by Alfredo Braunstein
- cur.forwardPos();
+ cur.top().forwardPos();
Why bogus? Why do you want to revert it?
To me it seems wrong (for the reasons explained below). But maybe I
don't understand it, could you explain? Was your intention to make the
same change also to the other instance of cur.forwardPos() in that
function? Right now it doesn't achieve what the commit message says
and doesn't stop the crash...
If I remember correctly, I thought the change was obviously needed, but I
knew deep inside that it was not fixing the real bug. It may be that I was
wrong and that we want to go inside the inset if it can contains changes.
Post by Alfredo Braunstein
Post by Jean-Marc Lasgouttes
I think that the functions should be rewritten from scratch :)
What about the following? I've factored out the change-finding part
(that could enter into insets) from the change-selection part, that
should not, and rewritten most of it. From my limited testing it seems
fine (more welcome of course). As a plus, the logic is tons simpler
and we save 25 lines.
A/
Text.cpp | 7 ---
lyxfind.cpp | 121 +++++++++++++++++++++++-------------------------------------
lyxfind.h | 5 ++
3 files changed, 54 insertions(+), 79 deletions(-)
Any kind soul that would help me test this? The testing should be done
on change tracking in general (i.e. next/previous change while
merging), and specifically "accept/reject change" while stepping on a
changed part. The scope is to determine if behaviour with the patch
applied is better than what we have and if there are no obvious
problems (e.g. crashes, like #9145).

Alfredo
Richard Heck
2014-10-14 15:33:02 UTC
Permalink
Post by Alfredo Braunstein
Post by Alfredo Braunstein
On Mon, Oct 13, 2014 at 6:27 PM, Jean-Marc Lasgouttes
Post by Alfredo Braunstein
Post by Jean-Marc Lasgouttes
Post by Alfredo Braunstein
- cur.forwardPos();
+ cur.top().forwardPos();
Why bogus? Why do you want to revert it?
To me it seems wrong (for the reasons explained below). But maybe I
don't understand it, could you explain? Was your intention to make the
same change also to the other instance of cur.forwardPos() in that
function? Right now it doesn't achieve what the commit message says
and doesn't stop the crash...
If I remember correctly, I thought the change was obviously needed, but I
knew deep inside that it was not fixing the real bug. It may be that I was
wrong and that we want to go inside the inset if it can contains changes.
Post by Alfredo Braunstein
Post by Jean-Marc Lasgouttes
I think that the functions should be rewritten from scratch :)
What about the following? I've factored out the change-finding part
(that could enter into insets) from the change-selection part, that
should not, and rewritten most of it. From my limited testing it seems
fine (more welcome of course). As a plus, the logic is tons simpler
and we save 25 lines.
A/
Text.cpp | 7 ---
lyxfind.cpp | 121 +++++++++++++++++++++++-------------------------------------
lyxfind.h | 5 ++
3 files changed, 54 insertions(+), 79 deletions(-)
Any kind soul that would help me test this? The testing should be done
on change tracking in general (i.e. next/previous change while
merging), and specifically "accept/reject change" while stepping on a
changed part. The scope is to determine if behaviour with the patch
applied is better than what we have and if there are no obvious
problems (e.g. crashes, like #9145).
Unfortunately, I do not have time right now. If no one else steps
forward, though, commit to master and wait for complaints. I'm not sure
how things got done back in the day, but that is often how we proceed
nowadays, except with really big stuff (like JMarc's str-metrics
rewrite). It's a function of our resource starvation.

Richard
Alfredo Braunstein
2014-10-14 15:43:17 UTC
Permalink
ok, done.

A/
Post by Alfredo Braunstein
Post by Alfredo Braunstein
On Mon, Oct 13, 2014 at 6:27 PM, Jean-Marc Lasgouttes
Post by Alfredo Braunstein
Post by Jean-Marc Lasgouttes
Post by Alfredo Braunstein
- cur.forwardPos();
+ cur.top().forwardPos();
Why bogus? Why do you want to revert it?
To me it seems wrong (for the reasons explained below). But maybe I
don't understand it, could you explain? Was your intention to make the
same change also to the other instance of cur.forwardPos() in that
function? Right now it doesn't achieve what the commit message says
and doesn't stop the crash...
If I remember correctly, I thought the change was obviously needed, but I
knew deep inside that it was not fixing the real bug. It may be that I was
wrong and that we want to go inside the inset if it can contains changes.
Post by Alfredo Braunstein
Post by Jean-Marc Lasgouttes
I think that the functions should be rewritten from scratch :)
What about the following? I've factored out the change-finding part
(that could enter into insets) from the change-selection part, that
should not, and rewritten most of it. From my limited testing it seems
fine (more welcome of course). As a plus, the logic is tons simpler
and we save 25 lines.
A/
Text.cpp | 7 ---
lyxfind.cpp | 121
+++++++++++++++++++++++-------------------------------------
lyxfind.h | 5 ++
3 files changed, 54 insertions(+), 79 deletions(-)
Any kind soul that would help me test this? The testing should be done
on change tracking in general (i.e. next/previous change while
merging), and specifically "accept/reject change" while stepping on a
changed part. The scope is to determine if behaviour with the patch
applied is better than what we have and if there are no obvious
problems (e.g. crashes, like #9145).
Unfortunately, I do not have time right now. If no one else steps forward,
though, commit to master and wait for complaints. I'm not sure how things
got done back in the day, but that is often how we proceed nowadays, except
with really big stuff (like JMarc's str-metrics rewrite). It's a function of
our resource starvation.
Richard
Loading...