bug#25599: 26.0.50; "Marker does not point anywhere" error signaled by primitive-undo near an overlay with an auto-removal insert-in-front-hooks value

bug#25599: 26.0.50; "Marker does not point anywhere" error signaled by primitive-undo near an overlay with an auto-removal insert-in-front-hooks value

[Dmitry Gutov]

In diff-hl, we create overlays to paint markers on the fringe. The
overlays are set to be deleted when text inside (or very close nearby)
is modified, for better user experience. Yet it seems to clash with 
"undo", original bug report here: 
https://github.com/dgutov/diff-hl/issues/84

Here's how to reproduce the problem without diff-hl installed:

1. Replace the contents of the scratch buffer with this:

--->
;; aaaaaaaaaaaaa
;; bbbbbbbbbbbbb

(defun overlay-modified (ov after-p _beg _end &optional length)
   (unless after-p
     (when (overlay-buffer ov)
       (delete-overlay ov))))

(save-excursion
   (goto-char (point-min))
   (let ((ov (make-overlay (line-beginning-position 2) 
(line-end-position 2))))
     (overlay-put ov 'insert-in-front-hooks '(overlay-modified))))
<---

2. Evaluate both forms at the end, the one that defines
`overlay-modified' and the one that creates the overlay.

3. Select the whole first line (;; aaa...), including the newline, so
that the region ends at the beginning of the second line.

4. Press C-w, killing the region, then M-x undo.

5. See the error "Marker does not point anywhere".

The backtrace looks like this:

Debugger entered--Lisp error: (error "Marker does not point anywhere")
   primitive-undo(1 ((#(";; aaaaaaaaaaaaa\n" 0 1 (fontified t face 
font-lock-comment-delimiter-face) 1 3 (fontified t face 
font-lock-comment-delimiter-face) 3 16 (fontified t face 
font-lock-comment-face) 16 17 (fontified t face font-lock-comment-face)) 
. 1) (#<marker at 1 in -scratch-> . -17) (#<marker in no buffer> . -17) 
(#<marker in no buffer> . -17) 18 nil undo-tree-canary))
   undo-more(1)
   undo(nil)
   funcall-interactively(undo nil)

The problem is not new, I see it in 24.5 as well.

In GNU Emacs 26.0.50.5 (x86_64-pc-linux-gnu, GTK+ Version 3.18.9)
  of 2017-01-23 built on zappa
Repository revision: 03de82fe7ca09ab40fbcae394d4fcdfe3374496e
Windowing system distributor 'The X.Org Foundation', version 11.0.11804000
System Description:	Ubuntu 16.04.1 LTS

bug#25599: Why edit markers after insert?

[nitish chandra]

[-- Attachment #1: Type: text/plain, Size: 303 bytes --]

In primitive-undo in simple.el, aroud line 2550 (in master) we adjust the
markers after the insertion.

(dolist (adj valid-marker-adjustments)
               (set-marker (car adj)
                           (- (car adj) (cdr adj))))

Why is this needed? Won't insert take care of adjusting the markers?

[-- Attachment #2: Type: text/html, Size: 398 bytes --]

bug#25599: Why edit markers after insert?

[Andreas Schwab]

On Mär 08 2017, nitish chandra <nitishchandrachinta@gmail.com> wrote:

> In primitive-undo in simple.el, aroud line 2550 (in master) we adjust the
> markers after the insertion.
>
> (dolist (adj valid-marker-adjustments)
>                (set-marker (car adj)
>                            (- (car adj) (cdr adj))))
>
> Why is this needed? Won't insert take care of adjusting the markers?

Only marker originally pointing inside the deleted region were recorded
here, and this is to make sure they regain their previous position after
the reinsertion.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

bug#25599: Why edit markers after insert?

[nitish chandra]

[-- Attachment #1: Type: text/plain, Size: 413 bytes --]

> Only marker originally pointing inside the deleted region were recorded
> here, and this is to make sure they regain their previous position after
> the reinsertion.
>

I removed this part of the code to see what changes.

1. I put a marker in a line
2. Deleted the line
3. Undo-ed the delete

The marker is still in the proper place. So markers in the deleted string
do seem to regain their position properly.

[-- Attachment #2: Type: text/html, Size: 745 bytes --]

bug#25599: Why edit markers after insert?

[nitish chandra]

[-- Attachment #1: Type: text/plain, Size: 715 bytes --]

Sorry, the above process threw some warnings. I didn't notice them before:

Warning (emacs): Encountered (#<marker at 1 in *scratch*> . -14) entry in
undo list with no matching (TEXT . POS) entry



On 8 March 2017 at 23:38, nitish chandra <nitishchandrachinta@gmail.com>
wrote:

>
> Only marker originally pointing inside the deleted region were recorded
>> here, and this is to make sure they regain their previous position after
>> the reinsertion.
>>
>
> I removed this part of the code to see what changes.
>
> 1. I put a marker in a line
> 2. Deleted the line
> 3. Undo-ed the delete
>
> The marker is still in the proper place. So markers in the deleted string
> do seem to regain their position properly.
>

[-- Attachment #2: Type: text/html, Size: 1460 bytes --]

bug#25599: Why edit markers after insert?

[nitish chandra]

[-- Attachment #1.1: Type: text/plain, Size: 186 bytes --]

The problem is being caused by insert invalidating some of the markers. So
before adjusting the markers, we can check if the marker is valid or not.
Following is a patch that does this.

[-- Attachment #1.2: Type: text/html, Size: 222 bytes --]

[-- Attachment #2: 0001-Check-for-invalid-markers-after-insertion-in-undo.patch --]
[-- Type: text/x-patch, Size: 1385 bytes --]

From ca02c15de284ed4a62fc6409caf76c3608ca61da Mon Sep 17 00:00:00 2001
From: nitishch <nitishchandrachinta@gmail.com>
Date: Fri, 10 Mar 2017 00:35:14 +0530
Subject: [PATCH] Check for invalid markers after insertion in undo

* lisp/simple.el: When undo-ing a text delete,
adjust the markers only after validating after
insert (Bug#25599)
---
 lisp/simple.el | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/lisp/simple.el b/lisp/simple.el
index f110c6f326..122178f767 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -2569,8 +2569,12 @@ primitive-undo
                (goto-char pos))
              ;; Adjust the valid marker adjustments
              (dolist (adj valid-marker-adjustments)
-               (set-marker (car adj)
-                           (- (car adj) (cdr adj))))))
+               ;; insert might have invalidated some of the
+               ;; markers. We update only the currently valid
+               ;; markers. See bug#25599
+               (if (marker-buffer (car adj))
+                   (set-marker (car adj)
+                               (- (car adj) (cdr adj)))))))
           ;; (MARKER . OFFSET) means a marker MARKER was adjusted by OFFSET.
           (`(,(and marker (pred markerp)) . ,(and offset (pred integerp)))
            (warn "Encountered %S entry in undo list with no matching (TEXT . POS) entry"
-- 
2.11.0

bug#25599: Why edit markers after insert?

[Dmitry Gutov]

On 09.03.2017 21:39, nitish chandra wrote:
> The problem is being caused by insert invalidating some of the markers. 
> So before adjusting the markers, we can check if the marker is valid or 
> not. Following is a patch that does this.

Thanks, Nitish. The patch works for me.

Could someone else more familiar with undo give a second opinion, please?

bug#25599: Why edit markers after insert?

[npostavs]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 09.03.2017 21:39, nitish chandra wrote:
>> The problem is being caused by insert invalidating some of the
>> markers. So before adjusting the markers, we can check if the marker
>> is valid or not. Following is a patch that does this.
>
> Thanks, Nitish. The patch works for me.
>
> Could someone else more familiar with undo give a second opinion, please?

Not that I'm very familiar with the undo code, but the patch seems
obviously correct to me.

+               ;; insert might have invalidated some of the
+               ;; markers. We update only the currently valid
+               ;; markers. See bug#25599

I would augment the comment to specifically mention modification hooks
though (missing double spacing too).

bug#25599: Why edit markers after insert?

[Dmitry Gutov]

On 6/11/17 9:10 PM, npostavs@users.sourceforge.net wrote:

> Not that I'm very familiar with the undo code, but the patch seems
> obviously correct to me.
> 
> +               ;; insert might have invalidated some of the
> +               ;; markers. We update only the currently valid
> +               ;; markers. See bug#25599
> 
> I would augment the comment to specifically mention modification hooks
> though (missing double spacing too).

Good point. Added and pushed.

Thanks all! Closing.