Paul Eggert <eggert@cs.ucla.edu> writes:

> On 03/30/2018 05:56 AM, Noam Postavsky wrote:
>> Shouldn't we at least do set-marker?
>>
>>      Fset_marker (XCAR (data), Qnil, Qnil)
>>      Fset_marker (XCDR (data), Qnil, Qnil)
>
> Possibly. And perhaps we should making similar changes to the other
> two calls to free_marker (in insdel.c's signal_before_change),

Oh, good catch.  Here's a test-case for that one:

  (with-temp-buffer
    (insert "1234567890")
    (setq buffer-undo-list nil)
    (let ((before-change-functions
           (list (lambda (beg end)
                   (delete-region (1- beg) (1+ end))))))
      (delete-region 2 5))
    (princ (format "%S" buffer-undo-list) #'external-debugging-output)
    (type-of (car (nth 3 buffer-undo-list))))

This prints

    (("678" . 1) ("12345" . 1) (#<marker in no buffer> . -1) (#<misc free cell> . -1) (#<misc free cell> . -4))

and then aborts.

> and removing the definition of free_marker while we're at it.

I think that is sensible, see following patch.